Re: [PATCH v2 22/24] platform/x86: thinkpad_acpi: Support for trackpoint doubletap

[Hans de Goede <hdegoede@redhat.com>]

Hi Mark,

On 4/24/24 8:19 PM, Mark Pearson wrote:
> Hi Hans,
> 
> On Wed, Apr 24, 2024, at 8:28 AM, Hans de Goede wrote:
>> From: Mark Pearson <mpearson-lenovo@squebb.ca>
>>
>> Lenovo trackpoints are adding the ability to generate a doubletap event.
>> This handles the doubletap event and sends the KEY_PROG4 event to
>> userspace. Despite the driver itself not using KEY_PROG1 - KEY_PROG3 this
>> still uses KEY_PROG4 because of some keys being remapped to KEY_PROG1 -
>> KEY_PROG3 by default by the upstream udev hwdb containing:
>>
>> evdev:name:ThinkPad Extra Buttons:dmi:bvn*:bvr*:bd*:svnLENOVO*:pn*:*
>>  ...
>>  KEYBOARD_KEY_17=prog1
>>  KEYBOARD_KEY_1a=f20       # Microphone mute button
>>  KEYBOARD_KEY_45=bookmarks
>>  KEYBOARD_KEY_46=prog2     # Fn + PrtSc, on Windows: Snipping tool
>>  KEYBOARD_KEY_4a=prog3     # Fn + Right shift, on Windows: No idea
>>
>> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>
>> Link: https://lore.kernel.org/r/20240417173124.9953-2-mpearson-lenovo@squebb.ca
>> [hdegoede@redhat.com: Adjust for switch to sparse-keymap keymaps]
>> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c 
>> b/drivers/platform/x86/thinkpad_acpi.c
>> index a53b00fecf1a..b6d6466215e1 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -248,6 +248,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 */
>>  };
>>
>>  
>> /****************************************************************************
>> @@ -3268,6 +3271,7 @@ static const struct key_entry keymap_lenovo[] 
>> __initconst = {
>>  	 * after switching to sparse keymap support. The mappings above use 
>> translated
>>  	 * scancodes to preserve uAPI compatibility, see 
>> tpacpi_input_send_key().
>>  	 */
>> +	{ KE_KEY, TP_HKEY_EV_TRACK_DOUBLETAP /* 0x8036 */, { KEY_PROG4 } },
>>  	{ KE_END }
>>  };
>>
>> @@ -3817,6 +3821,17 @@ static bool hotkey_notify_6xxx(const u32 hkey, 
>> bool *send_acpi_ev)
>>  	return true;
>>  }
>>
>> +static bool hotkey_notify_8xxx(const u32 hkey, bool *send_acpi_ev)
>> +{
>> +	switch (hkey) {
>> +	case TP_HKEY_EV_TRACK_DOUBLETAP:
>> +		tpacpi_input_send_key(hkey, send_acpi_ev);
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>>  static void hotkey_notify(struct ibm_struct *ibm, u32 event)
>>  {
>>  	u32 hkey;
>> @@ -3893,6 +3908,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, &send_acpi_ev);
>> +			break;
>>  		}
>>  		if (!known_ev) {
>>  			pr_notice("unhandled HKEY event 0x%04x\n", hkey);
>> -- 
>> 2.44.0
> 
> Instead of needing hotkey_notify_8xxx, now we are using the sparse_keymap can we just use hotkey_notify_hotkey for case 8? No need to check what the hkey is either.

I prefer to keep things consistent and have each case #: path call a separate
helper for those #xxx codes.

ATM some of the simpler cases handle things directly, but as more handling for
different events get added that becomes a bit messy IMHO. I would actually
like to see those other cases converted to use a small helper function too
(with a switch-case in the helper for future proofing) to make things consistent.

Patches to do this small cleanup are welcome.

Regards,

Hans