Received-SPF: pass (google.com: domain of linux-kernel+bounces-149130-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <98082080-0fcf-470f-afa5-76ec2bbffee7@redhat.com>
Date: Wed, 17 Apr 2024 21:39:54 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 2/4] platform/x86: thinkpad_acpi: Support for
 trackpoint doubletap
To: Mark Pearson <mpearson-lenovo@squebb.ca>
Cc: ilpo.jarvinen@linux.intel.com, hmh@hmh.eng.br,
 ibm-acpi-devel@lists.sourceforge.net, platform-driver-x86@vger.kernel.org,
 linux-kernel@vger.kernel.org, njoshi1@lenovo.com, vsankar@lenovo.com,
 peter.hutterer@redhat.com, Vishnu Sankar <vishnuocv@gmail.com>
References: <mpearson-lenovo@squebb.ca>
 <20240417173124.9953-1-mpearson-lenovo@squebb.ca>
 <20240417173124.9953-2-mpearson-lenovo@squebb.ca>
Content-Language: en-US, nl
From: Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20240417173124.9953-2-mpearson-lenovo@squebb.ca>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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@squebb.ca>
> Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>
> ---
> 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);