Received-SPF: pass (google.com: domain of linux-kernel+bounces-149162-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <55ded7c3-fbc5-4fa5-8b63-da4d7aa4966c@redhat.com>
Date: Wed, 17 Apr 2024 22:06:09 +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
From: Hans de Goede <hdegoede@redhat.com>
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>
 <98082080-0fcf-470f-afa5-76ec2bbffee7@redhat.com>
Content-Language: en-US, nl
In-Reply-To: <98082080-0fcf-470f-afa5-76ec2bbffee7@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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.

Regards,

Hans