[PATCH 0/7] ACPI: video / platform/x86: Fix Panasonic laptop missing keypresses

[PATCH 0/7] ACPI: video / platform/x86: Fix Panasonic laptop missing keypresses

[Hans de Goede]

Hi All,

Here is a series fixing the missing keypresses on some Panasonic Toughbook
models. These missing keypresses are caused by
commit ed83c9171829 ("platform/x86: panasonic-laptop: Resolve hotkey double
trigger bug"), which made the panasonic-laptop driver unconditionally drop
most WMI reported hotkeys.

This series reverts that commit and then adds some more selective filtering
to still avoid the double key-presses on some models, while avoiding
completely missing keypresses on others.

Rafael, these fixes rely on patch 1/7, which is a tweak to
the acpi_video_handles_brightness_key_presses() helper. Without this
tweak this series will cause a regression, so I would like to merge
the entire series through the pdx86 tree, may I have your ack for this?

Regards,

Hans


Hans de Goede (6):
  ACPI: video: Change how we determine if brightness key-presses are
    handled
  platform/x86: panasonic-laptop: sort includes alphabetically
  platform/x86: panasonic-laptop: revert "Resolve hotkey double trigger
    bug"
  platform/x86: panasonic-laptop: don't report duplicate brightness
    key-presses
  platform/x86: panasonic-laptop: filter out duplicate volume
    up/down/mute keypresses
  platform/x86: panasonic-laptop: Use acpi_video_get_backlight_type()

Stefan Seyfried (1):
  platform/x86: panasonic-laptop: de-obfuscate button codes

 drivers/acpi/acpi_video.c               |  13 ++-
 drivers/platform/x86/Kconfig            |   2 +
 drivers/platform/x86/panasonic-laptop.c | 112 ++++++++++++++++++------
 3 files changed, 91 insertions(+), 36 deletions(-)

-- 
2.36.0

[PATCH 1/7] ACPI: video: Change how we determine if brightness key-presses are handled

[Hans de Goede]

Some systems have an ACPI video bus but not ACPI video devices with
backlight capability. On these devices brightness key-presses are
(logically) not reported through the ACPI video bus.

Change how acpi_video_handles_brightness_key_presses() determines if
brightness key-presses are handled by the ACPI video driver to avoid
vendor specific drivers/platform/x86 drivers filtering out their
brightness key-presses even though they are the only ones reporting
these presses.

Fixes: ed83c9171829 ("platform/x86: panasonic-laptop: Resolve hotkey double trigger bug")
Reported-and-tested-by: Stefan Seyfried <seife+kernel@b1-systems.com>
Reported-and-tested-by: Kenneth Chan <kenneth.t.chan@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpi_video.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index b6ee27cb32f3..dc3c037d6313 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -79,6 +79,7 @@ module_param(device_id_scheme, bool, 0444);
 static int only_lcd = -1;
 module_param(only_lcd, int, 0444);
 
+static bool has_backlight;
 static int register_count;
 static DEFINE_MUTEX(register_count_mutex);
 static DEFINE_MUTEX(video_list_lock);
@@ -1230,6 +1231,9 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
 	acpi_video_device_bind(video, data);
 	acpi_video_device_find_cap(data);
 
+	if (data->cap._BCM && data->cap._BCL)
+		has_backlight = true;
+
 	mutex_lock(&video->device_list_lock);
 	list_add_tail(&data->entry, &video->video_device_list);
 	mutex_unlock(&video->device_list_lock);
@@ -2276,6 +2280,7 @@ void acpi_video_unregister(void)
 		cancel_delayed_work_sync(&video_bus_register_backlight_work);
 		acpi_bus_unregister_driver(&acpi_video_bus);
 		register_count = 0;
+		has_backlight = false;
 	}
 	mutex_unlock(&register_count_mutex);
 }
@@ -2294,13 +2299,7 @@ EXPORT_SYMBOL(acpi_video_register_backlight);
 
 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 &&
+	return has_backlight &&
 	       (report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS);
 }
 EXPORT_SYMBOL(acpi_video_handles_brightness_key_presses);
-- 
2.36.0

[PATCH 2/7] platform/x86: panasonic-laptop: de-obfuscate button codes

[Hans de Goede]

From: Stefan Seyfried <seife+kernel@b1-systems.com>

In the definition of panasonic_keymap[] the key codes are given in
decimal, later checks are done with hexadecimal values, which does
not help in understanding the code.
Additionally use two helper variables to shorten the code and make
the logic more obvious.

