| Message ID | 20240324210817.192033-3-mpearson-lenovo@squebb.ca (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/4] Input: Add trackpoint doubletap and system debug info keycodes | expand |
Hi Mark, On 3/24/24 10:07 PM, Mark Pearson wrote: > Lenovo trackpoints are adding the ability to generate a doubletap event. > This handles the doubletap event and sends the KEY_DOUBLECLICK event to > userspace. > > Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> > Signed-off-by: Vishnu Sankar <vsankar@lenovo.com> > --- > drivers/platform/x86/thinkpad_acpi.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index 82429e59999d..2bbb32c898e9 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -232,6 +232,7 @@ enum tpacpi_hkey_event_t { > > /* Misc */ > TP_HKEY_EV_RFKILL_CHANGED = 0x7000, /* rfkill switch changed */ > + TP_HKEY_EV_TRACKPOINT_DOUBLETAP = 0x8036, /* doubletap on Trackpoint*/ > }; > > /**************************************************************************** > @@ -4081,6 +4082,22 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event) > break; > } > fallthrough; /* to default */ This now no longer fallsthrough to default. IMHO the best thing to do here is add a new preparation patch which initializes known_ev to false inside the while before the switch-case (together with the send_acpi_ev and ignore_acpi_ev init). and then change this fallthrough to a break in the preparation patch. You can then also remove the default case altogether in this prep patch. > + case 8: > + /* 0x8036: Trackpoint doubletaps */ > + if (hkey == TP_HKEY_EV_TRACKPOINT_DOUBLETAP) { > + send_acpi_ev = true; > + ignore_acpi_ev = false; These 2 values are set as the default above the switch-case, please drop these 2 lines. > + known_ev = true; > + /* Send to user space */ > + mutex_lock(&tpacpi_inputdev_send_mutex); > + input_report_key(tpacpi_inputdev, KEY_DOUBLECLICK, 1); > + input_sync(tpacpi_inputdev); > + input_report_key(tpacpi_inputdev, KEY_DOUBLECLICK, 0); > + input_sync(tpacpi_inputdev); > + mutex_unlock(&tpacpi_inputdev_send_mutex); This code duplicates tpacpi_input_send_key(), what you want to do here is define a hotkey_keycode_map scancode range for new 0x8xxx codes like how this was done when extended scancodes where added to deal with the new 0x13xx hotkey event codes for the 2017+ models. See commit 696c6523ec8f ("platform/x86: thinkpad_acpi: add mapping for new hotkeys") Despite re-using tpacpi_input_send_key() there are 2 reasons why we want scancodes for these new "keys". 1. By adding the keys to the hotkey_keycode_map they automatically also get input_set_capability(tpacpi_inputdev, EV_KEY, hotkey_keycode_map[i]); called on them advertising to userspace that tpacpi_inputdev can actually generate these keypresses. Something which is currently lacking from your patch. Related to this did you test this with evtest? I think that the input core will suppress the events when you do not set the capability ? 2. This allows remapping scancodes to different KEY_foo values with hwdb entries. Regards, Hans > + break; > + } > + fallthrough; /* to default */ > default: > known_ev = false; > }
Hi Hans, Many thanks for the review. On Mon, Apr 8, 2024, at 9:04 AM, Hans de Goede wrote: > Hi Mark, > > On 3/24/24 10:07 PM, Mark Pearson wrote: >> Lenovo trackpoints are adding the ability to generate a doubletap event. >> This handles the doubletap event and sends the KEY_DOUBLECLICK event to >> userspace. >> >> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> >> Signed-off-by: Vishnu Sankar <vsankar@lenovo.com> >> --- >> drivers/platform/x86/thinkpad_acpi.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c >> index 82429e59999d..2bbb32c898e9 100644 >> --- a/drivers/platform/x86/thinkpad_acpi.c >> +++ b/drivers/platform/x86/thinkpad_acpi.c >> @@ -232,6 +232,7 @@ enum tpacpi_hkey_event_t { >> >> /* Misc */ >> TP_HKEY_EV_RFKILL_CHANGED = 0x7000, /* rfkill switch changed */ >> + TP_HKEY_EV_TRACKPOINT_DOUBLETAP = 0x8036, /* doubletap on Trackpoint*/ >> }; >> >> /**************************************************************************** >> @@ -4081,6 +4082,22 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event) >> break; >> } >> fallthrough; /* to default */ > > This now no longer fallsthrough to default. IMHO the best thing to do > here is add a new preparation patch which initializes known_ev to false > inside the while before the switch-case (together with the send_acpi_ev > and ignore_acpi_ev init). and then change this fallthrough to a break > in the preparation patch. You can then also remove the default case > altogether in this prep patch. > Ack - that makes sense. I'll look at doing that. >> + case 8: >> + /* 0x8036: Trackpoint doubletaps */ >> + if (hkey == TP_HKEY_EV_TRACKPOINT_DOUBLETAP) { >> + send_acpi_ev = true; >> + ignore_acpi_ev = false; > > These 2 values are set as the default above the switch-case, please > drop these 2 lines. Agreed. Will change. > >> + known_ev = true; >> + /* Send to user space */ >> + mutex_lock(&tpacpi_inputdev_send_mutex); >> + input_report_key(tpacpi_inputdev, KEY_DOUBLECLICK, 1); >> + input_sync(tpacpi_inputdev); >> + input_report_key(tpacpi_inputdev, KEY_DOUBLECLICK, 0); >> + input_sync(tpacpi_inputdev); >> + mutex_unlock(&tpacpi_inputdev_send_mutex); > > This code duplicates tpacpi_input_send_key(), what you want to do here > is define a hotkey_keycode_map scancode range for new 0x8xxx codes like how this > was done when extended scancodes where added to deal with the new 0x13xx hotkey > event codes for the 2017+ models. > > See commit 696c6523ec8f ("platform/x86: thinkpad_acpi: add mapping for > new hotkeys") > > Despite re-using tpacpi_input_send_key() there are 2 reasons why we want > scancodes for these new "keys". > > 1. By adding the keys to the hotkey_keycode_map they automatically > also get input_set_capability(tpacpi_inputdev, EV_KEY, hotkey_keycode_map[i]); > called on them advertising to userspace that tpacpi_inputdev can actually > generate these keypresses. Something which is currently lacking from your > patch. Related to this did you test this with evtest? I think that the input > core will suppress the events when you do not set the capability ? > > 2. This allows remapping scancodes to different KEY_foo values with hwdb > entries. > Will look into doing this. There was a reason originally I did it like this, but I can't remember what it was. I'll revisit it. I did test with evtest but I ended up having to cheat as there's quite a few layers in userspace and I got a bit bogged down chewing my way through those (building them against the right headers etc). I ended up using an already existing code to make sure it was doing the right thing in the driver - and then assumed that once the keycode was 'released', and the different user space projects updated per normal procedure, it would work. It's possible it meant I bypassed/missed this issue so I'll retry once I've made the updates. Mark
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 82429e59999d..2bbb32c898e9 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -232,6 +232,7 @@ enum tpacpi_hkey_event_t { /* Misc */ TP_HKEY_EV_RFKILL_CHANGED = 0x7000, /* rfkill switch changed */ + TP_HKEY_EV_TRACKPOINT_DOUBLETAP = 0x8036, /* doubletap on Trackpoint*/ }; /**************************************************************************** @@ -4081,6 +4082,22 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event) break; } fallthrough; /* to default */ + case 8: + /* 0x8036: Trackpoint doubletaps */ + if (hkey == TP_HKEY_EV_TRACKPOINT_DOUBLETAP) { + send_acpi_ev = true; + ignore_acpi_ev = false; + known_ev = true; + /* Send to user space */ + mutex_lock(&tpacpi_inputdev_send_mutex); + input_report_key(tpacpi_inputdev, KEY_DOUBLECLICK, 1); + input_sync(tpacpi_inputdev); + input_report_key(tpacpi_inputdev, KEY_DOUBLECLICK, 0); + input_sync(tpacpi_inputdev); + mutex_unlock(&tpacpi_inputdev_send_mutex); + break; + } + fallthrough; /* to default */ default: known_ev = false; }