[PATCH] input: Add new keyboard backlight control keys to match modern notebooks

[PATCH] input: Add new keyboard backlight control keys to match modern notebooks

[Werner Sembach]

The old three KEY_KBDILLUM* keycodes don't reflect the current situation
modern notebooks anymore. Especially the ones with RGB keyboards.

e.g.
- Clevo NL50NU has a toggle, an up, a down and a color-cycle key
- TongFang PH4ARX1 doesn't have a toggle key, but one that cycles through
  off, half-brightness, and full-brightness.

Also, on some devices these keys are already implemented in firmware. It
would still be nice if there is a way to let userspace know when one of
these keys is pressed to display the OSD, but don't advice it to actually
do anything. This is the intended purpose of the KEY_KBDILLUMCHANGE define.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 include/uapi/linux/input-event-codes.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 022a520e31fc2..05287bf9a77f7 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -803,6 +803,10 @@
 #define BTN_TRIGGER_HAPPY39		0x2e6
 #define BTN_TRIGGER_HAPPY40		0x2e7
 
+#define KEY_KBDILLUMCYCLE		0x2e8
+#define KEY_KBDILLUMCOLORCYCLE		0x2e9
+#define KEY_KBDILLUMCHANGE		0x2ea
+
 /* We avoid low common keys in module aliases so they don't get huge. */
 #define KEY_MIN_INTERESTING	KEY_MUTE
 #define KEY_MAX			0x2ff
-- 
2.34.1

Re: [PATCH] input: Add new keyboard backlight control keys to match modern notebooks

[Bastien Nocera]

On Tue, 2023-05-30 at 13:05 +0200, Werner Sembach wrote:
> The old three KEY_KBDILLUM* keycodes don't reflect the current
> situation
> modern notebooks anymore. Especially the ones with RGB keyboards.
> 
> e.g.
> - Clevo NL50NU has a toggle, an up, a down and a color-cycle key
> - TongFang PH4ARX1 doesn't have a toggle key, but one that cycles
> through
>   off, half-brightness, and full-brightness.
> 
> Also, on some devices these keys are already implemented in firmware.
> It
> would still be nice if there is a way to let userspace know when one
> of
> these keys is pressed to display the OSD, but don't advice it to
> actually
> do anything. This is the intended purpose of the KEY_KBDILLUMCHANGE
> define.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>

Can you please point to the user-space patches (or issues filed) that
would integrate the support for those keycodes, and make the key
presses do something?

Has anyone tested that those keycodes are fit for purpose when mixed
with other brightness changes that don't happen through key presses?

> ---
>  include/uapi/linux/input-event-codes.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/uapi/linux/input-event-codes.h
> b/include/uapi/linux/input-event-codes.h
> index 022a520e31fc2..05287bf9a77f7 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -803,6 +803,10 @@
>  #define BTN_TRIGGER_HAPPY39            0x2e6
>  #define BTN_TRIGGER_HAPPY40            0x2e7
>  
> +#define KEY_KBDILLUMCYCLE              0x2e8
> +#define KEY_KBDILLUMCOLORCYCLE         0x2e9
> +#define KEY_KBDILLUMCHANGE             0x2ea
> +
>  /* We avoid low common keys in module aliases so they don't get
> huge. */
>  #define KEY_MIN_INTERESTING    KEY_MUTE
>  #define KEY_MAX                        0x2ff

Re: [PATCH] input: Add new keyboard backlight control keys to match modern notebooks

[Werner Sembach]

Hi,

Am 30.05.23 um 15:33 schrieb Bastien Nocera:
> On Tue, 2023-05-30 at 13:05 +0200, Werner Sembach wrote:
>> The old three KEY_KBDILLUM* keycodes don't reflect the current
>> situation
>> modern notebooks anymore. Especially the ones with RGB keyboards.
>>
>> e.g.
>> - Clevo NL50NU has a toggle, an up, a down and a color-cycle key
>> - TongFang PH4ARX1 doesn't have a toggle key, but one that cycles
>> through
>>    off, half-brightness, and full-brightness.
>>
>> Also, on some devices these keys are already implemented in firmware.
>> It
>> would still be nice if there is a way to let userspace know when one
>> of
>> these keys is pressed to display the OSD, but don't advice it to
>> actually
>> do anything. This is the intended purpose of the KEY_KBDILLUMCHANGE
>> define.
>>
>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> Can you please point to the user-space patches (or issues filed) that
> would integrate the support for those keycodes, and make the key
> presses do something?

I'm sorry to say that these don't exist yet. So I guess the process is similar 
to DRM uAPI additions? 
https://docs.kernel.org/gpu/drm-uapi.html#open-source-userspace-requirements

