HID: asus: Add RGB Support to Asus Z13, Ally, unify backlight asus-wmi, and Z13 QOL

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 <[email protected]>

---
  drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++-------------------
  1 file changed, 46 insertions(+), 36 deletions(-)

drivers/hid/hid-asus.c +1 -1

index 46e3e42f9eb5f..8d4df1b6f143b 100644
@@ -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)
  {

drivers/hid/hid-asus.c +24 -11

+	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);

drivers/hid/hid-asus.c +10 -16

-		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);

Quoted 34 lines · Antheas Kapenekakis

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 <[email protected]>

---
  drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++-------------------
  1 file changed, 46 insertions(+), 36 deletions(-)

drivers/hid/hid-asus.c +1 -1

index 46e3e42f9eb5f..8d4df1b6f143b 100644
@@ -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)
  {

drivers/hid/hid-asus.c static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu · line 398

Quoted 10 lines

drivers/hid/hid-asus.c +8 -2

-     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.

Quoted 62 lines

drivers/hid/hid-asus.c +27 -17

+     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;

Quoted 1 line

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

Quoted 34 lines

drivers/hid/hid-asus.c +10 -16

-             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);

Quoted 52 lines · Luke D. Jones

Quoted 34 lines · Antheas Kapenekakis

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 <[email protected]>

---
   drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++-------------------
   1 file changed, 46 insertions(+), 36 deletions(-)

drivers/hid/hid-asus.c +1 -1

index 46e3e42f9eb5f..8d4df1b6f143b 100644
@@ -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)
   {

drivers/hid/hid-asus.c static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu · line 398

Quoted 10 lines

drivers/hid/hid-asus.c +8 -2

-     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).

Quoted 1 line

Please remove.

It is a context comment but can be removed.

Quoted 62 lines

Quoted 62 lines

drivers/hid/hid-asus.c +27 -17

+     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;

Quoted 8 lines

Quoted 1 line

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?

Quoted 6 lines

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.

Quoted 48 lines

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

Quoted 34 lines

drivers/hid/hid-asus.c +10 -16

-             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);

Quoted 131 lines · Antheas Kapenekakis

Quoted 52 lines · Luke D. Jones

Quoted 34 lines · Antheas Kapenekakis

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 <[email protected]>

---
   drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++-------------------
   1 file changed, 46 insertions(+), 36 deletions(-)

drivers/hid/hid-asus.c +1 -1

index 46e3e42f9eb5f..8d4df1b6f143b 100644
@@ -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)
   {

drivers/hid/hid-asus.c static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu · line 398

Quoted 10 lines

drivers/hid/hid-asus.c +8 -2

-     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).

Quoted 1 line

Please remove.

It is a context comment but can be removed.

Quoted 62 lines

Quoted 62 lines

drivers/hid/hid-asus.c +27 -17

+     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;

Quoted 8 lines

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 6 lines

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.

Quoted 48 lines

Quoted 48 lines

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

Quoted 34 lines

drivers/hid/hid-asus.c +10 -16

-             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);

Quoted 133 lines · Luke D. Jones

Quoted 131 lines · Antheas Kapenekakis

Quoted 52 lines · Luke D. Jones

Quoted 34 lines · Antheas Kapenekakis

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 <[email protected]>

---
   drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++-------------------
   1 file changed, 46 insertions(+), 36 deletions(-)

drivers/hid/hid-asus.c +1 -1

index 46e3e42f9eb5f..8d4df1b6f143b 100644
@@ -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)
   {

drivers/hid/hid-asus.c static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu · line 398

Quoted 10 lines

drivers/hid/hid-asus.c +8 -2

-     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).

Quoted 1 line

Please remove.

It is a context comment but can be removed.

Quoted 62 lines

Quoted 62 lines

drivers/hid/hid-asus.c +27 -17

+     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;

Quoted 8 lines

Quoted 1 line

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.

Quoted 7 lines

Quoted 2 lines

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

Quoted 69 lines

Quoted 8 lines

Quoted 6 lines

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.

Quoted 48 lines

Quoted 48 lines

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

Quoted 34 lines

drivers/hid/hid-asus.c +10 -16

-             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);

Quoted 153 lines · Antheas Kapenekakis

Quoted 133 lines · Luke D. Jones

Quoted 131 lines · Antheas Kapenekakis

Quoted 52 lines · Luke D. Jones

Quoted 34 lines · Antheas Kapenekakis

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 <[email protected]>

---
    drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++-------------------
    1 file changed, 46 insertions(+), 36 deletions(-)

drivers/hid/hid-asus.c +1 -1

index 46e3e42f9eb5f..8d4df1b6f143b 100644
@@ -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)
    {

drivers/hid/hid-asus.c static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu · line 398

Quoted 10 lines

drivers/hid/hid-asus.c +8 -2

-     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).

Quoted 1 line

Please remove.

It is a context comment but can be removed.

Quoted 62 lines

Quoted 62 lines

drivers/hid/hid-asus.c +27 -17

+     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;

Quoted 8 lines

Quoted 1 line

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 3 lines

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

Quoted 71 lines

Antheas

Quoted 69 lines

Quoted 8 lines

Quoted 6 lines

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.

Quoted 48 lines

Quoted 48 lines

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

Quoted 34 lines

drivers/hid/hid-asus.c +10 -16

-             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);

Quoted 133 lines · Luke D. Jones

Quoted 131 lines · Antheas Kapenekakis

Quoted 52 lines · Luke D. Jones

Quoted 34 lines · Antheas Kapenekakis

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 <[email protected]>

---
    drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++-------------------
    1 file changed, 46 insertions(+), 36 deletions(-)

drivers/hid/hid-asus.c +1 -1

index 46e3e42f9eb5f..8d4df1b6f143b 100644
@@ -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)
    {

drivers/hid/hid-asus.c static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu · line 398

Quoted 10 lines

drivers/hid/hid-asus.c +8 -2

-     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).

Quoted 1 line

Please remove.

It is a context comment but can be removed.

Quoted 62 lines

Quoted 62 lines

drivers/hid/hid-asus.c +27 -17

+     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;

Quoted 8 lines

Quoted 1 line

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.

