Message-Id: <0917e5bc-a198-4aa8-812e-31434408e78d@app.fastmail.com>
Date: Thu, 18 Apr 2024 08:24:40 -0400
From: "Mark Pearson" <mpearson-lenovo@...ebb.ca>
To: "Hans de Goede" <hdegoede@...hat.com>
Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 "Henrique de Moraes Holschuh" <hmh@....eng.br>,
 ibm-acpi-devel@...ts.sourceforge.net,
 "platform-driver-x86@...r.kernel.org" <platform-driver-x86@...r.kernel.org>,
 linux-kernel@...r.kernel.org, "Nitin Joshi1" <njoshi1@...ovo.com>,
 "Vishnu Sankar" <vsankar@...ovo.com>,
 "Peter Hutterer" <peter.hutterer@...hat.com>,
 "Vishnu Sankar" <vishnuocv@...il.com>
Subject: Re: [PATCH v2 2/4] platform/x86: thinkpad_acpi: Support for trackpoint
 doubletap

Hi Hans,

On Thu, Apr 18, 2024, at 7:34 AM, Hans de Goede wrote:
> Hi Mark,
>
> On 4/18/24 1:57 AM, Mark Pearson wrote:
>> 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@...ebb.ca>
>>>>> Signed-off-by: Vishnu Sankar <vishnuocv@...il.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.
>
> Thinking more about this I just realized that the input subsystem
> already has a mechanism for dealing with scancode ranges with
> (big) holes in them in the form of linux/input/sparse-keymap.h .
>
> I think that what needs to be done is convert the existing code
> to use sparse-keymap, keeping the mapping of the "MHKP"
> returned hkey codes to internal TP_ACPI_HOTKEYSCAN_* values
> for currently supported "MHKP" hkey codes for compatibility
> and then for new codes just directly map them in the sparse map
> aka the struct key_entry table. After converting the existing code
> to use sparse-keymap, then for the new events we would simply add:
>
>
> 	{ KE_KEY, 0x131d, { KEY_VENDOR} }, /* Fn + N, system debug info */
> 	{ KE_KEY, 0x8036, { KEY_PROG1 } }, /* Trackpoint doubletap */
>
> entries to the table without needing to define intermediate
> TP_ACPI_HOTKEYSCAN_* values for these.
>

Ah! I didn't know about sparse-keymap but it looks similar to what I was thinking and played with a bit last night. Agreed using existing infrastructure is better.

Only things I'd flag is that:
 - It did look like it would be useful to identify keys that the driver handles (there aren't many but a few). Maybe one of the other key types can help handle that?
 - There are also some keys that use the tpacpi_input_send_key_masked that might need some special consideration.

> I already have somewhat of a design for this in my head and I really
> believe this is the way forward as it uses existing kernel infra
> and it will avoid hitting this problem again when some other new
> "MHKP" hkey codes show up.
>
> I plan to start working on implementing conversion of the existing
> code to use sparse-keymap, which should result in a nice cleanup
> after lunch and I hope to have something for you to test no later
> then next Tuesday.
>

That would be amazing - do let me know if there is anything I can help with. Agreed this will help clean up a bunch of the keycode handling :)

Mark

Powered by blists - more mailing lists