Re: [PATCH] platform/x86: fujitsu-laptop: Simplify soft key handling

[Jonathan Woithe]

On Thu, Mar 22, 2018 at 06:27:16PM -0700, Darren Hart (VMware) wrote:
> The hardcoded BIT(X) used in the soft key handling can be confusing and
> prone to errors.
> 
> Instead, use the status FLAG_* defines for the sparse keymap index.
> Rather than check for each known bit, use a bitmask to filter for all
> known soft keys, and use the for_each_set_bit iterator.

I agree.

Reviewed-by: Jonathan Woithe <jwoithe@xxxxxxxxxx>

Regards
  jonathan

> Cc: Jan-Marek Glogowski <glogow@xxxxxxxxxx>
> Cc: Micha?? K??pie?? <kernel@xxxxxxxxxx>
> Cc: Jonathan Woithe <jwoithe@xxxxxxxxxx>
> Signed-off-by: Darren Hart (VMware) <dvhart@xxxxxxxxxxxxx>
> ---
> 
> Note: this applies on top of Jan-Marek's recent RFKILL and MICMUTE hotkey patch.
> I'd really appreciate a Tested-by from Jan-Marek in particular. You can get the
> entire series ready for testing here:
> 
> http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/review-dvhart-fujitsu
> 
> Also wondering if LID and DOCK might benefit from an input event...
> 
>  drivers/platform/x86/fujitsu-laptop.c | 47 ++++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index cd95b6f3a064..6afeaece2f50 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -91,6 +91,9 @@
>  #define FLAG_RFKILL			BIT(5)
>  #define FLAG_LID			BIT(8)
>  #define FLAG_DOCK			BIT(9)
> +#define FLAG_TOUCHPAD_TOGGLE		BIT(26)
> +#define FLAG_MICMUTE			BIT(29)
> +#define FLAG_SOFTKEYS			(FLAG_RFKILL | FLAG_TOUCHPAD_TOGGLE | FLAG_MICMUTE)
>  
>  /* FUNC interface - LED control */
>  #define FUNC_LED_OFF			BIT(0)
> @@ -456,14 +459,15 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
>  /* ACPI device for hotkey handling */
>  
>  static const struct key_entry keymap_default[] = {
> -	{ KE_KEY, KEY1_CODE, { KEY_PROG1 } },
> -	{ KE_KEY, KEY2_CODE, { KEY_PROG2 } },
> -	{ KE_KEY, KEY3_CODE, { KEY_PROG3 } },
> -	{ KE_KEY, KEY4_CODE, { KEY_PROG4 } },
> -	{ KE_KEY, KEY5_CODE, { KEY_RFKILL } },
> -	{ KE_KEY, BIT(5),    { KEY_RFKILL } },
> -	{ KE_KEY, BIT(26),   { KEY_TOUCHPAD_TOGGLE } },
> -	{ KE_KEY, BIT(29),   { KEY_MICMUTE } },
> +	{ KE_KEY, KEY1_CODE,            { KEY_PROG1 } },
> +	{ KE_KEY, KEY2_CODE,            { KEY_PROG2 } },
> +	{ KE_KEY, KEY3_CODE,            { KEY_PROG3 } },
> +	{ KE_KEY, KEY4_CODE,            { KEY_PROG4 } },
> +	{ KE_KEY, KEY5_CODE,            { KEY_RFKILL } },
> +	/* Soft keys read from status flags */
> +	{ KE_KEY, FLAG_RFKILL,          { KEY_RFKILL } },
> +	{ KE_KEY, FLAG_TOUCHPAD_TOGGLE, { KEY_TOUCHPAD_TOGGLE } },
> +	{ KE_KEY, FLAG_MICMUTE,         { KEY_MICMUTE } },
>  	{ KE_END, 0 }
>  };
>  
> @@ -903,7 +907,8 @@ static void acpi_fujitsu_laptop_release(struct acpi_device *device)
>  static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
>  {
>  	struct fujitsu_laptop *priv = acpi_driver_data(device);
> -	int scancode, i = 0, ret;
> +	unsigned long flags;
> +	int scancode, i = 0;
>  	unsigned int irb;
>  
>  	if (event != ACPI_FUJITSU_NOTIFY_CODE) {
> @@ -930,21 +935,17 @@ static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
>  					 "Unknown GIRB result [%x]\n", irb);
>  	}
>  
> -	/* On some models (first seen on the Skylake-based Lifebook
> -	 * E736/E746/E756), the touchpad toggle hotkey (Fn+F4) is
> -	 * handled in software; its state is queried using FUNC_FLAGS
> +	/*
> +	 * First seen on the Skylake-based Lifebook E736/E746/E756), the
> +	 * touchpad toggle hotkey (Fn+F4) is handled in software. Other models
> +	 * have since added additional "soft keys". These are reported in the
> +	 * status flags queried using FUNC_FLAGS.
>  	 */
> -	if (priv->flags_supported & (BIT(5) | BIT(26) | BIT(29))) {
> -		ret = call_fext_func(device, FUNC_FLAGS, 0x1, 0x0, 0x0);
> -		if (ret & BIT(5))
> -			sparse_keymap_report_event(priv->input,
> -						   BIT(5), 1, true);
> -		if (ret & BIT(26))
> -			sparse_keymap_report_event(priv->input,
> -						   BIT(26), 1, true);
> -		if (ret & BIT(29))
> -			sparse_keymap_report_event(priv->input,
> -						   BIT(29), 1, true);
> +	if (priv->flags_supported & (FLAG_SOFTKEYS)) {
> +		flags = call_fext_func(device, FUNC_FLAGS, 0x1, 0x0, 0x0);
> +		flags &= (FLAG_SOFTKEYS);
> +		for_each_set_bit(i, &flags, BITS_PER_LONG)
> +			sparse_keymap_report_event(priv->input, BIT(i), 1, true);
>  	}
>  }
>  
> -- 
> 2.14.3
> 
> 
> -- 
> Darren Hart
> VMware Open Source Technology Center

--