platform/x86: asus-wmi: Add keyboard backlight toggle support

Some Asus laptops like UX550GE has hotkey (Fn+F7) for keyboard backlight toggle. In this UX550GE, the hotkey incremet the level of brightness for each keypress from 1 to 3, and then switch it off when the brightness has been the max. This commit interprets the code 0xc7 generated from hotkey to KEY_KBDILLUMUP to increment the brightness, then pass KEY_KBDILLUMTOGGLE to user space after the brightness max been reached for switching the led off.

Signed-off-by Chris Chiu <[email protected]>

Signed-off-by Jian-Hong Pan <[email protected]>

Tested-by Jian-Hong Pan <[email protected]>

---

Notes:
v2:
- Remove redundant 'else' branch logic

 drivers/platform/x86/asus-nb-wmi.c | 1 +
 drivers/platform/x86/asus-wmi.c    | 5 +++++
 2 files changed, 6 insertions(+)

drivers/platform/x86/asus-nb-wmi.c +1 -0

index 136ff2b4cce5..14c502e18579 100644
@@ -496,6 +496,7 @@ static const struct key_entry asus_nb_wmi_keymap[] = {
        { KE_KEY, 0xC4, { KEY_KBDILLUMUP } },
        { KE_KEY, 0xC5, { KEY_KBDILLUMDOWN } },
        { KE_IGNORE, 0xC6, },  /* Ambient Light Sensor notification */
+       { KE_KEY, 0xC7, { KEY_KBDILLUMTOGGLE } },
        { KE_END, 0},
 };
 

drivers/platform/x86/asus-wmi.c +5 -1

index 1f6e68f0b646..4a3ba575c9ce 100644
@@ -67,6 +67,7 @@ MODULE_LICENSE("GPL");
 #define NOTIFY_BRNDOWN_MAX             0x2e
 #define NOTIFY_KBD_BRTUP               0xc4
 #define NOTIFY_KBD_BRTDWN              0xc5
+#define NOTIFY_KBD_BRTTOGGLE           0xc7
 
 /* WMI Methods */
 #define ASUS_WMI_METHODID_SPEC         0x43455053 /* BIOS SPECification */

@@ -1745,6 +1746,10 @@ static void asus_wmi_notify(u32 value, void *context)
                }
        }
 
+       if (code == NOTIFY_KBD_BRTTOGGLE)
+               if (asus->kbd_led_wk < asus->kbd_led.max_brightness)
+                       code = NOTIFY_KBD_BRTUP;
+
        if (is_display_toggle(code) &&
            asus->driver->quirks->no_display_toggle)
                goto exit;
--

2.14.1

Quoted 8 lines · Chris Chiu

Some Asus laptops like UX550GE has hotkey (Fn+F7) for keyboard backlight toggle. In this UX550GE, the hotkey incremet the level of brightness for each keypress from 1 to 3, and then switch it off when the brightness has been the max. This commit interprets the code 0xc7 generated from hotkey to KEY_KBDILLUMUP to increment the brightness, then pass KEY_KBDILLUMTOGGLE to user space after the brightness max been reached for switching the led off.

Pushed to my review and testing queue, thanks!

Quoted 10 lines

Quoted 8 lines

Some Asus laptops like UX550GE has hotkey (Fn+F7) for keyboard backlight toggle. In this UX550GE, the hotkey incremet the level of brightness for each keypress from 1 to 3, and then switch it off when the brightness has been the max. This commit interprets the code 0xc7 generated from hotkey to KEY_KBDILLUMUP to increment the brightness, then pass KEY_KBDILLUMTOGGLE to user space after the brightness max been reached for switching the led off.

Pushed to my review and testing queue, thanks!

We found that GNOME's handling of the toggle key is somewhat imperfect and it will need modifying before we achieve the Up-Up-Up-off-Up-Up-Up-off... cycle that we are looking for.

https://gitlab.gnome.org/GNOME/gnome-settings-daemon/issues/41

In that discussion an alternative perspective was raised:

Is it right for the kernel to modify the key sent to userspace, when it is then relying on the specific userspace action of it changing the brightness to the next expected level? (and this userspace behaviour is not even working right in the GNOME case)

Instead, would it make sense for the kernel to always report TOGGLE in this case, and for GNOME to interpret toggle as simply "cycle through all the available brightness levels"?

Hi Andy,

Quoted 24 lines · Daniel Drake

Quoted 10 lines

Quoted 8 lines

Some Asus laptops like UX550GE has hotkey (Fn+F7) for keyboard backlight toggle. In this UX550GE, the hotkey incremet the level of brightness for each keypress from 1 to 3, and then switch it off when the brightness has been the max. This commit interprets the code 0xc7 generated from hotkey to KEY_KBDILLUMUP to increment the brightness, then pass KEY_KBDILLUMTOGGLE to user space after the brightness max been reached for switching the led off.

Pushed to my review and testing queue, thanks!

We found that GNOME's handling of the toggle key is somewhat imperfect and it will need modifying before we achieve the Up-Up-Up-off-Up-Up-Up-off... cycle that we are looking for.

