| Message ID | 20250322102804.418000-10-lkml@antheas.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | HID: asus: Add RGB Support to Asus Z13, Ally, unify backlight asus-wmi, and Z13 QOL | expand |
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)); > + 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) }, > { } Tested as working on 0x1866 and 0x19B6 PID devices. No code review. They have a path of `/sys/class/leds/asus-0003:0B05:19B6.0001-led`, was this intentional? I was under the impression that you would have added the mcled to the base `asus::kbd_backlight`.. Apologies if the answer is in code, I've only tested for now, not much time for full review but I do have some questions/opinons: I *think* the majority of ROG are RGB, but now I regret not actually tracking this so I hope ASUS come back to me with some kind of query we can ask the MCU for this info. Because I think the majority are RGB I'm coming around to saying yes to this patch but with caveats: 1. mcled added to the base led 2. `asus:rgb:kbd_backlight` name if RGB, `asus:white:kbd_backlight` if white only, `asus::kbd_backlight` if unknown 3. a quirk system to label an MCU as white only (since the bulk are RGB) If you only do 1 + 2, or even just 1 I'm happy with that. I can take care of either 3, or 2 + 3. The last one just means I'll have to spend some time looking up specs or writing a crawler to find data. I will try to review the code in the next days, but if you change it based on the above I'll hold off until then. Cheers, Luke. P.S: on almost all white-only LED devices I've encountered the RED channel works for white intensity (green and blue do nothing). So even if we decide to blanket enable RGB for 0x1866 and 0x19B6 it won't cause issues beyond the naming and API facing.
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. 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. > + 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) }, > { }
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. > > + 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) }, > > { } >
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. 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) }, > > > { } > >
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) }, >>>> { } >>>
On 24/03/25 00:37, Antheas Kapenekakis 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. > Your proposal for naming in other reply is good :) If the intent is to keep 0-3 brightness and RGB controls separated the kbd_backlight doesn't need adjustment in naming, particularly if the RGB is *not* inside `asus:rgb:kbd_backlight`. >> 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. > Hmm, what about multicolour brightness? Otherwise yeah, understood and I'm fine with that. Given you now ustilise the kbd_dill<up/down> directly I don't see any issues there either. > 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. That's a shame. Oh well. I would have preferred RGB and brightness combined. At a glance I would have stored RGB in a global or similar, then if a listener has mcled available that value would be applied. But userpsace should be using udev libs and similar to find devices like this, so naming is more of a hint alongside the attributes. Meaning name changes or including mcled inside standard led shouldn't break things. Neither should keeping them separated as you have. Apologies for the bikeshedding on this. I had been reluctant to add RGB myself for a number of reasons: 1. Windows uses LampArray for the dynamic LED (new) 2. Otherwise you need to use MCU software mode, that's what that mode is for 3. I don't know of a capabilities request for MCU software mode 4. Or you're forced to set MCU mode static/solid 5. Single colour keybaords vs RGB, on same PID :( I considered adding attributes to mcled in sysfs for mode/speed/direction. But then I had to add an enourmous table of modes-per-model. At the end of the day I think your solution is fine and we don't have much other choice beyond trying to introduce a new API better suited for RGB keyboards with many features. I suspect it will also be fine for other laptops since the mode is set on write to RGB, so hidraw is still open to userspace. With the other changes in place I'll experiment a little with the laptops I have and see how it works. I have two that i will need to check HID and WMI on even though it's not an issue for the Z13 and Ally, it might highlight any pain points and improve things further. Could be a few days, or coming weekend sorry, I've got a massive amount of work on, besides my own kernel patches. Cheers, Luke. >>> + 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) }, >>> { } >>
On Sun, 23 Mar 2025 at 21:39, Luke D. Jones <luke@ljones.dev> wrote: > > On 24/03/25 00:37, Antheas Kapenekakis 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. > > > > Your proposal for naming in other reply is good :) > If the intent is to keep 0-3 brightness and RGB controls separated the > kbd_backlight doesn't need adjustment in naming, particularly if the RGB > is *not* inside `asus:rgb:kbd_backlight`. > > >> 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. > > > > Hmm, what about multicolour brightness? Otherwise yeah, understood and > I'm fine with that. Given you now ustilise the kbd_dill<up/down> > directly I don't see any issues there either. > > > 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. > > That's a shame. Oh well. > > I would have preferred RGB and brightness combined. At a glance I would > have stored RGB in a global or similar, then if a listener has mcled > available that value would be applied. There is also a brightness var in the usb endpoint thats duplicated. So it can be controlled individually. But writing a brightness to asus::kbd_brightness or using the keyboard shortcut will override it. Only way I could think of to manage it. > But userpsace should be using udev libs and similar to find devices like > this, so naming is more of a hint alongside the attributes. Meaning name > changes or including mcled inside standard led shouldn't break things. > Neither should keeping them separated as you have. > > Apologies for the bikeshedding on this. I had been reluctant to add RGB > myself for a number of reasons: > > 1. Windows uses LampArray for the dynamic LED (new) Yeah i played a bit with dynamic lighting on windows on the Ally. It looks like a mess with Armoury crate and windows fighting over the leds. > 2. Otherwise you need to use MCU software mode, that's what that mode is for > 3. I don't know of a capabilities request for MCU software mode > 4. Or you're forced to set MCU mode static/solid > 5. Single colour keybaords vs RGB, on same PID :( > > I considered adding attributes to mcled in sysfs for > mode/speed/direction. But then I had to add an enourmous table of > modes-per-model. > > At the end of the day I think your solution is fine and we don't have > much other choice beyond trying to introduce a new API better suited for > RGB keyboards with many features. I suspect it will also be fine for > other laptops since the mode is set on write to RGB, so hidraw is still > open to userspace. I think solid is a great start. I found myself using it this week. Then getting the KDE guys to add a color picker. > With the other changes in place I'll experiment a little with the > laptops I have and see how it works. I have two that i will need to > check HID and WMI on even though it's not an issue for the Z13 and Ally, > it might highlight any pain points and improve things further. Could be > a few days, or coming weekend sorry, I've got a massive amount of work > on, besides my own kernel patches. That's alright with me. I plan to send a V4 middle of the week. I will try to include a patch that inits 0x5d that can be reverted later if it is not needed. Also have some other stuff to deal with Bazzite. Mesa 25 was a big PITA Antheas > Cheers, > Luke. > > >>> + 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) }, > >>> { } > >> >
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)); + 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) }, { }
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(-)