All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Mark Pearson <mpearson-lenovo@squebb.ca>
Cc: ilpo.jarvinen@linux.intel.com, hmh@hmh.eng.br,
	ibm-acpi-devel@lists.sourceforge.net,
	platform-driver-x86@vger.kernel.org,
	linux-kernel@vger.kernel.org, njoshi1@lenovo.com,
	vsankar@lenovo.com, peter.hutterer@redhat.com,
	Vishnu Sankar <vishnuocv@gmail.com>
Subject: Re: [PATCH v2 2/4] platform/x86: thinkpad_acpi: Support for trackpoint doubletap
Date: Wed, 17 Apr 2024 22:06:09 +0200	[thread overview]
Message-ID: <55ded7c3-fbc5-4fa5-8b63-da4d7aa4966c@redhat.com> (raw)
In-Reply-To: <98082080-0fcf-470f-afa5-76ec2bbffee7@redhat.com>

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


  reply	other threads:[~2024-04-17 20:06 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-17 17:31 [PATCH v2 1/4] platform/x86: thinkpad_acpi: simplify known_ev handling Mark Pearson
2024-04-17 17:31 ` [PATCH v2 2/4] platform/x86: thinkpad_acpi: Support for trackpoint doubletap Mark Pearson
2024-04-17 19:39   ` Hans de Goede
2024-04-17 20:06     ` Hans de Goede [this message]
2024-04-17 23:57       ` Mark Pearson
2024-04-18 11:34         ` Hans de Goede
2024-04-18 12:24           ` Mark Pearson
2024-04-18 14:42             ` Hans de Goede
2024-04-18 15:03               ` Mark Pearson
2024-04-17 17:31 ` [PATCH v2 3/4] platform/x86: thinkpad_acpi: Support for system debug info hotkey Mark Pearson
2024-04-17 17:31 ` [PATCH v2 4/4] platform/x86: thinkpad_acpi: Support hotkey to disable trackpoint doubletap Mark Pearson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=55ded7c3-fbc5-4fa5-8b63-da4d7aa4966c@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=hmh@hmh.eng.br \
    --cc=ibm-acpi-devel@lists.sourceforge.net \
    --cc=ilpo.jarvinen@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mpearson-lenovo@squebb.ca \
    --cc=njoshi1@lenovo.com \
    --cc=peter.hutterer@redhat.com \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=vishnuocv@gmail.com \
    --cc=vsankar@lenovo.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.