Re: [PATCH 2/2] Input: atkbd - Fix TUXEDO NB02 notebook keyboards FN-keys

[Werner Sembach <wse@tuxedocomputers.com>]

Am 03.03.25 um 17:46 schrieb Mario Limonciello:
> On 3/3/2025 10:11, Werner Sembach wrote:
>> This small driver does 2 things:
>>
>> It remaps the touchpad toggle key from Control + Super + Hangaku/Zenkaku to
>> F21 to conform with established userspace defaults. Note that the
>> Hangaku/Zenkaku scancode used here is usually unused, with real
>> Hangaku/Zenkaku keys using the tilde scancode.
>>
>> It suppresses the reserved scancode produced by pressing the FN-key on its
>> own, which fixes a warning spamming the dmesg log otherwise.
>>
>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
>> ---
>>   MAINTAINERS                                 |  6 ++
>>   drivers/platform/x86/Kconfig                |  2 +
>>   drivers/platform/x86/Makefile               |  3 +
>>   drivers/platform/x86/tuxedo/Kbuild          |  6 ++
>>   drivers/platform/x86/tuxedo/Kconfig         |  6 ++
>>   drivers/platform/x86/tuxedo/nb02/Kbuild     |  7 ++
>>   drivers/platform/x86/tuxedo/nb02/Kconfig    | 15 ++++
>>   drivers/platform/x86/tuxedo/nb02/platform.c | 79 +++++++++++++++++++++
>>   8 files changed, 124 insertions(+)
>>   create mode 100644 drivers/platform/x86/tuxedo/Kbuild
>>   create mode 100644 drivers/platform/x86/tuxedo/Kconfig
>>   create mode 100644 drivers/platform/x86/tuxedo/nb02/Kbuild
>>   create mode 100644 drivers/platform/x86/tuxedo/nb02/Kconfig
>>   create mode 100644 drivers/platform/x86/tuxedo/nb02/platform.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 4ff26fa94895d..d3fbbcef813b0 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -24178,6 +24178,12 @@ T:    git 
>> git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git turbostat
>>   F:    tools/power/x86/turbostat/
>>   F:    tools/testing/selftests/turbostat/
>>   +TUXEDO DRIVERS
>> +M:    Werner Sembach <wse@tuxedocomputers.com>
>> +L:    platform-driver-x86@vger.kernel.org
>> +S:    Supported
>> +F:    drivers/platform/x86/tuxedo/
>> +
>>   TW5864 VIDEO4LINUX DRIVER
>>   M:    Bluecherry Maintainers <maintainers@bluecherrydvr.com>
>>   M:    Andrey Utkin <andrey.utkin@corp.bluecherry.net>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index 0258dd879d64b..9b78a1255c08e 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -1199,3 +1199,5 @@ config P2SB
>>         The main purpose of this library is to unhide P2SB device in case
>>         firmware kept it hidden on some platforms in order to access devices
>>         behind it.
>> +
>> +source "drivers/platform/x86/tuxedo/Kconfig"
>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>> index e1b1429470674..1562dcd7ad9a5 100644
>> --- a/drivers/platform/x86/Makefile
>> +++ b/drivers/platform/x86/Makefile
>> @@ -153,3 +153,6 @@ obj-$(CONFIG_WINMATE_FM07_KEYS)        += 
>> winmate-fm07-keys.o
>>     # SEL
>>   obj-$(CONFIG_SEL3350_PLATFORM)        += sel3350-platform.o
>> +
>> +# TUXEDO
>> +obj-y                    += tuxedo/
>> diff --git a/drivers/platform/x86/tuxedo/Kbuild 
>> b/drivers/platform/x86/tuxedo/Kbuild
>> new file mode 100644
>> index 0000000000000..e9c4243d438ba
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/Kbuild
>> @@ -0,0 +1,6 @@
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +#
>> +# TUXEDO X86 Platform Specific Drivers
>> +#
>> +
>> +obj-y    += nb02/
>> diff --git a/drivers/platform/x86/tuxedo/Kconfig 
>> b/drivers/platform/x86/tuxedo/Kconfig
>> new file mode 100644
>> index 0000000000000..e463f92135780
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/Kconfig
>> @@ -0,0 +1,6 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +#
>> +# TUXEDO X86 Platform Specific Drivers
>> +#
>> +
>> +source "drivers/platform/x86/tuxedo/nb02/Kconfig"
>> diff --git a/drivers/platform/x86/tuxedo/nb02/Kbuild 
>> b/drivers/platform/x86/tuxedo/nb02/Kbuild
>> new file mode 100644
>> index 0000000000000..8624a012cd683
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/nb02/Kbuild
>> @@ -0,0 +1,7 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +#
>> +# TUXEDO X86 Platform Specific Drivers
>> +#
>> +
>> +tuxedo_nb02_platform-y            := platform.o
>> +obj-$(CONFIG_TUXEDO_NB02_PLATFORM)    += tuxedo_nb02_platform.o
>> diff --git a/drivers/platform/x86/tuxedo/nb02/Kconfig 
>> b/drivers/platform/x86/tuxedo/nb02/Kconfig
>> new file mode 100644
>> index 0000000000000..848ab61164404
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/nb02/Kconfig
>> @@ -0,0 +1,15 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +#
>> +# TUXEDO X86 Platform Specific Drivers
>> +#
>> +
>> +menuconfig TUXEDO_NB02_PLATFORM
>> +    tristate "TUXEDO NB02 Platform Driver"
>> +    default m
>
> Don't most platform/x86 drivers default to 'n'?  It's up to distros and users 
> to turn on I thought.
Didn't give much tough to this particularly, will look into it
>
>> +    help
>> +      This driver implements miscellaneous things found on TUXEDO Notebooks
>> +      with board vendor NB02. For the time being this is only remapping the
>> +      touchpad toggle key to something supported by most Linux distros
>> +      out-of-the-box and supressing an unsupported scancode from the FN-key.
>
> suppressing
thanks for spotting
>
>> +
>> +      When compiled as a module it will be called tuxedo_nb02_platform.
>> diff --git a/drivers/platform/x86/tuxedo/nb02/platform.c 
>> b/drivers/platform/x86/tuxedo/nb02/platform.c
>> new file mode 100644
>> index 0000000000000..a6137c8b2d4aa
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/nb02/platform.c
>> @@ -0,0 +1,79 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2025 Werner Sembach wse@tuxedocomputers.com
>> + */
>> +
>> +#include <linux/i8042.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/serio.h>
>> +
>> +static u8 tux_nb02_touchp_toggle_seq[] = {
>> +    0xe0, 0x5b, // Super down
>> +    0x1d,       // Control down
>> +    0x76,       // Zenkaku/Hankaku down
>> +    0xf6,       // Zenkaku/Hankaku up
>> +    0x9d,       // Control up
>> +    0xe0, 0xdb  // Super up
>> +};
>> +
>> +static bool tux_nb02_i8042_filter(unsigned char data,
>> +                  unsigned char str,
>> +                  struct serio *port,
>> +                  __always_unused void *context)
>> +{
>> +    static u8 seq_pos;
>> +
>> +    if (unlikely(str & I8042_STR_AUXDATA))
>> +        return false;
>> +
>> +    pr_info("%#04x\n", data);
>
> A bit too noisy for a filter, no?
Yes, missed it when removing the debugging statements.
>
>> +
>> +    // Replace touchpad toggle key sequence with a singular press of the
>> +    // F21-key.
>
> Multi-line comments are supposed to be /* */
will fix
>
>> +    if (unlikely(data == tux_nb02_touchp_toggle_seq[seq_pos])) {
>> +        ++seq_pos;
>> +        if (seq_pos == ARRAY_SIZE(tux_nb02_touchp_toggle_seq)) {
>> +            seq_pos = 0;
>> +            serio_interrupt(port, 0x6c, 0); // F21 down
>> +            serio_interrupt(port, 0xec, 0); // F21 up
>> +        }
>> +        return true;
>> +    }
>> +
>> +    // Ignore bogus scancode produced by the FN-key. Reuse seq_pos as first
>> +    // byte of that is just the "extended"-byte.
>
> Multi-line comments are supposed to be /* */
will fix
>
>> +    if (unlikely(seq_pos == 1 && (data == 0x78 || data == 0xf8))) {
>> +        seq_pos = 0;
>> +        return true;
>> +    }
>> +
>> +    // Replay skipped sequence bytes if it did not finish and it was not a
>> +    // FN-key press.
>
> Multi-line comments are supposed to be /* */
will fix
>
>> +    if (unlikely(seq_pos)) {
>> +        for (u8 i; i < seq_pos; ++i)
>> +            serio_interrupt(port, tux_nb02_touchp_toggle_seq[i], 0);
>> +        seq_pos = 0;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +static int __init tux_nb02_plat_init(void)
>> +{
>> +    return i8042_install_filter(tux_nb02_i8042_filter, NULL);
>
> The driver is tri-state.  If it was 'm' it wouldn't load on anything but the 
> matching hardware.
>
> But think about what happens if a user compiled a kernel with this 'y'.
>
> The tux_nb02_plat_init() would install a filter even though they didn't have 
> matching hardware.
>
> I think you need some sort of assertion this driver matches the hardware and 
> return -ENODEV if it doesn't.
Ok
>
>> +}
>> +
>> +static void __exit tux_nb02_plat_exit(void)
>> +{
>> +    i8042_remove_filter(tux_nb02_i8042_filter);
>> +}
>> +
>> +module_init(tux_nb02_plat_init);
>> +module_exit(tux_nb02_plat_exit);
>> +
>> +MODULE_ALIAS("dmi:*:svnTUXEDO:*:rvnNB02:*");
>> +
>> +MODULE_DESCRIPTION("TUXEDO NB02 Platform");
>> +MODULE_AUTHOR("Werner Sembach <wse@tuxedocomputers.com>");
>> +MODULE_LICENSE("GPL");
>