* Re: [PATCH v11 10/11] platform/x86: asus-wmi: add keyboard brightness event handler
From: Denis Benato @ 2026-01-17 13:45 UTC (permalink / raw)
To: Antheas Kapenekakis, platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen
In-Reply-To: <20260116133150.5606-11-lkml@antheas.dev>
On 1/16/26 14:31, Antheas Kapenekakis wrote:
> The keyboard brightness control of Asus WMI keyboards is handled in
> kernel, which leads to the shortcut going from brightness 0, to 1,
> to 2, and 3.
>
> However, for HID keyboards it is exposed as a key and handled by the
> user's desktop environment. For the toggle button, this means that
> brightness control becomes on/off. In addition, in the absence of a
> DE, the keyboard brightness does not work.
>
> Therefore, expose an event handler for the keyboard brightness control
> which can then be used by hid-asus. Since this handler is called from
> an interrupt context, defer the actual work to a workqueue.
>
> In the process, introduce ASUS_EV_MAX_BRIGHTNESS to hold the constant
> for maximum brightness since it is shared between hid-asus/asus-wmi.
Reviewed-by: Denis Benato <benato.denis96@gmail.com>
> Reviewed-by: Luke D. Jones <luke@ljones.dev>
> Tested-by: Luke D. Jones <luke@ljones.dev>
> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
> drivers/platform/x86/asus-wmi.c | 46 +++++++++++++++++++---
> include/linux/platform_data/x86/asus-wmi.h | 13 ++++++
> 2 files changed, 54 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index df2365efb2b8..e65d91a11000 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -1719,6 +1719,44 @@ static void kbd_led_update_all(struct work_struct *work)
> }
> }
>
> +/*
> + * This function is called from hid-asus to inform asus-wmi of brightness
> + * changes initiated by the keyboard backlight keys.
> + */
> +int asus_hid_event(enum asus_hid_event event)
> +{
> + struct asus_wmi *asus;
> + int brightness;
> +
> + guard(spinlock_irqsave)(&asus_ref.lock);
> + asus = asus_ref.asus;
> + if (!asus || !asus->kbd_led_registered)
> + return -EBUSY;
> +
> + brightness = asus->kbd_led_wk;
> +
> + switch (event) {
> + case ASUS_EV_BRTUP:
> + brightness += 1;
> + break;
> + case ASUS_EV_BRTDOWN:
> + brightness -= 1;
> + break;
> + case ASUS_EV_BRTTOGGLE:
> + if (brightness >= ASUS_EV_MAX_BRIGHTNESS)
> + brightness = 0;
> + else
> + brightness += 1;
> + break;
> + }
> +
> + asus->kbd_led_wk = clamp_val(brightness, 0, ASUS_EV_MAX_BRIGHTNESS);
> + asus->kbd_led_notify = true;
> + queue_work(asus->led_workqueue, &asus->kbd_led_work);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(asus_hid_event);
> +
> /*
> * These functions actually update the LED's, and are called from a
> * workqueue. By doing this as separate work rather than when the LED
> @@ -1801,13 +1839,11 @@ static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
> {
> struct asus_hid_listener *listener;
> struct asus_wmi *asus;
> - int max_level;
>
> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
> - max_level = asus->kbd_led.max_brightness;
>
> scoped_guard(spinlock_irqsave, &asus_ref.lock)
> - asus->kbd_led_wk = clamp_val(value, 0, max_level);
> + asus->kbd_led_wk = clamp_val(value, 0, ASUS_EV_MAX_BRIGHTNESS);
>
> if (asus->kbd_led_avail)
> kbd_led_update(asus);
> @@ -2011,7 +2047,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
> asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> asus->kbd_led.brightness_set_blocking = kbd_led_set;
> asus->kbd_led.brightness_get = kbd_led_get;
> - asus->kbd_led.max_brightness = 3;
> + asus->kbd_led.max_brightness = ASUS_EV_MAX_BRIGHTNESS;
> asus->kbd_led_avail = !kbd_led_read(asus, &led_val, NULL);
> INIT_WORK(&asus->kbd_led_work, kbd_led_update_all);
>
> @@ -4530,7 +4566,7 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
> return;
> }
> if (code == NOTIFY_KBD_BRTTOGGLE) {
> - if (led_value == asus->kbd_led.max_brightness)
> + if (led_value == ASUS_EV_MAX_BRIGHTNESS)
> kbd_led_set_by_kbd(asus, 0);
> else
> kbd_led_set_by_kbd(asus, led_value + 1);
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index d347cffd05d5..7b872b5d0960 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -178,6 +178,14 @@ struct asus_hid_listener {
> void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
> };
>
> +enum asus_hid_event {
> + ASUS_EV_BRTUP,
> + ASUS_EV_BRTDOWN,
> + ASUS_EV_BRTTOGGLE,
> +};
> +
> +#define ASUS_EV_MAX_BRIGHTNESS 3
> +
> #if IS_REACHABLE(CONFIG_ASUS_WMI)
> void set_ally_mcu_hack(enum asus_ally_mcu_hack status);
> void set_ally_mcu_powersave(bool enabled);
> @@ -186,6 +194,7 @@ int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval);
> int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
> int asus_hid_register_listener(struct asus_hid_listener *cdev);
> void asus_hid_unregister_listener(struct asus_hid_listener *cdev);
> +int asus_hid_event(enum asus_hid_event event);
> #else
> static inline void set_ally_mcu_hack(enum asus_ally_mcu_hack status)
> {
> @@ -213,6 +222,10 @@ static inline int asus_hid_register_listener(struct asus_hid_listener *bdev)
> static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
> {
> }
> +static inline int asus_hid_event(enum asus_hid_event event)
> +{
> + return -ENODEV;
> +}
> #endif
>
> #endif /* __PLATFORM_DATA_X86_ASUS_WMI_H */
^ permalink raw reply
* Re: [PATCH v11 07/11] platform/x86: asus-wmi: Add support for multiple kbd led handlers
From: Antheas Kapenekakis @ 2026-01-17 13:49 UTC (permalink / raw)
To: Denis Benato
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
In-Reply-To: <3354c446-3e1c-40c7-ac08-43b3ef630d91@linux.dev>
On Sat, 17 Jan 2026 at 14:16, Denis Benato <denis.benato@linux.dev> wrote:
>
>
> On 1/16/26 14:31, Antheas Kapenekakis wrote:
> > Some devices, such as the Z13 have multiple Aura devices connected
> > to them by USB. In addition, they might have a WMI interface for
> > RGB. In Windows, Armoury Crate exposes a unified brightness slider
> > for all of them, with 3 brightness levels.
> >
> > Therefore, to be synergistic in Linux, and support existing tooling
> > such as UPower, allow adding listeners to the RGB device of the WMI
> > interface. If WMI does not exist, lazy initialize the interface.
> >
> > Since hid-asus and asus-wmi can both interact with the led objects
> > including from an atomic context, protect the brightness access with a
> > spinlock and update the values from a workqueue. Use this workqueue to
> > also process WMI keyboard events, so they are handled asynchronously.
> >
> > Acked-by: Benjamin Tissoires <bentiss@kernel.org>
> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > ---
> > drivers/platform/x86/asus-wmi.c | 183 ++++++++++++++++++---
> > include/linux/platform_data/x86/asus-wmi.h | 15 ++
> > 2 files changed, 173 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> > index 4aec7ec69250..df2365efb2b8 100644
> > --- a/drivers/platform/x86/asus-wmi.c
> > +++ b/drivers/platform/x86/asus-wmi.c
> > @@ -31,13 +31,13 @@
> > #include <linux/pci.h>
> > #include <linux/pci_hotplug.h>
> > #include <linux/platform_data/x86/asus-wmi.h>
> > -#include <linux/platform_data/x86/asus-wmi-leds-ids.h>
> > #include <linux/platform_device.h>
> > #include <linux/platform_profile.h>
> > #include <linux/power_supply.h>
> > #include <linux/rfkill.h>
> > #include <linux/seq_file.h>
> > #include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > #include <linux/types.h>
> > #include <linux/units.h>
> >
> > @@ -256,6 +256,9 @@ struct asus_wmi {
> > int tpd_led_wk;
> > struct led_classdev kbd_led;
> > int kbd_led_wk;
> > + bool kbd_led_notify;
> > + bool kbd_led_avail;
> > + bool kbd_led_registered;
> > struct led_classdev lightbar_led;
> > int lightbar_led_wk;
> > struct led_classdev micmute_led;
> > @@ -264,6 +267,7 @@ struct asus_wmi {
> > struct work_struct tpd_led_work;
> > struct work_struct wlan_led_work;
> > struct work_struct lightbar_led_work;
> > + struct work_struct kbd_led_work;
> >
> > struct asus_rfkill wlan;
> > struct asus_rfkill bluetooth;
> > @@ -1615,6 +1619,106 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
> >
> > /* LEDs ***********************************************************************/
> >
> > +struct asus_hid_ref {
> > + struct list_head listeners;
> > + struct asus_wmi *asus;
> > + /* Protects concurrent access from hid-asus and asus-wmi to leds */
> > + spinlock_t lock;
> > +};
> > +
> > +static struct asus_hid_ref asus_ref = {
> > + .listeners = LIST_HEAD_INIT(asus_ref.listeners),
> > + .asus = NULL,
> > + /*
> > + * Protects .asus, .asus.kbd_led_{wk,notify}, and .listener refs. Other
> > + * asus variables are read-only after .asus is set.
> > + *
> > + * The led cdev device is not protected because it calls backlight_get
> > + * during initialization, which would result in a nested lock attempt.
> > + *
> > + * The led cdev is safe to access without a lock because if
> > + * kbd_led_avail is true it is initialized before .asus is set and never
> > + * changed until .asus is dropped. If kbd_led_avail is false, the led
> > + * cdev is registered by the workqueue, which is single-threaded and
> > + * cancelled before asus-wmi would access the led cdev to unregister it.
> > + *
> > + * A spinlock is used, because the protected variables can be accessed
> > + * from an IRQ context from asus-hid.
> > + */
> > + .lock = __SPIN_LOCK_UNLOCKED(asus_ref.lock),
> > +};
> > +
> > +/*
> > + * Allows registering hid-asus listeners that want to be notified of
> > + * keyboard backlight changes.
> > + */
> > +int asus_hid_register_listener(struct asus_hid_listener *bdev)
> > +{
> > + struct asus_wmi *asus;
> > +
> > + guard(spinlock_irqsave)(&asus_ref.lock);
> > + list_add_tail(&bdev->list, &asus_ref.listeners);
> > + asus = asus_ref.asus;
> > + if (asus)
> > + queue_work(asus->led_workqueue, &asus->kbd_led_work);
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_hid_register_listener);
> > +
> > +/*
> > + * Allows unregistering hid-asus listeners that were added with
> > + * asus_hid_register_listener().
> > + */
> > +void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
> > +{
> > + guard(spinlock_irqsave)(&asus_ref.lock);
> > + list_del(&bdev->list);
> > +}
> > +EXPORT_SYMBOL_GPL(asus_hid_unregister_listener);
> > +
> > +static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
> > +
> > +static void kbd_led_update_all(struct work_struct *work)
> > +{
> > + struct asus_wmi *asus;
> > + bool registered, notify;
> > + int ret, value;
> > +
> > + asus = container_of(work, struct asus_wmi, kbd_led_work);
> > +
> > + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> > + registered = asus->kbd_led_registered;
> > + value = asus->kbd_led_wk;
> > + notify = asus->kbd_led_notify;
> > + }
> > +
> > + if (!registered) {
> > + /*
> > + * This workqueue runs under asus-wmi, which means probe has
> > + * completed and asus-wmi will keep running until it finishes.
> > + * Therefore, we can safely register the LED without holding
> > + * a spinlock.
> > + */
> > + ret = devm_led_classdev_register(&asus->platform_device->dev,
> > + &asus->kbd_led);
> > + if (!ret) {
> > + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> > + asus->kbd_led_registered = true;
> > + } else {
> > + pr_warn("Failed to register keyboard backlight LED: %d\n", ret);
> > + return;
> > + }
> > + }
> > +
> > + if (value >= 0)
> > + do_kbd_led_set(&asus->kbd_led, value);
> > + if (notify) {
> > + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> > + asus->kbd_led_notify = false;
> > + led_classdev_notify_brightness_hw_changed(&asus->kbd_led, value);
> > + }
> > +}
> > +
> > /*
> > * These functions actually update the LED's, and are called from a
> > * workqueue. By doing this as separate work rather than when the LED
> > @@ -1661,7 +1765,8 @@ static void kbd_led_update(struct asus_wmi *asus)
> > {
> > int ctrl_param = 0;
> >
> > - ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
> > + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> > + ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
> > asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
> > }
> >
> > @@ -1694,14 +1799,23 @@ static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
> >
> > static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
> > {
> > + struct asus_hid_listener *listener;
> > struct asus_wmi *asus;
> > int max_level;
> >
> > asus = container_of(led_cdev, struct asus_wmi, kbd_led);
> > max_level = asus->kbd_led.max_brightness;
> >
> > - asus->kbd_led_wk = clamp_val(value, 0, max_level);
> > - kbd_led_update(asus);
> > + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> > + asus->kbd_led_wk = clamp_val(value, 0, max_level);
> > +
> > + if (asus->kbd_led_avail)
> > + kbd_led_update(asus);
> > +
> > + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> > + list_for_each_entry(listener, &asus_ref.listeners, list)
> > + listener->brightness_set(listener, asus->kbd_led_wk);
> > + }
> > }
> >
> > static int kbd_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> > @@ -1716,10 +1830,11 @@ static int kbd_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> >
> > static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
> > {
> > - struct led_classdev *led_cdev = &asus->kbd_led;
> > -
> > - do_kbd_led_set(led_cdev, value);
> > - led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
> > + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> > + asus->kbd_led_wk = value;
> > + asus->kbd_led_notify = true;
> > + }
> > + queue_work(asus->led_workqueue, &asus->kbd_led_work);
> > }
> >
> > static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
> > @@ -1729,10 +1844,18 @@ static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
> >
> > asus = container_of(led_cdev, struct asus_wmi, kbd_led);
> >
> > + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> > + if (!asus->kbd_led_avail)
> > + return asus->kbd_led_wk;
> > + }
> > +
> > retval = kbd_led_read(asus, &value, NULL);
> > if (retval < 0)
> > return retval;
> >
> > + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> > + asus->kbd_led_wk = value;
> > +
> > return value;
> > }
> >
> > @@ -1844,7 +1967,9 @@ static int camera_led_set(struct led_classdev *led_cdev,
> >
> > static void asus_wmi_led_exit(struct asus_wmi *asus)
> > {
> > - led_classdev_unregister(&asus->kbd_led);
> > + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> > + asus_ref.asus = NULL;
> > +
> > led_classdev_unregister(&asus->tpd_led);
> > led_classdev_unregister(&asus->wlan_led);
> > led_classdev_unregister(&asus->lightbar_led);
> > @@ -1882,22 +2007,26 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
> > goto error;
> > }
> >
> > - if (!kbd_led_read(asus, &led_val, NULL) && !dmi_check_system(asus_use_hid_led_dmi_ids)) {
> > - pr_info("using asus-wmi for asus::kbd_backlight\n");
> > - asus->kbd_led_wk = led_val;
> > - asus->kbd_led.name = "asus::kbd_backlight";
> > - asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> > - asus->kbd_led.brightness_set_blocking = kbd_led_set;
> > - asus->kbd_led.brightness_get = kbd_led_get;
> > - asus->kbd_led.max_brightness = 3;
> > + asus->kbd_led.name = "asus::kbd_backlight";
> > + asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> > + asus->kbd_led.brightness_set_blocking = kbd_led_set;
> > + asus->kbd_led.brightness_get = kbd_led_get;
> > + asus->kbd_led.max_brightness = 3;
> > + asus->kbd_led_avail = !kbd_led_read(asus, &led_val, NULL);
> > + INIT_WORK(&asus->kbd_led_work, kbd_led_update_all);
> >
> > + if (asus->kbd_led_avail) {
> > + asus->kbd_led_wk = led_val;
> > if (num_rgb_groups != 0)
> > asus->kbd_led.groups = kbd_rgb_mode_groups;
> > + } else {
> > + asus->kbd_led_wk = -1;
> > + }
> >
> > - rv = led_classdev_register(&asus->platform_device->dev,
> > - &asus->kbd_led);
> > - if (rv)
> > - goto error;
> > + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> > + asus_ref.asus = asus;
> > + if (asus->kbd_led_avail || !list_empty(&asus_ref.listeners))
> > + queue_work(asus->led_workqueue, &asus->kbd_led_work);
> > }
> >
> > if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_WIRELESS_LED)
> > @@ -4372,6 +4501,7 @@ static int asus_wmi_get_event_code(union acpi_object *obj)
> >
> > static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
> > {
> > + enum led_brightness led_value;
> > unsigned int key_value = 1;
> > bool autorelease = 1;
> >
> > @@ -4388,19 +4518,22 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
> > return;
> > }
> >
> > + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> > + led_value = asus->kbd_led_wk;
> > +
> > if (code == NOTIFY_KBD_BRTUP) {
> > - kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
> > + kbd_led_set_by_kbd(asus, led_value + 1);
> > return;
> > }
> > if (code == NOTIFY_KBD_BRTDWN) {
> > - kbd_led_set_by_kbd(asus, asus->kbd_led_wk - 1);
> > + kbd_led_set_by_kbd(asus, led_value - 1);
> > return;
> > }
> > if (code == NOTIFY_KBD_BRTTOGGLE) {
> > - if (asus->kbd_led_wk == asus->kbd_led.max_brightness)
> > + if (led_value == asus->kbd_led.max_brightness)
> > kbd_led_set_by_kbd(asus, 0);
> This is the toggle leds button, right? I would expect that pressing the toggle
> button turns off leds if they are on and turns them on if they are off.
>
> so if (led_value > 0) { .... }.
>
> I see the previous code was equivalent to yours but is that what we want?
It is common to do 0->1->2->3->0 for the toggle. This is what is
currently done for WMI Asus keyboards and e.g., Thinkpads. This patch
unifies the behavior for USB keyboards too.
I would argue it is better, as you do not need to reach for a
userspace slider to set a lower brightness.
The current behavior of KDE is 0->3->0 and if the event goes to
userspace this is what happens currently. Unless the keyboard
reconnects, where brightness just stops working afterwards because
upower only probes at boot (I have a follow patch to fix this for
Duos).
Antheas
> > else
> > - kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
> > + kbd_led_set_by_kbd(asus, led_value + 1);
> > return;
> > }
> >
> > diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> > index 419491d4abca..d347cffd05d5 100644
> > --- a/include/linux/platform_data/x86/asus-wmi.h
> > +++ b/include/linux/platform_data/x86/asus-wmi.h
> > @@ -172,12 +172,20 @@ enum asus_ally_mcu_hack {
> > ASUS_WMI_ALLY_MCU_HACK_DISABLED,
> > };
> >
> > +/* Used to notify hid-asus when asus-wmi changes keyboard backlight */
> > +struct asus_hid_listener {
> > + struct list_head list;
> > + void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
> > +};
> > +
> > #if IS_REACHABLE(CONFIG_ASUS_WMI)
> > void set_ally_mcu_hack(enum asus_ally_mcu_hack status);
> > void set_ally_mcu_powersave(bool enabled);
> > int asus_wmi_get_devstate_dsts(u32 dev_id, u32 *retval);
> > int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval);
> > int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
> > +int asus_hid_register_listener(struct asus_hid_listener *cdev);
> > +void asus_hid_unregister_listener(struct asus_hid_listener *cdev);
> > #else
> > static inline void set_ally_mcu_hack(enum asus_ally_mcu_hack status)
> > {
> > @@ -198,6 +206,13 @@ static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
> > {
> > return -ENODEV;
> > }
> > +static inline int asus_hid_register_listener(struct asus_hid_listener *bdev)
> > +{
> > + return -ENODEV;
> > +}
> > +static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
> > +{
> > +}
> > #endif
> >
> > #endif /* __PLATFORM_DATA_X86_ASUS_WMI_H */
>
^ permalink raw reply
* Re: [PATCH v11 02/11] HID: asus: initialize additional endpoints only for legacy devices
From: Denis Benato @ 2026-01-17 13:51 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
In-Reply-To: <CAGwozwGyUpBq4GGvyDHj089a9-vxNOnqgSBys3-CC_+tKDywaA@mail.gmail.com>
On 1/17/26 00:10, Antheas Kapenekakis wrote:
> On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@linux.dev> wrote:
>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
>>
>>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
>>> However, ID1 initializations are only required for RGB control and are
>>> only supported for RGB capable devices. ID2 initializations are only
>>> required for initializing the Anime display endpoint which is only
>>> supported on devices with an Anime display. Both of these
>>> initializations are out of scope for this driver (this is a brightness
>>> control and keyboard shortcut driver) and they should not be performed
>>> for devices that do not support them in any case.
>>>
>>> At the same time, there are older NKEY devices that have only been
>>> tested with these initializations in the kernel and it is not possible
>>> to recheck them. There is a possibility that especially with the ID1
>>> initialization, certain laptop models might have their shortcuts stop
>>> working (currently unproven).
>>>
>>> For an abundance of caution, only initialize ID1/ID2 for those older
>>> NKEY devices by introducing a quirk for them and replacing the NKEY
>>> quirk in the block that performs the inits with that.
>>>
>>> In addition, as these initializations might not be supported by the
>>> affected devices, change the function to not bail if they fail.
>>>
>>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>> ---
>>> drivers/hid/hid-asus.c | 16 ++++++----------
>>> 1 file changed, 6 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>> index 323e6302bac5..dc7af12cf31a 100644
>>> --- a/drivers/hid/hid-asus.c
>>> +++ b/drivers/hid/hid-asus.c
>>> @@ -90,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>>> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
>>> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
>>> #define QUIRK_ROG_ALLY_XPAD BIT(13)
>>> +#define QUIRK_ROG_NKEY_LEGACY BIT(14)
>> These past days I have taken a look at new 2025 models and they do make use of ID2,
>> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
> Hi Denis,
> it is not the responsibility of this driver. ID2 is used by Anime
> models. It is a concession to make sure that we do not cause a
> regression that will cause warnings for a lot of users.
Who decided it is a concession?
Anyway I will move relevant code tied to these two to this driver,
so it doesn't make sense to remove them anyway.
>> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
>> No consequences.
> In your laptop. In the other user's laptop, the get feature report fails
for the response to be a failure (as it is supposed to be in mine and other models)
and to cause problems are two different things. Here I am saying that the hardware
correctly reports "unsupported" and nothing bad happens (if you ignore the return value).
>> Regardless the name is wrong: mine is a 2023 rog strix with
>> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
>> and surely isn't legacy.
> Sure, can you try removing the if block?
I have asked to distribute a kernel that init ID1 and ID2 regardless
of that quirk. We will soon know if it causes problems or not.
> If it works in your laptop, that is one less reason to keep it for 19b6
If it works in my laptop one more reason not to exclude code that
works and haven't caused any problem ever.
> Antheas
>
>>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
>>> QUIRK_NO_INIT_REPORTS | \
>>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>> return -ENODEV;
>>>
>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>> - if (ret < 0)
>>> - return ret;
>>> -
>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>> - if (ret < 0)
>>> - return ret;
>>> + if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>> }
>>>
>>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
>>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
>>> QUIRK_USE_KBD_BACKLIGHT },
>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
>>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@linux.dev> wrote:
>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
>>
>>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
>>> However, ID1 initializations are only required for RGB control and are
>>> only supported for RGB capable devices. ID2 initializations are only
>>> required for initializing the Anime display endpoint which is only
>>> supported on devices with an Anime display. Both of these
>>> initializations are out of scope for this driver (this is a brightness
>>> control and keyboard shortcut driver) and they should not be performed
>>> for devices that do not support them in any case.
>>>
>>> At the same time, there are older NKEY devices that have only been
>>> tested with these initializations in the kernel and it is not possible
>>> to recheck them. There is a possibility that especially with the ID1
>>> initialization, certain laptop models might have their shortcuts stop
>>> working (currently unproven).
>>>
>>> For an abundance of caution, only initialize ID1/ID2 for those older
>>> NKEY devices by introducing a quirk for them and replacing the NKEY
>>> quirk in the block that performs the inits with that.
>>>
>>> In addition, as these initializations might not be supported by the
>>> affected devices, change the function to not bail if they fail.
>>>
>>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>> ---
>>> drivers/hid/hid-asus.c | 16 ++++++----------
>>> 1 file changed, 6 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>> index 323e6302bac5..dc7af12cf31a 100644
>>> --- a/drivers/hid/hid-asus.c
>>> +++ b/drivers/hid/hid-asus.c
>>> @@ -90,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>>> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
>>> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
>>> #define QUIRK_ROG_ALLY_XPAD BIT(13)
>>> +#define QUIRK_ROG_NKEY_LEGACY BIT(14)
>> These past days I have taken a look at new 2025 models and they do make use of ID2,
>> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
>>
>> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
>> No consequences.
>>
>> Regardless the name is wrong: mine is a 2023 rog strix with
>> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
>> and surely isn't legacy.
>>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
>>> QUIRK_NO_INIT_REPORTS | \
>>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>> return -ENODEV;
>>>
>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>> - if (ret < 0)
>>> - return ret;
>>> -
>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>> - if (ret < 0)
>>> - return ret;
>>> + if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>> }
>>>
>>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
>>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
>>> QUIRK_USE_KBD_BACKLIGHT },
>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
>>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
^ permalink raw reply
* Re: [PATCH v11 07/11] platform/x86: asus-wmi: Add support for multiple kbd led handlers
From: Antheas Kapenekakis @ 2026-01-17 13:52 UTC (permalink / raw)
To: Denis Benato
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
In-Reply-To: <CAGwozwF0KaG0VJDJSPWfX8DVty-nZ+m8_ugDOk9wwHr+UbvYQA@mail.gmail.com>
On Sat, 17 Jan 2026 at 14:49, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>
> On Sat, 17 Jan 2026 at 14:16, Denis Benato <denis.benato@linux.dev> wrote:
> >
> >
> > On 1/16/26 14:31, Antheas Kapenekakis wrote:
> > > Some devices, such as the Z13 have multiple Aura devices connected
> > > to them by USB. In addition, they might have a WMI interface for
> > > RGB. In Windows, Armoury Crate exposes a unified brightness slider
> > > for all of them, with 3 brightness levels.
> > >
> > > Therefore, to be synergistic in Linux, and support existing tooling
> > > such as UPower, allow adding listeners to the RGB device of the WMI
> > > interface. If WMI does not exist, lazy initialize the interface.
> > >
> > > Since hid-asus and asus-wmi can both interact with the led objects
> > > including from an atomic context, protect the brightness access with a
> > > spinlock and update the values from a workqueue. Use this workqueue to
> > > also process WMI keyboard events, so they are handled asynchronously.
> > >
> > > Acked-by: Benjamin Tissoires <bentiss@kernel.org>
> > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > > ---
> > > drivers/platform/x86/asus-wmi.c | 183 ++++++++++++++++++---
> > > include/linux/platform_data/x86/asus-wmi.h | 15 ++
> > > 2 files changed, 173 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> > > index 4aec7ec69250..df2365efb2b8 100644
> > > --- a/drivers/platform/x86/asus-wmi.c
> > > +++ b/drivers/platform/x86/asus-wmi.c
> > > @@ -31,13 +31,13 @@
> > > #include <linux/pci.h>
> > > #include <linux/pci_hotplug.h>
> > > #include <linux/platform_data/x86/asus-wmi.h>
> > > -#include <linux/platform_data/x86/asus-wmi-leds-ids.h>
> > > #include <linux/platform_device.h>
> > > #include <linux/platform_profile.h>
> > > #include <linux/power_supply.h>
> > > #include <linux/rfkill.h>
> > > #include <linux/seq_file.h>
> > > #include <linux/slab.h>
> > > +#include <linux/spinlock.h>
> > > #include <linux/types.h>
> > > #include <linux/units.h>
> > >
> > > @@ -256,6 +256,9 @@ struct asus_wmi {
> > > int tpd_led_wk;
> > > struct led_classdev kbd_led;
> > > int kbd_led_wk;
> > > + bool kbd_led_notify;
> > > + bool kbd_led_avail;
> > > + bool kbd_led_registered;
> > > struct led_classdev lightbar_led;
> > > int lightbar_led_wk;
> > > struct led_classdev micmute_led;
> > > @@ -264,6 +267,7 @@ struct asus_wmi {
> > > struct work_struct tpd_led_work;
> > > struct work_struct wlan_led_work;
> > > struct work_struct lightbar_led_work;
> > > + struct work_struct kbd_led_work;
> > >
> > > struct asus_rfkill wlan;
> > > struct asus_rfkill bluetooth;
> > > @@ -1615,6 +1619,106 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
> > >
> > > /* LEDs ***********************************************************************/
> > >
> > > +struct asus_hid_ref {
> > > + struct list_head listeners;
> > > + struct asus_wmi *asus;
> > > + /* Protects concurrent access from hid-asus and asus-wmi to leds */
> > > + spinlock_t lock;
> > > +};
> > > +
> > > +static struct asus_hid_ref asus_ref = {
> > > + .listeners = LIST_HEAD_INIT(asus_ref.listeners),
> > > + .asus = NULL,
> > > + /*
> > > + * Protects .asus, .asus.kbd_led_{wk,notify}, and .listener refs. Other
> > > + * asus variables are read-only after .asus is set.
> > > + *
> > > + * The led cdev device is not protected because it calls backlight_get
> > > + * during initialization, which would result in a nested lock attempt.
> > > + *
> > > + * The led cdev is safe to access without a lock because if
> > > + * kbd_led_avail is true it is initialized before .asus is set and never
> > > + * changed until .asus is dropped. If kbd_led_avail is false, the led
> > > + * cdev is registered by the workqueue, which is single-threaded and
> > > + * cancelled before asus-wmi would access the led cdev to unregister it.
> > > + *
> > > + * A spinlock is used, because the protected variables can be accessed
> > > + * from an IRQ context from asus-hid.
> > > + */
> > > + .lock = __SPIN_LOCK_UNLOCKED(asus_ref.lock),
> > > +};
> > > +
> > > +/*
> > > + * Allows registering hid-asus listeners that want to be notified of
> > > + * keyboard backlight changes.
> > > + */
> > > +int asus_hid_register_listener(struct asus_hid_listener *bdev)
> > > +{
> > > + struct asus_wmi *asus;
> > > +
> > > + guard(spinlock_irqsave)(&asus_ref.lock);
> > > + list_add_tail(&bdev->list, &asus_ref.listeners);
> > > + asus = asus_ref.asus;
> > > + if (asus)
> > > + queue_work(asus->led_workqueue, &asus->kbd_led_work);
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(asus_hid_register_listener);
> > > +
> > > +/*
> > > + * Allows unregistering hid-asus listeners that were added with
> > > + * asus_hid_register_listener().
> > > + */
> > > +void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
> > > +{
> > > + guard(spinlock_irqsave)(&asus_ref.lock);
> > > + list_del(&bdev->list);
> > > +}
> > > +EXPORT_SYMBOL_GPL(asus_hid_unregister_listener);
> > > +
> > > +static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
> > > +
> > > +static void kbd_led_update_all(struct work_struct *work)
> > > +{
> > > + struct asus_wmi *asus;
> > > + bool registered, notify;
> > > + int ret, value;
> > > +
> > > + asus = container_of(work, struct asus_wmi, kbd_led_work);
> > > +
> > > + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> > > + registered = asus->kbd_led_registered;
> > > + value = asus->kbd_led_wk;
> > > + notify = asus->kbd_led_notify;
> > > + }
> > > +
> > > + if (!registered) {
> > > + /*
> > > + * This workqueue runs under asus-wmi, which means probe has
> > > + * completed and asus-wmi will keep running until it finishes.
> > > + * Therefore, we can safely register the LED without holding
> > > + * a spinlock.
> > > + */
> > > + ret = devm_led_classdev_register(&asus->platform_device->dev,
> > > + &asus->kbd_led);
> > > + if (!ret) {
> > > + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> > > + asus->kbd_led_registered = true;
> > > + } else {
> > > + pr_warn("Failed to register keyboard backlight LED: %d\n", ret);
> > > + return;
> > > + }
> > > + }
> > > +
> > > + if (value >= 0)
> > > + do_kbd_led_set(&asus->kbd_led, value);
> > > + if (notify) {
> > > + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> > > + asus->kbd_led_notify = false;
> > > + led_classdev_notify_brightness_hw_changed(&asus->kbd_led, value);
> > > + }
> > > +}
> > > +
> > > /*
> > > * These functions actually update the LED's, and are called from a
> > > * workqueue. By doing this as separate work rather than when the LED
> > > @@ -1661,7 +1765,8 @@ static void kbd_led_update(struct asus_wmi *asus)
> > > {
> > > int ctrl_param = 0;
> > >
> > > - ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
> > > + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> > > + ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
> > > asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
> > > }
> > >
> > > @@ -1694,14 +1799,23 @@ static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
> > >
> > > static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
> > > {
> > > + struct asus_hid_listener *listener;
> > > struct asus_wmi *asus;
> > > int max_level;
> > >
> > > asus = container_of(led_cdev, struct asus_wmi, kbd_led);
> > > max_level = asus->kbd_led.max_brightness;
> > >
> > > - asus->kbd_led_wk = clamp_val(value, 0, max_level);
> > > - kbd_led_update(asus);
> > > + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> > > + asus->kbd_led_wk = clamp_val(value, 0, max_level);
> > > +
> > > + if (asus->kbd_led_avail)
> > > + kbd_led_update(asus);
> > > +
> > > + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> > > + list_for_each_entry(listener, &asus_ref.listeners, list)
> > > + listener->brightness_set(listener, asus->kbd_led_wk);
> > > + }
> > > }
> > >
> > > static int kbd_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> > > @@ -1716,10 +1830,11 @@ static int kbd_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> > >
> > > static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
> > > {
> > > - struct led_classdev *led_cdev = &asus->kbd_led;
> > > -
> > > - do_kbd_led_set(led_cdev, value);
> > > - led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
> > > + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> > > + asus->kbd_led_wk = value;
> > > + asus->kbd_led_notify = true;
> > > + }
> > > + queue_work(asus->led_workqueue, &asus->kbd_led_work);
> > > }
> > >
> > > static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
> > > @@ -1729,10 +1844,18 @@ static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
> > >
> > > asus = container_of(led_cdev, struct asus_wmi, kbd_led);
> > >
> > > + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> > > + if (!asus->kbd_led_avail)
> > > + return asus->kbd_led_wk;
> > > + }
> > > +
> > > retval = kbd_led_read(asus, &value, NULL);
> > > if (retval < 0)
> > > return retval;
> > >
> > > + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> > > + asus->kbd_led_wk = value;
> > > +
> > > return value;
> > > }
> > >
> > > @@ -1844,7 +1967,9 @@ static int camera_led_set(struct led_classdev *led_cdev,
> > >
> > > static void asus_wmi_led_exit(struct asus_wmi *asus)
> > > {
> > > - led_classdev_unregister(&asus->kbd_led);
> > > + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> > > + asus_ref.asus = NULL;
> > > +
> > > led_classdev_unregister(&asus->tpd_led);
> > > led_classdev_unregister(&asus->wlan_led);
> > > led_classdev_unregister(&asus->lightbar_led);
> > > @@ -1882,22 +2007,26 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
> > > goto error;
> > > }
> > >
> > > - if (!kbd_led_read(asus, &led_val, NULL) && !dmi_check_system(asus_use_hid_led_dmi_ids)) {
> > > - pr_info("using asus-wmi for asus::kbd_backlight\n");
> > > - asus->kbd_led_wk = led_val;
> > > - asus->kbd_led.name = "asus::kbd_backlight";
> > > - asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> > > - asus->kbd_led.brightness_set_blocking = kbd_led_set;
> > > - asus->kbd_led.brightness_get = kbd_led_get;
> > > - asus->kbd_led.max_brightness = 3;
> > > + asus->kbd_led.name = "asus::kbd_backlight";
> > > + asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> > > + asus->kbd_led.brightness_set_blocking = kbd_led_set;
> > > + asus->kbd_led.brightness_get = kbd_led_get;
> > > + asus->kbd_led.max_brightness = 3;
> > > + asus->kbd_led_avail = !kbd_led_read(asus, &led_val, NULL);
> > > + INIT_WORK(&asus->kbd_led_work, kbd_led_update_all);
> > >
> > > + if (asus->kbd_led_avail) {
> > > + asus->kbd_led_wk = led_val;
> > > if (num_rgb_groups != 0)
> > > asus->kbd_led.groups = kbd_rgb_mode_groups;
> > > + } else {
> > > + asus->kbd_led_wk = -1;
> > > + }
> > >
> > > - rv = led_classdev_register(&asus->platform_device->dev,
> > > - &asus->kbd_led);
> > > - if (rv)
> > > - goto error;
> > > + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> > > + asus_ref.asus = asus;
> > > + if (asus->kbd_led_avail || !list_empty(&asus_ref.listeners))
> > > + queue_work(asus->led_workqueue, &asus->kbd_led_work);
> > > }
> > >
> > > if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_WIRELESS_LED)
> > > @@ -4372,6 +4501,7 @@ static int asus_wmi_get_event_code(union acpi_object *obj)
> > >
> > > static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
> > > {
> > > + enum led_brightness led_value;
> > > unsigned int key_value = 1;
> > > bool autorelease = 1;
> > >
> > > @@ -4388,19 +4518,22 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
> > > return;
> > > }
> > >
> > > + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> > > + led_value = asus->kbd_led_wk;
> > > +
> > > if (code == NOTIFY_KBD_BRTUP) {
> > > - kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
> > > + kbd_led_set_by_kbd(asus, led_value + 1);
> > > return;
> > > }
> > > if (code == NOTIFY_KBD_BRTDWN) {
> > > - kbd_led_set_by_kbd(asus, asus->kbd_led_wk - 1);
> > > + kbd_led_set_by_kbd(asus, led_value - 1);
> > > return;
> > > }
> > > if (code == NOTIFY_KBD_BRTTOGGLE) {
> > > - if (asus->kbd_led_wk == asus->kbd_led.max_brightness)
> > > + if (led_value == asus->kbd_led.max_brightness)
> > > kbd_led_set_by_kbd(asus, 0);
> > This is the toggle leds button, right? I would expect that pressing the toggle
> > button turns off leds if they are on and turns them on if they are off.
> >
> > so if (led_value > 0) { .... }.
> >
> > I see the previous code was equivalent to yours but is that what we want?
>
> It is common to do 0->1->2->3->0 for the toggle. This is what is
> currently done for WMI Asus keyboards and e.g., Thinkpads. This patch
> unifies the behavior for USB keyboards too.
>
> I would argue it is better, as you do not need to reach for a
> userspace slider to set a lower brightness.
>
> The current behavior of KDE is 0->3->0 and if the event goes to
> userspace this is what happens currently. Unless the keyboard
> reconnects, where brightness just stops working afterwards because
> upower only probes at boot (I have a follow patch to fix this for
> Duos).
Correction for the Duos, this series fixes it when reconnecting but
not at boot, the follow-up patch is for at boot.
> Antheas
>
> > > else
> > > - kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
> > > + kbd_led_set_by_kbd(asus, led_value + 1);
> > > return;
> > > }
> > >
> > > diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> > > index 419491d4abca..d347cffd05d5 100644
> > > --- a/include/linux/platform_data/x86/asus-wmi.h
> > > +++ b/include/linux/platform_data/x86/asus-wmi.h
> > > @@ -172,12 +172,20 @@ enum asus_ally_mcu_hack {
> > > ASUS_WMI_ALLY_MCU_HACK_DISABLED,
> > > };
> > >
> > > +/* Used to notify hid-asus when asus-wmi changes keyboard backlight */
> > > +struct asus_hid_listener {
> > > + struct list_head list;
> > > + void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
> > > +};
> > > +
> > > #if IS_REACHABLE(CONFIG_ASUS_WMI)
> > > void set_ally_mcu_hack(enum asus_ally_mcu_hack status);
> > > void set_ally_mcu_powersave(bool enabled);
> > > int asus_wmi_get_devstate_dsts(u32 dev_id, u32 *retval);
> > > int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval);
> > > int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
> > > +int asus_hid_register_listener(struct asus_hid_listener *cdev);
> > > +void asus_hid_unregister_listener(struct asus_hid_listener *cdev);
> > > #else
> > > static inline void set_ally_mcu_hack(enum asus_ally_mcu_hack status)
> > > {
> > > @@ -198,6 +206,13 @@ static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
> > > {
> > > return -ENODEV;
> > > }
> > > +static inline int asus_hid_register_listener(struct asus_hid_listener *bdev)
> > > +{
> > > + return -ENODEV;
> > > +}
> > > +static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
> > > +{
> > > +}
> > > #endif
> > >
> > > #endif /* __PLATFORM_DATA_X86_ASUS_WMI_H */
> >
^ permalink raw reply
* Re: [PATCH v11 07/11] platform/x86: asus-wmi: Add support for multiple kbd led handlers
From: Denis Benato @ 2026-01-17 13:56 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
In-Reply-To: <CAGwozwF0KaG0VJDJSPWfX8DVty-nZ+m8_ugDOk9wwHr+UbvYQA@mail.gmail.com>
On 1/17/26 14:49, Antheas Kapenekakis wrote:
> On Sat, 17 Jan 2026 at 14:16, Denis Benato <denis.benato@linux.dev> wrote:
>>
>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
>>> Some devices, such as the Z13 have multiple Aura devices connected
>>> to them by USB. In addition, they might have a WMI interface for
>>> RGB. In Windows, Armoury Crate exposes a unified brightness slider
>>> for all of them, with 3 brightness levels.
>>>
>>> Therefore, to be synergistic in Linux, and support existing tooling
>>> such as UPower, allow adding listeners to the RGB device of the WMI
>>> interface. If WMI does not exist, lazy initialize the interface.
>>>
>>> Since hid-asus and asus-wmi can both interact with the led objects
>>> including from an atomic context, protect the brightness access with a
>>> spinlock and update the values from a workqueue. Use this workqueue to
>>> also process WMI keyboard events, so they are handled asynchronously.
>>>
>>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>> ---
>>> drivers/platform/x86/asus-wmi.c | 183 ++++++++++++++++++---
>>> include/linux/platform_data/x86/asus-wmi.h | 15 ++
>>> 2 files changed, 173 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>>> index 4aec7ec69250..df2365efb2b8 100644
>>> --- a/drivers/platform/x86/asus-wmi.c
>>> +++ b/drivers/platform/x86/asus-wmi.c
>>> @@ -31,13 +31,13 @@
>>> #include <linux/pci.h>
>>> #include <linux/pci_hotplug.h>
>>> #include <linux/platform_data/x86/asus-wmi.h>
>>> -#include <linux/platform_data/x86/asus-wmi-leds-ids.h>
>>> #include <linux/platform_device.h>
>>> #include <linux/platform_profile.h>
>>> #include <linux/power_supply.h>
>>> #include <linux/rfkill.h>
>>> #include <linux/seq_file.h>
>>> #include <linux/slab.h>
>>> +#include <linux/spinlock.h>
>>> #include <linux/types.h>
>>> #include <linux/units.h>
>>>
>>> @@ -256,6 +256,9 @@ struct asus_wmi {
>>> int tpd_led_wk;
>>> struct led_classdev kbd_led;
>>> int kbd_led_wk;
>>> + bool kbd_led_notify;
>>> + bool kbd_led_avail;
>>> + bool kbd_led_registered;
>>> struct led_classdev lightbar_led;
>>> int lightbar_led_wk;
>>> struct led_classdev micmute_led;
>>> @@ -264,6 +267,7 @@ struct asus_wmi {
>>> struct work_struct tpd_led_work;
>>> struct work_struct wlan_led_work;
>>> struct work_struct lightbar_led_work;
>>> + struct work_struct kbd_led_work;
>>>
>>> struct asus_rfkill wlan;
>>> struct asus_rfkill bluetooth;
>>> @@ -1615,6 +1619,106 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
>>>
>>> /* LEDs ***********************************************************************/
>>>
>>> +struct asus_hid_ref {
>>> + struct list_head listeners;
>>> + struct asus_wmi *asus;
>>> + /* Protects concurrent access from hid-asus and asus-wmi to leds */
>>> + spinlock_t lock;
>>> +};
>>> +
>>> +static struct asus_hid_ref asus_ref = {
>>> + .listeners = LIST_HEAD_INIT(asus_ref.listeners),
>>> + .asus = NULL,
>>> + /*
>>> + * Protects .asus, .asus.kbd_led_{wk,notify}, and .listener refs. Other
>>> + * asus variables are read-only after .asus is set.
>>> + *
>>> + * The led cdev device is not protected because it calls backlight_get
>>> + * during initialization, which would result in a nested lock attempt.
>>> + *
>>> + * The led cdev is safe to access without a lock because if
>>> + * kbd_led_avail is true it is initialized before .asus is set and never
>>> + * changed until .asus is dropped. If kbd_led_avail is false, the led
>>> + * cdev is registered by the workqueue, which is single-threaded and
>>> + * cancelled before asus-wmi would access the led cdev to unregister it.
>>> + *
>>> + * A spinlock is used, because the protected variables can be accessed
>>> + * from an IRQ context from asus-hid.
>>> + */
>>> + .lock = __SPIN_LOCK_UNLOCKED(asus_ref.lock),
>>> +};
>>> +
>>> +/*
>>> + * Allows registering hid-asus listeners that want to be notified of
>>> + * keyboard backlight changes.
>>> + */
>>> +int asus_hid_register_listener(struct asus_hid_listener *bdev)
>>> +{
>>> + struct asus_wmi *asus;
>>> +
>>> + guard(spinlock_irqsave)(&asus_ref.lock);
>>> + list_add_tail(&bdev->list, &asus_ref.listeners);
>>> + asus = asus_ref.asus;
>>> + if (asus)
>>> + queue_work(asus->led_workqueue, &asus->kbd_led_work);
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(asus_hid_register_listener);
>>> +
>>> +/*
>>> + * Allows unregistering hid-asus listeners that were added with
>>> + * asus_hid_register_listener().
>>> + */
>>> +void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
>>> +{
>>> + guard(spinlock_irqsave)(&asus_ref.lock);
>>> + list_del(&bdev->list);
>>> +}
>>> +EXPORT_SYMBOL_GPL(asus_hid_unregister_listener);
>>> +
>>> +static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
>>> +
>>> +static void kbd_led_update_all(struct work_struct *work)
>>> +{
>>> + struct asus_wmi *asus;
>>> + bool registered, notify;
>>> + int ret, value;
>>> +
>>> + asus = container_of(work, struct asus_wmi, kbd_led_work);
>>> +
>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
>>> + registered = asus->kbd_led_registered;
>>> + value = asus->kbd_led_wk;
>>> + notify = asus->kbd_led_notify;
>>> + }
>>> +
>>> + if (!registered) {
>>> + /*
>>> + * This workqueue runs under asus-wmi, which means probe has
>>> + * completed and asus-wmi will keep running until it finishes.
>>> + * Therefore, we can safely register the LED without holding
>>> + * a spinlock.
>>> + */
>>> + ret = devm_led_classdev_register(&asus->platform_device->dev,
>>> + &asus->kbd_led);
>>> + if (!ret) {
>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
>>> + asus->kbd_led_registered = true;
>>> + } else {
>>> + pr_warn("Failed to register keyboard backlight LED: %d\n", ret);
>>> + return;
>>> + }
>>> + }
>>> +
>>> + if (value >= 0)
>>> + do_kbd_led_set(&asus->kbd_led, value);
>>> + if (notify) {
>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
>>> + asus->kbd_led_notify = false;
>>> + led_classdev_notify_brightness_hw_changed(&asus->kbd_led, value);
>>> + }
>>> +}
>>> +
>>> /*
>>> * These functions actually update the LED's, and are called from a
>>> * workqueue. By doing this as separate work rather than when the LED
>>> @@ -1661,7 +1765,8 @@ static void kbd_led_update(struct asus_wmi *asus)
>>> {
>>> int ctrl_param = 0;
>>>
>>> - ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
>>> + ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
>>> asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
>>> }
>>>
>>> @@ -1694,14 +1799,23 @@ static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
>>>
>>> static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
>>> {
>>> + struct asus_hid_listener *listener;
>>> struct asus_wmi *asus;
>>> int max_level;
>>>
>>> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>>> max_level = asus->kbd_led.max_brightness;
>>>
>>> - asus->kbd_led_wk = clamp_val(value, 0, max_level);
>>> - kbd_led_update(asus);
>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
>>> + asus->kbd_led_wk = clamp_val(value, 0, max_level);
>>> +
>>> + if (asus->kbd_led_avail)
>>> + kbd_led_update(asus);
>>> +
>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
>>> + list_for_each_entry(listener, &asus_ref.listeners, list)
>>> + listener->brightness_set(listener, asus->kbd_led_wk);
>>> + }
>>> }
>>>
>>> static int kbd_led_set(struct led_classdev *led_cdev, enum led_brightness value)
>>> @@ -1716,10 +1830,11 @@ static int kbd_led_set(struct led_classdev *led_cdev, enum led_brightness value)
>>>
>>> static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
>>> {
>>> - struct led_classdev *led_cdev = &asus->kbd_led;
>>> -
>>> - do_kbd_led_set(led_cdev, value);
>>> - led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
>>> + asus->kbd_led_wk = value;
>>> + asus->kbd_led_notify = true;
>>> + }
>>> + queue_work(asus->led_workqueue, &asus->kbd_led_work);
>>> }
>>>
>>> static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
>>> @@ -1729,10 +1844,18 @@ static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
>>>
>>> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>>>
>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
>>> + if (!asus->kbd_led_avail)
>>> + return asus->kbd_led_wk;
>>> + }
>>> +
>>> retval = kbd_led_read(asus, &value, NULL);
>>> if (retval < 0)
>>> return retval;
>>>
>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
>>> + asus->kbd_led_wk = value;
>>> +
>>> return value;
>>> }
>>>
>>> @@ -1844,7 +1967,9 @@ static int camera_led_set(struct led_classdev *led_cdev,
>>>
>>> static void asus_wmi_led_exit(struct asus_wmi *asus)
>>> {
>>> - led_classdev_unregister(&asus->kbd_led);
>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
>>> + asus_ref.asus = NULL;
>>> +
>>> led_classdev_unregister(&asus->tpd_led);
>>> led_classdev_unregister(&asus->wlan_led);
>>> led_classdev_unregister(&asus->lightbar_led);
>>> @@ -1882,22 +2007,26 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
>>> goto error;
>>> }
>>>
>>> - if (!kbd_led_read(asus, &led_val, NULL) && !dmi_check_system(asus_use_hid_led_dmi_ids)) {
>>> - pr_info("using asus-wmi for asus::kbd_backlight\n");
>>> - asus->kbd_led_wk = led_val;
>>> - asus->kbd_led.name = "asus::kbd_backlight";
>>> - asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
>>> - asus->kbd_led.brightness_set_blocking = kbd_led_set;
>>> - asus->kbd_led.brightness_get = kbd_led_get;
>>> - asus->kbd_led.max_brightness = 3;
>>> + asus->kbd_led.name = "asus::kbd_backlight";
>>> + asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
>>> + asus->kbd_led.brightness_set_blocking = kbd_led_set;
>>> + asus->kbd_led.brightness_get = kbd_led_get;
>>> + asus->kbd_led.max_brightness = 3;
>>> + asus->kbd_led_avail = !kbd_led_read(asus, &led_val, NULL);
>>> + INIT_WORK(&asus->kbd_led_work, kbd_led_update_all);
>>>
>>> + if (asus->kbd_led_avail) {
>>> + asus->kbd_led_wk = led_val;
>>> if (num_rgb_groups != 0)
>>> asus->kbd_led.groups = kbd_rgb_mode_groups;
>>> + } else {
>>> + asus->kbd_led_wk = -1;
>>> + }
>>>
>>> - rv = led_classdev_register(&asus->platform_device->dev,
>>> - &asus->kbd_led);
>>> - if (rv)
>>> - goto error;
>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
>>> + asus_ref.asus = asus;
>>> + if (asus->kbd_led_avail || !list_empty(&asus_ref.listeners))
>>> + queue_work(asus->led_workqueue, &asus->kbd_led_work);
>>> }
>>>
>>> if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_WIRELESS_LED)
>>> @@ -4372,6 +4501,7 @@ static int asus_wmi_get_event_code(union acpi_object *obj)
>>>
>>> static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
>>> {
>>> + enum led_brightness led_value;
>>> unsigned int key_value = 1;
>>> bool autorelease = 1;
>>>
>>> @@ -4388,19 +4518,22 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
>>> return;
>>> }
>>>
>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
>>> + led_value = asus->kbd_led_wk;
>>> +
>>> if (code == NOTIFY_KBD_BRTUP) {
>>> - kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
>>> + kbd_led_set_by_kbd(asus, led_value + 1);
>>> return;
>>> }
>>> if (code == NOTIFY_KBD_BRTDWN) {
>>> - kbd_led_set_by_kbd(asus, asus->kbd_led_wk - 1);
>>> + kbd_led_set_by_kbd(asus, led_value - 1);
>>> return;
>>> }
>>> if (code == NOTIFY_KBD_BRTTOGGLE) {
>>> - if (asus->kbd_led_wk == asus->kbd_led.max_brightness)
>>> + if (led_value == asus->kbd_led.max_brightness)
>>> kbd_led_set_by_kbd(asus, 0);
>> This is the toggle leds button, right? I would expect that pressing the toggle
>> button turns off leds if they are on and turns them on if they are off.
>>
>> so if (led_value > 0) { .... }.
>>
>> I see the previous code was equivalent to yours but is that what we want?
> It is common to do 0->1->2->3->0 for the toggle. This is what is
> currently done for WMI Asus keyboards and e.g., Thinkpads. This patch
> unifies the behavior for USB keyboards too.
>
> I would argue it is better, as you do not need to reach for a
> userspace slider to set a lower brightness.
>
> The current behavior of KDE is 0->3->0 and if the event goes to
> userspace this is what happens currently. Unless the keyboard
> reconnects, where brightness just stops working afterwards because
> upower only probes at boot (I have a follow patch to fix this for
> Duos).
Whatever goes for me if hid and wmi handles them the same,
especially if userspace does the same thing.
In 11/11 you do:
caseASUS_EV_BRTTOGGLE:
if(brightness >=ASUS_EV_MAX_BRIGHTNESS)
brightness =0;
else
brightness +=1;
break;
}
So perhaps here you should do
if (led_value >= asus->kbd_led.max_brightness)
purely for consistency?
> Antheas
>
>>> else
>>> - kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
>>> + kbd_led_set_by_kbd(asus, led_value + 1);
>>> return;
>>> }
>>>
>>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
>>> index 419491d4abca..d347cffd05d5 100644
>>> --- a/include/linux/platform_data/x86/asus-wmi.h
>>> +++ b/include/linux/platform_data/x86/asus-wmi.h
>>> @@ -172,12 +172,20 @@ enum asus_ally_mcu_hack {
>>> ASUS_WMI_ALLY_MCU_HACK_DISABLED,
>>> };
>>>
>>> +/* Used to notify hid-asus when asus-wmi changes keyboard backlight */
>>> +struct asus_hid_listener {
>>> + struct list_head list;
>>> + void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
>>> +};
>>> +
>>> #if IS_REACHABLE(CONFIG_ASUS_WMI)
>>> void set_ally_mcu_hack(enum asus_ally_mcu_hack status);
>>> void set_ally_mcu_powersave(bool enabled);
>>> int asus_wmi_get_devstate_dsts(u32 dev_id, u32 *retval);
>>> int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval);
>>> int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
>>> +int asus_hid_register_listener(struct asus_hid_listener *cdev);
>>> +void asus_hid_unregister_listener(struct asus_hid_listener *cdev);
>>> #else
>>> static inline void set_ally_mcu_hack(enum asus_ally_mcu_hack status)
>>> {
>>> @@ -198,6 +206,13 @@ static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
>>> {
>>> return -ENODEV;
>>> }
>>> +static inline int asus_hid_register_listener(struct asus_hid_listener *bdev)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
>>> +{
>>> +}
>>> #endif
>>>
>>> #endif /* __PLATFORM_DATA_X86_ASUS_WMI_H */
^ permalink raw reply
* Re: [PATCH v11 07/11] platform/x86: asus-wmi: Add support for multiple kbd led handlers
From: Antheas Kapenekakis @ 2026-01-17 14:11 UTC (permalink / raw)
To: Denis Benato
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
In-Reply-To: <2f7c8e80-274e-4cec-98bf-cd9cc49fb363@linux.dev>
On Sat, 17 Jan 2026 at 14:56, Denis Benato <denis.benato@linux.dev> wrote:
>
>
> On 1/17/26 14:49, Antheas Kapenekakis wrote:
> > On Sat, 17 Jan 2026 at 14:16, Denis Benato <denis.benato@linux.dev> wrote:
> >>
> >> On 1/16/26 14:31, Antheas Kapenekakis wrote:
> >>> Some devices, such as the Z13 have multiple Aura devices connected
> >>> to them by USB. In addition, they might have a WMI interface for
> >>> RGB. In Windows, Armoury Crate exposes a unified brightness slider
> >>> for all of them, with 3 brightness levels.
> >>>
> >>> Therefore, to be synergistic in Linux, and support existing tooling
> >>> such as UPower, allow adding listeners to the RGB device of the WMI
> >>> interface. If WMI does not exist, lazy initialize the interface.
> >>>
> >>> Since hid-asus and asus-wmi can both interact with the led objects
> >>> including from an atomic context, protect the brightness access with a
> >>> spinlock and update the values from a workqueue. Use this workqueue to
> >>> also process WMI keyboard events, so they are handled asynchronously.
> >>>
> >>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
> >>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>> ---
> >>> drivers/platform/x86/asus-wmi.c | 183 ++++++++++++++++++---
> >>> include/linux/platform_data/x86/asus-wmi.h | 15 ++
> >>> 2 files changed, 173 insertions(+), 25 deletions(-)
> >>>
> >>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> >>> index 4aec7ec69250..df2365efb2b8 100644
> >>> --- a/drivers/platform/x86/asus-wmi.c
> >>> +++ b/drivers/platform/x86/asus-wmi.c
> >>> @@ -31,13 +31,13 @@
> >>> #include <linux/pci.h>
> >>> #include <linux/pci_hotplug.h>
> >>> #include <linux/platform_data/x86/asus-wmi.h>
> >>> -#include <linux/platform_data/x86/asus-wmi-leds-ids.h>
> >>> #include <linux/platform_device.h>
> >>> #include <linux/platform_profile.h>
> >>> #include <linux/power_supply.h>
> >>> #include <linux/rfkill.h>
> >>> #include <linux/seq_file.h>
> >>> #include <linux/slab.h>
> >>> +#include <linux/spinlock.h>
> >>> #include <linux/types.h>
> >>> #include <linux/units.h>
> >>>
> >>> @@ -256,6 +256,9 @@ struct asus_wmi {
> >>> int tpd_led_wk;
> >>> struct led_classdev kbd_led;
> >>> int kbd_led_wk;
> >>> + bool kbd_led_notify;
> >>> + bool kbd_led_avail;
> >>> + bool kbd_led_registered;
> >>> struct led_classdev lightbar_led;
> >>> int lightbar_led_wk;
> >>> struct led_classdev micmute_led;
> >>> @@ -264,6 +267,7 @@ struct asus_wmi {
> >>> struct work_struct tpd_led_work;
> >>> struct work_struct wlan_led_work;
> >>> struct work_struct lightbar_led_work;
> >>> + struct work_struct kbd_led_work;
> >>>
> >>> struct asus_rfkill wlan;
> >>> struct asus_rfkill bluetooth;
> >>> @@ -1615,6 +1619,106 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
> >>>
> >>> /* LEDs ***********************************************************************/
> >>>
> >>> +struct asus_hid_ref {
> >>> + struct list_head listeners;
> >>> + struct asus_wmi *asus;
> >>> + /* Protects concurrent access from hid-asus and asus-wmi to leds */
> >>> + spinlock_t lock;
> >>> +};
> >>> +
> >>> +static struct asus_hid_ref asus_ref = {
> >>> + .listeners = LIST_HEAD_INIT(asus_ref.listeners),
> >>> + .asus = NULL,
> >>> + /*
> >>> + * Protects .asus, .asus.kbd_led_{wk,notify}, and .listener refs. Other
> >>> + * asus variables are read-only after .asus is set.
> >>> + *
> >>> + * The led cdev device is not protected because it calls backlight_get
> >>> + * during initialization, which would result in a nested lock attempt.
> >>> + *
> >>> + * The led cdev is safe to access without a lock because if
> >>> + * kbd_led_avail is true it is initialized before .asus is set and never
> >>> + * changed until .asus is dropped. If kbd_led_avail is false, the led
> >>> + * cdev is registered by the workqueue, which is single-threaded and
> >>> + * cancelled before asus-wmi would access the led cdev to unregister it.
> >>> + *
> >>> + * A spinlock is used, because the protected variables can be accessed
> >>> + * from an IRQ context from asus-hid.
> >>> + */
> >>> + .lock = __SPIN_LOCK_UNLOCKED(asus_ref.lock),
> >>> +};
> >>> +
> >>> +/*
> >>> + * Allows registering hid-asus listeners that want to be notified of
> >>> + * keyboard backlight changes.
> >>> + */
> >>> +int asus_hid_register_listener(struct asus_hid_listener *bdev)
> >>> +{
> >>> + struct asus_wmi *asus;
> >>> +
> >>> + guard(spinlock_irqsave)(&asus_ref.lock);
> >>> + list_add_tail(&bdev->list, &asus_ref.listeners);
> >>> + asus = asus_ref.asus;
> >>> + if (asus)
> >>> + queue_work(asus->led_workqueue, &asus->kbd_led_work);
> >>> + return 0;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(asus_hid_register_listener);
> >>> +
> >>> +/*
> >>> + * Allows unregistering hid-asus listeners that were added with
> >>> + * asus_hid_register_listener().
> >>> + */
> >>> +void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
> >>> +{
> >>> + guard(spinlock_irqsave)(&asus_ref.lock);
> >>> + list_del(&bdev->list);
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(asus_hid_unregister_listener);
> >>> +
> >>> +static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
> >>> +
> >>> +static void kbd_led_update_all(struct work_struct *work)
> >>> +{
> >>> + struct asus_wmi *asus;
> >>> + bool registered, notify;
> >>> + int ret, value;
> >>> +
> >>> + asus = container_of(work, struct asus_wmi, kbd_led_work);
> >>> +
> >>> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> >>> + registered = asus->kbd_led_registered;
> >>> + value = asus->kbd_led_wk;
> >>> + notify = asus->kbd_led_notify;
> >>> + }
> >>> +
> >>> + if (!registered) {
> >>> + /*
> >>> + * This workqueue runs under asus-wmi, which means probe has
> >>> + * completed and asus-wmi will keep running until it finishes.
> >>> + * Therefore, we can safely register the LED without holding
> >>> + * a spinlock.
> >>> + */
> >>> + ret = devm_led_classdev_register(&asus->platform_device->dev,
> >>> + &asus->kbd_led);
> >>> + if (!ret) {
> >>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> >>> + asus->kbd_led_registered = true;
> >>> + } else {
> >>> + pr_warn("Failed to register keyboard backlight LED: %d\n", ret);
> >>> + return;
> >>> + }
> >>> + }
> >>> +
> >>> + if (value >= 0)
> >>> + do_kbd_led_set(&asus->kbd_led, value);
> >>> + if (notify) {
> >>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> >>> + asus->kbd_led_notify = false;
> >>> + led_classdev_notify_brightness_hw_changed(&asus->kbd_led, value);
> >>> + }
> >>> +}
> >>> +
> >>> /*
> >>> * These functions actually update the LED's, and are called from a
> >>> * workqueue. By doing this as separate work rather than when the LED
> >>> @@ -1661,7 +1765,8 @@ static void kbd_led_update(struct asus_wmi *asus)
> >>> {
> >>> int ctrl_param = 0;
> >>>
> >>> - ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
> >>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> >>> + ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
> >>> asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
> >>> }
> >>>
> >>> @@ -1694,14 +1799,23 @@ static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
> >>>
> >>> static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
> >>> {
> >>> + struct asus_hid_listener *listener;
> >>> struct asus_wmi *asus;
> >>> int max_level;
> >>>
> >>> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
> >>> max_level = asus->kbd_led.max_brightness;
> >>>
> >>> - asus->kbd_led_wk = clamp_val(value, 0, max_level);
> >>> - kbd_led_update(asus);
> >>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> >>> + asus->kbd_led_wk = clamp_val(value, 0, max_level);
> >>> +
> >>> + if (asus->kbd_led_avail)
> >>> + kbd_led_update(asus);
> >>> +
> >>> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> >>> + list_for_each_entry(listener, &asus_ref.listeners, list)
> >>> + listener->brightness_set(listener, asus->kbd_led_wk);
> >>> + }
> >>> }
> >>>
> >>> static int kbd_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> >>> @@ -1716,10 +1830,11 @@ static int kbd_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> >>>
> >>> static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
> >>> {
> >>> - struct led_classdev *led_cdev = &asus->kbd_led;
> >>> -
> >>> - do_kbd_led_set(led_cdev, value);
> >>> - led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
> >>> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> >>> + asus->kbd_led_wk = value;
> >>> + asus->kbd_led_notify = true;
> >>> + }
> >>> + queue_work(asus->led_workqueue, &asus->kbd_led_work);
> >>> }
> >>>
> >>> static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
> >>> @@ -1729,10 +1844,18 @@ static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
> >>>
> >>> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
> >>>
> >>> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> >>> + if (!asus->kbd_led_avail)
> >>> + return asus->kbd_led_wk;
> >>> + }
> >>> +
> >>> retval = kbd_led_read(asus, &value, NULL);
> >>> if (retval < 0)
> >>> return retval;
> >>>
> >>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> >>> + asus->kbd_led_wk = value;
> >>> +
> >>> return value;
> >>> }
> >>>
> >>> @@ -1844,7 +1967,9 @@ static int camera_led_set(struct led_classdev *led_cdev,
> >>>
> >>> static void asus_wmi_led_exit(struct asus_wmi *asus)
> >>> {
> >>> - led_classdev_unregister(&asus->kbd_led);
> >>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> >>> + asus_ref.asus = NULL;
> >>> +
> >>> led_classdev_unregister(&asus->tpd_led);
> >>> led_classdev_unregister(&asus->wlan_led);
> >>> led_classdev_unregister(&asus->lightbar_led);
> >>> @@ -1882,22 +2007,26 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
> >>> goto error;
> >>> }
> >>>
> >>> - if (!kbd_led_read(asus, &led_val, NULL) && !dmi_check_system(asus_use_hid_led_dmi_ids)) {
> >>> - pr_info("using asus-wmi for asus::kbd_backlight\n");
> >>> - asus->kbd_led_wk = led_val;
> >>> - asus->kbd_led.name = "asus::kbd_backlight";
> >>> - asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> >>> - asus->kbd_led.brightness_set_blocking = kbd_led_set;
> >>> - asus->kbd_led.brightness_get = kbd_led_get;
> >>> - asus->kbd_led.max_brightness = 3;
> >>> + asus->kbd_led.name = "asus::kbd_backlight";
> >>> + asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> >>> + asus->kbd_led.brightness_set_blocking = kbd_led_set;
> >>> + asus->kbd_led.brightness_get = kbd_led_get;
> >>> + asus->kbd_led.max_brightness = 3;
> >>> + asus->kbd_led_avail = !kbd_led_read(asus, &led_val, NULL);
> >>> + INIT_WORK(&asus->kbd_led_work, kbd_led_update_all);
> >>>
> >>> + if (asus->kbd_led_avail) {
> >>> + asus->kbd_led_wk = led_val;
> >>> if (num_rgb_groups != 0)
> >>> asus->kbd_led.groups = kbd_rgb_mode_groups;
> >>> + } else {
> >>> + asus->kbd_led_wk = -1;
> >>> + }
> >>>
> >>> - rv = led_classdev_register(&asus->platform_device->dev,
> >>> - &asus->kbd_led);
> >>> - if (rv)
> >>> - goto error;
> >>> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> >>> + asus_ref.asus = asus;
> >>> + if (asus->kbd_led_avail || !list_empty(&asus_ref.listeners))
> >>> + queue_work(asus->led_workqueue, &asus->kbd_led_work);
> >>> }
> >>>
> >>> if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_WIRELESS_LED)
> >>> @@ -4372,6 +4501,7 @@ static int asus_wmi_get_event_code(union acpi_object *obj)
> >>>
> >>> static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
> >>> {
> >>> + enum led_brightness led_value;
> >>> unsigned int key_value = 1;
> >>> bool autorelease = 1;
> >>>
> >>> @@ -4388,19 +4518,22 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
> >>> return;
> >>> }
> >>>
> >>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> >>> + led_value = asus->kbd_led_wk;
> >>> +
> >>> if (code == NOTIFY_KBD_BRTUP) {
> >>> - kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
> >>> + kbd_led_set_by_kbd(asus, led_value + 1);
> >>> return;
> >>> }
> >>> if (code == NOTIFY_KBD_BRTDWN) {
> >>> - kbd_led_set_by_kbd(asus, asus->kbd_led_wk - 1);
> >>> + kbd_led_set_by_kbd(asus, led_value - 1);
> >>> return;
> >>> }
> >>> if (code == NOTIFY_KBD_BRTTOGGLE) {
> >>> - if (asus->kbd_led_wk == asus->kbd_led.max_brightness)
> >>> + if (led_value == asus->kbd_led.max_brightness)
> >>> kbd_led_set_by_kbd(asus, 0);
> >> This is the toggle leds button, right? I would expect that pressing the toggle
> >> button turns off leds if they are on and turns them on if they are off.
> >>
> >> so if (led_value > 0) { .... }.
> >>
> >> I see the previous code was equivalent to yours but is that what we want?
> > It is common to do 0->1->2->3->0 for the toggle. This is what is
> > currently done for WMI Asus keyboards and e.g., Thinkpads. This patch
> > unifies the behavior for USB keyboards too.
> >
> > I would argue it is better, as you do not need to reach for a
> > userspace slider to set a lower brightness.
> >
> > The current behavior of KDE is 0->3->0 and if the event goes to
> > userspace this is what happens currently. Unless the keyboard
> > reconnects, where brightness just stops working afterwards because
> > upower only probes at boot (I have a follow patch to fix this for
> > Duos).
> Whatever goes for me if hid and wmi handles them the same,
> especially if userspace does the same thing.
>
> In 11/11 you do:
>
> caseASUS_EV_BRTTOGGLE:
> if(brightness >=ASUS_EV_MAX_BRIGHTNESS)
> brightness =0;
> else
> brightness +=1;
> break;
> }
>
> So perhaps here you should do
>
> if (led_value >= asus->kbd_led.max_brightness)
>
>
> purely for consistency?
If I reroll the series I will do it, otherwise you can do a followup
Antheas
> > Antheas
> >
> >>> else
> >>> - kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
> >>> + kbd_led_set_by_kbd(asus, led_value + 1);
> >>> return;
> >>> }
> >>>
> >>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> >>> index 419491d4abca..d347cffd05d5 100644
> >>> --- a/include/linux/platform_data/x86/asus-wmi.h
> >>> +++ b/include/linux/platform_data/x86/asus-wmi.h
> >>> @@ -172,12 +172,20 @@ enum asus_ally_mcu_hack {
> >>> ASUS_WMI_ALLY_MCU_HACK_DISABLED,
> >>> };
> >>>
> >>> +/* Used to notify hid-asus when asus-wmi changes keyboard backlight */
> >>> +struct asus_hid_listener {
> >>> + struct list_head list;
> >>> + void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
> >>> +};
> >>> +
> >>> #if IS_REACHABLE(CONFIG_ASUS_WMI)
> >>> void set_ally_mcu_hack(enum asus_ally_mcu_hack status);
> >>> void set_ally_mcu_powersave(bool enabled);
> >>> int asus_wmi_get_devstate_dsts(u32 dev_id, u32 *retval);
> >>> int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval);
> >>> int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
> >>> +int asus_hid_register_listener(struct asus_hid_listener *cdev);
> >>> +void asus_hid_unregister_listener(struct asus_hid_listener *cdev);
> >>> #else
> >>> static inline void set_ally_mcu_hack(enum asus_ally_mcu_hack status)
> >>> {
> >>> @@ -198,6 +206,13 @@ static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
> >>> {
> >>> return -ENODEV;
> >>> }
> >>> +static inline int asus_hid_register_listener(struct asus_hid_listener *bdev)
> >>> +{
> >>> + return -ENODEV;
> >>> +}
> >>> +static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
> >>> +{
> >>> +}
> >>> #endif
> >>>
> >>> #endif /* __PLATFORM_DATA_X86_ASUS_WMI_H */
>
^ permalink raw reply
* Re: [PATCH v11 07/11] platform/x86: asus-wmi: Add support for multiple kbd led handlers
From: Denis Benato @ 2026-01-17 14:48 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
In-Reply-To: <CAGwozwEEd8+Gfrs=9zX2ES-tLpNQLmcWicRqHaWgd_UsCsHjPg@mail.gmail.com>
On 1/17/26 15:11, Antheas Kapenekakis wrote:
> On Sat, 17 Jan 2026 at 14:56, Denis Benato <denis.benato@linux.dev> wrote:
>>
>> On 1/17/26 14:49, Antheas Kapenekakis wrote:
>>> On Sat, 17 Jan 2026 at 14:16, Denis Benato <denis.benato@linux.dev> wrote:
>>>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
>>>>> Some devices, such as the Z13 have multiple Aura devices connected
>>>>> to them by USB. In addition, they might have a WMI interface for
>>>>> RGB. In Windows, Armoury Crate exposes a unified brightness slider
>>>>> for all of them, with 3 brightness levels.
>>>>>
>>>>> Therefore, to be synergistic in Linux, and support existing tooling
>>>>> such as UPower, allow adding listeners to the RGB device of the WMI
>>>>> interface. If WMI does not exist, lazy initialize the interface.
>>>>>
>>>>> Since hid-asus and asus-wmi can both interact with the led objects
>>>>> including from an atomic context, protect the brightness access with a
>>>>> spinlock and update the values from a workqueue. Use this workqueue to
>>>>> also process WMI keyboard events, so they are handled asynchronously.
>>>>>
>>>>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>>>> ---
>>>>> drivers/platform/x86/asus-wmi.c | 183 ++++++++++++++++++---
>>>>> include/linux/platform_data/x86/asus-wmi.h | 15 ++
>>>>> 2 files changed, 173 insertions(+), 25 deletions(-)
>>>>>
>>>>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>>>>> index 4aec7ec69250..df2365efb2b8 100644
>>>>> --- a/drivers/platform/x86/asus-wmi.c
>>>>> +++ b/drivers/platform/x86/asus-wmi.c
>>>>> @@ -31,13 +31,13 @@
>>>>> #include <linux/pci.h>
>>>>> #include <linux/pci_hotplug.h>
>>>>> #include <linux/platform_data/x86/asus-wmi.h>
>>>>> -#include <linux/platform_data/x86/asus-wmi-leds-ids.h>
>>>>> #include <linux/platform_device.h>
>>>>> #include <linux/platform_profile.h>
>>>>> #include <linux/power_supply.h>
>>>>> #include <linux/rfkill.h>
>>>>> #include <linux/seq_file.h>
>>>>> #include <linux/slab.h>
>>>>> +#include <linux/spinlock.h>
>>>>> #include <linux/types.h>
>>>>> #include <linux/units.h>
>>>>>
>>>>> @@ -256,6 +256,9 @@ struct asus_wmi {
>>>>> int tpd_led_wk;
>>>>> struct led_classdev kbd_led;
>>>>> int kbd_led_wk;
>>>>> + bool kbd_led_notify;
>>>>> + bool kbd_led_avail;
>>>>> + bool kbd_led_registered;
>>>>> struct led_classdev lightbar_led;
>>>>> int lightbar_led_wk;
>>>>> struct led_classdev micmute_led;
>>>>> @@ -264,6 +267,7 @@ struct asus_wmi {
>>>>> struct work_struct tpd_led_work;
>>>>> struct work_struct wlan_led_work;
>>>>> struct work_struct lightbar_led_work;
>>>>> + struct work_struct kbd_led_work;
>>>>>
>>>>> struct asus_rfkill wlan;
>>>>> struct asus_rfkill bluetooth;
>>>>> @@ -1615,6 +1619,106 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
>>>>>
>>>>> /* LEDs ***********************************************************************/
>>>>>
>>>>> +struct asus_hid_ref {
>>>>> + struct list_head listeners;
>>>>> + struct asus_wmi *asus;
>>>>> + /* Protects concurrent access from hid-asus and asus-wmi to leds */
>>>>> + spinlock_t lock;
>>>>> +};
>>>>> +
>>>>> +static struct asus_hid_ref asus_ref = {
>>>>> + .listeners = LIST_HEAD_INIT(asus_ref.listeners),
>>>>> + .asus = NULL,
>>>>> + /*
>>>>> + * Protects .asus, .asus.kbd_led_{wk,notify}, and .listener refs. Other
>>>>> + * asus variables are read-only after .asus is set.
>>>>> + *
>>>>> + * The led cdev device is not protected because it calls backlight_get
>>>>> + * during initialization, which would result in a nested lock attempt.
>>>>> + *
>>>>> + * The led cdev is safe to access without a lock because if
>>>>> + * kbd_led_avail is true it is initialized before .asus is set and never
>>>>> + * changed until .asus is dropped. If kbd_led_avail is false, the led
>>>>> + * cdev is registered by the workqueue, which is single-threaded and
>>>>> + * cancelled before asus-wmi would access the led cdev to unregister it.
>>>>> + *
>>>>> + * A spinlock is used, because the protected variables can be accessed
>>>>> + * from an IRQ context from asus-hid.
>>>>> + */
>>>>> + .lock = __SPIN_LOCK_UNLOCKED(asus_ref.lock),
>>>>> +};
>>>>> +
>>>>> +/*
>>>>> + * Allows registering hid-asus listeners that want to be notified of
>>>>> + * keyboard backlight changes.
>>>>> + */
>>>>> +int asus_hid_register_listener(struct asus_hid_listener *bdev)
>>>>> +{
>>>>> + struct asus_wmi *asus;
>>>>> +
>>>>> + guard(spinlock_irqsave)(&asus_ref.lock);
>>>>> + list_add_tail(&bdev->list, &asus_ref.listeners);
>>>>> + asus = asus_ref.asus;
>>>>> + if (asus)
>>>>> + queue_work(asus->led_workqueue, &asus->kbd_led_work);
>>>>> + return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(asus_hid_register_listener);
>>>>> +
>>>>> +/*
>>>>> + * Allows unregistering hid-asus listeners that were added with
>>>>> + * asus_hid_register_listener().
>>>>> + */
>>>>> +void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
>>>>> +{
>>>>> + guard(spinlock_irqsave)(&asus_ref.lock);
>>>>> + list_del(&bdev->list);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(asus_hid_unregister_listener);
>>>>> +
>>>>> +static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
>>>>> +
>>>>> +static void kbd_led_update_all(struct work_struct *work)
>>>>> +{
>>>>> + struct asus_wmi *asus;
>>>>> + bool registered, notify;
>>>>> + int ret, value;
>>>>> +
>>>>> + asus = container_of(work, struct asus_wmi, kbd_led_work);
>>>>> +
>>>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
>>>>> + registered = asus->kbd_led_registered;
>>>>> + value = asus->kbd_led_wk;
>>>>> + notify = asus->kbd_led_notify;
>>>>> + }
>>>>> +
>>>>> + if (!registered) {
>>>>> + /*
>>>>> + * This workqueue runs under asus-wmi, which means probe has
>>>>> + * completed and asus-wmi will keep running until it finishes.
>>>>> + * Therefore, we can safely register the LED without holding
>>>>> + * a spinlock.
>>>>> + */
>>>>> + ret = devm_led_classdev_register(&asus->platform_device->dev,
>>>>> + &asus->kbd_led);
>>>>> + if (!ret) {
>>>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
>>>>> + asus->kbd_led_registered = true;
>>>>> + } else {
>>>>> + pr_warn("Failed to register keyboard backlight LED: %d\n", ret);
>>>>> + return;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + if (value >= 0)
>>>>> + do_kbd_led_set(&asus->kbd_led, value);
>>>>> + if (notify) {
>>>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
>>>>> + asus->kbd_led_notify = false;
>>>>> + led_classdev_notify_brightness_hw_changed(&asus->kbd_led, value);
>>>>> + }
>>>>> +}
>>>>> +
>>>>> /*
>>>>> * These functions actually update the LED's, and are called from a
>>>>> * workqueue. By doing this as separate work rather than when the LED
>>>>> @@ -1661,7 +1765,8 @@ static void kbd_led_update(struct asus_wmi *asus)
>>>>> {
>>>>> int ctrl_param = 0;
>>>>>
>>>>> - ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
>>>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
>>>>> + ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
>>>>> asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
>>>>> }
>>>>>
>>>>> @@ -1694,14 +1799,23 @@ static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
>>>>>
>>>>> static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
>>>>> {
>>>>> + struct asus_hid_listener *listener;
>>>>> struct asus_wmi *asus;
>>>>> int max_level;
>>>>>
>>>>> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>>>>> max_level = asus->kbd_led.max_brightness;
>>>>>
>>>>> - asus->kbd_led_wk = clamp_val(value, 0, max_level);
>>>>> - kbd_led_update(asus);
>>>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
>>>>> + asus->kbd_led_wk = clamp_val(value, 0, max_level);
>>>>> +
>>>>> + if (asus->kbd_led_avail)
>>>>> + kbd_led_update(asus);
>>>>> +
>>>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
>>>>> + list_for_each_entry(listener, &asus_ref.listeners, list)
>>>>> + listener->brightness_set(listener, asus->kbd_led_wk);
>>>>> + }
>>>>> }
>>>>>
>>>>> static int kbd_led_set(struct led_classdev *led_cdev, enum led_brightness value)
>>>>> @@ -1716,10 +1830,11 @@ static int kbd_led_set(struct led_classdev *led_cdev, enum led_brightness value)
>>>>>
>>>>> static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
>>>>> {
>>>>> - struct led_classdev *led_cdev = &asus->kbd_led;
>>>>> -
>>>>> - do_kbd_led_set(led_cdev, value);
>>>>> - led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
>>>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
>>>>> + asus->kbd_led_wk = value;
>>>>> + asus->kbd_led_notify = true;
>>>>> + }
>>>>> + queue_work(asus->led_workqueue, &asus->kbd_led_work);
>>>>> }
>>>>>
>>>>> static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
>>>>> @@ -1729,10 +1844,18 @@ static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
>>>>>
>>>>> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>>>>>
>>>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
>>>>> + if (!asus->kbd_led_avail)
>>>>> + return asus->kbd_led_wk;
>>>>> + }
>>>>> +
>>>>> retval = kbd_led_read(asus, &value, NULL);
>>>>> if (retval < 0)
>>>>> return retval;
>>>>>
>>>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
>>>>> + asus->kbd_led_wk = value;
>>>>> +
>>>>> return value;
>>>>> }
>>>>>
>>>>> @@ -1844,7 +1967,9 @@ static int camera_led_set(struct led_classdev *led_cdev,
>>>>>
>>>>> static void asus_wmi_led_exit(struct asus_wmi *asus)
>>>>> {
>>>>> - led_classdev_unregister(&asus->kbd_led);
>>>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
>>>>> + asus_ref.asus = NULL;
>>>>> +
>>>>> led_classdev_unregister(&asus->tpd_led);
>>>>> led_classdev_unregister(&asus->wlan_led);
>>>>> led_classdev_unregister(&asus->lightbar_led);
>>>>> @@ -1882,22 +2007,26 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
>>>>> goto error;
>>>>> }
>>>>>
>>>>> - if (!kbd_led_read(asus, &led_val, NULL) && !dmi_check_system(asus_use_hid_led_dmi_ids)) {
>>>>> - pr_info("using asus-wmi for asus::kbd_backlight\n");
>>>>> - asus->kbd_led_wk = led_val;
>>>>> - asus->kbd_led.name = "asus::kbd_backlight";
>>>>> - asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
>>>>> - asus->kbd_led.brightness_set_blocking = kbd_led_set;
>>>>> - asus->kbd_led.brightness_get = kbd_led_get;
>>>>> - asus->kbd_led.max_brightness = 3;
>>>>> + asus->kbd_led.name = "asus::kbd_backlight";
>>>>> + asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
>>>>> + asus->kbd_led.brightness_set_blocking = kbd_led_set;
>>>>> + asus->kbd_led.brightness_get = kbd_led_get;
>>>>> + asus->kbd_led.max_brightness = 3;
>>>>> + asus->kbd_led_avail = !kbd_led_read(asus, &led_val, NULL);
>>>>> + INIT_WORK(&asus->kbd_led_work, kbd_led_update_all);
>>>>>
>>>>> + if (asus->kbd_led_avail) {
>>>>> + asus->kbd_led_wk = led_val;
>>>>> if (num_rgb_groups != 0)
>>>>> asus->kbd_led.groups = kbd_rgb_mode_groups;
>>>>> + } else {
>>>>> + asus->kbd_led_wk = -1;
>>>>> + }
>>>>>
>>>>> - rv = led_classdev_register(&asus->platform_device->dev,
>>>>> - &asus->kbd_led);
>>>>> - if (rv)
>>>>> - goto error;
>>>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
>>>>> + asus_ref.asus = asus;
>>>>> + if (asus->kbd_led_avail || !list_empty(&asus_ref.listeners))
>>>>> + queue_work(asus->led_workqueue, &asus->kbd_led_work);
>>>>> }
>>>>>
>>>>> if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_WIRELESS_LED)
>>>>> @@ -4372,6 +4501,7 @@ static int asus_wmi_get_event_code(union acpi_object *obj)
>>>>>
>>>>> static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
>>>>> {
>>>>> + enum led_brightness led_value;
>>>>> unsigned int key_value = 1;
>>>>> bool autorelease = 1;
>>>>>
>>>>> @@ -4388,19 +4518,22 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
>>>>> return;
>>>>> }
>>>>>
>>>>> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
>>>>> + led_value = asus->kbd_led_wk;
>>>>> +
>>>>> if (code == NOTIFY_KBD_BRTUP) {
>>>>> - kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
>>>>> + kbd_led_set_by_kbd(asus, led_value + 1);
>>>>> return;
>>>>> }
>>>>> if (code == NOTIFY_KBD_BRTDWN) {
>>>>> - kbd_led_set_by_kbd(asus, asus->kbd_led_wk - 1);
>>>>> + kbd_led_set_by_kbd(asus, led_value - 1);
>>>>> return;
>>>>> }
>>>>> if (code == NOTIFY_KBD_BRTTOGGLE) {
>>>>> - if (asus->kbd_led_wk == asus->kbd_led.max_brightness)
>>>>> + if (led_value == asus->kbd_led.max_brightness)
>>>>> kbd_led_set_by_kbd(asus, 0);
>>>> This is the toggle leds button, right? I would expect that pressing the toggle
>>>> button turns off leds if they are on and turns them on if they are off.
>>>>
>>>> so if (led_value > 0) { .... }.
>>>>
>>>> I see the previous code was equivalent to yours but is that what we want?
>>> It is common to do 0->1->2->3->0 for the toggle. This is what is
>>> currently done for WMI Asus keyboards and e.g., Thinkpads. This patch
>>> unifies the behavior for USB keyboards too.
>>>
>>> I would argue it is better, as you do not need to reach for a
>>> userspace slider to set a lower brightness.
>>>
>>> The current behavior of KDE is 0->3->0 and if the event goes to
>>> userspace this is what happens currently. Unless the keyboard
>>> reconnects, where brightness just stops working afterwards because
>>> upower only probes at boot (I have a follow patch to fix this for
>>> Duos).
>> Whatever goes for me if hid and wmi handles them the same,
>> especially if userspace does the same thing.
>>
>> In 11/11 you do:
>>
>> caseASUS_EV_BRTTOGGLE:
>> if(brightness >=ASUS_EV_MAX_BRIGHTNESS)
>> brightness =0;
>> else
>> brightness +=1;
>> break;
>> }
>>
>> So perhaps here you should do
>>
>> if (led_value >= asus->kbd_led.max_brightness)
>>
>>
>> purely for consistency?
> If I reroll the series I will do it, otherwise you can do a followup
The current version is functionally okay.
I would only change the name of _LEGACY and see this series merged.
Or I can do it after it has been merged together with this change,
whatever maintainers think it's best.
> Antheas
>
>>> Antheas
>>>
>>>>> else
>>>>> - kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
>>>>> + kbd_led_set_by_kbd(asus, led_value + 1);
>>>>> return;
>>>>> }
>>>>>
>>>>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
>>>>> index 419491d4abca..d347cffd05d5 100644
>>>>> --- a/include/linux/platform_data/x86/asus-wmi.h
>>>>> +++ b/include/linux/platform_data/x86/asus-wmi.h
>>>>> @@ -172,12 +172,20 @@ enum asus_ally_mcu_hack {
>>>>> ASUS_WMI_ALLY_MCU_HACK_DISABLED,
>>>>> };
>>>>>
>>>>> +/* Used to notify hid-asus when asus-wmi changes keyboard backlight */
>>>>> +struct asus_hid_listener {
>>>>> + struct list_head list;
>>>>> + void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
>>>>> +};
>>>>> +
>>>>> #if IS_REACHABLE(CONFIG_ASUS_WMI)
>>>>> void set_ally_mcu_hack(enum asus_ally_mcu_hack status);
>>>>> void set_ally_mcu_powersave(bool enabled);
>>>>> int asus_wmi_get_devstate_dsts(u32 dev_id, u32 *retval);
>>>>> int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval);
>>>>> int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
>>>>> +int asus_hid_register_listener(struct asus_hid_listener *cdev);
>>>>> +void asus_hid_unregister_listener(struct asus_hid_listener *cdev);
>>>>> #else
>>>>> static inline void set_ally_mcu_hack(enum asus_ally_mcu_hack status)
>>>>> {
>>>>> @@ -198,6 +206,13 @@ static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
>>>>> {
>>>>> return -ENODEV;
>>>>> }
>>>>> +static inline int asus_hid_register_listener(struct asus_hid_listener *bdev)
>>>>> +{
>>>>> + return -ENODEV;
>>>>> +}
>>>>> +static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
>>>>> +{
>>>>> +}
>>>>> #endif
>>>>>
>>>>> #endif /* __PLATFORM_DATA_X86_ASUS_WMI_H */
^ permalink raw reply
* Re: [PATCH v11 02/11] HID: asus: initialize additional endpoints only for legacy devices
From: Antheas Kapenekakis @ 2026-01-17 15:07 UTC (permalink / raw)
To: Denis Benato
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
In-Reply-To: <fe66c416-161c-489e-a38f-9dc7729c6ed7@linux.dev>
On Sat, 17 Jan 2026 at 14:51, Denis Benato <denis.benato@linux.dev> wrote:
>
>
> On 1/17/26 00:10, Antheas Kapenekakis wrote:
> > On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@linux.dev> wrote:
> >> On 1/16/26 14:31, Antheas Kapenekakis wrote:
> >>
> >>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
> >>> However, ID1 initializations are only required for RGB control and are
> >>> only supported for RGB capable devices. ID2 initializations are only
> >>> required for initializing the Anime display endpoint which is only
> >>> supported on devices with an Anime display. Both of these
> >>> initializations are out of scope for this driver (this is a brightness
> >>> control and keyboard shortcut driver) and they should not be performed
> >>> for devices that do not support them in any case.
> >>>
> >>> At the same time, there are older NKEY devices that have only been
> >>> tested with these initializations in the kernel and it is not possible
> >>> to recheck them. There is a possibility that especially with the ID1
> >>> initialization, certain laptop models might have their shortcuts stop
> >>> working (currently unproven).
> >>>
> >>> For an abundance of caution, only initialize ID1/ID2 for those older
> >>> NKEY devices by introducing a quirk for them and replacing the NKEY
> >>> quirk in the block that performs the inits with that.
> >>>
> >>> In addition, as these initializations might not be supported by the
> >>> affected devices, change the function to not bail if they fail.
> >>>
> >>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
> >>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>> ---
> >>> drivers/hid/hid-asus.c | 16 ++++++----------
> >>> 1 file changed, 6 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> >>> index 323e6302bac5..dc7af12cf31a 100644
> >>> --- a/drivers/hid/hid-asus.c
> >>> +++ b/drivers/hid/hid-asus.c
> >>> @@ -90,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> >>> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
> >>> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
> >>> #define QUIRK_ROG_ALLY_XPAD BIT(13)
> >>> +#define QUIRK_ROG_NKEY_LEGACY BIT(14)
> >> These past days I have taken a look at new 2025 models and they do make use of ID2,
> >> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
> > Hi Denis,
> > it is not the responsibility of this driver. ID2 is used by Anime
> > models. It is a concession to make sure that we do not cause a
> > regression that will cause warnings for a lot of users.
> Who decided it is a concession?
I would rather remove the extra calls unless they are shown to be
needed, which they might be for these PIDs.
The quirk is named legacy because we can't retest these devices. If we
can, then we could remove the quirk and the inits if not needed.
Antheas
> Anyway I will move relevant code tied to these two to this driver,
> so it doesn't make sense to remove them anyway.
> >> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
> >> No consequences.
> > In your laptop. In the other user's laptop, the get feature report fails
> for the response to be a failure (as it is supposed to be in mine and other models)
> and to cause problems are two different things. Here I am saying that the hardware
> correctly reports "unsupported" and nothing bad happens (if you ignore the return value).
> >> Regardless the name is wrong: mine is a 2023 rog strix with
> >> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
> >> and surely isn't legacy.
> > Sure, can you try removing the if block?
> I have asked to distribute a kernel that init ID1 and ID2 regardless
> of that quirk. We will soon know if it causes problems or not.
> > If it works in your laptop, that is one less reason to keep it for 19b6
> If it works in my laptop one more reason not to exclude code that
> works and haven't caused any problem ever.
> > Antheas
> >
> >>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
> >>> QUIRK_NO_INIT_REPORTS | \
> >>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> >>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>> return -ENODEV;
> >>>
> >>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>> - if (ret < 0)
> >>> - return ret;
> >>> -
> >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>> - if (ret < 0)
> >>> - return ret;
> >>> + if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
> >>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>> }
> >>>
> >>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
> >>> QUIRK_USE_KBD_BACKLIGHT },
> >>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
> >>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
> >>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
> >>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> > On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@linux.dev> wrote:
> >> On 1/16/26 14:31, Antheas Kapenekakis wrote:
> >>
> >>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
> >>> However, ID1 initializations are only required for RGB control and are
> >>> only supported for RGB capable devices. ID2 initializations are only
> >>> required for initializing the Anime display endpoint which is only
> >>> supported on devices with an Anime display. Both of these
> >>> initializations are out of scope for this driver (this is a brightness
> >>> control and keyboard shortcut driver) and they should not be performed
> >>> for devices that do not support them in any case.
> >>>
> >>> At the same time, there are older NKEY devices that have only been
> >>> tested with these initializations in the kernel and it is not possible
> >>> to recheck them. There is a possibility that especially with the ID1
> >>> initialization, certain laptop models might have their shortcuts stop
> >>> working (currently unproven).
> >>>
> >>> For an abundance of caution, only initialize ID1/ID2 for those older
> >>> NKEY devices by introducing a quirk for them and replacing the NKEY
> >>> quirk in the block that performs the inits with that.
> >>>
> >>> In addition, as these initializations might not be supported by the
> >>> affected devices, change the function to not bail if they fail.
> >>>
> >>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
> >>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>> ---
> >>> drivers/hid/hid-asus.c | 16 ++++++----------
> >>> 1 file changed, 6 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> >>> index 323e6302bac5..dc7af12cf31a 100644
> >>> --- a/drivers/hid/hid-asus.c
> >>> +++ b/drivers/hid/hid-asus.c
> >>> @@ -90,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> >>> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
> >>> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
> >>> #define QUIRK_ROG_ALLY_XPAD BIT(13)
> >>> +#define QUIRK_ROG_NKEY_LEGACY BIT(14)
> >> These past days I have taken a look at new 2025 models and they do make use of ID2,
> >> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
> >>
> >> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
> >> No consequences.
> >>
> >> Regardless the name is wrong: mine is a 2023 rog strix with
> >> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
> >> and surely isn't legacy.
> >>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
> >>> QUIRK_NO_INIT_REPORTS | \
> >>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> >>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>> return -ENODEV;
> >>>
> >>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>> - if (ret < 0)
> >>> - return ret;
> >>> -
> >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>> - if (ret < 0)
> >>> - return ret;
> >>> + if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
> >>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>> }
> >>>
> >>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
> >>> QUIRK_USE_KBD_BACKLIGHT },
> >>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
> >>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
> >>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
> >>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>
^ permalink raw reply
* Re: [PATCH v11 02/11] HID: asus: initialize additional endpoints only for legacy devices
From: Denis Benato @ 2026-01-17 15:13 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
In-Reply-To: <CAGwozwESNrQZ240_qDMf1pdpCRXooxstGcqP10cGp=q-F_SrAQ@mail.gmail.com>
On 1/17/26 16:07, Antheas Kapenekakis wrote:
> On Sat, 17 Jan 2026 at 14:51, Denis Benato <denis.benato@linux.dev> wrote:
>>
>> On 1/17/26 00:10, Antheas Kapenekakis wrote:
>>> On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@linux.dev> wrote:
>>>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
>>>>
>>>>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
>>>>> However, ID1 initializations are only required for RGB control and are
>>>>> only supported for RGB capable devices. ID2 initializations are only
>>>>> required for initializing the Anime display endpoint which is only
>>>>> supported on devices with an Anime display. Both of these
>>>>> initializations are out of scope for this driver (this is a brightness
>>>>> control and keyboard shortcut driver) and they should not be performed
>>>>> for devices that do not support them in any case.
>>>>>
>>>>> At the same time, there are older NKEY devices that have only been
>>>>> tested with these initializations in the kernel and it is not possible
>>>>> to recheck them. There is a possibility that especially with the ID1
>>>>> initialization, certain laptop models might have their shortcuts stop
>>>>> working (currently unproven).
>>>>>
>>>>> For an abundance of caution, only initialize ID1/ID2 for those older
>>>>> NKEY devices by introducing a quirk for them and replacing the NKEY
>>>>> quirk in the block that performs the inits with that.
>>>>>
>>>>> In addition, as these initializations might not be supported by the
>>>>> affected devices, change the function to not bail if they fail.
>>>>>
>>>>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>>>> ---
>>>>> drivers/hid/hid-asus.c | 16 ++++++----------
>>>>> 1 file changed, 6 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>>>> index 323e6302bac5..dc7af12cf31a 100644
>>>>> --- a/drivers/hid/hid-asus.c
>>>>> +++ b/drivers/hid/hid-asus.c
>>>>> @@ -90,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>>>>> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
>>>>> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
>>>>> #define QUIRK_ROG_ALLY_XPAD BIT(13)
>>>>> +#define QUIRK_ROG_NKEY_LEGACY BIT(14)
>>>> These past days I have taken a look at new 2025 models and they do make use of ID2,
>>>> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
>>> Hi Denis,
>>> it is not the responsibility of this driver. ID2 is used by Anime
>>> models. It is a concession to make sure that we do not cause a
>>> regression that will cause warnings for a lot of users.
>> Who decided it is a concession?
> I would rather remove the extra calls unless they are shown to be
> needed, which they might be for these PIDs.
They are needed on older laptop and to not regress userspace.
You just named _LEGACY an usb pid that is not legacy.
> The quirk is named legacy because we can't retest these devices. If we
> can, then we could remove the quirk and the inits if not needed.
We can't retest every device, and that pid is used in pre-2021 models,
and these are the unknown, I am criticizing the name of the quirk here,
not what it does.
I am also questioning if the quirk is even needed since sending
those commands to (at least) recent hardware that doesn't use
those endpoints carries no downsides, while removing them
surely does.
> Antheas
>
>> Anyway I will move relevant code tied to these two to this driver,
>> so it doesn't make sense to remove them anyway.
>>>> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
>>>> No consequences.
>>> In your laptop. In the other user's laptop, the get feature report fails
>> for the response to be a failure (as it is supposed to be in mine and other models)
>> and to cause problems are two different things. Here I am saying that the hardware
>> correctly reports "unsupported" and nothing bad happens (if you ignore the return value).
>>>> Regardless the name is wrong: mine is a 2023 rog strix with
>>>> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
>>>> and surely isn't legacy.
>>> Sure, can you try removing the if block?
>> I have asked to distribute a kernel that init ID1 and ID2 regardless
>> of that quirk. We will soon know if it causes problems or not.
>>> If it works in your laptop, that is one less reason to keep it for 19b6
>> If it works in my laptop one more reason not to exclude code that
>> works and haven't caused any problem ever.
>>> Antheas
>>>
>>>>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
>>>>> QUIRK_NO_INIT_REPORTS | \
>>>>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>>>>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>>>> return -ENODEV;
>>>>>
>>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>>>> - if (ret < 0)
>>>>> - return ret;
>>>>> -
>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>>>> - if (ret < 0)
>>>>> - return ret;
>>>>> + if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>>>> }
>>>>>
>>>>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
>>>>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
>>>>> QUIRK_USE_KBD_BACKLIGHT },
>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
>>>>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>> On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@linux.dev> wrote:
>>>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
>>>>
>>>>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
>>>>> However, ID1 initializations are only required for RGB control and are
>>>>> only supported for RGB capable devices. ID2 initializations are only
>>>>> required for initializing the Anime display endpoint which is only
>>>>> supported on devices with an Anime display. Both of these
>>>>> initializations are out of scope for this driver (this is a brightness
>>>>> control and keyboard shortcut driver) and they should not be performed
>>>>> for devices that do not support them in any case.
>>>>>
>>>>> At the same time, there are older NKEY devices that have only been
>>>>> tested with these initializations in the kernel and it is not possible
>>>>> to recheck them. There is a possibility that especially with the ID1
>>>>> initialization, certain laptop models might have their shortcuts stop
>>>>> working (currently unproven).
>>>>>
>>>>> For an abundance of caution, only initialize ID1/ID2 for those older
>>>>> NKEY devices by introducing a quirk for them and replacing the NKEY
>>>>> quirk in the block that performs the inits with that.
>>>>>
>>>>> In addition, as these initializations might not be supported by the
>>>>> affected devices, change the function to not bail if they fail.
>>>>>
>>>>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>>>> ---
>>>>> drivers/hid/hid-asus.c | 16 ++++++----------
>>>>> 1 file changed, 6 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>>>> index 323e6302bac5..dc7af12cf31a 100644
>>>>> --- a/drivers/hid/hid-asus.c
>>>>> +++ b/drivers/hid/hid-asus.c
>>>>> @@ -90,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>>>>> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
>>>>> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
>>>>> #define QUIRK_ROG_ALLY_XPAD BIT(13)
>>>>> +#define QUIRK_ROG_NKEY_LEGACY BIT(14)
>>>> These past days I have taken a look at new 2025 models and they do make use of ID2,
>>>> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
>>>>
>>>> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
>>>> No consequences.
>>>>
>>>> Regardless the name is wrong: mine is a 2023 rog strix with
>>>> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
>>>> and surely isn't legacy.
>>>>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
>>>>> QUIRK_NO_INIT_REPORTS | \
>>>>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>>>>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>>>> return -ENODEV;
>>>>>
>>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>>>> - if (ret < 0)
>>>>> - return ret;
>>>>> -
>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>>>> - if (ret < 0)
>>>>> - return ret;
>>>>> + if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>>>> }
>>>>>
>>>>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
>>>>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
>>>>> QUIRK_USE_KBD_BACKLIGHT },
>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
>>>>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
^ permalink raw reply
* Re: [PATCH v4 1/3] dt-bindings: touchscreen: trivial-touch: Drop 'interrupts' requirement for old Ilitek
From: Marek Vasut @ 2026-01-17 15:33 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: linux-input, Frank Li, Conor Dooley, Dmitry Torokhov, Job Noorman,
Krzysztof Kozlowski, Rob Herring, devicetree, linux-kernel,
linux-renesas-soc
In-Reply-To: <20260117-grinning-heavy-crab-11f245@quoll>
On 1/17/26 12:22 PM, Krzysztof Kozlowski wrote:
> On Sat, Jan 17, 2026 at 01:12:02AM +0100, Marek Vasut wrote:
>> The old Ilitek touch controllers V3 and V6 can operate without
>> interrupt line, in polling mode. Drop the 'interrupts' property
>> requirement for those four controllers. To avoid overloading the
>> trivial-touch, fork the old Ilitek V3/V6 touch controller binding
>> into separate document.
>
> One if: block is fine, so IMO, this should stay in original binding
> especially that more devices like some azoteq or semtech might have same
> rule of not requiring interrupt line. Anyway, no big deal.
I am not sure about the other non-ilitek devices, but the fruitboards do
use at least goodix and etm/edt touch controllers without interrupt line
too, those I have on my desk (those two have separate, more extensive,
binding document). I also suspect we will see more of those touch
controllers with optional interrupt line, so if we do, I think we can
re-combine the binding documents again ?
^ permalink raw reply
* Re: [PATCH v11 02/11] HID: asus: initialize additional endpoints only for legacy devices
From: Antheas Kapenekakis @ 2026-01-17 16:10 UTC (permalink / raw)
To: Denis Benato
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
In-Reply-To: <8102eb26-5206-49e6-a994-a9d3ea480255@linux.dev>
On Sat, 17 Jan 2026 at 16:13, Denis Benato <denis.benato@linux.dev> wrote:
>
>
> On 1/17/26 16:07, Antheas Kapenekakis wrote:
> > On Sat, 17 Jan 2026 at 14:51, Denis Benato <denis.benato@linux.dev> wrote:
> >>
> >> On 1/17/26 00:10, Antheas Kapenekakis wrote:
> >>> On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@linux.dev> wrote:
> >>>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
> >>>>
> >>>>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
> >>>>> However, ID1 initializations are only required for RGB control and are
> >>>>> only supported for RGB capable devices. ID2 initializations are only
> >>>>> required for initializing the Anime display endpoint which is only
> >>>>> supported on devices with an Anime display. Both of these
> >>>>> initializations are out of scope for this driver (this is a brightness
> >>>>> control and keyboard shortcut driver) and they should not be performed
> >>>>> for devices that do not support them in any case.
> >>>>>
> >>>>> At the same time, there are older NKEY devices that have only been
> >>>>> tested with these initializations in the kernel and it is not possible
> >>>>> to recheck them. There is a possibility that especially with the ID1
> >>>>> initialization, certain laptop models might have their shortcuts stop
> >>>>> working (currently unproven).
> >>>>>
> >>>>> For an abundance of caution, only initialize ID1/ID2 for those older
> >>>>> NKEY devices by introducing a quirk for them and replacing the NKEY
> >>>>> quirk in the block that performs the inits with that.
> >>>>>
> >>>>> In addition, as these initializations might not be supported by the
> >>>>> affected devices, change the function to not bail if they fail.
> >>>>>
> >>>>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
> >>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>>>> ---
> >>>>> drivers/hid/hid-asus.c | 16 ++++++----------
> >>>>> 1 file changed, 6 insertions(+), 10 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> >>>>> index 323e6302bac5..dc7af12cf31a 100644
> >>>>> --- a/drivers/hid/hid-asus.c
> >>>>> +++ b/drivers/hid/hid-asus.c
> >>>>> @@ -90,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> >>>>> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
> >>>>> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
> >>>>> #define QUIRK_ROG_ALLY_XPAD BIT(13)
> >>>>> +#define QUIRK_ROG_NKEY_LEGACY BIT(14)
> >>>> These past days I have taken a look at new 2025 models and they do make use of ID2,
> >>>> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
> >>> Hi Denis,
> >>> it is not the responsibility of this driver. ID2 is used by Anime
> >>> models. It is a concession to make sure that we do not cause a
> >>> regression that will cause warnings for a lot of users.
> >> Who decided it is a concession?
> > I would rather remove the extra calls unless they are shown to be
> > needed, which they might be for these PIDs.
> They are needed on older laptop and to not regress userspace.
>
> You just named _LEGACY an usb pid that is not legacy.
> > The quirk is named legacy because we can't retest these devices. If we
> > can, then we could remove the quirk and the inits if not needed.
> We can't retest every device, and that pid is used in pre-2021 models,
> and these are the unknown, I am criticizing the name of the quirk here,
> not what it does.
If you can test whether your device needs them that would be great.
> I am also questioning if the quirk is even needed since sending
> those commands to (at least) recent hardware that doesn't use
> those endpoints carries no downsides, while removing them
> surely does.
We have not found a device yet that needs them. I do not want to keep
sending unneeded commands. It could cause obscure bugs or interfere
with userspace software such as the one you maintain. So at least for
new hardware that is possible to test we should remove them.
Antheas
> > Antheas
> >
> >> Anyway I will move relevant code tied to these two to this driver,
> >> so it doesn't make sense to remove them anyway.
> >>>> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
> >>>> No consequences.
> >>> In your laptop. In the other user's laptop, the get feature report fails
> >> for the response to be a failure (as it is supposed to be in mine and other models)
> >> and to cause problems are two different things. Here I am saying that the hardware
> >> correctly reports "unsupported" and nothing bad happens (if you ignore the return value).
> >>>> Regardless the name is wrong: mine is a 2023 rog strix with
> >>>> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
> >>>> and surely isn't legacy.
> >>> Sure, can you try removing the if block?
> >> I have asked to distribute a kernel that init ID1 and ID2 regardless
> >> of that quirk. We will soon know if it causes problems or not.
> >>> If it works in your laptop, that is one less reason to keep it for 19b6
> >> If it works in my laptop one more reason not to exclude code that
> >> works and haven't caused any problem ever.
> >>> Antheas
> >>>
> >>>>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
> >>>>> QUIRK_NO_INIT_REPORTS | \
> >>>>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> >>>>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>>>> return -ENODEV;
> >>>>>
> >>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> >>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>>>> - if (ret < 0)
> >>>>> - return ret;
> >>>>> -
> >>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>>>> - if (ret < 0)
> >>>>> - return ret;
> >>>>> + if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
> >>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>>>> }
> >>>>>
> >>>>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>>>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
> >>>>> QUIRK_USE_KBD_BACKLIGHT },
> >>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
> >>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
> >>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
> >>>>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>> On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@linux.dev> wrote:
> >>>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
> >>>>
> >>>>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
> >>>>> However, ID1 initializations are only required for RGB control and are
> >>>>> only supported for RGB capable devices. ID2 initializations are only
> >>>>> required for initializing the Anime display endpoint which is only
> >>>>> supported on devices with an Anime display. Both of these
> >>>>> initializations are out of scope for this driver (this is a brightness
> >>>>> control and keyboard shortcut driver) and they should not be performed
> >>>>> for devices that do not support them in any case.
> >>>>>
> >>>>> At the same time, there are older NKEY devices that have only been
> >>>>> tested with these initializations in the kernel and it is not possible
> >>>>> to recheck them. There is a possibility that especially with the ID1
> >>>>> initialization, certain laptop models might have their shortcuts stop
> >>>>> working (currently unproven).
> >>>>>
> >>>>> For an abundance of caution, only initialize ID1/ID2 for those older
> >>>>> NKEY devices by introducing a quirk for them and replacing the NKEY
> >>>>> quirk in the block that performs the inits with that.
> >>>>>
> >>>>> In addition, as these initializations might not be supported by the
> >>>>> affected devices, change the function to not bail if they fail.
> >>>>>
> >>>>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
> >>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>>>> ---
> >>>>> drivers/hid/hid-asus.c | 16 ++++++----------
> >>>>> 1 file changed, 6 insertions(+), 10 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> >>>>> index 323e6302bac5..dc7af12cf31a 100644
> >>>>> --- a/drivers/hid/hid-asus.c
> >>>>> +++ b/drivers/hid/hid-asus.c
> >>>>> @@ -90,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> >>>>> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
> >>>>> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
> >>>>> #define QUIRK_ROG_ALLY_XPAD BIT(13)
> >>>>> +#define QUIRK_ROG_NKEY_LEGACY BIT(14)
> >>>> These past days I have taken a look at new 2025 models and they do make use of ID2,
> >>>> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
> >>>>
> >>>> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
> >>>> No consequences.
> >>>>
> >>>> Regardless the name is wrong: mine is a 2023 rog strix with
> >>>> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
> >>>> and surely isn't legacy.
> >>>>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
> >>>>> QUIRK_NO_INIT_REPORTS | \
> >>>>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> >>>>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>>>> return -ENODEV;
> >>>>>
> >>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> >>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>>>> - if (ret < 0)
> >>>>> - return ret;
> >>>>> -
> >>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>>>> - if (ret < 0)
> >>>>> - return ret;
> >>>>> + if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
> >>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>>>> }
> >>>>>
> >>>>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>>>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
> >>>>> QUIRK_USE_KBD_BACKLIGHT },
> >>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
> >>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
> >>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
> >>>>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>
^ permalink raw reply
* Re: [PATCH v11 02/11] HID: asus: initialize additional endpoints only for legacy devices
From: Denis Benato @ 2026-01-17 16:12 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
In-Reply-To: <CAGwozwH71LDiBFM-Ro9UpZNy29C6HHwCNZjwCS3F1hMfuUXZ4g@mail.gmail.com>
On 1/17/26 17:10, Antheas Kapenekakis wrote:
> On Sat, 17 Jan 2026 at 16:13, Denis Benato <denis.benato@linux.dev> wrote:
>>
>> On 1/17/26 16:07, Antheas Kapenekakis wrote:
>>> On Sat, 17 Jan 2026 at 14:51, Denis Benato <denis.benato@linux.dev> wrote:
>>>> On 1/17/26 00:10, Antheas Kapenekakis wrote:
>>>>> On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@linux.dev> wrote:
>>>>>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
>>>>>>
>>>>>>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
>>>>>>> However, ID1 initializations are only required for RGB control and are
>>>>>>> only supported for RGB capable devices. ID2 initializations are only
>>>>>>> required for initializing the Anime display endpoint which is only
>>>>>>> supported on devices with an Anime display. Both of these
>>>>>>> initializations are out of scope for this driver (this is a brightness
>>>>>>> control and keyboard shortcut driver) and they should not be performed
>>>>>>> for devices that do not support them in any case.
>>>>>>>
>>>>>>> At the same time, there are older NKEY devices that have only been
>>>>>>> tested with these initializations in the kernel and it is not possible
>>>>>>> to recheck them. There is a possibility that especially with the ID1
>>>>>>> initialization, certain laptop models might have their shortcuts stop
>>>>>>> working (currently unproven).
>>>>>>>
>>>>>>> For an abundance of caution, only initialize ID1/ID2 for those older
>>>>>>> NKEY devices by introducing a quirk for them and replacing the NKEY
>>>>>>> quirk in the block that performs the inits with that.
>>>>>>>
>>>>>>> In addition, as these initializations might not be supported by the
>>>>>>> affected devices, change the function to not bail if they fail.
>>>>>>>
>>>>>>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
>>>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>>>>>> ---
>>>>>>> drivers/hid/hid-asus.c | 16 ++++++----------
>>>>>>> 1 file changed, 6 insertions(+), 10 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>>>>>> index 323e6302bac5..dc7af12cf31a 100644
>>>>>>> --- a/drivers/hid/hid-asus.c
>>>>>>> +++ b/drivers/hid/hid-asus.c
>>>>>>> @@ -90,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>>>>>>> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
>>>>>>> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
>>>>>>> #define QUIRK_ROG_ALLY_XPAD BIT(13)
>>>>>>> +#define QUIRK_ROG_NKEY_LEGACY BIT(14)
>>>>>> These past days I have taken a look at new 2025 models and they do make use of ID2,
>>>>>> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
>>>>> Hi Denis,
>>>>> it is not the responsibility of this driver. ID2 is used by Anime
>>>>> models. It is a concession to make sure that we do not cause a
>>>>> regression that will cause warnings for a lot of users.
>>>> Who decided it is a concession?
>>> I would rather remove the extra calls unless they are shown to be
>>> needed, which they might be for these PIDs.
>> They are needed on older laptop and to not regress userspace.
>>
>> You just named _LEGACY an usb pid that is not legacy.
>>> The quirk is named legacy because we can't retest these devices. If we
>>> can, then we could remove the quirk and the inits if not needed.
>> We can't retest every device, and that pid is used in pre-2021 models,
>> and these are the unknown, I am criticizing the name of the quirk here,
>> not what it does.
> If you can test whether your device needs them that would be great.
That is pointless.
>> I am also questioning if the quirk is even needed since sending
>> those commands to (at least) recent hardware that doesn't use
>> those endpoints carries no downsides, while removing them
>> surely does.
> We have not found a device yet that needs them. I do not want to keep
> sending unneeded commands. It could cause obscure bugs or interfere
> with userspace software such as the one you maintain. So at least for
> new hardware that is possible to test we should remove them.
There is new hardware that needs them, as I said, including 2025 models.
> Antheas
>
>>> Antheas
>>>
>>>> Anyway I will move relevant code tied to these two to this driver,
>>>> so it doesn't make sense to remove them anyway.
>>>>>> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
>>>>>> No consequences.
>>>>> In your laptop. In the other user's laptop, the get feature report fails
>>>> for the response to be a failure (as it is supposed to be in mine and other models)
>>>> and to cause problems are two different things. Here I am saying that the hardware
>>>> correctly reports "unsupported" and nothing bad happens (if you ignore the return value).
>>>>>> Regardless the name is wrong: mine is a 2023 rog strix with
>>>>>> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
>>>>>> and surely isn't legacy.
>>>>> Sure, can you try removing the if block?
>>>> I have asked to distribute a kernel that init ID1 and ID2 regardless
>>>> of that quirk. We will soon know if it causes problems or not.
>>>>> If it works in your laptop, that is one less reason to keep it for 19b6
>>>> If it works in my laptop one more reason not to exclude code that
>>>> works and haven't caused any problem ever.
>>>>> Antheas
>>>>>
>>>>>>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
>>>>>>> QUIRK_NO_INIT_REPORTS | \
>>>>>>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>>>>>>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>>>>>> return -ENODEV;
>>>>>>>
>>>>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>>>>>> - if (ret < 0)
>>>>>>> - return ret;
>>>>>>> -
>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>>>>>> - if (ret < 0)
>>>>>>> - return ret;
>>>>>>> + if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
>>>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>>>>>> }
>>>>>>>
>>>>>>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
>>>>>>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
>>>>>>> QUIRK_USE_KBD_BACKLIGHT },
>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
>>>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
>>>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
>>>>>>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>>> On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@linux.dev> wrote:
>>>>>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
>>>>>>
>>>>>>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
>>>>>>> However, ID1 initializations are only required for RGB control and are
>>>>>>> only supported for RGB capable devices. ID2 initializations are only
>>>>>>> required for initializing the Anime display endpoint which is only
>>>>>>> supported on devices with an Anime display. Both of these
>>>>>>> initializations are out of scope for this driver (this is a brightness
>>>>>>> control and keyboard shortcut driver) and they should not be performed
>>>>>>> for devices that do not support them in any case.
>>>>>>>
>>>>>>> At the same time, there are older NKEY devices that have only been
>>>>>>> tested with these initializations in the kernel and it is not possible
>>>>>>> to recheck them. There is a possibility that especially with the ID1
>>>>>>> initialization, certain laptop models might have their shortcuts stop
>>>>>>> working (currently unproven).
>>>>>>>
>>>>>>> For an abundance of caution, only initialize ID1/ID2 for those older
>>>>>>> NKEY devices by introducing a quirk for them and replacing the NKEY
>>>>>>> quirk in the block that performs the inits with that.
>>>>>>>
>>>>>>> In addition, as these initializations might not be supported by the
>>>>>>> affected devices, change the function to not bail if they fail.
>>>>>>>
>>>>>>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
>>>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>>>>>> ---
>>>>>>> drivers/hid/hid-asus.c | 16 ++++++----------
>>>>>>> 1 file changed, 6 insertions(+), 10 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>>>>>> index 323e6302bac5..dc7af12cf31a 100644
>>>>>>> --- a/drivers/hid/hid-asus.c
>>>>>>> +++ b/drivers/hid/hid-asus.c
>>>>>>> @@ -90,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>>>>>>> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
>>>>>>> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
>>>>>>> #define QUIRK_ROG_ALLY_XPAD BIT(13)
>>>>>>> +#define QUIRK_ROG_NKEY_LEGACY BIT(14)
>>>>>> These past days I have taken a look at new 2025 models and they do make use of ID2,
>>>>>> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
>>>>>>
>>>>>> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
>>>>>> No consequences.
>>>>>>
>>>>>> Regardless the name is wrong: mine is a 2023 rog strix with
>>>>>> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
>>>>>> and surely isn't legacy.
>>>>>>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
>>>>>>> QUIRK_NO_INIT_REPORTS | \
>>>>>>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>>>>>>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>>>>>> return -ENODEV;
>>>>>>>
>>>>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>>>>>> - if (ret < 0)
>>>>>>> - return ret;
>>>>>>> -
>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>>>>>> - if (ret < 0)
>>>>>>> - return ret;
>>>>>>> + if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
>>>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>>>>>> }
>>>>>>>
>>>>>>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
>>>>>>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
>>>>>>> QUIRK_USE_KBD_BACKLIGHT },
>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
>>>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
>>>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
>>>>>>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
^ permalink raw reply
* Re: [PATCH v11 02/11] HID: asus: initialize additional endpoints only for legacy devices
From: Antheas Kapenekakis @ 2026-01-17 16:16 UTC (permalink / raw)
To: Denis Benato
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
In-Reply-To: <be8ba636-ae07-4d42-88ca-57ecf10b7dae@linux.dev>
On Sat, 17 Jan 2026 at 17:12, Denis Benato <denis.benato@linux.dev> wrote:
>
>
> On 1/17/26 17:10, Antheas Kapenekakis wrote:
> > On Sat, 17 Jan 2026 at 16:13, Denis Benato <denis.benato@linux.dev> wrote:
> >>
> >> On 1/17/26 16:07, Antheas Kapenekakis wrote:
> >>> On Sat, 17 Jan 2026 at 14:51, Denis Benato <denis.benato@linux.dev> wrote:
> >>>> On 1/17/26 00:10, Antheas Kapenekakis wrote:
> >>>>> On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@linux.dev> wrote:
> >>>>>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
> >>>>>>
> >>>>>>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
> >>>>>>> However, ID1 initializations are only required for RGB control and are
> >>>>>>> only supported for RGB capable devices. ID2 initializations are only
> >>>>>>> required for initializing the Anime display endpoint which is only
> >>>>>>> supported on devices with an Anime display. Both of these
> >>>>>>> initializations are out of scope for this driver (this is a brightness
> >>>>>>> control and keyboard shortcut driver) and they should not be performed
> >>>>>>> for devices that do not support them in any case.
> >>>>>>>
> >>>>>>> At the same time, there are older NKEY devices that have only been
> >>>>>>> tested with these initializations in the kernel and it is not possible
> >>>>>>> to recheck them. There is a possibility that especially with the ID1
> >>>>>>> initialization, certain laptop models might have their shortcuts stop
> >>>>>>> working (currently unproven).
> >>>>>>>
> >>>>>>> For an abundance of caution, only initialize ID1/ID2 for those older
> >>>>>>> NKEY devices by introducing a quirk for them and replacing the NKEY
> >>>>>>> quirk in the block that performs the inits with that.
> >>>>>>>
> >>>>>>> In addition, as these initializations might not be supported by the
> >>>>>>> affected devices, change the function to not bail if they fail.
> >>>>>>>
> >>>>>>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
> >>>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>>>>>> ---
> >>>>>>> drivers/hid/hid-asus.c | 16 ++++++----------
> >>>>>>> 1 file changed, 6 insertions(+), 10 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> >>>>>>> index 323e6302bac5..dc7af12cf31a 100644
> >>>>>>> --- a/drivers/hid/hid-asus.c
> >>>>>>> +++ b/drivers/hid/hid-asus.c
> >>>>>>> @@ -90,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> >>>>>>> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
> >>>>>>> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
> >>>>>>> #define QUIRK_ROG_ALLY_XPAD BIT(13)
> >>>>>>> +#define QUIRK_ROG_NKEY_LEGACY BIT(14)
> >>>>>> These past days I have taken a look at new 2025 models and they do make use of ID2,
> >>>>>> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
> >>>>> Hi Denis,
> >>>>> it is not the responsibility of this driver. ID2 is used by Anime
> >>>>> models. It is a concession to make sure that we do not cause a
> >>>>> regression that will cause warnings for a lot of users.
> >>>> Who decided it is a concession?
> >>> I would rather remove the extra calls unless they are shown to be
> >>> needed, which they might be for these PIDs.
> >> They are needed on older laptop and to not regress userspace.
> >>
> >> You just named _LEGACY an usb pid that is not legacy.
> >>> The quirk is named legacy because we can't retest these devices. If we
> >>> can, then we could remove the quirk and the inits if not needed.
> >> We can't retest every device, and that pid is used in pre-2021 models,
> >> and these are the unknown, I am criticizing the name of the quirk here,
> >> not what it does.
> > If you can test whether your device needs them that would be great.
> That is pointless.
> >> I am also questioning if the quirk is even needed since sending
> >> those commands to (at least) recent hardware that doesn't use
> >> those endpoints carries no downsides, while removing them
> >> surely does.
> > We have not found a device yet that needs them. I do not want to keep
> > sending unneeded commands. It could cause obscure bugs or interfere
> > with userspace software such as the one you maintain. So at least for
> > new hardware that is possible to test we should remove them.
> There is new hardware that needs them, as I said, including 2025 models.
I was not aware of that. As far as I know they are not needed. Do you
have a bug report with a specific laptop model I can look at?
Antheas
> > Antheas
> >
> >>> Antheas
> >>>
> >>>> Anyway I will move relevant code tied to these two to this driver,
> >>>> so it doesn't make sense to remove them anyway.
> >>>>>> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
> >>>>>> No consequences.
> >>>>> In your laptop. In the other user's laptop, the get feature report fails
> >>>> for the response to be a failure (as it is supposed to be in mine and other models)
> >>>> and to cause problems are two different things. Here I am saying that the hardware
> >>>> correctly reports "unsupported" and nothing bad happens (if you ignore the return value).
> >>>>>> Regardless the name is wrong: mine is a 2023 rog strix with
> >>>>>> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
> >>>>>> and surely isn't legacy.
> >>>>> Sure, can you try removing the if block?
> >>>> I have asked to distribute a kernel that init ID1 and ID2 regardless
> >>>> of that quirk. We will soon know if it causes problems or not.
> >>>>> If it works in your laptop, that is one less reason to keep it for 19b6
> >>>> If it works in my laptop one more reason not to exclude code that
> >>>> works and haven't caused any problem ever.
> >>>>> Antheas
> >>>>>
> >>>>>>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
> >>>>>>> QUIRK_NO_INIT_REPORTS | \
> >>>>>>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> >>>>>>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>>>>>> return -ENODEV;
> >>>>>>>
> >>>>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> >>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>>>>>> - if (ret < 0)
> >>>>>>> - return ret;
> >>>>>>> -
> >>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>>>>>> - if (ret < 0)
> >>>>>>> - return ret;
> >>>>>>> + if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
> >>>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>>>>>> }
> >>>>>>>
> >>>>>>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>>>>>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
> >>>>>>> QUIRK_USE_KBD_BACKLIGHT },
> >>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
> >>>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
> >>>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
> >>>>>>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>> On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@linux.dev> wrote:
> >>>>>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
> >>>>>>
> >>>>>>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
> >>>>>>> However, ID1 initializations are only required for RGB control and are
> >>>>>>> only supported for RGB capable devices. ID2 initializations are only
> >>>>>>> required for initializing the Anime display endpoint which is only
> >>>>>>> supported on devices with an Anime display. Both of these
> >>>>>>> initializations are out of scope for this driver (this is a brightness
> >>>>>>> control and keyboard shortcut driver) and they should not be performed
> >>>>>>> for devices that do not support them in any case.
> >>>>>>>
> >>>>>>> At the same time, there are older NKEY devices that have only been
> >>>>>>> tested with these initializations in the kernel and it is not possible
> >>>>>>> to recheck them. There is a possibility that especially with the ID1
> >>>>>>> initialization, certain laptop models might have their shortcuts stop
> >>>>>>> working (currently unproven).
> >>>>>>>
> >>>>>>> For an abundance of caution, only initialize ID1/ID2 for those older
> >>>>>>> NKEY devices by introducing a quirk for them and replacing the NKEY
> >>>>>>> quirk in the block that performs the inits with that.
> >>>>>>>
> >>>>>>> In addition, as these initializations might not be supported by the
> >>>>>>> affected devices, change the function to not bail if they fail.
> >>>>>>>
> >>>>>>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
> >>>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>>>>>> ---
> >>>>>>> drivers/hid/hid-asus.c | 16 ++++++----------
> >>>>>>> 1 file changed, 6 insertions(+), 10 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> >>>>>>> index 323e6302bac5..dc7af12cf31a 100644
> >>>>>>> --- a/drivers/hid/hid-asus.c
> >>>>>>> +++ b/drivers/hid/hid-asus.c
> >>>>>>> @@ -90,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> >>>>>>> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
> >>>>>>> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
> >>>>>>> #define QUIRK_ROG_ALLY_XPAD BIT(13)
> >>>>>>> +#define QUIRK_ROG_NKEY_LEGACY BIT(14)
> >>>>>> These past days I have taken a look at new 2025 models and they do make use of ID2,
> >>>>>> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
> >>>>>>
> >>>>>> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
> >>>>>> No consequences.
> >>>>>>
> >>>>>> Regardless the name is wrong: mine is a 2023 rog strix with
> >>>>>> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
> >>>>>> and surely isn't legacy.
> >>>>>>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
> >>>>>>> QUIRK_NO_INIT_REPORTS | \
> >>>>>>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> >>>>>>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>>>>>> return -ENODEV;
> >>>>>>>
> >>>>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> >>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>>>>>> - if (ret < 0)
> >>>>>>> - return ret;
> >>>>>>> -
> >>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>>>>>> - if (ret < 0)
> >>>>>>> - return ret;
> >>>>>>> + if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
> >>>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>>>>>> }
> >>>>>>>
> >>>>>>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>>>>>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
> >>>>>>> QUIRK_USE_KBD_BACKLIGHT },
> >>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
> >>>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
> >>>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
> >>>>>>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>
^ permalink raw reply
* Re: [PATCH v11 02/11] HID: asus: initialize additional endpoints only for legacy devices
From: Denis Benato @ 2026-01-17 17:05 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
In-Reply-To: <CAGwozwGdhu_chetK6uQ=FXctC1D8LABj5QxVff1B486EKYSLug@mail.gmail.com>
On 1/17/26 17:16, Antheas Kapenekakis wrote:
> On Sat, 17 Jan 2026 at 17:12, Denis Benato <denis.benato@linux.dev> wrote:
>>
>> On 1/17/26 17:10, Antheas Kapenekakis wrote:
>>> On Sat, 17 Jan 2026 at 16:13, Denis Benato <denis.benato@linux.dev> wrote:
>>>> On 1/17/26 16:07, Antheas Kapenekakis wrote:
>>>>> On Sat, 17 Jan 2026 at 14:51, Denis Benato <denis.benato@linux.dev> wrote:
>>>>>> On 1/17/26 00:10, Antheas Kapenekakis wrote:
>>>>>>> On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@linux.dev> wrote:
>>>>>>>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
>>>>>>>>
>>>>>>>>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
>>>>>>>>> However, ID1 initializations are only required for RGB control and are
>>>>>>>>> only supported for RGB capable devices. ID2 initializations are only
>>>>>>>>> required for initializing the Anime display endpoint which is only
>>>>>>>>> supported on devices with an Anime display. Both of these
>>>>>>>>> initializations are out of scope for this driver (this is a brightness
>>>>>>>>> control and keyboard shortcut driver) and they should not be performed
>>>>>>>>> for devices that do not support them in any case.
>>>>>>>>>
>>>>>>>>> At the same time, there are older NKEY devices that have only been
>>>>>>>>> tested with these initializations in the kernel and it is not possible
>>>>>>>>> to recheck them. There is a possibility that especially with the ID1
>>>>>>>>> initialization, certain laptop models might have their shortcuts stop
>>>>>>>>> working (currently unproven).
>>>>>>>>>
>>>>>>>>> For an abundance of caution, only initialize ID1/ID2 for those older
>>>>>>>>> NKEY devices by introducing a quirk for them and replacing the NKEY
>>>>>>>>> quirk in the block that performs the inits with that.
>>>>>>>>>
>>>>>>>>> In addition, as these initializations might not be supported by the
>>>>>>>>> affected devices, change the function to not bail if they fail.
>>>>>>>>>
>>>>>>>>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
>>>>>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>>>>>>>> ---
>>>>>>>>> drivers/hid/hid-asus.c | 16 ++++++----------
>>>>>>>>> 1 file changed, 6 insertions(+), 10 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>>>>>>>> index 323e6302bac5..dc7af12cf31a 100644
>>>>>>>>> --- a/drivers/hid/hid-asus.c
>>>>>>>>> +++ b/drivers/hid/hid-asus.c
>>>>>>>>> @@ -90,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>>>>>>>>> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
>>>>>>>>> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
>>>>>>>>> #define QUIRK_ROG_ALLY_XPAD BIT(13)
>>>>>>>>> +#define QUIRK_ROG_NKEY_LEGACY BIT(14)
>>>>>>>> These past days I have taken a look at new 2025 models and they do make use of ID2,
>>>>>>>> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
>>>>>>> Hi Denis,
>>>>>>> it is not the responsibility of this driver. ID2 is used by Anime
>>>>>>> models. It is a concession to make sure that we do not cause a
>>>>>>> regression that will cause warnings for a lot of users.
>>>>>> Who decided it is a concession?
>>>>> I would rather remove the extra calls unless they are shown to be
>>>>> needed, which they might be for these PIDs.
>>>> They are needed on older laptop and to not regress userspace.
>>>>
>>>> You just named _LEGACY an usb pid that is not legacy.
>>>>> The quirk is named legacy because we can't retest these devices. If we
>>>>> can, then we could remove the quirk and the inits if not needed.
>>>> We can't retest every device, and that pid is used in pre-2021 models,
>>>> and these are the unknown, I am criticizing the name of the quirk here,
>>>> not what it does.
>>> If you can test whether your device needs them that would be great.
>> That is pointless.
>>>> I am also questioning if the quirk is even needed since sending
>>>> those commands to (at least) recent hardware that doesn't use
>>>> those endpoints carries no downsides, while removing them
>>>> surely does.
>>> We have not found a device yet that needs them. I do not want to keep
>>> sending unneeded commands. It could cause obscure bugs or interfere
>>> with userspace software such as the one you maintain. So at least for
>>> new hardware that is possible to test we should remove them.
>> There is new hardware that needs them, as I said, including 2025 models.
> I was not aware of that. As far as I know they are not needed. Do you
> have a bug report with a specific laptop model I can look at?
There is current effort to integrate commands that requires those
initializations on 2025 laptop, why would I strip out a command
that I already know is required anyway?
No, this is not the way to go to knowingly and willingly cause
troubles (both known and unknown) to others just because
you think it's better this way.
Change the name of _LEGACY to something else, have this accepted
and then if I see it's appropriate to remove the if and send those
regardless I will.
> Antheas
>
>>> Antheas
>>>
>>>>> Antheas
>>>>>
>>>>>> Anyway I will move relevant code tied to these two to this driver,
>>>>>> so it doesn't make sense to remove them anyway.
>>>>>>>> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
>>>>>>>> No consequences.
>>>>>>> In your laptop. In the other user's laptop, the get feature report fails
>>>>>> for the response to be a failure (as it is supposed to be in mine and other models)
>>>>>> and to cause problems are two different things. Here I am saying that the hardware
>>>>>> correctly reports "unsupported" and nothing bad happens (if you ignore the return value).
>>>>>>>> Regardless the name is wrong: mine is a 2023 rog strix with
>>>>>>>> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
>>>>>>>> and surely isn't legacy.
>>>>>>> Sure, can you try removing the if block?
>>>>>> I have asked to distribute a kernel that init ID1 and ID2 regardless
>>>>>> of that quirk. We will soon know if it causes problems or not.
>>>>>>> If it works in your laptop, that is one less reason to keep it for 19b6
>>>>>> If it works in my laptop one more reason not to exclude code that
>>>>>> works and haven't caused any problem ever.
>>>>>>> Antheas
>>>>>>>
>>>>>>>>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
>>>>>>>>> QUIRK_NO_INIT_REPORTS | \
>>>>>>>>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>>>>>>>>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>>>>>>>> return -ENODEV;
>>>>>>>>>
>>>>>>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
>>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>>>>>>>> - if (ret < 0)
>>>>>>>>> - return ret;
>>>>>>>>> -
>>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>>>>>>>> - if (ret < 0)
>>>>>>>>> - return ret;
>>>>>>>>> + if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
>>>>>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>>>>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
>>>>>>>>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
>>>>>>>>> QUIRK_USE_KBD_BACKLIGHT },
>>>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
>>>>>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>>>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
>>>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
>>>>>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>>>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
>>>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
>>>>>>>>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>>>>> On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@linux.dev> wrote:
>>>>>>>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
>>>>>>>>
>>>>>>>>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
>>>>>>>>> However, ID1 initializations are only required for RGB control and are
>>>>>>>>> only supported for RGB capable devices. ID2 initializations are only
>>>>>>>>> required for initializing the Anime display endpoint which is only
>>>>>>>>> supported on devices with an Anime display. Both of these
>>>>>>>>> initializations are out of scope for this driver (this is a brightness
>>>>>>>>> control and keyboard shortcut driver) and they should not be performed
>>>>>>>>> for devices that do not support them in any case.
>>>>>>>>>
>>>>>>>>> At the same time, there are older NKEY devices that have only been
>>>>>>>>> tested with these initializations in the kernel and it is not possible
>>>>>>>>> to recheck them. There is a possibility that especially with the ID1
>>>>>>>>> initialization, certain laptop models might have their shortcuts stop
>>>>>>>>> working (currently unproven).
>>>>>>>>>
>>>>>>>>> For an abundance of caution, only initialize ID1/ID2 for those older
>>>>>>>>> NKEY devices by introducing a quirk for them and replacing the NKEY
>>>>>>>>> quirk in the block that performs the inits with that.
>>>>>>>>>
>>>>>>>>> In addition, as these initializations might not be supported by the
>>>>>>>>> affected devices, change the function to not bail if they fail.
>>>>>>>>>
>>>>>>>>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
>>>>>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>>>>>>>> ---
>>>>>>>>> drivers/hid/hid-asus.c | 16 ++++++----------
>>>>>>>>> 1 file changed, 6 insertions(+), 10 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>>>>>>>> index 323e6302bac5..dc7af12cf31a 100644
>>>>>>>>> --- a/drivers/hid/hid-asus.c
>>>>>>>>> +++ b/drivers/hid/hid-asus.c
>>>>>>>>> @@ -90,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>>>>>>>>> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
>>>>>>>>> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
>>>>>>>>> #define QUIRK_ROG_ALLY_XPAD BIT(13)
>>>>>>>>> +#define QUIRK_ROG_NKEY_LEGACY BIT(14)
>>>>>>>> These past days I have taken a look at new 2025 models and they do make use of ID2,
>>>>>>>> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
>>>>>>>>
>>>>>>>> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
>>>>>>>> No consequences.
>>>>>>>>
>>>>>>>> Regardless the name is wrong: mine is a 2023 rog strix with
>>>>>>>> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
>>>>>>>> and surely isn't legacy.
>>>>>>>>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
>>>>>>>>> QUIRK_NO_INIT_REPORTS | \
>>>>>>>>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>>>>>>>>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>>>>>>>> return -ENODEV;
>>>>>>>>>
>>>>>>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
>>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>>>>>>>> - if (ret < 0)
>>>>>>>>> - return ret;
>>>>>>>>> -
>>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>>>>>>>> - if (ret < 0)
>>>>>>>>> - return ret;
>>>>>>>>> + if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
>>>>>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>>>>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
>>>>>>>>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
>>>>>>>>> QUIRK_USE_KBD_BACKLIGHT },
>>>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
>>>>>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>>>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
>>>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
>>>>>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>>>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
>>>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
>>>>>>>>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
^ permalink raw reply
* [syzbot] [net?] [input?] [usb?] memory leak in ldisc_receive
From: syzbot @ 2026-01-17 17:34 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, linux-input, linux-kernel,
linux-usb, netdev, pabeni, syzkaller-bugs
Hello,
syzbot found the following issue on:
HEAD commit: b71e635feefc Merge tag 'cgroup-for-6.19-rc5-fixes' of git:..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1638a59a580000
kernel config: https://syzkaller.appspot.com/x/.config?x=87bc41cae23d2144
dashboard link: https://syzkaller.appspot.com/bug?extid=f9d847b2b84164fa69f3
compiler: gcc (Debian 12.2.0-14+deb12u1) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=162985fc580000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1035f922580000
Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/821cfcb36509/disk-b71e635f.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/80f2b6f81538/vmlinux-b71e635f.xz
kernel image: https://storage.googleapis.com/syzbot-assets/c5a9299bca98/bzImage-b71e635f.xz
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+f9d847b2b84164fa69f3@syzkaller.appspotmail.com
BUG: memory leak
unreferenced object 0xffff888109d88b00 (size 240):
comm "syz.0.17", pid 6088, jiffies 4294942822
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 20 8e 13 81 88 ff ff 00 00 00 00 00 00 00 00 . ..............
backtrace (crc cd2b8445):
kmemleak_alloc_recursive include/linux/kmemleak.h:44 [inline]
slab_post_alloc_hook mm/slub.c:4958 [inline]
slab_alloc_node mm/slub.c:5263 [inline]
kmem_cache_alloc_node_noprof+0x384/0x5a0 mm/slub.c:5315
__alloc_skb+0xe8/0x2b0 net/core/skbuff.c:679
__netdev_alloc_skb+0x72/0x230 net/core/skbuff.c:754
netdev_alloc_skb include/linux/skbuff.h:3484 [inline]
ldisc_receive+0x7d/0x210 drivers/net/caif/caif_serial.c:176
tiocsti drivers/tty/tty_io.c:2290 [inline]
tty_ioctl+0x329/0xd40 drivers/tty/tty_io.c:2706
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:597 [inline]
__se_sys_ioctl fs/ioctl.c:583 [inline]
__x64_sys_ioctl+0xf4/0x140 fs/ioctl.c:583
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0xa4/0xf80 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
BUG: memory leak
unreferenced object 0xffff88810a368500 (size 240):
comm "syz.0.18", pid 6193, jiffies 4294942941
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 20 94 13 81 88 ff ff 00 00 00 00 00 00 00 00 . ..............
backtrace (crc 5d1300cb):
kmemleak_alloc_recursive include/linux/kmemleak.h:44 [inline]
slab_post_alloc_hook mm/slub.c:4958 [inline]
slab_alloc_node mm/slub.c:5263 [inline]
kmem_cache_alloc_node_noprof+0x384/0x5a0 mm/slub.c:5315
__alloc_skb+0xe8/0x2b0 net/core/skbuff.c:679
__netdev_alloc_skb+0x72/0x230 net/core/skbuff.c:754
netdev_alloc_skb include/linux/skbuff.h:3484 [inline]
ldisc_receive+0x7d/0x210 drivers/net/caif/caif_serial.c:176
tiocsti drivers/tty/tty_io.c:2290 [inline]
tty_ioctl+0x329/0xd40 drivers/tty/tty_io.c:2706
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:597 [inline]
__se_sys_ioctl fs/ioctl.c:583 [inline]
__x64_sys_ioctl+0xf4/0x140 fs/ioctl.c:583
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0xa4/0xf80 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
BUG: memory leak
unreferenced object 0xffff88810a9e2c00 (size 704):
comm "syz.0.18", pid 6193, jiffies 4294942941
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace (crc b6200c):
kmemleak_alloc_recursive include/linux/kmemleak.h:44 [inline]
slab_post_alloc_hook mm/slub.c:4958 [inline]
slab_alloc_node mm/slub.c:5263 [inline]
kmem_cache_alloc_node_noprof+0x384/0x5a0 mm/slub.c:5315
kmalloc_reserve+0xe6/0x180 net/core/skbuff.c:586
__alloc_skb+0x111/0x2b0 net/core/skbuff.c:690
__netdev_alloc_skb+0x72/0x230 net/core/skbuff.c:754
netdev_alloc_skb include/linux/skbuff.h:3484 [inline]
ldisc_receive+0x7d/0x210 drivers/net/caif/caif_serial.c:176
tiocsti drivers/tty/tty_io.c:2290 [inline]
tty_ioctl+0x329/0xd40 drivers/tty/tty_io.c:2706
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:597 [inline]
__se_sys_ioctl fs/ioctl.c:583 [inline]
__x64_sys_ioctl+0xf4/0x140 fs/ioctl.c:583
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0xa4/0xf80 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
BUG: memory leak
unreferenced object 0xffff888109ddda00 (size 240):
comm "syz.0.19", pid 6262, jiffies 4294943059
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 50 8e 13 81 88 ff ff 00 00 00 00 00 00 00 00 .P..............
backtrace (crc 3f6ba7c1):
kmemleak_alloc_recursive include/linux/kmemleak.h:44 [inline]
slab_post_alloc_hook mm/slub.c:4958 [inline]
slab_alloc_node mm/slub.c:5263 [inline]
kmem_cache_alloc_node_noprof+0x384/0x5a0 mm/slub.c:5315
__alloc_skb+0xe8/0x2b0 net/core/skbuff.c:679
__netdev_alloc_skb+0x72/0x230 net/core/skbuff.c:754
netdev_alloc_skb include/linux/skbuff.h:3484 [inline]
ldisc_receive+0x7d/0x210 drivers/net/caif/caif_serial.c:176
tiocsti drivers/tty/tty_io.c:2290 [inline]
tty_ioctl+0x329/0xd40 drivers/tty/tty_io.c:2706
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:597 [inline]
__se_sys_ioctl fs/ioctl.c:583 [inline]
__x64_sys_ioctl+0xf4/0x140 fs/ioctl.c:583
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0xa4/0xf80 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
connection error: failed to recv *flatrpc.ExecutorMessageRawT: EOF
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title
If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.
If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)
If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report
If you want to undo deduplication, reply with:
#syz undup
^ permalink raw reply
* Re: [PATCH v4 1/3] dt-bindings: touchscreen: trivial-touch: Drop 'interrupts' requirement for old Ilitek
From: Krzysztof Kozlowski @ 2026-01-17 18:34 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-input, Frank Li, Conor Dooley, Dmitry Torokhov, Job Noorman,
Krzysztof Kozlowski, Rob Herring, devicetree, linux-kernel,
linux-renesas-soc
In-Reply-To: <38a146cf-8eee-4fbb-8783-231108a01b54@mailbox.org>
On 17/01/2026 16:33, Marek Vasut wrote:
> On 1/17/26 12:22 PM, Krzysztof Kozlowski wrote:
>> On Sat, Jan 17, 2026 at 01:12:02AM +0100, Marek Vasut wrote:
>>> The old Ilitek touch controllers V3 and V6 can operate without
>>> interrupt line, in polling mode. Drop the 'interrupts' property
>>> requirement for those four controllers. To avoid overloading the
>>> trivial-touch, fork the old Ilitek V3/V6 touch controller binding
>>> into separate document.
>>
>> One if: block is fine, so IMO, this should stay in original binding
>> especially that more devices like some azoteq or semtech might have same
>> rule of not requiring interrupt line. Anyway, no big deal.
> I am not sure about the other non-ilitek devices, but the fruitboards do
> use at least goodix and etm/edt touch controllers without interrupt line
> too, those I have on my desk (those two have separate, more extensive,
> binding document). I also suspect we will see more of those touch
> controllers with optional interrupt line, so if we do, I think we can
> re-combine the binding documents again ?
Yes.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v11 02/11] HID: asus: initialize additional endpoints only for legacy devices
From: Antheas Kapenekakis @ 2026-01-17 19:17 UTC (permalink / raw)
To: Denis Benato
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
In-Reply-To: <e4349b27-86e1-41d6-864f-5ad6d353dc46@linux.dev>
On Sat, 17 Jan 2026 at 18:05, Denis Benato <denis.benato@linux.dev> wrote:
>
>
> On 1/17/26 17:16, Antheas Kapenekakis wrote:
> > On Sat, 17 Jan 2026 at 17:12, Denis Benato <denis.benato@linux.dev> wrote:
> >>
> >> On 1/17/26 17:10, Antheas Kapenekakis wrote:
> >>> On Sat, 17 Jan 2026 at 16:13, Denis Benato <denis.benato@linux.dev> wrote:
> >>>> On 1/17/26 16:07, Antheas Kapenekakis wrote:
> >>>>> On Sat, 17 Jan 2026 at 14:51, Denis Benato <denis.benato@linux.dev> wrote:
> >>>>>> On 1/17/26 00:10, Antheas Kapenekakis wrote:
> >>>>>>> On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@linux.dev> wrote:
> >>>>>>>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
> >>>>>>>>
> >>>>>>>>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
> >>>>>>>>> However, ID1 initializations are only required for RGB control and are
> >>>>>>>>> only supported for RGB capable devices. ID2 initializations are only
> >>>>>>>>> required for initializing the Anime display endpoint which is only
> >>>>>>>>> supported on devices with an Anime display. Both of these
> >>>>>>>>> initializations are out of scope for this driver (this is a brightness
> >>>>>>>>> control and keyboard shortcut driver) and they should not be performed
> >>>>>>>>> for devices that do not support them in any case.
> >>>>>>>>>
> >>>>>>>>> At the same time, there are older NKEY devices that have only been
> >>>>>>>>> tested with these initializations in the kernel and it is not possible
> >>>>>>>>> to recheck them. There is a possibility that especially with the ID1
> >>>>>>>>> initialization, certain laptop models might have their shortcuts stop
> >>>>>>>>> working (currently unproven).
> >>>>>>>>>
> >>>>>>>>> For an abundance of caution, only initialize ID1/ID2 for those older
> >>>>>>>>> NKEY devices by introducing a quirk for them and replacing the NKEY
> >>>>>>>>> quirk in the block that performs the inits with that.
> >>>>>>>>>
> >>>>>>>>> In addition, as these initializations might not be supported by the
> >>>>>>>>> affected devices, change the function to not bail if they fail.
> >>>>>>>>>
> >>>>>>>>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
> >>>>>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>>>>>>>> ---
> >>>>>>>>> drivers/hid/hid-asus.c | 16 ++++++----------
> >>>>>>>>> 1 file changed, 6 insertions(+), 10 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> >>>>>>>>> index 323e6302bac5..dc7af12cf31a 100644
> >>>>>>>>> --- a/drivers/hid/hid-asus.c
> >>>>>>>>> +++ b/drivers/hid/hid-asus.c
> >>>>>>>>> @@ -90,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> >>>>>>>>> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
> >>>>>>>>> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
> >>>>>>>>> #define QUIRK_ROG_ALLY_XPAD BIT(13)
> >>>>>>>>> +#define QUIRK_ROG_NKEY_LEGACY BIT(14)
> >>>>>>>> These past days I have taken a look at new 2025 models and they do make use of ID2,
> >>>>>>>> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
> >>>>>>> Hi Denis,
> >>>>>>> it is not the responsibility of this driver. ID2 is used by Anime
> >>>>>>> models. It is a concession to make sure that we do not cause a
> >>>>>>> regression that will cause warnings for a lot of users.
> >>>>>> Who decided it is a concession?
> >>>>> I would rather remove the extra calls unless they are shown to be
> >>>>> needed, which they might be for these PIDs.
> >>>> They are needed on older laptop and to not regress userspace.
> >>>>
> >>>> You just named _LEGACY an usb pid that is not legacy.
> >>>>> The quirk is named legacy because we can't retest these devices. If we
> >>>>> can, then we could remove the quirk and the inits if not needed.
> >>>> We can't retest every device, and that pid is used in pre-2021 models,
> >>>> and these are the unknown, I am criticizing the name of the quirk here,
> >>>> not what it does.
> >>> If you can test whether your device needs them that would be great.
> >> That is pointless.
> >>>> I am also questioning if the quirk is even needed since sending
> >>>> those commands to (at least) recent hardware that doesn't use
> >>>> those endpoints carries no downsides, while removing them
> >>>> surely does.
> >>> We have not found a device yet that needs them. I do not want to keep
> >>> sending unneeded commands. It could cause obscure bugs or interfere
> >>> with userspace software such as the one you maintain. So at least for
> >>> new hardware that is possible to test we should remove them.
> >> There is new hardware that needs them, as I said, including 2025 models.
> > I was not aware of that. As far as I know they are not needed. Do you
> > have a bug report with a specific laptop model I can look at?
> There is current effort to integrate commands that requires those
> initializations on 2025 laptop, why would I strip out a command
> that I already know is required anyway?
Hi,
yes ID1 is required for RGB, I have a draft patch for it that would
lazily do it if RGB is supported.
I recall now a previous discussion about it being required for some
laptop shortcuts but we never found a laptop that needs it so I forgot
> No, this is not the way to go to knowingly and willingly cause
> troubles (both known and unknown) to others just because
> you think it's better this way.
>
> Change the name of _LEGACY to something else, have this accepted
> and then if I see it's appropriate to remove the if and send those
> regardless I will.
Sure, up to you if you want to change the name. What would you like it
be? I would like this series to merge
Antheas
> > Antheas
> >
> >>> Antheas
> >>>
> >>>>> Antheas
> >>>>>
> >>>>>> Anyway I will move relevant code tied to these two to this driver,
> >>>>>> so it doesn't make sense to remove them anyway.
> >>>>>>>> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
> >>>>>>>> No consequences.
> >>>>>>> In your laptop. In the other user's laptop, the get feature report fails
> >>>>>> for the response to be a failure (as it is supposed to be in mine and other models)
> >>>>>> and to cause problems are two different things. Here I am saying that the hardware
> >>>>>> correctly reports "unsupported" and nothing bad happens (if you ignore the return value).
> >>>>>>>> Regardless the name is wrong: mine is a 2023 rog strix with
> >>>>>>>> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
> >>>>>>>> and surely isn't legacy.
> >>>>>>> Sure, can you try removing the if block?
> >>>>>> I have asked to distribute a kernel that init ID1 and ID2 regardless
> >>>>>> of that quirk. We will soon know if it causes problems or not.
> >>>>>>> If it works in your laptop, that is one less reason to keep it for 19b6
> >>>>>> If it works in my laptop one more reason not to exclude code that
> >>>>>> works and haven't caused any problem ever.
> >>>>>>> Antheas
> >>>>>>>
> >>>>>>>>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
> >>>>>>>>> QUIRK_NO_INIT_REPORTS | \
> >>>>>>>>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> >>>>>>>>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>>>>>>>> return -ENODEV;
> >>>>>>>>>
> >>>>>>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> >>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>>>>>>>> - if (ret < 0)
> >>>>>>>>> - return ret;
> >>>>>>>>> -
> >>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>>>>>>>> - if (ret < 0)
> >>>>>>>>> - return ret;
> >>>>>>>>> + if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
> >>>>>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>>>>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>>>>>>>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
> >>>>>>>>> QUIRK_USE_KBD_BACKLIGHT },
> >>>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
> >>>>>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
> >>>>>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
> >>>>>>>>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>>>> On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@linux.dev> wrote:
> >>>>>>>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
> >>>>>>>>
> >>>>>>>>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
> >>>>>>>>> However, ID1 initializations are only required for RGB control and are
> >>>>>>>>> only supported for RGB capable devices. ID2 initializations are only
> >>>>>>>>> required for initializing the Anime display endpoint which is only
> >>>>>>>>> supported on devices with an Anime display. Both of these
> >>>>>>>>> initializations are out of scope for this driver (this is a brightness
> >>>>>>>>> control and keyboard shortcut driver) and they should not be performed
> >>>>>>>>> for devices that do not support them in any case.
> >>>>>>>>>
> >>>>>>>>> At the same time, there are older NKEY devices that have only been
> >>>>>>>>> tested with these initializations in the kernel and it is not possible
> >>>>>>>>> to recheck them. There is a possibility that especially with the ID1
> >>>>>>>>> initialization, certain laptop models might have their shortcuts stop
> >>>>>>>>> working (currently unproven).
> >>>>>>>>>
> >>>>>>>>> For an abundance of caution, only initialize ID1/ID2 for those older
> >>>>>>>>> NKEY devices by introducing a quirk for them and replacing the NKEY
> >>>>>>>>> quirk in the block that performs the inits with that.
> >>>>>>>>>
> >>>>>>>>> In addition, as these initializations might not be supported by the
> >>>>>>>>> affected devices, change the function to not bail if they fail.
> >>>>>>>>>
> >>>>>>>>> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
> >>>>>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>>>>>>>> ---
> >>>>>>>>> drivers/hid/hid-asus.c | 16 ++++++----------
> >>>>>>>>> 1 file changed, 6 insertions(+), 10 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> >>>>>>>>> index 323e6302bac5..dc7af12cf31a 100644
> >>>>>>>>> --- a/drivers/hid/hid-asus.c
> >>>>>>>>> +++ b/drivers/hid/hid-asus.c
> >>>>>>>>> @@ -90,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> >>>>>>>>> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
> >>>>>>>>> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
> >>>>>>>>> #define QUIRK_ROG_ALLY_XPAD BIT(13)
> >>>>>>>>> +#define QUIRK_ROG_NKEY_LEGACY BIT(14)
> >>>>>>>> These past days I have taken a look at new 2025 models and they do make use of ID2,
> >>>>>>>> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
> >>>>>>>>
> >>>>>>>> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
> >>>>>>>> No consequences.
> >>>>>>>>
> >>>>>>>> Regardless the name is wrong: mine is a 2023 rog strix with
> >>>>>>>> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
> >>>>>>>> and surely isn't legacy.
> >>>>>>>>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
> >>>>>>>>> QUIRK_NO_INIT_REPORTS | \
> >>>>>>>>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> >>>>>>>>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>>>>>>>> return -ENODEV;
> >>>>>>>>>
> >>>>>>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> >>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>>>>>>>> - if (ret < 0)
> >>>>>>>>> - return ret;
> >>>>>>>>> -
> >>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>>>>>>>> - if (ret < 0)
> >>>>>>>>> - return ret;
> >>>>>>>>> + if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
> >>>>>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>>>>>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>>>>>>>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
> >>>>>>>>> QUIRK_USE_KBD_BACKLIGHT },
> >>>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
> >>>>>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
> >>>>>>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>>>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>>>>>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>>>>>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
> >>>>>>>>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>
^ permalink raw reply
* Re: bcm5974 trackpad broken after USB reset
From: Henrik Rydberg @ 2026-01-18 3:33 UTC (permalink / raw)
To: Liam Mitchell; +Cc: Henrik Rydberg, linux-input
In-Reply-To: <CAOQ1CL6G6eDcX+Qth3D531h72wW3RmvecCWjr5nAT-UdDWg40w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 781 bytes --]
> Thanks for the pointer. The wellspring mode switch is definitely
> attempted. The bcm5974 driver does the mode switching in input_dev
> open/close hooks. These are called multiple times after reset. See the
> following logs produced after `sudo usbreset 05ac:0259`.
Indeed, the correct path appears to be taken, so that was not it. Maybe
the device internals do take some extra time to complete. Presumably
there is a status field in the control message data; printing it out
might reveal something.
Here is a completely untested patch which tries to verify that the mode
is properly set, and dumps the control data in the process. With a bit
of luck, this will just work because of the added delay, but if not, we
might be able to figure out the status change.
Henrik
[-- Attachment #2: 0001-untested-bcm5974-verify-mode-switch.patch --]
[-- Type: text/x-patch, Size: 1740 bytes --]
From a0bbd6b5a9a4f05c85a252ace8129cda0689fb45 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <rydberg@bitmath.se>
Date: Sun, 18 Jan 2026 04:16:07 +0100
Subject: [PATCH] [untested] bcm5974: verify mode switch
---
drivers/input/mouse/bcm5974.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
index dfdfb59cc8b5..cabe04d8e32d 100644
--- a/drivers/input/mouse/bcm5974.c
+++ b/drivers/input/mouse/bcm5974.c
@@ -648,6 +648,7 @@ static int bcm5974_wellspring_mode(struct bcm5974 *dev, bool on)
{
const struct bcm5974_config *c = &dev->cfg;
int retval = 0, size;
+ int retries = 3;
char *data;
/* Type 3 does not require a mode switch */
@@ -661,6 +662,7 @@ static int bcm5974_wellspring_mode(struct bcm5974 *dev, bool on)
goto out;
}
+ again:
/* read configuration */
size = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
BCM5974_WELLSPRING_MODE_READ_REQUEST_ID,
@@ -673,6 +675,18 @@ static int bcm5974_wellspring_mode(struct bcm5974 *dev, bool on)
goto out;
}
+ if (data[c->um_switch_idx] == (on ? c->um_switch_on : c->um_switch_off))
+ goto success;
+
+ if (!retries--) {
+ dprintk(2, "bcm5974: failed to switch to %s mode.\n",
+ on ? "wellspring" : "normal");
+ retval = -ENOENT;
+ goto out;
+ }
+
+ print_hex_dump_bytes("bcm5974: control data: ", DUMP_PREFIX_NONE, data, size);
+
/* apply the mode switch */
data[c->um_switch_idx] = on ? c->um_switch_on : c->um_switch_off;
@@ -688,6 +702,9 @@ static int bcm5974_wellspring_mode(struct bcm5974 *dev, bool on)
goto out;
}
+ goto again;
+
+ success:
dprintk(2, "bcm5974: switched to %s mode.\n",
on ? "wellspring" : "normal");
--
2.52.0
^ permalink raw reply related
* Re: bcm5974 trackpad broken after USB reset
From: Liam Mitchell @ 2026-01-18 9:59 UTC (permalink / raw)
To: Henrik Rydberg; +Cc: Henrik Rydberg, linux-input
In-Reply-To: <6880b5ba-ee76-4a9e-a116-16843a737b34@bitmath.se>
On Sun, 18 Jan 2026 at 04:32, Henrik Rydberg <rydberg@bitmath.se> wrote:
> Here is a completely untested patch which tries to verify that the mode
> is properly set, and dumps the control data in the process. With a bit
> of luck, this will just work because of the added delay, but if not, we
> might be able to figure out the status change.
No luck there. The control data is always exactly as written. I've
also tried logging the control data after longer delays but didn't see
anything different.
[ 391.262674] usbcore: bcm5974 4-1.8.2:1.2: forced unbind
[ 391.264601] bcm5974 4-1.8.2:1.2: trackpad urb shutting down: -2
[ 391.265182] bcm5974: control data: 01 05 00 00 00 00 00 00
........
[ 391.265992] bcm5974: switched to normal mode.
[ 391.290770] usbcore: usb 4-1.8-port2: not reset yet, waiting 10ms
[ 391.352802] usb 4-1.8.2: reset full-speed USB device number 5 using ehci-pci
[ 391.364919] usbcore: usb 4-1.8-port2: not reset yet, waiting 10ms
[ 391.447757] usbcore: usbhid 4-1.8.2:1.2: usb_probe_interface
[ 391.447771] usbcore: usbhid 4-1.8.2:1.2: usb_probe_interface - got id
[ 391.447779] usbhid: drivers/hid/usbhid/hid-core.c: HID probe called
for ifnum 2
[ 391.447820] hid: drivers/hid/hid-quirks.c: Found squirk 0x84000 for
HID device 0x05ac:0x0259
[ 391.449301] usbcore: bcm5974 4-1.8.2:1.2: usb_probe_interface
[ 391.449329] usbcore: bcm5974 4-1.8.2:1.2: usb_probe_interface - got id
[ 391.449692] input: bcm5974 as
/devices/pci0000:00/0000:00:1d.0/usb4/4-1/4-1.8/4-1.8.2/4-1.8.2:1.2/input/input16
[ 391.482694] bcm5974: control data: 08 05 00 00 00 00 00 00
........
[ 391.483487] bcm5974: switched to wellspring mode.
[ 391.493648] bcm5974 4-1.8.2:1.2: trackpad urb shutting down: -2
[ 391.494169] bcm5974: control data: 01 05 00 00 00 00 00 00
........
[ 391.495142] bcm5974: switched to normal mode.
[ 391.508166] bcm5974: control data: 08 05 00 00 00 00 00 00
........
[ 391.509157] bcm5974: switched to wellspring mode.
[ 391.514674] bcm5974 4-1.8.2:1.2: trackpad urb shutting down: -2
[ 391.515205] bcm5974: control data: 01 05 00 00 00 00 00 00
........
[ 391.516241] bcm5974: switched to normal mode.
[ 391.536717] bcm5974: control data: 08 05 00 00 00 00 00 00
........
[ 391.537626] bcm5974: switched to wellspring mode.
[ 391.545669] bcm5974 4-1.8.2:1.2: trackpad urb shutting down: -2
[ 391.546175] bcm5974: control data: 01 05 00 00 00 00 00 00
........
[ 391.547089] bcm5974: switched to normal mode.
[ 391.557783] bcm5974: control data: 08 05 00 00 00 00 00 00
........
[ 391.558488] bcm5974: switched to wellspring mode.
[ 391.568627] bcm5974 4-1.8.2:1.2: trackpad urb shutting down: -2
[ 391.569033] bcm5974: control data: 01 05 00 00 00 00 00 00
........
[ 391.569722] bcm5974: switched to normal mode.
[ 391.584744] bcm5974: control data: 08 05 00 00 00 00 00 00
........
[ 391.585475] bcm5974: switched to wellspring mode.
[ 392.181559] bcm5974: bad trackpad package, length: 8
Liam
^ permalink raw reply
* Re: [PATCH v4 3/3] Input: ili210x - add support for polling mode
From: Frank Li @ 2026-01-18 17:19 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-input, Conor Dooley, Dmitry Torokhov, Job Noorman,
Krzysztof Kozlowski, Rob Herring, devicetree, linux-kernel,
linux-renesas-soc
In-Reply-To: <20260117001215.59272-3-marek.vasut+renesas@mailbox.org>
On Sat, Jan 17, 2026 at 01:12:04AM +0100, Marek Vasut wrote:
> There are designs incorporating Ilitek ILI2xxx touch controller that
> do not connect interrupt pin, for example Waveshare 13.3" DSI display.
> To support such systems use polling mode for the input device when I2C
> client does not have interrupt assigned to it.
>
> Factor out ili210x_firmware_update_noirq() to allow conditional scoped
> guard around this code. The scoped guard has to be applied only in case
> the IRQ line is connected, and not applied otherwise.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Frank Li <Frank.Li@nxp.com>
> Cc: Job Noorman <job@noorman.info>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-input@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-renesas-soc@vger.kernel.org
> ---
> V2: Test client->irq > 0 for IRQ presence
> V3: - Rebase on dev_err_probe() conversion
> - Fix if (client->irq > 0) in ili210x_firmware_update_store()
> V4: No change
> ---
> drivers/input/touchscreen/ili210x.c | 76 +++++++++++++++++++++--------
> 1 file changed, 56 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> index 264eee3e61d0a..22917a5825778 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -327,9 +327,8 @@ static bool ili210x_report_events(struct ili210x *priv, u8 *touchdata)
> return contact;
> }
>
> -static irqreturn_t ili210x_irq(int irq, void *irq_data)
> +static void ili210x_process_events(struct ili210x *priv)
> {
> - struct ili210x *priv = irq_data;
> struct i2c_client *client = priv->client;
> const struct ili2xxx_chip *chip = priv->chip;
> u8 touchdata[ILI210X_DATA_SIZE] = { 0 };
> @@ -356,8 +355,22 @@ static irqreturn_t ili210x_irq(int irq, void *irq_data)
> usleep_range(time_delta, time_delta + 1000);
> }
> } while (!priv->stop && keep_polling);
> +}
> +
> +static irqreturn_t ili210x_irq(int irq, void *irq_data)
> +{
> + struct ili210x *priv = irq_data;
> +
> + ili210x_process_events(priv);
>
> return IRQ_HANDLED;
> +};
> +
> +static void ili210x_work_i2c_poll(struct input_dev *input)
> +{
> + struct ili210x *priv = input_get_drvdata(input);
> +
> + ili210x_process_events(priv);
> }
>
> static int ili251x_firmware_update_resolution(struct device *dev)
> @@ -829,12 +842,32 @@ static int ili210x_do_firmware_update(struct ili210x *priv,
> return 0;
> }
>
> +static ssize_t ili210x_firmware_update_noirq(struct device *dev,
> + const u8 *fwbuf, u16 ac_end, u16 df_end)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct ili210x *priv = i2c_get_clientdata(client);
> + const char *fwname = ILI251X_FW_FILENAME;
> + int error;
> +
> + dev_dbg(dev, "Firmware update started, firmware=%s\n", fwname);
> +
> + ili210x_hardware_reset(priv->reset_gpio);
> +
> + error = ili210x_do_firmware_update(priv, fwbuf, ac_end, df_end);
> +
> + ili210x_hardware_reset(priv->reset_gpio);
> +
> + dev_dbg(dev, "Firmware update ended, error=%i\n", error);
> +
> + return error;
> +}
> +
> static ssize_t ili210x_firmware_update_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> struct i2c_client *client = to_i2c_client(dev);
> - struct ili210x *priv = i2c_get_clientdata(client);
> const char *fwname = ILI251X_FW_FILENAME;
> u16 ac_end, df_end;
> int error;
> @@ -860,16 +893,12 @@ static ssize_t ili210x_firmware_update_store(struct device *dev,
> * the touch controller to disable the IRQs during update, so we have
> * to do it this way here.
> */
> - scoped_guard(disable_irq, &client->irq) {
> - dev_dbg(dev, "Firmware update started, firmware=%s\n", fwname);
> -
> - ili210x_hardware_reset(priv->reset_gpio);
> -
> - error = ili210x_do_firmware_update(priv, fwbuf, ac_end, df_end);
> -
> - ili210x_hardware_reset(priv->reset_gpio);
> -
> - dev_dbg(dev, "Firmware update ended, error=%i\n", error);
> + if (client->irq > 0) {
> + scoped_guard(disable_irq, &client->irq) {
> + error = ili210x_firmware_update_noirq(dev, fwbuf, ac_end, df_end);
> + }
> + } else {
> + error = ili210x_firmware_update_noirq(dev, fwbuf, ac_end, df_end);
> }
>
> return error ?: count;
> @@ -945,9 +974,6 @@ static int ili210x_i2c_probe(struct i2c_client *client)
> if (!chip)
> return dev_err_probe(&client->dev, -ENODEV, "unknown device model\n");
>
> - if (client->irq <= 0)
> - return dev_err_probe(dev, -EINVAL, "No IRQ!\n");
> -
> reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> if (IS_ERR(reset_gpio))
> return PTR_ERR(reset_gpio);
> @@ -997,10 +1023,20 @@ static int ili210x_i2c_probe(struct i2c_client *client)
> if (error)
> return dev_err_probe(dev, error, "Unable to set up slots\n");
>
> - error = devm_request_threaded_irq(dev, client->irq, NULL, ili210x_irq,
> - IRQF_ONESHOT, client->name, priv);
> - if (error)
> - return dev_err_probe(dev, error, "Unable to request touchscreen IRQ\n");
> + input_set_drvdata(input, priv);
> +
> + if (client->irq > 0) {
> + error = devm_request_threaded_irq(dev, client->irq, NULL, ili210x_irq,
> + IRQF_ONESHOT, client->name, priv);
> + if (error)
> + return dev_err_probe(dev, error, "Unable to request touchscreen IRQ\n");
> + } else {
> + error = input_setup_polling(input, ili210x_work_i2c_poll);
> + if (error)
> + return dev_err_probe(dev, error, "Could not set up polling mode\n");
> +
> + input_set_poll_interval(input, ILI2XXX_POLL_PERIOD);
> + }
>
> error = devm_add_action_or_reset(dev, ili210x_stop, priv);
> if (error)
> --
> 2.51.0
>
^ permalink raw reply
* [PATCH] HID: corsair: fix typos in comment
From: Owen Giles @ 2026-01-18 18:58 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel, Owen Giles
Fix typos in report descriptor comment: "obviousy" -> "obviously",
"Magimum" -> "Maximum", "poper" -> "proper", "due Logical" -> "due to
Logical".
Signed-off-by: Owen Giles <owen.a.giles@gmail.com>
---
drivers/hid/hid-corsair.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
index 62b99f5c3cf8..e11184cec551 100644
--- a/drivers/hid/hid-corsair.c
+++ b/drivers/hid/hid-corsair.c
@@ -681,8 +681,8 @@ static int corsair_input_mapping(struct hid_device *dev,
* The report descriptor of some of the Corsair gaming mice is
* non parseable as they define two consecutive Logical Minimum for
* the Usage Page (Consumer) in rdescs bytes 75 and 77 being 77 0x16
- * that should be obviousy 0x26 for Logical Magimum of 16 bits. This
- * prevents poper parsing of the report descriptor due Logical
+ * that should be obviously 0x26 for Logical Maximum of 16 bits. This
+ * prevents proper parsing of the report descriptor due to Logical
* Minimum being larger than Logical Maximum.
*
* This driver fixes the report descriptor for:
--
2.52.0
^ permalink raw reply related
* [PATCH v3 2/3] drivers: input: touchscreen: edt-ft5x06: Add FocalTech FT3518
From: Yedaya Katsman via B4 Relay @ 2026-01-18 20:29 UTC (permalink / raw)
To: Kamil Gołda, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
Yedaya Katsman, Dmitry Baryshkov
In-Reply-To: <20260118-touchscreen-patches-v3-0-1c6a729c5eb4@gmail.com>
From: Yedaya Katsman <yedaya.ka@gmail.com>
The driver also works with FT3518, which supports up to 10 touch points.
Add compatible data for it.
Co-developed-by: Kamil Gołda <kamil.golda@protonmail.com>
Signed-off-by: Kamil Gołda <kamil.golda@protonmail.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Yedaya Katsman <yedaya.ka@gmail.com>
---
drivers/input/touchscreen/edt-ft5x06.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index bf498bd4dea9651ac939fe137b1c0f05e8557962..d0ab644be0069b5ab29ed037fa090a4279870193 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1475,6 +1475,10 @@ static const struct edt_i2c_chip_data edt_ft5x06_data = {
.max_support_points = 5,
};
+static const struct edt_i2c_chip_data edt_ft3518_data = {
+ .max_support_points = 10,
+};
+
static const struct edt_i2c_chip_data edt_ft5452_data = {
.max_support_points = 5,
};
@@ -1503,6 +1507,7 @@ static const struct i2c_device_id edt_ft5x06_ts_id[] = {
{ .name = "edt-ft5x06", .driver_data = (long)&edt_ft5x06_data },
{ .name = "edt-ft5506", .driver_data = (long)&edt_ft5506_data },
{ .name = "ev-ft5726", .driver_data = (long)&edt_ft5506_data },
+ { .name = "ft3518", .driver_data = (long)&edt_ft3518_data },
{ .name = "ft5452", .driver_data = (long)&edt_ft5452_data },
/* Note no edt- prefix for compatibility with the ft6236.c driver */
{ .name = "ft6236", .driver_data = (long)&edt_ft6236_data },
@@ -1519,6 +1524,7 @@ static const struct of_device_id edt_ft5x06_of_match[] = {
{ .compatible = "edt,edt-ft5406", .data = &edt_ft5x06_data },
{ .compatible = "edt,edt-ft5506", .data = &edt_ft5506_data },
{ .compatible = "evervision,ev-ft5726", .data = &edt_ft5506_data },
+ { .compatible = "focaltech,ft3518", .data = &edt_ft3518_data },
{ .compatible = "focaltech,ft5426", .data = &edt_ft5506_data },
{ .compatible = "focaltech,ft5452", .data = &edt_ft5452_data },
/* Note focaltech vendor prefix for compatibility with ft6236.c */
--
2.52.0
^ permalink raw reply related
* [PATCH v3 3/3] arm64: dts: qcom: sm6125-xiaomi-laurel-sprout: Add Focaltech FT3518 touchscreen
From: Yedaya Katsman via B4 Relay @ 2026-01-18 20:29 UTC (permalink / raw)
To: Kamil Gołda, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
Yedaya Katsman
In-Reply-To: <20260118-touchscreen-patches-v3-0-1c6a729c5eb4@gmail.com>
From: Yedaya Katsman <yedaya.ka@gmail.com>
Add device tree node for the Focaltech FT3518 touchscreen on
Xiaomi Mi A3 (laurel-sprout).
Add pmx_ts_* gpio configurations and reference them in the touchscreen
node.
Note that gpio pin 83 for the regulator isn't documented downstream
except in the touchscreen node so it's not defined in the tlmm.
Enable qupv3_id_0 and i2c2 bus that the touchscreen is on.
Downstream references:
Link: https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/laurel-r-oss/arch/arm64/boot/dts/qcom/trinket-pinctrl.dtsi
Link: https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/laurel-r-oss/arch/arm64/boot/dts/qcom/laurel_sprout-qrd.dtsi
Co-developed-by: Kamil Gołda <kamil.golda@protonmail.com>
Signed-off-by: Kamil Gołda <kamil.golda@protonmail.com>
Signed-off-by: Yedaya Katsman <yedaya.ka@gmail.com>
---
.../boot/dts/qcom/sm6125-xiaomi-laurel-sprout.dts | 113 +++++++++++++++++++++
1 file changed, 113 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sm6125-xiaomi-laurel-sprout.dts b/arch/arm64/boot/dts/qcom/sm6125-xiaomi-laurel-sprout.dts
index 994fb0412fcbdf5466f87a325c48b697a37b514b..5e55acacee9585f34eead20661268103f0b7889c 100644
--- a/arch/arm64/boot/dts/qcom/sm6125-xiaomi-laurel-sprout.dts
+++ b/arch/arm64/boot/dts/qcom/sm6125-xiaomi-laurel-sprout.dts
@@ -82,6 +82,18 @@ key-volume-up {
};
};
+ ts_vdd_supply: regulator-ts-vdd {
+ compatible = "regulator-fixed";
+ regulator-name = "ts_vdd_supply";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+
+ gpio = <&tlmm 83 GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+
+ startup-delay-us = <70000>;
+ };
+
thermal-zones {
rf-pa0-thermal {
thermal-sensors = <&pm6125_adc_tm 0>;
@@ -128,6 +140,28 @@ &hsusb_phy1 {
status = "okay";
};
+&i2c2 {
+ status = "okay";
+
+ touchscreen@38 {
+ compatible = "focaltech,ft3518";
+ reg = <0x38>;
+ interrupts-extended = <&tlmm 88 IRQ_TYPE_EDGE_FALLING>;
+
+ vcc-supply = <&ts_vdd_supply>;
+
+ pinctrl-names = "pmx_ts_active","pmx_ts_suspend","pmx_ts_release";
+ pinctrl-0 = <&ts_int_active &ts_reset_active>;
+ pinctrl-1 = <&ts_int_suspend &ts_reset_suspend>;
+ pinctrl-2 = <&ts_release>;
+
+ reset-gpios = <&tlmm 87 GPIO_ACTIVE_LOW>;
+
+ touchscreen-size-x = <720>;
+ touchscreen-size-y = <1560>;
+ };
+};
+
&pm6125_adc {
pinctrl-names = "default";
pinctrl-0 = <&camera_flash_therm &emmc_ufs_therm>;
@@ -220,6 +254,10 @@ &pon_resin {
status = "okay";
};
+&qupv3_id_0 {
+ status = "okay";
+};
+
&rpm_requests {
regulators-0 {
compatible = "qcom,rpm-pm6125-regulators";
@@ -387,6 +425,81 @@ &sdhc_2 {
&tlmm {
gpio-reserved-ranges = <22 2>, <28 6>;
+
+ pmx_ts_reset_active {
+ ts_reset_active: ts_reset_active {
+ mux {
+ pins = "gpio87";
+ function = "gpio";
+ };
+
+ config {
+ pins = "gpio87";
+ drive-strength = <8>;
+ bias-pull-up;
+ };
+ };
+ };
+
+ pmx_ts_reset_suspend {
+ ts_reset_suspend: ts_reset_suspend {
+ mux {
+ pins = "gpio87";
+ function = "gpio";
+ };
+
+ config {
+ pins = "gpio87";
+ drive-strength = <2>;
+ bias-pull-down;
+ };
+ };
+ };
+
+ pmx_ts_int_active {
+ ts_int_active: ts_int_active {
+ mux {
+ pins = "gpio88";
+ function = "gpio";
+ };
+
+ config {
+ pins = "gpio88";
+ drive-strength = <8>;
+ bias-pull-up;
+ };
+ };
+ };
+
+ pmx_ts_int_suspend {
+ ts_int_suspend: ts_int_suspend {
+ mux {
+ pins = "gpio88";
+ function = "gpio";
+ };
+
+ config {
+ pins = "gpio88";
+ drive-strength = <2>;
+ bias-pull-down;
+ };
+ };
+ };
+
+ pmx_ts_release {
+ ts_release: ts_release {
+ mux {
+ pins = "gpio87", "gpio88";
+ function = "gpio";
+ };
+
+ config {
+ pins = "gpio87", "gpio88";
+ drive-strength = <2>;
+ bias-pull-down;
+ };
+ };
+ };
};
&ufs_mem_hc {
--
2.52.0
^ permalink raw reply related
* [PATCH v3 1/3] dt-bindings: input: touchscreen: edt-ft5x06: Add FocalTech FT3518
From: Yedaya Katsman via B4 Relay @ 2026-01-18 20:29 UTC (permalink / raw)
To: Kamil Gołda, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
Yedaya Katsman, Krzysztof Kozlowski
In-Reply-To: <20260118-touchscreen-patches-v3-0-1c6a729c5eb4@gmail.com>
From: Yedaya Katsman <yedaya.ka@gmail.com>
Document FocalTech FT3518 support by adding the compatible.
Co-developed-by: Kamil Gołda <kamil.golda@protonmail.com>
Signed-off-by: Kamil Gołda <kamil.golda@protonmail.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Yedaya Katsman <yedaya.ka@gmail.com>
---
Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
index 7d3edb58f72d84ed19fb87fdd136c97f855aba00..6f90522de8c0afbe2d9d1e04578316350f66ec58 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
@@ -39,6 +39,7 @@ properties:
- edt,edt-ft5406
- edt,edt-ft5506
- evervision,ev-ft5726
+ - focaltech,ft3518
- focaltech,ft5426
- focaltech,ft5452
- focaltech,ft6236
--
2.52.0
^ permalink raw reply related
* [PATCH v3 0/3] Support FT3518 touchscreen in xiaomi-laurel
From: Yedaya Katsman via B4 Relay @ 2026-01-18 20:29 UTC (permalink / raw)
To: Kamil Gołda, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
Yedaya Katsman, Krzysztof Kozlowski, Dmitry Baryshkov
Adds support for the touchscreen in the Xiaomi Mi A3 (xiaomi-laurel)
smartphone, FocalTech FT3518
Original tree was here:
Link: https://gitlab.postmarketos.org/SzczurekYT/linux/-/commits/laurel
Signed-off-by: Yedaya Katsman <yedaya.ka@gmail.com>
---
Changes in v3:
- Rename regulator node and reorder nodes
- Add gpio pin configuration for pmx_ts_* in sm6125, and reference in the
touchscreen configuration as pinctrl-*. Doesn't have configuration for
the gpio 83 pin since it isn't documented downstream.
- Link to v2: https://lore.kernel.org/r/20260114-touchscreen-patches-v2-0-4215f94c8aba@gmail.com
Changes in v2:
- Fixed name and email in signoffs
- Link to v1: https://lore.kernel.org/r/20260113-touchscreen-patches-v1-0-a10957f32dd8@gmail.com
---
Yedaya Katsman (3):
dt-bindings: input: touchscreen: edt-ft5x06: Add FocalTech FT3518
drivers: input: touchscreen: edt-ft5x06: Add FocalTech FT3518
arm64: dts: qcom: sm6125-xiaomi-laurel-sprout: Add Focaltech FT3518 touchscreen
.../bindings/input/touchscreen/edt-ft5x06.yaml | 1 +
.../boot/dts/qcom/sm6125-xiaomi-laurel-sprout.dts | 113 +++++++++++++++++++++
drivers/input/touchscreen/edt-ft5x06.c | 6 ++
3 files changed, 120 insertions(+)
---
base-commit: b71e635feefc852405b14620a7fc58c4c80c0f73
change-id: 20260113-touchscreen-patches-beb2526bd5fb
Best regards,
--
Yedaya Katsman <yedaya.ka@gmail.com>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox