[PATCH 00/11] HID: Asus: Add RGB Support to Asus Z13, Ally, unify backlight asus-wmi, and Z13 QOL

[PATCH 00/11] HID: Asus: Add RGB Support to Asus Z13, Ally, unify backlight asus-wmi, and Z13 QOL

[Antheas Kapenekakis]

This is a three part series which does the following:
  - Clean init sequence, fix the keyboard of the Z13 (touchpad,fan button)
  - Unifies backlight handling to happen under asus-wmi so that all Aura
    devices have synced brightness controls and the backlight button works
    properly when it is on a USB laptop keyboard instead of one w/ WMI.
  - Adds RGB support to hid-asus, solid colors only, and for the ROG Ally
    units and the Asus Z13 2025 first.

For context, see cover letter of V1.

The last two patches are still a bit experimental, the rest is getting to
be pretty stable by now. I will test my Ally in the weekend. Also, I am
not a fan of the asus-0003:0B05:1A30.0001-led name, so suggestions would
be appreciated.

---
V1: https://lore.kernel.org/all/20250319191320.10092-1-lkml@antheas.dev/

Changes since V1:
  - Add basic RGB support on hid-asus, (Z13/Ally) tested in KDE/Z13
  - Fix ifdef else having an invalid signature (reported by kernel robot)
  - Restore input arguments to init and keyboard function so they can
    be re-used for RGB controls.
  - Remove Z13 delay (it did not work to fix the touchpad) and replace it
    with a HID_GROUP_GENERIC quirk to allow hid-multitouch to load. Squash
    keyboard rename into it.
  - Unregister brightness listener before removing work queue to avoid
    a race condition causing corruption
  - Remove spurious mutex unlock in asus_brt_event
  - Place mutex lock in kbd_led_set after LED_UNREGISTERING check to avoid
    relocking the mutex and causing a deadlock when unregistering leds
  - Add extra check during unregistering to avoid calling unregister when
    no led device is registered.
  - Temporarily HID_QUIRK_INPUT_PER_APP from the ROG endpoint as it causes
    the driver to create 4 RGB handlers per device. I also suspect some
    extra events sneak through (KDE had the @@@@@@).

Antheas Kapenekakis (11):
  HID: asus: refactor init sequence per spec
  HID: asus: prevent binding to all HID devices on ROG
  HID: asus: add Asus Z13 2025 Fan key
  HID: Asus: add Z13 folio to generic group for multitouch to work
  platform/x86: asus-wmi: Add support for multiple kbd RGB handlers
  HID: asus: listen to the asus-wmi brightness device instead of
    creating one
  platform/x86: asus-wmi: remove unused keyboard backlight quirk
  platform/x86: asus-wmi: add keyboard brightness event handler
  HID: asus: add support for the asus-wmi brightness handler
  HID: asus: add basic RGB support
  HID: asus: add RGB support to the ROG Ally units

 drivers/hid/hid-asus.c                     | 342 ++++++++++++++++-----
 drivers/hid/hid-ids.h                      |   2 +-
 drivers/platform/x86/asus-wmi.c            | 138 ++++++++-
 include/linux/platform_data/x86/asus-wmi.h |  67 ++--
 4 files changed, 411 insertions(+), 138 deletions(-)


base-commit: 4701f33a10702d5fc577c32434eb62adde0a1ae1
-- 
2.48.1

[PATCH 01/11] HID: asus: refactor init sequence per spec

[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 <lkml@antheas.dev>
---
 drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 36 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 46e3e42f9eb5f..8d4df1b6f143b 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -48,7 +48,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 #define FEATURE_REPORT_ID 0x0d
 #define INPUT_REPORT_ID 0x5d
 #define FEATURE_KBD_REPORT_ID 0x5a
-#define FEATURE_KBD_REPORT_SIZE 16
+#define FEATURE_KBD_REPORT_SIZE 64
 #define FEATURE_KBD_LED_REPORT_ID1 0x5d
 #define FEATURE_KBD_LED_REPORT_ID2 0x5e
 
@@ -388,14 +388,41 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu
 
 static int asus_kbd_init(struct hid_device *hdev, u8 report_id)
 {
-	const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54,
-		     0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
+	/*
+	 * Asus handshake identifying us as a driver (0x5A)
+	 * 0x5A then ASCII for "ASUS Tech.Inc."
+	 * 0x5D is for userspace Windows applications.
+	 *
+	 * The handshake is first sent as a set_report, then retrieved
+	 * from a get_report to verify the response.
+	 */
+	const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20,
+		0x54, 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
+	u8 *readbuf;
 	int ret;
 
 	ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
-	if (ret < 0)
-		hid_err(hdev, "Asus failed to send init command: %d\n", ret);
+	if (ret < 0) {
+		hid_err(hdev, "Asus failed to send handshake: %d\n", ret);
+		return ret;
+	}
 
+	readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL);
+	if (!readbuf)
+		return -ENOMEM;
+
+	ret = hid_hw_raw_request(hdev, report_id, readbuf,
+				 FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
+				 HID_REQ_GET_REPORT);
+	if (ret < 0) {
+		hid_err(hdev, "Asus failed to receive handshake ack: %d\n", ret);
+	} else if (memcmp(readbuf, buf, sizeof(buf)) != 0) {
+		hid_err(hdev, "Asus handshake returned invalid response: %*ph\n",
+			FEATURE_KBD_REPORT_SIZE, readbuf);
+		// Do not return error if handshake is wrong to avoid regressions
+	}
+
+	kfree(readbuf);
 	return ret;
 }
 
@@ -417,7 +444,7 @@ static int asus_kbd_get_functions(struct hid_device *hdev,
 	if (!readbuf)
 		return -ENOMEM;
 
-	ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf,
+	ret = hid_hw_raw_request(hdev, report_id, readbuf,
 				 FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
 				 HID_REQ_GET_REPORT);
 	if (ret < 0) {
@@ -540,42 +567,25 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
 	unsigned char kbd_func;
 	int ret;
 
-	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
-		/* Initialize keyboard */
-		ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
-		if (ret < 0)
-			return ret;
-
-		/* The LED endpoint is initialised in two HID */
-		ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
-		if (ret < 0)
-			return ret;
-
-		ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
-		if (ret < 0)
-			return ret;
+	ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
+	if (ret < 0)
+		return ret;
 
-		if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
-			ret = asus_kbd_disable_oobe(hdev);
-			if (ret < 0)
-				return ret;
-		}
-	} else {
-		/* Initialize keyboard */
-		ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
-		if (ret < 0)
-			return ret;
+	/* Get keyboard functions */
+	ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
+	if (ret < 0)
+		return ret;
 
-		/* Get keyboard functions */
-		ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
+	if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
+		ret = asus_kbd_disable_oobe(hdev);
 		if (ret < 0)
 			return ret;
-
-		/* Check for backlight support */
-		if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
-			return -ENODEV;
 	}
 
+	/* Check for backlight support */
+	if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
+		return -ENODEV;
+
 	drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
 					      sizeof(struct asus_kbd_leds),
 					      GFP_KERNEL);
-- 
2.48.1

[PATCH 02/11] HID: asus: prevent binding to all HID devices on ROG

[Antheas Kapenekakis]

ROG keyboards are HID compliant. We only care about the endpoint that
produces vendor events (e.g., fan mode) and has the keyboard backlight.
If we attach to all the endpoints, we end up generating errors during
probe for two of them because they are missing the ->input attribute
and risk side effects during input fixups.

Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/hid/hid-asus.c | 54 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 8d4df1b6f143b..5eb70716702ef 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -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 | \
@@ -326,6 +327,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 +779,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)
@@ -851,6 +860,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
@@ -1026,8 +1039,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, found = 0;
 
 	drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
 	if (drvdata == NULL) {
@@ -1111,6 +1126,39 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
+	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
+		/*
+		 * The only application we care about on ROG NKEY keyboards is
+		 * 0xff310076. This is meant Asus drivers and uses report 0x54.
+		 */
+		rep_enum = &hdev->report_enum[HID_INPUT_REPORT];
+		list_for_each_entry(rep, &rep_enum->report_list, list) {
+			if (rep->application == 0xff310076)
+				found = true;
+		}
+
+		/*
+		 * If we didn't find the application, block hid-asus fixups
+		 * to prevent side effects on generic endpoints.
+		 *
+		 * We cannot -ENODEV here, as hid-generic checked our id_table
+		 * on its match and bailed so it will not take over the device.
+		 * We have to handle it transparently as part of this driver.
+		 */
+		if (!found) {
+			drvdata->quirks |= QUIRK_HANDLE_GENERIC;
+			hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
+		}
+
+		/*
+		 * Start all endpoints normally. Include the RGB endpoint
+		 * as it being the only one renamed looks out of place.
+		 * The ->input bail causes regressions in endpoints without
+		 * an input dev and is a NOOP on the RGB endpoint.
+		 */
+		return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+	}
+
 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	if (ret) {
 		hid_err(hdev, "Asus hw start failed: %d\n", ret);
@@ -1167,6 +1215,10 @@ static const __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 {
 	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
 
+	if (drvdata->quirks & QUIRK_HANDLE_GENERIC)
+		/* NOOP on generic HID devices to avoid side effects. */
+		return rdesc;
+
 	if (drvdata->quirks & QUIRK_FIX_NOTEBOOK_REPORT &&
 			*rsize >= 56 && rdesc[54] == 0x25 && rdesc[55] == 0x65) {
 		hid_info(hdev, "Fixing up Asus notebook report descriptor\n");
-- 
2.48.1

[PATCH 03/11] HID: asus: add Asus Z13 2025 Fan key

[Antheas Kapenekakis]

The ASUS Z13 2025 uses the vendor code 0xec for
its Fn+F5 fan key. Add a quirk for it.

Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/hid/hid-asus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 5eb70716702ef..b61b53d5ef240 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -898,6 +898,7 @@ static int asus_input_mapping(struct hid_device *hdev,
 		case 0x5c: asus_map_key_clear(KEY_PROG3);	break; /* Fn+Space Power4Gear */
 		case 0x99: asus_map_key_clear(KEY_PROG4);	break; /* Fn+F5 "fan" symbol */
 		case 0xae: asus_map_key_clear(KEY_PROG4);	break; /* Fn+F5 "fan" symbol */
+		case 0xec: asus_map_key_clear(KEY_PROG4);	break; /* Fn+F5 "fan" symbol (Z13 2025) */
 		case 0x92: asus_map_key_clear(KEY_CALC);	break; /* Fn+Ret "Calc" symbol */
 		case 0xb2: asus_map_key_clear(KEY_PROG2);	break; /* Fn+Left previous aura */
 		case 0xb3: asus_map_key_clear(KEY_PROG3);	break; /* Fn+Left next aura */
-- 
2.48.1

[PATCH 04/11] HID: Asus: add Z13 folio to generic group for multitouch to work

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

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

Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/hid/hid-asus.c | 6 +++---
 drivers/hid/hid-ids.h  | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index b61b53d5ef240..beb7df823b18e 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -1335,9 +1335,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 },
@@ -1367,6 +1364,9 @@ static const struct hid_device_id asus_devices[] = {
 	 * Note bind to the HID_GROUP_GENERIC group, so that we only bind to the keyboard
 	 * part, while letting hid-multitouch.c handle the touchpad.
 	 */
+	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
+		USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_ROG_Z13_FOLIO),
+	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
 	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
 		USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_T101HA_KEYBOARD) },
 	{ }
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 7e400624908e3..b1fe7582324ff 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -209,7 +209,7 @@
 #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD3 0x1822
 #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD	0x1866
 #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2	0x19b6
-#define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD3	0x1a30
+#define USB_DEVICE_ID_ASUSTEK_ROG_Z13_FOLIO		0x1a30
 #define USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR		0x18c6
 #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY		0x1abe
 #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X		0x1b4c
-- 
2.48.1

Re: [PATCH 04/11] HID: Asus: add Z13 folio to generic group for multitouch to work

[Luke D. Jones]

On 21/03/25 11:09, Antheas Kapenekakis wrote:
> The Asus Z13 folio has a multitouch touchpad that needs to bind
> to the hid-multitouch driver in order to work properly. So bind
> it to the HID_GROUP_GENERIC group to release the touchpad and
> move it to the bottom so that the comment applies to it.
> 
> While at it, change the generic KEYBOARD3 name to Z13_FOLIO.
> 
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
>   drivers/hid/hid-asus.c | 6 +++---
>   drivers/hid/hid-ids.h  | 2 +-
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index b61b53d5ef240..beb7df823b18e 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -1335,9 +1335,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 },
> @@ -1367,6 +1364,9 @@ static const struct hid_device_id asus_devices[] = {
>   	 * Note bind to the HID_GROUP_GENERIC group, so that we only bind to the keyboard
>   	 * part, while letting hid-multitouch.c handle the touchpad.
>   	 */
> +	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
> +		USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_ROG_Z13_FOLIO),
> +	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>   	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
>   		USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_T101HA_KEYBOARD) },
>   	{ }
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 7e400624908e3..b1fe7582324ff 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -209,7 +209,7 @@
>   #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD3 0x1822
>   #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD	0x1866
>   #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2	0x19b6
> -#define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD3	0x1a30
> +#define USB_DEVICE_ID_ASUSTEK_ROG_Z13_FOLIO		0x1a30
>   #define USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR		0x18c6
>   #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY		0x1abe
>   #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X		0x1b4c

Sorry, I got mixed up in Thunderbird since the V2 prefix is missing and 
was reviewing wrong series, I'll archive the last series to prevent 
confusing myself.

Reviewed-by: Luke D. Jones <luke@ljones.dev>

[PATCH 05/11] platform/x86: asus-wmi: Add support for multiple kbd RGB handlers

[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 <lkml@antheas.dev>
---
 drivers/platform/x86/asus-wmi.c            | 100 ++++++++++++++++++---
 include/linux/platform_data/x86/asus-wmi.h |  16 ++++
 2 files changed, 104 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 38ef778e8c19b..21e034be71b2f 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -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,46 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
 
 /* LEDs ***********************************************************************/
 
+LIST_HEAD(asus_brt_listeners);
+DEFINE_MUTEX(asus_brt_lock);
+struct asus_wmi *asus_brt_ref;
+
+int asus_brt_register_listener(struct asus_brt_listener *bdev)
+{
+	int ret;
+
+	mutex_lock(&asus_brt_lock);
+	list_add_tail(&bdev->list, &asus_brt_listeners);
+	if (asus_brt_ref) {
+		if (asus_brt_ref->kbd_led_registered && asus_brt_ref->kbd_led_wk >= 0)
+			bdev->notify(bdev, asus_brt_ref->kbd_led_wk);
+		else {
+			asus_brt_ref->kbd_led_registered = true;
+			ret = led_classdev_register(
+				&asus_brt_ref->platform_device->dev,
+				&asus_brt_ref->kbd_led);
+		}
+	}
+	mutex_unlock(&asus_brt_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(asus_brt_register_listener);
+
+void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
+{
+	mutex_lock(&asus_brt_lock);
+	list_del(&bdev->list);
+
+	if (asus_brt_ref && asus_brt_ref->kbd_led_registered &&
+	    list_empty(&asus_brt_listeners) && !asus_brt_ref->kbd_led_avail) {
+		led_classdev_unregister(&asus_brt_ref->kbd_led);
+		asus_brt_ref->kbd_led_registered = false;
+	}
+	mutex_unlock(&asus_brt_lock);
+}
+EXPORT_SYMBOL_GPL(asus_brt_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 +1608,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_brt_listener *listener;
 	struct asus_wmi *asus;
 	int max_level;
 
@@ -1573,7 +1616,12 @@ 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_brt_listeners, list)
+		listener->notify(listener, asus->kbd_led_wk);
 }
 
 static void kbd_led_set(struct led_classdev *led_cdev,
@@ -1583,15 +1631,21 @@ static void kbd_led_set(struct led_classdev *led_cdev,
 	if (led_cdev->flags & LED_UNREGISTERING)
 		return;
 
+	mutex_lock(&asus_brt_lock);
 	do_kbd_led_set(led_cdev, value);
+	mutex_unlock(&asus_brt_lock);
 }
 
 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;
+
+	mutex_lock(&asus_brt_lock);
+	led_cdev = &asus->kbd_led;
 
 	do_kbd_led_set(led_cdev, value);
 	led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
+	mutex_unlock(&asus_brt_lock);
 }
 
 static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
@@ -1601,6 +1655,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 +1773,12 @@ 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);
+	mutex_lock(&asus_brt_lock);
+	asus_brt_ref = NULL;
+	if (asus->kbd_led_registered)
+		led_classdev_unregister(&asus->kbd_led);
+	mutex_unlock(&asus_brt_lock);
+
 	led_classdev_unregister(&asus->tpd_led);
 	led_classdev_unregister(&asus->wlan_led);
 	led_classdev_unregister(&asus->lightbar_led);
@@ -1730,6 +1792,7 @@ 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;
+	bool has_listeners;
 
 	if (asus->kbd_rgb_dev)
 		kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;
@@ -1754,24 +1817,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;
+	mutex_lock(&asus_brt_lock);
+	has_listeners = !list_empty(&asus_brt_listeners);
+	mutex_unlock(&asus_brt_lock);
 
+	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;
 	}
 
+	mutex_lock(&asus_brt_lock);
+	asus_brt_ref = asus;
+	mutex_unlock(&asus_brt_lock);
+
 	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);
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 783e2a336861b..42e963b70acdb 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -157,14 +157,30 @@
 #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK	0x0000FF00
 #define ASUS_WMI_DSTS_LIGHTBAR_MASK	0x0000000F
 
+struct asus_brt_listener {
+	struct list_head list;
+	void (*notify)(struct asus_brt_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_brt_register_listener(struct asus_brt_listener *cdev);
+void asus_brt_unregister_listener(struct asus_brt_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_brt_register_listener(struct asus_brt_listener *bdev)
+{
+	return -ENODEV;
+}
+static inline void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
+{
+}
 #endif
 
 /* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
-- 
2.48.1

Re: [PATCH 05/11] platform/x86: asus-wmi: Add support for multiple kbd RGB handlers

[Luke D. Jones]

On 21/03/25 11:09, Antheas Kapenekakis wrote:
> 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 <lkml@antheas.dev>
> ---
>   drivers/platform/x86/asus-wmi.c            | 100 ++++++++++++++++++---
>   include/linux/platform_data/x86/asus-wmi.h |  16 ++++
>   2 files changed, 104 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 38ef778e8c19b..21e034be71b2f 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -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,46 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
>   
>   /* LEDs ***********************************************************************/
>   
> +LIST_HEAD(asus_brt_listeners);
> +DEFINE_MUTEX(asus_brt_lock);
> +struct asus_wmi *asus_brt_ref;

Could these 3 items contained in a new static struct, it would make it 
easier to see the associations since they're a group.

> +int asus_brt_register_listener(struct asus_brt_listener *bdev)
> +{
> +	int ret;
> +
> +	mutex_lock(&asus_brt_lock);
> +	list_add_tail(&bdev->list, &asus_brt_listeners);
> +	if (asus_brt_ref) {

ret is not initialised if this is false

> +		if (asus_brt_ref->kbd_led_registered && asus_brt_ref->kbd_led_wk >= 0)
> +			bdev->notify(bdev, asus_brt_ref->kbd_led_wk);
> +		else {
> +			asus_brt_ref->kbd_led_registered = true;
> +			ret = led_classdev_register(
> +				&asus_brt_ref->platform_device->dev,
> +				&asus_brt_ref->kbd_led);
> +		}
> +	}
> +	mutex_unlock(&asus_brt_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(asus_brt_register_listener);
> +
> +void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
> +{
> +	mutex_lock(&asus_brt_lock);
> +	list_del(&bdev->list);
> +
> +	if (asus_brt_ref && asus_brt_ref->kbd_led_registered &&
> +	    list_empty(&asus_brt_listeners) && !asus_brt_ref->kbd_led_avail) {
> +		led_classdev_unregister(&asus_brt_ref->kbd_led);
> +		asus_brt_ref->kbd_led_registered = false;
> +	}
> +	mutex_unlock(&asus_brt_lock);
> +}
> +EXPORT_SYMBOL_GPL(asus_brt_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 +1608,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_brt_listener *listener;
>   	struct asus_wmi *asus;
>   	int max_level;
>   
> @@ -1573,7 +1616,12 @@ 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_brt_listeners, list)
> +		listener->notify(listener, asus->kbd_led_wk);
>   }
>   
>   static void kbd_led_set(struct led_classdev *led_cdev,
> @@ -1583,15 +1631,21 @@ static void kbd_led_set(struct led_classdev *led_cdev,
>   	if (led_cdev->flags & LED_UNREGISTERING)
>   		return;
>   
> +	mutex_lock(&asus_brt_lock);
>   	do_kbd_led_set(led_cdev, value);
> +	mutex_unlock(&asus_brt_lock);
>   }
>   
>   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;
> +
> +	mutex_lock(&asus_brt_lock);
> +	led_cdev = &asus->kbd_led;
>   
>   	do_kbd_led_set(led_cdev, value);
>   	led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
> +	mutex_unlock(&asus_brt_lock);
>   }
>   
>   static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
> @@ -1601,6 +1655,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 +1773,12 @@ 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);
> +	mutex_lock(&asus_brt_lock);
> +	asus_brt_ref = NULL;
> +	if (asus->kbd_led_registered)
> +		led_classdev_unregister(&asus->kbd_led);
> +	mutex_unlock(&asus_brt_lock);
> +
>   	led_classdev_unregister(&asus->tpd_led);
>   	led_classdev_unregister(&asus->wlan_led);
>   	led_classdev_unregister(&asus->lightbar_led);
> @@ -1730,6 +1792,7 @@ 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;
> +	bool has_listeners;
>   
>   	if (asus->kbd_rgb_dev)
>   		kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;
> @@ -1754,24 +1817,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");

Okay so part of the reason the asus_use_hid_led_dmi_ids table was 
created is that some of those laptops had both WMI method, and HID 
method. The WMI method was given priority but on those laptops it didn't 
actually work. What was done was a sort of "blanket" use-hid. I don't 
know why ASUS did this.

> +	asus->kbd_led.name = "asus::kbd_backlight";

I'd like to see this changed to "asus:rgb:kbd_backlight" if RGB is 
supported but this might not be so feasible for the bulk of laptops. 
Given that the Z13 is using a new PID it may be okay there...

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

Per the comment 2x above we will get some laptops returning "yes I 
support this" even though it doesn't actually work. It raises two 
questions for me:
1. on machines where it *does* work and they also support HID, do we end 
up with a race condition?
2. what is the effect of that race?

I suspect we might need that quirk table still. Unfortunately I 
no-longer have a device where the WMI method was broken, but I will test 
on one 0x1866 device (2019) and a few 0x19b6

No other comments.

Cheers,
Luke.

> +
> +	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;
> +	mutex_lock(&asus_brt_lock);
> +	has_listeners = !list_empty(&asus_brt_listeners);
> +	mutex_unlock(&asus_brt_lock);
>   
> +	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;
>   	}
>   
> +	mutex_lock(&asus_brt_lock);
> +	asus_brt_ref = asus;
> +	mutex_unlock(&asus_brt_lock);
> +
>   	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);
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 783e2a336861b..42e963b70acdb 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -157,14 +157,30 @@
>   #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK	0x0000FF00
>   #define ASUS_WMI_DSTS_LIGHTBAR_MASK	0x0000000F
>   
> +struct asus_brt_listener {
> +	struct list_head list;
> +	void (*notify)(struct asus_brt_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_brt_register_listener(struct asus_brt_listener *cdev);
> +void asus_brt_unregister_listener(struct asus_brt_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_brt_register_listener(struct asus_brt_listener *bdev)
> +{
> +	return -ENODEV;
> +}
> +static inline void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
> +{
> +}
>   #endif
>   
>   /* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */

Re: [PATCH 05/11] platform/x86: asus-wmi: Add support for multiple kbd RGB handlers

[Antheas Kapenekakis]

On Sat, 22 Mar 2025 at 04:24, Luke D. Jones <luke@ljones.dev> wrote:
>
> On 21/03/25 11:09, Antheas Kapenekakis wrote:
> > 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 <lkml@antheas.dev>
> > ---
> >   drivers/platform/x86/asus-wmi.c            | 100 ++++++++++++++++++---
> >   include/linux/platform_data/x86/asus-wmi.h |  16 ++++
> >   2 files changed, 104 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> > index 38ef778e8c19b..21e034be71b2f 100644
> > --- a/drivers/platform/x86/asus-wmi.c
> > +++ b/drivers/platform/x86/asus-wmi.c
> > @@ -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,46 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
> >
> >   /* LEDs ***********************************************************************/
> >
> > +LIST_HEAD(asus_brt_listeners);
> > +DEFINE_MUTEX(asus_brt_lock);
> > +struct asus_wmi *asus_brt_ref;
>
> Could these 3 items contained in a new static struct, it would make it
> easier to see the associations since they're a group.

My V0 tried something like that, but it was messy. I will think about
it for V4. Let's do that when we decide the name as it will be hairy
to rebase both of those and I want to do it once.

> > +int asus_brt_register_listener(struct asus_brt_listener *bdev)
> > +{
> > +     int ret;
> > +
> > +     mutex_lock(&asus_brt_lock);
> > +     list_add_tail(&bdev->list, &asus_brt_listeners);
> > +     if (asus_brt_ref) {
>
> ret is not initialised if this is false

I already fixed that for V3.

> > +             if (asus_brt_ref->kbd_led_registered && asus_brt_ref->kbd_led_wk >= 0)

This nested && is a bug, I will fix that if I havent. We might end up
initializing twice.

> > +                     bdev->notify(bdev, asus_brt_ref->kbd_led_wk);
> > +             else {
> > +                     asus_brt_ref->kbd_led_registered = true;
> > +                     ret = led_classdev_register(
> > +                             &asus_brt_ref->platform_device->dev,
> > +                             &asus_brt_ref->kbd_led);
> > +             }

(2) If asus_wmi launched already before the usb devices but did not
support RGB, kbd_led_registered will be false but the struct will be
initialized. Therefore, we register the device here, and all works.

> > +     }
> > +     mutex_unlock(&asus_brt_lock);
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_brt_register_listener);
> > +
> > +void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
> > +{
> > +     mutex_lock(&asus_brt_lock);
> > +     list_del(&bdev->list);
> > +
> > +     if (asus_brt_ref && asus_brt_ref->kbd_led_registered &&
> > +         list_empty(&asus_brt_listeners) && !asus_brt_ref->kbd_led_avail) {
> > +             led_classdev_unregister(&asus_brt_ref->kbd_led);
> > +             asus_brt_ref->kbd_led_registered = false;
> > +     }
> > +     mutex_unlock(&asus_brt_lock);
> > +}
> > +EXPORT_SYMBOL_GPL(asus_brt_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 +1608,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_brt_listener *listener;
> >       struct asus_wmi *asus;
> >       int max_level;
> >
> > @@ -1573,7 +1616,12 @@ 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_brt_listeners, list)
> > +             listener->notify(listener, asus->kbd_led_wk);
> >   }
> >
> >   static void kbd_led_set(struct led_classdev *led_cdev,
> > @@ -1583,15 +1631,21 @@ static void kbd_led_set(struct led_classdev *led_cdev,
> >       if (led_cdev->flags & LED_UNREGISTERING)
> >               return;
> >
> > +     mutex_lock(&asus_brt_lock);
> >       do_kbd_led_set(led_cdev, value);
> > +     mutex_unlock(&asus_brt_lock);
> >   }
> >
> >   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;
> > +
> > +     mutex_lock(&asus_brt_lock);
> > +     led_cdev = &asus->kbd_led;
> >
> >       do_kbd_led_set(led_cdev, value);
> >       led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
> > +     mutex_unlock(&asus_brt_lock);
> >   }
> >
> >   static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
> > @@ -1601,6 +1655,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 +1773,12 @@ 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);
> > +     mutex_lock(&asus_brt_lock);
> > +     asus_brt_ref = NULL;
> > +     if (asus->kbd_led_registered)
> > +             led_classdev_unregister(&asus->kbd_led);
> > +     mutex_unlock(&asus_brt_lock);
> > +
> >       led_classdev_unregister(&asus->tpd_led);
> >       led_classdev_unregister(&asus->wlan_led);
> >       led_classdev_unregister(&asus->lightbar_led);
> > @@ -1730,6 +1792,7 @@ 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;
> > +     bool has_listeners;
> >
> >       if (asus->kbd_rgb_dev)
> >               kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;
> > @@ -1754,24 +1817,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");
>
> Okay so part of the reason the asus_use_hid_led_dmi_ids table was
> created is that some of those laptops had both WMI method, and HID
> method. The WMI method was given priority but on those laptops it didn't
> actually work. What was done was a sort of "blanket" use-hid. I don't
> know why ASUS did this.
>
> > +     asus->kbd_led.name = "asus::kbd_backlight";
>
> I'd like to see this changed to "asus:rgb:kbd_backlight" if RGB is
> supported but this might not be so feasible for the bulk of laptops.
> Given that the Z13 is using a new PID it may be okay there...

So for the Z13 it is not rgb, this is used just as the common
backlight handler for all rgb devices, so there is no multi-intensity.
The only devices that would be good for would be for those where
kbd_rgb_dev exists, and as this series tries to not affect them,
changing the name is out of scope.

> > +     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);
>
> Per the comment 2x above we will get some laptops returning "yes I
> support this" even though it doesn't actually work. It raises two
> questions for me:
> 1. on machines where it *does* work and they also support HID, do we end
> up with a race condition?
> 2. what is the effect of that race?
>
> I suspect we might need that quirk table still. Unfortunately I
> no-longer have a device where the WMI method was broken, but I will test
> on one 0x1866 device (2019) and a few 0x19b6
>
> No other comments.

We do not need a quirk anymore actually, the endpoint is created on
demand and there is no race condition. See (1) and (2). I almost gave
up twice writing this until i figured out how to remove the race
conditions.

> Cheers,
> Luke.
>
> > +
> > +     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;
> > +     mutex_lock(&asus_brt_lock);
> > +     has_listeners = !list_empty(&asus_brt_listeners);
> > +     mutex_unlock(&asus_brt_lock);
> >
> > +     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;
> >       }

