[PATCH 0/5] acpi-video and platform/x86 driver fixes

[PATCH 0/5] acpi-video and platform/x86 driver fixes

[Hans de Goede]

Hi All,

This patch-set is the result of the discussion surrounding the
backlight issues on the Dell Vostro V131. The first 3 patches
cleanup how some platform/x86 drivers detect if acpi-video
is handling brightness key presses and that they should
not report duplicate events. This is a cleanup which I wanted
to do for a while already and the Vostro V131 fix actually
benefits from this.

The last 2 patches together actually fix the issues on the
Vostro V131. Since the 2 platform patches in this set depend
on an acpi patch, I believe that it would be best if this
entire set gets merged via the acpi tree with acks
from the platform driver maintainer.

Regards,

Hans

[PATCH 1/5] acpi-video: Add a acpi_video_handles_brightness_key_presses() helper

[Hans de Goede]

Several drivers want to know if the acpi-video is generating key-presses
for brightness change hotkeys to avoid sending double key-events to
userspace for these. Currently these driver use this construct for this:

	if (acpi_video_get_backlight_type() == acpi_backlight_vendor)
		report_brightness_key_event();

This indirect way of detecting if acpi-video is active does not make the
code easier to understand, and in some cases it is wrong because just
because the preferred type != vendor does not mean that acpi-video is
actually listening for brightness events, e.g. there may be no acpi-video
bus on the system at all.

This commit adds a acpi_video_handles_brightness_key_presses() helper
function, making the code needing this functionality both easier to read
and more correct.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpi_video.c | 12 ++++++++++++
 include/acpi/video.h      |  6 ++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 3405f7a..2a649f3e 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -2072,6 +2072,18 @@ void acpi_video_unregister_backlight(void)
 	mutex_unlock(&register_count_mutex);
 }
 
+bool acpi_video_handles_brightness_key_presses(void)
+{
+	bool have_video_busses;
+
+	mutex_lock(&video_list_lock);
+	have_video_busses = !list_empty(&video_bus_head);
+	mutex_unlock(&video_list_lock);
+
+	return have_video_busses;
+}
+EXPORT_SYMBOL(acpi_video_handles_brightness_key_presses);
+
 /*
  * This is kind of nasty. Hardware using Intel chipsets may require
  * the video opregion code to be run first in order to initialise
diff --git a/include/acpi/video.h b/include/acpi/video.h
index c62392d..f11d342 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -2,6 +2,7 @@
 #define __ACPI_VIDEO_H
 
 #include <linux/errno.h> /* for ENODEV */
+#include <linux/types.h> /* for bool */
 
 struct acpi_device;
 
@@ -31,6 +32,7 @@ extern int acpi_video_get_edid(struct acpi_device *device, int type,
 			       int device_id, void **edid);
 extern enum acpi_backlight_type acpi_video_get_backlight_type(void);
 extern void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type type);
+extern bool acpi_video_handles_brightness_key_presses(void);
 #else
 static inline int acpi_video_register(void) { return 0; }
 static inline void acpi_video_unregister(void) { return; }
@@ -46,6 +48,10 @@ static inline enum acpi_backlight_type acpi_video_get_backlight_type(void)
 static inline void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type type)
 {
 }
+static inline bool acpi_video_handles_brightness_key_presses(void)
+{
+	return false;
+}
 #endif
 
 #endif
-- 
2.5.0

[PATCH 2/5] dell-wmi: Use acpi_video_handles_brightness_key_presses()

[Hans de Goede]

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())
 		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

[PATCH 3/5] thinkpad_acpi: Use acpi_video_handles_brightness_key_presses()

[Hans de Goede]

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/thinkpad_acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 0bed473..f453d5d 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3488,7 +3488,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 	/* Do not issue duplicate brightness change events to
 	 * userspace. tpacpi_detect_brightness_capabilities() must have
 	 * been called before this point  */
