Re: [PATCH v2 2/3] dell-wmi: Support new hotkeys on the XPS 13 9350 (Skylake)

Andy Lutomirski
2015-12-04T00:01:15Z

On Thu, Dec 3, 2015 at 3:52 PM, Darren Hart <dvhart@infradead.org> wrote:
> On Mon, Nov 30, 2015 at 05:02:00PM -0800, Andy Lutomirski wrote:
>> 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.
>>
>> So far, this doesn't send keypress events for any of the new
>> events. Depnding on whether we figure out exactly what needs to
>> happen to get the wireless button working in time for Linux 4.5,
>> we might want to temporarily handle it in dell-wmi.
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>> drivers/platform/x86/dell-wmi.c | 59 ++++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 52 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
>> index ad6e965c5862..baff658a3621 100644
>> --- a/drivers/platform/x86/dell-wmi.c
>> +++ b/drivers/platform/x86/dell-wmi.c
>> @@ -161,6 +161,27 @@ static const u16 bios_to_linux_keycode[256] __initconst = {
>> [255] = KEY_PROG3,
>> };
>>
>> +/* These are applied if the hk table is present and doesn't override them. */
>
>
> Please do not abbreviate in comments.

Will fix for v3.

>
>
>> +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)
>> @@ -319,9 +340,10 @@ static const struct key_entry * __init dell_wmi_prepare_new_keymap(void)
>> int hotkey_num = (dell_bios_hotkey_table->header.length - 4) /
>> sizeof(struct dell_bios_keymap_entry);
>> struct key_entry *keymap;
>> - int i;
>> + int i, pos = 0, num_bios_keys;
>>
>> - keymap = kcalloc(hotkey_num + 1, sizeof(struct key_entry), GFP_KERNEL);
>> + keymap = kcalloc(hotkey_num + ARRAY_SIZE(dell_wmi_extra_keymap),
>> + sizeof(struct key_entry), GFP_KERNEL);
>
> Did something change since our last round of review making the +1 for KE_END
> no longer necessary? It looks like it should be possible to use all the bios
> hotkey keymap and all the extra keymap, so we would still need +1 for KE_END.

No, my bad. That's embarrassing :(

--Andy