diff mbox

[v5,4/5] dell-wmi: Support new hotkeys on the XPS 13 9350 (Skylake)

Message ID 6492bf46d1042b6d6c80ade8792327797e668d09.1455553470.git.luto@kernel.org (mailing list archive)
State Changes Requested, archived
Headers show

Commit Message

Andy Lutomirski Feb. 15, 2016, 4:32 p.m. UTC
The XPS 13 9350 sends WMI keypress events that aren't enumerated in
the DMI table.  Add a table listing them.  To avoid breaking things
that worked before, these un-enumerated hotkeys won't be used if the
DMI table maps them to something else.

FWIW, it appears that the DMI table may be a legacy thing and we
might want to rethink how we handle events in general.  As an
example, a whole lot of things map to KEY_PROG3 via the DMI table.

This doesn't send keypress events for any of the new events.  They
appear to all be handled by other means (keyboard illumination is
handled automatically and rfkill is handled by intel-hid).

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---

Changes from v4:
 - Update changelog.

Changes from v3:
 - Rebase in top of interface version stuff.  (This changes context
   but not any diff lines.)
Changes from v2:
 - Factor check for already-known scancodes into a helper.
 - Un-abbreviate comments.
 - Fix off-by-one.
 - Rebase on top of of dmi_walk fixes.

Changes from v1:
 - The new hotkey code matches reality better.
 - Don't send key events for the new hotkeys.

 drivers/platform/x86/dell-wmi.c | 71 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 64 insertions(+), 7 deletions(-)

Comments

Pali Rohár Feb. 15, 2016, 5:20 p.m. UTC | #1
On Monday 15 February 2016 17:32:36 Andy Lutomirski wrote:
> +       /* Stealth mode toggle */
> +       { KE_IGNORE, 0x155, { KEY_RESERVED } },

Hi! Just one question, what does this "Stealth mode" means and what this 
toggle key/button doing?

I would propose for Laptops manufactures to revert back normal 
nonchiclet keyboard with full 105 normal keys (with F1-F12) instead 
inventing such useless and crappy/funny names for keys/buttons on 
laptops which replace PgUP/PgDown/SysRq and etc...
Mario Limonciello Feb. 15, 2016, 5:26 p.m. UTC | #2
On 02/15/2016 11:20 AM, Pali Rohár wrote:
> On Monday 15 February 2016 17:32:36 Andy Lutomirski wrote:
>> +       /* Stealth mode toggle */
>> +       { KE_IGNORE, 0x155, { KEY_RESERVED } },
> Hi! Just one question, what does this "Stealth mode" means and what this 
> toggle key/button doing?
>
> I would propose for Laptops manufactures to revert back normal 
> nonchiclet keyboard with full 105 normal keys (with F1-F12) instead 
> inventing such useless and crappy/funny names for keys/buttons on 
> laptops which replace PgUP/PgDown/SysRq and etc...
>
Pali,

Stealth mode will "disable all lights and sounds".  The event is for
notification only.  The actual change is performed by a combination of
the BIOS and EC. 

There is also a BIOS setting that disables the hotkey from doing anything.

Thanks,
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pali Rohár Feb. 15, 2016, 5:37 p.m. UTC | #3
On Monday 15 February 2016 18:26:24 Mario Limonciello wrote:
> On 02/15/2016 11:20 AM, Pali Rohár wrote:
> > On Monday 15 February 2016 17:32:36 Andy Lutomirski wrote:
> >> +       /* Stealth mode toggle */
> >> +       { KE_IGNORE, 0x155, { KEY_RESERVED } },
> > 
> > Hi! Just one question, what does this "Stealth mode" means and what
> > this toggle key/button doing?
> > 
> > I would propose for Laptops manufactures to revert back normal
> > nonchiclet keyboard with full 105 normal keys (with F1-F12) instead
> > inventing such useless and crappy/funny names for keys/buttons on
> > laptops which replace PgUP/PgDown/SysRq and etc...
> 
> Pali,
> 
> Stealth mode will "disable all lights and sounds".  The event is for
> notification only.  The actual change is performed by a combination
> of the BIOS and EC.
> 
> There is also a BIOS setting that disables the hotkey from doing
> anything.
> 
> Thanks,

Thank you! Now I can image what this line in diff means :-)

Anyway, I would propose some rule to and longer description for newly 
invented hot key events which are marked as KEY_RESERVED in kernel 
source code. Really sometimes it is hard to guess what it can means and 
constant KEY_RESERVED does not help much more.
Darren Hart Feb. 17, 2016, 7:29 a.m. UTC | #4
On Mon, Feb 15, 2016 at 06:37:16PM +0100, Pali Rohár wrote:
> On Monday 15 February 2016 18:26:24 Mario Limonciello wrote:
> > On 02/15/2016 11:20 AM, Pali Rohár wrote:
> > > On Monday 15 February 2016 17:32:36 Andy Lutomirski wrote:
> > >> +       /* Stealth mode toggle */
> > >> +       { KE_IGNORE, 0x155, { KEY_RESERVED } },
> > > 
> > > Hi! Just one question, what does this "Stealth mode" means and what
> > > this toggle key/button doing?
> > > 
> > > I would propose for Laptops manufactures to revert back normal
> > > nonchiclet keyboard with full 105 normal keys (with F1-F12) instead
> > > inventing such useless and crappy/funny names for keys/buttons on
> > > laptops which replace PgUP/PgDown/SysRq and etc...
> > 
> > Pali,
> > 
> > Stealth mode will "disable all lights and sounds".  The event is for
> > notification only.  The actual change is performed by a combination
> > of the BIOS and EC.
> > 
> > There is also a BIOS setting that disables the hotkey from doing
> > anything.
> > 
> > Thanks,
> 
> Thank you! Now I can image what this line in diff means :-)
> 
> Anyway, I would propose some rule to and longer description for newly 
> invented hot key events which are marked as KEY_RESERVED in kernel 
> source code. Really sometimes it is hard to guess what it can means and 
> constant KEY_RESERVED does not help much more.