Quoted 7 lines

Quoted 2 lines

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

Quoted 69 lines

Quoted 8 lines

Quoted 6 lines

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.

Quoted 48 lines

Quoted 48 lines

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

Quoted 34 lines

drivers/hid/hid-asus.c +10 -16

-             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);

ROG keyboards are HID compliant and only care about the endpoint that produces vendor events (e.g., fan mode) and has the keyboard backlight.

Therefore, handle all of the endpoints of ROG keyboards as compliant, by adding HID_QUIRK_INPUT_PER_APP and, for devices other than the vendor one, by adding QUIRK_HANDLE_GENERIC to stop mutating them.

Due to HID_QUIRK_INPUT_PER_APP, rgb register is moved into probe, as the input_* functions are called multiple times (4 for the Z13).

Signed-off-by Antheas Kapenekakis <[email protected]>

---
  drivers/hid/hid-asus.c | 69 ++++++++++++++++++++++++++++++++----------
  1 file changed, 53 insertions(+), 16 deletions(-)

drivers/hid/hid-asus.c +49 -16

index 8d4df1b6f143b..96461321c191c 100644
@@ -84,6 +84,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
  #define QUIRK_MEDION_E1239T		BIT(10)
  #define QUIRK_ROG_NKEY_KEYBOARD		BIT(11)
  #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
+#define QUIRK_HANDLE_GENERIC		BIT(13)
 
  #define I2C_KEYBOARD_QUIRKS			(QUIRK_FIX_NOTEBOOK_REPORT | \
  						 QUIRK_NO_INIT_REPORTS | \

@@ -120,7 +121,6 @@ struct asus_drvdata {
  	struct input_dev *tp_kbd_input;
  	struct asus_kbd_leds *kbd_backlight;
  	const struct asus_touchpad_info *tp;
-	bool enable_backlight;
  	struct power_supply *battery;
  	struct power_supply_desc battery_desc;
  	int battery_capacity;

@@ -326,6 +326,10 @@ static int asus_raw_event(struct hid_device *hdev,
  {
  	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
 
+	if (drvdata->quirks & QUIRK_HANDLE_GENERIC)
+		/* NOOP on generic HID devices to avoid side effects. */
+		return 0;
+
  	if (drvdata->battery && data[0] == BATTERY_REPORT_ID)
  		return asus_report_battery(drvdata, data, size);
 

@@ -774,6 +778,10 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
  	struct input_dev *input = hi->input;
  	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
 
+	if (drvdata->quirks & QUIRK_HANDLE_GENERIC)
+		/* NOOP on generic HID devices to avoid side effects. */
+		return 0;
+
  	/* T100CHI uses MULTI_INPUT, bind the touchpad to the mouse hid_input */
  	if (drvdata->quirks & QUIRK_T100CHI &&
  	    hi->report->id != T100CHI_MOUSE_REPORT_ID)

@@ -827,11 +835,6 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
 
  	drvdata->input = input;
 
-	if (drvdata->enable_backlight &&
-	    !asus_kbd_wmi_led_control_present(hdev) &&
-	    asus_kbd_register_leds(hdev))
-		hid_warn(hdev, "Failed to initialize backlight.\n");
-
  	return 0;
  }
 

@@ -851,6 +854,10 @@ static int asus_input_mapping(struct hid_device *hdev,
  		return -1;
  	}
 
+	if (drvdata->quirks & QUIRK_HANDLE_GENERIC)
+		/* NOOP on generic HID devices to avoid side effects. */
+		return 0;
+
  	/*
  	 * Ignore a bunch of bogus collections in the T100CHI descriptor.
  	 * This avoids a bunch of non-functional hid_input devices getting

@@ -901,15 +908,6 @@ static int asus_input_mapping(struct hid_device *hdev,
  			return -1;
  		}
 
-		/*
-		 * Check and enable backlight only on devices with UsagePage ==
-		 * 0xff31 to avoid initializing the keyboard firmware multiple
-		 * times on devices with multiple HID descriptors but same
-		 * PID/VID.
-		 */
-		if (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT)
-			drvdata->enable_backlight = true;
-
  		set_bit(EV_REP, hi->input->evbit);
  		return 1;
  	}

@@ -1026,8 +1024,10 @@ static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
 
  static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
  {
-	int ret;
+	struct hid_report_enum *rep_enum;
  	struct asus_drvdata *drvdata;
+	struct hid_report *rep;
+	int ret, is_vendor = 0;
 
  	drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
  	if (drvdata == NULL) {

@@ -1111,12 +1111,45 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
  		return ret;
  	}
 
+	/*
+	 * Check for the vendor interface (0xff31) to init the RGB.
+	 * and handle generic devices properly.
+	 */
+	rep_enum = &hdev->report_enum[HID_INPUT_REPORT];
+	list_for_each_entry(rep, &rep_enum->report_list, list) {
+		if ((rep->application & HID_USAGE_PAGE) == 0xff310000)
+			is_vendor = true;
+	}
+
+	/*
+	 * For ROG keyboards, make them hid compliant by
+	 * creating one input per application. For interfaces other than
+	 * the vendor one, disable hid-asus handlers.
+	 */
+	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
+		if (!is_vendor)
+			drvdata->quirks |= QUIRK_HANDLE_GENERIC;
+		hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
+	}
+
  	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
  	if (ret) {
  		hid_err(hdev, "Asus hw start failed: %d\n", ret);
  		return ret;
  	}
 
+	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");
+
+	/*
+	 * For ROG keyboards, skip rename for consistency and
+	 * ->input check as some devices do not have inputs.
+	 */
+	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD)
+		return 0;
+
  	if (!drvdata->input) {
  		hid_err(hdev, "Asus input not registered\n");
  		ret = -ENOMEM;

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 <[email protected]>

Signed-off-by Antheas Kapenekakis <[email protected]>

---

  drivers/hid/hid-asus.c | 6 +++---
  drivers/hid/hid-ids.h  | 2 +-
  2 files changed, 4 insertions(+), 4 deletions(-)

drivers/hid/hid-asus.c +3 -3

index 96461321c191c..e97fb76eda619 100644
@@ -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) },
  	{ }

drivers/hid/hid-ids.h +1 -1

index 7e400624908e3..b1fe7582324ff 100644
@@ -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

Quoted 5 lines · Antheas Kapenekakis

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.

Quoted 5 lines

While at it, change the generic KEYBOARD3 name to Z13_FOLIO.

Reviewed-by Luke D. Jones <[email protected]>

Signed-off-by Antheas Kapenekakis <[email protected]>

---

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.

Quoted 41 lines

  drivers/hid/hid-asus.c | 6 +++---
  drivers/hid/hid-ids.h  | 2 +-
  2 files changed, 4 insertions(+), 4 deletions(-)

drivers/hid/hid-asus.c +3 -3

index 96461321c191c..e97fb76eda619 100644
@@ -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) },
      { }

drivers/hid/hid-ids.h +1 -1

index 7e400624908e3..b1fe7582324ff 100644
@@ -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

Some devices, such as the Z13 have multiple AURA devices connected to them by USB. In addition, they might have a WMI interface for RGB. In Windows, Armoury Crate exposes a unified brightness slider for all of them, with 3 brightness levels.

Therefore, to be synergistic in Linux, and support existing tooling such as UPower, allow adding listeners to the RGB device of the WMI interface. If WMI does not exist, lazy initialize the interface.

Signed-off-by Antheas Kapenekakis <[email protected]>

---
  drivers/platform/x86/asus-wmi.c            | 113 ++++++++++++++++++---
  include/linux/platform_data/x86/asus-wmi.h |  16 +++
  2 files changed, 117 insertions(+), 12 deletions(-)

drivers/platform/x86/asus-wmi.c +70 -3

index 38ef778e8c19b..95ef9b1d321bb 100644
@@ -254,6 +254,8 @@ struct asus_wmi {
  	int tpd_led_wk;
  	struct led_classdev kbd_led;
  	int kbd_led_wk;
+	bool kbd_led_avail;
+	bool kbd_led_registered;
  	struct led_classdev lightbar_led;
  	int lightbar_led_wk;
  	struct led_classdev micmute_led;

@@ -1487,6 +1489,53 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
 
  /* LEDs ***********************************************************************/
 
+struct asus_hid_ref {
+	struct list_head listeners;
+	struct asus_wmi *asus;
+	spinlock_t lock;
+};
+
+struct asus_hid_ref asus_ref = {
+	.listeners = LIST_HEAD_INIT(asus_ref.listeners),
+	.asus = NULL,
+	.lock = __SPIN_LOCK_UNLOCKED(asus_ref.lock),
+};
+
+int asus_hid_register_listener(struct asus_hid_listener *bdev)
+{
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&asus_ref.lock, flags);
+	list_add_tail(&bdev->list, &asus_ref.listeners);
+	if (asus_ref.asus) {
+		if (asus_ref.asus->kbd_led_registered && asus_ref.asus->kbd_led_wk >= 0)
+			bdev->brightness_set(bdev, asus_ref.asus->kbd_led_wk);
+
+		if (!asus_ref.asus->kbd_led_registered) {
+			ret = led_classdev_register(
+				&asus_ref.asus->platform_device->dev,
+				&asus_ref.asus->kbd_led);
+			if (!ret)
+				asus_ref.asus->kbd_led_registered = true;
+		}
+	}
+	spin_unlock_irqrestore(&asus_ref.lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(asus_hid_register_listener);
+
+void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&asus_ref.lock, flags);
+	list_del(&bdev->list);
+	spin_unlock_irqrestore(&asus_ref.lock, flags);
+}
+EXPORT_SYMBOL_GPL(asus_hid_unregister_listener);
+
  /*
   * These functions actually update the LED's, and are called from a
   * workqueue. By doing this as separate work rather than when the LED

@@ -1566,6 +1615,7 @@ static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
 
  static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
  {
+	struct asus_hid_listener *listener;
  	struct asus_wmi *asus;
  	int max_level;
 

@@ -1573,25 +1623,39 @@ static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
  	max_level = asus->kbd_led.max_brightness;
 
  	asus->kbd_led_wk = clamp_val(value, 0, max_level);
-	kbd_led_update(asus);
+
+	if (asus->kbd_led_avail)
+		kbd_led_update(asus);
+
+	list_for_each_entry(listener, &asus_ref.listeners, list)
+		listener->brightness_set(listener, asus->kbd_led_wk);
  }
 
  static void kbd_led_set(struct led_classdev *led_cdev,
  			enum led_brightness value)
  {
+	unsigned long flags;
+
  	/* Prevent disabling keyboard backlight on module unregister */
  	if (led_cdev->flags & LED_UNREGISTERING)
  		return;
 
+	spin_lock_irqsave(&asus_ref.lock, flags);
  	do_kbd_led_set(led_cdev, value);
+	spin_unlock_irqrestore(&asus_ref.lock, flags);
  }
 
  static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
  {
-	struct led_classdev *led_cdev = &asus->kbd_led;
+	struct led_classdev *led_cdev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&asus_ref.lock, flags);
+	led_cdev = &asus->kbd_led;
 
  	do_kbd_led_set(led_cdev, value);
  	led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
+	spin_unlock_irqrestore(&asus_ref.lock, flags);
  }
 
  static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)

@@ -1601,6 +1665,9 @@ static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
 
  	asus = container_of(led_cdev, struct asus_wmi, kbd_led);
 
+	if (!asus->kbd_led_avail)
+		return asus->kbd_led_wk;
+
  	retval = kbd_led_read(asus, &value, NULL);
  	if (retval < 0)
  		return retval;

