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@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,
>>
>> 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.
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.
Regards,
Hans
