| Message ID | 20240417173124.9953-2-mpearson-lenovo@squebb.ca (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2,1/4] platform/x86: thinkpad_acpi: simplify known_ev handling | expand |
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, I see you got adding the new 0x13xx related hkeyscancodes right in the next patch in this series but I think such a comment as above will be helpful for future patches. If you agree with adding this comment I can add this while merging, no need to send a new version just for this. Regards, Hans > /* Hotkey keymap size */ > TPACPI_HOTKEY_MAP_LEN > @@ -3336,6 +3340,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm) > KEY_NOTIFICATION_CENTER, /* Notification Center */ > KEY_PICKUP_PHONE, /* Answer incoming call */ > KEY_HANGUP_PHONE, /* Decline incoming call */ > + KEY_PROG1, /* Trackpoint doubletap */ > }, > }; > > @@ -3996,6 +4001,15 @@ static bool hotkey_notify_6xxx(const u32 hkey, > return true; > } > > +static bool hotkey_notify_8xxx(const u32 hkey) > +{ > + if (hkey == TP_HKEY_EV_TRACK_DOUBLETAP) { > + tpacpi_input_send_key(TP_ACPI_HOTKEYSCAN_DOUBLETAP); > + return true; > + } > + return false; > +} > + > static void hotkey_notify(struct ibm_struct *ibm, u32 event) > { > u32 hkey; > @@ -4079,6 +4093,10 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event) > known_ev = true; > } > break; > + case 8: > + /* 0x8000-0x8FFF: misc2 */ > + known_ev = hotkey_notify_8xxx(hkey); > + break; > } > if (!known_ev) { > pr_notice("unhandled HKEY event 0x%04x\n", hkey);
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
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
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@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. 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
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@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. > > 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
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@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. >> >> 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). Regards, Hans
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@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. >>> >>> 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
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, + TP_ACPI_HOTKEYSCAN_DOUBLETAP, /* Hotkey keymap size */ TPACPI_HOTKEY_MAP_LEN @@ -3336,6 +3340,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm) KEY_NOTIFICATION_CENTER, /* Notification Center */ KEY_PICKUP_PHONE, /* Answer incoming call */ KEY_HANGUP_PHONE, /* Decline incoming call */ + KEY_PROG1, /* Trackpoint doubletap */ }, }; @@ -3996,6 +4001,15 @@ static bool hotkey_notify_6xxx(const u32 hkey, return true; } +static bool hotkey_notify_8xxx(const u32 hkey) +{ + if (hkey == TP_HKEY_EV_TRACK_DOUBLETAP) { + tpacpi_input_send_key(TP_ACPI_HOTKEYSCAN_DOUBLETAP); + return true; + } + return false; +} + static void hotkey_notify(struct ibm_struct *ibm, u32 event) { u32 hkey; @@ -4079,6 +4093,10 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event) known_ev = true; } break; + case 8: + /* 0x8000-0x8FFF: misc2 */ + known_ev = hotkey_notify_8xxx(hkey); + break; } if (!known_ev) { pr_notice("unhandled HKEY event 0x%04x\n", hkey);