Re: [PATCH v2 2/4] platform/x86: thinkpad_acpi: Support for trackpoint doubletap

[Hans de Goede]

Hi Mark,

Thank you for the new version of this series, overall this looks good,
one small remark below.

On 4/17/24 7:31 PM, Mark Pearson wrote:
> Lenovo trackpoints are adding the ability to generate a doubletap event.
> This handles the doubletap event and sends the KEY_PROG1 event to
> userspace.
> 
> Signed-off-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
> Signed-off-by: Vishnu Sankar <vishnuocv@xxxxxxxxx>
> ---
> Changes in v2:
>  - Use KEY_PROG1 instead of KEY_DOUBLETAP as input maintainer doesn't
>    want new un-specific key codes added.
>  - Add doubletap to hotkey scan code table and use existing hotkey
>    functionality.
>  - Tested using evtest, and then gnome settings to configure a custom shortcut
>    to launch an application.
> 
>  drivers/platform/x86/thinkpad_acpi.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 3b48d893280f..6d04d45e8d45 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -232,6 +232,9 @@ enum tpacpi_hkey_event_t {
>  
>  	/* Misc */
>  	TP_HKEY_EV_RFKILL_CHANGED	= 0x7000, /* rfkill switch changed */
> +
> +	/* Misc2 */
> +	TP_HKEY_EV_TRACK_DOUBLETAP      = 0x8036, /* trackpoint doubletap */
>  };
>  
>  /****************************************************************************
> @@ -1786,6 +1789,7 @@ enum {	/* hot key scan codes (derived from ACPI DSDT) */
>  	TP_ACPI_HOTKEYSCAN_NOTIFICATION_CENTER,
>  	TP_ACPI_HOTKEYSCAN_PICKUP_PHONE,
>  	TP_ACPI_HOTKEYSCAN_HANGUP_PHONE,

I understand why you've done this but I think this needs a comment,
something like:

        /*
         * For TP_HKEY_EV_TRACK_DOUBLETAP, unlike the codes above which map to:
         * (hkey_event - 0x1300) + TP_ACPI_HOTKEYSCAN_EXTENDED_START, this is
         * hardcoded for TP_HKEY_EV_TRACK_DOUBLETAP handling. Therefor this must
         * always be the last entry (after any 0x1300-0x13ff entries).
         */
+	TP_ACPI_HOTKEYSCAN_DOUBLETAP,

I see you got adding the new 0x13xx related hkeyscancodes right in the next
patch in this series but I think such a comment as above will be helpful
for future patches.

If you agree with adding this comment I can add this while merging, no need
to send a new version just for this.

Regards,

Hans





>  	/* Hotkey keymap size */
>  	TPACPI_HOTKEY_MAP_LEN
> @@ -3336,6 +3340,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
>  		KEY_NOTIFICATION_CENTER,	/* Notification Center */
>  		KEY_PICKUP_PHONE,		/* Answer incoming call */
>  		KEY_HANGUP_PHONE,		/* Decline incoming call */
> +		KEY_PROG1,                      /* Trackpoint doubletap */
>  		},
>  	};
>  
> @@ -3996,6 +4001,15 @@ static bool hotkey_notify_6xxx(const u32 hkey,
>  	return true;
>  }
>  
> +static bool hotkey_notify_8xxx(const u32 hkey)
> +{
> +	if (hkey == TP_HKEY_EV_TRACK_DOUBLETAP) {
> +		tpacpi_input_send_key(TP_ACPI_HOTKEYSCAN_DOUBLETAP);
> +		return true;
> +	}
> +	return false;
> +}
> +
>  static void hotkey_notify(struct ibm_struct *ibm, u32 event)
>  {
>  	u32 hkey;
> @@ -4079,6 +4093,10 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
>  				known_ev = true;
>  			}
>  			break;
> +		case 8:
> +			/* 0x8000-0x8FFF: misc2 */
> +			known_ev = hotkey_notify_8xxx(hkey);
> +			break;
>  		}
>  		if (!known_ev) {
>  			pr_notice("unhandled HKEY event 0x%04x\n", hkey);