[PATCH] platform/x86: asus-wmi: Fix keyboard brightness cannot be set to 0

[PATCH] platform/x86: asus-wmi: Fix keyboard brightness cannot be set to 0

[Jian-Hong Pan]

Some of ASUS laptops like UX431FL keyboard backlight cannot be set to
brightness 0. According to ASUS' information, the brightness should be
0x80 ~ 0x83. This patch fixes it by following the logic.

Fixes: e9809c0b9670 ("asus-wmi: add keyboard backlight support")
Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
---
 drivers/platform/x86/asus-wmi.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 821b08e01635..982f0cc8270c 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -512,13 +512,7 @@ static void kbd_led_update(struct asus_wmi *asus)
 {
 	int ctrl_param = 0;
 
-	/*
-	 * bits 0-2: level
-	 * bit 7: light on/off
-	 */
-	if (asus->kbd_led_wk > 0)
-		ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
-
+	ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
 	asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
 }
 
-- 
2.20.1

Re: [PATCH] platform/x86: asus-wmi: Fix keyboard brightness cannot be set to 0

[Daniel Drake]

On Mon, Dec 30, 2019 at 4:32 PM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
>
> Some of ASUS laptops like UX431FL keyboard backlight cannot be set to
> brightness 0. According to ASUS' information, the brightness should be
> 0x80 ~ 0x83. This patch fixes it by following the logic.
>
> Fixes: e9809c0b9670 ("asus-wmi: add keyboard backlight support")

The spec says says bit 7 is Set light on, and bits 0~3 are level,
similar to the comment being removed. But indeed it isn't entirely
clear about how to turn it off (since what does Light on but level 0
mean?).

This code goes back to 2011, so there's a risk of inversely affecting
old models with this change.

I checked our DSDT collection and the behaviour is quite inconsistent.

On the UX431FLC that you fixed with this patch, we reach:

        Method (SLKI, 1, NotSerialized)
        {
            Local0 = (Arg0 & 0x80)
            If (Local0)
            {
                Local1 = (Arg0 & 0x7F)
                If ((Local1 >= 0x04))
                {
                    Local1 = Zero
                }

                \_SB.PCI0.LPCB.H_EC.KBLL = Local1
                KBLV = Local1
            }

            Return (Local0)
        }

Nothing will happen unless bit 0x80 is set. So that's why your patch
fixes the problem in this case. But In 81 DSDTs examined this is the
only model that exhibits this behaviour. Perhaps it is the very latest
revision that will be rolled out from this point.

Many other models have this:

        Name (PWKB, Buffer (0x04)
        {
             0x00, 0x55, 0xAA, 0xFF                           // .U..
        })
        Method (SLKB, 1, NotSerialized)
        {
            KBLV = (Arg0 & 0x7F)
            If ((Arg0 & 0x80))
            {
                Local0 = DerefOf (PWKB [KBLV])
            }
            Else
            {
                Local0 = Zero
            }

            ST9E (0x1F, 0xFF, Local0)
            Return (One)
        }

for which your patch is also OK. You can follow it through and see
that value 0x0 and 0x80 both result in the same single register write
of value 0.

But there are 30 models (e.g. UX331UN) that will see a behaviour
change via this patch:

        Method (SLKB, 1, NotSerialized)
        {
            KBLV = (Arg0 & 0x7F)
            If ((Arg0 & 0x80))
            {
                Local0 = 0x0900
                Local0 += 0xF0
                WRAM (Local0, KBLV)
                Local0 = DerefOf (PWKB [KBLV])
            }
            Else
            {
                Local0 = Zero
            }

            ST9E (0x1F, 0xFF, Local0)
            Return (One)
        }

Here, writing 0x80 to turn off the keyboard LED will result in an
additional WRAM(0x9f0, 0) call that was not there before. I think we
should double check this detail.

Let's see if we can borrow one of the affected models and double check
this patch there before proceeding. I'll follow up internally.

(I also checked eeepc, but it seems like they don't have a keyboard backlight)


> Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
> ---
>  drivers/platform/x86/asus-wmi.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 821b08e01635..982f0cc8270c 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -512,13 +512,7 @@ static void kbd_led_update(struct asus_wmi *asus)
>  {
>         int ctrl_param = 0;
>
> -       /*
> -        * bits 0-2: level
> -        * bit 7: light on/off
> -        */
> -       if (asus->kbd_led_wk > 0)
> -               ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
> -
> +       ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
>         asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
>  }
>
> --
> 2.20.1
>

Re: [PATCH] platform/x86: asus-wmi: Fix keyboard brightness cannot be set to 0

[Daniel Drake]

On Tue, Dec 31, 2019 at 2:53 PM Daniel Drake <drake@endlessm.com> wrote:
> Here, writing 0x80 to turn off the keyboard LED will result in an
> additional WRAM(0x9f0, 0) call that was not there before. I think we
> should double check this detail.
>
> Let's see if we can borrow one of the affected models and double check
> this patch there before proceeding. I'll follow up internally.

Asus were unable to find a product sample with the affected behaviour.
They did provide us with one from the list I had made, but with a
newer BIOS version where that behaviour has been eliminated. They also
advised that always setting bit 7 is the way they do it on Windows. So
I don't think we have the opportunity for extra verification, but it
should be safe.

Reviewed-by: Daniel Drake <drake@endlessm.com>

Re: [PATCH] platform/x86: asus-wmi: Fix keyboard brightness cannot be set to 0

[Andy Shevchenko]

On Mon, Jan 13, 2020 at 5:47 AM Daniel Drake <drake@endlessm.com> wrote:
>
> On Tue, Dec 31, 2019 at 2:53 PM Daniel Drake <drake@endlessm.com> wrote:
> > Here, writing 0x80 to turn off the keyboard LED will result in an
> > additional WRAM(0x9f0, 0) call that was not there before. I think we
> > should double check this detail.
> >
> > Let's see if we can borrow one of the affected models and double check
> > this patch there before proceeding. I'll follow up internally.
>
> Asus were unable to find a product sample with the affected behaviour.
> They did provide us with one from the list I had made, but with a
> newer BIOS version where that behaviour has been eliminated. They also
> advised that always setting bit 7 is the way they do it on Windows. So
> I don't think we have the opportunity for extra verification, but it
> should be safe.
>

> Reviewed-by: Daniel Drake <drake@endlessm.com>

Thank you, Daniel! I'll queue it soon to my tree.

-- 
With Best Regards,
Andy Shevchenko