(1) If asus-wmi launches first and supports the WMI endpoint,
kbd_led_avail is true so the device is created. If it does not support
it but there is a usb device, then has_listeners is true so it is
still created.

> >
> > +     mutex_lock(&asus_brt_lock);
> > +     asus_brt_ref = asus;
> > +     mutex_unlock(&asus_brt_lock);
> > +
> >       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);
> > diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> > index 783e2a336861b..42e963b70acdb 100644
> > --- a/include/linux/platform_data/x86/asus-wmi.h
> > +++ b/include/linux/platform_data/x86/asus-wmi.h
> > @@ -157,14 +157,30 @@
> >   #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK       0x0000FF00
> >   #define ASUS_WMI_DSTS_LIGHTBAR_MASK 0x0000000F
> >
> > +struct asus_brt_listener {
> > +     struct list_head list;
> > +     void (*notify)(struct asus_brt_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_brt_register_listener(struct asus_brt_listener *cdev);
> > +void asus_brt_unregister_listener(struct asus_brt_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_brt_register_listener(struct asus_brt_listener *bdev)
> > +{
> > +     return -ENODEV;
> > +}
> > +static inline void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
> > +{
> > +}
> >   #endif
> >
> >   /* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
>

Re: [PATCH 05/11] platform/x86: asus-wmi: Add support for multiple kbd RGB handlers

[Luke D. Jones]

On 22/03/25 21:06, Antheas Kapenekakis wrote:
> On Sat, 22 Mar 2025 at 04:24, Luke D. Jones <luke@ljones.dev> wrote:
>>
>> On 21/03/25 11:09, Antheas Kapenekakis wrote:
>>> 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 <lkml@antheas.dev>
>>> ---
>>>    drivers/platform/x86/asus-wmi.c            | 100 ++++++++++++++++++---
>>>    include/linux/platform_data/x86/asus-wmi.h |  16 ++++
>>>    2 files changed, 104 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>>> index 38ef778e8c19b..21e034be71b2f 100644
>>> --- a/drivers/platform/x86/asus-wmi.c
>>> +++ b/drivers/platform/x86/asus-wmi.c
>>> @@ -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,46 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
>>>
>>>    /* LEDs ***********************************************************************/
>>>
>>> +LIST_HEAD(asus_brt_listeners);
>>> +DEFINE_MUTEX(asus_brt_lock);
>>> +struct asus_wmi *asus_brt_ref;
>>
>> Could these 3 items contained in a new static struct, it would make it
>> easier to see the associations since they're a group.
> 
> My V0 tried something like that, but it was messy. I will think about
> it for V4. Let's do that when we decide the name as it will be hairy
> to rebase both of those and I want to do it once.
> 
>>> +int asus_brt_register_listener(struct asus_brt_listener *bdev)
>>> +{
>>> +     int ret;
>>> +
>>> +     mutex_lock(&asus_brt_lock);
>>> +     list_add_tail(&bdev->list, &asus_brt_listeners);
>>> +     if (asus_brt_ref) {
>>
>> ret is not initialised if this is false
> 
> I already fixed that for V3.
> 
>>> +             if (asus_brt_ref->kbd_led_registered && asus_brt_ref->kbd_led_wk >= 0)
> 
> This nested && is a bug, I will fix that if I havent. We might end up
> initializing twice.
> 
>>> +                     bdev->notify(bdev, asus_brt_ref->kbd_led_wk);
>>> +             else {
>>> +                     asus_brt_ref->kbd_led_registered = true;
>>> +                     ret = led_classdev_register(
>>> +                             &asus_brt_ref->platform_device->dev,
>>> +                             &asus_brt_ref->kbd_led);
>>> +             }
> 
> (2) If asus_wmi launched already before the usb devices but did not
> support RGB, kbd_led_registered will be false but the struct will be
> initialized. Therefore, we register the device here, and all works.
> 
>>> +     }
>>> +     mutex_unlock(&asus_brt_lock);
>>> +
>>> +     return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(asus_brt_register_listener);
>>> +
>>> +void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
>>> +{
>>> +     mutex_lock(&asus_brt_lock);
>>> +     list_del(&bdev->list);
>>> +
>>> +     if (asus_brt_ref && asus_brt_ref->kbd_led_registered &&
>>> +         list_empty(&asus_brt_listeners) && !asus_brt_ref->kbd_led_avail) {
>>> +             led_classdev_unregister(&asus_brt_ref->kbd_led);
>>> +             asus_brt_ref->kbd_led_registered = false;
>>> +     }
>>> +     mutex_unlock(&asus_brt_lock);
>>> +}
>>> +EXPORT_SYMBOL_GPL(asus_brt_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 +1608,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_brt_listener *listener;
>>>        struct asus_wmi *asus;
>>>        int max_level;
>>>
>>> @@ -1573,7 +1616,12 @@ 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_brt_listeners, list)
>>> +             listener->notify(listener, asus->kbd_led_wk);
>>>    }
>>>
>>>    static void kbd_led_set(struct led_classdev *led_cdev,
>>> @@ -1583,15 +1631,21 @@ static void kbd_led_set(struct led_classdev *led_cdev,
>>>        if (led_cdev->flags & LED_UNREGISTERING)
>>>                return;
>>>
>>> +     mutex_lock(&asus_brt_lock);
>>>        do_kbd_led_set(led_cdev, value);
>>> +     mutex_unlock(&asus_brt_lock);
>>>    }
>>>
>>>    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;
>>> +
>>> +     mutex_lock(&asus_brt_lock);
>>> +     led_cdev = &asus->kbd_led;
>>>
>>>        do_kbd_led_set(led_cdev, value);
>>>        led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
>>> +     mutex_unlock(&asus_brt_lock);
>>>    }
>>>
>>>    static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
>>> @@ -1601,6 +1655,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 +1773,12 @@ 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);
>>> +     mutex_lock(&asus_brt_lock);
>>> +     asus_brt_ref = NULL;
>>> +     if (asus->kbd_led_registered)
>>> +             led_classdev_unregister(&asus->kbd_led);
>>> +     mutex_unlock(&asus_brt_lock);
>>> +
>>>        led_classdev_unregister(&asus->tpd_led);
>>>        led_classdev_unregister(&asus->wlan_led);
>>>        led_classdev_unregister(&asus->lightbar_led);
>>> @@ -1730,6 +1792,7 @@ 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;
>>> +     bool has_listeners;
>>>
>>>        if (asus->kbd_rgb_dev)
>>>                kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;
>>> @@ -1754,24 +1817,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");
>>
>> Okay so part of the reason the asus_use_hid_led_dmi_ids table was
>> created is that some of those laptops had both WMI method, and HID
>> method. The WMI method was given priority but on those laptops it didn't
>> actually work. What was done was a sort of "blanket" use-hid. I don't
>> know why ASUS did this.
>>
>>> +     asus->kbd_led.name = "asus::kbd_backlight";
>>
>> I'd like to see this changed to "asus:rgb:kbd_backlight" if RGB is
>> supported but this might not be so feasible for the bulk of laptops.
>> Given that the Z13 is using a new PID it may be okay there...
> 
> So for the Z13 it is not rgb, this is used just as the common
> backlight handler for all rgb devices, so there is no multi-intensity.
> The only devices that would be good for would be for those where
> kbd_rgb_dev exists, and as this series tries to not affect them,
> changing the name is out of scope.
> 
>>> +     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);
>>
>> Per the comment 2x above we will get some laptops returning "yes I
>> support this" even though it doesn't actually work. It raises two
>> questions for me:
>> 1. on machines where it *does* work and they also support HID, do we end
>> up with a race condition?
>> 2. what is the effect of that race?
>>
>> I suspect we might need that quirk table still. Unfortunately I
>> no-longer have a device where the WMI method was broken, but I will test
>> on one 0x1866 device (2019) and a few 0x19b6
>>
>> No other comments.
> 
> We do not need a quirk anymore actually, the endpoint is created on
> demand and there is no race condition. See (1) and (2). I almost gave
> up twice writing this until i figured out how to remove the race
> conditions.
> 
>> Cheers,
>> Luke.
>>
>>> +
>>> +     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;
>>> +     mutex_lock(&asus_brt_lock);
>>> +     has_listeners = !list_empty(&asus_brt_listeners);
>>> +     mutex_unlock(&asus_brt_lock);
>>>
>>> +     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;
>>>        }
> 
> (1) If asus-wmi launches first and supports the WMI endpoint,
> kbd_led_avail is true so the device is created. If it does not support
> it but there is a usb device, then has_listeners is true so it is
> still created.

The problem is that there are a small group of laptops that report the 
WMI method as supported, but when used it actually does nothing, so they 
need to be quirked. These are the ones that will regress, the GU605M is 
one, I think the ProArt series was another as they use the same build 
base as the GU605.

If both are created then hopefully it's a non-issue since the WMI 
endpoint is a no-op for set. But what about when both HID and WMI work 
and both are created?

