Received-SPF: pass (google.com: domain of linux-kernel+bounces-149334-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Feedback-ID: ibe194615:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-379-gabd37849b7-fm-20240408.001-gabd37849
Precedence: bulk
MIME-Version: 1.0
Message-Id: <a7f7d94a-f1c8-4d6a-9c65-b5de59b9f7c0@app.fastmail.com>
In-Reply-To: <55ded7c3-fbc5-4fa5-8b63-da4d7aa4966c@redhat.com>
References: <mpearson-lenovo@squebb.ca>
 <20240417173124.9953-1-mpearson-lenovo@squebb.ca>
 <20240417173124.9953-2-mpearson-lenovo@squebb.ca>
 <98082080-0fcf-470f-afa5-76ec2bbffee7@redhat.com>
 <55ded7c3-fbc5-4fa5-8b63-da4d7aa4966c@redhat.com>
Date: Wed, 17 Apr 2024 19:57:17 -0400
From: "Mark Pearson" <mpearson-lenovo@squebb.ca>
To: "Hans de Goede" <hdegoede@redhat.com>
Cc: =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>,
 "Henrique de Moraes Holschuh" <hmh@hmh.eng.br>,
 ibm-acpi-devel@lists.sourceforge.net,
 "platform-driver-x86@vger.kernel.org" <platform-driver-x86@vger.kernel.org>,
 linux-kernel@vger.kernel.org, "Nitin Joshi1" <njoshi1@lenovo.com>,
 "Vishnu Sankar" <vsankar@lenovo.com>,
 "Peter Hutterer" <peter.hutterer@redhat.com>,
 "Vishnu Sankar" <vishnuocv@gmail.com>
Subject: Re: [PATCH v2 2/4] platform/x86: thinkpad_acpi: Support for trackpoint
 doubletap
Content-Type: text/plain

Hi Hans,

On Wed, Apr 17, 2024, at 4:06 PM, Hans de Goede wrote:
> Hi Mark,
>
> On 4/17/24 9:39 PM, Hans de Goede wrote:
>> 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,
>
> Ugh, actually this will not work becuuse we want hotkeyscancodes to be stable
> because these are userspace API since they can be remapped using hwdb so we
> cannot have the hotkeyscancode changing when new 0x1300-0x13ff range entries
> get added.
>
> So we need to either grow the table a lot and reserve a whole bunch of space
> for future 0x13xx - 0x13ff codes or maybe something like this:
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c 
> b/drivers/platform/x86/thinkpad_acpi.c
> index 771aaa7ae4cf..af3279889ecc 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -1742,7 +1742,12 @@ enum {	/* hot key scan codes (derived from ACPI 
> DSDT) */
>  	TP_ACPI_HOTKEYSCAN_VOLUMEDOWN,
>  	TP_ACPI_HOTKEYSCAN_MUTE,
>  	TP_ACPI_HOTKEYSCAN_THINKPAD,
> -	TP_ACPI_HOTKEYSCAN_UNK1,
> +	/*
> +	 * Note this gets send both on 0x1019 and on 
> TP_HKEY_EV_TRACK_DOUBLETAP
> +	 * hotkey-events. 0x1019 events have never been seen on any actual hw
> +	 * and a scancode is needed for the special 0x8036 doubletap 
> hotkey-event.
> +	 */
> +	TP_ACPI_HOTKEYSCAN_DOUBLETAP,
>  	TP_ACPI_HOTKEYSCAN_UNK2,
>  	TP_ACPI_HOTKEYSCAN_UNK3,
>  	TP_ACPI_HOTKEYSCAN_UNK4,
>
> or just hardcode KEY_PROG1 like your previous patch does, but I'm not
> a fan of that because of loosing hwdb remapping functionality for this
> "key" then.
>
> Note I'm open to other suggestions.
>
Oh...I hadn't thought of that impact. That's not great :(

I have an idea, but want to prototype it to see if it works out or not. Will update once I've had a chance to play with it.

Mark