Fixes: ed83c9171829 ("platform/x86: panasonic-laptop: Resolve hotkey double trigger bug")
Signed-off-by: Stefan Seyfried <seife+kernel@b1-systems.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/panasonic-laptop.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index 37850d07987d..ca6137f4000f 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -762,6 +762,8 @@ static void acpi_pcc_generate_keyinput(struct pcc_acpi *pcc)
 	struct input_dev *hotk_input_dev = pcc->input_dev;
 	int rc;
 	unsigned long long result;
+	unsigned int key;
+	unsigned int updown;
 
 	rc = acpi_evaluate_integer(pcc->handle, METHOD_HKEY_QUERY,
 				   NULL, &result);
@@ -770,18 +772,22 @@ static void acpi_pcc_generate_keyinput(struct pcc_acpi *pcc)
 		return;
 	}
 
+	key = result & 0xf;
+	updown = result & 0x80; /* 0x80 == key down; 0x00 = key up */
+
 	/* hack: some firmware sends no key down for sleep / hibernate */
-	if ((result & 0xf) == 0x7 || (result & 0xf) == 0xa) {
-		if (result & 0x80)
+	if (key == 7 || key == 10) {
+		if (updown)
 			sleep_keydown_seen = 1;
 		if (!sleep_keydown_seen)
 			sparse_keymap_report_event(hotk_input_dev,
-					result & 0xf, 0x80, false);
+					key, 0x80, false);
 	}
 
-	if ((result & 0xf) == 0x7 || (result & 0xf) == 0x9 || (result & 0xf) == 0xa) {
+	/* for the magic values, see panasonic_keymap[] above */
+	if (key == 7 || key == 9 || key == 10) {
 		if (!sparse_keymap_report_event(hotk_input_dev,
-						result & 0xf, result & 0x80, false))
+						key, updown, false))
 			pr_err("Unknown hotkey event: 0x%04llx\n", result);
 	}
 }
-- 
2.36.0

[PATCH 3/7] platform/x86: panasonic-laptop: sort includes alphabetically

[Hans de Goede]

Sort includes alphabetically, small cleanup patch in preparation of
further changes.

Fixes: ed83c9171829 ("platform/x86: panasonic-laptop: Resolve hotkey double trigger bug")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/panasonic-laptop.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index ca6137f4000f..26e31ac09dc6 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -119,20 +119,19 @@
  *		- v0.1  start from toshiba_acpi driver written by John Belmonte
  */
 
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/init.h>
-#include <linux/types.h>
+#include <linux/acpi.h>
 #include <linux/backlight.h>
 #include <linux/ctype.h>
-#include <linux/seq_file.h>
-#include <linux/uaccess.h>
-#include <linux/slab.h>
-#include <linux/acpi.h>
+#include <linux/init.h>
 #include <linux/input.h>
 #include <linux/input/sparse-keymap.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/platform_device.h>
-
+#include <linux/seq_file.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
 
 MODULE_AUTHOR("Hiroshi Miura <miura@da-cha.org>");
 MODULE_AUTHOR("David Bronaugh <dbronaugh@linuxboxen.org>");
-- 
2.36.0

[PATCH 4/7] platform/x86: panasonic-laptop: revert "Resolve hotkey double trigger bug"

[Hans de Goede]

In hindsight blindly throwing away most of the key-press events is not
a good idea. So revert commit ed83c9171829 ("platform/x86:
panasonic-laptop: Resolve hotkey double trigger bug").

Fixes: ed83c9171829 ("platform/x86: panasonic-laptop: Resolve hotkey double trigger bug")
Reported-and-tested-by: Stefan Seyfried <seife+kernel@b1-systems.com>
Reported-and-tested-by: Kenneth Chan <kenneth.t.chan@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/panasonic-laptop.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index 26e31ac09dc6..2e6531dd15f9 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -783,12 +783,8 @@ static void acpi_pcc_generate_keyinput(struct pcc_acpi *pcc)
 					key, 0x80, false);
 	}
 
-	/* for the magic values, see panasonic_keymap[] above */
-	if (key == 7 || key == 9 || key == 10) {
-		if (!sparse_keymap_report_event(hotk_input_dev,
-						key, updown, false))
-			pr_err("Unknown hotkey event: 0x%04llx\n", result);
-	}
+	if (!sparse_keymap_report_event(hotk_input_dev, key, updown, false))
+		pr_err("Unknown hotkey event: 0x%04llx\n", result);
 }
 
 static void acpi_pcc_hotkey_notify(struct acpi_device *device, u32 event)
-- 
2.36.0

[PATCH 5/7] platform/x86: panasonic-laptop: don't report duplicate brightness key-presses

[Hans de Goede]

The brightness key-presses might also get reported by the ACPI video bus,
check for this and in this case don't report the presses to avoid reporting
2 presses for a single key-press.