>>>
>>> +     mutex_lock(&asus_brt_lock);
>>> +     asus_brt_ref = asus;
>>> +     mutex_unlock(&asus_brt_lock);
>>> +
>>>        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);
>>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
>>> index 783e2a336861b..42e963b70acdb 100644
>>> --- a/include/linux/platform_data/x86/asus-wmi.h
>>> +++ b/include/linux/platform_data/x86/asus-wmi.h
>>> @@ -157,14 +157,30 @@
>>>    #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK       0x0000FF00
>>>    #define ASUS_WMI_DSTS_LIGHTBAR_MASK 0x0000000F
>>>
>>> +struct asus_brt_listener {
>>> +     struct list_head list;
>>> +     void (*notify)(struct asus_brt_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_brt_register_listener(struct asus_brt_listener *cdev);
>>> +void asus_brt_unregister_listener(struct asus_brt_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_brt_register_listener(struct asus_brt_listener *bdev)
>>> +{
>>> +     return -ENODEV;
>>> +}
>>> +static inline void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
>>> +{
>>> +}
>>>    #endif
>>>
>>>    /* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
>>

Re: [PATCH 05/11] platform/x86: asus-wmi: Add support for multiple kbd RGB handlers

[Antheas Kapenekakis]

On Sat, 22 Mar 2025 at 09:57, Luke D. Jones <luke@ljones.dev> wrote:
>
> On 22/03/25 21:06, Antheas Kapenekakis wrote:
> > On Sat, 22 Mar 2025 at 04:24, Luke D. Jones <luke@ljones.dev> wrote:
> >>
> >> On 21/03/25 11:09, Antheas Kapenekakis wrote:
> >>> 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 <lkml@antheas.dev>
> >>> ---
> >>>    drivers/platform/x86/asus-wmi.c            | 100 ++++++++++++++++++---
> >>>    include/linux/platform_data/x86/asus-wmi.h |  16 ++++
> >>>    2 files changed, 104 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> >>> index 38ef778e8c19b..21e034be71b2f 100644
> >>> --- a/drivers/platform/x86/asus-wmi.c
> >>> +++ b/drivers/platform/x86/asus-wmi.c
> >>> @@ -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,46 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
> >>>
> >>>    /* LEDs ***********************************************************************/
> >>>
> >>> +LIST_HEAD(asus_brt_listeners);
> >>> +DEFINE_MUTEX(asus_brt_lock);
> >>> +struct asus_wmi *asus_brt_ref;
> >>
> >> Could these 3 items contained in a new static struct, it would make it
> >> easier to see the associations since they're a group.
> >
> > My V0 tried something like that, but it was messy. I will think about
> > it for V4. Let's do that when we decide the name as it will be hairy
> > to rebase both of those and I want to do it once.
> >
> >>> +int asus_brt_register_listener(struct asus_brt_listener *bdev)
> >>> +{
> >>> +     int ret;
> >>> +
> >>> +     mutex_lock(&asus_brt_lock);
> >>> +     list_add_tail(&bdev->list, &asus_brt_listeners);
> >>> +     if (asus_brt_ref) {
> >>
> >> ret is not initialised if this is false
> >
> > I already fixed that for V3.
> >
> >>> +             if (asus_brt_ref->kbd_led_registered && asus_brt_ref->kbd_led_wk >= 0)
> >
> > This nested && is a bug, I will fix that if I havent. We might end up
> > initializing twice.
> >
> >>> +                     bdev->notify(bdev, asus_brt_ref->kbd_led_wk);
> >>> +             else {
> >>> +                     asus_brt_ref->kbd_led_registered = true;
> >>> +                     ret = led_classdev_register(
> >>> +                             &asus_brt_ref->platform_device->dev,
> >>> +                             &asus_brt_ref->kbd_led);
> >>> +             }
> >
> > (2) If asus_wmi launched already before the usb devices but did not
> > support RGB, kbd_led_registered will be false but the struct will be
> > initialized. Therefore, we register the device here, and all works.
> >
> >>> +     }
> >>> +     mutex_unlock(&asus_brt_lock);
> >>> +
> >>> +     return ret;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(asus_brt_register_listener);
> >>> +
> >>> +void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
> >>> +{
> >>> +     mutex_lock(&asus_brt_lock);
> >>> +     list_del(&bdev->list);
> >>> +
> >>> +     if (asus_brt_ref && asus_brt_ref->kbd_led_registered &&
> >>> +         list_empty(&asus_brt_listeners) && !asus_brt_ref->kbd_led_avail) {
> >>> +             led_classdev_unregister(&asus_brt_ref->kbd_led);
> >>> +             asus_brt_ref->kbd_led_registered = false;
> >>> +     }
> >>> +     mutex_unlock(&asus_brt_lock);
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(asus_brt_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 +1608,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_brt_listener *listener;
> >>>        struct asus_wmi *asus;
> >>>        int max_level;
> >>>
> >>> @@ -1573,7 +1616,12 @@ 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_brt_listeners, list)
> >>> +             listener->notify(listener, asus->kbd_led_wk);
> >>>    }
> >>>
> >>>    static void kbd_led_set(struct led_classdev *led_cdev,
> >>> @@ -1583,15 +1631,21 @@ static void kbd_led_set(struct led_classdev *led_cdev,
> >>>        if (led_cdev->flags & LED_UNREGISTERING)
> >>>                return;
> >>>
> >>> +     mutex_lock(&asus_brt_lock);
> >>>        do_kbd_led_set(led_cdev, value);
> >>> +     mutex_unlock(&asus_brt_lock);
> >>>    }
> >>>
> >>>    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;
> >>> +
> >>> +     mutex_lock(&asus_brt_lock);
> >>> +     led_cdev = &asus->kbd_led;
> >>>
> >>>        do_kbd_led_set(led_cdev, value);
> >>>        led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
> >>> +     mutex_unlock(&asus_brt_lock);
> >>>    }
> >>>
> >>>    static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
> >>> @@ -1601,6 +1655,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 +1773,12 @@ 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);
> >>> +     mutex_lock(&asus_brt_lock);
> >>> +     asus_brt_ref = NULL;
> >>> +     if (asus->kbd_led_registered)
> >>> +             led_classdev_unregister(&asus->kbd_led);
> >>> +     mutex_unlock(&asus_brt_lock);
> >>> +
> >>>        led_classdev_unregister(&asus->tpd_led);
> >>>        led_classdev_unregister(&asus->wlan_led);
> >>>        led_classdev_unregister(&asus->lightbar_led);
> >>> @@ -1730,6 +1792,7 @@ 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;
> >>> +     bool has_listeners;
> >>>
> >>>        if (asus->kbd_rgb_dev)
> >>>                kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;
> >>> @@ -1754,24 +1817,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");
> >>
> >> Okay so part of the reason the asus_use_hid_led_dmi_ids table was
> >> created is that some of those laptops had both WMI method, and HID
> >> method. The WMI method was given priority but on those laptops it didn't
> >> actually work. What was done was a sort of "blanket" use-hid. I don't
> >> know why ASUS did this.
> >>
> >>> +     asus->kbd_led.name = "asus::kbd_backlight";
> >>
> >> I'd like to see this changed to "asus:rgb:kbd_backlight" if RGB is
> >> supported but this might not be so feasible for the bulk of laptops.
> >> Given that the Z13 is using a new PID it may be okay there...
> >
> > So for the Z13 it is not rgb, this is used just as the common
> > backlight handler for all rgb devices, so there is no multi-intensity.
> > The only devices that would be good for would be for those where
> > kbd_rgb_dev exists, and as this series tries to not affect them,
> > changing the name is out of scope.
> >
> >>> +     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);
> >>
> >> Per the comment 2x above we will get some laptops returning "yes I
> >> support this" even though it doesn't actually work. It raises two
> >> questions for me:
> >> 1. on machines where it *does* work and they also support HID, do we end
> >> up with a race condition?
> >> 2. what is the effect of that race?
> >>
> >> I suspect we might need that quirk table still. Unfortunately I
> >> no-longer have a device where the WMI method was broken, but I will test
> >> on one 0x1866 device (2019) and a few 0x19b6
> >>
> >> No other comments.
> >
> > We do not need a quirk anymore actually, the endpoint is created on
> > demand and there is no race condition. See (1) and (2). I almost gave
> > up twice writing this until i figured out how to remove the race
> > conditions.
> >
> >> Cheers,
> >> Luke.
> >>
> >>> +
> >>> +     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;
> >>> +     mutex_lock(&asus_brt_lock);
> >>> +     has_listeners = !list_empty(&asus_brt_listeners);
> >>> +     mutex_unlock(&asus_brt_lock);
> >>>
> >>> +     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;
> >>>        }
> >
> > (1) If asus-wmi launches first and supports the WMI endpoint,
> > kbd_led_avail is true so the device is created. If it does not support
> > it but there is a usb device, then has_listeners is true so it is
> > still created.
>
> The problem is that there are a small group of laptops that report the
> WMI method as supported, but when used it actually does nothing, so they
> need to be quirked. These are the ones that will regress, the GU605M is
> one, I think the ProArt series was another as they use the same build
> base as the GU605.
>
> If both are created then hopefully it's a non-issue since the WMI
> endpoint is a no-op for set. But what about when both HID and WMI work
> and both are created?

This series is created with the assumption that they both do
something. Hopefully it is not NOOP for get, otherwise the brightness
might get stuck.

Currently there is a little branch in the code that uses the previous
brightness as a reference if a WMI handler is missing.

> >>>
> >>> +     mutex_lock(&asus_brt_lock);
> >>> +     asus_brt_ref = asus;
> >>> +     mutex_unlock(&asus_brt_lock);
> >>> +
> >>>        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);
> >>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> >>> index 783e2a336861b..42e963b70acdb 100644
> >>> --- a/include/linux/platform_data/x86/asus-wmi.h
> >>> +++ b/include/linux/platform_data/x86/asus-wmi.h
> >>> @@ -157,14 +157,30 @@
> >>>    #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK       0x0000FF00
> >>>    #define ASUS_WMI_DSTS_LIGHTBAR_MASK 0x0000000F
> >>>
> >>> +struct asus_brt_listener {
> >>> +     struct list_head list;
> >>> +     void (*notify)(struct asus_brt_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_brt_register_listener(struct asus_brt_listener *cdev);
> >>> +void asus_brt_unregister_listener(struct asus_brt_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_brt_register_listener(struct asus_brt_listener *bdev)
> >>> +{
> >>> +     return -ENODEV;
> >>> +}
> >>> +static inline void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
> >>> +{
> >>> +}
> >>>    #endif
> >>>
> >>>    /* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
> >>
>

Re: [PATCH 05/11] platform/x86: asus-wmi: Add support for multiple kbd RGB handlers

[Antheas Kapenekakis]

On Sat, 22 Mar 2025 at 10:06, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>
> On Sat, 22 Mar 2025 at 09:57, Luke D. Jones <luke@ljones.dev> wrote:
> >
> > On 22/03/25 21:06, Antheas Kapenekakis wrote:
> > > On Sat, 22 Mar 2025 at 04:24, Luke D. Jones <luke@ljones.dev> wrote:
> > >>
> > >> On 21/03/25 11:09, Antheas Kapenekakis wrote:
> > >>> 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 <lkml@antheas.dev>
> > >>> ---
> > >>>    drivers/platform/x86/asus-wmi.c            | 100 ++++++++++++++++++---
> > >>>    include/linux/platform_data/x86/asus-wmi.h |  16 ++++
> > >>>    2 files changed, 104 insertions(+), 12 deletions(-)
> > >>>
> > >>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> > >>> index 38ef778e8c19b..21e034be71b2f 100644
> > >>> --- a/drivers/platform/x86/asus-wmi.c
> > >>> +++ b/drivers/platform/x86/asus-wmi.c
> > >>> @@ -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,46 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
> > >>>
> > >>>    /* LEDs ***********************************************************************/
> > >>>
> > >>> +LIST_HEAD(asus_brt_listeners);
> > >>> +DEFINE_MUTEX(asus_brt_lock);
> > >>> +struct asus_wmi *asus_brt_ref;
> > >>
> > >> Could these 3 items contained in a new static struct, it would make it
> > >> easier to see the associations since they're a group.
> > >
> > > My V0 tried something like that, but it was messy. I will think about
> > > it for V4. Let's do that when we decide the name as it will be hairy
> > > to rebase both of those and I want to do it once.
> > >
> > >>> +int asus_brt_register_listener(struct asus_brt_listener *bdev)
> > >>> +{
> > >>> +     int ret;
> > >>> +
> > >>> +     mutex_lock(&asus_brt_lock);
> > >>> +     list_add_tail(&bdev->list, &asus_brt_listeners);
> > >>> +     if (asus_brt_ref) {
> > >>
> > >> ret is not initialised if this is false
> > >
> > > I already fixed that for V3.
> > >
> > >>> +             if (asus_brt_ref->kbd_led_registered && asus_brt_ref->kbd_led_wk >= 0)
> > >
> > > This nested && is a bug, I will fix that if I havent. We might end up
> > > initializing twice.
> > >
> > >>> +                     bdev->notify(bdev, asus_brt_ref->kbd_led_wk);
> > >>> +             else {
> > >>> +                     asus_brt_ref->kbd_led_registered = true;
> > >>> +                     ret = led_classdev_register(
> > >>> +                             &asus_brt_ref->platform_device->dev,
> > >>> +                             &asus_brt_ref->kbd_led);
> > >>> +             }
> > >
> > > (2) If asus_wmi launched already before the usb devices but did not
> > > support RGB, kbd_led_registered will be false but the struct will be
> > > initialized. Therefore, we register the device here, and all works.
> > >
> > >>> +     }
> > >>> +     mutex_unlock(&asus_brt_lock);
> > >>> +
> > >>> +     return ret;
> > >>> +}
> > >>> +EXPORT_SYMBOL_GPL(asus_brt_register_listener);
> > >>> +
> > >>> +void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
> > >>> +{
> > >>> +     mutex_lock(&asus_brt_lock);
> > >>> +     list_del(&bdev->list);
> > >>> +
> > >>> +     if (asus_brt_ref && asus_brt_ref->kbd_led_registered &&
> > >>> +         list_empty(&asus_brt_listeners) && !asus_brt_ref->kbd_led_avail) {
> > >>> +             led_classdev_unregister(&asus_brt_ref->kbd_led);
> > >>> +             asus_brt_ref->kbd_led_registered = false;
> > >>> +     }
> > >>> +     mutex_unlock(&asus_brt_lock);
> > >>> +}
> > >>> +EXPORT_SYMBOL_GPL(asus_brt_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 +1608,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_brt_listener *listener;
> > >>>        struct asus_wmi *asus;
> > >>>        int max_level;
> > >>>
> > >>> @@ -1573,7 +1616,12 @@ 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_brt_listeners, list)
> > >>> +             listener->notify(listener, asus->kbd_led_wk);
> > >>>    }
> > >>>
> > >>>    static void kbd_led_set(struct led_classdev *led_cdev,
> > >>> @@ -1583,15 +1631,21 @@ static void kbd_led_set(struct led_classdev *led_cdev,
> > >>>        if (led_cdev->flags & LED_UNREGISTERING)
> > >>>                return;
> > >>>
> > >>> +     mutex_lock(&asus_brt_lock);
> > >>>        do_kbd_led_set(led_cdev, value);
> > >>> +     mutex_unlock(&asus_brt_lock);
> > >>>    }
> > >>>
> > >>>    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;
> > >>> +
> > >>> +     mutex_lock(&asus_brt_lock);
> > >>> +     led_cdev = &asus->kbd_led;
> > >>>
> > >>>        do_kbd_led_set(led_cdev, value);
> > >>>        led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
> > >>> +     mutex_unlock(&asus_brt_lock);
> > >>>    }
> > >>>
> > >>>    static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
> > >>> @@ -1601,6 +1655,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 +1773,12 @@ 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);
> > >>> +     mutex_lock(&asus_brt_lock);
> > >>> +     asus_brt_ref = NULL;
> > >>> +     if (asus->kbd_led_registered)
> > >>> +             led_classdev_unregister(&asus->kbd_led);
> > >>> +     mutex_unlock(&asus_brt_lock);
> > >>> +
> > >>>        led_classdev_unregister(&asus->tpd_led);
> > >>>        led_classdev_unregister(&asus->wlan_led);
> > >>>        led_classdev_unregister(&asus->lightbar_led);
> > >>> @@ -1730,6 +1792,7 @@ 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;
> > >>> +     bool has_listeners;
> > >>>
> > >>>        if (asus->kbd_rgb_dev)
> > >>>                kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;
> > >>> @@ -1754,24 +1817,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");
> > >>
> > >> Okay so part of the reason the asus_use_hid_led_dmi_ids table was
> > >> created is that some of those laptops had both WMI method, and HID
> > >> method. The WMI method was given priority but on those laptops it didn't
> > >> actually work. What was done was a sort of "blanket" use-hid. I don't
> > >> know why ASUS did this.
> > >>
> > >>> +     asus->kbd_led.name = "asus::kbd_backlight";
> > >>
> > >> I'd like to see this changed to "asus:rgb:kbd_backlight" if RGB is
> > >> supported but this might not be so feasible for the bulk of laptops.
> > >> Given that the Z13 is using a new PID it may be okay there...
> > >
> > > So for the Z13 it is not rgb, this is used just as the common
> > > backlight handler for all rgb devices, so there is no multi-intensity.
> > > The only devices that would be good for would be for those where
> > > kbd_rgb_dev exists, and as this series tries to not affect them,
> > > changing the name is out of scope.
> > >
> > >>> +     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);
> > >>
> > >> Per the comment 2x above we will get some laptops returning "yes I
> > >> support this" even though it doesn't actually work. It raises two
> > >> questions for me:
> > >> 1. on machines where it *does* work and they also support HID, do we end
> > >> up with a race condition?
> > >> 2. what is the effect of that race?
> > >>
> > >> I suspect we might need that quirk table still. Unfortunately I
> > >> no-longer have a device where the WMI method was broken, but I will test
> > >> on one 0x1866 device (2019) and a few 0x19b6
> > >>
> > >> No other comments.
> > >
> > > We do not need a quirk anymore actually, the endpoint is created on
> > > demand and there is no race condition. See (1) and (2). I almost gave
> > > up twice writing this until i figured out how to remove the race
> > > conditions.
> > >
> > >> Cheers,
> > >> Luke.
> > >>
> > >>> +
> > >>> +     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;
> > >>> +     mutex_lock(&asus_brt_lock);
> > >>> +     has_listeners = !list_empty(&asus_brt_listeners);
> > >>> +     mutex_unlock(&asus_brt_lock);
> > >>>
> > >>> +     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;
> > >>>        }
> > >
> > > (1) If asus-wmi launches first and supports the WMI endpoint,
> > > kbd_led_avail is true so the device is created. If it does not support
> > > it but there is a usb device, then has_listeners is true so it is
> > > still created.
> >
> > The problem is that there are a small group of laptops that report the
> > WMI method as supported, but when used it actually does nothing, so they
> > need to be quirked. These are the ones that will regress, the GU605M is
> > one, I think the ProArt series was another as they use the same build
> > base as the GU605.
> >
> > If both are created then hopefully it's a non-issue since the WMI
> > endpoint is a no-op for set. But what about when both HID and WMI work
> > and both are created?
>
> This series is created with the assumption that they both do
> something. Hopefully it is not NOOP for get, otherwise the brightness
> might get stuck.
>
> Currently there is a little branch in the code that uses the previous
> brightness as a reference if a WMI handler is missing.

By that I mean that it does not matter if it does nothing, it will
just be called.

