Re: [PATCH v3] platform/x86: samsung-galaxybook: Add samsung-galaxybook driver

[Hans de Goede <hdegoede@redhat.com>]

Hi Armin,

Thank you for reviewing this new driver.

On 17-Dec-24 10:31 PM, Armin Wolf wrote:
> Am 16.12.24 um 11:38 schrieb Joshua Grisham:

<snip>

>> +/*
>> + * Hotkey work and filters
>> + */
>> +
>> +static void galaxybook_kbd_backlight_hotkey_work(struct work_struct *work)
>> +{
>> +    struct samsung_galaxybook *galaxybook =
>> +        container_of(work, struct samsung_galaxybook, kbd_backlight_hotkey_work);
>> +
>> +    if (galaxybook->kbd_backlight.brightness < galaxybook->kbd_backlight.max_brightness)
>> +        kbd_backlight_acpi_set(galaxybook, galaxybook->kbd_backlight.brightness + 1);
>> +    else
>> +        kbd_backlight_acpi_set(galaxybook, 0);
>> +
>> +    led_classdev_notify_brightness_hw_changed(
>> +        &galaxybook->kbd_backlight,
>> +        galaxybook->kbd_backlight.brightness);
> 
> This is the exact reason why i think this should be done in userspace. You can replace this code
> with a simple input event submission using the KBDILLUM* key codes. This would also allow you to
> avoid any special locking in this case.
> 
> This would also allow userspace to configure the step with of the brightness changes.

As discussed in an earlier thread, there is really no good way to let
userspace handle this atm. KEY_KBDILLUMTOGGLE gets mapped to XF86KbdLightOnOff
while we really want Cycle, as we have in e.g. XF86MonBrightnessCycle.

I just checked gnome-settings-daemon and it does handle XF86KbdLightOnOff
but it only toggles on/off. In most laptops the cycle key is handled by
the embedded controller and this simply emulates that common setup to
match what userspace currently expects.

Handling this in userspace would require adding a new KEY_KBDILLUMCYCLE
and then adding support for that to xkb and maybe also upower and
gnome-settings-daemon and KDE and then wait for that all to land before
things start to work.

As for your argument about allowing to set the percentage to step,
note that this in kernel handling only increaeses the level by 1 on
the hotkey press that make sense because typically these kbds backlights
only have 2 - 4 levels.

So IMHO handling this in the kernel is an acceptable compromise,
(yes I do realize that this is a compromise).

If some need arises later to do move this to userspace we can always
add a module parameter to completely disable the in kernel handling and
instead send out a new KEY_KBDILLUMCYCLE key-code when this now
module parameter is set.

Regards,

Hans