Since we got the update from Mario, could you include the "disable all lights
and sounds" blurb in the KEY_RESERVED comment for stealth mode as part of the
next spin.

Pali, is that your only concern with this series?
Pali Rohár Feb. 17, 2016, 11:19 a.m. UTC | #5
On Tuesday 16 February 2016 23:29:42 Darren Hart wrote:
> On Mon, Feb 15, 2016 at 06:37:16PM +0100, Pali Rohár wrote:
> > On Monday 15 February 2016 18:26:24 Mario Limonciello wrote:
> > > On 02/15/2016 11:20 AM, Pali Rohár wrote:
> > > > On Monday 15 February 2016 17:32:36 Andy Lutomirski wrote:
> > > >> +       /* Stealth mode toggle */
> > > >> +       { KE_IGNORE, 0x155, { KEY_RESERVED } },
> > > > 
> > > > Hi! Just one question, what does this "Stealth mode" means and what
> > > > this toggle key/button doing?
> > > > 
> > > > I would propose for Laptops manufactures to revert back normal
> > > > nonchiclet keyboard with full 105 normal keys (with F1-F12) instead
> > > > inventing such useless and crappy/funny names for keys/buttons on
> > > > laptops which replace PgUP/PgDown/SysRq and etc...
> > > 
> > > Pali,
> > > 
> > > Stealth mode will "disable all lights and sounds".  The event is for
> > > notification only.  The actual change is performed by a combination
> > > of the BIOS and EC.
> > > 
> > > There is also a BIOS setting that disables the hotkey from doing
> > > anything.
> > > 
> > > Thanks,
> > 
> > Thank you! Now I can image what this line in diff means :-)
> > 
> > Anyway, I would propose some rule to and longer description for newly 
> > invented hot key events which are marked as KEY_RESERVED in kernel 
> > source code. Really sometimes it is hard to guess what it can means and 
> > constant KEY_RESERVED does not help much more.
> 
> Since we got the update from Mario, could you include the "disable all lights
> and sounds" blurb in the KEY_RESERVED comment for stealth mode as part of the
> next spin.
> 
> Pali, is that your only concern with this series?

Just I would like to know reason for NetworkManager in 5/5. But patches
looks good, so you can add my Acked-by.
diff mbox

Patch

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 32808a463325..e38258a82be5 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -169,6 +169,30 @@  static const u16 bios_to_linux_keycode[256] __initconst = {
 	[255]	= KEY_PROG3,
 };
 
+/*
+ * 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 = {
+	/* Fn-lock */
+	{ KE_IGNORE, 0x151, { KEY_RESERVED } },
+
+	/* Change keyboard illumination */
+	{ KE_IGNORE, 0x152, { KEY_KBDILLUMTOGGLE } },
+
+	/*
+	 * Radio disable (notify only -- there is no model for which the
+	 * WMI event is supposed to trigger an action).
+	 */
+	{ KE_IGNORE, 0x153, { KEY_RFKILL } },
+
+	/* RGB keyboard backlight control */
+	{ KE_IGNORE, 0x154, { KEY_RESERVED } },
+
+	/* Stealth mode toggle */
+	{ KE_IGNORE, 0x155, { KEY_RESERVED } },
+};
+
 static struct input_dev *dell_wmi_input_dev;
 
 static void dell_wmi_process_key(int reported_key)
@@ -340,13 +364,27 @@  static void dell_wmi_notify(u32 value, void *context)
 	kfree(obj);
 }
 
+static bool have_scancode(u32 scancode, const struct key_entry *keymap, int len)
+{
+	int i;
+
+	for (i = 0; i < len; i++)
+		if (keymap[i].code == scancode)
+			return true;
+
+	return false;
+}
+
 static void __init handle_dmi_entry(const struct dmi_header *dm,
+
 				    void *opaque)
+
 {
 	struct dell_dmi_results *results = opaque;
 	struct dell_bios_hotkey_table *table;
+	int hotkey_num, i, pos = 0;
 	struct key_entry *keymap;
-	int hotkey_num, i;
+	int num_bios_keys;
 
 	if (results->err || results->keymap)
 		return;		/* We already found the hotkey table. */
@@ -370,7 +408,8 @@  static void __init handle_dmi_entry(const struct dmi_header *dm,
 		return;
 	}
 
-	keymap = kcalloc(hotkey_num + 1, sizeof(struct key_entry), GFP_KERNEL);
+	keymap = kcalloc(hotkey_num + ARRAY_SIZE(dell_wmi_extra_keymap) + 1,
+			 sizeof(struct key_entry), GFP_KERNEL);
 	if (!keymap) {
 		results->err = -ENOMEM;
 		return;
@@ -398,14 +437,32 @@  static void __init handle_dmi_entry(const struct dmi_header *dm,
 		}
 
 		if (keycode == KEY_KBDILLUMTOGGLE)
-			keymap[i].type = KE_IGNORE;
+			keymap[pos].type = KE_IGNORE;
 		else
-			keymap[i].type = KE_KEY;
-		keymap[i].code = bios_entry->scancode;
-		keymap[i].keycode = keycode;
+			keymap[pos].type = KE_KEY;
+		keymap[pos].code = bios_entry->scancode;
+		keymap[pos].keycode = keycode;
+
+		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[hotkey_num].type = KE_END;
+	keymap[pos].type = KE_END;
 
 	results->keymap = keymap;
 }