Re: [PATCH v3 09/10] HID: asus: add basic RGB support

["Luke D. Jones" <luke@ljones.dev>]

On 24/03/25 03:45, Antheas Kapenekakis wrote:
> On Sun, 23 Mar 2025 at 12:37, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>>
>> On Sun, 23 Mar 2025 at 07:40, Luke D. Jones <luke@ljones.dev> wrote:
>>>
>>> On 22/03/25 23:28, Antheas Kapenekakis wrote:
>>>> Adds basic RGB support to hid-asus through multi-index. The interface
>>>> works quite well, but has not gone through much stability testing.
>>>> Applied on demand, if userspace does not touch the RGB sysfs, not
>>>> even initialization is done. Ensuring compatibility with existing
>>>> userspace programs.
>>>>
>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>>> ---
>>>>    drivers/hid/hid-asus.c | 169 +++++++++++++++++++++++++++++++++++++----
>>>>    1 file changed, 155 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>>> index 905453a4eb5b7..9d8ccfde5912e 100644
>>>> --- a/drivers/hid/hid-asus.c
>>>> +++ b/drivers/hid/hid-asus.c
>>>> @@ -30,6 +30,7 @@
>>>>    #include <linux/input/mt.h>
>>>>    #include <linux/usb.h> /* For to_usb_interface for T100 touchpad intf check */
>>>>    #include <linux/power_supply.h>
>>>> +#include <linux/led-class-multicolor.h>
>>>>    #include <linux/leds.h>
>>>>
>>>>    #include "hid-ids.h"
>>>> @@ -85,6 +86,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_HANDLE_GENERIC                BIT(13)
>>>> +#define QUIRK_ROG_NKEY_RGB           BIT(14)
>>>>
>>>>    #define I2C_KEYBOARD_QUIRKS                 (QUIRK_FIX_NOTEBOOK_REPORT | \
>>>>                                                 QUIRK_NO_INIT_REPORTS | \
>>>> @@ -97,9 +99,15 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>>>>
>>>>    struct asus_kbd_leds {
>>>>        struct asus_hid_listener listener;
>>>> +     struct led_classdev_mc mc_led;
>>>> +     struct mc_subled subled_info[3];
>>>>        struct hid_device *hdev;
>>>>        struct work_struct work;
>>>>        unsigned int brightness;
>>>> +     uint8_t rgb_colors[3];
>>>> +     bool rgb_init;
>>>> +     bool rgb_set;
>>>> +     bool rgb_registered;
>>>>        spinlock_t lock;
>>>>        bool removed;
>>>>    };
>>>> @@ -504,23 +512,67 @@ static void asus_schedule_work(struct asus_kbd_leds *led)
>>>>        spin_unlock_irqrestore(&led->lock, flags);
>>>>    }
>>>>
>>>> -static void asus_kbd_backlight_set(struct asus_hid_listener *listener,
>>>> +static void do_asus_kbd_backlight_set(struct asus_kbd_leds *led, int brightness)
>>>> +{
>>>> +     unsigned long flags;
>>>> +
>>>> +     spin_lock_irqsave(&led->lock, flags);
>>>> +     led->brightness = brightness;
>>>> +     spin_unlock_irqrestore(&led->lock, flags);
>>>> +
>>>> +     asus_schedule_work(led);
>>>> +}
>>>> +
>>>> +static void asus_kbd_listener_set(struct asus_hid_listener *listener,
>>>>                                   int brightness)
>>>>    {
>>>>        struct asus_kbd_leds *led = container_of(listener, struct asus_kbd_leds,
>>>>                                                 listener);
>>>> +     do_asus_kbd_backlight_set(led, brightness);
>>>> +     if (led->rgb_registered) {
>>>> +             led->mc_led.led_cdev.brightness = brightness;
>>>> +             led_classdev_notify_brightness_hw_changed(&led->mc_led.led_cdev,
>>>> +                                                       brightness);
>>>> +     }
>>>> +}
>>>> +
>>>> +static void asus_kbd_brightness_set(struct led_classdev *led_cdev,
>>>> +                                 enum led_brightness brightness)
>>>> +{
>>>> +     struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(led_cdev);
>>>> +     struct asus_kbd_leds *led = container_of(mc_cdev, struct asus_kbd_leds,
>>>> +                                              mc_led);
>>>>        unsigned long flags;
>>>>
>>>>        spin_lock_irqsave(&led->lock, flags);
>>>> -     led->brightness = brightness;
>>>> +     led->rgb_colors[0] = mc_cdev->subled_info[0].intensity;
>>>> +     led->rgb_colors[1] = mc_cdev->subled_info[1].intensity;
>>>> +     led->rgb_colors[2] = mc_cdev->subled_info[2].intensity;
>>>> +     led->rgb_set = true;
>>>>        spin_unlock_irqrestore(&led->lock, flags);
>>>>
>>>> -     asus_schedule_work(led);
>>>> +     do_asus_kbd_backlight_set(led, brightness);
>>>> +}
>>>> +
>>>> +static enum led_brightness asus_kbd_brightness_get(struct led_classdev *led_cdev)
>>>> +{
>>>> +     struct led_classdev_mc *mc_led;
>>>> +     struct asus_kbd_leds *led;
>>>> +     enum led_brightness brightness;
>>>> +     unsigned long flags;
>>>> +
>>>> +     mc_led = lcdev_to_mccdev(led_cdev);
>>>> +     led = container_of(mc_led, struct asus_kbd_leds, mc_led);
>>>> +
>>>> +     spin_lock_irqsave(&led->lock, flags);
>>>> +     brightness = led->brightness;
>>>> +     spin_unlock_irqrestore(&led->lock, flags);
>>>> +
>>>> +     return brightness;
>>>>    }
>>>>
>>>> -static void asus_kbd_backlight_work(struct work_struct *work)
>>>> +static void asus_kbd_backlight_work(struct asus_kbd_leds *led)
>>>>    {
>>>> -     struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work);
>>>>        u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 };
>>>>        int ret;
>>>>        unsigned long flags;
>>>> @@ -534,10 +586,69 @@ static void asus_kbd_backlight_work(struct work_struct *work)
>>>>                hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret);
>>>>    }
>>>>
>>>> +static void asus_kbd_rgb_work(struct asus_kbd_leds *led)
>>>> +{
>>>> +     u8 rgb_buf[][7] = {
>>>> +             { FEATURE_KBD_LED_REPORT_ID1, 0xB3 }, /* set mode */
>>>> +             { FEATURE_KBD_LED_REPORT_ID1, 0xB5 }, /* apply mode */
>>>> +             { FEATURE_KBD_LED_REPORT_ID1, 0xB4 }, /* save to mem */
>>>> +     };
>>>> +     unsigned long flags;
>>>> +     uint8_t colors[3];
>>>> +     bool rgb_init, rgb_set;
>>>> +     int ret;
>>>> +
>>>> +     spin_lock_irqsave(&led->lock, flags);
>>>> +     rgb_init = led->rgb_init;
>>>> +     rgb_set = led->rgb_set;
>>>> +     led->rgb_set = false;
>>>> +     colors[0] = led->rgb_colors[0];
>>>> +     colors[1] = led->rgb_colors[1];
>>>> +     colors[2] = led->rgb_colors[2];
>>>> +     spin_unlock_irqrestore(&led->lock, flags);
>>>> +
>>>> +     if (!rgb_set)
>>>> +             return;
>>>> +
>>>> +     if (rgb_init) {
>>>> +             ret = asus_kbd_init(led->hdev, FEATURE_KBD_LED_REPORT_ID1);
>>>> +             if (ret < 0) {
>>>> +                     hid_err(led->hdev, "Asus failed to init RGB: %d\n", ret);
>>>> +                     return;
>>>> +             }
>>>> +             spin_lock_irqsave(&led->lock, flags);
>>>> +             led->rgb_init = false;
>>>> +             spin_unlock_irqrestore(&led->lock, flags);
>>>> +     }
>>>> +
>>>> +     /* Protocol is: 54b3 zone (0=all) mode (0=solid) RGB */
>>>> +     rgb_buf[0][4] = colors[0];
>>>> +     rgb_buf[0][5] = colors[1];
>>>> +     rgb_buf[0][6] = colors[2];
>>>> +
>>>> +     for (size_t i = 0; i < ARRAY_SIZE(rgb_buf); i++) {
>>>> +             ret = asus_kbd_set_report(led->hdev, rgb_buf[i], sizeof(rgb_buf[i]));
>>>> +             if (ret < 0) {
>>>> +                     hid_err(led->hdev, "Asus failed to set RGB: %d\n", ret);
>>>> +                     return;
>>>> +             }
>>>> +     }
>>>> +}
>>>> +
>>>> +static void asus_kbd_work(struct work_struct *work)
>>>> +{
>>>> +     struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds,
>>>> +                                              work);
>>>> +     asus_kbd_backlight_work(led);
>>>> +     asus_kbd_rgb_work(led);
>>>> +}
>>>> +
>>>>    static int asus_kbd_register_leds(struct hid_device *hdev)
>>>>    {
>>>>        struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>>>>        unsigned char kbd_func;
>>>> +     struct asus_kbd_leds *leds;
>>>> +     bool no_led;
>>>>        int ret;
>>>>
>>>>        ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
>>>> @@ -565,21 +676,51 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>>>>        if (!drvdata->kbd_backlight)
>>>>                return -ENOMEM;
>>>>
>>>> -     drvdata->kbd_backlight->removed = false;
>>>> -     drvdata->kbd_backlight->brightness = 0;
>>>> -     drvdata->kbd_backlight->hdev = hdev;
>>>> -     drvdata->kbd_backlight->listener.brightness_set = asus_kbd_backlight_set;
>>>> -     INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work);
>>>> +     leds = drvdata->kbd_backlight;
>>>> +     leds->removed = false;
>>>> +     leds->brightness = 3;
>>>> +     leds->hdev = hdev;
>>>> +     leds->listener.brightness_set = asus_kbd_listener_set;
>>>> +
>>>> +     leds->rgb_colors[0] = 0;
>>>> +     leds->rgb_colors[1] = 0;
>>>> +     leds->rgb_colors[2] = 0;
>>>> +     leds->rgb_init = true;
>>>> +     leds->rgb_set = false;
>>>> +     leds->mc_led.led_cdev.name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
>>>> +                                     "asus-%s-led",
>>>> +                                     strlen(hdev->uniq) ?
>>>> +                                     hdev->uniq : dev_name(&hdev->dev));
>>>
>>> A quick note. This breaks convention for LED names. The style guide is
>>> at Documentation/leds/leds-class.rst. Per my parallel email to this one
>>> I would like to see the mentioned naming scheme `asus:rgb:kbd_backlight`
>>> adopted.
>>
>> Perhaps. It would be the first kbd_backlight driver to have "rgb" in
>> it. It is a bit out of scope for this series as I do not touch the
>> functionality of it but I can add a patch for it and a fixes
>> e305a71cea37a64c75 tag.
>>
>>> Expanding further on one of the points there you might need to
>>> move the led_classdev_mc in to asus-wmi to fulfil having the single
>>> sysfs endpoint. Since you're using the listner pattern it shouldn't be
>>> much work.
>>
>> I only want the brightness to sync, not the color. Only the brightness
>> between Aura devices needs to be the same. In this case
>> asus::kbd_backlight if it has a color controls the wmi color, and the
>> asus- devices control the usb.
>>
>> Also, groups are not dynamic so this is not possible. E.g., if you
>> setup a WMI listener that does not have RGB, and then the USB keyboard
>> connects you can no longer change the groups unless you reconnect the
>> device.
> 
> Sorry, I confused the patches. Yes you are right. I cannot do
> kbd_backlight because userspace might pick the wrong handler. And with
> this patch series on the Z13 there are 3, one for the common
> backlight, one for the keyboard, and one for the lightbar.
> 
> I can do asus-UNIQ:rgb:peripheral though. There is no appropriate
> function that is close enough currently. Also, since there can be
> multiple devices, UNIQ or something similar needs to be added,
> otherwise the led handler will suffix _X.
> 

