[PATCH v2 0/4] dell-wmi: Changes in WMI event code handling

[PATCH v2 0/4] dell-wmi: Changes in WMI event code handling

[Pali Rohár]

First patch describe problem about 0xe045 code. Second and third are just
cosmetic and last rework code which processing WMI events. It should be
properly tested on more Dell machines, to check that everything is still
working correctly.

Changes since v1:
* Fixed comments
* Fixed memory leak
* Added Tested-By lines
* Added event 0xe06e

Pali Rohár (4):
  dell-wmi: Ignore WMI event code 0xe045
  dell-wmi: Sort WMI event codes and update comments
  dell-wmi: Add information about other WMI event codes
  dell-wmi: Rework code for generating sparse keymap and processing WMI
    events

 drivers/platform/x86/dell-wmi.c |  290 +++++++++++++++++++++++----------------
 1 file changed, 169 insertions(+), 121 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/4] dell-wmi: Ignore WMI event code 0xe045

[Pali Rohár]

>From Dell we know that WMI event code 0xe045 is for Num Lock key, but it is
unclear due to message in commit 0b3f6109f0c9 ("dell-wmi: new driver for
hotkey control").

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Tested-by: Michał Kępień <kernel@kempniu.pl>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Link: https://lkml.org/lkml/2015/7/7/830
---
 drivers/platform/x86/dell-wmi.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 15c6f11..4d23c91 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -88,7 +88,6 @@ static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
 static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	{ KE_IGNORE, 0x003a, { KEY_CAPSLOCK } },
 
-	{ KE_KEY, 0xe045, { KEY_PROG1 } },
 	{ KE_KEY, 0xe009, { KEY_EJECTCD } },
 
 	/* These also contain the brightness level at offset 6 */
@@ -130,7 +129,19 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	{ KE_IGNORE, 0xe033, { KEY_KBDILLUMUP } },
 	{ KE_IGNORE, 0xe034, { KEY_KBDILLUMDOWN } },
 	{ KE_IGNORE, 0xe03a, { KEY_CAPSLOCK } },
+
+	/*
+	 * This entry is very suspicious!
+	 * Originally Matthew Garrett created this dell-wmi driver specially for
+	 * "button with a picture of a battery" which has event code 0xe045.
+	 * Later Mario Limonciello from Dell told us that event code 0xe045 is
+	 * reported by Num Lock and should be ignored because key is send also
+	 * by keyboard controller.
+	 * So for now we will ignore this event to prevent potential double
+	 * Num Lock key press.
+	 */
 	{ KE_IGNORE, 0xe045, { KEY_NUMLOCK } },
+
 	{ KE_IGNORE, 0xe046, { KEY_SCROLLLOCK } },
 	{ KE_IGNORE, 0xe0f7, { KEY_MUTE } },
 	{ KE_IGNORE, 0xe0f8, { KEY_VOLUMEDOWN } },
-- 
1.7.9.5

[PATCH v2 1/4] dell-wmi: Ignore WMI event code 0xe045

[Pali Rohár]

From Dell we know that WMI event code 0xe045 is for Num Lock key, but it is
unclear due to message in commit 0b3f6109f0c9 ("dell-wmi: new driver for
hotkey control").

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Tested-by: Michał Kępień <kernel@kempniu.pl>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Link: https://lkml.org/lkml/2015/7/7/830
---
 drivers/platform/x86/dell-wmi.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 15c6f11..4d23c91 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -88,7 +88,6 @@ static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
 static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	{ KE_IGNORE, 0x003a, { KEY_CAPSLOCK } },
 
-	{ KE_KEY, 0xe045, { KEY_PROG1 } },
 	{ KE_KEY, 0xe009, { KEY_EJECTCD } },
 
 	/* These also contain the brightness level at offset 6 */
@@ -130,7 +129,19 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	{ KE_IGNORE, 0xe033, { KEY_KBDILLUMUP } },
 	{ KE_IGNORE, 0xe034, { KEY_KBDILLUMDOWN } },
 	{ KE_IGNORE, 0xe03a, { KEY_CAPSLOCK } },
+
+	/*
+	 * This entry is very suspicious!
+	 * Originally Matthew Garrett created this dell-wmi driver specially for
+	 * "button with a picture of a battery" which has event code 0xe045.
+	 * Later Mario Limonciello from Dell told us that event code 0xe045 is
+	 * reported by Num Lock and should be ignored because key is send also
+	 * by keyboard controller.
+	 * So for now we will ignore this event to prevent potential double
+	 * Num Lock key press.
+	 */
 	{ KE_IGNORE, 0xe045, { KEY_NUMLOCK } },
+
 	{ KE_IGNORE, 0xe046, { KEY_SCROLLLOCK } },
 	{ KE_IGNORE, 0xe0f7, { KEY_MUTE } },
 	{ KE_IGNORE, 0xe0f8, { KEY_VOLUMEDOWN } },
-- 
1.7.9.5

[PATCH v2 2/4] dell-wmi: Sort WMI event codes and update comments

[Pali Rohár]

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Tested-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/dell-wmi.c |   29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 4d23c91..6750604 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -88,29 +88,29 @@ static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
 static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	{ KE_IGNORE, 0x003a, { KEY_CAPSLOCK } },
 
-	{ KE_KEY, 0xe009, { KEY_EJECTCD } },
-
-	/* These also contain the brightness level at offset 6 */
-	{ KE_KEY, 0xe006, { KEY_BRIGHTNESSUP } },
-	{ KE_KEY, 0xe005, { KEY_BRIGHTNESSDOWN } },
+	/* Key code is followed by brightness level */
+	{ KE_KEY,    0xe005, { KEY_BRIGHTNESSDOWN } },
+	{ KE_KEY,    0xe006, { KEY_BRIGHTNESSUP } },
 
 	/* Battery health status button */
-	{ KE_KEY, 0xe007, { KEY_BATTERY } },
+	{ KE_KEY,    0xe007, { KEY_BATTERY } },
 
-	/* Radio devices state change */
+	/* Radio devices state change, key code is followed by additional information */
 	{ KE_IGNORE, 0xe008, { KEY_RFKILL } },
 
-	/* The next device is at offset 6, the active devices are at
-	   offset 8 and the attached devices at offset 10 */
-	{ KE_KEY, 0xe00b, { KEY_SWITCHVIDEOMODE } },
+	{ KE_KEY,    0xe009, { KEY_EJECTCD } },
+
+	/* Key code is followed by: next device, active devices, attached devices */
+	{ KE_KEY,    0xe00b, { KEY_SWITCHVIDEOMODE } },
 
+	/* Key code is followed by keyboard illumination level */
 	{ KE_IGNORE, 0xe00c, { KEY_KBDILLUMTOGGLE } },
 
 	/* BIOS error detected */
 	{ KE_IGNORE, 0xe00d, { KEY_RESERVED } },
 
 	/* Wifi Catcher */
-	{ KE_KEY, 0xe011, {KEY_PROG2 } },
+	{ KE_KEY,    0xe011, { KEY_PROG2 } },
 
 	/* Ambient light sensor toggle */
 	{ KE_IGNORE, 0xe013, { KEY_RESERVED } },
@@ -118,12 +118,14 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	{ KE_IGNORE, 0xe020, { KEY_MUTE } },
 
 	/* Dell Instant Launch key */
-	{ KE_KEY, 0xe025, { KEY_PROG4 } },
-	{ KE_KEY, 0xe029, { KEY_PROG4 } },
+	{ KE_KEY,    0xe025, { KEY_PROG4 } },
 
 	/* Audio panel key */
 	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
 
+	/* Dell Instant Launch key */
+	{ KE_KEY,    0xe029, { KEY_PROG4 } },
+
 	{ KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
 	{ KE_IGNORE, 0xe030, { KEY_VOLUMEUP } },
 	{ KE_IGNORE, 0xe033, { KEY_KBDILLUMUP } },
@@ -146,6 +148,7 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	{ KE_IGNORE, 0xe0f7, { KEY_MUTE } },
 	{ KE_IGNORE, 0xe0f8, { KEY_VOLUMEDOWN } },
 	{ KE_IGNORE, 0xe0f9, { KEY_VOLUMEUP } },
+
 	{ KE_END, 0 }
 };
 
-- 
1.7.9.5

[PATCH v2 3/4] dell-wmi: Add information about other WMI event codes

[Pali Rohár]

ACPI DSDT tables have defined other WMI codes, but does not contain any
description when those codes are emitted. Some other codes can be found in
logs on internet. In this patch are all which I saw, but lot of them are
not tested properly (e.g. for duplicate events with AT keyboard). Now we
have all WMI event codes at one place and in future after proper testing
those codes can be correctly enabled or disabled...

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Tested-by: Michał Kępień <kernel@kempniu.pl>
Tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
---
 drivers/platform/x86/dell-wmi.c |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 6750604..a406f01 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -109,6 +109,9 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	/* BIOS error detected */
 	{ KE_IGNORE, 0xe00d, { KEY_RESERVED } },
 
+	/* Unknown, defined in ACPI DSDT */
+	/* { KE_IGNORE, 0xe00e, { KEY_RESERVED } }, */
+
 	/* Wifi Catcher */
 	{ KE_KEY,    0xe011, { KEY_PROG2 } },
 
@@ -117,21 +120,45 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 
 	{ KE_IGNORE, 0xe020, { KEY_MUTE } },
 
+	/* Unknown, defined in ACPI DSDT */
+	/* { KE_IGNORE, 0xe023, { KEY_RESERVED } }, */
+
+	/* Untested, Dell Instant Launch key on Inspiron 7520 */
+	/* { KE_IGNORE, 0xe024, { KEY_RESERVED } }, */
+
 	/* Dell Instant Launch key */
 	{ KE_KEY,    0xe025, { KEY_PROG4 } },
 
 	/* Audio panel key */
 	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
 
+	/* Untested, Multimedia key on Dell Vostro 3560 */
+	/* { KE_IGNORE, 0xe028, { KEY_RESERVED } }, */
+
 	/* Dell Instant Launch key */
 	{ KE_KEY,    0xe029, { KEY_PROG4 } },
 
+	/* Untested, Windows Mobility Center button on Inspiron 7520 */
+	/* { KE_IGNORE, 0xe02a, { KEY_RESERVED } }, */
+
+	/* Unknown, defined in ACPI DSDT */
+	/* { KE_IGNORE, 0xe02b, { KEY_RESERVED } }, */
+
+	/* Untested, Dell Audio With Preset Switch button on Inspiron 7520 */
+	/* { KE_IGNORE, 0xe02c, { KEY_RESERVED } }, */
+
 	{ KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
 	{ KE_IGNORE, 0xe030, { KEY_VOLUMEUP } },
 	{ KE_IGNORE, 0xe033, { KEY_KBDILLUMUP } },
 	{ KE_IGNORE, 0xe034, { KEY_KBDILLUMDOWN } },
 	{ KE_IGNORE, 0xe03a, { KEY_CAPSLOCK } },
 
+	/* NIC Link is Up */
+	{ KE_IGNORE, 0xe043, { KEY_RESERVED } },
+
+	/* NIC Link is Down */
+	{ KE_IGNORE, 0xe044, { KEY_RESERVED } },
+
 	/*
 	 * This entry is very suspicious!
 	 * Originally Matthew Garrett created this dell-wmi driver specially for
@@ -144,7 +171,15 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	 */
 	{ KE_IGNORE, 0xe045, { KEY_NUMLOCK } },
 
+	/* Scroll lock and also going to tablet mode on portable devices */
 	{ KE_IGNORE, 0xe046, { KEY_SCROLLLOCK } },
+
+	/* Untested, going from tablet mode on portable devices */
+	/* { KE_IGNORE, 0xe047, { KEY_RESERVED } }, */
+
+	/* Dell Support Center key */
+	{ KE_IGNORE, 0xe06e, { KEY_RESERVED } },
+
 	{ KE_IGNORE, 0xe0f7, { KEY_MUTE } },
 	{ KE_IGNORE, 0xe0f8, { KEY_VOLUMEDOWN } },
 	{ KE_IGNORE, 0xe0f9, { KEY_VOLUMEUP } },
-- 
1.7.9.5

[PATCH v2 4/4] dell-wmi: Rework code for generating sparse keymap and processing WMI events

[Pali Rohár]

This patch unify procedure for generating sparse keymap and unify also big
switch code for processing WMI events of different types. After this patch
dell-wmi driver does not differ between "old" and "new" hotkey type.

It construct sparse keymap table with all WMI codes. It is because on some
laptops (e.g. Dell Latitude E6440) ACPI/firmware send both event types (old
and new).

Each WMI code in sparse keymap table is prefixed by 16bit event type, so it
does not change functionality on laptops with "old" hotkey support (those
without scancodes in DMI).

This allow us to distinguish between same WMI codes with different types in
sparse keymap. Thanks to this WMI events of type 0x0011 were moved from big
switch into sparse keymap table too.

This patch also fixes possible bug in parsing WMI event buffer introduced
in commit 5ea2559726b7 ("dell-wmi: Add support for new Dell systems"). That
commit changed buffer type from int* to u16* without fixing code. More at:
http://lkml.iu.edu/hypermail/linux/kernel/1507.0/01950.html

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Tested-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/dell-wmi.c |  215 +++++++++++++++++++--------------------
 1 file changed, 107 insertions(+), 108 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index a406f01..fe831f3 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -80,12 +80,13 @@ static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
 };
 
 /*
+ * Keymap for WMI events of type 0x0000
+ *
  * Certain keys are flagged as KE_IGNORE. All of these are either
  * notifications (rather than requests for change) or are also sent
  * via the keyboard controller so should not be sent again.
  */
-
-static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
+static const struct key_entry dell_wmi_keymap_type_0000[] __initconst = {
 	{ KE_IGNORE, 0x003a, { KEY_CAPSLOCK } },
 
 	/* Key code is followed by brightness level */
@@ -183,12 +184,8 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	{ KE_IGNORE, 0xe0f7, { KEY_MUTE } },
 	{ KE_IGNORE, 0xe0f8, { KEY_VOLUMEDOWN } },
 	{ KE_IGNORE, 0xe0f9, { KEY_VOLUMEUP } },
-
-	{ KE_END, 0 }
 };
 
-static bool dell_new_hk_type;
-
 struct dell_bios_keymap_entry {
 	u16 scancode;
 	u16 keycode;
@@ -202,6 +199,7 @@ struct dell_bios_hotkey_table {
 
 struct dell_dmi_results {
 	int err;
+	int keymap_size;
 	struct key_entry *keymap;
 };
 
@@ -250,10 +248,12 @@ static const u16 bios_to_linux_keycode[256] __initconst = {
 };
 
 /*
+ * Keymap for WMI events of type 0x0010
+ *
  * These are applied if the 0xB2 DMI hotkey table is present and doesn't
  * override them.
  */
-static const struct key_entry dell_wmi_extra_keymap[] __initconst = {
+static const struct key_entry dell_wmi_keymap_type_0010[] __initconst = {
 	/* Fn-lock */
 	{ KE_IGNORE, 0x151, { KEY_RESERVED } },
 
@@ -273,21 +273,39 @@ static const struct key_entry dell_wmi_extra_keymap[] __initconst = {
 	{ KE_IGNORE, 0x155, { KEY_RESERVED } },
 };
 
+/*
+ * Keymap for WMI events of type 0x0011
+ */
+static const struct key_entry dell_wmi_keymap_type_0011[] __initconst = {
+	/* Battery unplugged */
+	{ KE_IGNORE, 0xfff0, { KEY_RESERVED } },
+
+	/* Battery inserted */
+	{ KE_IGNORE, 0xfff1, { KEY_RESERVED } },
+
+	/* Keyboard backlight level changed */
+	{ KE_IGNORE, 0x01e1, { KEY_RESERVED } },
+	{ KE_IGNORE, 0x02ea, { KEY_RESERVED } },
+	{ KE_IGNORE, 0x02eb, { KEY_RESERVED } },
+	{ KE_IGNORE, 0x02ec, { KEY_RESERVED } },
+	{ KE_IGNORE, 0x02f6, { KEY_RESERVED } },
+};
+
 static struct input_dev *dell_wmi_input_dev;
 
-static void dell_wmi_process_key(int reported_key)
+static void dell_wmi_process_key(int type, int code)
 {
 	const struct key_entry *key;
 
 	key = sparse_keymap_entry_from_scancode(dell_wmi_input_dev,
-						reported_key);
+						(type << 16) | code);
 	if (!key) {
-		pr_info("Unknown key with scancode 0x%x pressed\n",
-			reported_key);
+		pr_info("Unknown key with type 0x%04x and code 0x%04x pressed\n",
+			type, code);
 		return;
 	}
 
-	pr_debug("Key %x pressed\n", reported_key);
+	pr_debug("Key with type 0x%04x and code 0x%04x pressed\n", type, code);
 
 	/* Don't report brightness notifications that will also come via ACPI */
 	if ((key->keycode == KEY_BRIGHTNESSUP ||
@@ -295,7 +313,7 @@ static void dell_wmi_process_key(int reported_key)
 	    acpi_video_handles_brightness_key_presses())
 		return;
 
-	if (reported_key == 0xe025 && !wmi_requires_smbios_request)
+	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
 		return;
 
 	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
@@ -333,18 +351,6 @@ static void dell_wmi_notify(u32 value, void *context)
 
 	buffer_entry = (u16 *)obj->buffer.pointer;
 	buffer_size = obj->buffer.length/2;
-
-	if (!dell_new_hk_type) {
-		if (buffer_size >= 3 && buffer_entry[1] == 0x0)
-			dell_wmi_process_key(buffer_entry[2]);
-		else if (buffer_size >= 2)
-			dell_wmi_process_key(buffer_entry[1]);
-		else
-			pr_info("Received unknown WMI event\n");
-		kfree(obj);
-		return;
-	}
-
 	buffer_end = buffer_entry + buffer_size;
 
 	/*
@@ -379,59 +385,21 @@ static void dell_wmi_notify(u32 value, void *context)
 		pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
 
 		switch (buffer_entry[1]) {
-		case 0x00:
-			for (i = 2; i < len; ++i) {
-				switch (buffer_entry[i]) {
-				case 0xe043:
-					/* NIC Link is Up */
-					pr_debug("NIC Link is Up\n");
-					break;
-				case 0xe044:
-					/* NIC Link is Down */
-					pr_debug("NIC Link is Down\n");
-					break;
-				case 0xe045:
-					/* Unknown event but defined in DSDT */
-				default:
-					/* Unknown event */
-					pr_info("Unknown WMI event type 0x00: "
-						"0x%x\n", (int)buffer_entry[i]);
-					break;
-				}
-			}
+		case 0x0000:
+			/* One key pressed or event occurred */
+			if (len > 2)
+				dell_wmi_process_key(0x0000, buffer_entry[2]);
+			/* Other entries in buffer could contain additional information */
 			break;
-		case 0x10:
-			/* Keys pressed */
+		case 0x0010:
+			/* Sequence of keys pressed */
 			for (i = 2; i < len; ++i)
-				dell_wmi_process_key(buffer_entry[i]);
+				dell_wmi_process_key(0x0010, buffer_entry[i]);
 			break;
-		case 0x11:
-			for (i = 2; i < len; ++i) {
-				switch (buffer_entry[i]) {
-				case 0xfff0:
-					/* Battery unplugged */
-					pr_debug("Battery unplugged\n");
-					break;
-				case 0xfff1:
-					/* Battery inserted */
-					pr_debug("Battery inserted\n");
-					break;
-				case 0x01e1:
-				case 0x02ea:
-				case 0x02eb:
-				case 0x02ec:
-				case 0x02f6:
-					/* Keyboard backlight level changed */
-					pr_debug("Keyboard backlight level "
-						 "changed\n");
-					break;
-				default:
-					/* Unknown event */
-					pr_info("Unknown WMI event type 0x11: "
-						"0x%x\n", (int)buffer_entry[i]);
-					break;
-				}
-			}
+		case 0x0011:
+			/* Sequence of events occurred */
+			for (i = 2; i < len; ++i)
+				dell_wmi_process_key(0x0011, buffer_entry[i]);
 			break;
 		default:
 			/* Unknown event */
@@ -459,7 +427,6 @@ static bool have_scancode(u32 scancode, const struct key_entry *keymap, int len)
 }
 
 static void __init handle_dmi_entry(const struct dmi_header *dm,
-
 				    void *opaque)
 
 {
@@ -467,7 +434,6 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
 	struct dell_bios_hotkey_table *table;
 	int hotkey_num, i, pos = 0;
 	struct key_entry *keymap;
-	int num_bios_keys;
 
 	if (results->err || results->keymap)
 		return;		/* We already found the hotkey table. */
@@ -491,8 +457,7 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
 		return;
 	}
 
-	keymap = kcalloc(hotkey_num + ARRAY_SIZE(dell_wmi_extra_keymap) + 1,
-			 sizeof(struct key_entry), GFP_KERNEL);
+	keymap = kcalloc(hotkey_num, sizeof(struct key_entry), GFP_KERNEL);
 	if (!keymap) {
 		results->err = -ENOMEM;
 		return;
@@ -529,31 +494,15 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
 		pos++;
 	}
 
-	num_bios_keys = pos;
-
-	for (i = 0; i < ARRAY_SIZE(dell_wmi_extra_keymap); i++) {
-		const struct key_entry *entry = &dell_wmi_extra_keymap[i];
-
-		/*
-		 * Check if we've already found this scancode.  This takes
-		 * quadratic time, but it doesn't matter unless the list
-		 * of extra keys gets very long.
-		 */
-		if (!have_scancode(entry->code, keymap, num_bios_keys)) {
-			keymap[pos] = *entry;
-			pos++;
-		}
-	}
-
-	keymap[pos].type = KE_END;
-
 	results->keymap = keymap;
+	results->keymap_size = pos;
 }
 
 static int __init dell_wmi_input_setup(void)
 {
 	struct dell_dmi_results dmi_results = {};
-	int err;
+	struct key_entry *keymap;
+	int err, i, pos = 0;
 
 	dell_wmi_input_dev = input_allocate_device();
 	if (!dell_wmi_input_dev)
@@ -577,21 +526,71 @@ static int __init dell_wmi_input_setup(void)
 		goto err_free_dev;
 	}
 
-	if (dmi_results.keymap) {
-		dell_new_hk_type = true;
+	keymap = kcalloc(dmi_results.keymap_size +
+			 ARRAY_SIZE(dell_wmi_keymap_type_0000) +
+			 ARRAY_SIZE(dell_wmi_keymap_type_0010) +
+			 ARRAY_SIZE(dell_wmi_keymap_type_0011) +
+			 1,
+			 sizeof(struct key_entry), GFP_KERNEL);
+	if (!keymap) {
+		kfree(dmi_results.keymap);
+		err = -ENOMEM;
+		goto err_free_dev;
+	}
 
-		err = sparse_keymap_setup(dell_wmi_input_dev,
-					  dmi_results.keymap, NULL);
+	/* Append table with events of type 0x0010 which comes from DMI */
+	for (i = 0; i < dmi_results.keymap_size; i++) {
+		keymap[pos] = dmi_results.keymap[i];
+		keymap[pos].code |= (0x0010 << 16);
+		pos++;
+	}
+
+	kfree(dmi_results.keymap);
+
+	/* Append table with extra events of type 0x0010 which are not in DMI */
+	for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0010); i++) {
+		const struct key_entry *entry = &dell_wmi_keymap_type_0010[i];
 
 		/*
-		 * Sparse keymap library makes a copy of keymap so we
-		 * don't need the original one that was allocated.
+		 * Check if we've already found this scancode.  This takes
+		 * quadratic time, but it doesn't matter unless the list
+		 * of extra keys gets very long.
 		 */
-		kfree(dmi_results.keymap);
-	} else {
-		err = sparse_keymap_setup(dell_wmi_input_dev,
-					  dell_wmi_legacy_keymap, NULL);
+		if (dmi_results.keymap_size &&
+		    have_scancode(entry->code | (0x0010 << 16),
+				  keymap, dmi_results.keymap_size)
+		   )
+			continue;
+
+		keymap[pos] = *entry;
+		keymap[pos].code |= (0x0010 << 16);
+		pos++;
+	}
+
+	/* Append table with events of type 0x0011 */
+	for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0011); i++) {
+		keymap[pos] = dell_wmi_keymap_type_0011[i];
+		keymap[pos].code |= (0x0011 << 16);
+		pos++;
 	}
