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@xxxxxxxxx>
> Tested-by: MichaÅ KÄpieÅ <kernel@xxxxxxxxxx>
> ---
>  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