All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Mark Pearson" <mpearson-lenovo@squebb.ca>
To: "Hans de Goede" <hdegoede@redhat.com>
Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>,
	"Henrique de Moraes Holschuh" <hmh@hmh.eng.br>,
	ibm-acpi-devel@lists.sourceforge.net,
	"platform-driver-x86@vger.kernel.org"
	<platform-driver-x86@vger.kernel.org>,
	linux-kernel@vger.kernel.org, "Nitin Joshi1" <njoshi1@lenovo.com>,
	"Vishnu Sankar" <vsankar@lenovo.com>,
	"Peter Hutterer" <peter.hutterer@redhat.com>,
	"Vishnu Sankar" <vishnuocv@gmail.com>
Subject: Re: [PATCH v2 2/4] platform/x86: thinkpad_acpi: Support for trackpoint doubletap
Date: Thu, 18 Apr 2024 08:24:40 -0400	[thread overview]
Message-ID: <0917e5bc-a198-4aa8-812e-31434408e78d@app.fastmail.com> (raw)
In-Reply-To: <76d92fdc-ad0a-40a2-9e1b-d550f8e07267@redhat.com>

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


  reply	other threads:[~2024-04-18 12:23 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
2024-04-17 23:57       ` Mark Pearson
2024-04-18 11:34         ` Hans de Goede
2024-04-18 12:24           ` Mark Pearson [this message]
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=0917e5bc-a198-4aa8-812e-31434408e78d@app.fastmail.com \
    --to=mpearson-lenovo@squebb.ca \
    --cc=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=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.