[PATCH v2 1/4] platform/x86: thinkpad_acpi: simplify known_ev handling

[PATCH v2 1/4] platform/x86: thinkpad_acpi: simplify known_ev handling

[Mark Pearson]

Modify how known_ev event is handled in preparation for adding new keycode
range.

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
Changes in v2: 
 - New addition to series based on recommendations from review.
 - Note previous input patch was dropped so in numbering gets replaced by this.

 drivers/platform/x86/thinkpad_acpi.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 82429e59999d..3b48d893280f 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -4026,6 +4026,7 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
 
 		send_acpi_ev = true;
 		ignore_acpi_ev = false;
+		known_ev = false;
 
 		switch (hkey >> 12) {
 		case 1:
@@ -4051,8 +4052,6 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
 				/* FIXME: kick libata if SATA link offline */
 				known_ev = true;
 				break;
-			default:
-				known_ev = false;
 			}
 			break;
 		case 4:
@@ -4078,11 +4077,8 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
 				tpacpi_send_radiosw_update();
 				send_acpi_ev = 0;
 				known_ev = true;
-				break;
 			}
-			fallthrough;	/* to default */
-		default:
-			known_ev = false;
+			break;
 		}
 		if (!known_ev) {
 			pr_notice("unhandled HKEY event 0x%04x\n", hkey);
-- 
2.44.0

[PATCH v2 2/4] platform/x86: thinkpad_acpi: Support for trackpoint doubletap

[Mark Pearson]

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,
+	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);
-- 
2.44.0

[PATCH v2 3/4] platform/x86: thinkpad_acpi: Support for system debug info hotkey

[Mark Pearson]

New Lenovo platforms are adding the FN+N key to generate system debug
details that support can use for collecting important details on any
customer cases for Windows.
Add the infrastructure so we can do the same on Linux by generating a
SYS_VENDOR keycode to userspace.

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Nitin Joshi <njoshi1@lenovo.com>
---
Changes in v2: 
 - Improved comments on keycodes in init function.
 - Filled in missing gaps

 drivers/platform/x86/thinkpad_acpi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 6d04d45e8d45..a2f21e958d39 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -1789,6 +1789,10 @@ 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_AMT_TOGGLE,
+	TP_ACPI_HOTKEYSCAN_CAMERA_SHUTTER,
+	TP_ACPI_HOTKEYSCAN_DOUBLETAP_TOGGLE,
+	TP_ACPI_HOTKEYSCAN_SYS_DEBUG_INFO,
 	TP_ACPI_HOTKEYSCAN_DOUBLETAP,
 
 	/* Hotkey keymap size */
@@ -3340,6 +3344,10 @@ 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_UNKNOWN,			/* AMT_TOGGLE handled in driver, 0x31a */
+		KEY_UNKNOWN,			/* Camera Shutter Switch, 0X31b */
+		KEY_UNKNOWN,			/* DOUBLETAP_TOGGLE, 0x31c */
+		KEY_VENDOR,                     /* System debug info, 0x31D */
 		KEY_PROG1,                      /* Trackpoint doubletap */
 		},
 	};
-- 
2.44.0

[PATCH v2 4/4] platform/x86: thinkpad_acpi: Support hotkey to disable trackpoint doubletap

[Mark Pearson]