@@ -1716,7 +1783,14 @@ static int camera_led_set(struct led_classdev *led_cdev,
 
  static void asus_wmi_led_exit(struct asus_wmi *asus)
  {
-	led_classdev_unregister(&asus->kbd_led);
+	unsigned long flags;

drivers/platform/x86/asus-wmi.c +24 -9

@@ -1730,6 +1804,8 @@ static void asus_wmi_led_exit(struct asus_wmi *asus)
  static int asus_wmi_led_init(struct asus_wmi *asus)
  {
  	int rv = 0, num_rgb_groups = 0, led_val;
+	unsigned long flags;
+	bool has_listeners;
 
  	if (asus->kbd_rgb_dev)
  		kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;

@@ -1754,24 +1830,37 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
  			goto error;
  	}
 
-	if (!kbd_led_read(asus, &led_val, NULL) && !dmi_check_system(asus_use_hid_led_dmi_ids)) {
-		pr_info("using asus-wmi for asus::kbd_backlight\n");
+	asus->kbd_led.name = "asus::kbd_backlight";
+	asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
+	asus->kbd_led.brightness_set = kbd_led_set;
+	asus->kbd_led.brightness_get = kbd_led_get;
+	asus->kbd_led.max_brightness = 3;
+	asus->kbd_led_avail = !kbd_led_read(asus, &led_val, NULL);
+
+	if (asus->kbd_led_avail)
  		asus->kbd_led_wk = led_val;
-		asus->kbd_led.name = "asus::kbd_backlight";
-		asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
-		asus->kbd_led.brightness_set = kbd_led_set;
-		asus->kbd_led.brightness_get = kbd_led_get;
-		asus->kbd_led.max_brightness = 3;
+	else
+		asus->kbd_led_wk = -1;
+
+	if (asus->kbd_led_avail && num_rgb_groups != 0)
+		asus->kbd_led.groups = kbd_rgb_mode_groups;
 
-		if (num_rgb_groups != 0)
-			asus->kbd_led.groups = kbd_rgb_mode_groups;
+	spin_lock_irqsave(&asus_ref.lock, flags);
+	has_listeners = !list_empty(&asus_ref.listeners);
+	spin_unlock_irqrestore(&asus_ref.lock, flags);
 
+	if (asus->kbd_led_avail || has_listeners) {
  		rv = led_classdev_register(&asus->platform_device->dev,
  					   &asus->kbd_led);
  		if (rv)
  			goto error;
+		asus->kbd_led_registered = true;
  	}
 
+	spin_lock_irqsave(&asus_ref.lock, flags);
+	asus_ref.asus = asus;
+	spin_unlock_irqrestore(&asus_ref.lock, flags);
+
  	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_WIRELESS_LED)
  			&& (asus->driver->quirks->wapf > 0)) {
  		INIT_WORK(&asus->wlan_led_work, wlan_led_update);

include/linux/platform_data/x86/asus-wmi.h +9 -0

index 783e2a336861b..ec8b0c585a63f 100644
@@ -157,14 +157,30 @@
  #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK	0x0000FF00
  #define ASUS_WMI_DSTS_LIGHTBAR_MASK	0x0000000F
 
+struct asus_hid_listener {
+	struct list_head list;
+	void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
+};
+
  #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);
  #else
  static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
  					   u32 *retval)
  {
  	return -ENODEV;
  }
+

Some devices, such as the Z13 have multiple AURA devices connected to them by USB. In addition, they might have a WMI interface for RGB. In Windows, Armoury Crate exposes a unified brightness slider for all of them, with 3 brightness levels.

Therefore, to be synergistic in Linux, and support existing tooling such as UPower, allow adding listeners to the RGB device of the WMI interface. If WMI does not exist, lazy initialize the interface.

Signed-off-by Antheas Kapenekakis <[email protected]>

---
 drivers/platform/x86/asus-wmi.c            | 113 ++++++++++++++++++---
 include/linux/platform_data/x86/asus-wmi.h |  16 +++
 2 files changed, 117 insertions(+), 12 deletions(-)

drivers/platform/x86/asus-wmi.c +88 -10

index 38ef778e8c19b..95ef9b1d321bb 100644
@@ -254,6 +254,8 @@ struct asus_wmi {
 	int tpd_led_wk;
 	struct led_classdev kbd_led;
 	int kbd_led_wk;
+	bool kbd_led_avail;
+	bool kbd_led_registered;
 	struct led_classdev lightbar_led;
 	int lightbar_led_wk;
 	struct led_classdev micmute_led;

@@ -1487,6 +1489,53 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
 
 /* LEDs ***********************************************************************/
 
+struct asus_hid_ref {
+	struct list_head listeners;
+	struct asus_wmi *asus;
+	spinlock_t lock;
+};
+
+struct asus_hid_ref asus_ref = {
+	.listeners = LIST_HEAD_INIT(asus_ref.listeners),
+	.asus = NULL,
+	.lock = __SPIN_LOCK_UNLOCKED(asus_ref.lock),
+};
+
+int asus_hid_register_listener(struct asus_hid_listener *bdev)
+{
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&asus_ref.lock, flags);
+	list_add_tail(&bdev->list, &asus_ref.listeners);
+	if (asus_ref.asus) {
+		if (asus_ref.asus->kbd_led_registered && asus_ref.asus->kbd_led_wk >= 0)
+			bdev->brightness_set(bdev, asus_ref.asus->kbd_led_wk);
+
+		if (!asus_ref.asus->kbd_led_registered) {
+			ret = led_classdev_register(
+				&asus_ref.asus->platform_device->dev,
+				&asus_ref.asus->kbd_led);
+			if (!ret)
+				asus_ref.asus->kbd_led_registered = true;
+		}
+	}
+	spin_unlock_irqrestore(&asus_ref.lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(asus_hid_register_listener);
+
+void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&asus_ref.lock, flags);
+	list_del(&bdev->list);
+	spin_unlock_irqrestore(&asus_ref.lock, flags);
+}
+EXPORT_SYMBOL_GPL(asus_hid_unregister_listener);
+
 /*
  * These functions actually update the LED's, and are called from a
  * workqueue. By doing this as separate work rather than when the LED

@@ -1566,6 +1615,7 @@ static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
 
 static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
 {
+	struct asus_hid_listener *listener;
 	struct asus_wmi *asus;
 	int max_level;
 

@@ -1573,25 +1623,39 @@ static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
 	max_level = asus->kbd_led.max_brightness;
 
 	asus->kbd_led_wk = clamp_val(value, 0, max_level);
-	kbd_led_update(asus);
+
+	if (asus->kbd_led_avail)
+		kbd_led_update(asus);
+
+	list_for_each_entry(listener, &asus_ref.listeners, list)
+		listener->brightness_set(listener, asus->kbd_led_wk);
 }
 
 static void kbd_led_set(struct led_classdev *led_cdev,
 			enum led_brightness value)
 {
+	unsigned long flags;
+
 	/* Prevent disabling keyboard backlight on module unregister */
 	if (led_cdev->flags & LED_UNREGISTERING)
 		return;
 
+	spin_lock_irqsave(&asus_ref.lock, flags);
 	do_kbd_led_set(led_cdev, value);
+	spin_unlock_irqrestore(&asus_ref.lock, flags);
 }
 
 static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
 {
-	struct led_classdev *led_cdev = &asus->kbd_led;
+	struct led_classdev *led_cdev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&asus_ref.lock, flags);
+	led_cdev = &asus->kbd_led;
 
 	do_kbd_led_set(led_cdev, value);
 	led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