+
+	/*
+	 * Now append also table with "legacy" events of type 0x0000. Some of
+	 * them are reported also on laptops which have scancodes in DMI.
+	 */
+	for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0000); i++) {
+		keymap[pos] = dell_wmi_keymap_type_0000[i];
+		pos++;
+	}
+
+	keymap[pos].type = KE_END;
+
+	err = sparse_keymap_setup(dell_wmi_input_dev, keymap, NULL);
+	/*
+	 * Sparse keymap library makes a copy of keymap so we don't need the
+	 * original one that was allocated.
+	 */
+	kfree(keymap);
 	if (err)
 		goto err_free_dev;
 
-- 
1.7.9.5

Re: [PATCH v2 4/4] dell-wmi: Rework code for generating sparse keymap and processing WMI events

[Darren Hart]

On Wed, Jun 08, 2016 at 01:32:27AM +0200, Pali Rohár wrote:
> This patch unify procedure for generating sparse keymap and unify also big
> switch code for processing WMI events of different types. After this patch
> dell-wmi driver does not differ between "old" and "new" hotkey type.
> 
> It construct sparse keymap table with all WMI codes. It is because on some
> laptops (e.g. Dell Latitude E6440) ACPI/firmware send both event types (old
> and new).
> 
> Each WMI code in sparse keymap table is prefixed by 16bit event type, so it
> does not change functionality on laptops with "old" hotkey support (those
> without scancodes in DMI).
> 
> This allow us to distinguish between same WMI codes with different types in
> sparse keymap. Thanks to this WMI events of type 0x0011 were moved from big
> switch into sparse keymap table too.
> 
> This patch also fixes possible bug in parsing WMI event buffer introduced
> in commit 5ea2559726b7 ("dell-wmi: Add support for new Dell systems"). That
> commit changed buffer type from int* to u16* without fixing code. More at:
> http://lkml.iu.edu/hypermail/linux/kernel/1507.0/01950.html
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> Tested-by: Michał Kępień <kernel@kempniu.pl>
> ---
>  drivers/platform/x86/dell-wmi.c |  215 +++++++++++++++++++--------------------
>  1 file changed, 107 insertions(+), 108 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index a406f01..fe831f3 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c