The hotkey combination FN+G can be used to disable the trackpoint
doubletap feature on Windows.
Add matching functionality for Linux.

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>
---
Changes in v2: 
 - Improved comments on keycodes in init function

 drivers/platform/x86/thinkpad_acpi.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index a2f21e958d39..370b9285156c 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -167,6 +167,7 @@ enum tpacpi_hkey_event_t {
 	TP_HKEY_EV_VOL_MUTE		= 0x1017, /* Mixer output mute */
 	TP_HKEY_EV_PRIVACYGUARD_TOGGLE	= 0x130f, /* Toggle priv.guard on/off */
 	TP_HKEY_EV_AMT_TOGGLE		= 0x131a, /* Toggle AMT on/off */
+	TP_HKEY_EV_DOUBLETAP_TOGGLE	= 0x131c, /* Toggle trackpoint doubletap on/off */
 	TP_HKEY_EV_PROFILE_TOGGLE	= 0x131f, /* Toggle platform profile */
 
 	/* Reasons for waking up from S3/S4 */
@@ -356,6 +357,7 @@ static struct {
 	u32 hotkey_poll_active:1;
 	u32 has_adaptive_kbd:1;
 	u32 kbd_lang:1;
+	u32 trackpoint_doubletap:1;
 	struct quirk_entry *quirks;
 } tp_features;
 
@@ -3346,7 +3348,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 		KEY_HANGUP_PHONE,		/* Decline incoming call */
 		KEY_UNKNOWN,			/* AMT_TOGGLE handled in driver, 0x31a */
 		KEY_UNKNOWN,			/* Camera Shutter Switch, 0X31b */
-		KEY_UNKNOWN,			/* DOUBLETAP_TOGGLE, 0x31c */
+		KEY_UNKNOWN,			/* DOUBLETAP_TOGGLE handled in driver, 0x31c */
 		KEY_VENDOR,                     /* System debug info, 0x31D */
 		KEY_PROG1,                      /* Trackpoint doubletap */
 		},
@@ -3606,6 +3608,9 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 
 	hotkey_poll_setup_safe(true);
 
+	/* Enable doubletap by default */
+	tp_features.trackpoint_doubletap = 1;
+
 	return 0;
 }
 
@@ -3747,6 +3752,7 @@ static bool hotkey_notify_extended_hotkey(const u32 hkey)
 	case TP_HKEY_EV_PRIVACYGUARD_TOGGLE:
 	case TP_HKEY_EV_AMT_TOGGLE:
 	case TP_HKEY_EV_PROFILE_TOGGLE:
+	case TP_HKEY_EV_DOUBLETAP_TOGGLE:
 		tpacpi_driver_event(hkey);
 		return true;
 	}
@@ -4011,7 +4017,7 @@ static bool hotkey_notify_6xxx(const u32 hkey,
 
 static bool hotkey_notify_8xxx(const u32 hkey)
 {
-	if (hkey == TP_HKEY_EV_TRACK_DOUBLETAP) {
+	if (hkey == TP_HKEY_EV_TRACK_DOUBLETAP && tp_features.trackpoint_doubletap) {
 		tpacpi_input_send_key(TP_ACPI_HOTKEYSCAN_DOUBLETAP);
 		return true;
 	}
@@ -11229,6 +11235,8 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 		/* Notify user space the profile changed */
 		platform_profile_notify();
 	}
+	if (hkey_event == TP_HKEY_EV_DOUBLETAP_TOGGLE)
+		tp_features.trackpoint_doubletap = !tp_features.trackpoint_doubletap;
 }
 
 static void hotkey_driver_event(const unsigned int scancode)
-- 
2.44.0

Re: [PATCH v2 2/4] platform/x86: thinkpad_acpi: Support for trackpoint doubletap

[Hans de Goede]

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);

Re: [PATCH v2 2/4] platform/x86: thinkpad_acpi: Support for trackpoint doubletap

[Hans de Goede]

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

Re: [PATCH v2 2/4] platform/x86: thinkpad_acpi: Support for trackpoint doubletap

[Mark Pearson]

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

Re: [PATCH v2 2/4] platform/x86: thinkpad_acpi: Support for trackpoint doubletap

[Hans de Goede]

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

Re: [PATCH v2 2/4] platform/x86: thinkpad_acpi: Support for trackpoint doubletap

[Mark Pearson]

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

Re: [PATCH v2 2/4] platform/x86: thinkpad_acpi: Support for trackpoint doubletap

[Hans de Goede]

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

Re: [PATCH v2 2/4] platform/x86: thinkpad_acpi: Support for trackpoint doubletap

[Mark Pearson]

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