-	if (acpi_video_get_backlight_type() != acpi_backlight_vendor) {
+	if (acpi_video_handles_brightness_key_presses()) {
 		pr_info("This ThinkPad has standard ACPI backlight "
 			"brightness control, supported by the ACPI "
 			"video driver\n");
-- 
2.5.0

[PATCH 4/5] acpi-video: Add a module option to disable the reporting of keypresses

[Hans de Goede]

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");
+
 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

[PATCH 5/5] acpi-video: Add quirks for the Dell Vostro V131

[Hans de Goede]

The Dell Vostro V131 has an especially broken acpi-video implementation.

The backlight control bits work, but when the brightness is changed via
the acpi-video interface the backlight flickers annoyingly before settling
at the new brightness, switching to using the native interface fixes the
flickering so add a quirk for this (the vendor interface has the same
problem).

Brightness keypresses reported through the acpi-video-bus are also broken,
they get reported one event delayed, so if you press the brightness-up
hotkey on the keyboard nothing happens, then if you press brightness-down,
the previous brightness-up event gets reported. Since the keypresses are
also reported via wmi (if active) and via atkbd (when wmi is not active)
add a quirk to simply filter out the delayed (broken) events.

Reported-and-tested-by: Michał Kępień <kernel@kempniu.pl>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpi_video.c   | 25 +++++++++++++++++++++++++
 drivers/acpi/video_detect.c |  8 ++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 2971154..80b13d4 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -419,6 +419,13 @@ static int video_enable_only_lcd(const struct dmi_system_id *d)
 	return 0;
 }
 
+static int video_set_report_key_events(const struct dmi_system_id *id)
+{
+	if (report_key_events == -1)
+		report_key_events = (uintptr_t)id->driver_data;
+	return 0;
+}
+
 static struct dmi_system_id video_dmi_table[] = {
 	/*
 	 * Broken _BQC workaround http://bugzilla.kernel.org/show_bug.cgi?id=13121
@@ -507,6 +514,24 @@ static struct dmi_system_id video_dmi_table[] = {
 		DMI_MATCH(DMI_PRODUCT_NAME, "ESPRIMO Mobile M9410"),
 		},
 	},
+	/*
+	 * Some machines report wrong key events on the acpi-bus, suppress
+	 * key event reporting on these.  Note this is only intended to work
+	 * around events which are plain wrong. In some cases we get double
+	 * events, in this case acpi-video is considered the canonical source
+	 * and the events from the other source should be filtered. E.g.
+	 * by calling acpi_video_handles_brightness_key_presses() from the
+	 * vendor acpi/wmi driver or by using /lib/udev/hwdb.d/60-keyboard.hwdb
+	 */
+	{
+	 .callback = video_set_report_key_events,
+	 .driver_data = (void *)((uintptr_t)REPORT_OUTPUT_KEY_EVENTS),
+	 .ident = "Dell Vostro V131",
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+		DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
+		},
+	},
 	{}
 };
 
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index daaf1c4..8fe2682 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -279,6 +279,14 @@ static const struct dmi_system_id video_detect_dmi_table[] = {
 		DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro12,1"),
 		},
 	},
+	{
+	 .callback = video_detect_force_native,
+	 .ident = "Dell Vostro V131",
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+		DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
+		},
+	},
 	{ },
 };
 
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] dell-wmi: Use acpi_video_handles_brightness_key_presses()

[Darren Hart]

On Tue, Dec 22, 2015 at 07:09:49PM +0100, Hans de Goede wrote:

+ Pali

> 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())
>  		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
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 4/5] acpi-video: Add a module option to disable the reporting of keypresses

[Darren Hart]

On Tue, Dec 22, 2015 at 07:09:51PM +0100, Hans de Goede wrote:
> 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

Since report_key_events is used as a bitmask, it might be preferable to use
bitops.

> +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");
> +
>  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)) {

This and the above test would be more explicit if written as:

if (keycode && (report_key_events & BIT(REPORT_BRIGHTNESS_KEY_EVENTS))

Do you have a preference Rafael?

>  		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
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 0/5] acpi-video and platform/x86 driver fixes

[Darren Hart]

On Tue, Dec 22, 2015 at 07:09:47PM +0100, Hans de Goede wrote:
> Hi All,
> 
> This patch-set is the result of the discussion surrounding the
> backlight issues on the Dell Vostro V131. The first 3 patches
> cleanup how some platform/x86 drivers detect if acpi-video
> is handling brightness key presses and that they should
> not report duplicate events. This is a cleanup which I wanted
> to do for a while already and the Vostro V131 fix actually
> benefits from this.
> 
> The last 2 patches together actually fix the issues on the
> Vostro V131. Since the 2 platform patches in this set depend
> on an acpi patch, I believe that it would be best if this
> entire set gets merged via the acpi tree with acks
> from the platform driver maintainer.

Agreed on the path. For this situation, the addition of the module parameter
will facilitate identifying similarly broken paltforms to the Vostro V131, and
provide users with a temporary solution until the DMI match can be added. Very
practical.

I have no objection to the changes in platform-drivers-x86.

Reviewed-by: Darren Hart <dvhart@linux.intel.com>

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 2/5] dell-wmi: Use acpi_video_handles_brightness_key_presses()

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 666 bytes --]

On Tuesday 22 December 2015 20:53:19 Darren Hart wrote:
> On Tue, Dec 22, 2015 at 07:09:49PM +0100, Hans de Goede wrote:
> 
> + Pali
> 
> > 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>

After discussion I'm fine with this patch. Add my
Acked-by: Pali Rohár <pali.rohar@gmail.com>

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/5] thinkpad_acpi: Use acpi_video_handles_brightness_key_presses()

[Henrique de Moraes Holschuh]

On Tue, 22 Dec 2015, Hans de Goede wrote:
> 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.

If this new function will return false when acpi video is not reporting
keypresses *BUT* still allowing any sort of brightness changes (e.g. through
sysfs), I don't think it is safe in thinkpad-acpi's case.

So, will it return false in any situation whether acpi-video attached to the
backlight class?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 3/5] thinkpad_acpi: Use acpi_video_handles_brightness_key_presses()

[Hans de Goede]

Hi,

On 28-12-15 00:08, Henrique de Moraes Holschuh wrote:
> On Tue, 22 Dec 2015, Hans de Goede wrote:
>> 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.
>
> If this new function will return false when acpi video is not reporting
> keypresses *BUT* still allowing any sort of brightness changes (e.g. through
> sysfs), I don't think it is safe in thinkpad-acpi's case.

