Re: [ibm-acpi-devel] [PATCH 00/24] platform/x86: thinkpad_acpi: Refactor hotkey handling and add support for some new hotkeys

["Mark Pearson" <mpearson-lenovo@squebb.ca>]

On Sun, Apr 21, 2024, at 1:17 PM, Mark Pearson wrote:
> Thanks Hans!
>
> On Sun, Apr 21, 2024, at 11:44 AM, Hans de Goede wrote:
>> Hi All,
>>
>> My reply in the "[PATCH v2 1/4] platform/x86: thinkpad_acpi:
>> simplify known_ev handling" handling where I indicated that I would work
>> on converting the thinkpad_acpi hotkey handling to use sparse-keymaps
>> underestimated the work this required quite a bit.
>>
>> The hotkey code is quite old and crufty and full of special cases to
>> support many generations of ThinkPads, so I ended up doing some
>> significant refactoring and cleanup before doing the actual conversion
>> to sparse-keymaps.
>>
>> I have been vary careful to not introduce any changes wrt support for
>> the original ThinkPad models / hotkeys which use the hotkey_*_mask
>> related code.
>>
>> I've also done my best to avoid any *significant* functional change but
>> there are still some functional changes, which should all not impact
>> userspace compatibility:
>>
>> 1. Adaptive keyboard special keys will now also send EV_MSC events with
>>    the scancode, just like all the other hotkeys.
>>
>> 2. Rely on the input core for KEY_RESERVED suppression. This means that
>>    keys marked as KEY_RESERVED will still send EV_MSC evdev events when
>>    pressed (which helps users with mapping them to non reserved KEY_FOO
>>    values if desired).
>>
>> 3. Align the keycodes for volume up/down/mute and brightness up/down with
>>    those set by userspace through udev upstream's hwdb. Note these are all
>>    for keys which are suppressed by hotkey_reserved_mask by default.
>>    So this is only a functional change for users who override the default
>>    hotkey-mask *and* who do not have udev's default hwdb installed.
>>
>> 4. Suppress ACPI netlink event generation for unknown 0x1xxx hkey events to
>>    avoid userspace starting to rely on the netlink events for new hotkeys
>>    before these have been added to the keymap, only to have the netlink
>>    events get disabled by the adding of the new hotkeys to the keymap.
>>
>>    This should not cause a behavior change for existing models since all
>>    currently known 0x1xxx events have a mapping.
>>
>> Here is a quick breakdown of the patches in this series:
>>
>> Patch 1 - 2: Fix a small locking issue on rmmod the only problem here
>>    really is a lockdep warning, so I plan to route these fixes through
>>    for-next together with the rest to keep things simple.
>>
>> Patch 3 - 14: Do a bunch of cleanups and refactoring
>>
>> Patch 15: Implements functional change no 4. I really kinda want to just
>>    completely disable ACPI netlink event generation when also sending evdev
>>    events instead of reporting these twice. But for the 0x11xx / 0x13xx
>>    hkey events the kernel has send ACPI netlink events for years now. So
>>    this disables ACPI netlink events for any new hotkeys going forward.
>>
>> Patch 16 - 18: Refactor / cleanup reserved key handling
>>
>> Patch 19: Actually move to sparse-keymaps
>>
>> Patch 20: Update the keymap for 2 adaptive kbd Fn row keys
>>
>> Patch 21 - 24: Mark's original patches adding support for the new Fn + N
>>    key combo and for trackpoint doubletap slightly reworked to use
>>    the new sparse-keymap.
>>
>> Mark if you can make some time to review patches 1-20 that would be great.
>>
> Definitely will do.
>
> I'll do some testing on platforms here as much as I can. If there's 
> anything in particular that you think is worth taking extra care over 
> let me know (with a caveat that my team doesn't have all the platforms, 
> and for anything older than 5 years it can be hard to get hold of them)
>
> Thanks for your work on this.
>
> Mark
>
I've tested the series on a couple of platforms (Z13 G2 and X1 Carbon G12) and so far all looking good.
 - tried all hotkey combinations that are supported that I can think of and they work - including brightness control, volume control, platform profile toggle, airplane, snipping tool and privacy screen.
 - trackpoint double tap is working well on the Z13 G2 (set up custom key setting in gnome-settings and launched browser on a doubletap)
 - FN+N is working well on the X1 Carbon G12 (tested with evtest to confirm vendor key generated)

So for the series:
Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>

I have read through the patches, but not in enough depth for it to count as a review yet. Need a bit more time for that.

Mark