Fixes: ed83c9171829 ("platform/x86: panasonic-laptop: Resolve hotkey double trigger bug")
Reported-and-tested-by: Stefan Seyfried <seife+kernel@b1-systems.com>
Reported-and-tested-by: Kenneth Chan <kenneth.t.chan@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/Kconfig            | 1 +
 drivers/platform/x86/panasonic-laptop.c | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 263082fc5407..156cb7e3f422 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -943,6 +943,7 @@ config PANASONIC_LAPTOP
 	tristate "Panasonic Laptop Extras"
 	depends on INPUT && ACPI
 	depends on BACKLIGHT_CLASS_DEVICE
+	depends on ACPI_VIDEO=n || ACPI_VIDEO
 	select INPUT_SPARSEKMAP
 	help
 	  This driver adds support for access to backlight control and hotkeys
diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index 2e6531dd15f9..d65e6c2372ca 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -132,6 +132,7 @@
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>
+#include <acpi/video.h>
 
 MODULE_AUTHOR("Hiroshi Miura <miura@da-cha.org>");
 MODULE_AUTHOR("David Bronaugh <dbronaugh@linuxboxen.org>");
@@ -783,6 +784,13 @@ static void acpi_pcc_generate_keyinput(struct pcc_acpi *pcc)
 					key, 0x80, false);
 	}
 
+	/*
+	 * Don't report brightness key-presses if they are also reported
+	 * by the ACPI video bus.
+	 */
+	if ((key == 1 || key == 2) && acpi_video_handles_brightness_key_presses())
+		return;
+
 	if (!sparse_keymap_report_event(hotk_input_dev, key, updown, false))
 		pr_err("Unknown hotkey event: 0x%04llx\n", result);
 }
-- 
2.36.0

[PATCH 6/7] platform/x86: panasonic-laptop: filter out duplicate volume up/down/mute keypresses

[Hans de Goede]

On some Panasonic models the volume up/down/mute keypresses get
reported both through the Panasonic ACPI HKEY interface as well as
through the atkbd device.

Filter out the atkbd scan-codes for these to avoid reporting presses
twice.

Note normally we would leave the filtering of these to userspace by mapping
the scan-codes to KEY_UNKNOWN through /lib/udev/hwdb.d/60-keyboard.hwdb.
However in this case that would cause regressions since we were filtering
the Panasonic ACPI HKEY events before, so filter these in the kernel.

Fixes: ed83c9171829 ("platform/x86: panasonic-laptop: Resolve hotkey double trigger bug")
Reported-and-tested-by: Stefan Seyfried <seife+kernel@b1-systems.com>
Reported-and-tested-by: Kenneth Chan <kenneth.t.chan@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/Kconfig            |  1 +
 drivers/platform/x86/panasonic-laptop.c | 41 +++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 156cb7e3f422..6e22ac916f7a 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -944,6 +944,7 @@ config PANASONIC_LAPTOP
 	depends on INPUT && ACPI
 	depends on BACKLIGHT_CLASS_DEVICE
 	depends on ACPI_VIDEO=n || ACPI_VIDEO
+	depends on SERIO_I8042 || SERIO_I8042 = n
 	select INPUT_SPARSEKMAP
 	help
 	  This driver adds support for access to backlight control and hotkeys
diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index d65e6c2372ca..615e39cbbbf1 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -122,6 +122,7 @@
 #include <linux/acpi.h>
 #include <linux/backlight.h>
 #include <linux/ctype.h>
+#include <linux/i8042.h>
 #include <linux/init.h>
 #include <linux/input.h>
 #include <linux/input/sparse-keymap.h>
@@ -129,6 +130,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/seq_file.h>
+#include <linux/serio.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>
@@ -241,6 +243,42 @@ struct pcc_acpi {
 	struct platform_device	*platform;
 };
 
+/*
+ * On some Panasonic models the volume up / down / mute keys send duplicate
+ * keypress events over the PS/2 kbd interface, filter these out.
+ */
+static bool panasonic_i8042_filter(unsigned char data, unsigned char str,
+				   struct serio *port)
+{
+	static bool extended;
+
+	if (str & I8042_STR_AUXDATA)
+		return false;
+
+	if (data == 0xe0) {
+		extended = true;
+		return true;
+	} else if (extended) {
+		extended = false;
+
+		switch (data & 0x7f) {
+		case 0x20: /* e0 20 / e0 a0, Volume Mute press / release */
+		case 0x2e: /* e0 2e / e0 ae, Volume Down press / release */
+		case 0x30: /* e0 30 / e0 b0, Volume Up press / release */
+			return true;
+		default:
+			/*
+			 * Report the previously filtered e0 before continuing
+			 * with the next non-filtered byte.
+			 */
+			serio_interrupt(port, 0xe0, 0);
+			return false;
+		}
+	}
+
+	return false;
+}
+
 /* method access functions */
 static int acpi_pcc_write_sset(struct pcc_acpi *pcc, int func, int val)
 {
@@ -1006,6 +1044,7 @@ static int acpi_pcc_hotkey_add(struct acpi_device *device)
 		pcc->platform = NULL;
 	}
 
