| Message ID | 20250322102804.418000-2-lkml@antheas.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | HID: asus: Add RGB Support to Asus Z13, Ally, unify backlight asus-wmi, and Z13 QOL | expand |
On 22/03/25 23: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 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 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); >>>> >>
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. + */ + 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; - 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);
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(-)