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

["Luke D. Jones" <luke@xxxxxxxxxx>]

On 21/03/25 11:09, 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@xxxxxxxxxxx>
> ---
>   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 546603f5193c7..5e87923b35520 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_brt_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;
>   };
> @@ -505,23 +513,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_brt_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_brt_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;
> @@ -535,10 +587,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);
> @@ -566,21 +677,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.notify = 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.notify = 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_brt_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;
>   }
>   
>   /*
> @@ -1305,7 +1446,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 },
> @@ -1334,7 +1475,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) },
>   	{ }

Hi Antheas,

This is something I've wanted to do for a while now but a number of 
things stopped me. I've supported 80+ laptops over the years with either 
the 0x1866 or 0x19b6 PID, and now we have 4 new PID for devices using 
the ITE MCU. In all of this over I think more than 7 years ASUS has been 
remarkably consistent with the HID API so we're lucky there.. However:

- the PID I mention have been used for both white only, and RGB devices
- MCU LED modes vary between all except I think the first 3
- software mode (as asus calls it) LED addressing varies

there's just no way to tell what each of the 80 plus models supports 
besides the manual tracking I've been doing (and that is tiresome).

None of the above is an issue for the Z13 series using the 0x1a30 PID 
though. Unless ASUS decides to release a white only LED version :(

I've asked my contacts for some details about the HID protocol they're 
using, especially for a method to get capabilities if one exits. You'll 
be the first to know about it if I get the details as I'd very much like 
to have first class RGB for all these devices. ASUS are a bit funny 
though, I may get back a "No because security" or something equally 
nonsense - depends on which internal team I need it from and what the 
current politics between teams is.

In the meantime, maybe a NACK so we can get the rest of the series in 
faster? We should definitely iterate on RGB with an eye to support all 
devices if possible otherwise a patch just for Z13 would be fine if that 
ends up not being feasible.

Some things that might be of interest regarding LampArray for LED:
- An implementation of virtual LampArray 
https://lore.kernel.org/all/20250121225510.751444-2-wse@xxxxxxxxxxxxxxxxxxx/
- Windows blog: 
https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/dynamic-lighting-devices
- Windows docs: 
https://github.com/MicrosoftDocs/windows-dev-docs/blob/docs/uwp/devices-sensors/lighting-dynamic-lamparray.md
- PDF spec: 
https://www.usb.org/sites/default/files/hutrr84_-_lighting_and_illumination_page.pdf

LampArray is pretty new and I suspect your Z13 supports it. My G16 does 
too I think. I've seen very little mentioning or using it besides Tuxedo 
though so I'm not sure what use it is to us at this point, everything 
I've seen so far expects userspace to use it, not the kernel. There's 
been little interest anywhere I can see which is a shame as this is a 
much better interface. Sorry about this part being a bit off topic.

Cheers,
Luke.