That sounds appropriate yeah. Is there an issue with the suffix number 
being added? I've not encountered any myself.

Cheers.
Luke.

> Antheas
> 
>>>> +     leds->mc_led.led_cdev.flags = LED_BRIGHT_HW_CHANGED;
>>>> +     leds->mc_led.led_cdev.max_brightness = 3,
>>>> +     leds->mc_led.led_cdev.brightness_set = asus_kbd_brightness_set,
>>>> +     leds->mc_led.led_cdev.brightness_get = asus_kbd_brightness_get,
>>>> +     leds->mc_led.subled_info = leds->subled_info,
>>>> +     leds->mc_led.num_colors = ARRAY_SIZE(leds->subled_info),
>>>> +     leds->subled_info[0].color_index = LED_COLOR_ID_RED;
>>>> +     leds->subled_info[1].color_index = LED_COLOR_ID_GREEN;
>>>> +     leds->subled_info[2].color_index = LED_COLOR_ID_BLUE;
>>>> +
>>>> +     INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_work);
>>>>        spin_lock_init(&drvdata->kbd_backlight->lock);
>>>>
>>>>        ret = asus_hid_register_listener(&drvdata->kbd_backlight->listener);
>>>> +     no_led = !!ret;
>>>> +
>>>> +     if (drvdata->quirks & QUIRK_ROG_NKEY_RGB) {
>>>> +             ret = devm_led_classdev_multicolor_register(
>>>> +                     &hdev->dev, &leds->mc_led);
>>>> +             if (!ret)
>>>> +                     leds->rgb_registered = true;
>>>> +             no_led &= !!ret;
>>>> +     }
>>>>
>>>> -     if (ret < 0) {
>>>> +     if (no_led) {
>>>>                /* No need to have this still around */
>>>>                devm_kfree(&hdev->dev, drvdata->kbd_backlight);
>>>>        }
>>>>
>>>> -     return ret;
>>>> +     return no_led ? -ENODEV : 0;
>>>>    }
>>>>
>>>>    /*
>>>> @@ -1289,7 +1430,7 @@ static const struct hid_device_id asus_devices[] = {
>>>>          QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>>        { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>            USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
>>>> -       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>> +       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
>>>>        { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>>            USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY),
>>>>          QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>> @@ -1318,7 +1459,7 @@ static const struct hid_device_id asus_devices[] = {
>>>>         */
>>>>        { HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
>>>>                USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_ROG_Z13_FOLIO),
>>>> -       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>> +       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
>>>>        { HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
>>>>                USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_T101HA_KEYBOARD) },
>>>>        { }
>>>