Re: Dell Vostro V131 hotkeys revisited

["Pali Rohár" <pali.rohar@gmail.com>]

Hi Hans! See my comments below about your patches.

> From 9355058f5c1d7c815a293e0c0731d85f0a59b4a1 Mon Sep 17 00:00:00 2001
> From: Hans de Goede <hdegoede@redhat.com>
> Date: Thu, 17 Dec 2015 09:59:01 +0100
> Subject: [PATCH 2/4] dell-wmi: Use acpi_video_handles_brightness_key_presses()
> 
> Use the new acpi_video_handles_brightness_key_presses function to check
> if we should report brightness key-presses.
> 
> This makes the code both easier to read and makes it properly report
> key-presses when acpi-video is not reporting them for reasons other
> then the backlight type being vendor.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/dell-wmi.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index f2d77fe..cb8a9c2 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -43,8 +43,6 @@ MODULE_LICENSE("GPL");
>  
>  #define DELL_EVENT_GUID "9DBB5994-A997-11DA-B012-B622A1EF5492"
>  
> -static int acpi_video;
> -
>  MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
>  
>  /*
> @@ -159,7 +157,8 @@ static void dell_wmi_process_key(int reported_key)
>  
>  	/* Don't report brightness notifications that will also come via ACPI */
>  	if ((key->keycode == KEY_BRIGHTNESSUP ||
> -	     key->keycode == KEY_BRIGHTNESSDOWN) && acpi_video)
> +	     key->keycode == KEY_BRIGHTNESSDOWN) &&
> +	    acpi_video_handles_brightness_key_presses())

I do not like this, because that function uses mutex and is called every
time when brightness key event is received by wmi notify handler.

>  		return;
>  
>  	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
> @@ -398,7 +397,6 @@ static int __init dell_wmi_init(void)
>  	}
>  
>  	dmi_walk(find_hk_type, NULL);
> -	acpi_video = acpi_video_get_backlight_type() != acpi_backlight_vendor;
>  
>  	err = dell_wmi_input_setup();
>  	if (err)
> -- 
> 2.5.0
> 

> From 5e7ff99407aeb60c2f1516cdd80d7859646497dd Mon Sep 17 00:00:00 2001
> From: Hans de Goede <hdegoede@redhat.com>
> Date: Wed, 16 Dec 2015 11:19:00 +0100
> Subject: [PATCH 4/4] acpi-video: Add a module option to disable the reporting
>  of keypresses
> 
> Add a module option to disable the reporting of keypresses, in some buggy
> firmware implementatinon, the reported events are wrong. E.g. they lag
> reality by one event in the case triggering the writing of this patch.
> 
> In this case it is better to not forward these wrong events to userspace
> (esp.) when there is another source of the same events which is not buggy.
> 
> Note this is only intended to work around implementations which send
> events which are plain wrong. In some cases we get double events, e.g.
> from both acpi-video and the atkbd driver, in this case acpi-video is
> considered the canonical source, and the events from the other source
> should be filtered (using e.g. /lib/udev/hwdb.d/60-keyboard.hwdb).
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/acpi/acpi_video.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index 2a649f3e..2971154 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -77,6 +77,13 @@ module_param(allow_duplicates, bool, 0644);
>  static int disable_backlight_sysfs_if = -1;
>  module_param(disable_backlight_sysfs_if, int, 0444);
>  
> +#define REPORT_OUTPUT_KEY_EVENTS		0x01
> +#define REPORT_BRIGHTNESS_KEY_EVENTS		0x02
> +static int report_key_events = -1;
> +module_param(report_key_events, int, 0644);
> +MODULE_PARM_DESC(report_key_events,
> +	"0: none, 1: output changes, 2: brightness changes, 3: all");
> +

-1 is default value? Should not be it 3? Or -1 as some default which use
quirks?

>  static bool device_id_scheme = false;
>  module_param(device_id_scheme, bool, 0444);
>  
> @@ -1480,7 +1487,7 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event)
>  		/* Something vetoed the keypress. */
>  		keycode = 0;
>  
> -	if (keycode) {
> +	if (keycode && (report_key_events & REPORT_OUTPUT_KEY_EVENTS)) {
>  		input_report_key(input, keycode, 1);
>  		input_sync(input);
>  		input_report_key(input, keycode, 0);
> @@ -1544,7 +1551,7 @@ static void acpi_video_device_notify(acpi_handle handle, u32 event, void *data)
>  
>  	acpi_notifier_call_chain(device, event, 0);
>  
> -	if (keycode) {
> +	if (keycode && (report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS)) {
>  		input_report_key(input, keycode, 1);
>  		input_sync(input);
>  		input_report_key(input, keycode, 0);
> @@ -2080,7 +2087,8 @@ bool acpi_video_handles_brightness_key_presses(void)
>  	have_video_busses = !list_empty(&video_bus_head);
>  	mutex_unlock(&video_list_lock);
>  
> -	return have_video_busses;
> +	return have_video_busses &&
> +	       (report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS);
>  }
>  EXPORT_SYMBOL(acpi_video_handles_brightness_key_presses);
>  
> -- 
> 2.5.0
> 

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