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 <[email protected]>
> Tested-by: Michał Kępień <[email protected]>
> ---
> 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