...

> @@ -379,59 +385,21 @@ static void dell_wmi_notify(u32 value, void *context)
>  		pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
>  
>  		switch (buffer_entry[1]) {

...

> -		case 0x10:
> -			/* Keys pressed */
> +		case 0x0010:
> +			/* Sequence of keys pressed */
>  			for (i = 2; i < len; ++i)
> -				dell_wmi_process_key(buffer_entry[i]);
> +				dell_wmi_process_key(0x0010, buffer_entry[i]);
>  			break;
> -		case 0x11:
> -			for (i = 2; i < len; ++i) {
> -				switch (buffer_entry[i]) {
> -				case 0xfff0:
> -					/* Battery unplugged */
> -					pr_debug("Battery unplugged\n");
> -					break;
> -				case 0xfff1:
> -					/* Battery inserted */
> -					pr_debug("Battery inserted\n");
> -					break;
> -				case 0x01e1:
> -				case 0x02ea:
> -				case 0x02eb:
> -				case 0x02ec:
> -				case 0x02f6:
> -					/* Keyboard backlight level changed */
> -					pr_debug("Keyboard backlight level "
> -						 "changed\n");
> -					break;
> -				default:
> -					/* Unknown event */
> -					pr_info("Unknown WMI event type 0x11: "
> -						"0x%x\n", (int)buffer_entry[i]);
> -					break;
> -				}
> -			}
> +		case 0x0011:
> +			/* Sequence of events occurred */
> +			for (i = 2; i < len; ++i)
> +				dell_wmi_process_key(0x0011, buffer_entry[i]);

Since this is identical to case 0x010, let's avoid the duplication of code and
handle this with a fall-through, like:

		case 0x0010:
		case 0x0011:
			/* Sequence of events occurred */
			for (i = 2; i < len; ++i)
				dell_wmi_process_key(buffer_entry[1], buffer_entry[i]);

Checkpatch caught a couple comments over 80 characters as well, please correct
along with the above change request.

Otherwise, this looks like a good improvement to me. I'll likely reword some of
the commentary for concision and clarity before I merge it.

Thanks!

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2 4/4] dell-wmi: Rework code for generating sparse keymap and processing WMI events