+	i8042_install_filter(panasonic_i8042_filter);
 	return 0;
 
 out_platform:
@@ -1029,6 +1068,8 @@ static int acpi_pcc_hotkey_remove(struct acpi_device *device)
 	if (!device || !pcc)
 		return -EINVAL;
 
+	i8042_remove_filter(panasonic_i8042_filter);
+
 	if (pcc->platform) {
 		device_remove_file(&pcc->platform->dev, &dev_attr_cdpower);
 		platform_device_unregister(pcc->platform);
-- 
2.36.0

[PATCH 7/7] platform/x86: panasonic-laptop: Use acpi_video_get_backlight_type()

[Hans de Goede]

Use acpi_video_get_backlight_type() to determine if we should register
the panasonic specific backlight interface. To avoid registering this
on systems where the ACPI or GPU native backlight control methods
should be used instead.

Tested-by: Stefan Seyfried <seife+kernel@b1-systems.com>
Tested-by: Kenneth Chan <kenneth.t.chan@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/panasonic-laptop.c | 28 ++++++++++++++-----------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index 615e39cbbbf1..d9a095d2c0eb 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -998,19 +998,23 @@ static int acpi_pcc_hotkey_add(struct acpi_device *device)
 		pr_err("Couldn't retrieve BIOS data\n");
 		goto out_input;
 	}
-	/* initialize backlight */
-	memset(&props, 0, sizeof(struct backlight_properties));
-	props.type = BACKLIGHT_PLATFORM;
-	props.max_brightness = pcc->sinf[SINF_AC_MAX_BRIGHT];
-	pcc->backlight = backlight_device_register("panasonic", NULL, pcc,
-						   &pcc_backlight_ops, &props);
-	if (IS_ERR(pcc->backlight)) {
-		result = PTR_ERR(pcc->backlight);
-		goto out_input;
-	}
 
-	/* read the initial brightness setting from the hardware */
-	pcc->backlight->props.brightness = pcc->sinf[SINF_AC_CUR_BRIGHT];
+	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
+		/* initialize backlight */
+		memset(&props, 0, sizeof(struct backlight_properties));
+		props.type = BACKLIGHT_PLATFORM;
+		props.max_brightness = pcc->sinf[SINF_AC_MAX_BRIGHT];
+
+		pcc->backlight = backlight_device_register("panasonic", NULL, pcc,
+							   &pcc_backlight_ops, &props);
+		if (IS_ERR(pcc->backlight)) {
+			result = PTR_ERR(pcc->backlight);
+			goto out_input;
+		}
+
+		/* read the initial brightness setting from the hardware */
+		pcc->backlight->props.brightness = pcc->sinf[SINF_AC_CUR_BRIGHT];
+	}
 
 	/* Reset initial sticky key mode since the hardware register state is not consistent */
 	acpi_pcc_write_sset(pcc, SINF_STICKY_KEY, 0);
-- 
2.36.0

Re: [PATCH 0/7] ACPI: video / platform/x86: Fix Panasonic laptop missing keypresses

[Stefan Seyfried]