> > >>>
> > >>> +     mutex_lock(&asus_brt_lock);
> > >>> +     asus_brt_ref = asus;
> > >>> +     mutex_unlock(&asus_brt_lock);
> > >>> +
> > >>>        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);
> > >>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> > >>> index 783e2a336861b..42e963b70acdb 100644
> > >>> --- a/include/linux/platform_data/x86/asus-wmi.h
> > >>> +++ b/include/linux/platform_data/x86/asus-wmi.h
> > >>> @@ -157,14 +157,30 @@
> > >>>    #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK       0x0000FF00
> > >>>    #define ASUS_WMI_DSTS_LIGHTBAR_MASK 0x0000000F
> > >>>
> > >>> +struct asus_brt_listener {
> > >>> +     struct list_head list;
> > >>> +     void (*notify)(struct asus_brt_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_brt_register_listener(struct asus_brt_listener *cdev);
> > >>> +void asus_brt_unregister_listener(struct asus_brt_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_brt_register_listener(struct asus_brt_listener *bdev)
> > >>> +{
> > >>> +     return -ENODEV;
> > >>> +}
> > >>> +static inline void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
> > >>> +{
> > >>> +}
> > >>>    #endif
> > >>>
> > >>>    /* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
> > >>
> >

Re: [PATCH 05/11] platform/x86: asus-wmi: Add support for multiple kbd RGB handlers

[Luke D. Jones]

On 22/03/25 22:06, Antheas Kapenekakis wrote:
> On Sat, 22 Mar 2025 at 09:57, Luke D. Jones <luke@ljones.dev> wrote:
>>
>> On 22/03/25 21:06, Antheas Kapenekakis wrote:
>>> On Sat, 22 Mar 2025 at 04:24, Luke D. Jones <luke@ljones.dev> wrote:
>>>>
>>>> On 21/03/25 11:09, Antheas Kapenekakis wrote:
>>>>> 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 <lkml@antheas.dev>
>>>>> ---
>>>>>     drivers/platform/x86/asus-wmi.c            | 100 ++++++++++++++++++---
>>>>>     include/linux/platform_data/x86/asus-wmi.h |  16 ++++
>>>>>     2 files changed, 104 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>>>>> index 38ef778e8c19b..21e034be71b2f 100644
>>>>> --- a/drivers/platform/x86/asus-wmi.c
>>>>> +++ b/drivers/platform/x86/asus-wmi.c
>>>>> @@ -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,46 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
>>>>>
>>>>>     /* LEDs ***********************************************************************/
>>>>>
>>>>> +LIST_HEAD(asus_brt_listeners);
>>>>> +DEFINE_MUTEX(asus_brt_lock);
>>>>> +struct asus_wmi *asus_brt_ref;
>>>>
>>>> Could these 3 items contained in a new static struct, it would make it
>>>> easier to see the associations since they're a group.
>>>
>>> My V0 tried something like that, but it was messy. I will think about
>>> it for V4. Let's do that when we decide the name as it will be hairy
>>> to rebase both of those and I want to do it once.
>>>
>>>>> +int asus_brt_register_listener(struct asus_brt_listener *bdev)
>>>>> +{
>>>>> +     int ret;
>>>>> +
>>>>> +     mutex_lock(&asus_brt_lock);
>>>>> +     list_add_tail(&bdev->list, &asus_brt_listeners);
>>>>> +     if (asus_brt_ref) {
>>>>
>>>> ret is not initialised if this is false
>>>
>>> I already fixed that for V3.
>>>
>>>>> +             if (asus_brt_ref->kbd_led_registered && asus_brt_ref->kbd_led_wk >= 0)
>>>
>>> This nested && is a bug, I will fix that if I havent. We might end up
>>> initializing twice.
>>>
>>>>> +                     bdev->notify(bdev, asus_brt_ref->kbd_led_wk);
>>>>> +             else {
>>>>> +                     asus_brt_ref->kbd_led_registered = true;
>>>>> +                     ret = led_classdev_register(
>>>>> +                             &asus_brt_ref->platform_device->dev,
>>>>> +                             &asus_brt_ref->kbd_led);
>>>>> +             }
>>>
>>> (2) If asus_wmi launched already before the usb devices but did not
>>> support RGB, kbd_led_registered will be false but the struct will be
>>> initialized. Therefore, we register the device here, and all works.
>>>
>>>>> +     }
>>>>> +     mutex_unlock(&asus_brt_lock);
>>>>> +
>>>>> +     return ret;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(asus_brt_register_listener);
>>>>> +
>>>>> +void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
>>>>> +{
>>>>> +     mutex_lock(&asus_brt_lock);
>>>>> +     list_del(&bdev->list);
>>>>> +
>>>>> +     if (asus_brt_ref && asus_brt_ref->kbd_led_registered &&
>>>>> +         list_empty(&asus_brt_listeners) && !asus_brt_ref->kbd_led_avail) {
>>>>> +             led_classdev_unregister(&asus_brt_ref->kbd_led);
>>>>> +             asus_brt_ref->kbd_led_registered = false;
>>>>> +     }
>>>>> +     mutex_unlock(&asus_brt_lock);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(asus_brt_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 +1608,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_brt_listener *listener;
>>>>>         struct asus_wmi *asus;
>>>>>         int max_level;
>>>>>
>>>>> @@ -1573,7 +1616,12 @@ 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_brt_listeners, list)
>>>>> +             listener->notify(listener, asus->kbd_led_wk);
>>>>>     }
>>>>>
>>>>>     static void kbd_led_set(struct led_classdev *led_cdev,
>>>>> @@ -1583,15 +1631,21 @@ static void kbd_led_set(struct led_classdev *led_cdev,
>>>>>         if (led_cdev->flags & LED_UNREGISTERING)
>>>>>                 return;
>>>>>
>>>>> +     mutex_lock(&asus_brt_lock);
>>>>>         do_kbd_led_set(led_cdev, value);
>>>>> +     mutex_unlock(&asus_brt_lock);
>>>>>     }
>>>>>
>>>>>     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;
>>>>> +
>>>>> +     mutex_lock(&asus_brt_lock);
>>>>> +     led_cdev = &asus->kbd_led;
>>>>>
>>>>>         do_kbd_led_set(led_cdev, value);
>>>>>         led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
>>>>> +     mutex_unlock(&asus_brt_lock);
>>>>>     }
>>>>>
>>>>>     static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
>>>>> @@ -1601,6 +1655,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 +1773,12 @@ 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);
>>>>> +     mutex_lock(&asus_brt_lock);
>>>>> +     asus_brt_ref = NULL;
>>>>> +     if (asus->kbd_led_registered)
>>>>> +             led_classdev_unregister(&asus->kbd_led);
>>>>> +     mutex_unlock(&asus_brt_lock);
>>>>> +
>>>>>         led_classdev_unregister(&asus->tpd_led);
>>>>>         led_classdev_unregister(&asus->wlan_led);
>>>>>         led_classdev_unregister(&asus->lightbar_led);
>>>>> @@ -1730,6 +1792,7 @@ 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;
>>>>> +     bool has_listeners;
>>>>>
>>>>>         if (asus->kbd_rgb_dev)
>>>>>                 kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;
>>>>> @@ -1754,24 +1817,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");
>>>>
>>>> Okay so part of the reason the asus_use_hid_led_dmi_ids table was
>>>> created is that some of those laptops had both WMI method, and HID
>>>> method. The WMI method was given priority but on those laptops it didn't
>>>> actually work. What was done was a sort of "blanket" use-hid. I don't
>>>> know why ASUS did this.
>>>>
>>>>> +     asus->kbd_led.name = "asus::kbd_backlight";
>>>>
>>>> I'd like to see this changed to "asus:rgb:kbd_backlight" if RGB is
>>>> supported but this might not be so feasible for the bulk of laptops.
>>>> Given that the Z13 is using a new PID it may be okay there...
>>>
>>> So for the Z13 it is not rgb, this is used just as the common
>>> backlight handler for all rgb devices, so there is no multi-intensity.
>>> The only devices that would be good for would be for those where
>>> kbd_rgb_dev exists, and as this series tries to not affect them,
>>> changing the name is out of scope.
>>>
>>>>> +     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);
>>>>
>>>> Per the comment 2x above we will get some laptops returning "yes I
>>>> support this" even though it doesn't actually work. It raises two
>>>> questions for me:
>>>> 1. on machines where it *does* work and they also support HID, do we end
>>>> up with a race condition?
>>>> 2. what is the effect of that race?
>>>>
>>>> I suspect we might need that quirk table still. Unfortunately I
>>>> no-longer have a device where the WMI method was broken, but I will test
>>>> on one 0x1866 device (2019) and a few 0x19b6
>>>>
>>>> No other comments.
>>>
>>> We do not need a quirk anymore actually, the endpoint is created on
>>> demand and there is no race condition. See (1) and (2). I almost gave
>>> up twice writing this until i figured out how to remove the race
>>> conditions.
>>>
>>>> Cheers,
>>>> Luke.
>>>>
>>>>> +
>>>>> +     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;
>>>>> +     mutex_lock(&asus_brt_lock);
>>>>> +     has_listeners = !list_empty(&asus_brt_listeners);
>>>>> +     mutex_unlock(&asus_brt_lock);
>>>>>
>>>>> +     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;
>>>>>         }
>>>
>>> (1) If asus-wmi launches first and supports the WMI endpoint,
>>> kbd_led_avail is true so the device is created. If it does not support
>>> it but there is a usb device, then has_listeners is true so it is
>>> still created.
>>
>> The problem is that there are a small group of laptops that report the
>> WMI method as supported, but when used it actually does nothing, so they
>> need to be quirked. These are the ones that will regress, the GU605M is
>> one, I think the ProArt series was another as they use the same build
>> base as the GU605.
>>
>> If both are created then hopefully it's a non-issue since the WMI
>> endpoint is a no-op for set. But what about when both HID and WMI work
>> and both are created?
> 
> This series is created with the assumption that they both do
> something. Hopefully it is not NOOP for get, otherwise the brightness
> might get stuck.
> 
> Currently there is a little branch in the code that uses the previous
> brightness as a reference if a WMI handler is missing.

I will try to get a DSDT dump for the GU605. That should give some 
insight for what to expect.

>>>>>
>>>>> +     mutex_lock(&asus_brt_lock);
>>>>> +     asus_brt_ref = asus;
>>>>> +     mutex_unlock(&asus_brt_lock);
>>>>> +
>>>>>         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);
>>>>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
>>>>> index 783e2a336861b..42e963b70acdb 100644
>>>>> --- a/include/linux/platform_data/x86/asus-wmi.h
>>>>> +++ b/include/linux/platform_data/x86/asus-wmi.h
>>>>> @@ -157,14 +157,30 @@
>>>>>     #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK       0x0000FF00
>>>>>     #define ASUS_WMI_DSTS_LIGHTBAR_MASK 0x0000000F
>>>>>
>>>>> +struct asus_brt_listener {
>>>>> +     struct list_head list;
>>>>> +     void (*notify)(struct asus_brt_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_brt_register_listener(struct asus_brt_listener *cdev);
>>>>> +void asus_brt_unregister_listener(struct asus_brt_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_brt_register_listener(struct asus_brt_listener *bdev)
>>>>> +{
>>>>> +     return -ENODEV;
>>>>> +}
>>>>> +static inline void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
>>>>> +{
>>>>> +}
>>>>>     #endif
>>>>>
>>>>>     /* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
>>>>
>>

[PATCH 06/11] HID: asus: listen to the asus-wmi brightness device instead of creating one

[Antheas Kapenekakis]

Some ROG laptops expose multiple interfaces for controlling the
keyboard/RGB brightness. This creates a name conflict under
asus::kbd_brightness, where the second device ends up being
named asus::kbd_brightness_1 and they are both broken.

Therefore, register a listener to the asus-wmi brightness device
instead of creating a new one.

Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/hid/hid-asus.c | 65 +++++++-----------------------------------
 1 file changed, 11 insertions(+), 54 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index beb7df823b18e..b39d61b31ff23 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -96,7 +96,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 #define TRKID_SGN       ((TRKID_MAX + 1) >> 1)
 
 struct asus_kbd_leds {
-	struct led_classdev cdev;
+	struct asus_brt_listener listener;
 	struct hid_device *hdev;
 	struct work_struct work;
 	unsigned int brightness;
@@ -494,11 +494,11 @@ static void asus_schedule_work(struct asus_kbd_leds *led)
 	spin_unlock_irqrestore(&led->lock, flags);
 }
 
-static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
-				   enum led_brightness brightness)
+static void asus_kbd_backlight_set(struct asus_brt_listener *listener,
+				   int brightness)
 {
-	struct asus_kbd_leds *led = container_of(led_cdev, struct asus_kbd_leds,
-						 cdev);
+	struct asus_kbd_leds *led = container_of(listener, struct asus_kbd_leds,
+						 listener);
 	unsigned long flags;
 
 	spin_lock_irqsave(&led->lock, flags);
@@ -508,20 +508,6 @@ static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
 	asus_schedule_work(led);
 }
 
-static enum led_brightness asus_kbd_backlight_get(struct led_classdev *led_cdev)
-{
-	struct asus_kbd_leds *led = container_of(led_cdev, struct asus_kbd_leds,
-						 cdev);
-	enum led_brightness brightness;
-	unsigned long flags;
-
-	spin_lock_irqsave(&led->lock, flags);
-	brightness = led->brightness;
-	spin_unlock_irqrestore(&led->lock, flags);
-
-	return brightness;
-}
-
 static void asus_kbd_backlight_work(struct work_struct *work)
 {
 	struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work);
@@ -538,34 +524,6 @@ static void asus_kbd_backlight_work(struct work_struct *work)
 		hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret);
 }
 
-/* WMI-based keyboard backlight LED control (via asus-wmi driver) takes
- * precedence. We only activate HID-based backlight control when the
- * WMI control is not available.
- */
-static bool asus_kbd_wmi_led_control_present(struct hid_device *hdev)
-{
-	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
-	u32 value;
-	int ret;
-
-	if (!IS_ENABLED(CONFIG_ASUS_WMI))
-		return false;
-
-	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD &&
-			dmi_check_system(asus_use_hid_led_dmi_ids)) {
-		hid_info(hdev, "using HID for asus::kbd_backlight\n");
-		return false;
-	}
-
-	ret = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DSTS,
-				       ASUS_WMI_DEVID_KBD_BACKLIGHT, 0, &value);
-	hid_dbg(hdev, "WMI backlight check: rc %d value %x", ret, value);
-	if (ret)
-		return false;
-
-	return !!(value & ASUS_WMI_DSTS_PRESENCE_BIT);
-}
-
 static int asus_kbd_register_leds(struct hid_device *hdev)
 {
 	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
@@ -600,14 +558,12 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
 	drvdata->kbd_backlight->removed = false;
 	drvdata->kbd_backlight->brightness = 0;
 	drvdata->kbd_backlight->hdev = hdev;
-	drvdata->kbd_backlight->cdev.name = "asus::kbd_backlight";
-	drvdata->kbd_backlight->cdev.max_brightness = 3;
-	drvdata->kbd_backlight->cdev.brightness_set = asus_kbd_backlight_set;
-	drvdata->kbd_backlight->cdev.brightness_get = asus_kbd_backlight_get;
+	drvdata->kbd_backlight->listener.notify = asus_kbd_backlight_set;
 	INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work);
 	spin_lock_init(&drvdata->kbd_backlight->lock);
 
-	ret = devm_led_classdev_register(&hdev->dev, &drvdata->kbd_backlight->cdev);
+	ret = asus_brt_register_listener(&drvdata->kbd_backlight->listener);
+
 	if (ret < 0) {
 		/* No need to have this still around */
 		devm_kfree(&hdev->dev, drvdata->kbd_backlight);
@@ -837,7 +793,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");
 
@@ -1016,7 +971,7 @@ static int __maybe_unused asus_resume(struct hid_device *hdev) {
 
 	if (drvdata->kbd_backlight) {
 		const u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4,
-				drvdata->kbd_backlight->cdev.brightness };
+				drvdata->kbd_backlight->brightness };
 		ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
 		if (ret < 0) {
 			hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret);
@@ -1196,6 +1151,8 @@ static void asus_remove(struct hid_device *hdev)
 	unsigned long flags;
 
 	if (drvdata->kbd_backlight) {
+		asus_brt_unregister_listener(&drvdata->kbd_backlight->listener);
+
 		spin_lock_irqsave(&drvdata->kbd_backlight->lock, flags);
 		drvdata->kbd_backlight->removed = true;
 		spin_unlock_irqrestore(&drvdata->kbd_backlight->lock, flags);
-- 
2.48.1

[PATCH 07/11] platform/x86: asus-wmi: remove unused keyboard backlight quirk

[Antheas Kapenekakis]

The quirk for selecting whether keyboard backlight should be controlled
by HID or WMI is not needed anymore, so remove it.

Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 include/linux/platform_data/x86/asus-wmi.h | 40 ----------------------
 1 file changed, 40 deletions(-)

diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 42e963b70acdb..add04524031d8 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -183,44 +183,4 @@ static inline void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
 }
 #endif
 
-/* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
-static const struct dmi_system_id asus_use_hid_led_dmi_ids[] = {
-	{
-		.matches = {
-			DMI_MATCH(DMI_PRODUCT_FAMILY, "ROG Zephyrus"),
-		},
-	},
-	{
-		.matches = {
-			DMI_MATCH(DMI_PRODUCT_FAMILY, "ROG Strix"),
-		},
-	},
-	{
-		.matches = {
-			DMI_MATCH(DMI_PRODUCT_FAMILY, "ROG Flow"),
-		},
-	},
-	{
-		.matches = {
-			DMI_MATCH(DMI_PRODUCT_FAMILY, "ProArt P16"),
-		},
-	},
-	{
-		.matches = {
-			DMI_MATCH(DMI_BOARD_NAME, "GA403U"),
-		},
-	},
-	{
-		.matches = {
-			DMI_MATCH(DMI_BOARD_NAME, "GU605M"),
-		},
-	},
-	{
-		.matches = {
-			DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
-		},
-	},
-	{ },
-};
-
 #endif	/* __PLATFORM_DATA_X86_ASUS_WMI_H */
-- 
2.48.1

[PATCH 08/11] platform/x86: asus-wmi: add keyboard brightness event handler

[Antheas Kapenekakis]

Currenlty, the keyboard brightness control of Asus WMI keyboards is
handled in the kernel, which leads to the shortcut going from
brightness 0, to 1, to 2, and 3.

However, for HID keyboards it is exposed as a key and handled by the
user's desktop environment. For the toggle button, this means that
brightness control becomes on/off. In addition, in the absence of a
DE, the keyboard brightness does not work.

Therefore, expose an event handler for the keyboard brightness control
which can then be used by hid-asus.

Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/platform/x86/asus-wmi.c            | 38 ++++++++++++++++++++++
 include/linux/platform_data/x86/asus-wmi.h | 11 +++++++
 2 files changed, 49 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 21e034be71b2f..45999dda9e7ed 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -1529,6 +1529,44 @@ void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
 }
 EXPORT_SYMBOL_GPL(asus_brt_unregister_listener);
 
+static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
+
+int asus_brt_event(enum asus_brt_event event)
+{
+	int brightness;
+
+	mutex_lock(&asus_brt_lock);
+	if (!asus_brt_ref || !asus_brt_ref->kbd_led_registered) {
+		mutex_unlock(&asus_brt_lock);
+		return -EBUSY;
+	}
+	brightness = asus_brt_ref->kbd_led_wk;
+
+	switch (event) {
+	case ASUS_BRT_UP:
+		brightness += 1;
+		break;
+	case ASUS_BRT_DOWN:
+		brightness -= 1;
+		break;
+	case ASUS_BRT_TOGGLE:
+		if (brightness >= 3)
+			brightness = 0;
+		else
+			brightness += 1;
+		break;
+	}
+
+	do_kbd_led_set(&asus_brt_ref->kbd_led, brightness);
+	led_classdev_notify_brightness_hw_changed(&asus_brt_ref->kbd_led,
+						  asus_brt_ref->kbd_led_wk);
+
+	mutex_unlock(&asus_brt_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(asus_brt_event);
+
 /*
  * These functions actually update the LED's, and are called from a
  * workqueue. By doing this as separate work rather than when the LED
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index add04524031d8..2ac7912d8acd3 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -162,11 +162,18 @@ struct asus_brt_listener {
 	void (*notify)(struct asus_brt_listener *listener, int brightness);
 };
 
+enum asus_brt_event {
+	ASUS_BRT_UP,
+	ASUS_BRT_DOWN,
+	ASUS_BRT_TOGGLE,
+};
+
 #if IS_REACHABLE(CONFIG_ASUS_WMI)
 int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
 
 int asus_brt_register_listener(struct asus_brt_listener *cdev);
 void asus_brt_unregister_listener(struct asus_brt_listener *cdev);
+int asus_brt_event(enum asus_brt_event event);
 #else
 static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
 					   u32 *retval)
@@ -181,6 +188,10 @@ static inline int asus_brt_register_listener(struct asus_brt_listener *bdev)
 static inline void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
 {
 }
+static inline int asus_brt_event(enum asus_brt_event event)
+{
+	return -ENODEV;
+}
 #endif
 
 #endif	/* __PLATFORM_DATA_X86_ASUS_WMI_H */
-- 
2.48.1

Re: [PATCH 08/11] platform/x86: asus-wmi: add keyboard brightness event handler

[Luke D. Jones]

On 21/03/25 11:09, Antheas Kapenekakis wrote:
> Currenlty, the keyboard brightness control of Asus WMI keyboards is
> handled in the kernel, which leads to the shortcut going from
> brightness 0, to 1, to 2, and 3.
> 
> However, for HID keyboards it is exposed as a key and handled by the
> user's desktop environment. For the toggle button, this means that
> brightness control becomes on/off. In addition, in the absence of a
> DE, the keyboard brightness does not work.
> 
> Therefore, expose an event handler for the keyboard brightness control
> which can then be used by hid-asus.
> 
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
>   drivers/platform/x86/asus-wmi.c            | 38 ++++++++++++++++++++++
>   include/linux/platform_data/x86/asus-wmi.h | 11 +++++++
>   2 files changed, 49 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 21e034be71b2f..45999dda9e7ed 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -1529,6 +1529,44 @@ void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
>   }
>   EXPORT_SYMBOL_GPL(asus_brt_unregister_listener);
>   
> +static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
> +
> +int asus_brt_event(enum asus_brt_event event)
> +{
> +	int brightness;
> +
> +	mutex_lock(&asus_brt_lock);
> +	if (!asus_brt_ref || !asus_brt_ref->kbd_led_registered) {
> +		mutex_unlock(&asus_brt_lock);
> +		return -EBUSY;
> +	}
> +	brightness = asus_brt_ref->kbd_led_wk;
> +
> +	switch (event) {
> +	case ASUS_BRT_UP:
> +		brightness += 1;
> +		break;
> +	case ASUS_BRT_DOWN:
> +		brightness -= 1;
> +		break;
> +	case ASUS_BRT_TOGGLE:
> +		if (brightness >= 3)
> +			brightness = 0;
> +		else
> +			brightness += 1;
> +		break;
> +	}
> +
> +	do_kbd_led_set(&asus_brt_ref->kbd_led, brightness);
> +	led_classdev_notify_brightness_hw_changed(&asus_brt_ref->kbd_led,
> +						  asus_brt_ref->kbd_led_wk);
> +
> +	mutex_unlock(&asus_brt_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(asus_brt_event);
> +

I quick test on 6.14-rc7 gives me this:

[  288.039255] BUG: sleeping function called from invalid context at 
kernel/locking/mutex.c:258
[  288.039262] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0, 
name: swapper/17
[  288.039263] preempt_count: 101, expected: 0
[  288.039264] RCU nest depth: 0, expected: 0
[  288.039266] CPU: 17 UID: 0 PID: 0 Comm: swapper/17 Tainted: G 
W          6.14.0-rc7+ #164
[  288.039268] Tainted: [W]=WARN
[  288.039269] Hardware name: ASUSTeK COMPUTER INC. ROG Zephyrus M16 
GU604VY_GU604VY_00130747B/GU604VY, BIOS GU604VY.313 08/13/2024
[  288.039270] Call Trace:
[  288.039272]  <IRQ>
[  288.039273]  dump_stack_lvl+0x5d/0x80
[  288.039277]  __might_resched.cold+0xba/0xc9
[  288.039282]  mutex_lock+0x19/0x40
[  288.039285]  asus_brt_event+0x13/0xb0 [asus_wmi]
[  288.039292]  asus_event+0x91/0xa0 [hid_asus]
[  288.039295]  hid_process_event+0xae/0x120
[  288.039298]  hid_input_array_field+0x132/0x180
[  288.039300]  hid_report_raw_event+0x360/0x4e0
[  288.039302]  __hid_input_report.constprop.0+0xf1/0x180
[  288.039304]  hid_irq_in+0x17f/0x1b0
[  288.039306]  __usb_hcd_giveback_urb+0x98/0x110
[  288.039308]  usb_giveback_urb_bh+0xbd/0x150
[  288.039310]  process_one_work+0x171/0x290
[  288.039312]  bh_worker+0x1ac/0x210
[  288.039314]  tasklet_hi_action+0xe/0x30
[  288.039315]  handle_softirqs+0xdb/0x1f0
[  288.039317]  __irq_exit_rcu+0xc2/0xe0
[  288.039318]  common_interrupt+0x85/0xa0
[  288.039320]  </IRQ>
[  288.039320]  <TASK>
[  288.039321]  asm_common_interrupt+0x26/0x40
[  288.039323] RIP: 0010:cpuidle_enter_state+0xb9/0x2c0
[  288.039325] Code: 40 0f 84 1b 01 00 00 e8 35 e8 13 ff e8 40 f2 ff ff 
31 ff 49 89 c5 e8 c6 f0 12 ff 45 84 f6 0f 85 f2 00 00 00 fb 0f 1f 44 00 
00 <45> 85 ff 0f 88 cf 00 00 00 49 63 f7 48 8d 04 76 48 8d 04 86 49 8d
[  288.039326] RSP: 0018:ffffc90000253e90 EFLAGS: 00000246
[  288.039328] RAX: ffff888890680000 RBX: 0000000000000003 RCX: 
0000000000000000
[  288.039329] RDX: 00000043107862af RSI: fffffffd616e8210 RDI: 
0000000000000000
[  288.039329] RBP: ffff8888906ba370 R08: 0000000000000000 R09: 
0000000000000007
[  288.039330] R10: ffff88888ffa6098 R11: 0000000000000008 R12: 
ffffffff82fd4140
[  288.039331] R13: 00000043107862af R14: 0000000000000000 R15: 
0000000000000003
[  288.039332]  cpuidle_enter+0x28/0x40
[  288.039334]  do_idle+0x1a8/0x200
[  288.039336]  cpu_startup_entry+0x24/0x30
[  288.039337]  start_secondary+0x11e/0x140
[  288.039340]  common_startup_64+0x13e/0x141
[  288.039342]  </TASK>

I think you need to swap the mutex to a spin_lock_irqsave and associated 
spin_unlock_irqrestore plus DEFINE_SPINLOCK(asus_brt_lock).

I'm out of time tonight but I'll apply the above to your patches and 
report back tomorrow if you don't get to it before I do.

It might be worth checking any other mutex uses in the LED path too.

Cheers,
Luke.

>   /*
>    * These functions actually update the LED's, and are called from a
>    * workqueue. By doing this as separate work rather than when the LED
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index add04524031d8..2ac7912d8acd3 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -162,11 +162,18 @@ struct asus_brt_listener {
>   	void (*notify)(struct asus_brt_listener *listener, int brightness);
>   };
>   
> +enum asus_brt_event {
> +	ASUS_BRT_UP,
> +	ASUS_BRT_DOWN,
> +	ASUS_BRT_TOGGLE,
> +};
> +
>   #if IS_REACHABLE(CONFIG_ASUS_WMI)
>   int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
>   
>   int asus_brt_register_listener(struct asus_brt_listener *cdev);
>   void asus_brt_unregister_listener(struct asus_brt_listener *cdev);
> +int asus_brt_event(enum asus_brt_event event);
>   #else
>   static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
>   					   u32 *retval)
> @@ -181,6 +188,10 @@ static inline int asus_brt_register_listener(struct asus_brt_listener *bdev)
>   static inline void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
>   {
>   }
> +static inline int asus_brt_event(enum asus_brt_event event)
> +{
> +	return -ENODEV;
> +}
>   #endif
>   
>   #endif	/* __PLATFORM_DATA_X86_ASUS_WMI_H */

Re: [PATCH 08/11] platform/x86: asus-wmi: add keyboard brightness event handler

[Antheas Kapenekakis]

On Sat, 22 Mar 2025 at 05:31, Luke D. Jones <luke@ljones.dev> wrote:
>
> On 21/03/25 11:09, Antheas Kapenekakis wrote:
> > Currenlty, the keyboard brightness control of Asus WMI keyboards is
> > handled in the kernel, which leads to the shortcut going from
> > brightness 0, to 1, to 2, and 3.
> >
> > However, for HID keyboards it is exposed as a key and handled by the
> > user's desktop environment. For the toggle button, this means that
> > brightness control becomes on/off. In addition, in the absence of a
> > DE, the keyboard brightness does not work.
> >
> > Therefore, expose an event handler for the keyboard brightness control
> > which can then be used by hid-asus.
> >
> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > ---
> >   drivers/platform/x86/asus-wmi.c            | 38 ++++++++++++++++++++++
> >   include/linux/platform_data/x86/asus-wmi.h | 11 +++++++
> >   2 files changed, 49 insertions(+)
> >
> > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> > index 21e034be71b2f..45999dda9e7ed 100644
> > --- a/drivers/platform/x86/asus-wmi.c
> > +++ b/drivers/platform/x86/asus-wmi.c
> > @@ -1529,6 +1529,44 @@ void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
> >   }
> >   EXPORT_SYMBOL_GPL(asus_brt_unregister_listener);
> >
> > +static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
> > +
> > +int asus_brt_event(enum asus_brt_event event)
> > +{
> > +     int brightness;
> > +
> > +     mutex_lock(&asus_brt_lock);
> > +     if (!asus_brt_ref || !asus_brt_ref->kbd_led_registered) {
> > +             mutex_unlock(&asus_brt_lock);
> > +             return -EBUSY;
> > +     }
> > +     brightness = asus_brt_ref->kbd_led_wk;
> > +
> > +     switch (event) {
> > +     case ASUS_BRT_UP:
> > +             brightness += 1;
> > +             break;
> > +     case ASUS_BRT_DOWN:
> > +             brightness -= 1;
> > +             break;
> > +     case ASUS_BRT_TOGGLE:
> > +             if (brightness >= 3)
> > +                     brightness = 0;
> > +             else
> > +                     brightness += 1;
> > +             break;
> > +     }
> > +
> > +     do_kbd_led_set(&asus_brt_ref->kbd_led, brightness);
> > +     led_classdev_notify_brightness_hw_changed(&asus_brt_ref->kbd_led,
> > +                                               asus_brt_ref->kbd_led_wk);
> > +
> > +     mutex_unlock(&asus_brt_lock);
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_brt_event);
> > +
>
> I quick test on 6.14-rc7 gives me this:
>
> [  288.039255] BUG: sleeping function called from invalid context at
> kernel/locking/mutex.c:258
> [  288.039262] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0,
> name: swapper/17
> [  288.039263] preempt_count: 101, expected: 0
> [  288.039264] RCU nest depth: 0, expected: 0
> [  288.039266] CPU: 17 UID: 0 PID: 0 Comm: swapper/17 Tainted: G
> W          6.14.0-rc7+ #164
> [  288.039268] Tainted: [W]=WARN
> [  288.039269] Hardware name: ASUSTeK COMPUTER INC. ROG Zephyrus M16
> GU604VY_GU604VY_00130747B/GU604VY, BIOS GU604VY.313 08/13/2024
> [  288.039270] Call Trace:
> [  288.039272]  <IRQ>
> [  288.039273]  dump_stack_lvl+0x5d/0x80
> [  288.039277]  __might_resched.cold+0xba/0xc9
> [  288.039282]  mutex_lock+0x19/0x40
> [  288.039285]  asus_brt_event+0x13/0xb0 [asus_wmi]
> [  288.039292]  asus_event+0x91/0xa0 [hid_asus]
> [  288.039295]  hid_process_event+0xae/0x120
> [  288.039298]  hid_input_array_field+0x132/0x180
> [  288.039300]  hid_report_raw_event+0x360/0x4e0
> [  288.039302]  __hid_input_report.constprop.0+0xf1/0x180
> [  288.039304]  hid_irq_in+0x17f/0x1b0
> [  288.039306]  __usb_hcd_giveback_urb+0x98/0x110
> [  288.039308]  usb_giveback_urb_bh+0xbd/0x150
> [  288.039310]  process_one_work+0x171/0x290
> [  288.039312]  bh_worker+0x1ac/0x210
> [  288.039314]  tasklet_hi_action+0xe/0x30
> [  288.039315]  handle_softirqs+0xdb/0x1f0
> [  288.039317]  __irq_exit_rcu+0xc2/0xe0
> [  288.039318]  common_interrupt+0x85/0xa0
> [  288.039320]  </IRQ>
> [  288.039320]  <TASK>
> [  288.039321]  asm_common_interrupt+0x26/0x40
> [  288.039323] RIP: 0010:cpuidle_enter_state+0xb9/0x2c0
> [  288.039325] Code: 40 0f 84 1b 01 00 00 e8 35 e8 13 ff e8 40 f2 ff ff
> 31 ff 49 89 c5 e8 c6 f0 12 ff 45 84 f6 0f 85 f2 00 00 00 fb 0f 1f 44 00
> 00 <45> 85 ff 0f 88 cf 00 00 00 49 63 f7 48 8d 04 76 48 8d 04 86 49 8d
> [  288.039326] RSP: 0018:ffffc90000253e90 EFLAGS: 00000246
> [  288.039328] RAX: ffff888890680000 RBX: 0000000000000003 RCX:
> 0000000000000000
> [  288.039329] RDX: 00000043107862af RSI: fffffffd616e8210 RDI:
> 0000000000000000
> [  288.039329] RBP: ffff8888906ba370 R08: 0000000000000000 R09:
> 0000000000000007
> [  288.039330] R10: ffff88888ffa6098 R11: 0000000000000008 R12:
> ffffffff82fd4140
> [  288.039331] R13: 00000043107862af R14: 0000000000000000 R15:
> 0000000000000003
> [  288.039332]  cpuidle_enter+0x28/0x40
> [  288.039334]  do_idle+0x1a8/0x200
> [  288.039336]  cpu_startup_entry+0x24/0x30
> [  288.039337]  start_secondary+0x11e/0x140
> [  288.039340]  common_startup_64+0x13e/0x141
> [  288.039342]  </TASK>
>
> I think you need to swap the mutex to a spin_lock_irqsave and associated
> spin_unlock_irqrestore plus DEFINE_SPINLOCK(asus_brt_lock).
>
> I'm out of time tonight but I'll apply the above to your patches and
> report back tomorrow if you don't get to it before I do.
>
> It might be worth checking any other mutex uses in the LED path too.

Thank you for catching that, I will replace the mutex with a spinlock.
Might have to do with the WMI method being active as I got no such
issue in my device.

 I guess I will try to do a v3 today as that will hold back our kernel too.

> Cheers,
> Luke.
>
> >   /*
> >    * These functions actually update the LED's, and are called from a
> >    * workqueue. By doing this as separate work rather than when the LED
> > diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> > index add04524031d8..2ac7912d8acd3 100644
> > --- a/include/linux/platform_data/x86/asus-wmi.h
> > +++ b/include/linux/platform_data/x86/asus-wmi.h
> > @@ -162,11 +162,18 @@ struct asus_brt_listener {
> >       void (*notify)(struct asus_brt_listener *listener, int brightness);
> >   };
> >
> > +enum asus_brt_event {
> > +     ASUS_BRT_UP,
> > +     ASUS_BRT_DOWN,
> > +     ASUS_BRT_TOGGLE,
> > +};
> > +
> >   #if IS_REACHABLE(CONFIG_ASUS_WMI)
> >   int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
> >
> >   int asus_brt_register_listener(struct asus_brt_listener *cdev);
> >   void asus_brt_unregister_listener(struct asus_brt_listener *cdev);
> > +int asus_brt_event(enum asus_brt_event event);
> >   #else
> >   static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
> >                                          u32 *retval)
> > @@ -181,6 +188,10 @@ static inline int asus_brt_register_listener(struct asus_brt_listener *bdev)
> >   static inline void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
> >   {
> >   }
> > +static inline int asus_brt_event(enum asus_brt_event event)
> > +{
> > +     return -ENODEV;
> > +}
> >   #endif
> >
> >   #endif      /* __PLATFORM_DATA_X86_ASUS_WMI_H */
>

Re: [PATCH 08/11] platform/x86: asus-wmi: add keyboard brightness event handler

[Luke D. Jones]

On 22/03/25 21:12, Antheas Kapenekakis wrote:
> On Sat, 22 Mar 2025 at 05:31, Luke D. Jones <luke@ljones.dev> wrote:
>>
>> On 21/03/25 11:09, Antheas Kapenekakis wrote:
>>> Currenlty, the keyboard brightness control of Asus WMI keyboards is
>>> handled in the kernel, which leads to the shortcut going from
>>> brightness 0, to 1, to 2, and 3.
>>>
>>> However, for HID keyboards it is exposed as a key and handled by the
>>> user's desktop environment. For the toggle button, this means that
>>> brightness control becomes on/off. In addition, in the absence of a
>>> DE, the keyboard brightness does not work.
>>>
>>> Therefore, expose an event handler for the keyboard brightness control
>>> which can then be used by hid-asus.
>>>
>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>> ---
>>>    drivers/platform/x86/asus-wmi.c            | 38 ++++++++++++++++++++++
>>>    include/linux/platform_data/x86/asus-wmi.h | 11 +++++++
>>>    2 files changed, 49 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>>> index 21e034be71b2f..45999dda9e7ed 100644
>>> --- a/drivers/platform/x86/asus-wmi.c
>>> +++ b/drivers/platform/x86/asus-wmi.c
>>> @@ -1529,6 +1529,44 @@ void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
>>>    }
>>>    EXPORT_SYMBOL_GPL(asus_brt_unregister_listener);
>>>
>>> +static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
>>> +
>>> +int asus_brt_event(enum asus_brt_event event)
>>> +{
>>> +     int brightness;
>>> +
>>> +     mutex_lock(&asus_brt_lock);
>>> +     if (!asus_brt_ref || !asus_brt_ref->kbd_led_registered) {
>>> +             mutex_unlock(&asus_brt_lock);
>>> +             return -EBUSY;
>>> +     }
>>> +     brightness = asus_brt_ref->kbd_led_wk;
>>> +
>>> +     switch (event) {
>>> +     case ASUS_BRT_UP:
>>> +             brightness += 1;
>>> +             break;
>>> +     case ASUS_BRT_DOWN:
>>> +             brightness -= 1;
>>> +             break;
>>> +     case ASUS_BRT_TOGGLE:
>>> +             if (brightness >= 3)
>>> +                     brightness = 0;
>>> +             else
>>> +                     brightness += 1;
>>> +             break;
>>> +     }
>>> +
>>> +     do_kbd_led_set(&asus_brt_ref->kbd_led, brightness);
>>> +     led_classdev_notify_brightness_hw_changed(&asus_brt_ref->kbd_led,
>>> +                                               asus_brt_ref->kbd_led_wk);
>>> +
>>> +     mutex_unlock(&asus_brt_lock);
>>> +
>>> +     return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(asus_brt_event);
>>> +
>>
>> I quick test on 6.14-rc7 gives me this:
>>
>> [  288.039255] BUG: sleeping function called from invalid context at
>> kernel/locking/mutex.c:258
>> [  288.039262] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0,
>> name: swapper/17
>> [  288.039263] preempt_count: 101, expected: 0
>> [  288.039264] RCU nest depth: 0, expected: 0
>> [  288.039266] CPU: 17 UID: 0 PID: 0 Comm: swapper/17 Tainted: G
>> W          6.14.0-rc7+ #164
>> [  288.039268] Tainted: [W]=WARN
>> [  288.039269] Hardware name: ASUSTeK COMPUTER INC. ROG Zephyrus M16
>> GU604VY_GU604VY_00130747B/GU604VY, BIOS GU604VY.313 08/13/2024
>> [  288.039270] Call Trace:
>> [  288.039272]  <IRQ>
>> [  288.039273]  dump_stack_lvl+0x5d/0x80
>> [  288.039277]  __might_resched.cold+0xba/0xc9
>> [  288.039282]  mutex_lock+0x19/0x40
>> [  288.039285]  asus_brt_event+0x13/0xb0 [asus_wmi]
>> [  288.039292]  asus_event+0x91/0xa0 [hid_asus]
>> [  288.039295]  hid_process_event+0xae/0x120
>> [  288.039298]  hid_input_array_field+0x132/0x180
>> [  288.039300]  hid_report_raw_event+0x360/0x4e0
>> [  288.039302]  __hid_input_report.constprop.0+0xf1/0x180
>> [  288.039304]  hid_irq_in+0x17f/0x1b0
>> [  288.039306]  __usb_hcd_giveback_urb+0x98/0x110
>> [  288.039308]  usb_giveback_urb_bh+0xbd/0x150
>> [  288.039310]  process_one_work+0x171/0x290
>> [  288.039312]  bh_worker+0x1ac/0x210
>> [  288.039314]  tasklet_hi_action+0xe/0x30
>> [  288.039315]  handle_softirqs+0xdb/0x1f0
>> [  288.039317]  __irq_exit_rcu+0xc2/0xe0
>> [  288.039318]  common_interrupt+0x85/0xa0
>> [  288.039320]  </IRQ>
>> [  288.039320]  <TASK>
>> [  288.039321]  asm_common_interrupt+0x26/0x40
>> [  288.039323] RIP: 0010:cpuidle_enter_state+0xb9/0x2c0
>> [  288.039325] Code: 40 0f 84 1b 01 00 00 e8 35 e8 13 ff e8 40 f2 ff ff
>> 31 ff 49 89 c5 e8 c6 f0 12 ff 45 84 f6 0f 85 f2 00 00 00 fb 0f 1f 44 00
>> 00 <45> 85 ff 0f 88 cf 00 00 00 49 63 f7 48 8d 04 76 48 8d 04 86 49 8d
>> [  288.039326] RSP: 0018:ffffc90000253e90 EFLAGS: 00000246
>> [  288.039328] RAX: ffff888890680000 RBX: 0000000000000003 RCX:
>> 0000000000000000
>> [  288.039329] RDX: 00000043107862af RSI: fffffffd616e8210 RDI:
>> 0000000000000000
>> [  288.039329] RBP: ffff8888906ba370 R08: 0000000000000000 R09:
>> 0000000000000007
>> [  288.039330] R10: ffff88888ffa6098 R11: 0000000000000008 R12:
>> ffffffff82fd4140
>> [  288.039331] R13: 00000043107862af R14: 0000000000000000 R15:
>> 0000000000000003
>> [  288.039332]  cpuidle_enter+0x28/0x40
>> [  288.039334]  do_idle+0x1a8/0x200
>> [  288.039336]  cpu_startup_entry+0x24/0x30
>> [  288.039337]  start_secondary+0x11e/0x140
>> [  288.039340]  common_startup_64+0x13e/0x141
>> [  288.039342]  </TASK>
>>
>> I think you need to swap the mutex to a spin_lock_irqsave and associated
>> spin_unlock_irqrestore plus DEFINE_SPINLOCK(asus_brt_lock).
>>
>> I'm out of time tonight but I'll apply the above to your patches and
>> report back tomorrow if you don't get to it before I do.
>>
>> It might be worth checking any other mutex uses in the LED path too.
> 
> Thank you for catching that, I will replace the mutex with a spinlock.
> Might have to do with the WMI method being active as I got no such
> issue in my device.

This might highlight the HID + WMI issue I was talking about in the 
other replies and why i think the quirk table is still required.. Or 
perhaps an alternative would be to have HID block the WMI method for the 
0x19b6 PID? There are approximately 30 laptops I know of with both 
methods available.

I just checked the DSDT dump for Ally again and it looks like those are 
all good too. You might have lucked out and ended up with all devices 
with no WMI keyboard brightness :D

> 
>   I guess I will try to do a v3 today as that will hold back our kernel too.
> 
>> Cheers,
>> Luke.
>>
>>>    /*
>>>     * These functions actually update the LED's, and are called from a
>>>     * workqueue. By doing this as separate work rather than when the LED
>>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
>>> index add04524031d8..2ac7912d8acd3 100644
>>> --- a/include/linux/platform_data/x86/asus-wmi.h
>>> +++ b/include/linux/platform_data/x86/asus-wmi.h
>>> @@ -162,11 +162,18 @@ struct asus_brt_listener {
>>>        void (*notify)(struct asus_brt_listener *listener, int brightness);
>>>    };
>>>
>>> +enum asus_brt_event {
>>> +     ASUS_BRT_UP,
>>> +     ASUS_BRT_DOWN,
>>> +     ASUS_BRT_TOGGLE,
>>> +};
>>> +
>>>    #if IS_REACHABLE(CONFIG_ASUS_WMI)
>>>    int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
>>>
>>>    int asus_brt_register_listener(struct asus_brt_listener *cdev);
>>>    void asus_brt_unregister_listener(struct asus_brt_listener *cdev);
>>> +int asus_brt_event(enum asus_brt_event event);
>>>    #else
>>>    static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
>>>                                           u32 *retval)
>>> @@ -181,6 +188,10 @@ static inline int asus_brt_register_listener(struct asus_brt_listener *bdev)
>>>    static inline void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
>>>    {
>>>    }
>>> +static inline int asus_brt_event(enum asus_brt_event event)
>>> +{
>>> +     return -ENODEV;
>>> +}
>>>    #endif
>>>
>>>    #endif      /* __PLATFORM_DATA_X86_ASUS_WMI_H */
>>

Re: [PATCH 08/11] platform/x86: asus-wmi: add keyboard brightness event handler

[Antheas Kapenekakis]

On Sat, 22 Mar 2025 at 10:06, Luke D. Jones <luke@ljones.dev> wrote:
>
> On 22/03/25 21:12, Antheas Kapenekakis wrote:
> > On Sat, 22 Mar 2025 at 05:31, Luke D. Jones <luke@ljones.dev> wrote:
> >>
> >> On 21/03/25 11:09, Antheas Kapenekakis wrote:
> >>> Currenlty, the keyboard brightness control of Asus WMI keyboards is
> >>> handled in the kernel, which leads to the shortcut going from
> >>> brightness 0, to 1, to 2, and 3.
> >>>
> >>> However, for HID keyboards it is exposed as a key and handled by the
> >>> user's desktop environment. For the toggle button, this means that
> >>> brightness control becomes on/off. In addition, in the absence of a
> >>> DE, the keyboard brightness does not work.
> >>>
> >>> Therefore, expose an event handler for the keyboard brightness control
> >>> which can then be used by hid-asus.
> >>>
> >>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>> ---
> >>>    drivers/platform/x86/asus-wmi.c            | 38 ++++++++++++++++++++++
> >>>    include/linux/platform_data/x86/asus-wmi.h | 11 +++++++
> >>>    2 files changed, 49 insertions(+)
> >>>
> >>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> >>> index 21e034be71b2f..45999dda9e7ed 100644
> >>> --- a/drivers/platform/x86/asus-wmi.c
> >>> +++ b/drivers/platform/x86/asus-wmi.c
> >>> @@ -1529,6 +1529,44 @@ void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
> >>>    }
> >>>    EXPORT_SYMBOL_GPL(asus_brt_unregister_listener);
> >>>
> >>> +static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
> >>> +
> >>> +int asus_brt_event(enum asus_brt_event event)
> >>> +{
> >>> +     int brightness;
> >>> +
> >>> +     mutex_lock(&asus_brt_lock);
> >>> +     if (!asus_brt_ref || !asus_brt_ref->kbd_led_registered) {
> >>> +             mutex_unlock(&asus_brt_lock);
> >>> +             return -EBUSY;
> >>> +     }
> >>> +     brightness = asus_brt_ref->kbd_led_wk;
> >>> +
> >>> +     switch (event) {
> >>> +     case ASUS_BRT_UP:
> >>> +             brightness += 1;
> >>> +             break;
> >>> +     case ASUS_BRT_DOWN:
> >>> +             brightness -= 1;
> >>> +             break;
> >>> +     case ASUS_BRT_TOGGLE:
> >>> +             if (brightness >= 3)
> >>> +                     brightness = 0;
> >>> +             else
> >>> +                     brightness += 1;
> >>> +             break;
> >>> +     }
> >>> +
> >>> +     do_kbd_led_set(&asus_brt_ref->kbd_led, brightness);
> >>> +     led_classdev_notify_brightness_hw_changed(&asus_brt_ref->kbd_led,
> >>> +                                               asus_brt_ref->kbd_led_wk);
> >>> +
> >>> +     mutex_unlock(&asus_brt_lock);
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(asus_brt_event);
> >>> +
> >>
> >> I quick test on 6.14-rc7 gives me this:
> >>
> >> [  288.039255] BUG: sleeping function called from invalid context at
> >> kernel/locking/mutex.c:258
> >> [  288.039262] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0,
> >> name: swapper/17
> >> [  288.039263] preempt_count: 101, expected: 0
> >> [  288.039264] RCU nest depth: 0, expected: 0
> >> [  288.039266] CPU: 17 UID: 0 PID: 0 Comm: swapper/17 Tainted: G
> >> W          6.14.0-rc7+ #164
> >> [  288.039268] Tainted: [W]=WARN
> >> [  288.039269] Hardware name: ASUSTeK COMPUTER INC. ROG Zephyrus M16
> >> GU604VY_GU604VY_00130747B/GU604VY, BIOS GU604VY.313 08/13/2024
> >> [  288.039270] Call Trace:
> >> [  288.039272]  <IRQ>
> >> [  288.039273]  dump_stack_lvl+0x5d/0x80
> >> [  288.039277]  __might_resched.cold+0xba/0xc9
> >> [  288.039282]  mutex_lock+0x19/0x40
> >> [  288.039285]  asus_brt_event+0x13/0xb0 [asus_wmi]
> >> [  288.039292]  asus_event+0x91/0xa0 [hid_asus]
> >> [  288.039295]  hid_process_event+0xae/0x120
> >> [  288.039298]  hid_input_array_field+0x132/0x180
> >> [  288.039300]  hid_report_raw_event+0x360/0x4e0
> >> [  288.039302]  __hid_input_report.constprop.0+0xf1/0x180
> >> [  288.039304]  hid_irq_in+0x17f/0x1b0
> >> [  288.039306]  __usb_hcd_giveback_urb+0x98/0x110
> >> [  288.039308]  usb_giveback_urb_bh+0xbd/0x150
> >> [  288.039310]  process_one_work+0x171/0x290
> >> [  288.039312]  bh_worker+0x1ac/0x210
> >> [  288.039314]  tasklet_hi_action+0xe/0x30
> >> [  288.039315]  handle_softirqs+0xdb/0x1f0
> >> [  288.039317]  __irq_exit_rcu+0xc2/0xe0
> >> [  288.039318]  common_interrupt+0x85/0xa0
> >> [  288.039320]  </IRQ>
> >> [  288.039320]  <TASK>
> >> [  288.039321]  asm_common_interrupt+0x26/0x40
> >> [  288.039323] RIP: 0010:cpuidle_enter_state+0xb9/0x2c0
> >> [  288.039325] Code: 40 0f 84 1b 01 00 00 e8 35 e8 13 ff e8 40 f2 ff ff
> >> 31 ff 49 89 c5 e8 c6 f0 12 ff 45 84 f6 0f 85 f2 00 00 00 fb 0f 1f 44 00
> >> 00 <45> 85 ff 0f 88 cf 00 00 00 49 63 f7 48 8d 04 76 48 8d 04 86 49 8d
> >> [  288.039326] RSP: 0018:ffffc90000253e90 EFLAGS: 00000246
> >> [  288.039328] RAX: ffff888890680000 RBX: 0000000000000003 RCX:
> >> 0000000000000000
> >> [  288.039329] RDX: 00000043107862af RSI: fffffffd616e8210 RDI:
> >> 0000000000000000
> >> [  288.039329] RBP: ffff8888906ba370 R08: 0000000000000000 R09:
> >> 0000000000000007
> >> [  288.039330] R10: ffff88888ffa6098 R11: 0000000000000008 R12:
> >> ffffffff82fd4140
> >> [  288.039331] R13: 00000043107862af R14: 0000000000000000 R15:
> >> 0000000000000003
> >> [  288.039332]  cpuidle_enter+0x28/0x40
> >> [  288.039334]  do_idle+0x1a8/0x200
> >> [  288.039336]  cpu_startup_entry+0x24/0x30
> >> [  288.039337]  start_secondary+0x11e/0x140
> >> [  288.039340]  common_startup_64+0x13e/0x141
> >> [  288.039342]  </TASK>
> >>
> >> I think you need to swap the mutex to a spin_lock_irqsave and associated
> >> spin_unlock_irqrestore plus DEFINE_SPINLOCK(asus_brt_lock).
> >>
> >> I'm out of time tonight but I'll apply the above to your patches and
> >> report back tomorrow if you don't get to it before I do.
> >>
> >> It might be worth checking any other mutex uses in the LED path too.
> >
> > Thank you for catching that, I will replace the mutex with a spinlock.
> > Might have to do with the WMI method being active as I got no such
> > issue in my device.
>
> This might highlight the HID + WMI issue I was talking about in the
> other replies and why i think the quirk table is still required.. Or
> perhaps an alternative would be to have HID block the WMI method for the
> 0x19b6 PID? There are approximately 30 laptops I know of with both
> methods available.
>
> I just checked the DSDT dump for Ally again and it looks like those are
> all good too. You might have lucked out and ended up with all devices
> with no WMI keyboard brightness :D

Well we for sure will need to test a device that has both. The intent
of this series is to align both the WMI and HID, which is why it is
placed in WMI. If it NOOPs it should be ok.

However if the set noops and the get returns dummy data that might be an issue.

> >
> >   I guess I will try to do a v3 today as that will hold back our kernel too.
> >
> >> Cheers,
> >> Luke.
> >>
> >>>    /*
> >>>     * These functions actually update the LED's, and are called from a
> >>>     * workqueue. By doing this as separate work rather than when the LED
> >>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> >>> index add04524031d8..2ac7912d8acd3 100644
> >>> --- a/include/linux/platform_data/x86/asus-wmi.h
> >>> +++ b/include/linux/platform_data/x86/asus-wmi.h
> >>> @@ -162,11 +162,18 @@ struct asus_brt_listener {
> >>>        void (*notify)(struct asus_brt_listener *listener, int brightness);
> >>>    };
> >>>
> >>> +enum asus_brt_event {
> >>> +     ASUS_BRT_UP,
> >>> +     ASUS_BRT_DOWN,
> >>> +     ASUS_BRT_TOGGLE,
> >>> +};
> >>> +
> >>>    #if IS_REACHABLE(CONFIG_ASUS_WMI)
> >>>    int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
> >>>
> >>>    int asus_brt_register_listener(struct asus_brt_listener *cdev);
> >>>    void asus_brt_unregister_listener(struct asus_brt_listener *cdev);
> >>> +int asus_brt_event(enum asus_brt_event event);
> >>>    #else
> >>>    static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
> >>>                                           u32 *retval)
> >>> @@ -181,6 +188,10 @@ static inline int asus_brt_register_listener(struct asus_brt_listener *bdev)
> >>>    static inline void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
> >>>    {
> >>>    }
> >>> +static inline int asus_brt_event(enum asus_brt_event event)
> >>> +{
> >>> +     return -ENODEV;
> >>> +}
> >>>    #endif
> >>>
> >>>    #endif      /* __PLATFORM_DATA_X86_ASUS_WMI_H */
> >>
>

Re: [PATCH 08/11] platform/x86: asus-wmi: add keyboard brightness event handler

[Luke D. Jones]

On 22/03/25 22:13, Antheas Kapenekakis wrote:
> On Sat, 22 Mar 2025 at 10:06, Luke D. Jones <luke@ljones.dev> wrote:
>>
>> On 22/03/25 21:12, Antheas Kapenekakis wrote:
>>> On Sat, 22 Mar 2025 at 05:31, Luke D. Jones <luke@ljones.dev> wrote:
>>>>
>>>> On 21/03/25 11:09, Antheas Kapenekakis wrote:
>>>>> Currenlty, the keyboard brightness control of Asus WMI keyboards is
>>>>> handled in the kernel, which leads to the shortcut going from
>>>>> brightness 0, to 1, to 2, and 3.
>>>>>
>>>>> However, for HID keyboards it is exposed as a key and handled by the
>>>>> user's desktop environment. For the toggle button, this means that
>>>>> brightness control becomes on/off. In addition, in the absence of a
>>>>> DE, the keyboard brightness does not work.
>>>>>
>>>>> Therefore, expose an event handler for the keyboard brightness control
>>>>> which can then be used by hid-asus.
>>>>>
>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>>>> ---
>>>>>     drivers/platform/x86/asus-wmi.c            | 38 ++++++++++++++++++++++
>>>>>     include/linux/platform_data/x86/asus-wmi.h | 11 +++++++
>>>>>     2 files changed, 49 insertions(+)
>>>>>
>>>>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>>>>> index 21e034be71b2f..45999dda9e7ed 100644
>>>>> --- a/drivers/platform/x86/asus-wmi.c
>>>>> +++ b/drivers/platform/x86/asus-wmi.c
>>>>> @@ -1529,6 +1529,44 @@ void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
>>>>>     }
>>>>>     EXPORT_SYMBOL_GPL(asus_brt_unregister_listener);
>>>>>
>>>>> +static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
>>>>> +
>>>>> +int asus_brt_event(enum asus_brt_event event)
>>>>> +{
>>>>> +     int brightness;
>>>>> +
>>>>> +     mutex_lock(&asus_brt_lock);
>>>>> +     if (!asus_brt_ref || !asus_brt_ref->kbd_led_registered) {
>>>>> +             mutex_unlock(&asus_brt_lock);
>>>>> +             return -EBUSY;
>>>>> +     }
>>>>> +     brightness = asus_brt_ref->kbd_led_wk;
>>>>> +
>>>>> +     switch (event) {
>>>>> +     case ASUS_BRT_UP:
>>>>> +             brightness += 1;
>>>>> +             break;
>>>>> +     case ASUS_BRT_DOWN:
>>>>> +             brightness -= 1;
>>>>> +             break;
>>>>> +     case ASUS_BRT_TOGGLE:
>>>>> +             if (brightness >= 3)
>>>>> +                     brightness = 0;
>>>>> +             else
>>>>> +                     brightness += 1;
>>>>> +             break;
>>>>> +     }
>>>>> +
>>>>> +     do_kbd_led_set(&asus_brt_ref->kbd_led, brightness);
>>>>> +     led_classdev_notify_brightness_hw_changed(&asus_brt_ref->kbd_led,
>>>>> +                                               asus_brt_ref->kbd_led_wk);
>>>>> +
>>>>> +     mutex_unlock(&asus_brt_lock);
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(asus_brt_event);
>>>>> +
>>>>
>>>> I quick test on 6.14-rc7 gives me this:
>>>>
>>>> [  288.039255] BUG: sleeping function called from invalid context at
>>>> kernel/locking/mutex.c:258
>>>> [  288.039262] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0,
>>>> name: swapper/17
>>>> [  288.039263] preempt_count: 101, expected: 0
>>>> [  288.039264] RCU nest depth: 0, expected: 0
>>>> [  288.039266] CPU: 17 UID: 0 PID: 0 Comm: swapper/17 Tainted: G
>>>> W          6.14.0-rc7+ #164
>>>> [  288.039268] Tainted: [W]=WARN
>>>> [  288.039269] Hardware name: ASUSTeK COMPUTER INC. ROG Zephyrus M16
>>>> GU604VY_GU604VY_00130747B/GU604VY, BIOS GU604VY.313 08/13/2024
>>>> [  288.039270] Call Trace:
>>>> [  288.039272]  <IRQ>
>>>> [  288.039273]  dump_stack_lvl+0x5d/0x80
>>>> [  288.039277]  __might_resched.cold+0xba/0xc9
>>>> [  288.039282]  mutex_lock+0x19/0x40
>>>> [  288.039285]  asus_brt_event+0x13/0xb0 [asus_wmi]
>>>> [  288.039292]  asus_event+0x91/0xa0 [hid_asus]
>>>> [  288.039295]  hid_process_event+0xae/0x120
>>>> [  288.039298]  hid_input_array_field+0x132/0x180
>>>> [  288.039300]  hid_report_raw_event+0x360/0x4e0
>>>> [  288.039302]  __hid_input_report.constprop.0+0xf1/0x180
>>>> [  288.039304]  hid_irq_in+0x17f/0x1b0
>>>> [  288.039306]  __usb_hcd_giveback_urb+0x98/0x110
>>>> [  288.039308]  usb_giveback_urb_bh+0xbd/0x150
>>>> [  288.039310]  process_one_work+0x171/0x290
>>>> [  288.039312]  bh_worker+0x1ac/0x210
>>>> [  288.039314]  tasklet_hi_action+0xe/0x30
>>>> [  288.039315]  handle_softirqs+0xdb/0x1f0
>>>> [  288.039317]  __irq_exit_rcu+0xc2/0xe0
>>>> [  288.039318]  common_interrupt+0x85/0xa0
>>>> [  288.039320]  </IRQ>
>>>> [  288.039320]  <TASK>
>>>> [  288.039321]  asm_common_interrupt+0x26/0x40
>>>> [  288.039323] RIP: 0010:cpuidle_enter_state+0xb9/0x2c0
>>>> [  288.039325] Code: 40 0f 84 1b 01 00 00 e8 35 e8 13 ff e8 40 f2 ff ff
>>>> 31 ff 49 89 c5 e8 c6 f0 12 ff 45 84 f6 0f 85 f2 00 00 00 fb 0f 1f 44 00
>>>> 00 <45> 85 ff 0f 88 cf 00 00 00 49 63 f7 48 8d 04 76 48 8d 04 86 49 8d
>>>> [  288.039326] RSP: 0018:ffffc90000253e90 EFLAGS: 00000246
>>>> [  288.039328] RAX: ffff888890680000 RBX: 0000000000000003 RCX:
>>>> 0000000000000000
>>>> [  288.039329] RDX: 00000043107862af RSI: fffffffd616e8210 RDI:
>>>> 0000000000000000
>>>> [  288.039329] RBP: ffff8888906ba370 R08: 0000000000000000 R09:
>>>> 0000000000000007
>>>> [  288.039330] R10: ffff88888ffa6098 R11: 0000000000000008 R12:
>>>> ffffffff82fd4140
>>>> [  288.039331] R13: 00000043107862af R14: 0000000000000000 R15:
>>>> 0000000000000003
>>>> [  288.039332]  cpuidle_enter+0x28/0x40
>>>> [  288.039334]  do_idle+0x1a8/0x200
>>>> [  288.039336]  cpu_startup_entry+0x24/0x30
>>>> [  288.039337]  start_secondary+0x11e/0x140
>>>> [  288.039340]  common_startup_64+0x13e/0x141
>>>> [  288.039342]  </TASK>
>>>>
>>>> I think you need to swap the mutex to a spin_lock_irqsave and associated
>>>> spin_unlock_irqrestore plus DEFINE_SPINLOCK(asus_brt_lock).
>>>>
>>>> I'm out of time tonight but I'll apply the above to your patches and
>>>> report back tomorrow if you don't get to it before I do.
>>>>
>>>> It might be worth checking any other mutex uses in the LED path too.
>>>
>>> Thank you for catching that, I will replace the mutex with a spinlock.
>>> Might have to do with the WMI method being active as I got no such
>>> issue in my device.
>>
>> This might highlight the HID + WMI issue I was talking about in the
>> other replies and why i think the quirk table is still required.. Or
>> perhaps an alternative would be to have HID block the WMI method for the
>> 0x19b6 PID? There are approximately 30 laptops I know of with both
>> methods available.
>>
>> I just checked the DSDT dump for Ally again and it looks like those are
>> all good too. You might have lucked out and ended up with all devices
>> with no WMI keyboard brightness :D
> 
> Well we for sure will need to test a device that has both. The intent
> of this series is to align both the WMI and HID, which is why it is
> placed in WMI. If it NOOPs it should be ok.
> 
> However if the set noops and the get returns dummy data that might be an issue.

Unfortunately I don't recall the exact device now sorry. I though it was 
the GU605, but that like the GA605, ProArt, Ally, and Z13 all missing 
the WMI method, so those are fine.

This is an sample of some of the other laptops:

GL553VE.dsl
37871:                    If ((IIA0 == 0x00050021))
37872-                    {
37873-                        If (GLKB (One))
37874-                        {
37875-                            Local0 <<= 0x08
37876-                            Local0 += GLKB (0x02)
--
38581:                    If ((IIA0 == 0x00050021))
38582-                    {
38583-                        SLKB (IIA1)
38584-                        Return (One)
38585-                    }
38586-

GU603Z-3.10.dsl
90702:                    If ((IIA0 == 0x00050021))
90703-                    {
90704-                        Return (0xFFFFFFFE)
90705-                    }
90706-
90707-                    If ((IIA0 == 0x00100051))
--
91125:                    If ((IIA0 == 0x00050021))
91126-                    {
91127-                        ^^PC00.LPCB.EC0.SLKB (IIA1)
91128-                        Return (One)
91129-                    }
91130-

G713Q.dsl
8454:                If ((IIA0 == 0x00050021))
8455-                {
8456-                    Return (Zero)
8457-                }
8458-
8459-                If ((IIA0 == 0x00050031))
--
9092:                If ((IIA0 == 0x00050021))
9093-                {
9094-                    Return (Zero)
9095-                }

H7606WI.dsl
9881:                If ((IIA0 == 0x00050021))
9882-                {
9883-                    Local0 = Zero
9884-                    Local0 = ^^PCI0.SBRG.EC0.KBLL /* 
\_SB_.PCI0.SBRG.EC0_.KBLL */
9885-                    Local0 |= 0x00350000
9886-                    Return (Local0)
--
10663:                If ((IIA0 == 0x00050021))
10664-                {
10665-                    Local0 = Zero
10666-                    Local0 = (IIA1 & 0x83)
10667-                    ^^PCI0.SBRG.EC0.KBLL = Local0
10668-                    Return (One)

GU603-b310-dsdt.dsl
91115:                    If ((IIA0 == 0x00050011))
91116-                    {
91117-                        If ((IIA1 == 0x02))
91118-                        {
91119-                            ^^PC00.LPCB.EC0.BLCT = One
91120-                        }
91121-
91122-                        Return (One)
91123-                    }

GU502GU.dsl
59909:                    If ((IIA0 == 0x00050011))
59910-                    {
59911-                        If ((IIA1 == 0x02))
59912-                        {
59913-                            ^^PCI0.LPCB.EC0.BLCT = One
59914-                        }
59915-
59916-                        Return (One)
59917-                    }

Hopefully that's enough data-points to work with? Let me know if there's 
anything else i can provide or clarify.

>>>
>>>    I guess I will try to do a v3 today as that will hold back our kernel too.
>>>
>>>> Cheers,
>>>> Luke.
>>>>
>>>>>     /*
>>>>>      * These functions actually update the LED's, and are called from a
>>>>>      * workqueue. By doing this as separate work rather than when the LED
>>>>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
>>>>> index add04524031d8..2ac7912d8acd3 100644
>>>>> --- a/include/linux/platform_data/x86/asus-wmi.h
>>>>> +++ b/include/linux/platform_data/x86/asus-wmi.h
>>>>> @@ -162,11 +162,18 @@ struct asus_brt_listener {
>>>>>         void (*notify)(struct asus_brt_listener *listener, int brightness);
>>>>>     };
>>>>>
>>>>> +enum asus_brt_event {
>>>>> +     ASUS_BRT_UP,
>>>>> +     ASUS_BRT_DOWN,
>>>>> +     ASUS_BRT_TOGGLE,
>>>>> +};
>>>>> +
>>>>>     #if IS_REACHABLE(CONFIG_ASUS_WMI)
>>>>>     int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
>>>>>
>>>>>     int asus_brt_register_listener(struct asus_brt_listener *cdev);
>>>>>     void asus_brt_unregister_listener(struct asus_brt_listener *cdev);
>>>>> +int asus_brt_event(enum asus_brt_event event);
>>>>>     #else
>>>>>     static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
>>>>>                                            u32 *retval)
>>>>> @@ -181,6 +188,10 @@ static inline int asus_brt_register_listener(struct asus_brt_listener *bdev)
>>>>>     static inline void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
>>>>>     {
>>>>>     }
>>>>> +static inline int asus_brt_event(enum asus_brt_event event)
>>>>> +{
>>>>> +     return -ENODEV;
>>>>> +}
>>>>>     #endif
>>>>>
>>>>>     #endif      /* __PLATFORM_DATA_X86_ASUS_WMI_H */
>>>>
>>

Re: [PATCH 08/11] platform/x86: asus-wmi: add keyboard brightness event handler

[Antheas Kapenekakis]

On Sat, 22 Mar 2025 at 10:34, Luke D. Jones <luke@ljones.dev> wrote:
>
> On 22/03/25 22:13, Antheas Kapenekakis wrote:
> > On Sat, 22 Mar 2025 at 10:06, Luke D. Jones <luke@ljones.dev> wrote:
> >>
> >> On 22/03/25 21:12, Antheas Kapenekakis wrote:
> >>> On Sat, 22 Mar 2025 at 05:31, Luke D. Jones <luke@ljones.dev> wrote:
> >>>>
> >>>> On 21/03/25 11:09, Antheas Kapenekakis wrote:
> >>>>> Currenlty, the keyboard brightness control of Asus WMI keyboards is
> >>>>> handled in the kernel, which leads to the shortcut going from
> >>>>> brightness 0, to 1, to 2, and 3.
> >>>>>
> >>>>> However, for HID keyboards it is exposed as a key and handled by the
> >>>>> user's desktop environment. For the toggle button, this means that
> >>>>> brightness control becomes on/off. In addition, in the absence of a
> >>>>> DE, the keyboard brightness does not work.
> >>>>>
> >>>>> Therefore, expose an event handler for the keyboard brightness control
> >>>>> which can then be used by hid-asus.
> >>>>>
> >>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>>>> ---
> >>>>>     drivers/platform/x86/asus-wmi.c            | 38 ++++++++++++++++++++++
> >>>>>     include/linux/platform_data/x86/asus-wmi.h | 11 +++++++
> >>>>>     2 files changed, 49 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> >>>>> index 21e034be71b2f..45999dda9e7ed 100644
> >>>>> --- a/drivers/platform/x86/asus-wmi.c
> >>>>> +++ b/drivers/platform/x86/asus-wmi.c
> >>>>> @@ -1529,6 +1529,44 @@ void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
> >>>>>     }
> >>>>>     EXPORT_SYMBOL_GPL(asus_brt_unregister_listener);
> >>>>>
> >>>>> +static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
> >>>>> +
> >>>>> +int asus_brt_event(enum asus_brt_event event)
> >>>>> +{
> >>>>> +     int brightness;
> >>>>> +
> >>>>> +     mutex_lock(&asus_brt_lock);
> >>>>> +     if (!asus_brt_ref || !asus_brt_ref->kbd_led_registered) {
> >>>>> +             mutex_unlock(&asus_brt_lock);
> >>>>> +             return -EBUSY;
> >>>>> +     }
> >>>>> +     brightness = asus_brt_ref->kbd_led_wk;
> >>>>> +
> >>>>> +     switch (event) {
> >>>>> +     case ASUS_BRT_UP:
> >>>>> +             brightness += 1;
> >>>>> +             break;
> >>>>> +     case ASUS_BRT_DOWN:
> >>>>> +             brightness -= 1;
> >>>>> +             break;
> >>>>> +     case ASUS_BRT_TOGGLE:
> >>>>> +             if (brightness >= 3)
> >>>>> +                     brightness = 0;
> >>>>> +             else
> >>>>> +                     brightness += 1;
> >>>>> +             break;
> >>>>> +     }
> >>>>> +
> >>>>> +     do_kbd_led_set(&asus_brt_ref->kbd_led, brightness);
> >>>>> +     led_classdev_notify_brightness_hw_changed(&asus_brt_ref->kbd_led,
> >>>>> +                                               asus_brt_ref->kbd_led_wk);
> >>>>> +
> >>>>> +     mutex_unlock(&asus_brt_lock);
> >>>>> +
> >>>>> +     return 0;
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(asus_brt_event);
> >>>>> +
> >>>>
> >>>> I quick test on 6.14-rc7 gives me this:
> >>>>
> >>>> [  288.039255] BUG: sleeping function called from invalid context at
> >>>> kernel/locking/mutex.c:258
> >>>> [  288.039262] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0,
> >>>> name: swapper/17
> >>>> [  288.039263] preempt_count: 101, expected: 0
> >>>> [  288.039264] RCU nest depth: 0, expected: 0
> >>>> [  288.039266] CPU: 17 UID: 0 PID: 0 Comm: swapper/17 Tainted: G
> >>>> W          6.14.0-rc7+ #164
> >>>> [  288.039268] Tainted: [W]=WARN
> >>>> [  288.039269] Hardware name: ASUSTeK COMPUTER INC. ROG Zephyrus M16
> >>>> GU604VY_GU604VY_00130747B/GU604VY, BIOS GU604VY.313 08/13/2024
> >>>> [  288.039270] Call Trace:
> >>>> [  288.039272]  <IRQ>
> >>>> [  288.039273]  dump_stack_lvl+0x5d/0x80
> >>>> [  288.039277]  __might_resched.cold+0xba/0xc9
> >>>> [  288.039282]  mutex_lock+0x19/0x40
> >>>> [  288.039285]  asus_brt_event+0x13/0xb0 [asus_wmi]
> >>>> [  288.039292]  asus_event+0x91/0xa0 [hid_asus]
> >>>> [  288.039295]  hid_process_event+0xae/0x120
> >>>> [  288.039298]  hid_input_array_field+0x132/0x180
> >>>> [  288.039300]  hid_report_raw_event+0x360/0x4e0
> >>>> [  288.039302]  __hid_input_report.constprop.0+0xf1/0x180
> >>>> [  288.039304]  hid_irq_in+0x17f/0x1b0
> >>>> [  288.039306]  __usb_hcd_giveback_urb+0x98/0x110
> >>>> [  288.039308]  usb_giveback_urb_bh+0xbd/0x150
> >>>> [  288.039310]  process_one_work+0x171/0x290
> >>>> [  288.039312]  bh_worker+0x1ac/0x210
> >>>> [  288.039314]  tasklet_hi_action+0xe/0x30
> >>>> [  288.039315]  handle_softirqs+0xdb/0x1f0
> >>>> [  288.039317]  __irq_exit_rcu+0xc2/0xe0
> >>>> [  288.039318]  common_interrupt+0x85/0xa0
> >>>> [  288.039320]  </IRQ>
> >>>> [  288.039320]  <TASK>
> >>>> [  288.039321]  asm_common_interrupt+0x26/0x40
> >>>> [  288.039323] RIP: 0010:cpuidle_enter_state+0xb9/0x2c0
> >>>> [  288.039325] Code: 40 0f 84 1b 01 00 00 e8 35 e8 13 ff e8 40 f2 ff ff
> >>>> 31 ff 49 89 c5 e8 c6 f0 12 ff 45 84 f6 0f 85 f2 00 00 00 fb 0f 1f 44 00
> >>>> 00 <45> 85 ff 0f 88 cf 00 00 00 49 63 f7 48 8d 04 76 48 8d 04 86 49 8d
> >>>> [  288.039326] RSP: 0018:ffffc90000253e90 EFLAGS: 00000246
> >>>> [  288.039328] RAX: ffff888890680000 RBX: 0000000000000003 RCX:
> >>>> 0000000000000000
> >>>> [  288.039329] RDX: 00000043107862af RSI: fffffffd616e8210 RDI:
> >>>> 0000000000000000
> >>>> [  288.039329] RBP: ffff8888906ba370 R08: 0000000000000000 R09:
> >>>> 0000000000000007
> >>>> [  288.039330] R10: ffff88888ffa6098 R11: 0000000000000008 R12:
> >>>> ffffffff82fd4140
> >>>> [  288.039331] R13: 00000043107862af R14: 0000000000000000 R15:
> >>>> 0000000000000003
> >>>> [  288.039332]  cpuidle_enter+0x28/0x40
> >>>> [  288.039334]  do_idle+0x1a8/0x200
> >>>> [  288.039336]  cpu_startup_entry+0x24/0x30
> >>>> [  288.039337]  start_secondary+0x11e/0x140
> >>>> [  288.039340]  common_startup_64+0x13e/0x141
> >>>> [  288.039342]  </TASK>
> >>>>
> >>>> I think you need to swap the mutex to a spin_lock_irqsave and associated
> >>>> spin_unlock_irqrestore plus DEFINE_SPINLOCK(asus_brt_lock).
> >>>>
> >>>> I'm out of time tonight but I'll apply the above to your patches and
> >>>> report back tomorrow if you don't get to it before I do.
> >>>>
> >>>> It might be worth checking any other mutex uses in the LED path too.
> >>>
> >>> Thank you for catching that, I will replace the mutex with a spinlock.
> >>> Might have to do with the WMI method being active as I got no such
> >>> issue in my device.
> >>
> >> This might highlight the HID + WMI issue I was talking about in the
> >> other replies and why i think the quirk table is still required.. Or
> >> perhaps an alternative would be to have HID block the WMI method for the
> >> 0x19b6 PID? There are approximately 30 laptops I know of with both
> >> methods available.
> >>
> >> I just checked the DSDT dump for Ally again and it looks like those are
> >> all good too. You might have lucked out and ended up with all devices
> >> with no WMI keyboard brightness :D
> >
> > Well we for sure will need to test a device that has both. The intent
> > of this series is to align both the WMI and HID, which is why it is
> > placed in WMI. If it NOOPs it should be ok.
> >
> > However if the set noops and the get returns dummy data that might be an issue.
>
> Unfortunately I don't recall the exact device now sorry. I though it was
> the GU605, but that like the GA605, ProArt, Ally, and Z13 all missing
> the WMI method, so those are fine.
>
> This is an sample of some of the other laptops:
>
> GL553VE.dsl
> 37871:                    If ((IIA0 == 0x00050021))
> 37872-                    {
> 37873-                        If (GLKB (One))
> 37874-                        {
> 37875-                            Local0 <<= 0x08
> 37876-                            Local0 += GLKB (0x02)
> --
> 38581:                    If ((IIA0 == 0x00050021))
> 38582-                    {
> 38583-                        SLKB (IIA1)
> 38584-                        Return (One)
> 38585-                    }
> 38586-
>
> GU603Z-3.10.dsl
> 90702:                    If ((IIA0 == 0x00050021))
> 90703-                    {
> 90704-                        Return (0xFFFFFFFE)
> 90705-                    }
> 90706-
> 90707-                    If ((IIA0 == 0x00100051))
> --
> 91125:                    If ((IIA0 == 0x00050021))
> 91126-                    {
> 91127-                        ^^PC00.LPCB.EC0.SLKB (IIA1)
> 91128-                        Return (One)
> 91129-                    }
> 91130-
>
> G713Q.dsl
> 8454:                If ((IIA0 == 0x00050021))
> 8455-                {
> 8456-                    Return (Zero)
> 8457-                }
> 8458-
> 8459-                If ((IIA0 == 0x00050031))
> --
> 9092:                If ((IIA0 == 0x00050021))
> 9093-                {
> 9094-                    Return (Zero)
> 9095-                }
>
> H7606WI.dsl
> 9881:                If ((IIA0 == 0x00050021))
> 9882-                {
> 9883-                    Local0 = Zero
> 9884-                    Local0 = ^^PCI0.SBRG.EC0.KBLL /*
> \_SB_.PCI0.SBRG.EC0_.KBLL */
> 9885-                    Local0 |= 0x00350000
> 9886-                    Return (Local0)
> --
> 10663:                If ((IIA0 == 0x00050021))
> 10664-                {
> 10665-                    Local0 = Zero
> 10666-                    Local0 = (IIA1 & 0x83)
> 10667-                    ^^PCI0.SBRG.EC0.KBLL = Local0
> 10668-                    Return (One)
>
> GU603-b310-dsdt.dsl
> 91115:                    If ((IIA0 == 0x00050011))
> 91116-                    {
> 91117-                        If ((IIA1 == 0x02))
> 91118-                        {
> 91119-                            ^^PC00.LPCB.EC0.BLCT = One
> 91120-                        }
> 91121-
> 91122-                        Return (One)
> 91123-                    }
>
> GU502GU.dsl
> 59909:                    If ((IIA0 == 0x00050011))
> 59910-                    {
> 59911-                        If ((IIA1 == 0x02))
> 59912-                        {
> 59913-                            ^^PCI0.LPCB.EC0.BLCT = One
> 59914-                        }
> 59915-
> 59916-                        Return (One)
> 59917-                    }
>
> Hopefully that's enough data-points to work with? Let me know if there's
> anything else i can provide or clarify.
>
> >>>
> >>>    I guess I will try to do a v3 today as that will hold back our kernel too.
> >>>
> >>>> Cheers,
> >>>> Luke.
> >>>>
> >>>>>     /*
> >>>>>      * These functions actually update the LED's, and are called from a
> >>>>>      * workqueue. By doing this as separate work rather than when the LED
> >>>>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> >>>>> index add04524031d8..2ac7912d8acd3 100644
> >>>>> --- a/include/linux/platform_data/x86/asus-wmi.h
> >>>>> +++ b/include/linux/platform_data/x86/asus-wmi.h
> >>>>> @@ -162,11 +162,18 @@ struct asus_brt_listener {
> >>>>>         void (*notify)(struct asus_brt_listener *listener, int brightness);
> >>>>>     };
> >>>>>
> >>>>> +enum asus_brt_event {
> >>>>> +     ASUS_BRT_UP,
> >>>>> +     ASUS_BRT_DOWN,
> >>>>> +     ASUS_BRT_TOGGLE,
> >>>>> +};
> >>>>> +
> >>>>>     #if IS_REACHABLE(CONFIG_ASUS_WMI)
> >>>>>     int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
> >>>>>
> >>>>>     int asus_brt_register_listener(struct asus_brt_listener *cdev);
> >>>>>     void asus_brt_unregister_listener(struct asus_brt_listener *cdev);
> >>>>> +int asus_brt_event(enum asus_brt_event event);
> >>>>>     #else
> >>>>>     static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
> >>>>>                                            u32 *retval)
> >>>>> @@ -181,6 +188,10 @@ static inline int asus_brt_register_listener(struct asus_brt_listener *bdev)
> >>>>>     static inline void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
> >>>>>     {
> >>>>>     }
> >>>>> +static inline int asus_brt_event(enum asus_brt_event event)
> >>>>> +{
> >>>>> +     return -ENODEV;
> >>>>> +}
> >>>>>     #endif
> >>>>>
> >>>>>     #endif      /* __PLATFORM_DATA_X86_ASUS_WMI_H */
> >>>>
> >>
>

SBRG.EC0.KBLL being used should mean that the brightness is saved.
Since it is used as a backlight reference if it is stuck that would be
a problem. Other than that it should be OK.

The quirk also contains some laptop IDs, testing one of those should be enough.

Antheas

[PATCH 09/11] HID: asus: add support for the asus-wmi brightness handler

[Antheas Kapenekakis]

If the asus-wmi brightness handler is available, send the
keyboard brightness events to it instead of passing them
to userspace. If it is not, fall back to sending them to it.

Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/hid/hid-asus.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index b39d61b31ff23..546603f5193c7 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -319,6 +319,17 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
 			 usage->hid & HID_USAGE);
 	}
 
+	if (usage->type == EV_KEY && value) {
+		switch (usage->code) {
+		case KEY_KBDILLUMUP:
+			return !asus_brt_event(ASUS_BRT_UP);
+		case KEY_KBDILLUMDOWN:
+			return !asus_brt_event(ASUS_BRT_DOWN);
+		case KEY_KBDILLUMTOGGLE:
+			return !asus_brt_event(ASUS_BRT_TOGGLE);
+		}
+	}
+
 	return 0;
 }
 
-- 
2.48.1

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

[Antheas Kapenekakis]

Adds basic RGB support to hid-asus through multi-index. The interface
works quite well, but has not gone through much stability testing.
Applied on demand, if userspace does not touch the RGB sysfs, not
even initialization is done. Ensuring compatibility with existing
userspace programs.

Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/hid/hid-asus.c | 169 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 155 insertions(+), 14 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 546603f5193c7..5e87923b35520 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -30,6 +30,7 @@
 #include <linux/input/mt.h>
 #include <linux/usb.h> /* For to_usb_interface for T100 touchpad intf check */
 #include <linux/power_supply.h>
+#include <linux/led-class-multicolor.h>
 #include <linux/leds.h>
 
 #include "hid-ids.h"
@@ -85,6 +86,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 #define QUIRK_ROG_NKEY_KEYBOARD		BIT(11)
 #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
 #define QUIRK_HANDLE_GENERIC		BIT(13)
+#define QUIRK_ROG_NKEY_RGB		BIT(14)
 
 #define I2C_KEYBOARD_QUIRKS			(QUIRK_FIX_NOTEBOOK_REPORT | \
 						 QUIRK_NO_INIT_REPORTS | \
@@ -97,9 +99,15 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 
 struct asus_kbd_leds {
 	struct asus_brt_listener listener;
+	struct led_classdev_mc mc_led;
+	struct mc_subled subled_info[3];
 	struct hid_device *hdev;
 	struct work_struct work;
 	unsigned int brightness;
+	uint8_t rgb_colors[3];
+	bool rgb_init;
+	bool rgb_set;
+	bool rgb_registered;
 	spinlock_t lock;
 	bool removed;
 };
@@ -505,23 +513,67 @@ static void asus_schedule_work(struct asus_kbd_leds *led)
 	spin_unlock_irqrestore(&led->lock, flags);
 }
 
-static void asus_kbd_backlight_set(struct asus_brt_listener *listener,
+static void do_asus_kbd_backlight_set(struct asus_kbd_leds *led, int brightness)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&led->lock, flags);
+	led->brightness = brightness;
+	spin_unlock_irqrestore(&led->lock, flags);
+
+	asus_schedule_work(led);
+}
+
+static void asus_kbd_listener_set(struct asus_brt_listener *listener,
 				   int brightness)
 {
 	struct asus_kbd_leds *led = container_of(listener, struct asus_kbd_leds,
 						 listener);
+	do_asus_kbd_backlight_set(led, brightness);
+	if (led->rgb_registered) {
+		led->mc_led.led_cdev.brightness = brightness;
+		led_classdev_notify_brightness_hw_changed(&led->mc_led.led_cdev,
+							  brightness);
+	}
+}
+
+static void asus_kbd_brightness_set(struct led_classdev *led_cdev,
+				    enum led_brightness brightness)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(led_cdev);
+	struct asus_kbd_leds *led = container_of(mc_cdev, struct asus_kbd_leds,
+						 mc_led);
 	unsigned long flags;
 
 	spin_lock_irqsave(&led->lock, flags);
-	led->brightness = brightness;
+	led->rgb_colors[0] = mc_cdev->subled_info[0].intensity;
+	led->rgb_colors[1] = mc_cdev->subled_info[1].intensity;
+	led->rgb_colors[2] = mc_cdev->subled_info[2].intensity;
+	led->rgb_set = true;
 	spin_unlock_irqrestore(&led->lock, flags);
 
-	asus_schedule_work(led);
+	do_asus_kbd_backlight_set(led, brightness);
+}
+
+static enum led_brightness asus_kbd_brightness_get(struct led_classdev *led_cdev)
+{
+	struct led_classdev_mc *mc_led;
+	struct asus_kbd_leds *led;
+	enum led_brightness brightness;
+	unsigned long flags;
+
+	mc_led = lcdev_to_mccdev(led_cdev);
+	led = container_of(mc_led, struct asus_kbd_leds, mc_led);
+
+	spin_lock_irqsave(&led->lock, flags);
+	brightness = led->brightness;
+	spin_unlock_irqrestore(&led->lock, flags);
+
+	return brightness;
 }
 
-static void asus_kbd_backlight_work(struct work_struct *work)
+static void asus_kbd_backlight_work(struct asus_kbd_leds *led)
 {
-	struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work);
 	u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 };
 	int ret;
 	unsigned long flags;
@@ -535,10 +587,69 @@ static void asus_kbd_backlight_work(struct work_struct *work)
 		hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret);
 }
 
+static void asus_kbd_rgb_work(struct asus_kbd_leds *led)
+{
+	u8 rgb_buf[][7] = {
+		{ FEATURE_KBD_LED_REPORT_ID1, 0xB3 }, /* set mode */
+		{ FEATURE_KBD_LED_REPORT_ID1, 0xB5 }, /* apply mode */
+		{ FEATURE_KBD_LED_REPORT_ID1, 0xB4 }, /* save to mem */
+	};
+	unsigned long flags;
+	uint8_t colors[3];
+	bool rgb_init, rgb_set;
+	int ret;
+
+	spin_lock_irqsave(&led->lock, flags);
+	rgb_init = led->rgb_init;
+	rgb_set = led->rgb_set;
+	led->rgb_set = false;
+	colors[0] = led->rgb_colors[0];
+	colors[1] = led->rgb_colors[1];
+	colors[2] = led->rgb_colors[2];
+	spin_unlock_irqrestore(&led->lock, flags);
+
+	if (!rgb_set)
+		return;
+
+	if (rgb_init) {
+		ret = asus_kbd_init(led->hdev, FEATURE_KBD_LED_REPORT_ID1);
+		if (ret < 0) {
+			hid_err(led->hdev, "Asus failed to init RGB: %d\n", ret);
+			return;
+		}
+		spin_lock_irqsave(&led->lock, flags);
+		led->rgb_init = false;
+		spin_unlock_irqrestore(&led->lock, flags);
+	}
+
+	/* Protocol is: 54b3 zone (0=all) mode (0=solid) RGB */
+	rgb_buf[0][4] = colors[0];
+	rgb_buf[0][5] = colors[1];
+	rgb_buf[0][6] = colors[2];
+
+	for (size_t i = 0; i < ARRAY_SIZE(rgb_buf); i++) {
+		ret = asus_kbd_set_report(led->hdev, rgb_buf[i], sizeof(rgb_buf[i]));
+		if (ret < 0) {
+			hid_err(led->hdev, "Asus failed to set RGB: %d\n", ret);
+			return;
+		}
+	}
+}
+
+static void asus_kbd_work(struct work_struct *work)
+{
+	struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds,
+						 work);
+	asus_kbd_backlight_work(led);
+	asus_kbd_rgb_work(led);
+}
+
 static int asus_kbd_register_leds(struct hid_device *hdev)
 {
 	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
 	unsigned char kbd_func;
+	struct asus_kbd_leds *leds;
+	bool no_led;
 	int ret;
 
 	ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
@@ -566,21 +677,51 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
 	if (!drvdata->kbd_backlight)
 		return -ENOMEM;
 
-	drvdata->kbd_backlight->removed = false;
-	drvdata->kbd_backlight->brightness = 0;
-	drvdata->kbd_backlight->hdev = hdev;
-	drvdata->kbd_backlight->listener.notify = asus_kbd_backlight_set;
-	INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work);
+	leds = drvdata->kbd_backlight;
+	leds->removed = false;
+	leds->brightness = 3;
+	leds->hdev = hdev;
+	leds->listener.notify = asus_kbd_listener_set;
+
+	leds->rgb_colors[0] = 0;
+	leds->rgb_colors[1] = 0;
+	leds->rgb_colors[2] = 0;
+	leds->rgb_init = true;
+	leds->rgb_set = false;
+	leds->mc_led.led_cdev.name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
+					"asus-%s-led",
+					strlen(hdev->uniq) ?
+					hdev->uniq : dev_name(&hdev->dev));
+	leds->mc_led.led_cdev.flags = LED_BRIGHT_HW_CHANGED;
+	leds->mc_led.led_cdev.max_brightness = 3,
+	leds->mc_led.led_cdev.brightness_set = asus_kbd_brightness_set,
+	leds->mc_led.led_cdev.brightness_get = asus_kbd_brightness_get,
+	leds->mc_led.subled_info = leds->subled_info,
+	leds->mc_led.num_colors = ARRAY_SIZE(leds->subled_info),
+	leds->subled_info[0].color_index = LED_COLOR_ID_RED;
+	leds->subled_info[1].color_index = LED_COLOR_ID_GREEN;
+	leds->subled_info[2].color_index = LED_COLOR_ID_BLUE;
+
+	INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_work);
 	spin_lock_init(&drvdata->kbd_backlight->lock);
 
 	ret = asus_brt_register_listener(&drvdata->kbd_backlight->listener);