[Michał Kępień]

> > -		case 0x10:
> > -			/* Keys pressed */
> > +		case 0x0010:
> > +			/* Sequence of keys pressed */
> >  			for (i = 2; i < len; ++i)
> > -				dell_wmi_process_key(buffer_entry[i]);
> > +				dell_wmi_process_key(0x0010, buffer_entry[i]);
> >  			break;
> > -		case 0x11:
> > -			for (i = 2; i < len; ++i) {
> > -				switch (buffer_entry[i]) {
> > -				case 0xfff0:
> > -					/* Battery unplugged */
> > -					pr_debug("Battery unplugged\n");
> > -					break;
> > -				case 0xfff1:
> > -					/* Battery inserted */
> > -					pr_debug("Battery inserted\n");
> > -					break;
> > -				case 0x01e1:
> > -				case 0x02ea:
> > -				case 0x02eb:
> > -				case 0x02ec:
> > -				case 0x02f6:
> > -					/* Keyboard backlight level changed */
> > -					pr_debug("Keyboard backlight level "
> > -						 "changed\n");
> > -					break;
> > -				default:
> > -					/* Unknown event */
> > -					pr_info("Unknown WMI event type 0x11: "
> > -						"0x%x\n", (int)buffer_entry[i]);
> > -					break;
> > -				}
> > -			}
> > +		case 0x0011:
> > +			/* Sequence of events occurred */
> > +			for (i = 2; i < len; ++i)
> > +				dell_wmi_process_key(0x0011, buffer_entry[i]);
> 
> Since this is identical to case 0x010, let's avoid the duplication of code and
> handle this with a fall-through, like:
> 
> 		case 0x0010:
> 		case 0x0011:
> 			/* Sequence of events occurred */
> 			for (i = 2; i < len; ++i)
> 				dell_wmi_process_key(buffer_entry[1], buffer_entry[i]);

