Re: [PATCH] platform/x86: dell-laptop: Filter out spurious keyboard backlight change events

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 05-01-18 11:40, Andy Shevchenko wrote:
> On Thu, Jan 4, 2018 at 10:52 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> On some Dell XPS models WMI events of type 0x0000 reporting a keycode of
>> 0xe00c get reported when the brightness of the LCD panel changes.
>>
>> This leads to us reporting false-positive kbd_led change events to
>> userspace which in turn leads to the kbd backlight OSD showing when it
>> should not.
>>
>> We already read the current keyboard backlight brightness value when
>> reporting events because the led_classdev_notify_brightness_hw_changed
>> API requires this. Compare this value to the last known value and filter
>> out duplicate events, fixing this.
>>
>> Note the fixed issue is esp. a problem on XPS models with an ambient light
>> sensor and automatic brightness adjustments turned on, this causes the kbd
>> backlight OSD to show all the time there.
>>
> 
> Should it have Fixes tag?

Good point, yes it should probably have a:

Fixes: 9c656b0799 ("platform/x86: dell-*: Call new led hw_changed API on kbd brightness change")

Regards,

Hans


> 
> Pali, any comment on this?
> 
>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514969
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/platform/x86/dell-laptop.c | 24 ++++++++++++++++++++++--
>>   1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
>> index cd4725e7e0b5..2ef3297a9efc 100644
>> --- a/drivers/platform/x86/dell-laptop.c
>> +++ b/drivers/platform/x86/dell-laptop.c
>> @@ -1133,6 +1133,7 @@ static u8 kbd_previous_mode_bit;
>>
>>   static bool kbd_led_present;
>>   static DEFINE_MUTEX(kbd_led_mutex);
>> +static enum led_brightness kbd_led_level;
>>
>>   /*
>>    * NOTE: there are three ways to set the keyboard backlight level.
>> @@ -1947,6 +1948,7 @@ static enum led_brightness kbd_led_level_get(struct led_classdev *led_cdev)
>>   static int kbd_led_level_set(struct led_classdev *led_cdev,
>>                               enum led_brightness value)
>>   {
>> +       enum led_brightness new_value = value;
>>          struct kbd_state state;
>>          struct kbd_state new_state;
>>          u16 num;
>> @@ -1976,6 +1978,9 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
>>          }
>>
>>   out:
>> +       if (ret == 0)
>> +               kbd_led_level = new_value;
>> +
>>          mutex_unlock(&kbd_led_mutex);
>>          return ret;
>>   }
>> @@ -2003,6 +2008,9 @@ static int __init kbd_led_init(struct device *dev)
>>                  if (kbd_led.max_brightness)
>>                          kbd_led.max_brightness--;
>>          }
>> +
>> +       kbd_led_level = kbd_led_level_get(NULL);
>> +
>>          ret = led_classdev_register(dev, &kbd_led);
>>          if (ret)
>>                  kbd_led_present = false;
>> @@ -2027,13 +2035,25 @@ static void kbd_led_exit(void)
>>   static int dell_laptop_notifier_call(struct notifier_block *nb,
>>                                       unsigned long action, void *data)
>>   {
>> +       bool changed = false;
>> +       enum led_brightness new_kbd_led_level;
>> +
>>          switch (action) {
>>          case DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED:
>>                  if (!kbd_led_present)
>>                          break;
>>
>> -               led_classdev_notify_brightness_hw_changed(&kbd_led,
>> -                                               kbd_led_level_get(&kbd_led));
>> +               mutex_lock(&kbd_led_mutex);
>> +               new_kbd_led_level = kbd_led_level_get(&kbd_led);
>> +               if (kbd_led_level != new_kbd_led_level) {
>> +                       kbd_led_level = new_kbd_led_level;
>> +                       changed = true;
>> +               }
>> +               mutex_unlock(&kbd_led_mutex);
>> +
>> +               if (changed)
>> +                       led_classdev_notify_brightness_hw_changed(&kbd_led,
>> +                                                               kbd_led_level);
>>                  break;
>>          }
>>
>> --
>> 2.14.3
>>
> 
> 
>