From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-178.mta1.migadu.com (out-178.mta1.migadu.com [95.215.58.178]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D9F15288D6 for ; Sat, 17 Jan 2026 14:48:26 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.215.58.178 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768661309; cv=none; b=d6GagvbkNKmV6+udJKhFggK9Eufk/c3ORkcaFsmedmO/mlEX1xbH9z/rzQsjvfHVdSK4Ctk6DfXAisvhBmbJd+uFLHgVxlhJEbM4qdJTLv6VjvQ3SFwOS/u7McgvnOvRQBhqYmO0ZPUwiFQslBu4zabzuZT9KdBEtqPY7G6hw2E= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768661309; c=relaxed/simple; bh=2GIqozryh0/kiqAEINkJVnEMU2jXVlykOUuS/Hud4wU=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=h6XJrCtV6CO9J93DoGcZIMj/xDL3+ehRWTgDlJhyzpRToMDzkWn4GE6S03S0PUL7HU/fSn3D/nrHNHqNxhQlcXmbTDt8FWwdeV8r8I2jAi2f/mI6LFdeDgRLan/k455IqjmVDz7GuKj++l6x9u4KEI/9tlMUn/QloR7i0EILvgQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=K/8VyHhO; arc=none smtp.client-ip=95.215.58.178 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="K/8VyHhO" Message-ID: <22651aa1-5677-4d76-8850-49e6a348a257@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1768661294; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Su3auMRDRXm79lvBwNS8BIrGoXS1B1TGPYdscG5OEQ8=; b=K/8VyHhOKNZ909ktO1ZPAZSMnHzy5pQnMAYT3A4bBlgDvrC/uYpaf+qm4T7/gqqHpK347n nBNL8e8se21P2P/QXBjQdz1eV6xyhAxa6+7wIjRWsFQBpD/8k5yRkqnXhpJepMmZKYutcD vO21DQtrqHs2lQjre5Lt0cjSC2az9jg= Date: Sat, 17 Jan 2026 15:48:09 +0100 Precedence: bulk X-Mailing-List: linux-input@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: [PATCH v11 07/11] platform/x86: asus-wmi: Add support for multiple kbd led handlers To: Antheas Kapenekakis Cc: platform-driver-x86@vger.kernel.org, linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, Jiri Kosina , Benjamin Tissoires , Corentin Chary , "Luke D . Jones" , Hans de Goede , =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= References: <20260116133150.5606-1-lkml@antheas.dev> <20260116133150.5606-8-lkml@antheas.dev> <3354c446-3e1c-40c7-ac08-43b3ef630d91@linux.dev> <2f7c8e80-274e-4cec-98bf-cd9cc49fb363@linux.dev> Content-Language: en-US, it-IT, en-US-large X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Denis Benato In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT On 1/17/26 15:11, Antheas Kapenekakis wrote: > On Sat, 17 Jan 2026 at 14:56, Denis Benato wrote: >> >> On 1/17/26 14:49, Antheas Kapenekakis wrote: >>> On Sat, 17 Jan 2026 at 14:16, Denis Benato 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 >>>>> Signed-off-by: Antheas Kapenekakis >>>>> --- >>>>> 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 >>>>> #include >>>>> #include >>>>> -#include >>>>> #include >>>>> #include >>>>> #include >>>>> #include >>>>> #include >>>>> #include >>>>> +#include >>>>> #include >>>>> #include >>>>> >>>>> @@ -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 */