>
> Has anyone tested that those keycodes are fit for purpose when mixed
> with other brightness changes that don't happen through key presses?

Color control is not yet implemented in any DE afaik, so there is not yet a 
collision with the color cycle key.

For the brightness cycle key, I would assume that it functions the same as the 
brightness up key unless brightness == brightness max. In this case it sets 
brightness to 0. I don't see a logical collision here as brightness up and 
brightness down are already implemented just fine in most DEs

>
>> ---
>>   include/uapi/linux/input-event-codes.h | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/include/uapi/linux/input-event-codes.h
>> b/include/uapi/linux/input-event-codes.h
>> index 022a520e31fc2..05287bf9a77f7 100644
>> --- a/include/uapi/linux/input-event-codes.h
>> +++ b/include/uapi/linux/input-event-codes.h
>> @@ -803,6 +803,10 @@
>>   #define BTN_TRIGGER_HAPPY39            0x2e6
>>   #define BTN_TRIGGER_HAPPY40            0x2e7
>>   
>> +#define KEY_KBDILLUMCYCLE              0x2e8
>> +#define KEY_KBDILLUMCOLORCYCLE         0x2e9
>> +#define KEY_KBDILLUMCHANGE             0x2ea
>> +
>>   /* We avoid low common keys in module aliases so they don't get
>> huge. */
>>   #define KEY_MIN_INTERESTING    KEY_MUTE
>>   #define KEY_MAX                        0x2ff

Re: [PATCH] input: Add new keyboard backlight control keys to match modern notebooks

[Werner Sembach]

Am 30.05.23 um 16:28 schrieb Werner Sembach:
> Hi,
>
> Am 30.05.23 um 15:33 schrieb Bastien Nocera:
>> On Tue, 2023-05-30 at 13:05 +0200, Werner Sembach wrote:
>>> The old three KEY_KBDILLUM* keycodes don't reflect the current
>>> situation
>>> modern notebooks anymore. Especially the ones with RGB keyboards.
>>>
>>> e.g.
>>> - Clevo NL50NU has a toggle, an up, a down and a color-cycle key
>>> - TongFang PH4ARX1 doesn't have a toggle key, but one that cycles
>>> through
>>>    off, half-brightness, and full-brightness.
>>>
>>> Also, on some devices these keys are already implemented in firmware.
>>> It
>>> would still be nice if there is a way to let userspace know when one
>>> of
>>> these keys is pressed to display the OSD, but don't advice it to
>>> actually
>>> do anything. This is the intended purpose of the KEY_KBDILLUMCHANGE
>>> define.
>>>
>>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
>> Can you please point to the user-space patches (or issues filed) that
>> would integrate the support for those keycodes, and make the key
>> presses do something?
>
> I'm sorry to say that these don't exist yet. So I guess the process is similar 
> to DRM uAPI additions? 
> https://docs.kernel.org/gpu/drm-uapi.html#open-source-userspace-requirements
I asked the KDE and Gnome maintainers for feedback: 
https://bugs.kde.org/show_bug.cgi?id=470453 
https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/746
>
>>
>> Has anyone tested that those keycodes are fit for purpose when mixed
>> with other brightness changes that don't happen through key presses?
>
> Color control is not yet implemented in any DE afaik, so there is not yet a 
> collision with the color cycle key.
>
> For the brightness cycle key, I would assume that it functions the same as the 
> brightness up key unless brightness == brightness max. In this case it sets 
> brightness to 0. I don't see a logical collision here as brightness up and 
> brightness down are already implemented just fine in most DEs
>
>>
>>> ---
>>>   include/uapi/linux/input-event-codes.h | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/include/uapi/linux/input-event-codes.h
>>> b/include/uapi/linux/input-event-codes.h
>>> index 022a520e31fc2..05287bf9a77f7 100644
>>> --- a/include/uapi/linux/input-event-codes.h
>>> +++ b/include/uapi/linux/input-event-codes.h
>>> @@ -803,6 +803,10 @@
>>>   #define BTN_TRIGGER_HAPPY39            0x2e6
>>>   #define BTN_TRIGGER_HAPPY40            0x2e7
>>>   +#define KEY_KBDILLUMCYCLE              0x2e8
>>> +#define KEY_KBDILLUMCOLORCYCLE         0x2e9
>>> +#define KEY_KBDILLUMCHANGE             0x2ea
>>> +
>>>   /* We avoid low common keys in module aliases so they don't get
>>> huge. */
>>>   #define KEY_MIN_INTERESTING    KEY_MUTE
>>>   #define KEY_MAX                        0x2ff

