Message-ID: <30e7629f-7827-4776-85b1-5b09ad271e3c@ljones.dev>
Date: Sat, 22 Mar 2025 22:21:32 +1300
From: "Luke D. Jones" <luke@...nes.dev>
To: Antheas Kapenekakis <lkml@...heas.dev>
Cc: platform-driver-x86@...r.kernel.org, linux-input@...r.kernel.org,
 linux-kernel@...r.kernel.org, Jiri Kosina <jikos@...nel.org>,
 Benjamin Tissoires <bentiss@...nel.org>,
 Corentin Chary <corentin.chary@...il.com>,
 Hans de Goede <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH 05/11] platform/x86: asus-wmi: Add support for multiple
 kbd RGB handlers

On 22/03/25 22:06, Antheas Kapenekakis wrote:
> On Sat, 22 Mar 2025 at 09:57, Luke D. Jones <luke@...nes.dev> wrote:
>>
>> On 22/03/25 21:06, Antheas Kapenekakis wrote:
>>> On Sat, 22 Mar 2025 at 04:24, Luke D. Jones <luke@...nes.dev> wrote:
>>>>
>>>> On 21/03/25 11:09, 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.
>>>>>
>>>>> Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
>>>>> ---
>>>>>     drivers/platform/x86/asus-wmi.c            | 100 ++++++++++++++++++---
>>>>>     include/linux/platform_data/x86/asus-wmi.h |  16 ++++
>>>>>     2 files changed, 104 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>>>>> index 38ef778e8c19b..21e034be71b2f 100644
>>>>> --- a/drivers/platform/x86/asus-wmi.c
>>>>> +++ b/drivers/platform/x86/asus-wmi.c
>>>>> @@ -254,6 +254,8 @@ struct asus_wmi {
>>>>>         int tpd_led_wk;
>>>>>         struct led_classdev kbd_led;
>>>>>         int kbd_led_wk;
>>>>> +     bool kbd_led_avail;
>>>>> +     bool kbd_led_registered;
>>>>>         struct led_classdev lightbar_led;
>>>>>         int lightbar_led_wk;
>>>>>         struct led_classdev micmute_led;
>>>>> @@ -1487,6 +1489,46 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
>>>>>
>>>>>     /* LEDs ***********************************************************************/
>>>>>
>>>>> +LIST_HEAD(asus_brt_listeners);
>>>>> +DEFINE_MUTEX(asus_brt_lock);
>>>>> +struct asus_wmi *asus_brt_ref;
>>>>
>>>> Could these 3 items contained in a new static struct, it would make it
>>>> easier to see the associations since they're a group.
>>>
>>> My V0 tried something like that, but it was messy. I will think about
>>> it for V4. Let's do that when we decide the name as it will be hairy
>>> to rebase both of those and I want to do it once.
>>>
>>>>> +int asus_brt_register_listener(struct asus_brt_listener *bdev)
>>>>> +{
>>>>> +     int ret;
>>>>> +
>>>>> +     mutex_lock(&asus_brt_lock);
>>>>> +     list_add_tail(&bdev->list, &asus_brt_listeners);
>>>>> +     if (asus_brt_ref) {
>>>>
>>>> ret is not initialised if this is false
>>>
>>> I already fixed that for V3.
>>>
>>>>> +             if (asus_brt_ref->kbd_led_registered && asus_brt_ref->kbd_led_wk >= 0)
>>>
>>> This nested && is a bug, I will fix that if I havent. We might end up
>>> initializing twice.
>>>
>>>>> +                     bdev->notify(bdev, asus_brt_ref->kbd_led_wk);
>>>>> +             else {
>>>>> +                     asus_brt_ref->kbd_led_registered = true;
>>>>> +                     ret = led_classdev_register(
>>>>> +                             &asus_brt_ref->platform_device->dev,
>>>>> +                             &asus_brt_ref->kbd_led);
>>>>> +             }
>>>
>>> (2) If asus_wmi launched already before the usb devices but did not
>>> support RGB, kbd_led_registered will be false but the struct will be
>>> initialized. Therefore, we register the device here, and all works.
>>>
>>>>> +     }
>>>>> +     mutex_unlock(&asus_brt_lock);
>>>>> +
>>>>> +     return ret;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(asus_brt_register_listener);
>>>>> +
>>>>> +void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
>>>>> +{
>>>>> +     mutex_lock(&asus_brt_lock);
>>>>> +     list_del(&bdev->list);
>>>>> +
>>>>> +     if (asus_brt_ref && asus_brt_ref->kbd_led_registered &&
>>>>> +         list_empty(&asus_brt_listeners) && !asus_brt_ref->kbd_led_avail) {
>>>>> +             led_classdev_unregister(&asus_brt_ref->kbd_led);
>>>>> +             asus_brt_ref->kbd_led_registered = false;
>>>>> +     }
>>>>> +     mutex_unlock(&asus_brt_lock);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(asus_brt_unregister_listener);
>>>>> +
>>>>>     /*
>>>>>      * These functions actually update the LED's, and are called from a
>>>>>      * workqueue. By doing this as separate work rather than when the LED
>>>>> @@ -1566,6 +1608,7 @@ 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_brt_listener *listener;
>>>>>         struct asus_wmi *asus;
>>>>>         int max_level;
>>>>>
>>>>> @@ -1573,7 +1616,12 @@ static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
>>>>>         max_level = asus->kbd_led.max_brightness;
>>>>>
>>>>>         asus->kbd_led_wk = clamp_val(value, 0, max_level);
>>>>> -     kbd_led_update(asus);
>>>>> +
>>>>> +     if (asus->kbd_led_avail)
>>>>> +             kbd_led_update(asus);
>>>>> +
>>>>> +     list_for_each_entry(listener, &asus_brt_listeners, list)
>>>>> +             listener->notify(listener, asus->kbd_led_wk);
>>>>>     }
>>>>>
>>>>>     static void kbd_led_set(struct led_classdev *led_cdev,
>>>>> @@ -1583,15 +1631,21 @@ static void kbd_led_set(struct led_classdev *led_cdev,
>>>>>         if (led_cdev->flags & LED_UNREGISTERING)
>>>>>                 return;
>>>>>
>>>>> +     mutex_lock(&asus_brt_lock);
>>>>>         do_kbd_led_set(led_cdev, value);
>>>>> +     mutex_unlock(&asus_brt_lock);
>>>>>     }
>>>>>
>>>>>     static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
>>>>>     {
>>>>> -     struct led_classdev *led_cdev = &asus->kbd_led;
>>>>> +     struct led_classdev *led_cdev;
>>>>> +
>>>>> +     mutex_lock(&asus_brt_lock);
>>>>> +     led_cdev = &asus->kbd_led;
>>>>>
>>>>>         do_kbd_led_set(led_cdev, value);
>>>>>         led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
>>>>> +     mutex_unlock(&asus_brt_lock);
>>>>>     }
>>>>>
>>>>>     static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
>>>>> @@ -1601,6 +1655,9 @@ static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
>>>>>
>>>>>         asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>>>>>
>>>>> +     if (!asus->kbd_led_avail)
>>>>> +             return asus->kbd_led_wk;
>>>>> +
>>>>>         retval = kbd_led_read(asus, &value, NULL);
>>>>>         if (retval < 0)
>>>>>                 return retval;
>>>>> @@ -1716,7 +1773,12 @@ 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);
>>>>> +     mutex_lock(&asus_brt_lock);
>>>>> +     asus_brt_ref = NULL;
>>>>> +     if (asus->kbd_led_registered)
>>>>> +             led_classdev_unregister(&asus->kbd_led);
>>>>> +     mutex_unlock(&asus_brt_lock);
>>>>> +
>>>>>         led_classdev_unregister(&asus->tpd_led);
>>>>>         led_classdev_unregister(&asus->wlan_led);
>>>>>         led_classdev_unregister(&asus->lightbar_led);
>>>>> @@ -1730,6 +1792,7 @@ static void asus_wmi_led_exit(struct asus_wmi *asus)
>>>>>     static int asus_wmi_led_init(struct asus_wmi *asus)
>>>>>     {
>>>>>         int rv = 0, num_rgb_groups = 0, led_val;
>>>>> +     bool has_listeners;
>>>>>
>>>>>         if (asus->kbd_rgb_dev)
>>>>>                 kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;
>>>>> @@ -1754,24 +1817,37 @@ 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");
>>>>
>>>> Okay so part of the reason the asus_use_hid_led_dmi_ids table was
>>>> created is that some of those laptops had both WMI method, and HID
>>>> method. The WMI method was given priority but on those laptops it didn't
>>>> actually work. What was done was a sort of "blanket" use-hid. I don't
>>>> know why ASUS did this.
>>>>
>>>>> +     asus->kbd_led.name = "asus::kbd_backlight";
>>>>
>>>> I'd like to see this changed to "asus:rgb:kbd_backlight" if RGB is
>>>> supported but this might not be so feasible for the bulk of laptops.
>>>> Given that the Z13 is using a new PID it may be okay there...
>>>
>>> So for the Z13 it is not rgb, this is used just as the common
>>> backlight handler for all rgb devices, so there is no multi-intensity.
>>> The only devices that would be good for would be for those where
>>> kbd_rgb_dev exists, and as this series tries to not affect them,
>>> changing the name is out of scope.
>>>
>>>>> +     asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
>>>>> +     asus->kbd_led.brightness_set = 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);
>>>>
>>>> Per the comment 2x above we will get some laptops returning "yes I
>>>> support this" even though it doesn't actually work. It raises two
>>>> questions for me:
>>>> 1. on machines where it *does* work and they also support HID, do we end
>>>> up with a race condition?
>>>> 2. what is the effect of that race?
>>>>
>>>> I suspect we might need that quirk table still. Unfortunately I
>>>> no-longer have a device where the WMI method was broken, but I will test
>>>> on one 0x1866 device (2019) and a few 0x19b6
>>>>
>>>> No other comments.
>>>
>>> We do not need a quirk anymore actually, the endpoint is created on
>>> demand and there is no race condition. See (1) and (2). I almost gave
>>> up twice writing this until i figured out how to remove the race
>>> conditions.
>>>
>>>> Cheers,
>>>> Luke.
>>>>
>>>>> +
>>>>> +     if (asus->kbd_led_avail)
>>>>>                 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 = kbd_led_set;
>>>>> -             asus->kbd_led.brightness_get = kbd_led_get;
>>>>> -             asus->kbd_led.max_brightness = 3;
>>>>> +     else
>>>>> +             asus->kbd_led_wk = -1;
>>>>> +
>>>>> +     if (asus->kbd_led_avail && num_rgb_groups != 0)
>>>>> +             asus->kbd_led.groups = kbd_rgb_mode_groups;
>>>>>
>>>>> -             if (num_rgb_groups != 0)
>>>>> -                     asus->kbd_led.groups = kbd_rgb_mode_groups;
>>>>> +     mutex_lock(&asus_brt_lock);
>>>>> +     has_listeners = !list_empty(&asus_brt_listeners);
>>>>> +     mutex_unlock(&asus_brt_lock);
>>>>>
>>>>> +     if (asus->kbd_led_avail || has_listeners) {
>>>>>                 rv = led_classdev_register(&asus->platform_device->dev,
>>>>>                                            &asus->kbd_led);
>>>>>                 if (rv)
>>>>>                         goto error;
>>>>> +             asus->kbd_led_registered = true;
>>>>>         }
>>>
>>> (1) If asus-wmi launches first and supports the WMI endpoint,
>>> kbd_led_avail is true so the device is created. If it does not support
>>> it but there is a usb device, then has_listeners is true so it is
>>> still created.
>>
>> The problem is that there are a small group of laptops that report the
>> WMI method as supported, but when used it actually does nothing, so they
>> need to be quirked. These are the ones that will regress, the GU605M is
>> one, I think the ProArt series was another as they use the same build
>> base as the GU605.
>>
>> If both are created then hopefully it's a non-issue since the WMI
>> endpoint is a no-op for set. But what about when both HID and WMI work
>> and both are created?
> 
> This series is created with the assumption that they both do
> something. Hopefully it is not NOOP for get, otherwise the brightness
> might get stuck.
> 
> Currently there is a little branch in the code that uses the previous
> brightness as a reference if a WMI handler is missing.

I will try to get a DSDT dump for the GU605. That should give some 
insight for what to expect.

>>>>>
>>>>> +     mutex_lock(&asus_brt_lock);
>>>>> +     asus_brt_ref = asus;
>>>>> +     mutex_unlock(&asus_brt_lock);
>>>>> +
>>>>>         if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_WIRELESS_LED)
>>>>>                         && (asus->driver->quirks->wapf > 0)) {
>>>>>                 INIT_WORK(&asus->wlan_led_work, wlan_led_update);
>>>>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
>>>>> index 783e2a336861b..42e963b70acdb 100644
>>>>> --- a/include/linux/platform_data/x86/asus-wmi.h
>>>>> +++ b/include/linux/platform_data/x86/asus-wmi.h
>>>>> @@ -157,14 +157,30 @@
>>>>>     #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK       0x0000FF00
>>>>>     #define ASUS_WMI_DSTS_LIGHTBAR_MASK 0x0000000F
>>>>>
>>>>> +struct asus_brt_listener {
>>>>> +     struct list_head list;
>>>>> +     void (*notify)(struct asus_brt_listener *listener, int brightness);
>>>>> +};
>>>>> +
>>>>>     #if IS_REACHABLE(CONFIG_ASUS_WMI)
>>>>>     int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
>>>>> +
>>>>> +int asus_brt_register_listener(struct asus_brt_listener *cdev);
>>>>> +void asus_brt_unregister_listener(struct asus_brt_listener *cdev);
>>>>>     #else
>>>>>     static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
>>>>>                                            u32 *retval)
>>>>>     {
>>>>>         return -ENODEV;
>>>>>     }
>>>>> +
>>>>> +static inline int asus_brt_register_listener(struct asus_brt_listener *bdev)
>>>>> +{
>>>>> +     return -ENODEV;
>>>>> +}
>>>>> +static inline void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
>>>>> +{
>>>>> +}
>>>>>     #endif
>>>>>
>>>>>     /* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
>>>>
>>

Powered by blists - more mailing lists