| Message ID | 20250325184601.10990-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 Tue, 25 Mar 2025, 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/Kconfig | 1 + > drivers/hid/hid-asus.c | 171 +++++++++++++++++++++++++++++++++++++---- > 2 files changed, 156 insertions(+), 16 deletions(-) > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > index dfc245867a46a..d324c6ab997de 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -151,6 +151,7 @@ config HID_APPLEIR > config HID_ASUS > tristate "Asus" > depends on USB_HID > + depends on LEDS_CLASS_MULTICOLOR > depends on LEDS_CLASS > depends on ASUS_WMI || ASUS_WMI=n > select POWER_SUPPLY > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c > index a4d1160460935..c135c9ff87b74 100644 > --- a/drivers/hid/hid-asus.c > +++ b/drivers/hid/hid-asus.c > @@ -23,6 +23,7 @@ > /* > */ > > +#include <linux/array_size.h> > #include <linux/dmi.h> > #include <linux/hid.h> > #include <linux/module.h> > @@ -30,6 +31,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" > @@ -52,6 +54,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad"); > #define FEATURE_KBD_REPORT_SIZE 64 > #define FEATURE_KBD_LED_REPORT_ID1 0x5d > #define FEATURE_KBD_LED_REPORT_ID2 0x5e > +#define FEATURE_KBD_LED_REPORT_SIZE 7 > > #define SUPPORT_KBD_BACKLIGHT BIT(0) > > @@ -86,6 +89,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 | \ > @@ -98,9 +102,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; > + u8 rgb_colors[3]; > + bool rgb_init; > + bool rgb_set; > + bool rgb_registered; > spinlock_t lock; > bool removed; > }; > @@ -501,23 +511,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; > @@ -531,10 +585,68 @@ 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[][FEATURE_KBD_LED_REPORT_SIZE] = { > + { 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 */ Hmm, I don't know why I didn't notice this the last scan... Since you're anyway adding those comments to explain the literals (I assume), wouldn't it be simply better to add defines for 0xB* and forgo the comments. Usually when one adds any comment, it's always good to ask first oneself if things could be named such that the code itself explains itself. This leaves comments mostly for places where something really odd/tricky/complex occurs as the rest can be covered by simply naming better. > + }; > + 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 */ BTW, this comment is very cryptic to me and I'm unable to connect it with the code below. My only guess is that each non-parenthesized word is explaining one index but things don't add up given what rgb_buf[0][0] and [0][1] have. > + 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; > int ret; > > ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); > @@ -562,20 +674,47 @@ 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 = ASUS_EV_MAX_BRIGHTNESS; > + 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:rgb:peripheral", > + 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 = ASUS_EV_MAX_BRIGHTNESS; > + 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); > - if (ret < 0) { > - /* No need to have this still around */ > - devm_kfree(&hdev->dev, drvdata->kbd_backlight); > + /* Asus-wmi might not be accessible so this is not fatal. */ > + if (!ret) > + hid_warn(hdev, "Asus-wmi brightness listener not registered\n"); Is the condition correct way around given the message? Please also note that you don't need to send an update every day or so after minor comments like this. We're in merge window currently which means I likely won't be applying any next material until -rc1 has been released. > + if (drvdata->quirks & QUIRK_ROG_NKEY_RGB) { > + ret = devm_led_classdev_multicolor_register(&hdev->dev, &leds->mc_led); > + if (!ret) > + leds->rgb_registered = true; > + return ret; > } > > - return ret; > + return 0; > } > > /* > @@ -1282,7 +1421,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 }, > @@ -1311,7 +1450,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 Wed, 26 Mar 2025 at 09:54, Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > On Tue, 25 Mar 2025, 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/Kconfig | 1 + > > drivers/hid/hid-asus.c | 171 +++++++++++++++++++++++++++++++++++++---- > > 2 files changed, 156 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > > index dfc245867a46a..d324c6ab997de 100644 > > --- a/drivers/hid/Kconfig > > +++ b/drivers/hid/Kconfig > > @@ -151,6 +151,7 @@ config HID_APPLEIR > > config HID_ASUS > > tristate "Asus" > > depends on USB_HID > > + depends on LEDS_CLASS_MULTICOLOR > > depends on LEDS_CLASS > > depends on ASUS_WMI || ASUS_WMI=n > > select POWER_SUPPLY > > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c > > index a4d1160460935..c135c9ff87b74 100644 > > --- a/drivers/hid/hid-asus.c > > +++ b/drivers/hid/hid-asus.c > > @@ -23,6 +23,7 @@ > > /* > > */ > > > > +#include <linux/array_size.h> > > #include <linux/dmi.h> > > #include <linux/hid.h> > > #include <linux/module.h> > > @@ -30,6 +31,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" > > @@ -52,6 +54,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad"); > > #define FEATURE_KBD_REPORT_SIZE 64 > > #define FEATURE_KBD_LED_REPORT_ID1 0x5d > > #define FEATURE_KBD_LED_REPORT_ID2 0x5e > > +#define FEATURE_KBD_LED_REPORT_SIZE 7 > > > > #define SUPPORT_KBD_BACKLIGHT BIT(0) > > > > @@ -86,6 +89,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 | \ > > @@ -98,9 +102,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; > > + u8 rgb_colors[3]; > > + bool rgb_init; > > + bool rgb_set; > > + bool rgb_registered; > > spinlock_t lock; > > bool removed; > > }; > > @@ -501,23 +511,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; > > @@ -531,10 +585,68 @@ 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[][FEATURE_KBD_LED_REPORT_SIZE] = { > > + { 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 */ > > Hmm, I don't know why I didn't notice this the last scan... Since you're > anyway adding those comments to explain the literals (I assume), wouldn't > it be simply better to add defines for 0xB* and forgo the comments. > > Usually when one adds any comment, it's always good to ask first oneself > if things could be named such that the code itself explains itself. This > leaves comments mostly for places where something really > odd/tricky/complex occurs as the rest can be covered by simply naming > better. I guess I can turn them into FEATURE_KBD_LED_CMD_SETMODE etc. > > + }; > > + 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 */ > > BTW, this comment is very cryptic to me and I'm unable to connect it with > the code below. My only guess is that each non-parenthesized word is > explaining one index but things don't add up given what rgb_buf[0][0] and > [0][1] have. Maybe i fatfingered 54 and it should be 5a. Protocol is 54b3 zone mode R G B. So colors go to indexes 4, 5, 6 > > + 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; > > int ret; > > > > ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); > > @@ -562,20 +674,47 @@ 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 = ASUS_EV_MAX_BRIGHTNESS; > > + 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:rgb:peripheral", > > + 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 = ASUS_EV_MAX_BRIGHTNESS; > > + 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); > > - if (ret < 0) { > > - /* No need to have this still around */ > > - devm_kfree(&hdev->dev, drvdata->kbd_backlight); > > + /* Asus-wmi might not be accessible so this is not fatal. */ > > + if (!ret) > > + hid_warn(hdev, "Asus-wmi brightness listener not registered\n"); > > Is the condition correct way around given the message? You are right. > Please also note that you don't need to send an update every day or so > after minor comments like this. We're in merge window currently which > means I likely won't be applying any next material until -rc1 has been > released. If this is 6.16 material I am happy to put a pause on this for the next 1-3 weeks. Antheas > > + if (drvdata->quirks & QUIRK_ROG_NKEY_RGB) { > > + ret = devm_led_classdev_multicolor_register(&hdev->dev, &leds->mc_led); > > + if (!ret) > > + leds->rgb_registered = true; > > + return ret; > > } > > > > - return ret; > > + return 0; > > } > > > > /* > > @@ -1282,7 +1421,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 }, > > @@ -1311,7 +1450,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) }, > > { } > > > > -- > i. >
On Wed, 26 Mar 2025, Antheas Kapenekakis wrote: > On Wed, 26 Mar 2025 at 09:54, Ilpo Järvinen > <ilpo.jarvinen@linux.intel.com> wrote: > > > > On Tue, 25 Mar 2025, 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/Kconfig | 1 + > > > drivers/hid/hid-asus.c | 171 +++++++++++++++++++++++++++++++++++++---- > > > 2 files changed, 156 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > > > index dfc245867a46a..d324c6ab997de 100644 > > > + }; > > > + 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 */ > > > > BTW, this comment is very cryptic to me and I'm unable to connect it with > > the code below. My only guess is that each non-parenthesized word is > > explaining one index but things don't add up given what rgb_buf[0][0] and > > [0][1] have. > > Maybe i fatfingered 54 and it should be 5a. Protocol is 54b3 zone mode > R G B. So colors go to indexes 4, 5, 6 Ah. I suggest you add the spaces between the bytes to make it more obvious. Although, this could be a constructed as struct as well in which case the struct itself would document the format without need to cryptic comments nor use of numeric indexes. > > > + 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; > > > + } > > > + } > > > +} > > > ret = asus_hid_register_listener(&drvdata->kbd_backlight->listener); > > > - if (ret < 0) { > > > - /* No need to have this still around */ > > > - devm_kfree(&hdev->dev, drvdata->kbd_backlight); > > > + /* Asus-wmi might not be accessible so this is not fatal. */ > > > + if (!ret) > > > + hid_warn(hdev, "Asus-wmi brightness listener not registered\n"); > > > > Is the condition correct way around given the message? > > You are right. > > > Please also note that you don't need to send an update every day or so > > after minor comments like this. We're in merge window currently which > > means I likely won't be applying any next material until -rc1 has been > > released. > > If this is 6.16 material I am happy to put a pause on this for the > next 1-3 weeks. You don't need to "pause" for the merge window, in some subsystem there's mandatory pause during merge window but I find that unnecessary. I know people on pdx86 do review during merge window so no need to wait when working with patches related to pdx86. Just don't expect patches get applied during the merge window or right after it (the latter tends to be the most busiest time of cycle for me) :-). It's more about the frequency, how often to send a series which is relatively large. Large number of versions end up just filling inboxes (and patchwork's pending patches list) and we don't have time to read them all through so I suggest waiting like 3 days at minimum between versions when the series is large or complex to give time to go through the series. This is not a hard rule, so if there are e.g. many significant changes, feel free to "violate" it in that case.
On Wed, 26 Mar 2025, Ilpo Järvinen wrote: > You don't need to "pause" for the merge window, in some subsystem > there's mandatory pause during merge window but I find that unnecessary. > I know people on pdx86 do review during merge window so no need to wait > when working with patches related to pdx86. Just don't expect patches > get applied during the merge window or right after it (the latter tends to > be the most busiest time of cycle for me) :-). > > It's more about the frequency, how often to send a series which is > relatively large. Large number of versions end up just filling inboxes > (and patchwork's pending patches list) and we don't have time to read them > all through so I suggest waiting like 3 days at minimum between versions > when the series is large or complex to give time to go through the series. > > This is not a hard rule, so if there are e.g. many significant changes, > feel free to "violate" it in that case. Exactly. I am unlikely to do much review during the merge window myself, but I'll pick up the patchset and followup once the merge window is over, so feel free to keep discussing and polishing it with me on CC :) Thanks,
On Wed, 26 Mar 2025 at 12:00, Jiri Kosina <jikos@kernel.org> wrote: > > On Wed, 26 Mar 2025, Ilpo Järvinen wrote: > > > You don't need to "pause" for the merge window, in some subsystem > > there's mandatory pause during merge window but I find that unnecessary. > > I know people on pdx86 do review during merge window so no need to wait > > when working with patches related to pdx86. Just don't expect patches > > get applied during the merge window or right after it (the latter tends to > > be the most busiest time of cycle for me) :-). > > > > It's more about the frequency, how often to send a series which is > > relatively large. Large number of versions end up just filling inboxes > > (and patchwork's pending patches list) and we don't have time to read them > > all through so I suggest waiting like 3 days at minimum between versions > > when the series is large or complex to give time to go through the series. > > > > This is not a hard rule, so if there are e.g. many significant changes, > > feel free to "violate" it in that case. > > Exactly. I am unlikely to do much review during the merge window myself, > but I'll pick up the patchset and followup once the merge window is over, > so feel free to keep discussing and polishing it with me on CC :) > > Thanks, I think we have reached a good point with this series. We can pick up again when you guys are ready. I will switch gears and look a bit into msi-wmi-platform for the MSI Claw with Armin and we can revisit this come rc1. Let's try to get through it early in 6.16 so that Luke can also do what he wants to with the Ally, and let's push the oxpec move as well, so I can get those two off my plate. Antheas > -- > Jiri Kosina > SUSE Labs >
On 30/03/25 21:39, Antheas Kapenekakis wrote: > On Wed, 26 Mar 2025 at 12:00, Jiri Kosina <jikos@kernel.org> wrote: >> >> On Wed, 26 Mar 2025, Ilpo Järvinen wrote: >> >>> You don't need to "pause" for the merge window, in some subsystem >>> there's mandatory pause during merge window but I find that unnecessary. >>> I know people on pdx86 do review during merge window so no need to wait >>> when working with patches related to pdx86. Just don't expect patches >>> get applied during the merge window or right after it (the latter tends to >>> be the most busiest time of cycle for me) :-). >>> >>> It's more about the frequency, how often to send a series which is >>> relatively large. Large number of versions end up just filling inboxes >>> (and patchwork's pending patches list) and we don't have time to read them >>> all through so I suggest waiting like 3 days at minimum between versions >>> when the series is large or complex to give time to go through the series. >>> >>> This is not a hard rule, so if there are e.g. many significant changes, >>> feel free to "violate" it in that case. >> >> Exactly. I am unlikely to do much review during the merge window myself, >> but I'll pick up the patchset and followup once the merge window is over, >> so feel free to keep discussing and polishing it with me on CC :) >> >> Thanks, > > I think we have reached a good point with this series. We can pick up > again when you guys are ready. > > I will switch gears and look a bit into msi-wmi-platform for the MSI > Claw with Armin and we can revisit this come rc1. > > Let's try to get through it early in 6.16 so that Luke can also do > what he wants to with the Ally, and let's push the oxpec move as well, > so I can get those two off my plate. I see Ilpo and Jiri mentioned no need to pause development. But it does look like everything is in good state so far, and no doubt you might have some ideas after a few days (plus that did of helpful info asus dropped in my lap). If you do anything significant and would like someone to test on a few variety of laptop please let me know privately and I'll do what I can. Cheers, Luke. > Antheas > >> -- >> Jiri Kosina >> SUSE Labs >>
On Mon, 31 Mar 2025 at 10:19, Luke D. Jones <luke@ljones.dev> wrote: > > On 30/03/25 21:39, Antheas Kapenekakis wrote: > > On Wed, 26 Mar 2025 at 12:00, Jiri Kosina <jikos@kernel.org> wrote: > >> > >> On Wed, 26 Mar 2025, Ilpo Järvinen wrote: > >> > >>> You don't need to "pause" for the merge window, in some subsystem > >>> there's mandatory pause during merge window but I find that unnecessary. > >>> I know people on pdx86 do review during merge window so no need to wait > >>> when working with patches related to pdx86. Just don't expect patches > >>> get applied during the merge window or right after it (the latter tends to > >>> be the most busiest time of cycle for me) :-). > >>> > >>> It's more about the frequency, how often to send a series which is > >>> relatively large. Large number of versions end up just filling inboxes > >>> (and patchwork's pending patches list) and we don't have time to read them > >>> all through so I suggest waiting like 3 days at minimum between versions > >>> when the series is large or complex to give time to go through the series. > >>> > >>> This is not a hard rule, so if there are e.g. many significant changes, > >>> feel free to "violate" it in that case. > >> > >> Exactly. I am unlikely to do much review during the merge window myself, > >> but I'll pick up the patchset and followup once the merge window is over, > >> so feel free to keep discussing and polishing it with me on CC :) > >> > >> Thanks, > > > > I think we have reached a good point with this series. We can pick up > > again when you guys are ready. > > > > I will switch gears and look a bit into msi-wmi-platform for the MSI > > Claw with Armin and we can revisit this come rc1. > > > > Let's try to get through it early in 6.16 so that Luke can also do > > what he wants to with the Ally, and let's push the oxpec move as well, > > so I can get those two off my plate. > > I see Ilpo and Jiri mentioned no need to pause development. But it does > look like everything is in good state so far, and no doubt you might > have some ideas after a few days (plus that did of helpful info asus > dropped in my lap). > > If you do anything significant and would like someone to test on a few > variety of laptop please let me know privately and I'll do what I can. Yeah, I think the comments I got so far I could fix in an hour or so, so I would like some more to pool in. It is good we have a way to parse the keyboard check [1] now, but all this boils down to removing the RGB quirk and tweaking how the result of that function is parsed. If you have some deadtime atm, you can check the MSI Claw draft I am working on [2]. It is still too early to send. The fwupd integration is causing some issues, such as it being picky and requiring all optional vars are implemented, and I think the potential interference with shift mode (platform profile in MSI laptops) will be confusing as well. But the same issues are present in Asus and Lenovo Legion too, so MSI is not unique in that regard. Antheas [1] https://github.com/torvalds/linux/blob/4e82c87058f45e79eeaa4d5bcc3b38dd3dce7209/drivers/hid/hid-asus.c#L402-L434 [2] https://github.com/bazzite-org/patchwork/tree/msi > Cheers, > Luke. > > > Antheas > > > >> -- > >> Jiri Kosina > >> SUSE Labs > >> >
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index dfc245867a46a..d324c6ab997de 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -151,6 +151,7 @@ config HID_APPLEIR config HID_ASUS tristate "Asus" depends on USB_HID + depends on LEDS_CLASS_MULTICOLOR depends on LEDS_CLASS depends on ASUS_WMI || ASUS_WMI=n select POWER_SUPPLY diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c index a4d1160460935..c135c9ff87b74 100644 --- a/drivers/hid/hid-asus.c +++ b/drivers/hid/hid-asus.c @@ -23,6 +23,7 @@ /* */ +#include <linux/array_size.h> #include <linux/dmi.h> #include <linux/hid.h> #include <linux/module.h> @@ -30,6 +31,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" @@ -52,6 +54,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad"); #define FEATURE_KBD_REPORT_SIZE 64 #define FEATURE_KBD_LED_REPORT_ID1 0x5d #define FEATURE_KBD_LED_REPORT_ID2 0x5e +#define FEATURE_KBD_LED_REPORT_SIZE 7 #define SUPPORT_KBD_BACKLIGHT BIT(0) @@ -86,6 +89,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 | \ @@ -98,9 +102,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; + u8 rgb_colors[3]; + bool rgb_init; + bool rgb_set; + bool rgb_registered; spinlock_t lock; bool removed; }; @@ -501,23 +511,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; @@ -531,10 +585,68 @@ 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[][FEATURE_KBD_LED_REPORT_SIZE] = { + { 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; int ret; ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); @@ -562,20 +674,47 @@ 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 = ASUS_EV_MAX_BRIGHTNESS; + 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:rgb:peripheral", + 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 = ASUS_EV_MAX_BRIGHTNESS; + 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); - if (ret < 0) { - /* No need to have this still around */ - devm_kfree(&hdev->dev, drvdata->kbd_backlight); + /* Asus-wmi might not be accessible so this is not fatal. */ + if (!ret) + hid_warn(hdev, "Asus-wmi brightness listener not registered\n"); + + if (drvdata->quirks & QUIRK_ROG_NKEY_RGB) { + ret = devm_led_classdev_multicolor_register(&hdev->dev, &leds->mc_led); + if (!ret) + leds->rgb_registered = true; + return ret; } - return ret; + return 0; } /* @@ -1282,7 +1421,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 }, @@ -1311,7 +1450,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/Kconfig | 1 + drivers/hid/hid-asus.c | 171 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 156 insertions(+), 16 deletions(-)