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 */
+		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;
 		}
-- 
2.44.0

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