On 24.06.22 13:23, Hans de Goede wrote:
> Hi All,
> 
> Here is a series fixing the missing keypresses on some Panasonic Toughbook
> models. These missing keypresses are caused by
> commit ed83c9171829 ("platform/x86: panasonic-laptop: Resolve hotkey double
> trigger bug"), which made the panasonic-laptop driver unconditionally drop
> most WMI reported hotkeys.
> 
> This series reverts that commit and then adds some more selective filtering
> to still avoid the double key-presses on some models, while avoiding
> completely missing keypresses on others.
> 
> Rafael, these fixes rely on patch 1/7, which is a tweak to
> the acpi_video_handles_brightness_key_presses() helper. Without this
> tweak this series will cause a regression, so I would like to merge
> the entire series through the pdx86 tree, may I have your ack for this?
> 
> Regards,
> 
> Hans
> 
> 
> Hans de Goede (6):
>    ACPI: video: Change how we determine if brightness key-presses are
>      handled
>    platform/x86: panasonic-laptop: sort includes alphabetically
>    platform/x86: panasonic-laptop: revert "Resolve hotkey double trigger
>      bug"
>    platform/x86: panasonic-laptop: don't report duplicate brightness
>      key-presses
>    platform/x86: panasonic-laptop: filter out duplicate volume
>      up/down/mute keypresses
>    platform/x86: panasonic-laptop: Use acpi_video_get_backlight_type()
> 
> Stefan Seyfried (1):
>    platform/x86: panasonic-laptop: de-obfuscate button codes
> 
>   drivers/acpi/acpi_video.c               |  13 ++-
>   drivers/platform/x86/Kconfig            |   2 +
>   drivers/platform/x86/panasonic-laptop.c | 112 ++++++++++++++++++------
>   3 files changed, 91 insertions(+), 36 deletions(-)

The whole series works without any manual setup on my Toughbook CF-51:

Tested-by: Stefan Seyfried <seife+kernel@b1-systems.com>

Thanks again!

Stefan
-- 
Stefan Seyfried

"For a successful technology, reality must take precedence over
  public relations, for nature cannot be fooled." -- Richard Feynman

Re: [PATCH 1/7] ACPI: video: Change how we determine if brightness key-presses are handled

[Rafael J. Wysocki]

On Fri, Jun 24, 2022 at 1:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Some systems have an ACPI video bus but not ACPI video devices with
> backlight capability. On these devices brightness key-presses are
> (logically) not reported through the ACPI video bus.
>
> Change how acpi_video_handles_brightness_key_presses() determines if
> brightness key-presses are handled by the ACPI video driver to avoid
> vendor specific drivers/platform/x86 drivers filtering out their
> brightness key-presses even though they are the only ones reporting
> these presses.
>
> Fixes: ed83c9171829 ("platform/x86: panasonic-laptop: Resolve hotkey double trigger bug")
> Reported-and-tested-by: Stefan Seyfried <seife+kernel@b1-systems.com>
> Reported-and-tested-by: Kenneth Chan <kenneth.t.chan@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

and I'm assuming that you'll take this one along with the rest of the series.

> ---
>  drivers/acpi/acpi_video.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index b6ee27cb32f3..dc3c037d6313 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -79,6 +79,7 @@ module_param(device_id_scheme, bool, 0444);
>  static int only_lcd = -1;
>  module_param(only_lcd, int, 0444);
>
> +static bool has_backlight;
>  static int register_count;
>  static DEFINE_MUTEX(register_count_mutex);
>  static DEFINE_MUTEX(video_list_lock);
> @@ -1230,6 +1231,9 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
>         acpi_video_device_bind(video, data);
>         acpi_video_device_find_cap(data);
>
> +       if (data->cap._BCM && data->cap._BCL)
> +               has_backlight = true;
> +
>         mutex_lock(&video->device_list_lock);
>         list_add_tail(&data->entry, &video->video_device_list);
>         mutex_unlock(&video->device_list_lock);
> @@ -2276,6 +2280,7 @@ void acpi_video_unregister(void)
>                 cancel_delayed_work_sync(&video_bus_register_backlight_work);
>                 acpi_bus_unregister_driver(&acpi_video_bus);
>                 register_count = 0;
> +               has_backlight = false;
>         }
>         mutex_unlock(&register_count_mutex);
>  }
> @@ -2294,13 +2299,7 @@ EXPORT_SYMBOL(acpi_video_register_backlight);
>
>  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 &&
> +       return has_backlight &&
>                (report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS);
>  }
>  EXPORT_SYMBOL(acpi_video_handles_brightness_key_presses);
> --
> 2.36.0
>

Re: [PATCH 0/7] ACPI: video / platform/x86: Fix Panasonic laptop missing keypresses

[Kenneth Chan]

Hi Hans and Stefan,

