Re: Adding a new platform driver samsung-galaxybook

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

Hi Joshua,

On 18-Nov-24 2:51 PM, Joshua Grisham wrote:
> Hello! I have created a platform driver for Samsung Galaxy Book series
> notebooks which has now gone through several iterations and
> contributions from several other community members.

Thank you for your work on this and thank you for submitting your
driver upstream. It is always good to have support for more laptop
models in the mainline kernel.

And my apologies for being slow with responding to this.

<snip>

> 3. usage of the i8042 filter and ACPI hotkey notifications to handle a
> few of the hotkey actions within the driver itself instead of just
> emitting input events and allow userspace to handle the actions
> (namely cycling through keyboard backlight levels, performance modes,
> etc)
> 
> This last item (executing hotkey actions in kernel space) is not
> totally unprecedented either, as I have seen there seems to exist
> similar i8042 filters driving hotkey actions in msi-laptop,
> toshiba_acpi, and dell-laptop and ACPI notifications from hotkeys
> driving actions in several x86 platform drivers as well (dell-laptop,
> acer-wmi, asus-laptop, ideapad-laptop, etc; this is an even more
> common pattern than using an i8042 filter, it seems).
> 
> The problem with just emitting the "right" input events and relying on
> the userspace to handle this stuff in the right way is that 1) there
> are not really keycodes that exist for exactly the keys we want here
> (even though "Keyboard Backlight Cycle" and some kind of "Performance
> Mode" hotkeys are very common on laptops today) and 2) functionality
> for how to handle these kind of events do not really support these
> use-cases either.

I agree that for performance-mode-cycling handling the hotkey in
the kernel is the right thing to do, this is already done by
several other drivers and we even have a helper for this:
platform_profile_cycle() please make sure you use this helper and
otherwise this is fine.

For kbd-backlight-brightness cycling most laptops actually implement
this in the embedded-controller and we only get an event that
the brightness was changed (with if we are lucky also the new
brightness level inside the event).

As you say we have KEY_KBDILLUMTOGGLE/XF86KbdLightOnOff and
KEY_KBDILLUM[UP|DOWN]/XF86KbdBrightness[Up|Down] but those don't
match what we want here and if we add a new keycode for that
userspace support will also need to be added.

So I think it is best to just emulate what the laptops where
the cycling is directly done by the embedded-control do.

That is:

1. Add LED_BRIGHT_HW_CHANGED to the flags of the led_classdev
for the "xxx:kbd_backlight" led class device you expose

2. Filter out kbd-backlight-cycle keypresses and on such
a keypress:

2.1 Determine new brightness level
2.2 Apply new brightness level
2.3 Call:

led_classdev_notify_brightness_hw_changed(&kbd_backlight_led_classdev, new_brightness_level);

This last call emulates what the kernel reports to userspace
on the events we get from embedded controllers after they have
changed the brightness level based on the cycle-key getting
pressed.

With this in place you should get a nice OSD notification like
for volume up/down in at least GNOME when changing the kbd
backlight level.

Note if you rmmod + modprobe your laptop driver you need
to restart upower afterwards for this to work. IIRC gnome-shell
does not need a logout + login, just restarting upower is enough.
But I might be wrong about not needing to restart gnome-shell...

I hope this helps and I'm looking forward to you submitting
your driver upstream.

Regards,

Hans