I believe it was Pali's intention to make a distinction between keys
being pressed (0x0010) and other events occuring (0x0011), so perhaps a
comment after the first case label could be useful?

    case 0x0010:
        /* Sequence of keys pressed; fall through */
    case 0x0011:
        /* Sequence of events occurred */
        for (i = 2; i < len; ++i)
            dell_wmi_process_key(buffer_entry[1], buffer_entry[i]);

I'll leave it to Pali to decide, I'm just throwing in my two cents.

-- 
Best regards,
Michał Kępień

Re: [PATCH v2 4/4] dell-wmi: Rework code for generating sparse keymap and processing WMI events

[Pali Rohár]

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

On Thursday 09 June 2016 13:27:21 Michał Kępień wrote:
> > > -		case 0x10:
> > > -			/* Keys pressed */
> > > +		case 0x0010:
> > > +			/* Sequence of keys pressed */
> > > 
> > >  			for (i = 2; i < len; ++i)
> > > 
> > > -				dell_wmi_process_key(buffer_entry[i]);
> > > +				dell_wmi_process_key(0x0010, buffer_entry[i]);
> > > 
> > >  			break;
> > > 
> > > -		case 0x11:
> > > -			for (i = 2; i < len; ++i) {
> > > -				switch (buffer_entry[i]) {
> > > -				case 0xfff0:
> > > -					/* Battery unplugged */
> > > -					pr_debug("Battery unplugged\n");
> > > -					break;
> > > -				case 0xfff1:
> > > -					/* Battery inserted */
> > > -					pr_debug("Battery inserted\n");
> > > -					break;
> > > -				case 0x01e1:
> > > -				case 0x02ea:
> > > -				case 0x02eb:
> > > -				case 0x02ec:
> > > -				case 0x02f6:
> > > -					/* Keyboard backlight level changed */
> > > -					pr_debug("Keyboard backlight level "
> > > -						 "changed\n");
> > > -					break;
> > > -				default:
> > > -					/* Unknown event */
> > > -					pr_info("Unknown WMI event type 0x11: "
> > > -						"0x%x\n", (int)buffer_entry[i]);
> > > -					break;
> > > -				}
> > > -			}
> > > +		case 0x0011:
> > > +			/* Sequence of events occurred */
> > > +			for (i = 2; i < len; ++i)
> > > +				dell_wmi_process_key(0x0011, buffer_entry[i]);
> > 
> > Since this is identical to case 0x010, let's avoid the duplication
> > of code and
> > 
> > handle this with a fall-through, like:
> > 		case 0x0010:
> > 		case 0x0011:
> > 			/* Sequence of events occurred */
> > 			for (i = 2; i < len; ++i)
> > 			
> > 				dell_wmi_process_key(buffer_entry[1], buffer_entry[i]);
> 
> I believe it was Pali's intention to make a distinction between keys
> being pressed (0x0010) and other events occuring (0x0011), so perhaps
> a comment after the first case label could be useful?
> 
>     case 0x0010:
>         /* Sequence of keys pressed; fall through */
>     case 0x0011:
>         /* Sequence of events occurred */
>         for (i = 2; i < len; ++i)
>             dell_wmi_process_key(buffer_entry[1], buffer_entry[i]);
> 
> I'll leave it to Pali to decide, I'm just throwing in my two cents.