https://gitlab.gnome.org/GNOME/gnome-settings-daemon/issues/41

In that discussion an alternative perspective was raised:

Is it right for the kernel to modify the key sent to userspace, when it is then relying on the specific userspace action of it changing the brightness to the next expected level? (and this userspace behaviour is not even working right in the GNOME case)

Quoted 3 lines

Instead, would it make sense for the kernel to always report TOGGLE in this case, and for GNOME to interpret toggle as simply "cycle through all the available brightness levels"?

Any comments on this? I am tempted to send a patch to just make this key always emit TOGGLE from the kernel given that we have a tentative agreement on implementing the brightness cycle within GNOME.

Hi Andy,

Quoted 32 lines · Daniel Drake

Quoted 13 lines

Quoted 12 lines

Some Asus laptops like UX550GE has hotkey (Fn+F7) for keyboard backlight toggle. In this UX550GE, the hotkey incremet the level
of brightness for each keypress from 1 to 3, and then switch it off when the brightness has been the max. This commit interprets
the code 0xc7 generated from hotkey to KEY_KBDILLUMUP to increment
the brightness, then pass KEY_KBDILLUMTOGGLE to user space after
the brightness max been reached for switching the led off.

Quoted 1 line

Pushed to my review and testing queue, thanks!

We found that GNOME's handling of the toggle key is somewhat imperfect
and it will need modifying before we achieve the Up-Up-Up-off-Up-Up-Up-off... cycle that we are looking for.

https://gitlab.gnome.org/GNOME/gnome-settings-daemon/issues/41

In that discussion an alternative perspective was raised:

Is it right for the kernel to modify the key sent to userspace, when
it is then relying on the specific userspace action of it changing the
brightness to the next expected level? (and this userspace behaviour
is not even working right in the GNOME case)

Quoted 5 lines

Instead, would it make sense for the kernel to always report TOGGLE in
this case, and for GNOME to interpret toggle as simply "cycle through
all the available brightness levels"?

Any comments on this? I am tempted to send a patch to just make this key always emit TOGGLE from the kernel given that we have a tentative agreement on implementing the brightness cycle within GNOME.

Daniel

I had a quick chat with Bastien about this. The conclusion was that reusing the TOGGLE key may be problematic for gnome-settings-daemon. And the alternative of a new CYCLE key also has some caveats.

The most straight forward solution is likely to simply handle the brightness change in the kernel and not report the key to userspace at all. This should work just fine and at least GNOME will show an on screen display in response to the brightness change.

Do you think that approach would work well?

Quoted 10 lines · Benjamin Berg

I had a quick chat with Bastien about this. The conclusion was that reusing the TOGGLE key may be problematic for gnome-settings-daemon. And the alternative of a new CYCLE key also has some caveats.

The most straight forward solution is likely to simply handle the brightness change in the kernel and not report the key to userspace at all. This should work just fine and at least GNOME will show an on screen display in response to the brightness change.

Do you think that approach would work well?

Am I right we are waiting for Daniel's answer?

Btw, I mistakenly thought that patch in the queue for-next, while it's not. So, I'm going to drop it now.
Feel free to re-submit a new (better) version.

--
With Best Regards,
Andy Shevchenko

On Tue, May 22, 2018 at 5:18 PM, Andy Shevchenko

Quoted 3 lines

Btw, I mistakenly thought that patch in the queue for-next, while it's not. So, I'm going to drop it now.
Feel free to re-submit a new (better) version.

So it's better to handle NOTIFY_KBD_BRTUP, BRTDOWN, BRTTOGGLE (c4, c5, c7) in the driver and no need to pass it up to userspace? I'll work out a patch for this.

Quoted 5 lines · Chris Chiu

On Tue, May 22, 2018 at 5:18 PM, Andy Shevchenko

Quoted 3 lines

Btw, I mistakenly thought that patch in the queue for-next, while it's not. So, I'm going to drop it now.
Feel free to re-submit a new (better) version.

Quoted 3 lines

So it's better to handle NOTIFY_KBD_BRTUP, BRTDOWN, BRTTOGGLE (c4, c5, c7) in the driver and no need to pass it up to userspace? I'll work out a patch for this.

Yes, let's explore this approach before submitting another patch to Andy.

Thanks
Daniel

I've made my change to set the brightness level directly in the driver, but the
OSD doesn't show correctly correspond to the level value. The brightness shows OK in /sys/class/led/xxxx/brighness but the OSD always shows level 0. I thought GNOME should read the brightness from /sys before showing OSD?

Hi,

Quoted 5 lines · Chris Chiu

I've made my change to set the brightness level directly in the driver, but the
OSD doesn't show correctly correspond to the level value. The brightness shows OK in /sys/class/led/xxxx/brighness but the OSD always shows level 0. I thought GNOME should read the brightness from /sys before showing OSD?

Sorry for the late response.

There is a special mechanism to report that the HW changed the brightness. This works using the "brightness_hw_changed" sysfs attribute. So you will need to set the LED_BRIGHT_HW_CHANGED flag on the LED and then call led_classdev_notify_brightness_hw_changed to make it work.

Userspace should correctly show the OSD when this is done.