On Tue, 21 Jun 2022 at 02:10, Stefan Seyfried
<stefan.seyfried@googlemail.com> wrote:
>
> Well, I looked into the acpi_video.c module and that one is to blame.
> By default, it assumes that both "OUTPUT_KEY_EVENTS" and
> "BRIGHTNESS_KEY_EVENTS" should be handled by this module.
> But on the CF-51, this does not happen. "Video Bus" does not generate
> any key events (I'm not sure about output, but plugging in a VGA monitor
> and enabling/disabling it with xrandr or tapping the "display" fn-f3
> hotkey does not get anything from "Video Bus" input device.
>

The "display" Fn-F3 hotkey doesn't generate any key events on mine
either. I have no external VGA monitors to test it anyway.

Apart from that, the patches work perfectly on my Let's Note CF-W5.
Cheers, Hans!

Tested-by: Kenneth Chan <kenneth.t.chan@gmail.com>


On Sat, 25 Jun 2022 at 03:49, Stefan Seyfried
<stefan.seyfried@googlemail.com> wrote:
>
> On 24.06.22 13:23, Hans de Goede wrote:
> > Hi All,
> >
> > Here is a series fixing the missing keypresses on some Panasonic Toughbook
> > models. These missing keypresses are caused by
> > commit ed83c9171829 ("platform/x86: panasonic-laptop: Resolve hotkey double
> > trigger bug"), which made the panasonic-laptop driver unconditionally drop
> > most WMI reported hotkeys.
> >
> > This series reverts that commit and then adds some more selective filtering
> > to still avoid the double key-presses on some models, while avoiding
> > completely missing keypresses on others.
> >
> > Rafael, these fixes rely on patch 1/7, which is a tweak to
> > the acpi_video_handles_brightness_key_presses() helper. Without this
> > tweak this series will cause a regression, so I would like to merge
> > the entire series through the pdx86 tree, may I have your ack for this?
> >
> > Regards,
> >
> > Hans
> >
> >
> > Hans de Goede (6):
> >    ACPI: video: Change how we determine if brightness key-presses are
> >      handled
> >    platform/x86: panasonic-laptop: sort includes alphabetically
> >    platform/x86: panasonic-laptop: revert "Resolve hotkey double trigger
> >      bug"
> >    platform/x86: panasonic-laptop: don't report duplicate brightness
> >      key-presses
> >    platform/x86: panasonic-laptop: filter out duplicate volume
> >      up/down/mute keypresses
> >    platform/x86: panasonic-laptop: Use acpi_video_get_backlight_type()
> >
> > Stefan Seyfried (1):
> >    platform/x86: panasonic-laptop: de-obfuscate button codes
> >
> >   drivers/acpi/acpi_video.c               |  13 ++-
> >   drivers/platform/x86/Kconfig            |   2 +
> >   drivers/platform/x86/panasonic-laptop.c | 112 ++++++++++++++++++------
> >   3 files changed, 91 insertions(+), 36 deletions(-)
>
> The whole series works without any manual setup on my Toughbook CF-51:
>
> Tested-by: Stefan Seyfried <seife+kernel@b1-systems.com>
>
> Thanks again!
>
> Stefan
> --
> Stefan Seyfried
>
> "For a successful technology, reality must take precedence over
>   public relations, for nature cannot be fooled." -- Richard Feynman

-- 
Kenneth

Re: [PATCH 1/7] ACPI: video: Change how we determine if brightness key-presses are handled

[Andy Shevchenko]

On Fri, Jun 24, 2022 at 01:23:34PM +0200, Hans de Goede wrote:
> Some systems have an ACPI video bus but not ACPI video devices with
> backlight capability. On these devices brightness key-presses are
> (logically) not reported through the ACPI video bus.
> 
> Change how acpi_video_handles_brightness_key_presses() determines if
> brightness key-presses are handled by the ACPI video driver to avoid
> vendor specific drivers/platform/x86 drivers filtering out their
> brightness key-presses even though they are the only ones reporting
> these presses.

...

> -	return have_video_busses &&
> +	return has_backlight &&
>  	       (report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS);

I would combine on one line.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/7] platform/x86: panasonic-laptop: de-obfuscate button codes

[Andy Shevchenko]

On Fri, Jun 24, 2022 at 01:23:35PM +0200, Hans de Goede wrote:
> From: Stefan Seyfried <seife+kernel@b1-systems.com>
> 
> In the definition of panasonic_keymap[] the key codes are given in
> decimal, later checks are done with hexadecimal values, which does
> not help in understanding the code.
> Additionally use two helper variables to shorten the code and make
> the logic more obvious.

(Note, all comments are up to you, I understand that this is a fix and maybe
 better to make code neat in a separate change)

...

>  	struct input_dev *hotk_input_dev = pcc->input_dev;
>  	int rc;
>  	unsigned long long result;
> +	unsigned int key;
> +	unsigned int updown;

Perhaps make them more like reversed xmas tree order?

...

>  			sparse_keymap_report_event(hotk_input_dev,
> -					result & 0xf, 0x80, false);
> +					key, 0x80, false);

Maybe move one or more parameters to the previous line?

...

>  		if (!sparse_keymap_report_event(hotk_input_dev,
> -						result & 0xf, result & 0x80, false))
> +						key, updown, false))
>  			pr_err("Unknown hotkey event: 0x%04llx\n", result);

Ditto.

Although I would even go for

	rc = sparse_...;
	if (!rc)
		pr_err(...);


pattern.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/7] platform/x86: panasonic-laptop: filter out duplicate volume up/down/mute keypresses

[Andy Shevchenko]

On Fri, Jun 24, 2022 at 01:23:39PM +0200, Hans de Goede wrote:
> On some Panasonic models the volume up/down/mute keypresses get
> reported both through the Panasonic ACPI HKEY interface as well as
> through the atkbd device.
> 
> Filter out the atkbd scan-codes for these to avoid reporting presses
> twice.
> 
> Note normally we would leave the filtering of these to userspace by mapping
> the scan-codes to KEY_UNKNOWN through /lib/udev/hwdb.d/60-keyboard.hwdb.
> However in this case that would cause regressions since we were filtering
> the Panasonic ACPI HKEY events before, so filter these in the kernel.

