[PATCH v2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[PATCH v2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[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.

Signed-off-by: Chris Chiu <chiu@endlessm.com>
Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
Tested-by: Jian-Hong Pan <jian-hong@endlessm.com>
---

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(+)

diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c
index 136ff2b4cce5..14c502e18579 100644
--- a/drivers/platform/x86/asus-nb-wmi.c
+++ b/drivers/platform/x86/asus-nb-wmi.c
@@ -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},
 };
 
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 1f6e68f0b646..4a3ba575c9ce 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -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

Re: [PATCH v2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[Andy Shevchenko]

On Thu, May 3, 2018 at 6:04 AM, Chris Chiu <chiu@endlessm.com> wrote:
> 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!

> Signed-off-by: Chris Chiu <chiu@endlessm.com>
> Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
> Tested-by: Jian-Hong Pan <jian-hong@endlessm.com>
> ---
>
> 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(+)
>
> diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c
> index 136ff2b4cce5..14c502e18579 100644
> --- a/drivers/platform/x86/asus-nb-wmi.c
> +++ b/drivers/platform/x86/asus-nb-wmi.c
> @@ -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},
>  };
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 1f6e68f0b646..4a3ba575c9ce 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -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
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[Daniel Drake]

Hi Andy,

On Mon, May 7, 2018 at 8:10 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, May 3, 2018 at 6:04 AM, Chris Chiu <chiu@endlessm.com> wrote:
> > 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"?

We'd be interested in your thoughts.

Thanks
Daniel

Re: [PATCH v2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[Daniel Drake]

Hi Andy,

On Mon, May 7, 2018 at 8:46 AM, Daniel Drake <drake@endlessm.com> wrote:
> > > 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"?

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

Re: [PATCH v2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[Andy Shevchenko]

On Tue, May 15, 2018 at 3:25 AM, Daniel Drake <drake@endlessm.com> wrote:
> Hi Andy,
>
> On Mon, May 7, 2018 at 8:46 AM, Daniel Drake <drake@endlessm.com> wrote:
>> > > 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"?
>
> 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.

The patch is slipped to our for-next queue. We have about 1-2 weeks to
fix things there if the initial approach is not what you want.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[Benjamin Berg]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Hi 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?

Benjamin


On Mon, 2018-05-14 at 18:25 -0600, Daniel Drake wrote:
> Hi Andy,
> 
> On Mon, May 7, 2018 at 8:46 AM, Daniel Drake <drake@endlessm.com>
> wrote:
> > > > 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"?
> 
> 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
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEED2NO4vMS33W8E4AFq6ZWhpmFY3AFAlr6434ACgkQq6ZWhpmF
Y3D97A/9Hqi/xvHU5jcgtKQ2AH3pN445whjXrnUPpcN3I6zgeo/2kXHxn+DEFzD/
xsBLRy2hzfvw5HXWsclf8u31TajmP9OOihHTMB+Ng7ZJohIEgwPjIMr/eaHiiLYr
W9aamm/5DJF5cwYXmiQ46+y+Wbc5ZJbu0wSgiMDY2uEhlph/9J3NGRadb6xEWJX+
Rrp2te5lqwIvHKv4tF/HOrXev6R77AdpHy+WYrA1IKdG996kFDAZcLErk5EL/J0U
+p+pkGbul/JHKcwU77L/Sg8daM1GMQfxWKbR9k21Fow27iaJRMn9Vn9s8HEmHJ+5
S8cBIo71EjNHXO2oDaSzI9Ng1q+Zy7XjHtbK8/MmauQ/3n/z3tt8ZkW4/FumdMkm
ZZwmS9OxOTc+G9hc3SLkggRF1ygbaCgsZaynyEiBHSq37V0G9WBL6BaDW9P6jEIV
7aPJN6SEBLqXStBZiwNXn/yr1AC7duhnIvzxYZj/SUdCk18aDS/tGIehGTqTumGV
2jhyHQek2fp7J0AZH8oOnU9rAIdKtPb3vf5DJuCOD2TtB/yMZDJVj1Uc1g565IPs
KcVdMdiqTy7m65BAwjJyrFu04GIi02SBFFEsdKGOkAcuPPWpG+4EnUwGpArwxwYv
SaUnUD4DRvsoyZHeXETsa7ePh9aBZwmNR+AGSoLj8UY+vbh3/dg=
=szZs
-----END PGP SIGNATURE-----

Re: [PATCH v2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[Andy Shevchenko]

On Tue, May 15, 2018 at 4:41 PM, Benjamin Berg <bberg@redhat.com> wrote:

> 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

Re: [PATCH v2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[Chris Chiu]

On Tue, May 22, 2018 at 5:18 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, May 15, 2018 at 4:41 PM, Benjamin Berg <bberg@redhat.com> wrote:
>
>> 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

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.

Chris

Re: [PATCH v2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[Daniel Drake]

On Tue, May 22, 2018 at 4:11 AM, Chris Chiu <chiu@endlessm.com> wrote:
> On Tue, May 22, 2018 at 5:18 PM, Andy Shevchenko
>> 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.

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

Thanks
Daniel

Re: [PATCH v2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[Chris Chiu]

On Tue, May 22, 2018 at 9:48 PM, Daniel Drake <drake@endlessm.com> wrote:
> On Tue, May 22, 2018 at 4:11 AM, Chris Chiu <chiu@endlessm.com> wrote:
>> On Tue, May 22, 2018 at 5:18 PM, Andy Shevchenko
>>> 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.
>
> Yes, let's explore this approach before submitting another patch to Andy.
>
> Thanks
> Daniel

Hi Andy,
    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?

Chris

Re: [PATCH v2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[Benjamin Berg]

[-- Attachment #1: Type: text/plain, Size: 765 bytes --]

Hi,

On Thu, 2018-05-24 at 16:33 +0800, Chris Chiu wrote:
> 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.

Benjamin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[Darren Hart]

On Mon, Jun 04, 2018 at 11:27:43AM +0200, Benjamin Berg wrote:
> Hi,
> 
> On Thu, 2018-05-24 at 16:33 +0800, Chris Chiu wrote:
> > 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.

This makes sense. Userspace can't be aware of every platforms sys files,
so there needs to be a common mechanism. LED_BRIGHT_HW_CHANGED makes
sense.

-- 
Darren Hart
VMware Open Source Technology Center