+	no_led = !!ret;
+
+	if (drvdata->quirks & QUIRK_ROG_NKEY_RGB) {
+		ret = devm_led_classdev_multicolor_register(
+			&hdev->dev, &leds->mc_led);
+		if (!ret)
+			leds->rgb_registered = true;
+		no_led &= !!ret;
+	}
 
-	if (ret < 0) {
+	if (no_led) {
 		/* No need to have this still around */
 		devm_kfree(&hdev->dev, drvdata->kbd_backlight);
 	}
 
-	return ret;
+	return no_led ? -ENODEV : 0;
 }
 
 /*
@@ -1305,7 +1446,7 @@ static const struct hid_device_id asus_devices[] = {
 	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
 	    USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
-	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
+	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
 	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY),
 	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
@@ -1334,7 +1475,7 @@ static const struct hid_device_id asus_devices[] = {
 	 */
 	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
 		USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_ROG_Z13_FOLIO),
-	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
+	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
 	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
 		USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_T101HA_KEYBOARD) },
 	{ }
-- 
2.48.1

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

[Luke D. Jones]

On 21/03/25 11:09, Antheas Kapenekakis wrote:
> Adds basic RGB support to hid-asus through multi-index. The interface
> works quite well, but has not gone through much stability testing.
> Applied on demand, if userspace does not touch the RGB sysfs, not
> even initialization is done. Ensuring compatibility with existing
> userspace programs.
> 
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
>   drivers/hid/hid-asus.c | 169 +++++++++++++++++++++++++++++++++++++----
>   1 file changed, 155 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 546603f5193c7..5e87923b35520 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -30,6 +30,7 @@
>   #include <linux/input/mt.h>
>   #include <linux/usb.h> /* For to_usb_interface for T100 touchpad intf check */
>   #include <linux/power_supply.h>
> +#include <linux/led-class-multicolor.h>
>   #include <linux/leds.h>
>   
>   #include "hid-ids.h"
> @@ -85,6 +86,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>   #define QUIRK_ROG_NKEY_KEYBOARD		BIT(11)
>   #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
>   #define QUIRK_HANDLE_GENERIC		BIT(13)
> +#define QUIRK_ROG_NKEY_RGB		BIT(14)
>   
>   #define I2C_KEYBOARD_QUIRKS			(QUIRK_FIX_NOTEBOOK_REPORT | \
>   						 QUIRK_NO_INIT_REPORTS | \
> @@ -97,9 +99,15 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>   
>   struct asus_kbd_leds {
>   	struct asus_brt_listener listener;
> +	struct led_classdev_mc mc_led;
> +	struct mc_subled subled_info[3];
>   	struct hid_device *hdev;
>   	struct work_struct work;
>   	unsigned int brightness;
> +	uint8_t rgb_colors[3];
> +	bool rgb_init;
> +	bool rgb_set;
> +	bool rgb_registered;
>   	spinlock_t lock;
>   	bool removed;
>   };
> @@ -505,23 +513,67 @@ static void asus_schedule_work(struct asus_kbd_leds *led)
>   	spin_unlock_irqrestore(&led->lock, flags);
>   }
>   
> -static void asus_kbd_backlight_set(struct asus_brt_listener *listener,
> +static void do_asus_kbd_backlight_set(struct asus_kbd_leds *led, int brightness)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&led->lock, flags);
> +	led->brightness = brightness;
> +	spin_unlock_irqrestore(&led->lock, flags);
> +
> +	asus_schedule_work(led);
> +}
> +
> +static void asus_kbd_listener_set(struct asus_brt_listener *listener,
>   				   int brightness)
>   {
>   	struct asus_kbd_leds *led = container_of(listener, struct asus_kbd_leds,
>   						 listener);
> +	do_asus_kbd_backlight_set(led, brightness);
> +	if (led->rgb_registered) {
> +		led->mc_led.led_cdev.brightness = brightness;
> +		led_classdev_notify_brightness_hw_changed(&led->mc_led.led_cdev,
> +							  brightness);
> +	}
> +}
> +
> +static void asus_kbd_brightness_set(struct led_classdev *led_cdev,
> +				    enum led_brightness brightness)
> +{
> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(led_cdev);
> +	struct asus_kbd_leds *led = container_of(mc_cdev, struct asus_kbd_leds,
> +						 mc_led);
>   	unsigned long flags;
>   
>   	spin_lock_irqsave(&led->lock, flags);
> -	led->brightness = brightness;
> +	led->rgb_colors[0] = mc_cdev->subled_info[0].intensity;
> +	led->rgb_colors[1] = mc_cdev->subled_info[1].intensity;
> +	led->rgb_colors[2] = mc_cdev->subled_info[2].intensity;
> +	led->rgb_set = true;
>   	spin_unlock_irqrestore(&led->lock, flags);
>   
> -	asus_schedule_work(led);
> +	do_asus_kbd_backlight_set(led, brightness);
> +}
> +
> +static enum led_brightness asus_kbd_brightness_get(struct led_classdev *led_cdev)
> +{
> +	struct led_classdev_mc *mc_led;
> +	struct asus_kbd_leds *led;
> +	enum led_brightness brightness;
> +	unsigned long flags;
> +
> +	mc_led = lcdev_to_mccdev(led_cdev);
> +	led = container_of(mc_led, struct asus_kbd_leds, mc_led);
> +
> +	spin_lock_irqsave(&led->lock, flags);
> +	brightness = led->brightness;
> +	spin_unlock_irqrestore(&led->lock, flags);
> +
> +	return brightness;
>   }
>   
> -static void asus_kbd_backlight_work(struct work_struct *work)
> +static void asus_kbd_backlight_work(struct asus_kbd_leds *led)
>   {
> -	struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work);
>   	u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 };
>   	int ret;
>   	unsigned long flags;
> @@ -535,10 +587,69 @@ static void asus_kbd_backlight_work(struct work_struct *work)
>   		hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret);
>   }
>   
> +static void asus_kbd_rgb_work(struct asus_kbd_leds *led)
> +{
> +	u8 rgb_buf[][7] = {
> +		{ FEATURE_KBD_LED_REPORT_ID1, 0xB3 }, /* set mode */
> +		{ FEATURE_KBD_LED_REPORT_ID1, 0xB5 }, /* apply mode */
> +		{ FEATURE_KBD_LED_REPORT_ID1, 0xB4 }, /* save to mem */
> +	};
> +	unsigned long flags;
> +	uint8_t colors[3];
> +	bool rgb_init, rgb_set;
> +	int ret;
> +
> +	spin_lock_irqsave(&led->lock, flags);
> +	rgb_init = led->rgb_init;
> +	rgb_set = led->rgb_set;
> +	led->rgb_set = false;
> +	colors[0] = led->rgb_colors[0];
> +	colors[1] = led->rgb_colors[1];
> +	colors[2] = led->rgb_colors[2];
> +	spin_unlock_irqrestore(&led->lock, flags);
> +
> +	if (!rgb_set)
> +		return;
> +
> +	if (rgb_init) {
> +		ret = asus_kbd_init(led->hdev, FEATURE_KBD_LED_REPORT_ID1);
> +		if (ret < 0) {
> +			hid_err(led->hdev, "Asus failed to init RGB: %d\n", ret);
> +			return;
> +		}
> +		spin_lock_irqsave(&led->lock, flags);
> +		led->rgb_init = false;
> +		spin_unlock_irqrestore(&led->lock, flags);
> +	}
> +
> +	/* Protocol is: 54b3 zone (0=all) mode (0=solid) RGB */
> +	rgb_buf[0][4] = colors[0];
> +	rgb_buf[0][5] = colors[1];
> +	rgb_buf[0][6] = colors[2];
> +
> +	for (size_t i = 0; i < ARRAY_SIZE(rgb_buf); i++) {
> +		ret = asus_kbd_set_report(led->hdev, rgb_buf[i], sizeof(rgb_buf[i]));
> +		if (ret < 0) {
> +			hid_err(led->hdev, "Asus failed to set RGB: %d\n", ret);
> +			return;
> +		}
> +	}
> +}
> +
> +static void asus_kbd_work(struct work_struct *work)
> +{
> +	struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds,
> +						 work);
> +	asus_kbd_backlight_work(led);
> +	asus_kbd_rgb_work(led);
> +}
> +
>   static int asus_kbd_register_leds(struct hid_device *hdev)
>   {
>   	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>   	unsigned char kbd_func;
> +	struct asus_kbd_leds *leds;
> +	bool no_led;
>   	int ret;
>   
>   	ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> @@ -566,21 +677,51 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>   	if (!drvdata->kbd_backlight)
>   		return -ENOMEM;
>   
> -	drvdata->kbd_backlight->removed = false;
> -	drvdata->kbd_backlight->brightness = 0;
> -	drvdata->kbd_backlight->hdev = hdev;
> -	drvdata->kbd_backlight->listener.notify = asus_kbd_backlight_set;
> -	INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work);
> +	leds = drvdata->kbd_backlight;
> +	leds->removed = false;
> +	leds->brightness = 3;
> +	leds->hdev = hdev;
> +	leds->listener.notify = asus_kbd_listener_set;
> +
> +	leds->rgb_colors[0] = 0;
> +	leds->rgb_colors[1] = 0;
> +	leds->rgb_colors[2] = 0;
> +	leds->rgb_init = true;
> +	leds->rgb_set = false;
> +	leds->mc_led.led_cdev.name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
> +					"asus-%s-led",
> +					strlen(hdev->uniq) ?
> +					hdev->uniq : dev_name(&hdev->dev));
> +	leds->mc_led.led_cdev.flags = LED_BRIGHT_HW_CHANGED;
> +	leds->mc_led.led_cdev.max_brightness = 3,
> +	leds->mc_led.led_cdev.brightness_set = asus_kbd_brightness_set,
> +	leds->mc_led.led_cdev.brightness_get = asus_kbd_brightness_get,
> +	leds->mc_led.subled_info = leds->subled_info,
> +	leds->mc_led.num_colors = ARRAY_SIZE(leds->subled_info),
> +	leds->subled_info[0].color_index = LED_COLOR_ID_RED;
> +	leds->subled_info[1].color_index = LED_COLOR_ID_GREEN;
> +	leds->subled_info[2].color_index = LED_COLOR_ID_BLUE;
> +
> +	INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_work);
>   	spin_lock_init(&drvdata->kbd_backlight->lock);
>   
>   	ret = asus_brt_register_listener(&drvdata->kbd_backlight->listener);
> +	no_led = !!ret;
> +
> +	if (drvdata->quirks & QUIRK_ROG_NKEY_RGB) {
> +		ret = devm_led_classdev_multicolor_register(
> +			&hdev->dev, &leds->mc_led);
> +		if (!ret)
> +			leds->rgb_registered = true;
> +		no_led &= !!ret;
> +	}
>   
> -	if (ret < 0) {
> +	if (no_led) {
>   		/* No need to have this still around */
>   		devm_kfree(&hdev->dev, drvdata->kbd_backlight);
>   	}
>   
> -	return ret;
> +	return no_led ? -ENODEV : 0;
>   }
>   
>   /*
> @@ -1305,7 +1446,7 @@ static const struct hid_device_id asus_devices[] = {
>   	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>   	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>   	    USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
> -	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> +	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
>   	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>   	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY),
>   	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> @@ -1334,7 +1475,7 @@ static const struct hid_device_id asus_devices[] = {
>   	 */
>   	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
>   		USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_ROG_Z13_FOLIO),
> -	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> +	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
>   	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
>   		USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_T101HA_KEYBOARD) },
>   	{ }

Hi Antheas,

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

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

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

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

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

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

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

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

Cheers,
Luke.

[PATCH 11/11] HID: asus: add RGB support to the ROG Ally units

[Antheas Kapenekakis]

Apply the RGB quirk to the QOG Ally units to enable basic RGB support.

Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/hid/hid-asus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 5e87923b35520..589b32b508bbf 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -1449,10 +1449,10 @@ static const struct hid_device_id asus_devices[] = {
 	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
 	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY),
-	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
+	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
 	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X),
-	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
+	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
 	    USB_DEVICE_ID_ASUSTEK_ROG_CLAYMORE_II_KEYBOARD),
 	  QUIRK_ROG_CLAYMORE_II_KEYBOARD },
-- 
2.48.1

Re: [PATCH 11/11] HID: asus: add RGB support to the ROG Ally units

[Luke D. Jones]

On 21/03/25 11:09, Antheas Kapenekakis wrote:
> Apply the RGB quirk to the QOG Ally units to enable basic RGB support.
> 
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
>   drivers/hid/hid-asus.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 5e87923b35520..589b32b508bbf 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -1449,10 +1449,10 @@ static const struct hid_device_id asus_devices[] = {
>   	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
>   	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>   	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY),
> -	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> +	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
>   	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>   	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X),
> -	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> +	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
>   	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>   	    USB_DEVICE_ID_ASUSTEK_ROG_CLAYMORE_II_KEYBOARD),
>   	  QUIRK_ROG_CLAYMORE_II_KEYBOARD },

I need to NACK this one sorry, if only because I added the RGB control 
in hid-asus-ally as a per-LED control and it works very well. You'll see 
it once I submit that series upstream again.

The distinction between MCU mode and Software mode for RGB is frankly a 
pain in the arse. For Ally we will want software mode (per-led) as that 
allows just one USB write for all LEDs, and no need to do a set/apply to 
make the LEDs change. The benefit being that the LEDs can change rapidly 
and there will be no "blink".

I'll write more on patch 10

Cheers,
Luke.

Re: [PATCH 11/11] HID: asus: add RGB support to the ROG Ally units

[Antheas Kapenekakis]

On Sat, 22 Mar 2025 at 03:30, Luke D. Jones <luke@ljones.dev> wrote:
>
> On 21/03/25 11:09, Antheas Kapenekakis wrote:
> > Apply the RGB quirk to the QOG Ally units to enable basic RGB support.
> >
> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > ---
> >   drivers/hid/hid-asus.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> > index 5e87923b35520..589b32b508bbf 100644
> > --- a/drivers/hid/hid-asus.c
> > +++ b/drivers/hid/hid-asus.c
> > @@ -1449,10 +1449,10 @@ static const struct hid_device_id asus_devices[] = {
> >         QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
> >       { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >           USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY),
> > -       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> > +       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
> >       { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >           USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X),
> > -       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> > +       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
> >       { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >           USB_DEVICE_ID_ASUSTEK_ROG_CLAYMORE_II_KEYBOARD),
> >         QUIRK_ROG_CLAYMORE_II_KEYBOARD },
>
> I need to NACK this one sorry, if only because I added the RGB control
> in hid-asus-ally as a per-LED control and it works very well. You'll see
> it once I submit that series upstream again.

Depending on when your driver is ready to merge, it might be
beneficial for this to merge ahead of time for some basic support.
Then you can yank it after your driver is in. For your driver, I think
it would be good to make it separate from hid-asus and yank ally
completely from here.

> The distinction between MCU mode and Software mode for RGB is frankly a
> pain in the arse. For Ally we will want software mode (per-led) as that
> allows just one USB write for all LEDs, and no need to do a set/apply to
> make the LEDs change. The benefit being that the LEDs can change rapidly
> and there will be no "blink".

The blink happens when the B4(/B5) command is sent. If they are not,
it is perfectly fine. B4 just needs to be sent initially, as it
switches the controller to solid mode if it is not there already. Then
B4/B5 could be sent on shutdown to save the color to memory. I
purposely did not do it as it would break the driver if userspace
controls the leds inbetween led switches and it is needlessly
complicated for what this support is meant for (basic RGB).

Also, multizone is expected to be of little utility, so it is not
worth making concessions over. I never found a use for it or anyone
ask for it. If single zone has performance benefits, it should be used
instead. A lot of devices do not support multizone, and when they do,
it is in different forms, so it is not something that can be
intuitively put into a kernel driver imo.

Aura mode is especially buggy during boot and resume, since the
controller briefly switches from the MCU mode to that, so it uses a
stale color which is ugly. Even when you try to match the colors, as
you did, it is not 1-1. You know this too. In addition, Aura mode will
break userspace Aura programs running through Wine. Although they are
already broken due to the hiddevs merging which I had to revert for
V2. But we could fix that, and I will try to for V3.

> I'll write more on patch 10
>
> Cheers,
> Luke.

Re: [PATCH 11/11] HID: asus: add RGB support to the ROG Ally units

[Luke D. Jones]

On 22/03/25 20:56, Antheas Kapenekakis wrote:
> On Sat, 22 Mar 2025 at 03:30, Luke D. Jones <luke@ljones.dev> wrote:
>>
>> On 21/03/25 11:09, Antheas Kapenekakis wrote:
>>> Apply the RGB quirk to the QOG Ally units to enable basic RGB support.
>>>
>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>> ---
>>>    drivers/hid/hid-asus.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>> index 5e87923b35520..589b32b508bbf 100644
>>> --- a/drivers/hid/hid-asus.c
>>> +++ b/drivers/hid/hid-asus.c
>>> @@ -1449,10 +1449,10 @@ static const struct hid_device_id asus_devices[] = {
>>>          QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
>>>        { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>            USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY),
>>> -       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>> +       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
>>>        { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>            USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X),
>>> -       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>> +       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
>>>        { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>            USB_DEVICE_ID_ASUSTEK_ROG_CLAYMORE_II_KEYBOARD),
>>>          QUIRK_ROG_CLAYMORE_II_KEYBOARD },
>>
>> I need to NACK this one sorry, if only because I added the RGB control
>> in hid-asus-ally as a per-LED control and it works very well. You'll see
>> it once I submit that series upstream again.
> 
> Depending on when your driver is ready to merge, it might be
> beneficial for this to merge ahead of time for some basic support.
> Then you can yank it after your driver is in. For your driver, I think
> it would be good to make it separate from hid-asus and yank ally
> completely from here.

The driver is fully standalone and that is what I do yes.

I do think it would be better for you to do the RGB part separate to the 
main lot of patches as those can definitely be signed off and merged a 
lot quicker. You still have the bazzite kernel right? I hope it's 
acceptable to carry just the RGB there for a tiny bit longer.

>> The distinction between MCU mode and Software mode for RGB is frankly a
>> pain in the arse. For Ally we will want software mode (per-led) as that
>> allows just one USB write for all LEDs, and no need to do a set/apply to
>> make the LEDs change. The benefit being that the LEDs can change rapidly
>> and there will be no "blink".
> 
> The blink happens when the B4(/B5) command is sent. If they are not,
> it is perfectly fine. B4 just needs to be sent initially, as it
> switches the controller to solid mode if it is not there already. Then
> B4/B5 could be sent on shutdown to save the color to memory. I
> purposely did not do it as it would break the driver if userspace
> controls the leds inbetween led switches and it is needlessly
> complicated for what this support is meant for (basic RGB).

Hmm, I thought the colour wasn't actually applied until at least a "set" 
was sent. Maybe it's on older devices.. I haven't looked at that for a 
while now.

> Also, multizone is expected to be of little utility, so it is not
> worth making concessions over. I never found a use for it or anyone
> ask for it. If single zone has performance benefits, it should be used
> instead. A lot of devices do not support multizone, and when they do,
> it is in different forms, so it is not something that can be
> intuitively put into a kernel driver imo.

Would you like to know how many varieties of single, multi, and per key 
there are? I have a rather large spread sheet tracking everything so 
far. Per-key + bars is something like 12-13 packets to send :|

> Aura mode is especially buggy during boot and resume, since the
> controller briefly switches from the MCU mode to that, so it uses a
> stale color which is ugly. Even when you try to match the colors, as
> you did, it is not 1-1. You know this too. In addition, Aura mode will
> break userspace Aura programs running through Wine. Although they are
> already broken due to the hiddevs merging which I had to revert for
> V2. But we could fix that, and I will try to for V3.

Aura programs can set the device back to MCU mode. Or they should be 
able to. The RGB setup is done only when called through the mcled class, 
and on suspend (to colour match and set static).

Cheers,
Luke.

>> I'll write more on patch 10
>>
>> Cheers,
>> Luke.

Re: [PATCH 11/11] HID: asus: add RGB support to the ROG Ally units

[Antheas Kapenekakis]

On Sat, 22 Mar 2025 at 10:53, Luke D. Jones <luke@ljones.dev> wrote:
>
> On 22/03/25 20:56, Antheas Kapenekakis wrote:
> > On Sat, 22 Mar 2025 at 03:30, Luke D. Jones <luke@ljones.dev> wrote:
> >>
> >> On 21/03/25 11:09, Antheas Kapenekakis wrote:
> >>> Apply the RGB quirk to the QOG Ally units to enable basic RGB support.
> >>>
> >>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>> ---
> >>>    drivers/hid/hid-asus.c | 4 ++--
> >>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> >>> index 5e87923b35520..589b32b508bbf 100644
> >>> --- a/drivers/hid/hid-asus.c
> >>> +++ b/drivers/hid/hid-asus.c
> >>> @@ -1449,10 +1449,10 @@ static const struct hid_device_id asus_devices[] = {
> >>>          QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
> >>>        { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>            USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY),
> >>> -       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>> +       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
> >>>        { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>            USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X),
> >>> -       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>> +       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
> >>>        { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>            USB_DEVICE_ID_ASUSTEK_ROG_CLAYMORE_II_KEYBOARD),
> >>>          QUIRK_ROG_CLAYMORE_II_KEYBOARD },
> >>
> >> I need to NACK this one sorry, if only because I added the RGB control
> >> in hid-asus-ally as a per-LED control and it works very well. You'll see
> >> it once I submit that series upstream again.
> >
> > Depending on when your driver is ready to merge, it might be
> > beneficial for this to merge ahead of time for some basic support.
> > Then you can yank it after your driver is in. For your driver, I think
> > it would be good to make it separate from hid-asus and yank ally
> > completely from here.
>
> The driver is fully standalone and that is what I do yes.
>
> I do think it would be better for you to do the RGB part separate to the
> main lot of patches as those can definitely be signed off and merged a
> lot quicker. You still have the bazzite kernel right? I hope it's
> acceptable to carry just the RGB there for a tiny bit longer.

Sure, I will keep it for v3 as that is almost done now but afaik it
does not have to merge together.

> >> The distinction between MCU mode and Software mode for RGB is frankly a
> >> pain in the arse. For Ally we will want software mode (per-led) as that
> >> allows just one USB write for all LEDs, and no need to do a set/apply to
> >> make the LEDs change. The benefit being that the LEDs can change rapidly
> >> and there will be no "blink".
> >
> > The blink happens when the B4(/B5) command is sent. If they are not,
> > it is perfectly fine. B4 just needs to be sent initially, as it
> > switches the controller to solid mode if it is not there already. Then
> > B4/B5 could be sent on shutdown to save the color to memory. I
> > purposely did not do it as it would break the driver if userspace
> > controls the leds inbetween led switches and it is needlessly
> > complicated for what this support is meant for (basic RGB).
>
> Hmm, I thought the colour wasn't actually applied until at least a "set"
> was sent. Maybe it's on older devices.. I haven't looked at that for a
> while now.

I would have to recheck, but I am pretty sure that as long as you are
in the solid mode, color changes apply instantly. There are no flashes
on my end.

> > Also, multizone is expected to be of little utility, so it is not
> > worth making concessions over. I never found a use for it or anyone
> > ask for it. If single zone has performance benefits, it should be used
> > instead. A lot of devices do not support multizone, and when they do,
> > it is in different forms, so it is not something that can be
> > intuitively put into a kernel driver imo.
>
> Would you like to know how many varieties of single, multi, and per key
> there are? I have a rather large spread sheet tracking everything so
> far. Per-key + bars is something like 12-13 packets to send :|

I think when it comes to the kernel, doing just a solid color would go
a long way.

> > Aura mode is especially buggy during boot and resume, since the
> > controller briefly switches from the MCU mode to that, so it uses a
> > stale color which is ugly. Even when you try to match the colors, as
> > you did, it is not 1-1. You know this too. In addition, Aura mode will
> > break userspace Aura programs running through Wine. Although they are
> > already broken due to the hiddevs merging which I had to revert for
> > V2. But we could fix that, and I will try to for V3.
>
> Aura programs can set the device back to MCU mode. Or they should be
> able to. The RGB setup is done only when called through the mcled class,
> and on suspend (to colour match and set static).

I recall one of the previous versions of your patch doing a dirty
brightness set during the controller init. If you fix that it should
be alright.

Antheas

> Cheers,
> Luke.
>
> >> I'll write more on patch 10
> >>
> >> Cheers,
> >> Luke.
>

Re: [PATCH 00/11] HID: Asus: Add RGB Support to Asus Z13, Ally, unify backlight asus-wmi, and Z13 QOL

[Luke D. Jones]

On 21/03/25 11:09, Antheas Kapenekakis wrote:
> This is a three part series which does the following:
>    - Clean init sequence, fix the keyboard of the Z13 (touchpad,fan button)
>    - Unifies backlight handling to happen under asus-wmi so that all Aura
>      devices have synced brightness controls and the backlight button works
>      properly when it is on a USB laptop keyboard instead of one w/ WMI.
>    - Adds RGB support to hid-asus, solid colors only, and for the ROG Ally
>      units and the Asus Z13 2025 first.
> 
> For context, see cover letter of V1.
> 
> The last two patches are still a bit experimental, the rest is getting to
> be pretty stable by now. I will test my Ally in the weekend. Also, I am
> not a fan of the asus-0003:0B05:1A30.0001-led name, so suggestions would
> be appreciated.
> 
> ---
> V1: https://lore.kernel.org/all/20250319191320.10092-1-lkml@antheas.dev/

Hi Antheas, just a very quick note before I review - did you forget to 
add `-v2` to git format-patch? Don't do it now, it's just a reminder for 
next version.

Also I guess asus_brt_ means asus_bright_, but maybe we can rename to 
asus_led_ or even asus_rgbw_? I think something a tad more descriptive 
while still keeping the length down would help future contributors 
quickly understand intent. I'll mention it again when I get to that 
actual patch during test and review.

Looks like good progress so far.

Cheers,
Luke.

> Changes since V1:
>    - Add basic RGB support on hid-asus, (Z13/Ally) tested in KDE/Z13
>    - Fix ifdef else having an invalid signature (reported by kernel robot)
>    - Restore input arguments to init and keyboard function so they can
>      be re-used for RGB controls.
>    - Remove Z13 delay (it did not work to fix the touchpad) and replace it
>      with a HID_GROUP_GENERIC quirk to allow hid-multitouch to load. Squash
>      keyboard rename into it.
>    - Unregister brightness listener before removing work queue to avoid
>      a race condition causing corruption
>    - Remove spurious mutex unlock in asus_brt_event
>    - Place mutex lock in kbd_led_set after LED_UNREGISTERING check to avoid
>      relocking the mutex and causing a deadlock when unregistering leds
>    - Add extra check during unregistering to avoid calling unregister when
>      no led device is registered.
>    - Temporarily HID_QUIRK_INPUT_PER_APP from the ROG endpoint as it causes
>      the driver to create 4 RGB handlers per device. I also suspect some
>      extra events sneak through (KDE had the @@@@@@).
> 
> Antheas Kapenekakis (11):
>    HID: asus: refactor init sequence per spec
>    HID: asus: prevent binding to all HID devices on ROG
>    HID: asus: add Asus Z13 2025 Fan key
>    HID: Asus: add Z13 folio to generic group for multitouch to work
>    platform/x86: asus-wmi: Add support for multiple kbd RGB handlers
>    HID: asus: listen to the asus-wmi brightness device instead of
>      creating one
>    platform/x86: asus-wmi: remove unused keyboard backlight quirk
>    platform/x86: asus-wmi: add keyboard brightness event handler
>    HID: asus: add support for the asus-wmi brightness handler
>    HID: asus: add basic RGB support
>    HID: asus: add RGB support to the ROG Ally units
> 
>   drivers/hid/hid-asus.c                     | 342 ++++++++++++++++-----
>   drivers/hid/hid-ids.h                      |   2 +-
>   drivers/platform/x86/asus-wmi.c            | 138 ++++++++-
>   include/linux/platform_data/x86/asus-wmi.h |  67 ++--
>   4 files changed, 411 insertions(+), 138 deletions(-)
> 
> 
> base-commit: 4701f33a10702d5fc577c32434eb62adde0a1ae1

Re: [PATCH 00/11] HID: Asus: Add RGB Support to Asus Z13, Ally, unify backlight asus-wmi, and Z13 QOL

[Antheas Kapenekakis]

On Fri, 21 Mar 2025 at 01:03, Luke D. Jones <luke@ljones.dev> wrote:
>
> On 21/03/25 11:09, Antheas Kapenekakis wrote:
> > This is a three part series which does the following:
> >    - Clean init sequence, fix the keyboard of the Z13 (touchpad,fan button)
> >    - Unifies backlight handling to happen under asus-wmi so that all Aura
> >      devices have synced brightness controls and the backlight button works
> >      properly when it is on a USB laptop keyboard instead of one w/ WMI.
> >    - Adds RGB support to hid-asus, solid colors only, and for the ROG Ally
> >      units and the Asus Z13 2025 first.
> >
> > For context, see cover letter of V1.
> >
> > The last two patches are still a bit experimental, the rest is getting to
> > be pretty stable by now. I will test my Ally in the weekend. Also, I am
> > not a fan of the asus-0003:0B05:1A30.0001-led name, so suggestions would
> > be appreciated.
> >
> > ---
> > V1: https://lore.kernel.org/all/20250319191320.10092-1-lkml@antheas.dev/
>
> Hi Antheas, just a very quick note before I review - did you forget to
> add `-v2` to git format-patch? Don't do it now, it's just a reminder for
> next version.

Yes I did. It's been a long day.

> Also I guess asus_brt_ means asus_bright_, but maybe we can rename to
> asus_led_ or even asus_rgbw_? I think something a tad more descriptive
> while still keeping the length down would help future contributors
> quickly understand intent. I'll mention it again when I get to that
> actual patch during test and review.

I am not particularly happy with brt either, I chose it because the
events are named BRT. rgbw is a bit misleading, the notifier will
never do RGB. Since Aura devices can hotplug they need their own led
device. It will always passthrough brightness only. Perhaps led is
misleading for that reason as well. Bright seems a bit weird, and
brightness seems a bit long. So I am a bit stuck

What I think would be better is to refocus from leds, to maybe
hid_ref. As I'd like the fan key to passthrough to asus-wmi too to
cycle the profiles. I'd also like to tweak the profile cycling
behavior a bit and make it customizable. But very minor changes, just
to cycling behavior. Essentially, I want to get to a point where doing
Fn+Fan cycles the profiles properly without userspace, and then
userspace can take over the cycler and update the KDE slider live.

However, that refactor on 6.14 for platform profiles was brutal. So I
have to wait for fedora to get on 6.14 for me to even start thinking
about that. Otherwise I will not be able to deploy any changes on
Bazzite. I currently carry 287 patches (~100 is XDNA+NTSYNC+platform
profiles minus Kurt's series) for 6.13. I would also like try to
target Thinkpads too, and maybe the Legion Go with that, but it
depends on how much progress Derek makes on his driver by then.

As far as the Z13 is concerned, it will be this patch series + 1-3
patches to tweak the ROG button on the side to stop acting like a wifi
killswitch.

Antheas

> Looks like good progress so far.
>
> Cheers,
> Luke.
>
> > Changes since V1:
> >    - Add basic RGB support on hid-asus, (Z13/Ally) tested in KDE/Z13
> >    - Fix ifdef else having an invalid signature (reported by kernel robot)
> >    - Restore input arguments to init and keyboard function so they can
> >      be re-used for RGB controls.
> >    - Remove Z13 delay (it did not work to fix the touchpad) and replace it
> >      with a HID_GROUP_GENERIC quirk to allow hid-multitouch to load. Squash
> >      keyboard rename into it.
> >    - Unregister brightness listener before removing work queue to avoid
> >      a race condition causing corruption
> >    - Remove spurious mutex unlock in asus_brt_event
> >    - Place mutex lock in kbd_led_set after LED_UNREGISTERING check to avoid
> >      relocking the mutex and causing a deadlock when unregistering leds
> >    - Add extra check during unregistering to avoid calling unregister when
> >      no led device is registered.
> >    - Temporarily HID_QUIRK_INPUT_PER_APP from the ROG endpoint as it causes
> >      the driver to create 4 RGB handlers per device. I also suspect some
> >      extra events sneak through (KDE had the @@@@@@).
> >
> > Antheas Kapenekakis (11):
> >    HID: asus: refactor init sequence per spec
> >    HID: asus: prevent binding to all HID devices on ROG
> >    HID: asus: add Asus Z13 2025 Fan key
> >    HID: Asus: add Z13 folio to generic group for multitouch to work
> >    platform/x86: asus-wmi: Add support for multiple kbd RGB handlers
> >    HID: asus: listen to the asus-wmi brightness device instead of
> >      creating one
> >    platform/x86: asus-wmi: remove unused keyboard backlight quirk
> >    platform/x86: asus-wmi: add keyboard brightness event handler
> >    HID: asus: add support for the asus-wmi brightness handler
> >    HID: asus: add basic RGB support
> >    HID: asus: add RGB support to the ROG Ally units
> >
> >   drivers/hid/hid-asus.c                     | 342 ++++++++++++++++-----
> >   drivers/hid/hid-ids.h                      |   2 +-
> >   drivers/platform/x86/asus-wmi.c            | 138 ++++++++-
> >   include/linux/platform_data/x86/asus-wmi.h |  67 ++--
> >   4 files changed, 411 insertions(+), 138 deletions(-)
> >
> >
> > base-commit: 4701f33a10702d5fc577c32434eb62adde0a1ae1
>

Re: [PATCH 00/11] HID: Asus: Add RGB Support to Asus Z13, Ally, unify backlight asus-wmi, and Z13 QOL

[Luke D. Jones]

On 21/03/25 13:23, Antheas Kapenekakis wrote:
> On Fri, 21 Mar 2025 at 01:03, Luke D. Jones <luke@ljones.dev> wrote:
>>
>> On 21/03/25 11:09, Antheas Kapenekakis wrote:
>>> This is a three part series which does the following:
>>>     - Clean init sequence, fix the keyboard of the Z13 (touchpad,fan button)
>>>     - Unifies backlight handling to happen under asus-wmi so that all Aura
>>>       devices have synced brightness controls and the backlight button works
>>>       properly when it is on a USB laptop keyboard instead of one w/ WMI.
>>>     - Adds RGB support to hid-asus, solid colors only, and for the ROG Ally
>>>       units and the Asus Z13 2025 first.
>>>
>>> For context, see cover letter of V1.
>>>
>>> The last two patches are still a bit experimental, the rest is getting to
>>> be pretty stable by now. I will test my Ally in the weekend. Also, I am
>>> not a fan of the asus-0003:0B05:1A30.0001-led name, so suggestions would
>>> be appreciated.
>>>
>>> ---
>>> V1: https://lore.kernel.org/all/20250319191320.10092-1-lkml@antheas.dev/
>>
>> Hi Antheas, just a very quick note before I review - did you forget to
>> add `-v2` to git format-patch? Don't do it now, it's just a reminder for
>> next version.
> 
> Yes I did. It's been a long day.

Fair enough mate, know how that feels.

>> Also I guess asus_brt_ means asus_bright_, but maybe we can rename to
>> asus_led_ or even asus_rgbw_? I think something a tad more descriptive
>> while still keeping the length down would help future contributors
>> quickly understand intent. I'll mention it again when I get to that
>> actual patch during test and review.
> 
> I am not particularly happy with brt either, I chose it because the
> events are named BRT. rgbw is a bit misleading, the notifier will
> never do RGB. Since Aura devices can hotplug they need their own led
> device. It will always passthrough brightness only. Perhaps led is
> misleading for that reason as well. Bright seems a bit weird, and
> brightness seems a bit long. So I am a bit stuck
> 
> What I think would be better is to refocus from leds, to maybe
> hid_ref. As I'd like the fan key to passthrough to asus-wmi too to
> cycle the profiles. I'd also like to tweak the profile cycling
> behavior a bit and make it customizable. But very minor changes, just
> to cycling behavior. Essentially, I want to get to a point where doing
> Fn+Fan cycles the profiles properly without userspace, and then
> userspace can take over the cycler and update the KDE slider live.

Personally I'd like to see that. I intended to do something similar last 
year but just never got a round to it. I'd rather move everything that's 
logical to move, from userspace to kernel. Custom ordering would be 
great to see, a logical profile progression etc, lets get this series 
landed first.

> However, that refactor on 6.14 for platform profiles was brutal. So I
> have to wait for fedora to get on 6.14 for me to even start thinking
> about that. Otherwise I will not be able to deploy any changes on

About that - I'm working on it also, as it needs to tie in with the PPT 
stuff, plus fan-curves. I'll keep you informed. The recent asus-armoury 
driver was a first step for it as I needed to get the PPT part in. Once 
it lands I'll submit work on the platform_profile stuff including custom 
profile.

> Bazzite. I currently carry 287 patches (~100 is XDNA+NTSYNC+platform
> profiles minus Kurt's series) for 6.13. I would also like try to
> target Thinkpads too, and maybe the Legion Go with that, but it
> depends on how much progress Derek makes on his driver by then.

Don't spread yourself too thin. I can take care of most of the (new 
feature) asus stuff (that i have hardware for testing with) leaving you 
to focus on other things if you need. Anyhow, I'll keep you in sync - 
just be aware I'll be a little slow due to work and family demands.

> As far as the Z13 is concerned, it will be this patch series + 1-3
> patches to tweak the ROG button on the side to stop acting like a wifi
> killswitch.

Ack

Cheers,
Luke.

> Antheas
> 
>> Looks like good progress so far.
>>
>> Cheers,
>> Luke.
>>
>>> Changes since V1:
>>>     - Add basic RGB support on hid-asus, (Z13/Ally) tested in KDE/Z13
>>>     - Fix ifdef else having an invalid signature (reported by kernel robot)
>>>     - Restore input arguments to init and keyboard function so they can
>>>       be re-used for RGB controls.
>>>     - Remove Z13 delay (it did not work to fix the touchpad) and replace it
>>>       with a HID_GROUP_GENERIC quirk to allow hid-multitouch to load. Squash
>>>       keyboard rename into it.
>>>     - Unregister brightness listener before removing work queue to avoid
>>>       a race condition causing corruption
>>>     - Remove spurious mutex unlock in asus_brt_event
>>>     - Place mutex lock in kbd_led_set after LED_UNREGISTERING check to avoid
>>>       relocking the mutex and causing a deadlock when unregistering leds
>>>     - Add extra check during unregistering to avoid calling unregister when
>>>       no led device is registered.
>>>     - Temporarily HID_QUIRK_INPUT_PER_APP from the ROG endpoint as it causes
>>>       the driver to create 4 RGB handlers per device. I also suspect some
>>>       extra events sneak through (KDE had the @@@@@@).
>>>
>>> Antheas Kapenekakis (11):
>>>     HID: asus: refactor init sequence per spec
>>>     HID: asus: prevent binding to all HID devices on ROG
>>>     HID: asus: add Asus Z13 2025 Fan key
>>>     HID: Asus: add Z13 folio to generic group for multitouch to work
>>>     platform/x86: asus-wmi: Add support for multiple kbd RGB handlers
>>>     HID: asus: listen to the asus-wmi brightness device instead of
>>>       creating one
>>>     platform/x86: asus-wmi: remove unused keyboard backlight quirk
>>>     platform/x86: asus-wmi: add keyboard brightness event handler
>>>     HID: asus: add support for the asus-wmi brightness handler
>>>     HID: asus: add basic RGB support
>>>     HID: asus: add RGB support to the ROG Ally units
>>>
>>>    drivers/hid/hid-asus.c                     | 342 ++++++++++++++++-----
>>>    drivers/hid/hid-ids.h                      |   2 +-
>>>    drivers/platform/x86/asus-wmi.c            | 138 ++++++++-
>>>    include/linux/platform_data/x86/asus-wmi.h |  67 ++--
>>>    4 files changed, 411 insertions(+), 138 deletions(-)
>>>
>>>
>>> base-commit: 4701f33a10702d5fc577c32434eb62adde0a1ae1
>>