| Message ID | 20250322102804.418000-1-lkml@antheas.dev |
|---|---|
| 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:27, Antheas Kapenekakis wrote: > Currently, asus_kbd_init() uses a reverse engineered init sequence > from Windows, which contains the handshakes from multiple programs. > Keep the main one, which is 0x5a (meant for brightness drivers). > > In addition, perform a get_response and check if the response is the > same. To avoid regressions, print an error if the response does not > match instead of rejecting device. > > Then, refactor asus_kbd_get_functions() to use the same ID it is called > with, instead of hardcoding it to 0x5a so that it may be used for 0x0d > in the future. > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > --- > drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++------------------- > 1 file changed, 46 insertions(+), 36 deletions(-) > > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c > index 46e3e42f9eb5f..8d4df1b6f143b 100644 > --- a/drivers/hid/hid-asus.c > +++ b/drivers/hid/hid-asus.c > @@ -48,7 +48,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad"); > #define FEATURE_REPORT_ID 0x0d > #define INPUT_REPORT_ID 0x5d > #define FEATURE_KBD_REPORT_ID 0x5a > -#define FEATURE_KBD_REPORT_SIZE 16 > +#define FEATURE_KBD_REPORT_SIZE 64 > #define FEATURE_KBD_LED_REPORT_ID1 0x5d > #define FEATURE_KBD_LED_REPORT_ID2 0x5e > > @@ -388,14 +388,41 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu > > static int asus_kbd_init(struct hid_device *hdev, u8 report_id) > { > - const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54, > - 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; > + /* > + * Asus handshake identifying us as a driver (0x5A) > + * 0x5A then ASCII for "ASUS Tech.Inc." > + * 0x5D is for userspace Windows applications. > + * > + * The handshake is first sent as a set_report, then retrieved > + * from a get_report to verify the response. > + */ This entire comment is not required, especially not the last paragraph. From what I've seen in .dll reversing attempts there's no real distinction from driver/app and it's simply an init/enable sequence common to almost every ITE MCU that ASUS have used (slash, anime, Ally). Please remove. > + const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, > + 0x54, 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; > + u8 *readbuf; > int ret; > > ret = asus_kbd_set_report(hdev, buf, sizeof(buf)); > - if (ret < 0) > - hid_err(hdev, "Asus failed to send init command: %d\n", ret); > + if (ret < 0) { > + hid_err(hdev, "Asus failed to send handshake: %d\n", ret); > + return ret; > + } > > + readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL); > + if (!readbuf) > + return -ENOMEM; > + > + ret = hid_hw_raw_request(hdev, report_id, readbuf, > + FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT, > + HID_REQ_GET_REPORT); > + if (ret < 0) { > + hid_err(hdev, "Asus failed to receive handshake ack: %d\n", ret); > + } else if (memcmp(readbuf, buf, sizeof(buf)) != 0) { > + hid_err(hdev, "Asus handshake returned invalid response: %*ph\n", > + FEATURE_KBD_REPORT_SIZE, readbuf); > + // Do not return error if handshake is wrong to avoid regressions > + } > + > + kfree(readbuf); > return ret; > } > > @@ -417,7 +444,7 @@ static int asus_kbd_get_functions(struct hid_device *hdev, > if (!readbuf) > return -ENOMEM; > > - ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf, > + ret = hid_hw_raw_request(hdev, report_id, readbuf, > FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT, > HID_REQ_GET_REPORT); > if (ret < 0) { > @@ -540,42 +567,25 @@ static int asus_kbd_register_leds(struct hid_device *hdev) > unsigned char kbd_func; > int ret; > > - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) { > - /* Initialize keyboard */ > - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); > - if (ret < 0) > - return ret; > - > - /* The LED endpoint is initialised in two HID */ > - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1); > - if (ret < 0) > - return ret; > - > - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2); > - if (ret < 0) > - return ret; > + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); > + if (ret < 0) > + return ret; > I don't have any non-ROG equipment to test. There have been some cases of Vivobook using the same MCU, but these otherwise used something else. And the oldest hardware I have uses a 0x1866, which uses the same init sequence but works with both 0x5A and 0x5D report ID for init (on same endpoint). There are instances of other laptops that accept both 0x5A and 0x5D, and some that have only 0x5D. I think you will need to change this to try both 0x5A and 0x5D sorry. The older models using 0x1854, 0x1869, 0x1866 PID may regress and although I'm reasonably confident there won't be issues due to age of those, it's not a risk I'm willing to take, I've spent all morning trawling through archives of info and I can't actually verify this other than from my memory. I mentioned 0x5E being used for some of the oddball devices like slash and anime, don't worry about that one, it's a bridge that can be crossed at a later time. But it does illustrate that other report ID have been used for init. Otherwise the cleanup is good. No other comments required and I'll sign off with the above corrections. Cheers, Luke > - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { > - ret = asus_kbd_disable_oobe(hdev); > - if (ret < 0) > - return ret; > - } > - } else { > - /* Initialize keyboard */ > - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); > - if (ret < 0) > - return ret; > + /* Get keyboard functions */ > + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID); > + if (ret < 0) > + return ret; > > - /* Get keyboard functions */ > - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID); > + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { > + ret = asus_kbd_disable_oobe(hdev); > if (ret < 0) > return ret; > - > - /* Check for backlight support */ > - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT)) > - return -ENODEV; > } > > + /* Check for backlight support */ > + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT)) > + return -ENODEV; > + > drvdata->kbd_backlight = devm_kzalloc(&hdev->dev, > sizeof(struct asus_kbd_leds), > GFP_KERNEL);
On Sat, 22 Mar 2025 at 23:01, Luke D. Jones <luke@ljones.dev> wrote: > > On 22/03/25 23:27, Antheas Kapenekakis wrote: > > Currently, asus_kbd_init() uses a reverse engineered init sequence > > from Windows, which contains the handshakes from multiple programs. > > Keep the main one, which is 0x5a (meant for brightness drivers). > > > > In addition, perform a get_response and check if the response is the > > same. To avoid regressions, print an error if the response does not > > match instead of rejecting device. > > > > Then, refactor asus_kbd_get_functions() to use the same ID it is called > > with, instead of hardcoding it to 0x5a so that it may be used for 0x0d > > in the future. > > > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > > --- > > drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++------------------- > > 1 file changed, 46 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c > > index 46e3e42f9eb5f..8d4df1b6f143b 100644 > > --- a/drivers/hid/hid-asus.c > > +++ b/drivers/hid/hid-asus.c > > @@ -48,7 +48,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad"); > > #define FEATURE_REPORT_ID 0x0d > > #define INPUT_REPORT_ID 0x5d > > #define FEATURE_KBD_REPORT_ID 0x5a > > -#define FEATURE_KBD_REPORT_SIZE 16 > > +#define FEATURE_KBD_REPORT_SIZE 64 > > #define FEATURE_KBD_LED_REPORT_ID1 0x5d > > #define FEATURE_KBD_LED_REPORT_ID2 0x5e > > > > @@ -388,14 +388,41 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu > > > > static int asus_kbd_init(struct hid_device *hdev, u8 report_id) > > { > > - const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54, > > - 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; > > + /* > > + * Asus handshake identifying us as a driver (0x5A) > > + * 0x5A then ASCII for "ASUS Tech.Inc." > > + * 0x5D is for userspace Windows applications. > > + * > > + * The handshake is first sent as a set_report, then retrieved > > + * from a get_report to verify the response. > > + */ > > This entire comment is not required, especially not the last paragraph. > From what I've seen in .dll reversing attempts there's no real > distinction from driver/app and it's simply an init/enable sequence > common to almost every ITE MCU that ASUS have used (slash, anime, Ally). > > Please remove. It is a context comment but can be removed. > > + const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, > > + 0x54, 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; > > + u8 *readbuf; > > int ret; > > > > ret = asus_kbd_set_report(hdev, buf, sizeof(buf)); > > - if (ret < 0) > > - hid_err(hdev, "Asus failed to send init command: %d\n", ret); > > + if (ret < 0) { > > + hid_err(hdev, "Asus failed to send handshake: %d\n", ret); > > + return ret; > > + } > > > > + readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL); > > + if (!readbuf) > > + return -ENOMEM; > > + > > + ret = hid_hw_raw_request(hdev, report_id, readbuf, > > + FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT, > > + HID_REQ_GET_REPORT); > > + if (ret < 0) { > > + hid_err(hdev, "Asus failed to receive handshake ack: %d\n", ret); > > + } else if (memcmp(readbuf, buf, sizeof(buf)) != 0) { > > + hid_err(hdev, "Asus handshake returned invalid response: %*ph\n", > > + FEATURE_KBD_REPORT_SIZE, readbuf); > > + // Do not return error if handshake is wrong to avoid regressions > > + } > > + > > + kfree(readbuf); > > return ret; > > } > > > > @@ -417,7 +444,7 @@ static int asus_kbd_get_functions(struct hid_device *hdev, > > if (!readbuf) > > return -ENOMEM; > > > > - ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf, > > + ret = hid_hw_raw_request(hdev, report_id, readbuf, > > FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT, > > HID_REQ_GET_REPORT); > > if (ret < 0) { > > @@ -540,42 +567,25 @@ static int asus_kbd_register_leds(struct hid_device *hdev) > > unsigned char kbd_func; > > int ret; > > > > - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) { > > - /* Initialize keyboard */ > > - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); > > - if (ret < 0) > > - return ret; > > - > > - /* The LED endpoint is initialised in two HID */ > > - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1); > > - if (ret < 0) > > - return ret; > > - > > - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2); > > - if (ret < 0) > > - return ret; > > + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); > > + if (ret < 0) > > + return ret; > > > > I don't have any non-ROG equipment to test. There have been some cases > of Vivobook using the same MCU, but these otherwise used something else. > And the oldest hardware I have uses a 0x1866, which uses the same init > sequence but works with both 0x5A and 0x5D report ID for init (on same > endpoint). There are instances of other laptops that accept both 0x5A > and 0x5D, and some that have only 0x5D. The driver sets the brightness using 0x5a and accepts keyboard shortcuts only from 0x5a. Therefore it needs to init only with 0x5a. How would those laptops regress and in what way? > I think you will need to change this to try both 0x5A and 0x5D sorry. > The older models using 0x1854, 0x1869, 0x1866 PID may regress and > although I'm reasonably confident there won't be issues due to age of > those, it's not a risk I'm willing to take, I've spent all morning > trawling through archives of info and I can't actually verify this other > than from my memory. For devices that support RGB, only when RGB is set, 0x5D is initialized too. > I mentioned 0x5E being used for some of the oddball devices like slash > and anime, don't worry about that one, it's a bridge that can be crossed > at a later time. But it does illustrate that other report ID have been > used for init. > > Otherwise the cleanup is good. > > No other comments required and I'll sign off with the above corrections. > > Cheers, > Luke > > > - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { > > - ret = asus_kbd_disable_oobe(hdev); > > - if (ret < 0) > > - return ret; > > - } > > - } else { > > - /* Initialize keyboard */ > > - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); > > - if (ret < 0) > > - return ret; > > + /* Get keyboard functions */ > > + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID); > > + if (ret < 0) > > + return ret; > > > > - /* Get keyboard functions */ > > - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID); > > + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { > > + ret = asus_kbd_disable_oobe(hdev); > > if (ret < 0) > > return ret; > > - > > - /* Check for backlight support */ > > - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT)) > > - return -ENODEV; > > } > > > > + /* Check for backlight support */ > > + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT)) > > + return -ENODEV; > > + > > drvdata->kbd_backlight = devm_kzalloc(&hdev->dev, > > sizeof(struct asus_kbd_leds), > > GFP_KERNEL); >
On 23/03/25 12:05, Antheas Kapenekakis wrote: > On Sat, 22 Mar 2025 at 23:01, Luke D. Jones <luke@ljones.dev> wrote: >> >> On 22/03/25 23:27, Antheas Kapenekakis wrote: >>> Currently, asus_kbd_init() uses a reverse engineered init sequence >>> from Windows, which contains the handshakes from multiple programs. >>> Keep the main one, which is 0x5a (meant for brightness drivers). >>> >>> In addition, perform a get_response and check if the response is the >>> same. To avoid regressions, print an error if the response does not >>> match instead of rejecting device. >>> >>> Then, refactor asus_kbd_get_functions() to use the same ID it is called >>> with, instead of hardcoding it to 0x5a so that it may be used for 0x0d >>> in the future. >>> >>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> >>> --- >>> drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++------------------- >>> 1 file changed, 46 insertions(+), 36 deletions(-) >>> >>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c >>> index 46e3e42f9eb5f..8d4df1b6f143b 100644 >>> --- a/drivers/hid/hid-asus.c >>> +++ b/drivers/hid/hid-asus.c >>> @@ -48,7 +48,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad"); >>> #define FEATURE_REPORT_ID 0x0d >>> #define INPUT_REPORT_ID 0x5d >>> #define FEATURE_KBD_REPORT_ID 0x5a >>> -#define FEATURE_KBD_REPORT_SIZE 16 >>> +#define FEATURE_KBD_REPORT_SIZE 64 >>> #define FEATURE_KBD_LED_REPORT_ID1 0x5d >>> #define FEATURE_KBD_LED_REPORT_ID2 0x5e >>> >>> @@ -388,14 +388,41 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu >>> >>> static int asus_kbd_init(struct hid_device *hdev, u8 report_id) >>> { >>> - const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54, >>> - 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; >>> + /* >>> + * Asus handshake identifying us as a driver (0x5A) >>> + * 0x5A then ASCII for "ASUS Tech.Inc." >>> + * 0x5D is for userspace Windows applications. >>> + * >>> + * The handshake is first sent as a set_report, then retrieved >>> + * from a get_report to verify the response. >>> + */ >> >> This entire comment is not required, especially not the last paragraph. >> From what I've seen in .dll reversing attempts there's no real >> distinction from driver/app and it's simply an init/enable sequence >> common to almost every ITE MCU that ASUS have used (slash, anime, Ally). >> >> Please remove. > > It is a context comment but can be removed. > >>> + const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, >>> + 0x54, 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; >>> + u8 *readbuf; >>> int ret; >>> >>> ret = asus_kbd_set_report(hdev, buf, sizeof(buf)); >>> - if (ret < 0) >>> - hid_err(hdev, "Asus failed to send init command: %d\n", ret); >>> + if (ret < 0) { >>> + hid_err(hdev, "Asus failed to send handshake: %d\n", ret); >>> + return ret; >>> + } >>> >>> + readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL); >>> + if (!readbuf) >>> + return -ENOMEM; >>> + >>> + ret = hid_hw_raw_request(hdev, report_id, readbuf, >>> + FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT, >>> + HID_REQ_GET_REPORT); >>> + if (ret < 0) { >>> + hid_err(hdev, "Asus failed to receive handshake ack: %d\n", ret); >>> + } else if (memcmp(readbuf, buf, sizeof(buf)) != 0) { >>> + hid_err(hdev, "Asus handshake returned invalid response: %*ph\n", >>> + FEATURE_KBD_REPORT_SIZE, readbuf); >>> + // Do not return error if handshake is wrong to avoid regressions >>> + } >>> + >>> + kfree(readbuf); >>> return ret; >>> } >>> >>> @@ -417,7 +444,7 @@ static int asus_kbd_get_functions(struct hid_device *hdev, >>> if (!readbuf) >>> return -ENOMEM; >>> >>> - ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf, >>> + ret = hid_hw_raw_request(hdev, report_id, readbuf, >>> FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT, >>> HID_REQ_GET_REPORT); >>> if (ret < 0) { >>> @@ -540,42 +567,25 @@ static int asus_kbd_register_leds(struct hid_device *hdev) >>> unsigned char kbd_func; >>> int ret; >>> >>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) { >>> - /* Initialize keyboard */ >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); >>> - if (ret < 0) >>> - return ret; >>> - >>> - /* The LED endpoint is initialised in two HID */ >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1); >>> - if (ret < 0) >>> - return ret; >>> - >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2); >>> - if (ret < 0) >>> - return ret; >>> + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); >>> + if (ret < 0) >>> + return ret; >>> >> >> I don't have any non-ROG equipment to test. There have been some cases >> of Vivobook using the same MCU, but these otherwise used something else. >> And the oldest hardware I have uses a 0x1866, which uses the same init >> sequence but works with both 0x5A and 0x5D report ID for init (on same >> endpoint). There are instances of other laptops that accept both 0x5A >> and 0x5D, and some that have only 0x5D. > > The driver sets the brightness using 0x5a and accepts keyboard > shortcuts only from 0x5a. Therefore it needs to init only with 0x5a. > > How would those laptops regress and in what way? > The media keys fail to work (vol, mic, rog). Can you please accept that I do know some laptops used only 0x5D, and these are older models, around 5+ years. The only thing I have to go on is my memory unfortunately, and I've been trying to find the concrete examples. >> I think you will need to change this to try both 0x5A and 0x5D sorry. >> The older models using 0x1854, 0x1869, 0x1866 PID may regress and >> although I'm reasonably confident there won't be issues due to age of >> those, it's not a risk I'm willing to take, I've spent all morning >> trawling through archives of info and I can't actually verify this other >> than from my memory. > > For devices that support RGB, only when RGB is set, 0x5D is initialized too. Sure. But as I've said above.. Please add both to init. It's only done once, and it doesn't hurt anything plus doesn't risk regressing older hardware. If I can get the proper evidence that only 0x5A is required I'm happy to use only that, but until then I don't want that risk. And it's only a small thing here. Cheers, Luke. > >> I mentioned 0x5E being used for some of the oddball devices like slash >> and anime, don't worry about that one, it's a bridge that can be crossed >> at a later time. But it does illustrate that other report ID have been >> used for init. >> >> Otherwise the cleanup is good. >> >> No other comments required and I'll sign off with the above corrections. >> >> Cheers, >> Luke >> >>> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { >>> - ret = asus_kbd_disable_oobe(hdev); >>> - if (ret < 0) >>> - return ret; >>> - } >>> - } else { >>> - /* Initialize keyboard */ >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); >>> - if (ret < 0) >>> - return ret; >>> + /* Get keyboard functions */ >>> + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID); >>> + if (ret < 0) >>> + return ret; >>> >>> - /* Get keyboard functions */ >>> - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID); >>> + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { >>> + ret = asus_kbd_disable_oobe(hdev); >>> if (ret < 0) >>> return ret; >>> - >>> - /* Check for backlight support */ >>> - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT)) >>> - return -ENODEV; >>> } >>> >>> + /* Check for backlight support */ >>> + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT)) >>> + return -ENODEV; >>> + >>> drvdata->kbd_backlight = devm_kzalloc(&hdev->dev, >>> sizeof(struct asus_kbd_leds), >>> GFP_KERNEL); >>
On 22/03/25 23:27, Antheas Kapenekakis wrote: > The Asus Z13 folio has a multitouch touchpad that needs to bind > to the hid-multitouch driver in order to work properly. So bind > it to the HID_GROUP_GENERIC group to release the touchpad and > move it to the bottom so that the comment applies to it. > > While at it, change the generic KEYBOARD3 name to Z13_FOLIO. > > Reviewed-by: Luke D. Jones <luke@ljones.dev> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > --- Never been clear on tag order but I've always put authors at top. Just something I noticed as it's different to what i do. > drivers/hid/hid-asus.c | 6 +++--- > drivers/hid/hid-ids.h | 2 +- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c > index 96461321c191c..e97fb76eda619 100644 > --- a/drivers/hid/hid-asus.c > +++ b/drivers/hid/hid-asus.c > @@ -1319,9 +1319,6 @@ static const struct hid_device_id asus_devices[] = { > { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK, > USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2), > QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD }, > - { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK, > - USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD3), > - 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 }, > @@ -1351,6 +1348,9 @@ static const struct hid_device_id asus_devices[] = { > * Note bind to the HID_GROUP_GENERIC group, so that we only bind to the keyboard > * part, while letting hid-multitouch.c handle the touchpad. > */ > + { 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 }, > { HID_DEVICE(BUS_USB, HID_GROUP_GENERIC, > USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_T101HA_KEYBOARD) }, > { } > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > index 7e400624908e3..b1fe7582324ff 100644 > --- a/drivers/hid/hid-ids.h > +++ b/drivers/hid/hid-ids.h > @@ -209,7 +209,7 @@ > #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD3 0x1822 > #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD 0x1866 > #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2 0x19b6 > -#define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD3 0x1a30 > +#define USB_DEVICE_ID_ASUSTEK_ROG_Z13_FOLIO 0x1a30 > #define USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR 0x18c6 > #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY 0x1abe > #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X 0x1b4c
On Sun, 23 Mar 2025 at 00:31, Luke D. Jones <luke@ljones.dev> wrote: > > On 22/03/25 23:27, Antheas Kapenekakis wrote: > > The Asus Z13 folio has a multitouch touchpad that needs to bind > > to the hid-multitouch driver in order to work properly. So bind > > it to the HID_GROUP_GENERIC group to release the touchpad and > > move it to the bottom so that the comment applies to it. > > > > While at it, change the generic KEYBOARD3 name to Z13_FOLIO. > > > > Reviewed-by: Luke D. Jones <luke@ljones.dev> > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > > --- > > Never been clear on tag order but I've always put authors at top. Just > something I noticed as it's different to what i do. Tag was added by b4 Antheas > > drivers/hid/hid-asus.c | 6 +++--- > > drivers/hid/hid-ids.h | 2 +- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c > > index 96461321c191c..e97fb76eda619 100644 > > --- a/drivers/hid/hid-asus.c > > +++ b/drivers/hid/hid-asus.c > > @@ -1319,9 +1319,6 @@ static const struct hid_device_id asus_devices[] = { > > { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK, > > USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2), > > QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD }, > > - { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK, > > - USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD3), > > - 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 }, > > @@ -1351,6 +1348,9 @@ static const struct hid_device_id asus_devices[] = { > > * Note bind to the HID_GROUP_GENERIC group, so that we only bind to the keyboard > > * part, while letting hid-multitouch.c handle the touchpad. > > */ > > + { 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 }, > > { HID_DEVICE(BUS_USB, HID_GROUP_GENERIC, > > USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_T101HA_KEYBOARD) }, > > { } > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > > index 7e400624908e3..b1fe7582324ff 100644 > > --- a/drivers/hid/hid-ids.h > > +++ b/drivers/hid/hid-ids.h > > @@ -209,7 +209,7 @@ > > #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD3 0x1822 > > #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD 0x1866 > > #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2 0x19b6 > > -#define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD3 0x1a30 > > +#define USB_DEVICE_ID_ASUSTEK_ROG_Z13_FOLIO 0x1a30 > > #define USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR 0x18c6 > > #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY 0x1abe > > #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X 0x1b4c >
On Sun, 23 Mar 2025 at 00:29, Luke D. Jones <luke@ljones.dev> wrote: > > On 23/03/25 12:05, Antheas Kapenekakis wrote: > > On Sat, 22 Mar 2025 at 23:01, Luke D. Jones <luke@ljones.dev> wrote: > >> > >> On 22/03/25 23:27, Antheas Kapenekakis wrote: > >>> Currently, asus_kbd_init() uses a reverse engineered init sequence > >>> from Windows, which contains the handshakes from multiple programs. > >>> Keep the main one, which is 0x5a (meant for brightness drivers). > >>> > >>> In addition, perform a get_response and check if the response is the > >>> same. To avoid regressions, print an error if the response does not > >>> match instead of rejecting device. > >>> > >>> Then, refactor asus_kbd_get_functions() to use the same ID it is called > >>> with, instead of hardcoding it to 0x5a so that it may be used for 0x0d > >>> in the future. > >>> > >>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > >>> --- > >>> drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++------------------- > >>> 1 file changed, 46 insertions(+), 36 deletions(-) > >>> > >>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c > >>> index 46e3e42f9eb5f..8d4df1b6f143b 100644 > >>> --- a/drivers/hid/hid-asus.c > >>> +++ b/drivers/hid/hid-asus.c > >>> @@ -48,7 +48,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad"); > >>> #define FEATURE_REPORT_ID 0x0d > >>> #define INPUT_REPORT_ID 0x5d > >>> #define FEATURE_KBD_REPORT_ID 0x5a > >>> -#define FEATURE_KBD_REPORT_SIZE 16 > >>> +#define FEATURE_KBD_REPORT_SIZE 64 > >>> #define FEATURE_KBD_LED_REPORT_ID1 0x5d > >>> #define FEATURE_KBD_LED_REPORT_ID2 0x5e > >>> > >>> @@ -388,14 +388,41 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu > >>> > >>> static int asus_kbd_init(struct hid_device *hdev, u8 report_id) > >>> { > >>> - const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54, > >>> - 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; > >>> + /* > >>> + * Asus handshake identifying us as a driver (0x5A) > >>> + * 0x5A then ASCII for "ASUS Tech.Inc." > >>> + * 0x5D is for userspace Windows applications. > >>> + * > >>> + * The handshake is first sent as a set_report, then retrieved > >>> + * from a get_report to verify the response. > >>> + */ > >> > >> This entire comment is not required, especially not the last paragraph. > >> From what I've seen in .dll reversing attempts there's no real > >> distinction from driver/app and it's simply an init/enable sequence > >> common to almost every ITE MCU that ASUS have used (slash, anime, Ally). > >> > >> Please remove. > > > > It is a context comment but can be removed. > > > >>> + const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, > >>> + 0x54, 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; > >>> + u8 *readbuf; > >>> int ret; > >>> > >>> ret = asus_kbd_set_report(hdev, buf, sizeof(buf)); > >>> - if (ret < 0) > >>> - hid_err(hdev, "Asus failed to send init command: %d\n", ret); > >>> + if (ret < 0) { > >>> + hid_err(hdev, "Asus failed to send handshake: %d\n", ret); > >>> + return ret; > >>> + } > >>> > >>> + readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL); > >>> + if (!readbuf) > >>> + return -ENOMEM; > >>> + > >>> + ret = hid_hw_raw_request(hdev, report_id, readbuf, > >>> + FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT, > >>> + HID_REQ_GET_REPORT); > >>> + if (ret < 0) { > >>> + hid_err(hdev, "Asus failed to receive handshake ack: %d\n", ret); > >>> + } else if (memcmp(readbuf, buf, sizeof(buf)) != 0) { > >>> + hid_err(hdev, "Asus handshake returned invalid response: %*ph\n", > >>> + FEATURE_KBD_REPORT_SIZE, readbuf); > >>> + // Do not return error if handshake is wrong to avoid regressions > >>> + } > >>> + > >>> + kfree(readbuf); > >>> return ret; > >>> } > >>> > >>> @@ -417,7 +444,7 @@ static int asus_kbd_get_functions(struct hid_device *hdev, > >>> if (!readbuf) > >>> return -ENOMEM; > >>> > >>> - ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf, > >>> + ret = hid_hw_raw_request(hdev, report_id, readbuf, > >>> FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT, > >>> HID_REQ_GET_REPORT); > >>> if (ret < 0) { > >>> @@ -540,42 +567,25 @@ static int asus_kbd_register_leds(struct hid_device *hdev) > >>> unsigned char kbd_func; > >>> int ret; > >>> > >>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) { > >>> - /* Initialize keyboard */ > >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); > >>> - if (ret < 0) > >>> - return ret; > >>> - > >>> - /* The LED endpoint is initialised in two HID */ > >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1); > >>> - if (ret < 0) > >>> - return ret; > >>> - > >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2); > >>> - if (ret < 0) > >>> - return ret; > >>> + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); > >>> + if (ret < 0) > >>> + return ret; > >>> > >> > >> I don't have any non-ROG equipment to test. There have been some cases > >> of Vivobook using the same MCU, but these otherwise used something else. > >> And the oldest hardware I have uses a 0x1866, which uses the same init > >> sequence but works with both 0x5A and 0x5D report ID for init (on same > >> endpoint). There are instances of other laptops that accept both 0x5A > >> and 0x5D, and some that have only 0x5D. > > > > The driver sets the brightness using 0x5a and accepts keyboard > > shortcuts only from 0x5a. Therefore it needs to init only with 0x5a. > > > > How would those laptops regress and in what way? > > > > The media keys fail to work (vol, mic, rog). Can you please accept that > I do know some laptops used only 0x5D, and these are older models, > around 5+ years. The only thing I have to go on is my memory > unfortunately, and I've been trying to find the concrete examples. I just looked at the history. Yeah it seems you added ID1 in 2020 with some other commands. But on the same commit you blocked 0x5d and 0x5e, so it means those keyboards use 0x5a to send keyboard events. Nevertheless, it is not worth looking up or risking regressions for old hardware. I will readd 0x5d, 0x5e for USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD and USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2, since those are the ones you added the inits with. But I will still keep them off for the Z13 and Ally. By the way, Antheas > >> I think you will need to change this to try both 0x5A and 0x5D sorry. > >> The older models using 0x1854, 0x1869, 0x1866 PID may regress and > >> although I'm reasonably confident there won't be issues due to age of > >> those, it's not a risk I'm willing to take, I've spent all morning > >> trawling through archives of info and I can't actually verify this other > >> than from my memory. > > > > For devices that support RGB, only when RGB is set, 0x5D is initialized too. > > Sure. But as I've said above.. Please add both to init. It's only done > once, and it doesn't hurt anything plus doesn't risk regressing older > hardware. > > If I can get the proper evidence that only 0x5A is required I'm happy to > use only that, but until then I don't want that risk. And it's only a > small thing here. > > Cheers, > Luke. > > > > >> I mentioned 0x5E being used for some of the oddball devices like slash > >> and anime, don't worry about that one, it's a bridge that can be crossed > >> at a later time. But it does illustrate that other report ID have been > >> used for init. > >> > >> Otherwise the cleanup is good. > >> > >> No other comments required and I'll sign off with the above corrections. > >> > >> Cheers, > >> Luke > >> > >>> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { > >>> - ret = asus_kbd_disable_oobe(hdev); > >>> - if (ret < 0) > >>> - return ret; > >>> - } > >>> - } else { > >>> - /* Initialize keyboard */ > >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); > >>> - if (ret < 0) > >>> - return ret; > >>> + /* Get keyboard functions */ > >>> + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID); > >>> + if (ret < 0) > >>> + return ret; > >>> > >>> - /* Get keyboard functions */ > >>> - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID); > >>> + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { > >>> + ret = asus_kbd_disable_oobe(hdev); > >>> if (ret < 0) > >>> return ret; > >>> - > >>> - /* Check for backlight support */ > >>> - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT)) > >>> - return -ENODEV; > >>> } > >>> > >>> + /* Check for backlight support */ > >>> + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT)) > >>> + return -ENODEV; > >>> + > >>> drvdata->kbd_backlight = devm_kzalloc(&hdev->dev, > >>> sizeof(struct asus_kbd_leds), > >>> GFP_KERNEL); > >> >
On Sun, 23 Mar 2025 at 00:53, Antheas Kapenekakis <lkml@antheas.dev> wrote: > > On Sun, 23 Mar 2025 at 00:29, Luke D. Jones <luke@ljones.dev> wrote: > > > > On 23/03/25 12:05, Antheas Kapenekakis wrote: > > > On Sat, 22 Mar 2025 at 23:01, Luke D. Jones <luke@ljones.dev> wrote: > > >> > > >> On 22/03/25 23:27, Antheas Kapenekakis wrote: > > >>> Currently, asus_kbd_init() uses a reverse engineered init sequence > > >>> from Windows, which contains the handshakes from multiple programs. > > >>> Keep the main one, which is 0x5a (meant for brightness drivers). > > >>> > > >>> In addition, perform a get_response and check if the response is the > > >>> same. To avoid regressions, print an error if the response does not > > >>> match instead of rejecting device. > > >>> > > >>> Then, refactor asus_kbd_get_functions() to use the same ID it is called > > >>> with, instead of hardcoding it to 0x5a so that it may be used for 0x0d > > >>> in the future. > > >>> > > >>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > > >>> --- > > >>> drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++------------------- > > >>> 1 file changed, 46 insertions(+), 36 deletions(-) > > >>> > > >>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c > > >>> index 46e3e42f9eb5f..8d4df1b6f143b 100644 > > >>> --- a/drivers/hid/hid-asus.c > > >>> +++ b/drivers/hid/hid-asus.c > > >>> @@ -48,7 +48,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad"); > > >>> #define FEATURE_REPORT_ID 0x0d > > >>> #define INPUT_REPORT_ID 0x5d > > >>> #define FEATURE_KBD_REPORT_ID 0x5a > > >>> -#define FEATURE_KBD_REPORT_SIZE 16 > > >>> +#define FEATURE_KBD_REPORT_SIZE 64 > > >>> #define FEATURE_KBD_LED_REPORT_ID1 0x5d > > >>> #define FEATURE_KBD_LED_REPORT_ID2 0x5e > > >>> > > >>> @@ -388,14 +388,41 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu > > >>> > > >>> static int asus_kbd_init(struct hid_device *hdev, u8 report_id) > > >>> { > > >>> - const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54, > > >>> - 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; > > >>> + /* > > >>> + * Asus handshake identifying us as a driver (0x5A) > > >>> + * 0x5A then ASCII for "ASUS Tech.Inc." > > >>> + * 0x5D is for userspace Windows applications. > > >>> + * > > >>> + * The handshake is first sent as a set_report, then retrieved > > >>> + * from a get_report to verify the response. > > >>> + */ > > >> > > >> This entire comment is not required, especially not the last paragraph. > > >> From what I've seen in .dll reversing attempts there's no real > > >> distinction from driver/app and it's simply an init/enable sequence > > >> common to almost every ITE MCU that ASUS have used (slash, anime, Ally). > > >> > > >> Please remove. > > > > > > It is a context comment but can be removed. > > > > > >>> + const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, > > >>> + 0x54, 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; > > >>> + u8 *readbuf; > > >>> int ret; > > >>> > > >>> ret = asus_kbd_set_report(hdev, buf, sizeof(buf)); > > >>> - if (ret < 0) > > >>> - hid_err(hdev, "Asus failed to send init command: %d\n", ret); > > >>> + if (ret < 0) { > > >>> + hid_err(hdev, "Asus failed to send handshake: %d\n", ret); > > >>> + return ret; > > >>> + } > > >>> > > >>> + readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL); > > >>> + if (!readbuf) > > >>> + return -ENOMEM; > > >>> + > > >>> + ret = hid_hw_raw_request(hdev, report_id, readbuf, > > >>> + FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT, > > >>> + HID_REQ_GET_REPORT); > > >>> + if (ret < 0) { > > >>> + hid_err(hdev, "Asus failed to receive handshake ack: %d\n", ret); > > >>> + } else if (memcmp(readbuf, buf, sizeof(buf)) != 0) { > > >>> + hid_err(hdev, "Asus handshake returned invalid response: %*ph\n", > > >>> + FEATURE_KBD_REPORT_SIZE, readbuf); > > >>> + // Do not return error if handshake is wrong to avoid regressions > > >>> + } > > >>> + > > >>> + kfree(readbuf); > > >>> return ret; > > >>> } > > >>> > > >>> @@ -417,7 +444,7 @@ static int asus_kbd_get_functions(struct hid_device *hdev, > > >>> if (!readbuf) > > >>> return -ENOMEM; > > >>> > > >>> - ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf, > > >>> + ret = hid_hw_raw_request(hdev, report_id, readbuf, > > >>> FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT, > > >>> HID_REQ_GET_REPORT); > > >>> if (ret < 0) { > > >>> @@ -540,42 +567,25 @@ static int asus_kbd_register_leds(struct hid_device *hdev) > > >>> unsigned char kbd_func; > > >>> int ret; > > >>> > > >>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) { > > >>> - /* Initialize keyboard */ > > >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); > > >>> - if (ret < 0) > > >>> - return ret; > > >>> - > > >>> - /* The LED endpoint is initialised in two HID */ > > >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1); > > >>> - if (ret < 0) > > >>> - return ret; > > >>> - > > >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2); > > >>> - if (ret < 0) > > >>> - return ret; > > >>> + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); > > >>> + if (ret < 0) > > >>> + return ret; > > >>> > > >> > > >> I don't have any non-ROG equipment to test. There have been some cases > > >> of Vivobook using the same MCU, but these otherwise used something else. > > >> And the oldest hardware I have uses a 0x1866, which uses the same init > > >> sequence but works with both 0x5A and 0x5D report ID for init (on same > > >> endpoint). There are instances of other laptops that accept both 0x5A > > >> and 0x5D, and some that have only 0x5D. > > > > > > The driver sets the brightness using 0x5a and accepts keyboard > > > shortcuts only from 0x5a. Therefore it needs to init only with 0x5a. > > > > > > How would those laptops regress and in what way? > > > > > > > The media keys fail to work (vol, mic, rog). Can you please accept that > > I do know some laptops used only 0x5D, and these are older models, > > around 5+ years. The only thing I have to go on is my memory > > unfortunately, and I've been trying to find the concrete examples. > > I just looked at the history. Yeah it seems you added ID1 in 2020 with > some other commands. But on the same commit you blocked 0x5d and 0x5e, > so it means those keyboards use 0x5a to send keyboard events. > > Nevertheless, it is not worth looking up or risking regressions for > old hardware. I will readd 0x5d, 0x5e for > USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD and > USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2, since those are the ones you > added the inits with. > > But I will still keep them off for the Z13 and Ally. > > By the way, I guess I did not finish this. I was going to express some concern about the Claymore keyboard. But it seems it does not have a backlight quirk so it is ok > Antheas > > > >> I think you will need to change this to try both 0x5A and 0x5D sorry. > > >> The older models using 0x1854, 0x1869, 0x1866 PID may regress and > > >> although I'm reasonably confident there won't be issues due to age of > > >> those, it's not a risk I'm willing to take, I've spent all morning > > >> trawling through archives of info and I can't actually verify this other > > >> than from my memory. > > > > > > For devices that support RGB, only when RGB is set, 0x5D is initialized too. > > > > Sure. But as I've said above.. Please add both to init. It's only done > > once, and it doesn't hurt anything plus doesn't risk regressing older > > hardware. > > > > If I can get the proper evidence that only 0x5A is required I'm happy to > > use only that, but until then I don't want that risk. And it's only a > > small thing here. > > > > Cheers, > > Luke. > > > > > > > >> I mentioned 0x5E being used for some of the oddball devices like slash > > >> and anime, don't worry about that one, it's a bridge that can be crossed > > >> at a later time. But it does illustrate that other report ID have been > > >> used for init. > > >> > > >> Otherwise the cleanup is good. > > >> > > >> No other comments required and I'll sign off with the above corrections. > > >> > > >> Cheers, > > >> Luke > > >> > > >>> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { > > >>> - ret = asus_kbd_disable_oobe(hdev); > > >>> - if (ret < 0) > > >>> - return ret; > > >>> - } > > >>> - } else { > > >>> - /* Initialize keyboard */ > > >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); > > >>> - if (ret < 0) > > >>> - return ret; > > >>> + /* Get keyboard functions */ > > >>> + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID); > > >>> + if (ret < 0) > > >>> + return ret; > > >>> > > >>> - /* Get keyboard functions */ > > >>> - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID); > > >>> + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { > > >>> + ret = asus_kbd_disable_oobe(hdev); > > >>> if (ret < 0) > > >>> return ret; > > >>> - > > >>> - /* Check for backlight support */ > > >>> - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT)) > > >>> - return -ENODEV; > > >>> } > > >>> > > >>> + /* Check for backlight support */ > > >>> + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT)) > > >>> + return -ENODEV; > > >>> + > > >>> drvdata->kbd_backlight = devm_kzalloc(&hdev->dev, > > >>> sizeof(struct asus_kbd_leds), > > >>> GFP_KERNEL); > > >> > >
On 22/03/25 23:28, Antheas Kapenekakis wrote: > Currenlty, the keyboard brightness control of Asus WMI keyboards is > handled in the kernel, which leads to the shortcut going from > brightness 0, to 1, to 2, and 3. > > However, for HID keyboards it is exposed as a key and handled by the > user's desktop environment. For the toggle button, this means that > brightness control becomes on/off. In addition, in the absence of a > DE, the keyboard brightness does not work. > > Therefore, expose an event handler for the keyboard brightness control > which can then be used by hid-asus. > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > --- > drivers/platform/x86/asus-wmi.c | 39 ++++++++++++++++++++++ > include/linux/platform_data/x86/asus-wmi.h | 11 ++++++ > 2 files changed, 50 insertions(+) > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c > index 95ef9b1d321bb..5ebe141294ecf 100644 > --- a/drivers/platform/x86/asus-wmi.c > +++ b/drivers/platform/x86/asus-wmi.c > @@ -1536,6 +1536,45 @@ void asus_hid_unregister_listener(struct asus_hid_listener *bdev) > } > EXPORT_SYMBOL_GPL(asus_hid_unregister_listener); > > +static void do_kbd_led_set(struct led_classdev *led_cdev, int value); > + > +int asus_hid_event(enum asus_hid_event event) > +{ > + unsigned long flags; > + int brightness; > + > + spin_lock_irqsave(&asus_ref.lock, flags); > + if (!asus_ref.asus || !asus_ref.asus->kbd_led_registered) { > + spin_unlock_irqrestore(&asus_ref.lock, flags); > + return -EBUSY; > + } > + brightness = asus_ref.asus->kbd_led_wk; > + > + switch (event) { > + case ASUS_EV_BRTUP: > + brightness += 1; > + break; > + case ASUS_EV_BRTDOWN: > + brightness -= 1; > + break; > + case ASUS_EV_BRTTOGGLE: > + if (brightness >= 3) > + brightness = 0; > + else > + brightness += 1; > + break; > + } > + > + do_kbd_led_set(&asus_ref.asus->kbd_led, brightness); > + led_classdev_notify_brightness_hw_changed(&asus_ref.asus->kbd_led, > + asus_ref.asus->kbd_led_wk); > + > + spin_unlock_irqrestore(&asus_ref.lock, flags); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(asus_hid_event); > + > /* > * These functions actually update the LED's, and are called from a > * workqueue. By doing this as separate work rather than when the LED > diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h > index c513b5a732323..9adbe8abef090 100644 > --- a/include/linux/platform_data/x86/asus-wmi.h > +++ b/include/linux/platform_data/x86/asus-wmi.h > @@ -162,11 +162,18 @@ struct asus_hid_listener { > void (*brightness_set)(struct asus_hid_listener *listener, int brightness); > }; > > +enum asus_hid_event { > + ASUS_EV_BRTUP, > + ASUS_EV_BRTDOWN, > + ASUS_EV_BRTTOGGLE, > +}; > + > #if IS_REACHABLE(CONFIG_ASUS_WMI) > int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval); > > int asus_hid_register_listener(struct asus_hid_listener *cdev); > void asus_hid_unregister_listener(struct asus_hid_listener *cdev); > +int asus_hid_event(enum asus_hid_event event); > #else > static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, > u32 *retval) > @@ -181,6 +188,10 @@ static inline int asus_hid_register_listener(struct asus_hid_listener *bdev) > static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev) > { > } > +static inline int asus_hid_event(enum asus_hid_event event) > +{ > + return -ENODEV; > +} > #endif > > #endif /* __PLATFORM_DATA_X86_ASUS_WMI_H */ Reviewed-by: Luke D. Jones <luke@ljones.dev> Tested-by: Luke D. Jones <luke@ljones.dev>
On 23/03/25 12:54, Antheas Kapenekakis wrote: > On Sun, 23 Mar 2025 at 00:53, Antheas Kapenekakis <lkml@antheas.dev> wrote: >> >> On Sun, 23 Mar 2025 at 00:29, Luke D. Jones <luke@ljones.dev> wrote: >>> >>> On 23/03/25 12:05, Antheas Kapenekakis wrote: >>>> On Sat, 22 Mar 2025 at 23:01, Luke D. Jones <luke@ljones.dev> wrote: >>>>> >>>>> On 22/03/25 23:27, Antheas Kapenekakis wrote: >>>>>> Currently, asus_kbd_init() uses a reverse engineered init sequence >>>>>> from Windows, which contains the handshakes from multiple programs. >>>>>> Keep the main one, which is 0x5a (meant for brightness drivers). >>>>>> >>>>>> In addition, perform a get_response and check if the response is the >>>>>> same. To avoid regressions, print an error if the response does not >>>>>> match instead of rejecting device. >>>>>> >>>>>> Then, refactor asus_kbd_get_functions() to use the same ID it is called >>>>>> with, instead of hardcoding it to 0x5a so that it may be used for 0x0d >>>>>> in the future. >>>>>> >>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> >>>>>> --- >>>>>> drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++------------------- >>>>>> 1 file changed, 46 insertions(+), 36 deletions(-) >>>>>> >>>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c >>>>>> index 46e3e42f9eb5f..8d4df1b6f143b 100644 >>>>>> --- a/drivers/hid/hid-asus.c >>>>>> +++ b/drivers/hid/hid-asus.c >>>>>> @@ -48,7 +48,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad"); >>>>>> #define FEATURE_REPORT_ID 0x0d >>>>>> #define INPUT_REPORT_ID 0x5d >>>>>> #define FEATURE_KBD_REPORT_ID 0x5a >>>>>> -#define FEATURE_KBD_REPORT_SIZE 16 >>>>>> +#define FEATURE_KBD_REPORT_SIZE 64 >>>>>> #define FEATURE_KBD_LED_REPORT_ID1 0x5d >>>>>> #define FEATURE_KBD_LED_REPORT_ID2 0x5e >>>>>> >>>>>> @@ -388,14 +388,41 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu >>>>>> >>>>>> static int asus_kbd_init(struct hid_device *hdev, u8 report_id) >>>>>> { >>>>>> - const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54, >>>>>> - 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; >>>>>> + /* >>>>>> + * Asus handshake identifying us as a driver (0x5A) >>>>>> + * 0x5A then ASCII for "ASUS Tech.Inc." >>>>>> + * 0x5D is for userspace Windows applications. >>>>>> + * >>>>>> + * The handshake is first sent as a set_report, then retrieved >>>>>> + * from a get_report to verify the response. >>>>>> + */ >>>>> >>>>> This entire comment is not required, especially not the last paragraph. >>>>> From what I've seen in .dll reversing attempts there's no real >>>>> distinction from driver/app and it's simply an init/enable sequence >>>>> common to almost every ITE MCU that ASUS have used (slash, anime, Ally). >>>>> >>>>> Please remove. >>>> >>>> It is a context comment but can be removed. >>>> >>>>>> + const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, >>>>>> + 0x54, 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; >>>>>> + u8 *readbuf; >>>>>> int ret; >>>>>> >>>>>> ret = asus_kbd_set_report(hdev, buf, sizeof(buf)); >>>>>> - if (ret < 0) >>>>>> - hid_err(hdev, "Asus failed to send init command: %d\n", ret); >>>>>> + if (ret < 0) { >>>>>> + hid_err(hdev, "Asus failed to send handshake: %d\n", ret); >>>>>> + return ret; >>>>>> + } >>>>>> >>>>>> + readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL); >>>>>> + if (!readbuf) >>>>>> + return -ENOMEM; >>>>>> + >>>>>> + ret = hid_hw_raw_request(hdev, report_id, readbuf, >>>>>> + FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT, >>>>>> + HID_REQ_GET_REPORT); >>>>>> + if (ret < 0) { >>>>>> + hid_err(hdev, "Asus failed to receive handshake ack: %d\n", ret); >>>>>> + } else if (memcmp(readbuf, buf, sizeof(buf)) != 0) { >>>>>> + hid_err(hdev, "Asus handshake returned invalid response: %*ph\n", >>>>>> + FEATURE_KBD_REPORT_SIZE, readbuf); >>>>>> + // Do not return error if handshake is wrong to avoid regressions >>>>>> + } >>>>>> + >>>>>> + kfree(readbuf); >>>>>> return ret; >>>>>> } >>>>>> >>>>>> @@ -417,7 +444,7 @@ static int asus_kbd_get_functions(struct hid_device *hdev, >>>>>> if (!readbuf) >>>>>> return -ENOMEM; >>>>>> >>>>>> - ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf, >>>>>> + ret = hid_hw_raw_request(hdev, report_id, readbuf, >>>>>> FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT, >>>>>> HID_REQ_GET_REPORT); >>>>>> if (ret < 0) { >>>>>> @@ -540,42 +567,25 @@ static int asus_kbd_register_leds(struct hid_device *hdev) >>>>>> unsigned char kbd_func; >>>>>> int ret; >>>>>> >>>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) { >>>>>> - /* Initialize keyboard */ >>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); >>>>>> - if (ret < 0) >>>>>> - return ret; >>>>>> - >>>>>> - /* The LED endpoint is initialised in two HID */ >>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1); >>>>>> - if (ret < 0) >>>>>> - return ret; >>>>>> - >>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2); >>>>>> - if (ret < 0) >>>>>> - return ret; >>>>>> + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> >>>>> >>>>> I don't have any non-ROG equipment to test. There have been some cases >>>>> of Vivobook using the same MCU, but these otherwise used something else. >>>>> And the oldest hardware I have uses a 0x1866, which uses the same init >>>>> sequence but works with both 0x5A and 0x5D report ID for init (on same >>>>> endpoint). There are instances of other laptops that accept both 0x5A >>>>> and 0x5D, and some that have only 0x5D. >>>> >>>> The driver sets the brightness using 0x5a and accepts keyboard >>>> shortcuts only from 0x5a. Therefore it needs to init only with 0x5a. >>>> >>>> How would those laptops regress and in what way? >>>> >>> >>> The media keys fail to work (vol, mic, rog). Can you please accept that >>> I do know some laptops used only 0x5D, and these are older models, >>> around 5+ years. The only thing I have to go on is my memory >>> unfortunately, and I've been trying to find the concrete examples. >> >> I just looked at the history. Yeah it seems you added ID1 in 2020 with >> some other commands. But on the same commit you blocked 0x5d and 0x5e, >> so it means those keyboards use 0x5a to send keyboard events. >> >> Nevertheless, it is not worth looking up or risking regressions for >> old hardware. I will readd 0x5d, 0x5e for >> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD and >> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2, since those are the ones you >> added the inits with. >> >> But I will still keep them off for the Z13 and Ally. >> >> By the way, > > I guess I did not finish this. I was going to express some concern > about the Claymore keyboard. But it seems it does not have a backlight > quirk so it is ok > The external keyboards are a bit funky yeah. I don't think the Claymore is in very wide use at all either. In any case you're correct. >> Antheas >> >>>>> I think you will need to change this to try both 0x5A and 0x5D sorry. >>>>> The older models using 0x1854, 0x1869, 0x1866 PID may regress and >>>>> although I'm reasonably confident there won't be issues due to age of >>>>> those, it's not a risk I'm willing to take, I've spent all morning >>>>> trawling through archives of info and I can't actually verify this other >>>>> than from my memory. >>>> >>>> For devices that support RGB, only when RGB is set, 0x5D is initialized too. >>> >>> Sure. But as I've said above.. Please add both to init. It's only done >>> once, and it doesn't hurt anything plus doesn't risk regressing older >>> hardware. >>> >>> If I can get the proper evidence that only 0x5A is required I'm happy to >>> use only that, but until then I don't want that risk. And it's only a >>> small thing here. >>> >>> Cheers, >>> Luke. >>> >>>> >>>>> I mentioned 0x5E being used for some of the oddball devices like slash >>>>> and anime, don't worry about that one, it's a bridge that can be crossed >>>>> at a later time. But it does illustrate that other report ID have been >>>>> used for init. >>>>> >>>>> Otherwise the cleanup is good. >>>>> >>>>> No other comments required and I'll sign off with the above corrections. >>>>> >>>>> Cheers, >>>>> Luke >>>>> >>>>>> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { >>>>>> - ret = asus_kbd_disable_oobe(hdev); >>>>>> - if (ret < 0) >>>>>> - return ret; >>>>>> - } >>>>>> - } else { >>>>>> - /* Initialize keyboard */ >>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); >>>>>> - if (ret < 0) >>>>>> - return ret; >>>>>> + /* Get keyboard functions */ >>>>>> + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> >>>>>> - /* Get keyboard functions */ >>>>>> - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID); >>>>>> + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { >>>>>> + ret = asus_kbd_disable_oobe(hdev); >>>>>> if (ret < 0) >>>>>> return ret; >>>>>> - >>>>>> - /* Check for backlight support */ >>>>>> - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT)) >>>>>> - return -ENODEV; >>>>>> } >>>>>> >>>>>> + /* Check for backlight support */ >>>>>> + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT)) >>>>>> + return -ENODEV; >>>>>> + >>>>>> drvdata->kbd_backlight = devm_kzalloc(&hdev->dev, >>>>>> sizeof(struct asus_kbd_leds), >>>>>> GFP_KERNEL); >>>>> >>>
On 23/03/25 12:53, Antheas Kapenekakis wrote: > On Sun, 23 Mar 2025 at 00:29, Luke D. Jones <luke@ljones.dev> wrote: >> >> On 23/03/25 12:05, Antheas Kapenekakis wrote: >>> On Sat, 22 Mar 2025 at 23:01, Luke D. Jones <luke@ljones.dev> wrote: >>>> >>>> On 22/03/25 23:27, Antheas Kapenekakis wrote: >>>>> Currently, asus_kbd_init() uses a reverse engineered init sequence >>>>> from Windows, which contains the handshakes from multiple programs. >>>>> Keep the main one, which is 0x5a (meant for brightness drivers). >>>>> >>>>> In addition, perform a get_response and check if the response is the >>>>> same. To avoid regressions, print an error if the response does not >>>>> match instead of rejecting device. >>>>> >>>>> Then, refactor asus_kbd_get_functions() to use the same ID it is called >>>>> with, instead of hardcoding it to 0x5a so that it may be used for 0x0d >>>>> in the future. >>>>> >>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> >>>>> --- >>>>> drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++------------------- >>>>> 1 file changed, 46 insertions(+), 36 deletions(-) >>>>> >>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c >>>>> index 46e3e42f9eb5f..8d4df1b6f143b 100644 >>>>> --- a/drivers/hid/hid-asus.c >>>>> +++ b/drivers/hid/hid-asus.c >>>>> @@ -48,7 +48,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad"); >>>>> #define FEATURE_REPORT_ID 0x0d >>>>> #define INPUT_REPORT_ID 0x5d >>>>> #define FEATURE_KBD_REPORT_ID 0x5a >>>>> -#define FEATURE_KBD_REPORT_SIZE 16 >>>>> +#define FEATURE_KBD_REPORT_SIZE 64 >>>>> #define FEATURE_KBD_LED_REPORT_ID1 0x5d >>>>> #define FEATURE_KBD_LED_REPORT_ID2 0x5e >>>>> >>>>> @@ -388,14 +388,41 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu >>>>> >>>>> static int asus_kbd_init(struct hid_device *hdev, u8 report_id) >>>>> { >>>>> - const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54, >>>>> - 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; >>>>> + /* >>>>> + * Asus handshake identifying us as a driver (0x5A) >>>>> + * 0x5A then ASCII for "ASUS Tech.Inc." >>>>> + * 0x5D is for userspace Windows applications. >>>>> + * >>>>> + * The handshake is first sent as a set_report, then retrieved >>>>> + * from a get_report to verify the response. >>>>> + */ >>>> >>>> This entire comment is not required, especially not the last paragraph. >>>> From what I've seen in .dll reversing attempts there's no real >>>> distinction from driver/app and it's simply an init/enable sequence >>>> common to almost every ITE MCU that ASUS have used (slash, anime, Ally). >>>> >>>> Please remove. >>> >>> It is a context comment but can be removed. >>> >>>>> + const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, >>>>> + 0x54, 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; >>>>> + u8 *readbuf; >>>>> int ret; >>>>> >>>>> ret = asus_kbd_set_report(hdev, buf, sizeof(buf)); >>>>> - if (ret < 0) >>>>> - hid_err(hdev, "Asus failed to send init command: %d\n", ret); >>>>> + if (ret < 0) { >>>>> + hid_err(hdev, "Asus failed to send handshake: %d\n", ret); >>>>> + return ret; >>>>> + } >>>>> >>>>> + readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL); >>>>> + if (!readbuf) >>>>> + return -ENOMEM; >>>>> + >>>>> + ret = hid_hw_raw_request(hdev, report_id, readbuf, >>>>> + FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT, >>>>> + HID_REQ_GET_REPORT); >>>>> + if (ret < 0) { >>>>> + hid_err(hdev, "Asus failed to receive handshake ack: %d\n", ret); >>>>> + } else if (memcmp(readbuf, buf, sizeof(buf)) != 0) { >>>>> + hid_err(hdev, "Asus handshake returned invalid response: %*ph\n", >>>>> + FEATURE_KBD_REPORT_SIZE, readbuf); >>>>> + // Do not return error if handshake is wrong to avoid regressions >>>>> + } >>>>> + >>>>> + kfree(readbuf); >>>>> return ret; >>>>> } >>>>> >>>>> @@ -417,7 +444,7 @@ static int asus_kbd_get_functions(struct hid_device *hdev, >>>>> if (!readbuf) >>>>> return -ENOMEM; >>>>> >>>>> - ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf, >>>>> + ret = hid_hw_raw_request(hdev, report_id, readbuf, >>>>> FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT, >>>>> HID_REQ_GET_REPORT); >>>>> if (ret < 0) { >>>>> @@ -540,42 +567,25 @@ static int asus_kbd_register_leds(struct hid_device *hdev) >>>>> unsigned char kbd_func; >>>>> int ret; >>>>> >>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) { >>>>> - /* Initialize keyboard */ >>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); >>>>> - if (ret < 0) >>>>> - return ret; >>>>> - >>>>> - /* The LED endpoint is initialised in two HID */ >>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1); >>>>> - if (ret < 0) >>>>> - return ret; >>>>> - >>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2); >>>>> - if (ret < 0) >>>>> - return ret; >>>>> + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>> >>>> I don't have any non-ROG equipment to test. There have been some cases >>>> of Vivobook using the same MCU, but these otherwise used something else. >>>> And the oldest hardware I have uses a 0x1866, which uses the same init >>>> sequence but works with both 0x5A and 0x5D report ID for init (on same >>>> endpoint). There are instances of other laptops that accept both 0x5A >>>> and 0x5D, and some that have only 0x5D. >>> >>> The driver sets the brightness using 0x5a and accepts keyboard >>> shortcuts only from 0x5a. Therefore it needs to init only with 0x5a. >>> >>> How would those laptops regress and in what way? >>> >> >> The media keys fail to work (vol, mic, rog). Can you please accept that >> I do know some laptops used only 0x5D, and these are older models, >> around 5+ years. The only thing I have to go on is my memory >> unfortunately, and I've been trying to find the concrete examples. > > I just looked at the history. Yeah it seems you added ID1 in 2020 with > some other commands. But on the same commit you blocked 0x5d and 0x5e, > so it means those keyboards use 0x5a to send keyboard events. > > Nevertheless, it is not worth looking up or risking regressions for > old hardware. I will readd 0x5d, 0x5e for > USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD and > USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2, since those are the ones you > added the inits with. > Thank you. Please know that I'm not trying to be a pain, I only don't want to take that risk when I know things were working for all before now, and that I know at least some laptops required 0x5D historically. I do know you can drop 0x5E here. I saw we have: #define INPUT_REPORT_ID 0x5d #define FEATURE_KBD_LED_REPORT_ID1 0x5d Maybe unifying to just one of these can be added as a cleanup? Cheers, Luke. > But I will still keep them off for the Z13 and Ally. > > By the way, > > Antheas > >>>> I think you will need to change this to try both 0x5A and 0x5D sorry. >>>> The older models using 0x1854, 0x1869, 0x1866 PID may regress and >>>> although I'm reasonably confident there won't be issues due to age of >>>> those, it's not a risk I'm willing to take, I've spent all morning >>>> trawling through archives of info and I can't actually verify this other >>>> than from my memory. >>> >>> For devices that support RGB, only when RGB is set, 0x5D is initialized too. >> >> Sure. But as I've said above.. Please add both to init. It's only done >> once, and it doesn't hurt anything plus doesn't risk regressing older >> hardware. >> >> If I can get the proper evidence that only 0x5A is required I'm happy to >> use only that, but until then I don't want that risk. And it's only a >> small thing here. >> >> Cheers, >> Luke. >> >>> >>>> I mentioned 0x5E being used for some of the oddball devices like slash >>>> and anime, don't worry about that one, it's a bridge that can be crossed >>>> at a later time. But it does illustrate that other report ID have been >>>> used for init. >>>> >>>> Otherwise the cleanup is good. >>>> >>>> No other comments required and I'll sign off with the above corrections. >>>> >>>> Cheers, >>>> Luke >>>> >>>>> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { >>>>> - ret = asus_kbd_disable_oobe(hdev); >>>>> - if (ret < 0) >>>>> - return ret; >>>>> - } >>>>> - } else { >>>>> - /* Initialize keyboard */ >>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID); >>>>> - if (ret < 0) >>>>> - return ret; >>>>> + /* Get keyboard functions */ >>>>> + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> - /* Get keyboard functions */ >>>>> - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID); >>>>> + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { >>>>> + ret = asus_kbd_disable_oobe(hdev); >>>>> if (ret < 0) >>>>> return ret; >>>>> - >>>>> - /* Check for backlight support */ >>>>> - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT)) >>>>> - return -ENODEV; >>>>> } >>>>> >>>>> + /* Check for backlight support */ >>>>> + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT)) >>>>> + return -ENODEV; >>>>> + >>>>> drvdata->kbd_backlight = devm_kzalloc(&hdev->dev, >>>>> sizeof(struct asus_kbd_leds), >>>>> GFP_KERNEL); >>>> >>
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 22/03/25 23:27, Antheas Kapenekakis wrote: > Some ROG laptops expose multiple interfaces for controlling the > keyboard/RGB brightness. This creates a name conflict under > asus::kbd_brightness, where the second device ends up being > named asus::kbd_brightness_1 and they are both broken. > > Therefore, register a listener to the asus-wmi brightness device > instead of creating a new one. > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > --- > drivers/hid/hid-asus.c | 65 +++++++----------------------------------- > 1 file changed, 11 insertions(+), 54 deletions(-) > > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c > index e97fb76eda619..c40b5c14c797f 100644 > --- a/drivers/hid/hid-asus.c > +++ b/drivers/hid/hid-asus.c > @@ -96,7 +96,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad"); > #define TRKID_SGN ((TRKID_MAX + 1) >> 1) > > struct asus_kbd_leds { > - struct led_classdev cdev; > + struct asus_hid_listener listener; > struct hid_device *hdev; > struct work_struct work; > unsigned int brightness; > @@ -493,11 +493,11 @@ static void asus_schedule_work(struct asus_kbd_leds *led) > spin_unlock_irqrestore(&led->lock, flags); > } > > -static void asus_kbd_backlight_set(struct led_classdev *led_cdev, > - enum led_brightness brightness) > +static void asus_kbd_backlight_set(struct asus_hid_listener *listener, > + int brightness) > { > - struct asus_kbd_leds *led = container_of(led_cdev, struct asus_kbd_leds, > - cdev); > + struct asus_kbd_leds *led = container_of(listener, struct asus_kbd_leds, > + listener); > unsigned long flags; > > spin_lock_irqsave(&led->lock, flags); > @@ -507,20 +507,6 @@ static void asus_kbd_backlight_set(struct led_classdev *led_cdev, > asus_schedule_work(led); > } > > -static enum led_brightness asus_kbd_backlight_get(struct led_classdev *led_cdev) > -{ > - struct asus_kbd_leds *led = container_of(led_cdev, struct asus_kbd_leds, > - cdev); > - enum led_brightness brightness; > - unsigned long flags; > - > - 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) > { > struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work); > @@ -537,34 +523,6 @@ static void asus_kbd_backlight_work(struct work_struct *work) > hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret); > } > > -/* WMI-based keyboard backlight LED control (via asus-wmi driver) takes > - * precedence. We only activate HID-based backlight control when the > - * WMI control is not available. > - */ > -static bool asus_kbd_wmi_led_control_present(struct hid_device *hdev) > -{ > - struct asus_drvdata *drvdata = hid_get_drvdata(hdev); > - u32 value; > - int ret; > - > - if (!IS_ENABLED(CONFIG_ASUS_WMI)) > - return false; > - > - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD && > - dmi_check_system(asus_use_hid_led_dmi_ids)) { > - hid_info(hdev, "using HID for asus::kbd_backlight\n"); > - return false; > - } > - > - ret = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DSTS, > - ASUS_WMI_DEVID_KBD_BACKLIGHT, 0, &value); > - hid_dbg(hdev, "WMI backlight check: rc %d value %x", ret, value); > - if (ret) > - return false; > - > - return !!(value & ASUS_WMI_DSTS_PRESENCE_BIT); > -} > - > static int asus_kbd_register_leds(struct hid_device *hdev) > { > struct asus_drvdata *drvdata = hid_get_drvdata(hdev); > @@ -599,14 +557,12 @@ static int asus_kbd_register_leds(struct hid_device *hdev) > drvdata->kbd_backlight->removed = false; > drvdata->kbd_backlight->brightness = 0; > drvdata->kbd_backlight->hdev = hdev; > - drvdata->kbd_backlight->cdev.name = "asus::kbd_backlight"; > - drvdata->kbd_backlight->cdev.max_brightness = 3; > - drvdata->kbd_backlight->cdev.brightness_set = asus_kbd_backlight_set; > - drvdata->kbd_backlight->cdev.brightness_get = asus_kbd_backlight_get; > + drvdata->kbd_backlight->listener.brightness_set = asus_kbd_backlight_set; > INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work); > spin_lock_init(&drvdata->kbd_backlight->lock); > > - ret = devm_led_classdev_register(&hdev->dev, &drvdata->kbd_backlight->cdev); > + 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); > @@ -1000,7 +956,7 @@ static int __maybe_unused asus_resume(struct hid_device *hdev) { > > if (drvdata->kbd_backlight) { > const u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, > - drvdata->kbd_backlight->cdev.brightness }; > + drvdata->kbd_backlight->brightness }; > ret = asus_kbd_set_report(hdev, buf, sizeof(buf)); > if (ret < 0) { > hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret); > @@ -1139,7 +1095,6 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id) > } > > if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) && > - !asus_kbd_wmi_led_control_present(hdev) && > asus_kbd_register_leds(hdev)) > hid_warn(hdev, "Failed to initialize backlight.\n"); > > @@ -1180,6 +1135,8 @@ static void asus_remove(struct hid_device *hdev) > unsigned long flags; > > if (drvdata->kbd_backlight) { > + asus_hid_unregister_listener(&drvdata->kbd_backlight->listener); > + > spin_lock_irqsave(&drvdata->kbd_backlight->lock, flags); > drvdata->kbd_backlight->removed = true; > spin_unlock_irqrestore(&drvdata->kbd_backlight->lock, flags); Reviewed-by: Luke D. Jones <luke@ljones.dev>
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) }, > >>> { } > >> >
This is a three part series which does the following: - Clean init sequence, fix the keyboard of the Z13 (touchpad,fan button) - Unifies backlight handling to happen under asus-wmi so that all Aura devices have synced brightness controls and the backlight button works properly when it is on a USB laptop keyboard instead of one w/ WMI. - Adds RGB support to hid-asus, solid colors only, and for the ROG Ally units and the Asus Z13 2025 first. In V3, RGB controls are pretty stable, but as per Luke there needs to be a discussion about how it should be merged, so the last two patches can be considered as separate. The Z13 Folio has a unique pid, and so do the ally units, so RGB can be safely enabled on those. For the rest, there are cases where the same pid is used in laptops that have a white only keyboard. For more context, see cover letter of V1. --- V2: https://lore.kernel.org/all/20250320220924.5023-1-lkml@antheas.dev/ V1: https://lore.kernel.org/all/20250319191320.10092-1-lkml@antheas.dev/ Changes since V2: - Check lazy init succeds in asus-wmi before setting register variable - make explicit check in asus_hid_register_listener for listener existing to avoid re-init - rename asus_brt to asus_hid in most places and harmonize everything - switch to a spinlock instead of a mutex to avoid kernel ooops - fixup hid device quirks to avoid multiple RGB devices while still exposing all input vendor devices. This includes moving rgb init to probe instead of the input_configured callbacks. - Remove fan key (during retest it appears to be 0xae that is already supported by hid-asus) - Never unregister asus::kbd_backlight while asus-wmi is active, as that - removes fds from userspace and breaks backlight functionality. All - current mainline drivers do not support backlight hotplugging, so most userspace software (e.g., KDE, UPower) is built with that assumption. For the Ally, since it disconnects its controller during sleep, this caused the backlight slider to not work in KDE. Changes since V1: - Add basic RGB support on hid-asus, (Z13/Ally) tested in KDE/Z13 - Fix ifdef else having an invalid signature (reported by kernel robot) - Restore input arguments to init and keyboard function so they can be re-used for RGB controls. - Remove Z13 delay (it did not work to fix the touchpad) and replace it with a HID_GROUP_GENERIC quirk to allow hid-multitouch to load. Squash keyboard rename into it. - Unregister brightness listener before removing work queue to avoid a race condition causing corruption - Remove spurious mutex unlock in asus_brt_event - Place mutex lock in kbd_led_set after LED_UNREGISTERING check to avoid relocking the mutex and causing a deadlock when unregistering leds - Add extra check during unregistering to avoid calling unregister when no led device is registered. - Temporarily HID_QUIRK_INPUT_PER_APP from the ROG endpoint as it causes the driver to create 4 RGB handlers per device. I also suspect some extra events sneak through (KDE had the @@@@@@). Antheas Kapenekakis (10): HID: asus: refactor init sequence per spec HID: asus: prevent binding to all HID devices on ROG HID: Asus: add Z13 folio to generic group for multitouch to work platform/x86: asus-wmi: Add support for multiple kbd RGB handlers HID: asus: listen to the asus-wmi brightness device instead of creating one platform/x86: asus-wmi: remove unused keyboard backlight quirk platform/x86: asus-wmi: add keyboard brightness event handler HID: asus: add support for the asus-wmi brightness handler HID: asus: add basic RGB support HID: asus: add RGB support to the ROG Ally units drivers/hid/hid-asus.c | 354 +++++++++++++++------ drivers/hid/hid-ids.h | 2 +- drivers/platform/x86/asus-wmi.c | 152 ++++++++- include/linux/platform_data/x86/asus-wmi.h | 67 ++-- 4 files changed, 423 insertions(+), 152 deletions(-) base-commit: 4701f33a10702d5fc577c32434eb62adde0a1ae1