...

> +	if (data == 0xe0) {
> +		extended = true;
> +		return true;
> +	} else if (extended) {

'else' is not needed.

> +		extended = false;
> +
> +		switch (data & 0x7f) {
> +		case 0x20: /* e0 20 / e0 a0, Volume Mute press / release */
> +		case 0x2e: /* e0 2e / e0 ae, Volume Down press / release */
> +		case 0x30: /* e0 30 / e0 b0, Volume Up press / release */
> +			return true;
> +		default:
> +			/*
> +			 * Report the previously filtered e0 before continuing
> +			 * with the next non-filtered byte.
> +			 */
> +			serio_interrupt(port, 0xe0, 0);

> +			return false;

Could be 'break;' but...

> +		}
> +	}

> +	return false;

You can go with 'if (!extended)' above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/7] ACPI: video / platform/x86: Fix Panasonic laptop missing keypresses

[Andy Shevchenko]

On Fri, Jun 24, 2022 at 01:23:33PM +0200, Hans de Goede wrote:
> Hi All,
> 
> Here is a series fixing the missing keypresses on some Panasonic Toughbook
> models. These missing keypresses are caused by
> commit ed83c9171829 ("platform/x86: panasonic-laptop: Resolve hotkey double
> trigger bug"), which made the panasonic-laptop driver unconditionally drop
> most WMI reported hotkeys.
> 
> This series reverts that commit and then adds some more selective filtering
> to still avoid the double key-presses on some models, while avoiding
> completely missing keypresses on others.
> 
> Rafael, these fixes rely on patch 1/7, which is a tweak to
> the acpi_video_handles_brightness_key_presses() helper. Without this
> tweak this series will cause a regression, so I would like to merge
> the entire series through the pdx86 tree, may I have your ack for this?


I followed this series on Bugzilla,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

with or without my comments addressed (they are optional)

> Regards,
> 
> Hans
> 
> 
> Hans de Goede (6):
>   ACPI: video: Change how we determine if brightness key-presses are
>     handled
>   platform/x86: panasonic-laptop: sort includes alphabetically
>   platform/x86: panasonic-laptop: revert "Resolve hotkey double trigger
>     bug"
>   platform/x86: panasonic-laptop: don't report duplicate brightness
>     key-presses
>   platform/x86: panasonic-laptop: filter out duplicate volume
>     up/down/mute keypresses
>   platform/x86: panasonic-laptop: Use acpi_video_get_backlight_type()
> 
> Stefan Seyfried (1):
>   platform/x86: panasonic-laptop: de-obfuscate button codes
> 
>  drivers/acpi/acpi_video.c               |  13 ++-
>  drivers/platform/x86/Kconfig            |   2 +
>  drivers/platform/x86/panasonic-laptop.c | 112 ++++++++++++++++++------
>  3 files changed, 91 insertions(+), 36 deletions(-)
> 
> -- 
> 2.36.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/7] ACPI: video / platform/x86: Fix Panasonic laptop missing keypresses

[Hans de Goede]

Hi All,

On 6/24/22 13:23, Hans de Goede wrote:
> Hi All,
> 
> Here is a series fixing the missing keypresses on some Panasonic Toughbook
> models. These missing keypresses are caused by
> commit ed83c9171829 ("platform/x86: panasonic-laptop: Resolve hotkey double
> trigger bug"), which made the panasonic-laptop driver unconditionally drop
> most WMI reported hotkeys.
> 
> This series reverts that commit and then adds some more selective filtering
> to still avoid the double key-presses on some models, while avoiding
> completely missing keypresses on others.
> 
> Rafael, these fixes rely on patch 1/7, which is a tweak to
> the acpi_video_handles_brightness_key_presses() helper. Without this
> tweak this series will cause a regression, so I would like to merge
> the entire series through the pdx86 tree, may I have your ack for this?

I've added this series to my review-hans (soon to be for-next) branch now.
Patches 1-6 have also been added to the fixes branch.

Regards,

Hans

> 
> Regards,
> 
> Hans
> 
> 
> Hans de Goede (6):
>   ACPI: video: Change how we determine if brightness key-presses are
>     handled
>   platform/x86: panasonic-laptop: sort includes alphabetically
>   platform/x86: panasonic-laptop: revert "Resolve hotkey double trigger
>     bug"
>   platform/x86: panasonic-laptop: don't report duplicate brightness
>     key-presses
>   platform/x86: panasonic-laptop: filter out duplicate volume
>     up/down/mute keypresses
>   platform/x86: panasonic-laptop: Use acpi_video_get_backlight_type()
> 
> Stefan Seyfried (1):
>   platform/x86: panasonic-laptop: de-obfuscate button codes
> 
>  drivers/acpi/acpi_video.c               |  13 ++-
>  drivers/platform/x86/Kconfig            |   2 +
>  drivers/platform/x86/panasonic-laptop.c | 112 ++++++++++++++++++------
>  3 files changed, 91 insertions(+), 36 deletions(-)
> 

Re: [PATCH 0/7] ACPI: video / platform/x86: Fix Panasonic laptop missing keypresses

[Hans de Goede]

Hi,

On 6/26/22 08:58, Kenneth Chan wrote:
> Hi Hans and Stefan,
> 
> On Tue, 21 Jun 2022 at 02:10, Stefan Seyfried
> <stefan.seyfried@googlemail.com> wrote:
>>
>> Well, I looked into the acpi_video.c module and that one is to blame.
>> By default, it assumes that both "OUTPUT_KEY_EVENTS" and
>> "BRIGHTNESS_KEY_EVENTS" should be handled by this module.
>> But on the CF-51, this does not happen. "Video Bus" does not generate
>> any key events (I'm not sure about output, but plugging in a VGA monitor
>> and enabling/disabling it with xrandr or tapping the "display" fn-f3
>> hotkey does not get anything from "Video Bus" input device.
>>
> 
> The "display" Fn-F3 hotkey doesn't generate any key events on mine
> either.

Hmm, well that is unrelated to the double / missing other hotkeys,
but still worth investigating.

Normally that key just sends Windows-key (Super/Meta) + P, did you
check the atkbd for this being outputted?

You could install acpid and then run acpi_listen and see if any
events are reported for the key-press.

You could also try adding:

wmi.debug_event=Y to your kernel commandline and see if:

dmesg -w 

Shows any new output when the key is pressed ?


> I have no external VGA monitors to test it anyway.

Typically the key-press is handled independently from there
actually being an external monitor.

> Apart from that, the patches work perfectly on my Let's Note CF-W5.
> Cheers, Hans!
> 
> Tested-by: Kenneth Chan <kenneth.t.chan@gmail.com>

Thanks (to both of you).

Regards,

Hans


> 
> 
> On Sat, 25 Jun 2022 at 03:49, Stefan Seyfried
> <stefan.seyfried@googlemail.com> wrote:
>>
>> On 24.06.22 13:23, Hans de Goede wrote:
>>> Hi All,
>>>
>>> Here is a series fixing the missing keypresses on some Panasonic Toughbook
>>> models. These missing keypresses are caused by
>>> commit ed83c9171829 ("platform/x86: panasonic-laptop: Resolve hotkey double
>>> trigger bug"), which made the panasonic-laptop driver unconditionally drop
>>> most WMI reported hotkeys.
>>>
>>> This series reverts that commit and then adds some more selective filtering
>>> to still avoid the double key-presses on some models, while avoiding
>>> completely missing keypresses on others.
>>>
>>> Rafael, these fixes rely on patch 1/7, which is a tweak to
>>> the acpi_video_handles_brightness_key_presses() helper. Without this
>>> tweak this series will cause a regression, so I would like to merge
>>> the entire series through the pdx86 tree, may I have your ack for this?
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>> Hans de Goede (6):
>>>    ACPI: video: Change how we determine if brightness key-presses are
>>>      handled
>>>    platform/x86: panasonic-laptop: sort includes alphabetically
>>>    platform/x86: panasonic-laptop: revert "Resolve hotkey double trigger
>>>      bug"
>>>    platform/x86: panasonic-laptop: don't report duplicate brightness
>>>      key-presses
>>>    platform/x86: panasonic-laptop: filter out duplicate volume
>>>      up/down/mute keypresses
>>>    platform/x86: panasonic-laptop: Use acpi_video_get_backlight_type()
>>>
>>> Stefan Seyfried (1):
>>>    platform/x86: panasonic-laptop: de-obfuscate button codes
>>>
>>>   drivers/acpi/acpi_video.c               |  13 ++-
>>>   drivers/platform/x86/Kconfig            |   2 +
>>>   drivers/platform/x86/panasonic-laptop.c | 112 ++++++++++++++++++------
>>>   3 files changed, 91 insertions(+), 36 deletions(-)
>>
>> The whole series works without any manual setup on my Toughbook CF-51:
>>
>> Tested-by: Stefan Seyfried <seife+kernel@b1-systems.com>
>>
>> Thanks again!
>>
>> Stefan
>> --
>> Stefan Seyfried
>>
>> "For a successful technology, reality must take precedence over
>>   public relations, for nature cannot be fooled." -- Richard Feynman
>