Have you closely looked at the patch? It only applies to this bit of
thinkpad-acpi code:

         /* Do not issue duplicate brightness change events to
          * userspace. tpacpi_detect_brightness_capabilities() must have
          * been called before this point  */
         if (acpi_video_handles_brightness_key_presses()) {
                 pr_info("This ThinkPad has standard ACPI backlight "
                         "brightness control, supported by the ACPI "
                         "video driver\n");
                 pr_notice("Disabling thinkpad-acpi brightness events "
                           "by default...\n");

                 /* Disable brightness up/down on Lenovo thinkpads when
                  * ACPI is handling them, otherwise it is plain impossible
                  * for userspace to do something even remotely sane */
                 hotkey_reserved_mask |=
                         (1 << TP_ACPI_HOTKEYSCAN_FNHOME)
                         | (1 << TP_ACPI_HOTKEYSCAN_FNEND);
                 hotkey_unmap(TP_ACPI_HOTKEYSCAN_FNHOME);
                 hotkey_unmap(TP_ACPI_HOTKEYSCAN_FNEND);
         }

So it only disables hotkey press reporting, not to the thinkpad_acpi
backlight handling code, that still is using acpi_video_get_backlight_type()  :

         if (acpi_video_get_backlight_type() != acpi_backlight_vendor) {
                 if (brightness_enable > 1) {
                         pr_info("Standard ACPI backlight interface "
                                 "available, not loading native one\n");
                         return 1;
                 } else if (brightness_enable == 1) {
                         pr_warn("Cannot enable backlight brightness support, "
                                 "ACPI is already handling it.  Refer to the "
                                 "acpi_backlight kernel parameter.\n");
                         return 1;
                 }
         } else if (tp_features.bright_acpimode && brightness_enable > 1) {
                 pr_notice("Standard ACPI backlight interface not "
                           "available, thinkpad_acpi native "
                           "brightness control enabled\n");
         }

> So, will it return false in any situation whether acpi-video attached to the
> backlight class?

When the new video.report_key_events kernel cmdline option gets manually set to
a non default value then yes it may report false while acpi-video is attached
to the backlight class. By default, no it will never return false when acpi-video
is attached to the backlight class.

This should not matter though, as said this new function is only used to suppress
the reporting of key-presses. I realize that doing so will also cause
tpacpi_driver_event() to get called on brightness hotkey presses, but that has:

         if (ibm_backlight_device) {
                 switch (hkey_event) {
                 case TP_HKEY_EV_BRGHT_UP:
                 case TP_HKEY_EV_BRGHT_DOWN:
                         tpacpi_brightness_notify_change();
                 }
         }

And the creating of ibm_backlight_device is still protected by

if (acpi_video_get_backlight_type() != acpi_backlight_vendor) return 1;

So this new check really does nothing other then NOT suppress the hotkey presses
reported by thinkpad-acpi when for some reason a user has asked acpi-video to
suppress its reporting of hotkey presses, which is exactly what it should do.

IMHO this only shows that having a separate function to detect if acpi-video
is reporting keypresses is the right thing to do.

Regards,

Hans

Re: [PATCH 3/5] thinkpad_acpi: Use acpi_video_handles_brightness_key_presses()

[Henrique de Moraes Holschuh]

On Tue, Dec 29, 2015, at 10:27, Hans de Goede wrote:
> So it only disables hotkey press reporting, not to the thinkpad_acpi
> backlight handling code, that still is using
> acpi_video_get_backlight_type()  :

...

> This should not matter though, as said this new function is only used to
> suppress
> the reporting of key-presses. I realize that doing so will also cause
> tpacpi_driver_event() to get called on brightness hotkey presses, but
> that has:
> 
>          if (ibm_backlight_device) {
>                  switch (hkey_event) {
>                  case TP_HKEY_EV_BRGHT_UP:
>                  case TP_HKEY_EV_BRGHT_DOWN:
>                          tpacpi_brightness_notify_change();
>                  }
>          }
> 
> And the creating of ibm_backlight_device is still protected by
> 
> if (acpi_video_get_backlight_type() != acpi_backlight_vendor) return 1;

Indeed.  I have no objections to the patch, then.

Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 0/5] acpi-video and platform/x86 driver fixes

[Rafael J. Wysocki]

On Tuesday, December 22, 2015 07:09:47 PM Hans de Goede wrote:
> Hi All,
> 
> This patch-set is the result of the discussion surrounding the
> backlight issues on the Dell Vostro V131. The first 3 patches
> cleanup how some platform/x86 drivers detect if acpi-video
> is handling brightness key presses and that they should
> not report duplicate events. This is a cleanup which I wanted
> to do for a while already and the Vostro V131 fix actually
> benefits from this.
> 
> The last 2 patches together actually fix the issues on the
> Vostro V131. Since the 2 platform patches in this set depend
> on an acpi patch, I believe that it would be best if this
> entire set gets merged via the acpi tree with acks
> from the platform driver maintainer.

Series applied, thanks!