[PATCH] platform/x86: dell-laptop: Fix bogus keyboard backlight sysfs interface

[PATCH] platform/x86: dell-laptop: Fix bogus keyboard backlight sysfs interface

[Kai-Heng Feng]

Dell Latitude 3160 does not have keyboard backlight, but there is a
sysfs interface for it, which does nothing at all.

KBD_LED_OFF_TOKEN is the only token can be found. Since it doesn't have
KBD_LED_ON_TOKEN or KBD_LED_AUTO_*_TOKEN, it should be safe to assume it
does not support keyboard backlight.

Reports keyboard backlight is supported only when tokens other than
KBD_LED_OFF_TOKEN can be found.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/platform/x86/dell-laptop.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index ec202094bd50..743d7ce8c0c8 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1510,7 +1510,11 @@ static void kbd_init(void)
 	ret = kbd_init_info();
 	kbd_init_tokens();
 
-	if (kbd_token_bits != 0 || ret == 0)
+	/*
+	 * If KBD_LED_OFF_TOKEN is the only token,
+	 * consider there is no keyboard backlight.
+	 */
+	if ((kbd_token_bits & ~BIT(5)) != 0 || ret == 0)
 		kbd_led_present = true;
 }
 
-- 
2.13.1

Re: [PATCH] platform/x86: dell-laptop: Fix bogus keyboard backlight sysfs interface

[Pali Rohár]

Hi!

On Friday 16 June 2017 15:35:39 Kai-Heng Feng wrote:
> Dell Latitude 3160 does not have keyboard backlight, but there is a
> sysfs interface for it, which does nothing at all.
> 
> KBD_LED_OFF_TOKEN is the only token can be found. Since it doesn't have
> KBD_LED_ON_TOKEN or KBD_LED_AUTO_*_TOKEN, it should be safe to assume it
> does not support keyboard backlight.
> 
> Reports keyboard backlight is supported only when tokens other than
> KBD_LED_OFF_TOKEN can be found.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/platform/x86/dell-laptop.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index ec202094bd50..743d7ce8c0c8 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -1510,7 +1510,11 @@ static void kbd_init(void)
>  	ret = kbd_init_info();
>  	kbd_init_tokens();
>  
> -	if (kbd_token_bits != 0 || ret == 0)
> +	/*
> +	 * If KBD_LED_OFF_TOKEN is the only token,
> +	 * consider there is no keyboard backlight.
> +	 */
> +	if ((kbd_token_bits & ~BIT(5)) != 0 || ret == 0)

Should not this check to be rather:

(kbd_token_bits != 0 && (kbd_token_bits & BIT(KBD_LED_OFF_TOKEN)) != BIT(KBD_LED_OFF_TOKEN))

To express that we have at least one token at it is different from
KBD_LED_OFF_TOKEN token?

>  		kbd_led_present = true;
>  }
>  

And more important, there are three ways how to control keyboard
backlight level:

1) Via SMBIOS token
2) Via SMBIOS call 4/11/0x2 (arg2, byte0)
3) Via SMBIOS call 4/11/0x2 (arg3, byte2)

You are adding special case when only one SMBIOS toekn OFF is present
which belongs to 1).

Therefore there should be same check for 2) and 3) that there are more
the one option to set...

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH] platform/x86: dell-laptop: Fix bogus keyboard backlight sysfs interface

[Kai-Heng Feng]

On Fri, Jun 16, 2017 at 3:52 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> Should not this check to be rather:
>
> (kbd_token_bits != 0 && (kbd_token_bits & BIT(KBD_LED_OFF_TOKEN)) != BIT(KBD_LED_OFF_TOKEN))
>
> To express that we have at least one token at it is different from
> KBD_LED_OFF_TOKEN token?

Yes, this expresses the intention more clearly. I'll use it instead.

>
>>               kbd_led_present = true;
>>  }
>>
>
> And more important, there are three ways how to control keyboard
> backlight level:
>
> 1) Via SMBIOS token
> 2) Via SMBIOS call 4/11/0x2 (arg2, byte0)
> 3) Via SMBIOS call 4/11/0x2 (arg3, byte2)
>
> You are adding special case when only one SMBIOS toekn OFF is present
> which belongs to 1).
>
> Therefore there should be same check for 2) and 3) that there are more
> the one option to set...

