Re: [PATCH 1/2] platform/x86: uniwill-laptop: Fix brightness notify for 3 level keyboards

Am 25.07.26 um 16:44 schrieb Armin Wolf:
> Am 25.07.26 um 01:00 schrieb Werner Sembach:
>
>> The devices with 3 brightness level keyboards seem to send
>> UNIWILL_OSD_KB_LED_LEVEL0, UNIWILL_OSD_KB_LED_LEVEL2, and
>> UNIWILL_OSD_KB_LED_LEVEL4 and not UNIWILL_OSD_KB_LED_LEVEL0,
>> UNIWILL_OSD_KB_LED_LEVEL1, and UNIWILL_OSD_KB_LED_LEVEL2 so this patch does
>> not return the brightness based on one of these events, but the brightness
>> read from hardware.
>>
>> Signed-off-by: Werner Sembach <[email protected]>
>> ---
>>   drivers/platform/x86/uniwill/uniwill-acpi.c | 22 +++------------------
>>   1 file changed, 3 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c 
>> b/drivers/platform/x86/uniwill/uniwill-acpi.c
>> index 7a2eeaec4c961..d9a33be4f4a37 100644
>> --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
>> +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
>> @@ -1626,7 +1626,7 @@ static int uniwill_led_init(struct uniwill_data *data)
>>                                &init_data);
>>   }
>>   -static int uniwill_notify_kbd_led(struct uniwill_data *data, int brightness)
>> +static int uniwill_notify_kbd_led(struct uniwill_data *data)
>>   {
>>       struct led_classdev *led_cdev;
>>       int ret;
>> @@ -1643,7 +1643,7 @@ static int uniwill_notify_kbd_led(struct uniwill_data 
>> *data, int brightness)
>>       if (ret < 0)
>>           return ret;
>>   -    led_classdev_notify_brightness_hw_changed(led_cdev, brightness);
>> +    led_classdev_notify_brightness_hw_changed(led_cdev, led_cdev->brightness);
>
> WMI events are usually delivered with a large latency, a userspace application 
> could
> have changed the LED brightness value in the meantime.
>
> Please check the maximum keyboard backlight brightness inside 
> uniwill_notify_kbd_led()
> and translate the brightness values instead:
>
> /* Comment here */
> switch (data->kbd_led_max_brightness) {
> case 4:
>     break;
> case 2:
>     switch (brightness) {
>     case 0:
>         break;
>     case 2:
>         brightness = 1;
>         break;
>     case 4:
>         brightness = 2;
>         break;
>     default:
>         return -EINVAL;
>     }
>     break;
> default:
>     return -EINVAL;
> }
>
> Thanks,
> Armin Wolf
ack, but I did it with some ifs to make the footprint smaller
>
>>         return 0;
>>   }
>> @@ -2215,30 +2215,14 @@ static int uniwill_notifier_call(struct 
>> notifier_block *nb, unsigned long action
>>             return NOTIFY_OK;
>>       case UNIWILL_OSD_KB_LED_LEVEL0:
>> -        if (!uniwill_device_supports(data, UNIWILL_FEATURE_KEYBOARD_BACKLIGHT))
>> -            return NOTIFY_DONE;
>> -
>> -        return notifier_from_errno(uniwill_notify_kbd_led(data, 0));
>>       case UNIWILL_OSD_KB_LED_LEVEL1:
>> -        if (!uniwill_device_supports(data, UNIWILL_FEATURE_KEYBOARD_BACKLIGHT))
>> -            return NOTIFY_DONE;
>> -
>> -        return notifier_from_errno(uniwill_notify_kbd_led(data, 1));
>>       case UNIWILL_OSD_KB_LED_LEVEL2:
>> -        if (!uniwill_device_supports(data, UNIWILL_FEATURE_KEYBOARD_BACKLIGHT))
>> -            return NOTIFY_DONE;
>> -
>> -        return notifier_from_errno(uniwill_notify_kbd_led(data, 2));
>>       case UNIWILL_OSD_KB_LED_LEVEL3:
>> -        if (!uniwill_device_supports(data, UNIWILL_FEATURE_KEYBOARD_BACKLIGHT))
>> -            return NOTIFY_DONE;
>> -
>> -        return notifier_from_errno(uniwill_notify_kbd_led(data, 3));
>>       case UNIWILL_OSD_KB_LED_LEVEL4:
>>           if (!uniwill_device_supports(data, 
>> UNIWILL_FEATURE_KEYBOARD_BACKLIGHT))
>>               return NOTIFY_DONE;
>>   -        return notifier_from_errno(uniwill_notify_kbd_led(data, 4));
>> +        return notifier_from_errno(uniwill_notify_kbd_led(data));
>>       default:
>>           mutex_lock(&data->input_lock);
>>           sparse_keymap_report_event(data->input_device, action, 1, true);
>>
>> base-commit: caf8342512c3056005f475d350eeca089c3c6623