+	spin_unlock_irqrestore(&asus_ref.lock, flags);
 }
 
 static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)

@@ -1601,6 +1665,9 @@ static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
 
 	asus = container_of(led_cdev, struct asus_wmi, kbd_led);
 
+	if (!asus->kbd_led_avail)
+		return asus->kbd_led_wk;
+
 	retval = kbd_led_read(asus, &value, NULL);
 	if (retval < 0)
 		return retval;

@@ -1716,7 +1783,14 @@ static int camera_led_set(struct led_classdev *led_cdev,
 
 static void asus_wmi_led_exit(struct asus_wmi *asus)
 {
-	led_classdev_unregister(&asus->kbd_led);
+	unsigned long flags;
+
+	spin_lock_irqsave(&asus_ref.lock, flags);
+	asus_ref.asus = NULL;
+	if (asus->kbd_led_registered)
+		led_classdev_unregister(&asus->kbd_led);
+	spin_unlock_irqrestore(&asus_ref.lock, flags);
+
 	led_classdev_unregister(&asus->tpd_led);
 	led_classdev_unregister(&asus->wlan_led);
 	led_classdev_unregister(&asus->lightbar_led);

@@ -1730,6 +1804,8 @@ static void asus_wmi_led_exit(struct asus_wmi *asus)
 static int asus_wmi_led_init(struct asus_wmi *asus)
 {
 	int rv = 0, num_rgb_groups = 0, led_val;
+	unsigned long flags;
+	bool has_listeners;
 
 	if (asus->kbd_rgb_dev)
 		kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;

@@ -1754,24 +1830,37 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
 			goto error;
 	}
 
-	if (!kbd_led_read(asus, &led_val, NULL) && !dmi_check_system(asus_use_hid_led_dmi_ids)) {
-		pr_info("using asus-wmi for asus::kbd_backlight\n");
+	asus->kbd_led.name = "asus::kbd_backlight";
+	asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
+	asus->kbd_led.brightness_set = kbd_led_set;
+	asus->kbd_led.brightness_get = kbd_led_get;
+	asus->kbd_led.max_brightness = 3;
+	asus->kbd_led_avail = !kbd_led_read(asus, &led_val, NULL);
+
+	if (asus->kbd_led_avail)
 		asus->kbd_led_wk = led_val;
-		asus->kbd_led.name = "asus::kbd_backlight";
-		asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
-		asus->kbd_led.brightness_set = kbd_led_set;
-		asus->kbd_led.brightness_get = kbd_led_get;
-		asus->kbd_led.max_brightness = 3;
+	else

drivers/platform/x86/asus-wmi.c +2 -0

+	spin_unlock_irqrestore(&asus_ref.lock, flags);
+
 	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_WIRELESS_LED)
 			&& (asus->driver->quirks->wapf > 0)) {
 		INIT_WORK(&asus->wlan_led_work, wlan_led_update);

include/linux/platform_data/x86/asus-wmi.h +16 -0

index 783e2a336861b..ec8b0c585a63f 100644
@@ -157,14 +157,30 @@
 #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK	0x0000FF00
 #define ASUS_WMI_DSTS_LIGHTBAR_MASK	0x0000000F
 
+struct asus_hid_listener {
+	struct list_head list;
+	void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
+};
+
 #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);
 #else
 static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
 					   u32 *retval)
 {
 	return -ENODEV;
 }
+
+static inline int asus_hid_register_listener(struct asus_hid_listener *bdev)
+{
+	return -ENODEV;
+}
+static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
+{
+}
 #endif
 
 /* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */

Hi Antheas,

Note not a full review, just taking a generic look at the new API between asus-wmi and asus-hid.

Quoted 193 lines · Antheas Kapenekakis

Some devices, such as the Z13 have multiple AURA devices connected to them by USB. In addition, they might have a WMI interface for RGB. In Windows, Armoury Crate exposes a unified brightness slider for all of them, with 3 brightness levels.

Therefore, to be synergistic in Linux, and support existing tooling such as UPower, allow adding listeners to the RGB device of the WMI interface. If WMI does not exist, lazy initialize the interface.

Signed-off-by Antheas Kapenekakis <[email protected]>

---
 drivers/platform/x86/asus-wmi.c            | 113 ++++++++++++++++++---
 include/linux/platform_data/x86/asus-wmi.h |  16 +++
 2 files changed, 117 insertions(+), 12 deletions(-)

drivers/platform/x86/asus-wmi.c +92 -10

index 38ef778e8c19b..95ef9b1d321bb 100644
@@ -254,6 +254,8 @@ struct asus_wmi {
      int tpd_led_wk;
      struct led_classdev kbd_led;
      int kbd_led_wk;
+     bool kbd_led_avail;
+     bool kbd_led_registered;
      struct led_classdev lightbar_led;
      int lightbar_led_wk;
      struct led_classdev micmute_led;

@@ -1487,6 +1489,53 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
 
 /* LEDs ***********************************************************************/
 
+struct asus_hid_ref {
+     struct list_head listeners;
+     struct asus_wmi *asus;
+     spinlock_t lock;
+};
+
+struct asus_hid_ref asus_ref = {
+     .listeners = LIST_HEAD_INIT(asus_ref.listeners),
+     .asus = NULL,
+     .lock = __SPIN_LOCK_UNLOCKED(asus_ref.lock),
+};
+
+int asus_hid_register_listener(struct asus_hid_listener *bdev)
+{
+     unsigned long flags;
+     int ret = 0;
+
+     spin_lock_irqsave(&asus_ref.lock, flags);
+     list_add_tail(&bdev->list, &asus_ref.listeners);
+     if (asus_ref.asus) {
+             if (asus_ref.asus->kbd_led_registered && asus_ref.asus->kbd_led_wk >= 0)
+                     bdev->brightness_set(bdev, asus_ref.asus->kbd_led_wk);
+
+             if (!asus_ref.asus->kbd_led_registered) {
+                     ret = led_classdev_register(
+                             &asus_ref.asus->platform_device->dev,
+                             &asus_ref.asus->kbd_led);
+                     if (!ret)
+                             asus_ref.asus->kbd_led_registered = true;
+             }
+     }
+     spin_unlock_irqrestore(&asus_ref.lock, flags);
+
+     return ret;
+}
+EXPORT_SYMBOL_GPL(asus_hid_register_listener);
+
+void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
+{
+     unsigned long flags;
+
+     spin_lock_irqsave(&asus_ref.lock, flags);
+     list_del(&bdev->list);
+     spin_unlock_irqrestore(&asus_ref.lock, flags);
+}
+EXPORT_SYMBOL_GPL(asus_hid_unregister_listener);
+
 /*
  * These functions actually update the LED's, and are called from a
  * workqueue. By doing this as separate work rather than when the LED

@@ -1566,6 +1615,7 @@ static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
 
 static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
 {
+     struct asus_hid_listener *listener;
      struct asus_wmi *asus;
      int max_level;
 

@@ -1573,25 +1623,39 @@ static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
      max_level = asus->kbd_led.max_brightness;
 
      asus->kbd_led_wk = clamp_val(value, 0, max_level);
-     kbd_led_update(asus);
+
+     if (asus->kbd_led_avail)
+             kbd_led_update(asus);
+
+     list_for_each_entry(listener, &asus_ref.listeners, list)
+             listener->brightness_set(listener, asus->kbd_led_wk);
 }
 
 static void kbd_led_set(struct led_classdev *led_cdev,
                      enum led_brightness value)
 {
+     unsigned long flags;
+
      /* Prevent disabling keyboard backlight on module unregister */
      if (led_cdev->flags & LED_UNREGISTERING)
              return;
 
+     spin_lock_irqsave(&asus_ref.lock, flags);
      do_kbd_led_set(led_cdev, value);
+     spin_unlock_irqrestore(&asus_ref.lock, flags);
 }
 
 static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
 {
-     struct led_classdev *led_cdev = &asus->kbd_led;
+     struct led_classdev *led_cdev;
+     unsigned long flags;
+
+     spin_lock_irqsave(&asus_ref.lock, flags);
+     led_cdev = &asus->kbd_led;
 
      do_kbd_led_set(led_cdev, value);
      led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
+     spin_unlock_irqrestore(&asus_ref.lock, flags);
 }
 
 static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)