I am not familiar with SMBIOS call.
Can you point out where 2) and 3) functions are?

>
> --
> Pali Rohár
> pali.rohar@gmail.com

Re: [PATCH] platform/x86: dell-laptop: Fix bogus keyboard backlight sysfs interface

[Pali Rohár]

On Friday 16 June 2017 17:46:58 Kai-Heng Feng wrote:
> On Fri, Jun 16, 2017 at 3:52 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > Should not this check to be rather:
> >
> > (kbd_token_bits != 0 && (kbd_token_bits & BIT(KBD_LED_OFF_TOKEN)) != BIT(KBD_LED_OFF_TOKEN))
> >
> > To express that we have at least one token at it is different from
> > KBD_LED_OFF_TOKEN token?
> 
> Yes, this expresses the intention more clearly. I'll use it instead.

Err... second part of condition is wrong. It should be:

(kbd_token_bits & ~BIT(KBD_LED_OFF_TOKEN))

(Remove off token bit and check that there is some other bit set too)

> >
> >>               kbd_led_present = true;
> >>  }
> >>
> >
> > And more important, there are three ways how to control keyboard
> > backlight level:
> >
> > 1) Via SMBIOS token
> > 2) Via SMBIOS call 4/11/0x2 (arg2, byte0)
> > 3) Via SMBIOS call 4/11/0x2 (arg3, byte2)
> >
> > You are adding special case when only one SMBIOS toekn OFF is present
> > which belongs to 1).
> >
> > Therefore there should be same check for 2) and 3) that there are more
> > the one option to set...
> 
> I am not familiar with SMBIOS call.
> Can you point out where 2) and 3) functions are?

See function kbd_led_level_set() (which calls kbd_set_level()). Also see
comment above function kbd_get_info().

For 2) there is kbd_mode_levels_count and for 3) there is
kbd_info.levels.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH] platform/x86: dell-laptop: Fix bogus keyboard backlight sysfs interface

[Andy Shevchenko]

On Fri, Jun 16, 2017 at 10:52 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Friday 16 June 2017 15:35:39 Kai-Heng Feng wrote:

>> -     if (kbd_token_bits != 0 || ret == 0)
>> +     /*
>> +      * If KBD_LED_OFF_TOKEN is the only token,
>> +      * consider there is no keyboard backlight.
>> +      */
>> +     if ((kbd_token_bits & ~BIT(5)) != 0 || ret == 0)
>
> Should not this check to be rather:
>
> (kbd_token_bits != 0 && (kbd_token_bits & BIT(KBD_LED_OFF_TOKEN)) != BIT(KBD_LED_OFF_TOKEN))
>
> To express that we have at least one token at it is different from
> KBD_LED_OFF_TOKEN token?

Isn't the same to

(kbd_token_bits ^ BIT(KBD_LED_OFF_TOKEN))

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: dell-laptop: Fix bogus keyboard backlight sysfs interface

[Andy Shevchenko]

On Fri, Jun 16, 2017 at 10:35 AM, Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
> Dell Latitude 3160 does not have keyboard backlight, but there is a
> sysfs interface for it, which does nothing at all.
>
> KBD_LED_OFF_TOKEN is the only token can be found. Since it doesn't have
> KBD_LED_ON_TOKEN or KBD_LED_AUTO_*_TOKEN, it should be safe to assume it
> does not support keyboard backlight.
>
> Reports keyboard backlight is supported only when tokens other than
> KBD_LED_OFF_TOKEN can be found.

Okay, I'm waiting for v2 with Pali's or mine suggestion applied.

>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/platform/x86/dell-laptop.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index ec202094bd50..743d7ce8c0c8 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -1510,7 +1510,11 @@ static void kbd_init(void)
>         ret = kbd_init_info();
>         kbd_init_tokens();
>
> -       if (kbd_token_bits != 0 || ret == 0)
> +       /*
> +        * If KBD_LED_OFF_TOKEN is the only token,
> +        * consider there is no keyboard backlight.
> +        */
> +       if ((kbd_token_bits & ~BIT(5)) != 0 || ret == 0)
>                 kbd_led_present = true;
>  }
>
> --
> 2.13.1
>



-- 
With Best Regards,
Andy Shevchenko