Yes, that is truth, to visually distinguish between events and keys.

-- 
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 v2 0/4] dell-wmi: Changes in WMI event code handling

[Michał Kępień]

> First patch describe problem about 0xe045 code. Second and third are just
> cosmetic and last rework code which processing WMI events. It should be
> properly tested on more Dell machines, to check that everything is still
> working correctly.
> 
> Changes since v1:
> * Fixed comments
> * Fixed memory leak
> * Added Tested-By lines
> * Added event 0xe06e

Pali, assuming that you will address Darren's concerns (empty commit
message for patch 2/4, switch fall-through in patch 4/4 and some
comments over 80 characters in patches 2/4 and 4/4), feel free to add:

Reviewed-by: Michał Kępień <kernel@kempniu.pl>

as I don't see any further issues with this series.

-- 
Best regards,
Michał Kępień

Re: [PATCH v2 4/4] dell-wmi: Rework code for generating sparse keymap and processing WMI events

[Darren Hart]

On Thu, Jun 09, 2016 at 01:33:48PM +0200, Pali Rohár wrote:
> On Thursday 09 June 2016 13:27:21 Michał Kępień wrote:
> > > > -		case 0x10:
> > > > -			/* Keys pressed */
> > > > +		case 0x0010:
> > > > +			/* Sequence of keys pressed */
> > > > 
> > > >  			for (i = 2; i < len; ++i)
> > > > 
> > > > -				dell_wmi_process_key(buffer_entry[i]);
> > > > +				dell_wmi_process_key(0x0010, buffer_entry[i]);
> > > > 
> > > >  			break;
> > > > 
> > > > -		case 0x11:
> > > > -			for (i = 2; i < len; ++i) {
> > > > -				switch (buffer_entry[i]) {
> > > > -				case 0xfff0:
> > > > -					/* Battery unplugged */
> > > > -					pr_debug("Battery unplugged\n");
> > > > -					break;
> > > > -				case 0xfff1:
> > > > -					/* Battery inserted */
> > > > -					pr_debug("Battery inserted\n");
> > > > -					break;
> > > > -				case 0x01e1:
> > > > -				case 0x02ea:
> > > > -				case 0x02eb:
> > > > -				case 0x02ec:
> > > > -				case 0x02f6:
> > > > -					/* Keyboard backlight level changed */
> > > > -					pr_debug("Keyboard backlight level "
> > > > -						 "changed\n");
> > > > -					break;
> > > > -				default:
> > > > -					/* Unknown event */
> > > > -					pr_info("Unknown WMI event type 0x11: "
> > > > -						"0x%x\n", (int)buffer_entry[i]);
> > > > -					break;
> > > > -				}
> > > > -			}
> > > > +		case 0x0011:
> > > > +			/* Sequence of events occurred */
> > > > +			for (i = 2; i < len; ++i)
> > > > +				dell_wmi_process_key(0x0011, buffer_entry[i]);
> > > 
> > > Since this is identical to case 0x010, let's avoid the duplication
> > > of code and
> > > 
> > > handle this with a fall-through, like:
> > > 		case 0x0010:
> > > 		case 0x0011:
> > > 			/* Sequence of events occurred */
> > > 			for (i = 2; i < len; ++i)
> > > 			
> > > 				dell_wmi_process_key(buffer_entry[1], buffer_entry[i]);
> > 
> > I believe it was Pali's intention to make a distinction between keys
> > being pressed (0x0010) and other events occuring (0x0011), so perhaps
> > a comment after the first case label could be useful?
> > 
> >     case 0x0010:
> >         /* Sequence of keys pressed; fall through */
> >     case 0x0011:
> >         /* Sequence of events occurred */
> >         for (i = 2; i < len; ++i)
> >             dell_wmi_process_key(buffer_entry[1], buffer_entry[i]);
> > 
> > I'll leave it to Pali to decide, I'm just throwing in my two cents.
> 
> Yes, that is truth, to visually distinguish between events and keys.

The proposed comment accomplishes that without the duplication of code.

-- 
Darren Hart
Intel Open Source Technology Center