Hi Hans
On Thu, Apr 18, 2024, at 10:42 AM, Hans de Goede wrote:
> Hi,
>
> On 4/18/24 2:24 PM, Mark Pearson wrote:
>> 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@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.
>>>
>>
>> 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 :)
>
> I noticed a small problem while working on this. The hwdb shipped with
> systemd has:
>
> # thinkpad_acpi driver
> evdev:name:ThinkPad Extra Buttons:dmi:bvn*:bvr*:bd*:svnIBM*:pn*:*
> KEYBOARD_KEY_01=battery # Fn+F2
> KEYBOARD_KEY_02=screenlock # Fn+F3
> KEYBOARD_KEY_03=sleep # Fn+F4
> KEYBOARD_KEY_04=wlan # Fn+F5
> KEYBOARD_KEY_06=switchvideomode # Fn+F7
> KEYBOARD_KEY_07=zoom # Fn+F8 screen
> expand
> KEYBOARD_KEY_08=f24 # Fn+F9 undock
> KEYBOARD_KEY_0b=suspend # Fn+F12
> KEYBOARD_KEY_0f=brightnessup # Fn+Home
> KEYBOARD_KEY_10=brightnessdown # Fn+End
> KEYBOARD_KEY_11=kbdillumtoggle # Fn+PgUp -
> ThinkLight
> KEYBOARD_KEY_13=zoom # Fn+Space
> KEYBOARD_KEY_14=volumeup
> KEYBOARD_KEY_15=volumedown
> KEYBOARD_KEY_16=mute
> KEYBOARD_KEY_17=prog1 #
> ThinkPad/ThinkVantage button (high k
>
> Notice the last line, this last line maps the old thinkpad /
> thinkvantage key: https://www.thinkwiki.org/wiki/ThinkPad_Button
> which is define by the kernel as KEY_VENDOR to KEY_PROG1 to
> use a keycode below 240 for X11 compatiblity which does not
> handle higher keycodes.
>
> This means that in practice at least on older models
> KEY_PROG1 is already taken and the thinkpad / thinkvantage key
> does the same (open lenovo help center / sysinfo) as
> what the new Fn + N key combo does. So it does makes sense
> to map Fn + N to KEY_VENDOR so those align but given the existing
> remapping of the thinkpad / thinkvantage key to PROG1 I think
> it would be better to not use PROG1 for the doubletap.
>
> I guess we can just use PROG2 instead to avoid the overlap
> with the remapped old ThinkPad / ThinkVantage buttons
> (which are more like Fn + N then doubletap).
>
Interesting as there has never been a Linux version of Vantage (we've looked into it and want to take pieces of it, but most of what is in Vantage doesn't make a lot of sense for Linux).
Using Prog2 sounds simple to me - no problems with that.
Mark
