From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751525AbeAELFu (ORCPT + 1 other); Fri, 5 Jan 2018 06:05:50 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:45744 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751482AbeAELFs (ORCPT ); Fri, 5 Jan 2018 06:05:48 -0500 X-Google-Smtp-Source: ACJfBotX/kxHfk52oyuPueVLuoa7s0TWDZsGdK15K/OFXwBOU65h/Az1qWLOaZJvupStZbWBpJTt8A== Subject: Re: [PATCH] platform/x86: dell-laptop: Filter out spurious keyboard backlight change events To: Andy Shevchenko Cc: Darren Hart , Andy Shevchenko , =?UTF-8?Q?Pali_Roh=c3=a1r?= , Platform Driver , Linux Kernel Mailing List References: <20180104085256.31431-1-hdegoede@redhat.com> From: Hans de Goede Message-ID: <39d77a3a-8fde-0075-21e5-2318c94295ec@redhat.com> Date: Fri, 5 Jan 2018 12:05:45 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: Hi, On 05-01-18 11:40, Andy Shevchenko wrote: > On Thu, Jan 4, 2018 at 10:52 AM, Hans de Goede 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 >> --- >> 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 >> > > >