@@ -1601,6 +1665,9 @@ static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
 
      asus = container_of(led_cdev, struct asus_wmi, kbd_led);
 
+     if (!asus->kbd_led_avail)
+             return asus->kbd_led_wk;
+
      retval = kbd_led_read(asus, &value, NULL);
      if (retval < 0)
              return retval;

@@ -1716,7 +1783,14 @@ static int camera_led_set(struct led_classdev *led_cdev,
 
 static void asus_wmi_led_exit(struct asus_wmi *asus)
 {
-     led_classdev_unregister(&asus->kbd_led);
+     unsigned long flags;
+
+     spin_lock_irqsave(&asus_ref.lock, flags);
+     asus_ref.asus = NULL;
+     if (asus->kbd_led_registered)
+             led_classdev_unregister(&asus->kbd_led);
+     spin_unlock_irqrestore(&asus_ref.lock, flags);
+
      led_classdev_unregister(&asus->tpd_led);
      led_classdev_unregister(&asus->wlan_led);
      led_classdev_unregister(&asus->lightbar_led);

@@ -1730,6 +1804,8 @@ static void asus_wmi_led_exit(struct asus_wmi *asus)
 static int asus_wmi_led_init(struct asus_wmi *asus)
 {
      int rv = 0, num_rgb_groups = 0, led_val;
+     unsigned long flags;
+     bool has_listeners;
 
      if (asus->kbd_rgb_dev)
              kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;

@@ -1754,24 +1830,37 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
                      goto error;
      }
 
-     if (!kbd_led_read(asus, &led_val, NULL) && !dmi_check_system(asus_use_hid_led_dmi_ids)) {
-             pr_info("using asus-wmi for asus::kbd_backlight\n");
+     asus->kbd_led.name = "asus::kbd_backlight";
+     asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
+     asus->kbd_led.brightness_set = kbd_led_set;
+     asus->kbd_led.brightness_get = kbd_led_get;
+     asus->kbd_led.max_brightness = 3;
+     asus->kbd_led_avail = !kbd_led_read(asus, &led_val, NULL);
+
+     if (asus->kbd_led_avail)
              asus->kbd_led_wk = led_val;
-             asus->kbd_led.name = "asus::kbd_backlight";
-             asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
-             asus->kbd_led.brightness_set = kbd_led_set;
-             asus->kbd_led.brightness_get = kbd_led_get;
-             asus->kbd_led.max_brightness = 3;
+     else
+             asus->kbd_led_wk = -1;
+
+     if (asus->kbd_led_avail && num_rgb_groups != 0)
+             asus->kbd_led.groups = kbd_rgb_mode_groups;
 

drivers/platform/x86/asus-wmi.c static int asus_wmi_led_init(struct asus_wmi *asus) · line 1850

Quoted 5 lines

drivers/platform/x86/asus-wmi.c +3 -2

-             if (num_rgb_groups != 0)
-                     asus->kbd_led.groups = kbd_rgb_mode_groups;
+     spin_lock_irqsave(&asus_ref.lock, flags);
+     has_listeners = !list_empty(&asus_ref.listeners);
+     spin_unlock_irqrestore(&asus_ref.lock, flags);

It seems to me that you should also call brightness_set() on all the kbds already in the list so that their brightness gets synced with the wmi kbd-backlight brightness when the wmi driver loads later then the hid driver ?

Quoted 9 lines

drivers/platform/x86/asus-wmi.c +2 -0

+     if (asus->kbd_led_avail || has_listeners) {
              rv = led_classdev_register(&asus->platform_device->dev,
                                         &asus->kbd_led);
              if (rv)
                      goto error;
+             asus->kbd_led_registered = true;
      }
 

drivers/platform/x86/asus-wmi.c static int asus_wmi_led_init(struct asus_wmi *asus) · line 1860

Quoted 2 lines

drivers/platform/x86/asus-wmi.c +2 -0

+     spin_lock_irqsave(&asus_ref.lock, flags);
+     asus_ref.asus = asus;

There is race here where a hid keyboard might show up between the 2 places you take the lock, in that case if there is no wmi kbd-backlight then you will not register the led_classdev when asus_hid_register_listener() gets called between the unlock and the lock... I'm not sure what the best way is to fix this.

Regards,

Hans

Quoted 40 lines

drivers/platform/x86/asus-wmi.c +2 -0

+     spin_unlock_irqrestore(&asus_ref.lock, flags);
+
      if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_WIRELESS_LED)
                      && (asus->driver->quirks->wapf > 0)) {
              INIT_WORK(&asus->wlan_led_work, wlan_led_update);

include/linux/platform_data/x86/asus-wmi.h +16 -0

index 783e2a336861b..ec8b0c585a63f 100644
@@ -157,14 +157,30 @@
 #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK        0x0000FF00
 #define ASUS_WMI_DSTS_LIGHTBAR_MASK  0x0000000F
 
+struct asus_hid_listener {
+     struct list_head list;
+     void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
+};
+
 #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);
 #else
 static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
                                         u32 *retval)
 {
      return -ENODEV;
 }
+
+static inline int asus_hid_register_listener(struct asus_hid_listener *bdev)
+{
+     return -ENODEV;
+}
+static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
+{
+}
 #endif
 
 /* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */

Quoted 200 lines · Hans de Goede

Hi Antheas,

Note not a full review, just taking a generic look at the new API between asus-wmi and asus-hid.

Quoted 193 lines · Antheas Kapenekakis

Some devices, such as the Z13 have multiple AURA devices connected to them by USB. In addition, they might have a WMI interface for RGB. In Windows, Armoury Crate exposes a unified brightness slider for all of them, with 3 brightness levels.

Therefore, to be synergistic in Linux, and support existing tooling such as UPower, allow adding listeners to the RGB device of the WMI interface. If WMI does not exist, lazy initialize the interface.

Signed-off-by Antheas Kapenekakis <[email protected]>

---
 drivers/platform/x86/asus-wmi.c            | 113 ++++++++++++++++++---
 include/linux/platform_data/x86/asus-wmi.h |  16 +++
 2 files changed, 117 insertions(+), 12 deletions(-)

drivers/platform/x86/asus-wmi.c +92 -10

index 38ef778e8c19b..95ef9b1d321bb 100644
@@ -254,6 +254,8 @@ struct asus_wmi {
      int tpd_led_wk;
      struct led_classdev kbd_led;
      int kbd_led_wk;
+     bool kbd_led_avail;
+     bool kbd_led_registered;
      struct led_classdev lightbar_led;
      int lightbar_led_wk;
      struct led_classdev micmute_led;

@@ -1487,6 +1489,53 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
 
 /* LEDs ***********************************************************************/
 
+struct asus_hid_ref {
+     struct list_head listeners;
+     struct asus_wmi *asus;
+     spinlock_t lock;
+};
+
+struct asus_hid_ref asus_ref = {
+     .listeners = LIST_HEAD_INIT(asus_ref.listeners),
+     .asus = NULL,
+     .lock = __SPIN_LOCK_UNLOCKED(asus_ref.lock),
+};
+
+int asus_hid_register_listener(struct asus_hid_listener *bdev)
+{
+     unsigned long flags;
+     int ret = 0;
+
+     spin_lock_irqsave(&asus_ref.lock, flags);
+     list_add_tail(&bdev->list, &asus_ref.listeners);
+     if (asus_ref.asus) {
+             if (asus_ref.asus->kbd_led_registered && asus_ref.asus->kbd_led_wk >= 0)
+                     bdev->brightness_set(bdev, asus_ref.asus->kbd_led_wk);
+
+             if (!asus_ref.asus->kbd_led_registered) {
+                     ret = led_classdev_register(
+                             &asus_ref.asus->platform_device->dev,
+                             &asus_ref.asus->kbd_led);
+                     if (!ret)
+                             asus_ref.asus->kbd_led_registered = true;
+             }
+     }
+     spin_unlock_irqrestore(&asus_ref.lock, flags);
+
+     return ret;
+}
+EXPORT_SYMBOL_GPL(asus_hid_register_listener);
+
+void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
+{
+     unsigned long flags;
+
+     spin_lock_irqsave(&asus_ref.lock, flags);
+     list_del(&bdev->list);
+     spin_unlock_irqrestore(&asus_ref.lock, flags);
+}
+EXPORT_SYMBOL_GPL(asus_hid_unregister_listener);
+
 /*
  * These functions actually update the LED's, and are called from a
  * workqueue. By doing this as separate work rather than when the LED

@@ -1566,6 +1615,7 @@ static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
 
 static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
 {
+     struct asus_hid_listener *listener;
      struct asus_wmi *asus;
      int max_level;
 

@@ -1573,25 +1623,39 @@ static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
      max_level = asus->kbd_led.max_brightness;
 
      asus->kbd_led_wk = clamp_val(value, 0, max_level);
-     kbd_led_update(asus);
+
+     if (asus->kbd_led_avail)
+             kbd_led_update(asus);
+
+     list_for_each_entry(listener, &asus_ref.listeners, list)
+             listener->brightness_set(listener, asus->kbd_led_wk);
 }
 
 static void kbd_led_set(struct led_classdev *led_cdev,
                      enum led_brightness value)
 {
+     unsigned long flags;
+
      /* Prevent disabling keyboard backlight on module unregister */
      if (led_cdev->flags & LED_UNREGISTERING)
              return;
 
+     spin_lock_irqsave(&asus_ref.lock, flags);
      do_kbd_led_set(led_cdev, value);
+     spin_unlock_irqrestore(&asus_ref.lock, flags);
 }
 
 static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
 {
-     struct led_classdev *led_cdev = &asus->kbd_led;
+     struct led_classdev *led_cdev;
+     unsigned long flags;
+
+     spin_lock_irqsave(&asus_ref.lock, flags);
+     led_cdev = &asus->kbd_led;
 
      do_kbd_led_set(led_cdev, value);
      led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
+     spin_unlock_irqrestore(&asus_ref.lock, flags);
 }
 
 static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)