Re: [PATCH] input: Add new keyboard backlight control keys to match modern notebooks

[Werner Sembach]

Am 30.05.23 um 13:05 schrieb Werner Sembach:
> The old three KEY_KBDILLUM* keycodes don't reflect the current situation
> modern notebooks anymore. Especially the ones with RGB keyboards.
>
> e.g.
> - Clevo NL50NU has a toggle, an up, a down and a color-cycle key
> - TongFang PH4ARX1 doesn't have a toggle key, but one that cycles through
>    off, half-brightness, and full-brightness.
>
> Also, on some devices these keys are already implemented in firmware. It
> would still be nice if there is a way to let userspace know when one of
> these keys is pressed to display the OSD, but don't advice it to actually
> do anything. This is the intended purpose of the KEY_KBDILLUMCHANGE define.
Nevermind the KEY_KBDILLUMCHANGE. I just found out there is already a way to 
communicate this from kernel to userspace via sysfs 
https://docs.kernel.org/leds/leds-class.html#led-registration-api -> 
brightness_hw_changed
>
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>   include/uapi/linux/input-event-codes.h | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index 022a520e31fc2..05287bf9a77f7 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -803,6 +803,10 @@
>   #define BTN_TRIGGER_HAPPY39		0x2e6
>   #define BTN_TRIGGER_HAPPY40		0x2e7
>   
> +#define KEY_KBDILLUMCYCLE		0x2e8
> +#define KEY_KBDILLUMCOLORCYCLE		0x2e9
> +#define KEY_KBDILLUMCHANGE		0x2ea
> +
>   /* We avoid low common keys in module aliases so they don't get huge. */
>   #define KEY_MIN_INTERESTING	KEY_MUTE
>   #define KEY_MAX			0x2ff

Re: [PATCH] input: Add new keyboard backlight control keys to match modern notebooks

[Hans de Goede]

Hi Werner,

Thank you for your patch.

On 5/30/23 13:05, Werner Sembach wrote:
> The old three KEY_KBDILLUM* keycodes don't reflect the current situation
> modern notebooks anymore. Especially the ones with RGB keyboards.
> 
> e.g.
> - Clevo NL50NU has a toggle, an up, a down and a color-cycle key
> - TongFang PH4ARX1 doesn't have a toggle key, but one that cycles through
>   off, half-brightness, and full-brightness.
> 
> Also, on some devices these keys are already implemented in firmware. It
> would still be nice if there is a way to let userspace know when one of
> these keys is pressed to display the OSD, but don't advice it to actually
> do anything. This is the intended purpose of the KEY_KBDILLUMCHANGE define.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>  include/uapi/linux/input-event-codes.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index 022a520e31fc2..05287bf9a77f7 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -803,6 +803,10 @@
>  #define BTN_TRIGGER_HAPPY39		0x2e6
>  #define BTN_TRIGGER_HAPPY40		0x2e7
>  
> +#define KEY_KBDILLUMCYCLE		0x2e8

I do not really see what the difference is between this and the existing KEY_KBDILLUMTOGGLE, userspace can already choice whether it toggles to a number of states or just toggles on/off.

So IMHO this one should be dropped.

> +#define KEY_KBDILLUMCOLORCYCLE		0x2e9

This one is fine.

> +#define KEY_KBDILLUMCHANGE		0x2ea

Keyboard backlight support should be exported to userspace as a LED class device, see e.g. :

drivers/platform/x86/thinkpad_acpi.c    : tpacpi_led_kbdlight
drivers/platform/x86/dell/dell-laptop.c : kbd_led

And the LED class device sysfs API already has a mechanism for signalling kbd-brightness changes triggered by the hw itself (e.g. by the embedded controller) to userspace. See the use of the 
LED_BRIGHT_HW_CHANGED flag and the calling of led_classdev_notify_brightness_hw_changed() in the 2 above drivers.

So strong NACK for adding KEY_KBDILLUMCHANGE, this is duplicate with the led_classdev_notify_brightness_hw_changed() functionality which is already supported by userspace. E.g. GNOME will show an OSD notification similar to the sound volume change OSD when changing the keyboard brightness through EC handled hotkeys on ThinkPads and various Dell models.

TL;DR: to me only KEY_KBDILLUMCOLORCYCLE makes sense, assuming that this needs to be handled by userspace, if this is handled in the EC then this too should simply call led_classdev_notify_brightness_hw_changed()

Regards,

Hans





> +
>  /* We avoid low common keys in module aliases so they don't get huge. */
>  #define KEY_MIN_INTERESTING	KEY_MUTE
>  #define KEY_MAX			0x2ff