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