@@ -1601,6 +1665,9 @@ static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
 
      asus = container_of(led_cdev, struct asus_wmi, kbd_led);
 
+     if (!asus->kbd_led_avail)
+             return asus->kbd_led_wk;
+
      retval = kbd_led_read(asus, &value, NULL);
      if (retval < 0)
              return retval;

@@ -1716,7 +1783,14 @@ static int camera_led_set(struct led_classdev *led_cdev,
 
 static void asus_wmi_led_exit(struct asus_wmi *asus)
 {
-     led_classdev_unregister(&asus->kbd_led);
+     unsigned long flags;
+
+     spin_lock_irqsave(&asus_ref.lock, flags);
+     asus_ref.asus = NULL;
+     if (asus->kbd_led_registered)
+             led_classdev_unregister(&asus->kbd_led);
+     spin_unlock_irqrestore(&asus_ref.lock, flags);
+
      led_classdev_unregister(&asus->tpd_led);
      led_classdev_unregister(&asus->wlan_led);
      led_classdev_unregister(&asus->lightbar_led);

@@ -1730,6 +1804,8 @@ static void asus_wmi_led_exit(struct asus_wmi *asus)
 static int asus_wmi_led_init(struct asus_wmi *asus)
 {
      int rv = 0, num_rgb_groups = 0, led_val;
+     unsigned long flags;
+     bool has_listeners;
 
      if (asus->kbd_rgb_dev)
              kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;

@@ -1754,24 +1830,37 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
                      goto error;
      }
 
-     if (!kbd_led_read(asus, &led_val, NULL) && !dmi_check_system(asus_use_hid_led_dmi_ids)) {
-             pr_info("using asus-wmi for asus::kbd_backlight\n");
+     asus->kbd_led.name = "asus::kbd_backlight";
+     asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
+     asus->kbd_led.brightness_set = kbd_led_set;
+     asus->kbd_led.brightness_get = kbd_led_get;
+     asus->kbd_led.max_brightness = 3;
+     asus->kbd_led_avail = !kbd_led_read(asus, &led_val, NULL);
+
+     if (asus->kbd_led_avail)
              asus->kbd_led_wk = led_val;
-             asus->kbd_led.name = "asus::kbd_backlight";
-             asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
-             asus->kbd_led.brightness_set = kbd_led_set;
-             asus->kbd_led.brightness_get = kbd_led_get;
-             asus->kbd_led.max_brightness = 3;
+     else
+             asus->kbd_led_wk = -1;
+
+     if (asus->kbd_led_avail && num_rgb_groups != 0)
+             asus->kbd_led.groups = kbd_rgb_mode_groups;
 

Quoted 10 lines

drivers/platform/x86/asus-wmi.c static int asus_wmi_led_init(struct asus_wmi *asus) · line 1850

Quoted 5 lines

drivers/platform/x86/asus-wmi.c +3 -2

-             if (num_rgb_groups != 0)
-                     asus->kbd_led.groups = kbd_rgb_mode_groups;
+     spin_lock_irqsave(&asus_ref.lock, flags);
+     has_listeners = !list_empty(&asus_ref.listeners);
+     spin_unlock_irqrestore(&asus_ref.lock, flags);

It seems to me that you should also call brightness_set() on all the kbds already in the list so that their brightness gets synced with the wmi kbd-backlight brightness when the wmi driver loads later then the hid driver ?

You raise a good point here. Let me think about it.

But yes, if we do a notify when kbd_led_wk != -1 if the hid device connects after, we should do it for the before case too.

Quoted 9 lines

Quoted 9 lines

drivers/platform/x86/asus-wmi.c +2 -0

+     if (asus->kbd_led_avail || has_listeners) {
              rv = led_classdev_register(&asus->platform_device->dev,
                                         &asus->kbd_led);
              if (rv)
                      goto error;
+             asus->kbd_led_registered = true;
      }
 

Quoted 8 lines

drivers/platform/x86/asus-wmi.c static int asus_wmi_led_init(struct asus_wmi *asus) · line 1860

Quoted 2 lines

drivers/platform/x86/asus-wmi.c +2 -0

+     spin_lock_irqsave(&asus_ref.lock, flags);
+     asus_ref.asus = asus;

There is race here where a hid keyboard might show up between the 2 places you take the lock, in that case if there is no wmi kbd-backlight then you will not register the led_classdev when asus_hid_register_listener() gets called between the unlock and the lock... I'm not sure what the best way is to fix this.

Thanks for catching that, probably a recheck:

has_listeners = !list_empty(&asus_ref.listeners);
if (has_listeners) goto register;

Quoted 41 lines

Quoted 40 lines

drivers/platform/x86/asus-wmi.c +2 -0

+     spin_unlock_irqrestore(&asus_ref.lock, flags);
+
      if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_WIRELESS_LED)
                      && (asus->driver->quirks->wapf > 0)) {
              INIT_WORK(&asus->wlan_led_work, wlan_led_update);

include/linux/platform_data/x86/asus-wmi.h +16 -0

index 783e2a336861b..ec8b0c585a63f 100644
@@ -157,14 +157,30 @@
 #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK        0x0000FF00
 #define ASUS_WMI_DSTS_LIGHTBAR_MASK  0x0000000F
 
+struct asus_hid_listener {
+     struct list_head list;
+     void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
+};
+
 #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);
 #else
 static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
                                         u32 *retval)
 {
      return -ENODEV;
 }
+
+static inline int asus_hid_register_listener(struct asus_hid_listener *bdev)
+{
+     return -ENODEV;
+}
+static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
+{
+}
 #endif
 
 /* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */