Re: [PATCH 2/2] platform/x86: panasonic-laptop: allow to use all hotkeys
From: Hans de Goede
Date: Fri Jun 17 2022 - 07:07:55 EST
Hi,
On 6/17/22 09:51, Kenneth Chan wrote:
> On Thu, 16 Jun 2022 at 03:28, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> Kenneth, can you check with e.g. evemu-record or evtest
>> where the double events are coming from ? Obviously one of
>> the events is coming from the panasonic-laptop driver, but
>> where is the other event coming from. Is it coming from the
>> atkbd driver; or ... ? Maybe from the acpi-video driver
>> for the brightness keys ?
>>
>
> Here's the evtest results:
>
> acpi-video driver generates KEY_BRIGHTNESSDOWN, KEY_BRIGHTNESSUP
> atkbd generates KEY_MUTE, KEY_VOLUMEUP, KEY_VOLUMEDOWN
>
> Hotkey_input=Y (i.e. before patch ed83c9171829)
> panasonic-laptop driver generates all keys, i.e. KEY_SLEEP,
> KEY_BATTERY, KEY_SUSPEND plus all the above keys
>
> hotkey_input=N
> panasonic-laptop driver generates KEY_SLEEP, KEY_BATTERY and KEY_SUSPEND
>
> So the duplicated brightness and volume key events come from the atkbd
> and acpi-video drivers on my CF-W5. I haven't looked at the other
> platform drivers. I'm wondering if they honour atkbd and acpi-driver
> events in a case like this, or just report everything.
Thank you for providing this info. Can you please give
the attached patch series a try, this includes Stefan's 1/2 patch
and replaces Stefan's 2/2 patch.
This will hopefully fix the double key-presses for you, while
also keeping everything working for Stefan without requiring
a module option or DMI quirks.
Stefan can you also give this series a try please?
###
Looking at this has also brought up an unrelated backlight question:
Kenneth, since you have acpi-video reporting keypresses you will
likely also have an acpi_video (or perhaps a native intel) backlight
under /sys/class/backlight and I noticed that panasonic-laptop
uncondirionally registers its backlight so you may very well end
up with 2 backlight controls under /sys/class/backlight, which
we generally try to avoid (so that userspace does not have to
guess which one to use).
Can you do:
ls /sys/class/backlight
and let me know the output?
Also if there are 2 backlights there then please do:
cat /sys/class/backlight/<name>/max_brightness
to find out the range (0-value)
and then try if they both work by doing:
echo $number > /sys/class/backlight/<name>/brightness
with different $number values in the range and see
if this actually changes the brightness.
While we are at it, Stefan can you do the same please?
Regards,
Hans
>
> Attached is the dmidecode of my CF-W5, just to be verbose.
>
>> Hence the module parameter so that the two known users of this module (Kenneth and me) can adjust this to their needs.
>>
>> Now about the DMI match: I can do that.
>> But let's face it: the panasonic laptops are pretty rare in the wild, so even if I'm "whitelisting" the CF-51, then probably other models will need the same treatment and we have no real way of finding out which ones, unless people complain.
>> So even if I add the DMI match (which is a good idea anyhow because then "my" model will work out of the box, while right now I need to add a module parameter or switch it on later), I'd still vote for having a possibility for overriding the DMI results.
>
> In an ideal world, more panasonic-laptop users will send us their
> DSDT. I think the most uptodate model has a MAT0035 device_id (it
> increments for each generation) while our driver is at the very
> outdated MAT0021. But before it happens, I agree with Stefan on that
> point.
> From 64fc644ca7c7e8830b0a86f9cfa39069c59eeeca Mon Sep 17 00:00:00 2001
From: Stefan Seyfried <seife+kernel@xxxxxxxxxxxxxx>
Date: Sun, 12 Jun 2022 11:05:06 +0200
Subject: [PATCH 1/5] platform/x86: panasonic-laptop: de-obfuscate button codes
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.
Signed-off-by: Stefan Seyfried <seife+kernel@xxxxxxxxxxxxxx>
Link: https://lore.kernel.org/r/20220612090507.20648-2-stefan.seyfried@xxxxxxxxxxxxxx
Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
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
From 029166f13632f4774eafb09596fc87d2a29a7429 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Fri, 17 Jun 2022 12:39:51 +0200
Subject: [PATCH 2/5] platform/x86: panasonic-laptop: Sort includes
alphabetically
Sort includes alphabetically, small cleanup patch in preparation of
further changes.
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
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@xxxxxxxxxx>");
MODULE_AUTHOR("David Bronaugh <dbronaugh@xxxxxxxxxxxxxx>");
--
2.36.0
From dc7e2e718871715e3359af9e3b6e794b83c4b383 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Fri, 17 Jun 2022 11:36:43 +0200
Subject: [PATCH 3/5] platform/x86: panasonic-laptop: revert "Resolve hotkey
double trigger bug"
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-by: Stefan Seyfried <seife+kernel@xxxxxxxxxxxxxx>
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
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
From eba597920c5b64a616358526bbcf2d389f8cef9c Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Fri, 17 Jun 2022 11:49:58 +0200
Subject: [PATCH 4/5] platform/x86: panasonic-laptop: Don't report duplicate
brightness key-presses
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-by: Kenneth Chan <kenneth.t.chan@xxxxxxxxx>
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
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 0ac77d0553da..a6de14c3aca1 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -942,6 +942,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@xxxxxxxxxx>");
MODULE_AUTHOR("David Bronaugh <dbronaugh@xxxxxxxxxxxxxx>");
@@ -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
From d057b76da8193c8410284100f9efc32e0006ad09 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Fri, 17 Jun 2022 12:35:24 +0200
Subject: [PATCH 5/5] platform/x86: panasonic-laptop: filter out duplicate
volume up/down/mute keypresses
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-by: Kenneth Chan <kenneth.t.chan@xxxxxxxxx>
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
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 a6de14c3aca1..0f723c34a637 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -943,6 +943,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..a4c82b3a81cf 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 0x21: /* e0 21 / e0 a1, Volume Down press / release */
+ case 0x23: /* e0 23 / e0 a3, Volume Mute press / release */
+ case 0x32: /* e0 32 / e0 b2, 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