[PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling

[PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling

[Antheas Kapenekakis]

This is a two part series which does the following:
  - Clean-up init sequence
  - Unify 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.

For more context, see cover letter of V1. Since V5, I removed some patches
to make this easier to merge.

---
V7: https://lore.kernel.org/all/20251018101759.4089-1-lkml@antheas.dev/
V6: https://lore.kernel.org/all/20251013201535.6737-1-lkml@antheas.dev/
V5: https://lore.kernel.org/all/20250325184601.10990-1-lkml@antheas.dev/
V4: https://lore.kernel.org/lkml/20250324210151.6042-1-lkml@antheas.dev/
V3: https://lore.kernel.org/lkml/20250322102804.418000-1-lkml@antheas.dev/
V2: https://lore.kernel.org/all/20250320220924.5023-1-lkml@antheas.dev/
V1: https://lore.kernel.org/all/20250319191320.10092-1-lkml@antheas.dev/

Changes since V7:
  - Readd legacy init quirk for Dennis
  - Remove HID_QUIRK_INPUT_PER_APP as a courtesy to asusctl
  - Fix warning due to enum_backlight receiving negative values

Changes since V6:
  - Split initialization refactor into three patches, update commit text
    to be clearer in what it does
  - Replace spinlock accesses with guard and scoped guard in all patches
  - Add missing includes mentioned by Ilpo
  - Reflow, tweak comment in prevent binding to all HID devices on ROG
  - Replace asus_ref.asus with local reference in all patches
  - Add missing kernel doc comments
  - Other minor nits from Ilpo
  - User reported warning due to scheduling work while holding a spinlock.
    Restructure patch for multiple handlers to limit when spinlock is held to
    variable access only. In parallel, setup a workqueue to handle registration
    of led device and setting brightness. This is required as registering the
    led device triggers kbd_led_get which needs to hold the spinlock to
    protect the led_wk value. The workqueue is also required for the hid
    event passthrough to avoid scheduling work while holding the spinlock.
    Apply the workqueue to wmi brightness buttons as well, as that was
    omitted before this series and WMI access was performed.
  - On "HID: asus: prevent binding to all HID devices on ROG", rename
    quirk HANDLE_GENERIC to SKIP_REPORT_FIXUP and only skip report fixup.
    This allows other quirks to apply (applies quirk that fixes keyboard
    being named as a pointer device).

Changes since V5:
  - It's been a long time
  - Remove addition of RGB as that had some comments I need to work on
  - Remove folio patch (already merged)
  - Remove legacy fix patch 11 from V4. There is a small chance that
    without this patch, some old NKEY keyboards might not respond to
    RGB commands according to Luke, but the kernel driver does not do
    RGB currently. The 0x5d init is done by Armoury crate software in
    Windows. If an issue is found, we can re-add it or just remove patches
    1/2 before merging. However, init could use the cleanup.

Changes since V4:
  - Fix KConfig (reported by kernel robot)
  - Fix Ilpo's nits, if I missed anything lmk

Changes since V3:
  - Add initializer for 0x5d for old NKEY keyboards until it is verified
    that it is not needed for their media keys to function.
  - Cover init in asus-wmi with spinlock as per Hans
  - If asus-wmi registers WMI handler with brightness, init the brightness
    in USB Asus keyboards, per Hans.
  - Change hid handler name to asus-UNIQ:rgb:peripheral to match led class
  - Fix oops when unregistering asus-wmi by moving unregister outside of
    the spin lock (but after the asus reference is set to null)

Changes since V2:
  - Check lazy init succeds in asus-wmi before setting register variable
  - make explicit check in asus_hid_register_listener for listener existing
    to avoid re-init
  - rename asus_brt to asus_hid in most places and harmonize everything
  - switch to a spinlock instead of a mutex to avoid kernel ooops
  - fixup hid device quirks to avoid multiple RGB devices while still exposing
    all input vendor devices. This includes moving rgb init to probe
    instead of the input_configured callbacks.
  - Remove fan key (during retest it appears to be 0xae that is already
    supported by hid-asus)
  - Never unregister asus::kbd_backlight while asus-wmi is active, as that
  - removes fds from userspace and breaks backlight functionality. All
  - current mainline drivers do not support backlight hotplugging, so most
    userspace software (e.g., KDE, UPower) is built with that assumption.
    For the Ally, since it disconnects its controller during sleep, this
    caused the backlight slider to not work in KDE.

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

Antheas Kapenekakis (10):
  HID: asus: simplify RGB init sequence
  HID: asus: use same report_id in response
  HID: asus: fortify keyboard handshake
  HID: asus: prevent binding to all HID devices on ROG
  HID: asus: initialize LED endpoint early for old NKEY keyboards
  platform/x86: asus-wmi: Add support for multiple kbd led 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

 drivers/hid/hid-asus.c                     | 222 +++++++++++----------
 drivers/platform/x86/asus-wmi.c            | 214 +++++++++++++++++---
 include/linux/platform_data/x86/asus-wmi.h |  70 +++----
 3 files changed, 331 insertions(+), 175 deletions(-)


base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada
-- 
2.51.2

[PATCH v8 01/10] HID: asus: simplify RGB init sequence

[Antheas Kapenekakis]

Currently, RGB initialization forks depending on whether a device is
NKEY. Then, NKEY devices are initialized using 0x5a, 0x5d, 0x5e
endpoints, and non-NKEY devices with 0x5a and then a
backlight check, which is omitted for NKEY devices.

Remove the fork, using a common initialization sequence for both,
where they are both only initialized with 0x5a, then checked for
backlight support. This patch should not affect existing functionality.

0x5d and 0x5e endpoint initializations are performed by Windows
userspace programs associated with different usages that reside under
the vendor HID. Specifically, 0x5d is used by Armoury Crate, which
controls RGB and 0x5e by an animation program for certain Asus laptops.
Neither is used currently in the driver.

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

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index a444d41e53b6..7ea1037c3979 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -638,50 +638,32 @@ 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;
-
-		if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
-			ret = asus_kbd_disable_oobe(hdev);
-			if (ret < 0)
-				return ret;
-		}
-
-		if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
-			intf = to_usb_interface(hdev->dev.parent);
-			udev = interface_to_usbdev(intf);
-			validate_mcu_fw_version(hdev,
-				le16_to_cpu(udev->descriptor.idProduct));
-		}
+	ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
+	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;
+	if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
+		intf = to_usb_interface(hdev->dev.parent);
+		udev = interface_to_usbdev(intf);
+		validate_mcu_fw_version(
+			hdev, le16_to_cpu(udev->descriptor.idProduct));
 	}
 
+	/* 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.51.2

[PATCH v8 02/10] HID: asus: use same report_id in response

[Antheas Kapenekakis]

Currently, asus_kbd_get_functions prods the device using feature
report report_id, but then is hardcoded to check the response through
FEATURE_KBD_REPORT_ID. This only works if report_id is that value
(currently true). So, use report_id in the response as well to
maintain functionality if that value changes in the future.

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

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 7ea1037c3979..4676b7f20caf 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -422,7 +422,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) {
-- 
2.51.2

[PATCH v8 03/10] HID: asus: fortify keyboard handshake

[Antheas Kapenekakis]

Handshaking with an Asus device involves sending it a feature report
with the string "ASUS Tech.Inc." and then reading it back to verify the
handshake was successful, under the feature ID the interaction will
take place.

Currently, the driver only does the first part. Add the readback to
verify the handshake was successful. As this could cause breakages,
allow the verification to fail with a dmesg error until we verify
all devices work with it (they seem to).

Since the response is more than 16 bytes, increase the buffer size
to 64 as well to avoid overflow errors.

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

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 4676b7f20caf..03f0d86936fc 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
 
@@ -393,14 +393,40 @@ 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)
 {
+	/*
+	 * The handshake is first sent as a set_report, then retrieved
+	 * from a get_report. They should be equal.
+	 */
 	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_warn(hdev, "Asus handshake returned invalid response: %*ph\n",
+			FEATURE_KBD_REPORT_SIZE, readbuf);
+		/*
+		 * Do not return error if handshake is wrong until this is
+		 * verified to work for all devices.
+		 */
+	}
 
+	kfree(readbuf);
 	return ret;
 }
 
-- 
2.51.2

[PATCH v8 04/10] HID: asus: prevent binding to all HID devices on ROG

[Antheas Kapenekakis]

Currently, when hid-asus is not loaded, NKEY keyboards load as ~6
event devices with a pretty ASUSTEK name. When it loads, it concatenates
all applications per HID endpoint, renames them, and prints errors
when some of them do not have an input device.

Therefore, change probe so that this is no longer the case. Stop
renaming the devices, omit the check for .input which causes errors
on e.g., the Z13 for some hiddev only devices, and move RGB checks
into probe.

Reviewed-by: Luke D. Jones <luke@ljones.dev>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/hid/hid-asus.c | 52 ++++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 17 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 03f0d86936fc..726f5d8e22d1 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -47,6 +47,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 #define T100CHI_MOUSE_REPORT_ID 0x06
 #define FEATURE_REPORT_ID 0x0d
 #define INPUT_REPORT_ID 0x5d
+#define HID_USAGE_PAGE_VENDOR 0xff310000
 #define FEATURE_KBD_REPORT_ID 0x5a
 #define FEATURE_KBD_REPORT_SIZE 64
 #define FEATURE_KBD_LED_REPORT_ID1 0x5d
@@ -89,6 +90,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_ROG_ALLY_XPAD		BIT(13)
+#define QUIRK_SKIP_REPORT_FIXUP		BIT(14)
 
 #define I2C_KEYBOARD_QUIRKS			(QUIRK_FIX_NOTEBOOK_REPORT | \
 						 QUIRK_NO_INIT_REPORTS | \
@@ -125,7 +127,6 @@ struct asus_drvdata {
 	struct input_dev *tp_kbd_input;
 	struct asus_kbd_leds *kbd_backlight;
 	const struct asus_touchpad_info *tp;
-	bool enable_backlight;
 	struct power_supply *battery;
 	struct power_supply_desc battery_desc;
 	int battery_capacity;
@@ -316,7 +317,7 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
 static int asus_event(struct hid_device *hdev, struct hid_field *field,
 		      struct hid_usage *usage, __s32 value)
 {
-	if ((usage->hid & HID_USAGE_PAGE) == 0xff310000 &&
+	if ((usage->hid & HID_USAGE_PAGE) == HID_USAGE_PAGE_VENDOR &&
 	    (usage->hid & HID_USAGE) != 0x00 &&
 	    (usage->hid & HID_USAGE) != 0xff && !usage->type) {
 		hid_warn(hdev, "Unmapped Asus vendor usagepage code 0x%02x\n",
@@ -931,11 +932,6 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
 
 	drvdata->input = input;
 
-	if (drvdata->enable_backlight &&
-	    !asus_kbd_wmi_led_control_present(hdev) &&
-	    asus_kbd_register_leds(hdev))
-		hid_warn(hdev, "Failed to initialize backlight.\n");
-
 	return 0;
 }
 
@@ -1008,15 +1004,6 @@ static int asus_input_mapping(struct hid_device *hdev,
 			return -1;
 		}
 
-		/*
-		 * Check and enable backlight only on devices with UsagePage ==
-		 * 0xff31 to avoid initializing the keyboard firmware multiple
-		 * times on devices with multiple HID descriptors but same
-		 * PID/VID.
-		 */
-		if (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT)
-			drvdata->enable_backlight = true;
-
 		set_bit(EV_REP, hi->input->evbit);
 		return 1;
 	}
@@ -1133,8 +1120,10 @@ static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
 
 static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
-	int ret;
+	struct hid_report_enum *rep_enum;
 	struct asus_drvdata *drvdata;
+	struct hid_report *rep;
+	int ret, is_vendor = 0;
 
 	drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
 	if (drvdata == NULL) {
@@ -1218,12 +1207,37 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
+	/* Check for vendor for RGB init and handle generic devices properly. */
+	rep_enum = &hdev->report_enum[HID_INPUT_REPORT];
+	list_for_each_entry(rep, &rep_enum->report_list, list) {
+		if ((rep->application & HID_USAGE_PAGE) == HID_USAGE_PAGE_VENDOR)
+			is_vendor = true;
+	}
+
+	/*
+	 * For ROG keyboards, disable fixups except vendor devices.
+	 */
+	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD && !is_vendor)
+		drvdata->quirks |= QUIRK_SKIP_REPORT_FIXUP;
+
 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	if (ret) {
 		hid_err(hdev, "Asus hw start failed: %d\n", ret);
 		return ret;
 	}
 
+	if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
+	    !asus_kbd_wmi_led_control_present(hdev) &&
+	    asus_kbd_register_leds(hdev))
+		hid_warn(hdev, "Failed to initialize backlight.\n");
+
+	/*
+	 * For ROG keyboards, skip rename for consistency and ->input check as
+	 * some devices do not have inputs.
+	 */
+	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD)
+		return 0;
+
 	/*
 	 * Check that input registration succeeded. Checking that
 	 * HID_CLAIMED_INPUT is set prevents a UAF when all input devices
@@ -1352,6 +1366,10 @@ static const __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 		rdesc = new_rdesc;
 	}
 
+	/* Vendor fixups should only apply to NKEY vendor devices. */
+	if (drvdata->quirks & QUIRK_SKIP_REPORT_FIXUP)
+		return rdesc;
+
 	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD &&
 			*rsize == 331 && rdesc[190] == 0x85 && rdesc[191] == 0x5a &&
 			rdesc[204] == 0x95 && rdesc[205] == 0x05) {
-- 
2.51.2

[PATCH v8 05/10] HID: asus: initialize LED endpoint early for old NKEY keyboards

[Antheas Kapenekakis]

These keyboards have always had initialization in the kernel for 0x5d.
At this point, it is hard to verify again and we risk regressions by
removing this. Therefore, initialize with 0x5d as well.

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

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 726f5d8e22d1..221c7195e885 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -91,6 +91,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
 #define QUIRK_ROG_ALLY_XPAD		BIT(13)
 #define QUIRK_SKIP_REPORT_FIXUP		BIT(14)
+#define QUIRK_ROG_NKEY_LEGACY		BIT(15)
 
 #define I2C_KEYBOARD_QUIRKS			(QUIRK_FIX_NOTEBOOK_REPORT | \
 						 QUIRK_NO_INIT_REPORTS | \
@@ -669,6 +670,16 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
 	if (ret < 0)
 		return ret;
 
+	if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
+		/*
+		 * These keyboards might need 0x5d for shortcuts to work.
+		 * As it has been more than 5 years, it is hard to verify.
+		 */
+		ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
+		if (ret < 0)
+			return ret;
+	}
+
 	/* Get keyboard functions */
 	ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
 	if (ret < 0)
@@ -1409,10 +1420,10 @@ static const struct hid_device_id asus_devices[] = {
 	  QUIRK_USE_KBD_BACKLIGHT },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
 	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
-	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
+	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
 	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
-	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
+	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
 	    USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
 	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
-- 
2.51.2

[PATCH v8 06/10] platform/x86: asus-wmi: Add support for multiple kbd led 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.

Since both hid-asus and asus-wmi can both interact with the led
objects including from an atomic context, protect the brightness
access with a spinlock and update the values from a workqueue.
Use this workqueue to process WMI keyboard events as well, so they
are processed asynchronously.

Reviewed-by: Luke D. Jones <luke@ljones.dev>
Tested-by: Luke D. Jones <luke@ljones.dev>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/platform/x86/asus-wmi.c            | 174 ++++++++++++++++++---
 include/linux/platform_data/x86/asus-wmi.h |  17 ++
 2 files changed, 167 insertions(+), 24 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index e72a2b5d158e..5f23aedbf34f 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -36,6 +36,7 @@
 #include <linux/rfkill.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 #include <linux/types.h>
 #include <linux/units.h>
 
@@ -258,6 +259,9 @@ struct asus_wmi {
 	int tpd_led_wk;
 	struct led_classdev kbd_led;
 	int kbd_led_wk;
+	bool kbd_led_notify;
+	bool kbd_led_avail;
+	bool kbd_led_registered;
 	struct led_classdev lightbar_led;
 	int lightbar_led_wk;
 	struct led_classdev micmute_led;
@@ -266,6 +270,7 @@ struct asus_wmi {
 	struct work_struct tpd_led_work;
 	struct work_struct wlan_led_work;
 	struct work_struct lightbar_led_work;
+	struct work_struct kbd_led_work;
 
 	struct asus_rfkill wlan;
 	struct asus_rfkill bluetooth;
@@ -1530,6 +1535,99 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
 
 /* LEDs ***********************************************************************/
 
+struct asus_hid_ref {
+	struct list_head listeners;
+	struct asus_wmi *asus;
+	/* Protects concurrent access from hid-asus and asus-wmi to leds */
+	spinlock_t lock;
+};
+
+static struct asus_hid_ref asus_ref = {
+	.listeners = LIST_HEAD_INIT(asus_ref.listeners),
+	.asus = NULL,
+	/*
+	 * Protects .asus, .asus.kbd_led_{wk,notify}, and .listener refs. Other
+	 * asus variables are read-only after .asus is set. Except the led cdev
+	 * device if not kbd_led_avail. That becomes read-only after the
+	 * first hid-asus listener registers and triggers the work queue. It is
+	 * then not referenced again until unregistering, which happens after
+	 * .asus ref is dropped. Since .asus needs to be accessed by hid-asus
+	 * IRQs to check if forwarding events is possible, a spinlock is used.
+	 */
+	.lock = __SPIN_LOCK_UNLOCKED(asus_ref.lock),
+};
+
+/*
+ * Allows registering hid-asus listeners that want to be notified of
+ * keyboard backlight changes.
+ */
+int asus_hid_register_listener(struct asus_hid_listener *bdev)
+{
+	struct asus_wmi *asus;
+
+	guard(spinlock_irqsave)(&asus_ref.lock);
+	list_add_tail(&bdev->list, &asus_ref.listeners);
+	asus = asus_ref.asus;
+	if (asus)
+		queue_work(asus->led_workqueue, &asus->kbd_led_work);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(asus_hid_register_listener);
+
+/*
+ * Allows unregistering hid-asus listeners that were added with
+ * asus_hid_register_listener().
+ */
+void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
+{
+	guard(spinlock_irqsave)(&asus_ref.lock);
+	list_del(&bdev->list);
+}
+EXPORT_SYMBOL_GPL(asus_hid_unregister_listener);
+
+static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
+
+static void kbd_led_update_all(struct work_struct *work)
+{
+	struct asus_wmi *asus;
+	bool registered, notify;
+	int ret, value;
+
+	asus = container_of(work, struct asus_wmi, kbd_led_work);
+
+	scoped_guard(spinlock_irqsave, &asus_ref.lock) {
+		registered = asus->kbd_led_registered;
+		value = asus->kbd_led_wk;
+		notify = asus->kbd_led_notify;
+	}
+
+	if (!registered) {
+		/*
+		 * This workqueue runs under asus-wmi, which means probe has
+		 * completed and asus-wmi will keep running until it finishes.
+		 * Therefore, we can safely register the LED without holding
+		 * a spinlock.
+		 */
+		ret = devm_led_classdev_register(&asus->platform_device->dev,
+					    &asus->kbd_led);
+		if (!ret) {
+			scoped_guard(spinlock_irqsave, &asus_ref.lock)
+				asus->kbd_led_registered = true;
+		} else {
+			pr_warn("Failed to register keyboard backlight LED: %d\n", ret);
+			return;
+		}
+	}
+
+	if (value >= 0)
+		do_kbd_led_set(&asus->kbd_led, value);
+	if (notify) {
+		scoped_guard(spinlock_irqsave, &asus_ref.lock)
+			asus->kbd_led_notify = false;
+		led_classdev_notify_brightness_hw_changed(&asus->kbd_led, value);
+	}
+}
+
 /*
  * These functions actually update the LED's, and are called from a
  * workqueue. By doing this as separate work rather than when the LED
@@ -1576,7 +1674,8 @@ static void kbd_led_update(struct asus_wmi *asus)
 {
 	int ctrl_param = 0;
 
-	ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
+	scoped_guard(spinlock_irqsave, &asus_ref.lock)
+		ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
 	asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
 }
 
@@ -1609,14 +1708,23 @@ static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
 
 static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
 {
+	struct asus_hid_listener *listener;
 	struct asus_wmi *asus;
 	int max_level;
 
 	asus = container_of(led_cdev, struct asus_wmi, kbd_led);
 	max_level = asus->kbd_led.max_brightness;
 
-	asus->kbd_led_wk = clamp_val(value, 0, max_level);
-	kbd_led_update(asus);
+	scoped_guard(spinlock_irqsave, &asus_ref.lock)
+		asus->kbd_led_wk = clamp_val(value, 0, max_level);
+
+	if (asus->kbd_led_avail)
+		kbd_led_update(asus);
+
+	scoped_guard(spinlock_irqsave, &asus_ref.lock) {
+		list_for_each_entry(listener, &asus_ref.listeners, list)
+			listener->brightness_set(listener, asus->kbd_led_wk);
+	}
 }
 
 static void kbd_led_set(struct led_classdev *led_cdev,
@@ -1631,10 +1739,11 @@ static void kbd_led_set(struct led_classdev *led_cdev,
 
 static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
 {
-	struct led_classdev *led_cdev = &asus->kbd_led;
-
-	do_kbd_led_set(led_cdev, value);
-	led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
+	scoped_guard(spinlock_irqsave, &asus_ref.lock) {
+		asus->kbd_led_wk = value;
+		asus->kbd_led_notify = true;
+	}
+	queue_work(asus->led_workqueue, &asus->kbd_led_work);
 }
 
 static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
@@ -1644,10 +1753,18 @@ static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
 
 	asus = container_of(led_cdev, struct asus_wmi, kbd_led);
 
+	scoped_guard(spinlock_irqsave, &asus_ref.lock) {
+		if (!asus->kbd_led_avail)
+			return asus->kbd_led_wk;
+	}
+
 	retval = kbd_led_read(asus, &value, NULL);
 	if (retval < 0)
 		return retval;
 
+	scoped_guard(spinlock_irqsave, &asus_ref.lock)
+		asus->kbd_led_wk = value;
+
 	return value;
 }
 
@@ -1759,7 +1876,9 @@ 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);
+	scoped_guard(spinlock_irqsave, &asus_ref.lock)
+		asus_ref.asus = NULL;
+
 	led_classdev_unregister(&asus->tpd_led);
 	led_classdev_unregister(&asus->wlan_led);
 	led_classdev_unregister(&asus->lightbar_led);
@@ -1797,22 +1916,25 @@ 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_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;
+	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);
+	INIT_WORK(&asus->kbd_led_work, kbd_led_update_all);
 
+	if (asus->kbd_led_avail) {
+		asus->kbd_led_wk = led_val;
 		if (num_rgb_groups != 0)
 			asus->kbd_led.groups = kbd_rgb_mode_groups;
+	} else
+		asus->kbd_led_wk = -1;
 
-		rv = led_classdev_register(&asus->platform_device->dev,
-					   &asus->kbd_led);
-		if (rv)
-			goto error;
+	scoped_guard(spinlock_irqsave, &asus_ref.lock) {
+		asus_ref.asus = asus;
+		if (asus->kbd_led_avail || !list_empty(&asus_ref.listeners))
+			queue_work(asus->led_workqueue, &asus->kbd_led_work);
 	}
 
 	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_WIRELESS_LED)
@@ -4272,6 +4394,7 @@ static int asus_wmi_get_event_code(union acpi_object *obj)
 
 static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
 {
+	enum led_brightness led_value;
 	unsigned int key_value = 1;
 	bool autorelease = 1;
 
@@ -4288,19 +4411,22 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
 		return;
 	}
 
+	scoped_guard(spinlock_irqsave, &asus_ref.lock)
+		led_value = asus->kbd_led_wk;
+
 	if (code == NOTIFY_KBD_BRTUP) {
-		kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
+		kbd_led_set_by_kbd(asus, led_value + 1);
 		return;
 	}
 	if (code == NOTIFY_KBD_BRTDWN) {
-		kbd_led_set_by_kbd(asus, asus->kbd_led_wk - 1);
+		kbd_led_set_by_kbd(asus, led_value - 1);
 		return;
 	}
 	if (code == NOTIFY_KBD_BRTTOGGLE) {
-		if (asus->kbd_led_wk == asus->kbd_led.max_brightness)
+		if (led_value == asus->kbd_led.max_brightness)
 			kbd_led_set_by_kbd(asus, 0);
 		else
-			kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
+			kbd_led_set_by_kbd(asus, led_value + 1);
 		return;
 	}
 
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 8a515179113d..1165039013b1 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -163,11 +163,20 @@ enum asus_ally_mcu_hack {
 	ASUS_WMI_ALLY_MCU_HACK_DISABLED,
 };
 
+/* Used to notify hid-asus when asus-wmi changes keyboard backlight */
+struct asus_hid_listener {
+	struct list_head list;
+	void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
+};
+
 #if IS_REACHABLE(CONFIG_ASUS_WMI)
 void set_ally_mcu_hack(enum asus_ally_mcu_hack status);
 void set_ally_mcu_powersave(bool enabled);
 int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval);
 int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
+
+int asus_hid_register_listener(struct asus_hid_listener *cdev);
+void asus_hid_unregister_listener(struct asus_hid_listener *cdev);
 #else
 static inline void set_ally_mcu_hack(enum asus_ally_mcu_hack status)
 {
@@ -184,6 +193,14 @@ static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
 {
 	return -ENODEV;
 }
+
+static inline int asus_hid_register_listener(struct asus_hid_listener *bdev)
+{
+	return -ENODEV;
+}
+static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
+{
+}
 #endif
 
 /* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
-- 
2.51.2

[PATCH v8 07/10] 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.

Reviewed-by: Luke D. Jones <luke@ljones.dev>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/hid/hid-asus.c | 64 +++++++-----------------------------------
 1 file changed, 10 insertions(+), 54 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 221c7195e885..e5d3f28c1fad 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -103,7 +103,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 #define TRKID_SGN       ((TRKID_MAX + 1) >> 1)
 
 struct asus_kbd_leds {
-	struct led_classdev cdev;
+	struct asus_hid_listener listener;
 	struct hid_device *hdev;
 	struct work_struct work;
 	unsigned int brightness;
@@ -495,11 +495,11 @@ static void asus_schedule_work(struct asus_kbd_leds *led)
 	spin_unlock_irqrestore(&led->lock, flags);
 }
 
-static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
-				   enum led_brightness brightness)
+static void asus_kbd_backlight_set(struct asus_hid_listener *listener,
+				   int brightness)
 {
-	struct asus_kbd_leds *led = container_of(led_cdev, struct asus_kbd_leds,
-						 cdev);
+	struct asus_kbd_leds *led = container_of(listener, struct asus_kbd_leds,
+						 listener);
 	unsigned long flags;
 
 	spin_lock_irqsave(&led->lock, flags);
@@ -509,20 +509,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);
@@ -539,34 +525,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);
-}
-
 /*
  * We don't care about any other part of the string except the version section.
  * Example strings: FGA80100.RC72LA.312_T01, FGA80100.RC71LS.318_T01
@@ -711,14 +669,11 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
 	drvdata->kbd_backlight->removed = false;
 	drvdata->kbd_backlight->brightness = 0;
 	drvdata->kbd_backlight->hdev = hdev;
-	drvdata->kbd_backlight->cdev.name = "asus::kbd_backlight";
-	drvdata->kbd_backlight->cdev.max_brightness = 3;
-	drvdata->kbd_backlight->cdev.brightness_set = asus_kbd_backlight_set;
-	drvdata->kbd_backlight->cdev.brightness_get = asus_kbd_backlight_get;
+	drvdata->kbd_backlight->listener.brightness_set = asus_kbd_backlight_set;
 	INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work);
 	spin_lock_init(&drvdata->kbd_backlight->lock);
 
-	ret = devm_led_classdev_register(&hdev->dev, &drvdata->kbd_backlight->cdev);
+	ret = asus_hid_register_listener(&drvdata->kbd_backlight->listener);
 	if (ret < 0) {
 		/* No need to have this still around */
 		devm_kfree(&hdev->dev, drvdata->kbd_backlight);
@@ -1107,7 +1062,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);
@@ -1238,7 +1193,6 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	}
 
 	if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
-	    !asus_kbd_wmi_led_control_present(hdev) &&
 	    asus_kbd_register_leds(hdev))
 		hid_warn(hdev, "Failed to initialize backlight.\n");
 
@@ -1285,6 +1239,8 @@ static void asus_remove(struct hid_device *hdev)
 	unsigned long flags;
 
 	if (drvdata->kbd_backlight) {
+		asus_hid_unregister_listener(&drvdata->kbd_backlight->listener);
+
 		spin_lock_irqsave(&drvdata->kbd_backlight->lock, flags);
 		drvdata->kbd_backlight->removed = true;
 		spin_unlock_irqrestore(&drvdata->kbd_backlight->lock, flags);
-- 
2.51.2

[PATCH v8 08/10] 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.

Reviewed-by: Luke D. Jones <luke@ljones.dev>
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 1165039013b1..d8c5269854b0 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -203,44 +203,4 @@ static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
 }
 #endif
 
-/* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
-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.51.2

[PATCH v8 09/10] platform/x86: asus-wmi: add keyboard brightness event handler

[Antheas Kapenekakis]

The keyboard brightness control of Asus WMI keyboards is handled in
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. Since this handler is called from
an interrupt context, defer the actual work to a workqueue.

In the process, introduce ASUS_EV_MAX_BRIGHTNESS to hold the constant
for maximum brightness since it is shared between hid-asus/asus-wmi.

Reviewed-by: Luke D. Jones <luke@ljones.dev>
Tested-by: Luke D. Jones <luke@ljones.dev>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/platform/x86/asus-wmi.c            | 46 +++++++++++++++++++---
 include/linux/platform_data/x86/asus-wmi.h | 13 ++++++
 2 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 5f23aedbf34f..a7482c97cc5b 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -1628,6 +1628,44 @@ static void kbd_led_update_all(struct work_struct *work)
 	}
 }
 
+/*
+ * This function is called from hid-asus to inform asus-wmi of brightness
+ * changes initiated by the keyboard backlight keys.
+ */
+int asus_hid_event(enum asus_hid_event event)
+{
+	struct asus_wmi *asus;
+	int brightness;
+
+	guard(spinlock_irqsave)(&asus_ref.lock);
+	asus = asus_ref.asus;
+	if (!asus || !asus->kbd_led_registered)
+		return -EBUSY;
+
+	brightness = asus->kbd_led_wk;
+
+	switch (event) {
+	case ASUS_EV_BRTUP:
+		brightness += 1;
+		break;
+	case ASUS_EV_BRTDOWN:
+		brightness -= 1;
+		break;
+	case ASUS_EV_BRTTOGGLE:
+		if (brightness >= ASUS_EV_MAX_BRIGHTNESS)
+			brightness = 0;
+		else
+			brightness += 1;
+		break;
+	}
+
+	asus->kbd_led_wk = clamp_val(brightness, 0, ASUS_EV_MAX_BRIGHTNESS);
+	asus->kbd_led_notify = true;
+	queue_work(asus->led_workqueue, &asus->kbd_led_work);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(asus_hid_event);
+
 /*
  * These functions actually update the LED's, and are called from a
  * workqueue. By doing this as separate work rather than when the LED
@@ -1710,13 +1748,11 @@ static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
 {
 	struct asus_hid_listener *listener;
 	struct asus_wmi *asus;
-	int max_level;
 
 	asus = container_of(led_cdev, struct asus_wmi, kbd_led);
-	max_level = asus->kbd_led.max_brightness;
 
 	scoped_guard(spinlock_irqsave, &asus_ref.lock)
-		asus->kbd_led_wk = clamp_val(value, 0, max_level);
+		asus->kbd_led_wk = clamp_val(value, 0, ASUS_EV_MAX_BRIGHTNESS);
 
 	if (asus->kbd_led_avail)
 		kbd_led_update(asus);
@@ -1920,7 +1956,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
 	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.max_brightness = ASUS_EV_MAX_BRIGHTNESS;
 	asus->kbd_led_avail = !kbd_led_read(asus, &led_val, NULL);
 	INIT_WORK(&asus->kbd_led_work, kbd_led_update_all);
 
@@ -4423,7 +4459,7 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
 		return;
 	}
 	if (code == NOTIFY_KBD_BRTTOGGLE) {
-		if (led_value == asus->kbd_led.max_brightness)
+		if (led_value == ASUS_EV_MAX_BRIGHTNESS)
 			kbd_led_set_by_kbd(asus, 0);
 		else
 			kbd_led_set_by_kbd(asus, led_value + 1);
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index d8c5269854b0..3f679598b629 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -169,6 +169,14 @@ struct asus_hid_listener {
 	void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
 };
 
+enum asus_hid_event {
+	ASUS_EV_BRTUP,
+	ASUS_EV_BRTDOWN,
+	ASUS_EV_BRTTOGGLE,
+};
+
+#define ASUS_EV_MAX_BRIGHTNESS 3
+
 #if IS_REACHABLE(CONFIG_ASUS_WMI)
 void set_ally_mcu_hack(enum asus_ally_mcu_hack status);
 void set_ally_mcu_powersave(bool enabled);
@@ -177,6 +185,7 @@ int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
 
 int asus_hid_register_listener(struct asus_hid_listener *cdev);
 void asus_hid_unregister_listener(struct asus_hid_listener *cdev);
+int asus_hid_event(enum asus_hid_event event);
 #else
 static inline void set_ally_mcu_hack(enum asus_ally_mcu_hack status)
 {
@@ -201,6 +210,10 @@ static inline int asus_hid_register_listener(struct asus_hid_listener *bdev)
 static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
 {
 }
+static inline int asus_hid_event(enum asus_hid_event event)
+{
+	return -ENODEV;
+}
 #endif
 
 #endif	/* __PLATFORM_DATA_X86_ASUS_WMI_H */
-- 
2.51.2

[PATCH v8 10/10] 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.

Reviewed-by: Luke D. Jones <luke@ljones.dev>
Tested-by: Luke D. Jones <luke@ljones.dev>
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 e5d3f28c1fad..de64451e315d 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -325,6 +325,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_hid_event(ASUS_EV_BRTUP);
+		case KEY_KBDILLUMDOWN:
+			return !asus_hid_event(ASUS_EV_BRTDOWN);
+		case KEY_KBDILLUMTOGGLE:
+			return !asus_hid_event(ASUS_EV_BRTTOGGLE);
+		}
+	}
+
 	return 0;
 }
 
-- 
2.51.2

Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling

[Antheas Kapenekakis]

On Sat, 1 Nov 2025 at 11:47, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>
> This is a two part series which does the following:
>   - Clean-up init sequence
>   - Unify 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.
>
> For more context, see cover letter of V1. Since V5, I removed some patches
> to make this easier to merge.

Small bump for this.

Unrelated but I was b4ing this series on Ubuntu 24 and got BADSIG:
DKIM/antheas.dev. Is there a reference for fixing this on my host?
Perhaps it would help with spam

Antheas

> ---
> V7: https://lore.kernel.org/all/20251018101759.4089-1-lkml@antheas.dev/
> V6: https://lore.kernel.org/all/20251013201535.6737-1-lkml@antheas.dev/
> V5: https://lore.kernel.org/all/20250325184601.10990-1-lkml@antheas.dev/
> V4: https://lore.kernel.org/lkml/20250324210151.6042-1-lkml@antheas.dev/
> V3: https://lore.kernel.org/lkml/20250322102804.418000-1-lkml@antheas.dev/
> V2: https://lore.kernel.org/all/20250320220924.5023-1-lkml@antheas.dev/
> V1: https://lore.kernel.org/all/20250319191320.10092-1-lkml@antheas.dev/
>
> Changes since V7:
>   - Readd legacy init quirk for Dennis
>   - Remove HID_QUIRK_INPUT_PER_APP as a courtesy to asusctl
>   - Fix warning due to enum_backlight receiving negative values
>
> Changes since V6:
>   - Split initialization refactor into three patches, update commit text
>     to be clearer in what it does
>   - Replace spinlock accesses with guard and scoped guard in all patches
>   - Add missing includes mentioned by Ilpo
>   - Reflow, tweak comment in prevent binding to all HID devices on ROG
>   - Replace asus_ref.asus with local reference in all patches
>   - Add missing kernel doc comments
>   - Other minor nits from Ilpo
>   - User reported warning due to scheduling work while holding a spinlock.
>     Restructure patch for multiple handlers to limit when spinlock is held to
>     variable access only. In parallel, setup a workqueue to handle registration
>     of led device and setting brightness. This is required as registering the
>     led device triggers kbd_led_get which needs to hold the spinlock to
>     protect the led_wk value. The workqueue is also required for the hid
>     event passthrough to avoid scheduling work while holding the spinlock.
>     Apply the workqueue to wmi brightness buttons as well, as that was
>     omitted before this series and WMI access was performed.
>   - On "HID: asus: prevent binding to all HID devices on ROG", rename
>     quirk HANDLE_GENERIC to SKIP_REPORT_FIXUP and only skip report fixup.
>     This allows other quirks to apply (applies quirk that fixes keyboard
>     being named as a pointer device).
>
> Changes since V5:
>   - It's been a long time
>   - Remove addition of RGB as that had some comments I need to work on
>   - Remove folio patch (already merged)
>   - Remove legacy fix patch 11 from V4. There is a small chance that
>     without this patch, some old NKEY keyboards might not respond to
>     RGB commands according to Luke, but the kernel driver does not do
>     RGB currently. The 0x5d init is done by Armoury crate software in
>     Windows. If an issue is found, we can re-add it or just remove patches
>     1/2 before merging. However, init could use the cleanup.
>
> Changes since V4:
>   - Fix KConfig (reported by kernel robot)
>   - Fix Ilpo's nits, if I missed anything lmk
>
> Changes since V3:
>   - Add initializer for 0x5d for old NKEY keyboards until it is verified
>     that it is not needed for their media keys to function.
>   - Cover init in asus-wmi with spinlock as per Hans
>   - If asus-wmi registers WMI handler with brightness, init the brightness
>     in USB Asus keyboards, per Hans.
>   - Change hid handler name to asus-UNIQ:rgb:peripheral to match led class
>   - Fix oops when unregistering asus-wmi by moving unregister outside of
>     the spin lock (but after the asus reference is set to null)
>
> Changes since V2:
>   - Check lazy init succeds in asus-wmi before setting register variable
>   - make explicit check in asus_hid_register_listener for listener existing
>     to avoid re-init
>   - rename asus_brt to asus_hid in most places and harmonize everything
>   - switch to a spinlock instead of a mutex to avoid kernel ooops
>   - fixup hid device quirks to avoid multiple RGB devices while still exposing
>     all input vendor devices. This includes moving rgb init to probe
>     instead of the input_configured callbacks.
>   - Remove fan key (during retest it appears to be 0xae that is already
>     supported by hid-asus)
>   - Never unregister asus::kbd_backlight while asus-wmi is active, as that
>   - removes fds from userspace and breaks backlight functionality. All
>   - current mainline drivers do not support backlight hotplugging, so most
>     userspace software (e.g., KDE, UPower) is built with that assumption.
>     For the Ally, since it disconnects its controller during sleep, this
>     caused the backlight slider to not work in KDE.
>
> Changes since V1:
>   - Add basic RGB support on hid-asus, (Z13/Ally) tested in KDE/Z13
>   - Fix ifdef else having an invalid signature (reported by kernel robot)
>   - Restore input arguments to init and keyboard function so they can
>     be re-used for RGB controls.
>   - Remove Z13 delay (it did not work to fix the touchpad) and replace it
>     with a HID_GROUP_GENERIC quirk to allow hid-multitouch to load. Squash
>     keyboard rename into it.
>   - Unregister brightness listener before removing work queue to avoid
>     a race condition causing corruption
>   - Remove spurious mutex unlock in asus_brt_event
>   - Place mutex lock in kbd_led_set after LED_UNREGISTERING check to avoid
>     relocking the mutex and causing a deadlock when unregistering leds
>   - Add extra check during unregistering to avoid calling unregister when
>     no led device is registered.
>   - Temporarily HID_QUIRK_INPUT_PER_APP from the ROG endpoint as it causes
>     the driver to create 4 RGB handlers per device. I also suspect some
>     extra events sneak through (KDE had the @@@@@@).
>
> Antheas Kapenekakis (10):
>   HID: asus: simplify RGB init sequence
>   HID: asus: use same report_id in response
>   HID: asus: fortify keyboard handshake
>   HID: asus: prevent binding to all HID devices on ROG
>   HID: asus: initialize LED endpoint early for old NKEY keyboards
>   platform/x86: asus-wmi: Add support for multiple kbd led 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
>
>  drivers/hid/hid-asus.c                     | 222 +++++++++++----------
>  drivers/platform/x86/asus-wmi.c            | 214 +++++++++++++++++---
>  include/linux/platform_data/x86/asus-wmi.h |  70 +++----
>  3 files changed, 331 insertions(+), 175 deletions(-)
>
>
> base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada
> --
> 2.51.2
>
>

Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling

[Ilpo Järvinen]

On Wed, 12 Nov 2025, Antheas Kapenekakis wrote:

> On Sat, 1 Nov 2025 at 11:47, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >
> > This is a two part series which does the following:
> >   - Clean-up init sequence
> >   - Unify 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.
> >
> > For more context, see cover letter of V1. Since V5, I removed some patches
> > to make this easier to merge.
> 
> Small bump for this.

I looked at v8 earlier but then got an impression some of Denis' comments 
against v7 were not taken into account in v8, which implies there will be 
delay until I've time to delve into the details (I need to understand 
things pretty deeply in such a case, which does take lots of time).

Alternatively, if Denis says v8 is acceptable, then I don't need to spend 
so much time on it, but somehow I've a feeling he isn't happy with v8 
but just hasn't voiced it again...

Please do realize that ignoring reviewer feedback without a very very good 
reason always risks adding delay or friction into getting things 
upstreamed. Especially, when the review comes from a person who has been 
around for me to learn to trust their reviews or from a maintainer of the 
code in question.

> Unrelated but I was b4ing this series on Ubuntu 24 and got BADSIG:
> DKIM/antheas.dev. Is there a reference for fixing this on my host?
> Perhaps it would help with spam

I see BADSIG very often these days from b4 (thanks to gmail expiring 
things after 7 days or so, I recall hearing somewhere), I just ignore them 
entirely.

AFAIK, that has never caused any delay to any patch in pdx86 domain so if 
that is what you thought is happening here, it's not the case.
If your patch does appear in the pdx86 patchwork, there's even less reason 
to worry as I mostly pick patches to process using patchwork's list.

-- 
 i.

> 
> Antheas
> 
> > ---
> > V7: https://lore.kernel.org/all/20251018101759.4089-1-lkml@antheas.dev/
> > V6: https://lore.kernel.org/all/20251013201535.6737-1-lkml@antheas.dev/
> > V5: https://lore.kernel.org/all/20250325184601.10990-1-lkml@antheas.dev/
> > V4: https://lore.kernel.org/lkml/20250324210151.6042-1-lkml@antheas.dev/
> > V3: https://lore.kernel.org/lkml/20250322102804.418000-1-lkml@antheas.dev/
> > V2: https://lore.kernel.org/all/20250320220924.5023-1-lkml@antheas.dev/
> > V1: https://lore.kernel.org/all/20250319191320.10092-1-lkml@antheas.dev/
> >
> > Changes since V7:
> >   - Readd legacy init quirk for Dennis
> >   - Remove HID_QUIRK_INPUT_PER_APP as a courtesy to asusctl
> >   - Fix warning due to enum_backlight receiving negative values
> >
> > Changes since V6:
> >   - Split initialization refactor into three patches, update commit text
> >     to be clearer in what it does
> >   - Replace spinlock accesses with guard and scoped guard in all patches
> >   - Add missing includes mentioned by Ilpo
> >   - Reflow, tweak comment in prevent binding to all HID devices on ROG
> >   - Replace asus_ref.asus with local reference in all patches
> >   - Add missing kernel doc comments
> >   - Other minor nits from Ilpo
> >   - User reported warning due to scheduling work while holding a spinlock.
> >     Restructure patch for multiple handlers to limit when spinlock is held to
> >     variable access only. In parallel, setup a workqueue to handle registration
> >     of led device and setting brightness. This is required as registering the
> >     led device triggers kbd_led_get which needs to hold the spinlock to
> >     protect the led_wk value. The workqueue is also required for the hid
> >     event passthrough to avoid scheduling work while holding the spinlock.
> >     Apply the workqueue to wmi brightness buttons as well, as that was
> >     omitted before this series and WMI access was performed.
> >   - On "HID: asus: prevent binding to all HID devices on ROG", rename
> >     quirk HANDLE_GENERIC to SKIP_REPORT_FIXUP and only skip report fixup.
> >     This allows other quirks to apply (applies quirk that fixes keyboard
> >     being named as a pointer device).
> >
> > Changes since V5:
> >   - It's been a long time
> >   - Remove addition of RGB as that had some comments I need to work on
> >   - Remove folio patch (already merged)
> >   - Remove legacy fix patch 11 from V4. There is a small chance that
> >     without this patch, some old NKEY keyboards might not respond to
> >     RGB commands according to Luke, but the kernel driver does not do
> >     RGB currently. The 0x5d init is done by Armoury crate software in
> >     Windows. If an issue is found, we can re-add it or just remove patches
> >     1/2 before merging. However, init could use the cleanup.
> >
> > Changes since V4:
> >   - Fix KConfig (reported by kernel robot)
> >   - Fix Ilpo's nits, if I missed anything lmk
> >
> > Changes since V3:
> >   - Add initializer for 0x5d for old NKEY keyboards until it is verified
> >     that it is not needed for their media keys to function.
> >   - Cover init in asus-wmi with spinlock as per Hans
> >   - If asus-wmi registers WMI handler with brightness, init the brightness
> >     in USB Asus keyboards, per Hans.
> >   - Change hid handler name to asus-UNIQ:rgb:peripheral to match led class
> >   - Fix oops when unregistering asus-wmi by moving unregister outside of
> >     the spin lock (but after the asus reference is set to null)
> >
> > Changes since V2:
> >   - Check lazy init succeds in asus-wmi before setting register variable
> >   - make explicit check in asus_hid_register_listener for listener existing
> >     to avoid re-init
> >   - rename asus_brt to asus_hid in most places and harmonize everything
> >   - switch to a spinlock instead of a mutex to avoid kernel ooops
> >   - fixup hid device quirks to avoid multiple RGB devices while still exposing
> >     all input vendor devices. This includes moving rgb init to probe
> >     instead of the input_configured callbacks.
> >   - Remove fan key (during retest it appears to be 0xae that is already
> >     supported by hid-asus)
> >   - Never unregister asus::kbd_backlight while asus-wmi is active, as that
> >   - removes fds from userspace and breaks backlight functionality. All
> >   - current mainline drivers do not support backlight hotplugging, so most
> >     userspace software (e.g., KDE, UPower) is built with that assumption.
> >     For the Ally, since it disconnects its controller during sleep, this
> >     caused the backlight slider to not work in KDE.
> >
> > Changes since V1:
> >   - Add basic RGB support on hid-asus, (Z13/Ally) tested in KDE/Z13
> >   - Fix ifdef else having an invalid signature (reported by kernel robot)
> >   - Restore input arguments to init and keyboard function so they can
> >     be re-used for RGB controls.
> >   - Remove Z13 delay (it did not work to fix the touchpad) and replace it
> >     with a HID_GROUP_GENERIC quirk to allow hid-multitouch to load. Squash
> >     keyboard rename into it.
> >   - Unregister brightness listener before removing work queue to avoid
> >     a race condition causing corruption
> >   - Remove spurious mutex unlock in asus_brt_event
> >   - Place mutex lock in kbd_led_set after LED_UNREGISTERING check to avoid
> >     relocking the mutex and causing a deadlock when unregistering leds
> >   - Add extra check during unregistering to avoid calling unregister when
> >     no led device is registered.
> >   - Temporarily HID_QUIRK_INPUT_PER_APP from the ROG endpoint as it causes
> >     the driver to create 4 RGB handlers per device. I also suspect some
> >     extra events sneak through (KDE had the @@@@@@).
> >
> > Antheas Kapenekakis (10):
> >   HID: asus: simplify RGB init sequence
> >   HID: asus: use same report_id in response
> >   HID: asus: fortify keyboard handshake
> >   HID: asus: prevent binding to all HID devices on ROG
> >   HID: asus: initialize LED endpoint early for old NKEY keyboards
> >   platform/x86: asus-wmi: Add support for multiple kbd led 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
> >
> >  drivers/hid/hid-asus.c                     | 222 +++++++++++----------
> >  drivers/platform/x86/asus-wmi.c            | 214 +++++++++++++++++---
> >  include/linux/platform_data/x86/asus-wmi.h |  70 +++----
> >  3 files changed, 331 insertions(+), 175 deletions(-)
> >
> >
> > base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada
> > --
> > 2.51.2
> >
> >
> 

Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling

[Antheas Kapenekakis]

On Wed, 12 Nov 2025 at 14:22, Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Wed, 12 Nov 2025, Antheas Kapenekakis wrote:
>
> > On Sat, 1 Nov 2025 at 11:47, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> > >
> > > This is a two part series which does the following:
> > >   - Clean-up init sequence
> > >   - Unify 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.
> > >
> > > For more context, see cover letter of V1. Since V5, I removed some patches
> > > to make this easier to merge.
> >
> > Small bump for this.
>
> I looked at v8 earlier but then got an impression some of Denis' comments
> against v7 were not taken into account in v8, which implies there will be
> delay until I've time to delve into the details (I need to understand
> things pretty deeply in such a case, which does take lots of time).
>
> Alternatively, if Denis says v8 is acceptable, then I don't need to spend
> so much time on it, but somehow I've a feeling he isn't happy with v8
> but just hasn't voiced it again...
>
> Please do realize that ignoring reviewer feedback without a very very good
> reason always risks adding delay or friction into getting things
> upstreamed. Especially, when the review comes from a person who has been
> around for me to learn to trust their reviews or from a maintainer of the
> code in question.

Sure, sorry if it came out this way. Dennis had two comments on the V7
version of the series.

The first one was that asusctl has a hang bug, which he hasn't had
time to look into yet. This should have been fixed by dropping the
HID_QUIRK_INPUT_PER_APP. I retested the series and that QUIRK was a
bit of a NOOP that does not need to be added in the future.

The second is he is concerned with dropping the 0x5d/0x5e inits. Luke
said (back in March) that it is fine to drop 0x5e because it is only
used for ANIME displays. However, for 0x5d, it is hard to verify some
of the older laptops because they have only been tested with 0x5d and
we do not have the hardware in question to test.

For this series, I re-added "initialize LED endpoint early for old
NKEY keyboards" that re-adds 0x5d for the keyboards that cannot be
tested again so this comment should be resolved too. With that in
mind, we do end up with an additional quirk/command that may be
unneeded and will remain there forever, but since it was a point of
contention, it is not worth arguing over.

So both comments should be resolved

@Denis: can give an ack if this is the case?

As for Derek's comment, he has a PR for his project where he removes
the name match for Ally devices with ample time for it to be merged
until kernel 6.19 is released. In addition, that specific software for
full functionality relies on OOT drivers on the distros it ships with,
so it is minimally affected in either case.

Moreover, that specific commit is needed for Xbox Ally devices anyway,
as the kernel kicks one of the RGB endpoints because it does not
register an input device (the check skipped by early return) so
userspace becomes unable to control RGB on a stock kernel
(hidraw/hiddev nodes are gone). For more context here, this specific
endpoint implements the RGB Lamparray protocol for Windows dynamic
lighting, and I think in an attempt to make it work properly in
Windows, Asus made it so Windows has to first disable dynamic lighting
for armoury crate RGB commands to work (the 0x5a ones over the 0xff31
hid page).

Hopefully this clears things up

Antheas

> > Unrelated but I was b4ing this series on Ubuntu 24 and got BADSIG:
> > DKIM/antheas.dev. Is there a reference for fixing this on my host?
> > Perhaps it would help with spam
>
> I see BADSIG very often these days from b4 (thanks to gmail expiring
> things after 7 days or so, I recall hearing somewhere), I just ignore them
> entirely.
>
> AFAIK, that has never caused any delay to any patch in pdx86 domain so if
> that is what you thought is happening here, it's not the case.
> If your patch does appear in the pdx86 patchwork, there's even less reason
> to worry as I mostly pick patches to process using patchwork's list.

Turns out I had to update my DNS records. It should be good now.

> --
>  i.
>
> >
> > Antheas
> >
> > > ---
> > > V7: https://lore.kernel.org/all/20251018101759.4089-1-lkml@antheas.dev/
> > > V6: https://lore.kernel.org/all/20251013201535.6737-1-lkml@antheas.dev/
> > > V5: https://lore.kernel.org/all/20250325184601.10990-1-lkml@antheas.dev/
> > > V4: https://lore.kernel.org/lkml/20250324210151.6042-1-lkml@antheas.dev/
> > > V3: https://lore.kernel.org/lkml/20250322102804.418000-1-lkml@antheas.dev/
> > > V2: https://lore.kernel.org/all/20250320220924.5023-1-lkml@antheas.dev/
> > > V1: https://lore.kernel.org/all/20250319191320.10092-1-lkml@antheas.dev/
> > >
> > > Changes since V7:
> > >   - Readd legacy init quirk for Dennis
> > >   - Remove HID_QUIRK_INPUT_PER_APP as a courtesy to asusctl
> > >   - Fix warning due to enum_backlight receiving negative values
> > >
> > > Changes since V6:
> > >   - Split initialization refactor into three patches, update commit text
> > >     to be clearer in what it does
> > >   - Replace spinlock accesses with guard and scoped guard in all patches
> > >   - Add missing includes mentioned by Ilpo
> > >   - Reflow, tweak comment in prevent binding to all HID devices on ROG
> > >   - Replace asus_ref.asus with local reference in all patches
> > >   - Add missing kernel doc comments
> > >   - Other minor nits from Ilpo
> > >   - User reported warning due to scheduling work while holding a spinlock.
> > >     Restructure patch for multiple handlers to limit when spinlock is held to
> > >     variable access only. In parallel, setup a workqueue to handle registration
> > >     of led device and setting brightness. This is required as registering the
> > >     led device triggers kbd_led_get which needs to hold the spinlock to
> > >     protect the led_wk value. The workqueue is also required for the hid
> > >     event passthrough to avoid scheduling work while holding the spinlock.
> > >     Apply the workqueue to wmi brightness buttons as well, as that was
> > >     omitted before this series and WMI access was performed.
> > >   - On "HID: asus: prevent binding to all HID devices on ROG", rename
> > >     quirk HANDLE_GENERIC to SKIP_REPORT_FIXUP and only skip report fixup.
> > >     This allows other quirks to apply (applies quirk that fixes keyboard
> > >     being named as a pointer device).
> > >
> > > Changes since V5:
> > >   - It's been a long time
> > >   - Remove addition of RGB as that had some comments I need to work on
> > >   - Remove folio patch (already merged)
> > >   - Remove legacy fix patch 11 from V4. There is a small chance that
> > >     without this patch, some old NKEY keyboards might not respond to
> > >     RGB commands according to Luke, but the kernel driver does not do
> > >     RGB currently. The 0x5d init is done by Armoury crate software in
> > >     Windows. If an issue is found, we can re-add it or just remove patches
> > >     1/2 before merging. However, init could use the cleanup.
> > >
> > > Changes since V4:
> > >   - Fix KConfig (reported by kernel robot)
> > >   - Fix Ilpo's nits, if I missed anything lmk
> > >
> > > Changes since V3:
> > >   - Add initializer for 0x5d for old NKEY keyboards until it is verified
> > >     that it is not needed for their media keys to function.
> > >   - Cover init in asus-wmi with spinlock as per Hans
> > >   - If asus-wmi registers WMI handler with brightness, init the brightness
> > >     in USB Asus keyboards, per Hans.
> > >   - Change hid handler name to asus-UNIQ:rgb:peripheral to match led class
> > >   - Fix oops when unregistering asus-wmi by moving unregister outside of
> > >     the spin lock (but after the asus reference is set to null)
> > >
> > > Changes since V2:
> > >   - Check lazy init succeds in asus-wmi before setting register variable
> > >   - make explicit check in asus_hid_register_listener for listener existing
> > >     to avoid re-init
> > >   - rename asus_brt to asus_hid in most places and harmonize everything
> > >   - switch to a spinlock instead of a mutex to avoid kernel ooops
> > >   - fixup hid device quirks to avoid multiple RGB devices while still exposing
> > >     all input vendor devices. This includes moving rgb init to probe
> > >     instead of the input_configured callbacks.
> > >   - Remove fan key (during retest it appears to be 0xae that is already
> > >     supported by hid-asus)
> > >   - Never unregister asus::kbd_backlight while asus-wmi is active, as that
> > >   - removes fds from userspace and breaks backlight functionality. All
> > >   - current mainline drivers do not support backlight hotplugging, so most
> > >     userspace software (e.g., KDE, UPower) is built with that assumption.
> > >     For the Ally, since it disconnects its controller during sleep, this
> > >     caused the backlight slider to not work in KDE.
> > >
> > > Changes since V1:
> > >   - Add basic RGB support on hid-asus, (Z13/Ally) tested in KDE/Z13
> > >   - Fix ifdef else having an invalid signature (reported by kernel robot)
> > >   - Restore input arguments to init and keyboard function so they can
> > >     be re-used for RGB controls.
> > >   - Remove Z13 delay (it did not work to fix the touchpad) and replace it
> > >     with a HID_GROUP_GENERIC quirk to allow hid-multitouch to load. Squash
> > >     keyboard rename into it.
> > >   - Unregister brightness listener before removing work queue to avoid
> > >     a race condition causing corruption
> > >   - Remove spurious mutex unlock in asus_brt_event
> > >   - Place mutex lock in kbd_led_set after LED_UNREGISTERING check to avoid
> > >     relocking the mutex and causing a deadlock when unregistering leds
> > >   - Add extra check during unregistering to avoid calling unregister when
> > >     no led device is registered.
> > >   - Temporarily HID_QUIRK_INPUT_PER_APP from the ROG endpoint as it causes
> > >     the driver to create 4 RGB handlers per device. I also suspect some
> > >     extra events sneak through (KDE had the @@@@@@).
> > >
> > > Antheas Kapenekakis (10):
> > >   HID: asus: simplify RGB init sequence
> > >   HID: asus: use same report_id in response
> > >   HID: asus: fortify keyboard handshake
> > >   HID: asus: prevent binding to all HID devices on ROG
> > >   HID: asus: initialize LED endpoint early for old NKEY keyboards
> > >   platform/x86: asus-wmi: Add support for multiple kbd led 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
> > >
> > >  drivers/hid/hid-asus.c                     | 222 +++++++++++----------
> > >  drivers/platform/x86/asus-wmi.c            | 214 +++++++++++++++++---
> > >  include/linux/platform_data/x86/asus-wmi.h |  70 +++----
> > >  3 files changed, 331 insertions(+), 175 deletions(-)
> > >
> > >
> > > base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada
> > > --
> > > 2.51.2
> > >
> > >
> >
>

Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling

[Denis Benato]

On 11/12/25 14:41, Antheas Kapenekakis wrote:
> On Wed, 12 Nov 2025 at 14:22, Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
>> On Wed, 12 Nov 2025, Antheas Kapenekakis wrote:
>>
>>> On Sat, 1 Nov 2025 at 11:47, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>>>> This is a two part series which does the following:
>>>>   - Clean-up init sequence
>>>>   - Unify 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.
>>>>
>>>> For more context, see cover letter of V1. Since V5, I removed some patches
>>>> to make this easier to merge.
>>> Small bump for this.
>> I looked at v8 earlier but then got an impression some of Denis' comments
>> against v7 were not taken into account in v8, which implies there will be
>> delay until I've time to delve into the details (I need to understand
>> things pretty deeply in such a case, which does take lots of time).
>>
>> Alternatively, if Denis says v8 is acceptable, then I don't need to spend
>> so much time on it, but somehow I've a feeling he isn't happy with v8
>> but just hasn't voiced it again...
>>
>> Please do realize that ignoring reviewer feedback without a very very good
>> reason always risks adding delay or friction into getting things
>> upstreamed. Especially, when the review comes from a person who has been
>> around for me to learn to trust their reviews or from a maintainer of the
>> code in question.
> Sure, sorry if it came out this way. Dennis had two comments on the V7
> version of the series.
>
> The first one was that asusctl has a hang bug, which he hasn't had
> time to look into yet. This should have been fixed by dropping the
> HID_QUIRK_INPUT_PER_APP. I retested the series and that QUIRK was a
> bit of a NOOP that does not need to be added in the future.
So it is supposed to not regress it now, correct?
> The second is he is concerned with dropping the 0x5d/0x5e inits. Luke
> said (back in March) that it is fine to drop 0x5e because it is only
> used for ANIME displays. However, for 0x5d, it is hard to verify some
> of the older laptops because they have only been tested with 0x5d and
> we do not have the hardware in question to test.
>
> For this series, I re-added "initialize LED endpoint early for old
> NKEY keyboards" that re-adds 0x5d for the keyboards that cannot be
> tested again so this comment should be resolved too. With that in
> mind, we do end up with an additional quirk/command that may be
> unneeded and will remain there forever, but since it was a point of
> contention, it is not worth arguing over.
>
> So both comments should be resolved
The driver should also not late-initialize anything.

Windows doesn't do it and the official asus application
can freely send LEDs changing commands to either WMI or USB
so I don't see any reason to do things differently [than windows]
and not prepare every USB endpoint to receive commands,
this has not been addressed unless I confused v7 and v8?
> @Denis: can give an ack if this is the case?
>
> As for Derek's comment, he has a PR for his project where he removes
> the name match for Ally devices with ample time for it to be merged
> until kernel 6.19 is released. In addition, that specific software for
> full functionality relies on OOT drivers on the distros it ships with,
> so it is minimally affected in either case.
The part we are talking about depends on this driver (hid-asus)
and there are people on asus-linux community using inputplumber
for non-ally devices (the OOT driver is only for ally devices)
therefore it is very important to us (and various other distributions)
not to break that software in any way.

Weighting pros and cons of changing the name I am not sure
changing name brings any benefit? Or am I missing something here?
It's simply used by userspace so the hardware should be loading
regardless of the name...

Along with IP and your tool and asusctl there is also openrgb,
and a newborn application for asus devices (I don't have contacts
with that dev nor I remember the name of the tool)
and I am not even that sure these are all asus-related
applications.

Excercise EXTRA care touching this area as these are enough changes
to make it difficult to understand what exactly is the problem if
someone shows up with LEDs malfunctioning, laptop not entering sleep
anymore or something else entirely. Plus over time
ASUS has used various workarounds for windows problems
and I am not eager to find out what those are since there is only
a realistic way it's going to happen....
> Moreover, that specific commit is needed for Xbox Ally devices anyway,
> as the kernel kicks one of the RGB endpoints because it does not
> register an input device (the check skipped by early return) so
> userspace becomes unable to control RGB on a stock kernel
> (hidraw/hiddev nodes are gone). For more context here, this specific
> endpoint implements the RGB Lamparray protocol for Windows dynamic
> lighting, and I think in an attempt to make it work properly in
> Windows, Asus made it so Windows has to first disable dynamic lighting
> for armoury crate RGB commands to work (the 0x5a ones over the 0xff31
> hid page).
Yes once ASUS introduces something new it sticks with that for
future models so it's expected more and more laptops will have
the same problem: I am not questioning if these patches are needed
as they very clearly are; I am questioning if everything that these
patches are doing are worth doing and aren't breaking/regressing
either tools or the flow of actions between the EC and these USB devices.
> Hopefully this clears things up
>
> Antheas
>
>>> Unrelated but I was b4ing this series on Ubuntu 24 and got BADSIG:
>>> DKIM/antheas.dev. Is there a reference for fixing this on my host?
>>> Perhaps it would help with spam
>> I see BADSIG very often these days from b4 (thanks to gmail expiring
>> things after 7 days or so, I recall hearing somewhere), I just ignore them
>> entirely.
>>
>> AFAIK, that has never caused any delay to any patch in pdx86 domain so if
>> that is what you thought is happening here, it's not the case.
>> If your patch does appear in the pdx86 patchwork, there's even less reason
>> to worry as I mostly pick patches to process using patchwork's list.
> Turns out I had to update my DNS records. It should be good now.
>
>> --
>>  i.
>>
>>> Antheas
>>>
>>>> ---
>>>> V7: https://lore.kernel.org/all/20251018101759.4089-1-lkml@antheas.dev/
>>>> V6: https://lore.kernel.org/all/20251013201535.6737-1-lkml@antheas.dev/
>>>> V5: https://lore.kernel.org/all/20250325184601.10990-1-lkml@antheas.dev/
>>>> V4: https://lore.kernel.org/lkml/20250324210151.6042-1-lkml@antheas.dev/
>>>> V3: https://lore.kernel.org/lkml/20250322102804.418000-1-lkml@antheas.dev/
>>>> V2: https://lore.kernel.org/all/20250320220924.5023-1-lkml@antheas.dev/
>>>> V1: https://lore.kernel.org/all/20250319191320.10092-1-lkml@antheas.dev/
>>>>
>>>> Changes since V7:
>>>>   - Readd legacy init quirk for Dennis
>>>>   - Remove HID_QUIRK_INPUT_PER_APP as a courtesy to asusctl
>>>>   - Fix warning due to enum_backlight receiving negative values
>>>>
>>>> Changes since V6:
>>>>   - Split initialization refactor into three patches, update commit text
>>>>     to be clearer in what it does
>>>>   - Replace spinlock accesses with guard and scoped guard in all patches
>>>>   - Add missing includes mentioned by Ilpo
>>>>   - Reflow, tweak comment in prevent binding to all HID devices on ROG
>>>>   - Replace asus_ref.asus with local reference in all patches
>>>>   - Add missing kernel doc comments
>>>>   - Other minor nits from Ilpo
>>>>   - User reported warning due to scheduling work while holding a spinlock.
>>>>     Restructure patch for multiple handlers to limit when spinlock is held to
>>>>     variable access only. In parallel, setup a workqueue to handle registration
>>>>     of led device and setting brightness. This is required as registering the
>>>>     led device triggers kbd_led_get which needs to hold the spinlock to
>>>>     protect the led_wk value. The workqueue is also required for the hid
>>>>     event passthrough to avoid scheduling work while holding the spinlock.
>>>>     Apply the workqueue to wmi brightness buttons as well, as that was
>>>>     omitted before this series and WMI access was performed.
>>>>   - On "HID: asus: prevent binding to all HID devices on ROG", rename
>>>>     quirk HANDLE_GENERIC to SKIP_REPORT_FIXUP and only skip report fixup.
>>>>     This allows other quirks to apply (applies quirk that fixes keyboard
>>>>     being named as a pointer device).
>>>>
>>>> Changes since V5:
>>>>   - It's been a long time
>>>>   - Remove addition of RGB as that had some comments I need to work on
>>>>   - Remove folio patch (already merged)
>>>>   - Remove legacy fix patch 11 from V4. There is a small chance that
>>>>     without this patch, some old NKEY keyboards might not respond to
>>>>     RGB commands according to Luke, but the kernel driver does not do
>>>>     RGB currently. The 0x5d init is done by Armoury crate software in
>>>>     Windows. If an issue is found, we can re-add it or just remove patches
>>>>     1/2 before merging. However, init could use the cleanup.
>>>>
>>>> Changes since V4:
>>>>   - Fix KConfig (reported by kernel robot)
>>>>   - Fix Ilpo's nits, if I missed anything lmk
>>>>
>>>> Changes since V3:
>>>>   - Add initializer for 0x5d for old NKEY keyboards until it is verified
>>>>     that it is not needed for their media keys to function.
>>>>   - Cover init in asus-wmi with spinlock as per Hans
>>>>   - If asus-wmi registers WMI handler with brightness, init the brightness
>>>>     in USB Asus keyboards, per Hans.
>>>>   - Change hid handler name to asus-UNIQ:rgb:peripheral to match led class
>>>>   - Fix oops when unregistering asus-wmi by moving unregister outside of
>>>>     the spin lock (but after the asus reference is set to null)
>>>>
>>>> Changes since V2:
>>>>   - Check lazy init succeds in asus-wmi before setting register variable
>>>>   - make explicit check in asus_hid_register_listener for listener existing
>>>>     to avoid re-init
>>>>   - rename asus_brt to asus_hid in most places and harmonize everything
>>>>   - switch to a spinlock instead of a mutex to avoid kernel ooops
>>>>   - fixup hid device quirks to avoid multiple RGB devices while still exposing
>>>>     all input vendor devices. This includes moving rgb init to probe
>>>>     instead of the input_configured callbacks.
>>>>   - Remove fan key (during retest it appears to be 0xae that is already
>>>>     supported by hid-asus)
>>>>   - Never unregister asus::kbd_backlight while asus-wmi is active, as that
>>>>   - removes fds from userspace and breaks backlight functionality. All
>>>>   - current mainline drivers do not support backlight hotplugging, so most
>>>>     userspace software (e.g., KDE, UPower) is built with that assumption.
>>>>     For the Ally, since it disconnects its controller during sleep, this
>>>>     caused the backlight slider to not work in KDE.
>>>>
>>>> Changes since V1:
>>>>   - Add basic RGB support on hid-asus, (Z13/Ally) tested in KDE/Z13
>>>>   - Fix ifdef else having an invalid signature (reported by kernel robot)
>>>>   - Restore input arguments to init and keyboard function so they can
>>>>     be re-used for RGB controls.
>>>>   - Remove Z13 delay (it did not work to fix the touchpad) and replace it
>>>>     with a HID_GROUP_GENERIC quirk to allow hid-multitouch to load. Squash
>>>>     keyboard rename into it.
>>>>   - Unregister brightness listener before removing work queue to avoid
>>>>     a race condition causing corruption
>>>>   - Remove spurious mutex unlock in asus_brt_event
>>>>   - Place mutex lock in kbd_led_set after LED_UNREGISTERING check to avoid
>>>>     relocking the mutex and causing a deadlock when unregistering leds
>>>>   - Add extra check during unregistering to avoid calling unregister when
>>>>     no led device is registered.
>>>>   - Temporarily HID_QUIRK_INPUT_PER_APP from the ROG endpoint as it causes
>>>>     the driver to create 4 RGB handlers per device. I also suspect some
>>>>     extra events sneak through (KDE had the @@@@@@).
>>>>
>>>> Antheas Kapenekakis (10):
>>>>   HID: asus: simplify RGB init sequence
>>>>   HID: asus: use same report_id in response
>>>>   HID: asus: fortify keyboard handshake
>>>>   HID: asus: prevent binding to all HID devices on ROG
>>>>   HID: asus: initialize LED endpoint early for old NKEY keyboards
>>>>   platform/x86: asus-wmi: Add support for multiple kbd led 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
>>>>
>>>>  drivers/hid/hid-asus.c                     | 222 +++++++++++----------
>>>>  drivers/platform/x86/asus-wmi.c            | 214 +++++++++++++++++---
>>>>  include/linux/platform_data/x86/asus-wmi.h |  70 +++----
>>>>  3 files changed, 331 insertions(+), 175 deletions(-)
>>>>
>>>>
>>>> base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada
>>>> --
>>>> 2.51.2
>>>>
>>>>

Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling

[Antheas Kapenekakis]

On Wed, 12 Nov 2025 at 20:51, Denis Benato <benato.denis96@gmail.com> wrote:
>
>
> On 11/12/25 14:41, Antheas Kapenekakis wrote:
> > On Wed, 12 Nov 2025 at 14:22, Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:
> >> On Wed, 12 Nov 2025, Antheas Kapenekakis wrote:
> >>
> >>> On Sat, 1 Nov 2025 at 11:47, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >>>> This is a two part series which does the following:
> >>>>   - Clean-up init sequence
> >>>>   - Unify 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.
> >>>>
> >>>> For more context, see cover letter of V1. Since V5, I removed some patches
> >>>> to make this easier to merge.
> >>> Small bump for this.
> >> I looked at v8 earlier but then got an impression some of Denis' comments
> >> against v7 were not taken into account in v8, which implies there will be
> >> delay until I've time to delve into the details (I need to understand
> >> things pretty deeply in such a case, which does take lots of time).
> >>
> >> Alternatively, if Denis says v8 is acceptable, then I don't need to spend
> >> so much time on it, but somehow I've a feeling he isn't happy with v8
> >> but just hasn't voiced it again...
> >>
> >> Please do realize that ignoring reviewer feedback without a very very good
> >> reason always risks adding delay or friction into getting things
> >> upstreamed. Especially, when the review comes from a person who has been
> >> around for me to learn to trust their reviews or from a maintainer of the
> >> code in question.
> > Sure, sorry if it came out this way. Dennis had two comments on the V7
> > version of the series.
> >
> > The first one was that asusctl has a hang bug, which he hasn't had
> > time to look into yet. This should have been fixed by dropping the
> > HID_QUIRK_INPUT_PER_APP. I retested the series and that QUIRK was a
> > bit of a NOOP that does not need to be added in the future.
> So it is supposed to not regress it now, correct?
> > The second is he is concerned with dropping the 0x5d/0x5e inits. Luke
> > said (back in March) that it is fine to drop 0x5e because it is only
> > used for ANIME displays. However, for 0x5d, it is hard to verify some
> > of the older laptops because they have only been tested with 0x5d and
> > we do not have the hardware in question to test.
> >
> > For this series, I re-added "initialize LED endpoint early for old
> > NKEY keyboards" that re-adds 0x5d for the keyboards that cannot be
> > tested again so this comment should be resolved too. With that in
> > mind, we do end up with an additional quirk/command that may be
> > unneeded and will remain there forever, but since it was a point of
> > contention, it is not worth arguing over.
> >
> > So both comments should be resolved
> The driver should also not late-initialize anything.
>
> Windows doesn't do it and the official asus application
> can freely send LEDs changing commands to either WMI or USB
> so I don't see any reason to do things differently [than windows]
> and not prepare every USB endpoint to receive commands,
> this has not been addressed unless I confused v7 and v8?

Yes, it's been added on v8. 0x5d is init for the laptops it is
problematic for. Not because it does not work, but because it has not
been verified to work for those laptops.

> > @Denis: can give an ack if this is the case?
> >
> > As for Derek's comment, he has a PR for his project where he removes
> > the name match for Ally devices with ample time for it to be merged
> > until kernel 6.19 is released. In addition, that specific software for
> > full functionality relies on OOT drivers on the distros it ships with,
> > so it is minimally affected in either case.
> The part we are talking about depends on this driver (hid-asus)
> and there are people on asus-linux community using inputplumber
> for non-ally devices (the OOT driver is only for ally devices)
> therefore it is very important to us (and various other distributions)
> not to break that software in any way.

This driver is only used for Ally devices. If you mean that people
remap their keyboards using inputplumber I guess they could but I have
not seen it.

> Weighting pros and cons of changing the name I am not sure
> changing name brings any benefit? Or am I missing something here?
> It's simply used by userspace so the hardware should be loading
> regardless of the name...

Users see the name of their devices in their settings menu. They
should be accurate. Also, the early entry needs to be added anyway to
prevent kicking devices.

> Along with IP and your tool and asusctl there is also openrgb,
> and a newborn application for asus devices (I don't have contacts
> with that dev nor I remember the name of the tool)
> and I am not even that sure these are all asus-related
> applications.

My tool never checked for names, it briefly did for around a month
after its creation for some devices until capability matches. Around
6.1 and 6.7 the kernel changed the names of most USB devices and that
caused issues. It currently only uses name matches for VID/PID 0/0
devices created by the kernel. Specifically, WMI and AT Keyboards. I
am not sure there is a workaround for those. Asusctl also does not use
names either.

> Excercise EXTRA care touching this area as these are enough changes
> to make it difficult to understand what exactly is the problem if
> someone shows up with LEDs malfunctioning, laptop not entering sleep
> anymore or something else entirely. Plus over time
> ASUS has used various workarounds for windows problems
> and I am not eager to find out what those are since there is only
> a realistic way it's going to happen....

These changes are not doing something extraordinary. It's just a minor cleanup.

> > Moreover, that specific commit is needed for Xbox Ally devices anyway,
> > as the kernel kicks one of the RGB endpoints because it does not
> > register an input device (the check skipped by early return) so
> > userspace becomes unable to control RGB on a stock kernel
> > (hidraw/hiddev nodes are gone). For more context here, this specific
> > endpoint implements the RGB Lamparray protocol for Windows dynamic
> > lighting, and I think in an attempt to make it work properly in
> > Windows, Asus made it so Windows has to first disable dynamic lighting
> > for armoury crate RGB commands to work (the 0x5a ones over the 0xff31
> > hid page).
> Yes once ASUS introduces something new it sticks with that for
> future models so it's expected more and more laptops will have
> the same problem: I am not questioning if these patches are needed
> as they very clearly are; I am questioning if everything that these
> patches are doing are worth doing and aren't breaking/regressing
> either tools or the flow of actions between the EC and these USB devices.

Well, this series is needed to account for that. Sending the disable
command is out of scope for now though.

Antheas

> > Hopefully this clears things up
> >
> > Antheas
> >
> >>> Unrelated but I was b4ing this series on Ubuntu 24 and got BADSIG:
> >>> DKIM/antheas.dev. Is there a reference for fixing this on my host?
> >>> Perhaps it would help with spam
> >> I see BADSIG very often these days from b4 (thanks to gmail expiring
> >> things after 7 days or so, I recall hearing somewhere), I just ignore them
> >> entirely.
> >>
> >> AFAIK, that has never caused any delay to any patch in pdx86 domain so if
> >> that is what you thought is happening here, it's not the case.
> >> If your patch does appear in the pdx86 patchwork, there's even less reason
> >> to worry as I mostly pick patches to process using patchwork's list.
> > Turns out I had to update my DNS records. It should be good now.
> >
> >> --
> >>  i.
> snipp
> >>>> 2.51.2
> >>>>
> >>>>
>

Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling

[Antheas Kapenekakis]

On Wed, 12 Nov 2025 at 23:08, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>
> snip
> > > Sure, sorry if it came out this way. Dennis had two comments on the V7
> > > version of the series.
> > >
> > > The first one was that asusctl has a hang bug, which he hasn't had
> > > time to look into yet. This should have been fixed by dropping the
> > > HID_QUIRK_INPUT_PER_APP. I retested the series and that QUIRK was a
> > > bit of a NOOP that does not need to be added in the future.
> > So it is supposed to not regress it now, correct?

I missed this. Spaces. Yes, quirk input per app created around 3 more
input devices per USB device, plus the dynamic lighting one, so you
went from 2 to 6, and there seems to be an issue with asusctl and a
large number of event devices, that caused some intermittent freezing
when the user typed on certain keyboards. I removed it. Although, not
a problem with the kernel itself, per say.

Antheas

> > > The second is he is concerned with dropping the 0x5d/0x5e inits. Luke
> > > said (back in March) that it is fine to drop 0x5e because it is only
> > > > snip
> > >>>>
> >

Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling

[Denis Benato]

On 11/13/25 00:22, Antheas Kapenekakis wrote:
> On Wed, 12 Nov 2025 at 23:08, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>> snip
>>>> Sure, sorry if it came out this way. Dennis had two comments on the V7
>>>> version of the series.
>>>>
>>>> The first one was that asusctl has a hang bug, which he hasn't had
>>>> time to look into yet. This should have been fixed by dropping the
>>>> HID_QUIRK_INPUT_PER_APP. I retested the series and that QUIRK was a
>>>> bit of a NOOP that does not need to be added in the future.
>>> So it is supposed to not regress it now, correct?
> I missed this. Spaces. Yes, quirk input per app created around 3 more
> input devices per USB device, plus the dynamic lighting one, so you
> went from 2 to 6, and there seems to be an issue with asusctl and a
> large number of event devices, that caused some intermittent freezing
> when the user typed on certain keyboards. I removed it. Although, not
> a problem with the kernel itself, per say.
Okay, I will ask asus-linux kernel man to load this patchset then.
> Antheas
>
>>>> The second is he is concerned with dropping the 0x5d/0x5e inits. Luke
>>>> said (back in March) that it is fine to drop 0x5e because it is only
>>>>> snip

Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling

[Denis Benato]

On 11/12/25 23:08, Antheas Kapenekakis wrote:
> On Wed, 12 Nov 2025 at 20:51, Denis Benato <benato.denis96@gmail.com> wrote:
>>
>> On 11/12/25 14:41, Antheas Kapenekakis wrote:
>>> On Wed, 12 Nov 2025 at 14:22, Ilpo Järvinen
>>> <ilpo.jarvinen@linux.intel.com> wrote:
>>>> On Wed, 12 Nov 2025, Antheas Kapenekakis wrote:
>>>>
>>>>> On Sat, 1 Nov 2025 at 11:47, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>>>>>> This is a two part series which does the following:
>>>>>>   - Clean-up init sequence
>>>>>>   - Unify 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.
>>>>>>
>>>>>> For more context, see cover letter of V1. Since V5, I removed some patches
>>>>>> to make this easier to merge.
>>>>> Small bump for this.
>>>> I looked at v8 earlier but then got an impression some of Denis' comments
>>>> against v7 were not taken into account in v8, which implies there will be
>>>> delay until I've time to delve into the details (I need to understand
>>>> things pretty deeply in such a case, which does take lots of time).
>>>>
>>>> Alternatively, if Denis says v8 is acceptable, then I don't need to spend
>>>> so much time on it, but somehow I've a feeling he isn't happy with v8
>>>> but just hasn't voiced it again...
>>>>
>>>> Please do realize that ignoring reviewer feedback without a very very good
>>>> reason always risks adding delay or friction into getting things
>>>> upstreamed. Especially, when the review comes from a person who has been
>>>> around for me to learn to trust their reviews or from a maintainer of the
>>>> code in question.
>>> Sure, sorry if it came out this way. Dennis had two comments on the V7
>>> version of the series.
>>>
>>> The first one was that asusctl has a hang bug, which he hasn't had
>>> time to look into yet. This should have been fixed by dropping the
>>> HID_QUIRK_INPUT_PER_APP. I retested the series and that QUIRK was a
>>> bit of a NOOP that does not need to be added in the future.
>> So it is supposed to not regress it now, correct?
>>> The second is he is concerned with dropping the 0x5d/0x5e inits. Luke
>>> said (back in March) that it is fine to drop 0x5e because it is only
>>> used for ANIME displays. However, for 0x5d, it is hard to verify some
>>> of the older laptops because they have only been tested with 0x5d and
>>> we do not have the hardware in question to test.
>>>
>>> For this series, I re-added "initialize LED endpoint early for old
>>> NKEY keyboards" that re-adds 0x5d for the keyboards that cannot be
>>> tested again so this comment should be resolved too. With that in
>>> mind, we do end up with an additional quirk/command that may be
>>> unneeded and will remain there forever, but since it was a point of
>>> contention, it is not worth arguing over.
>>>
>>> So both comments should be resolved
>> The driver should also not late-initialize anything.
>>
>> Windows doesn't do it and the official asus application
>> can freely send LEDs changing commands to either WMI or USB
>> so I don't see any reason to do things differently [than windows]
>> and not prepare every USB endpoint to receive commands,
>> this has not been addressed unless I confused v7 and v8?
> Yes, it's been added on v8. 0x5d is init for the laptops it is
> problematic for. Not because it does not work, but because it has not
> been verified to work for those laptops.
I am not sure I am reading this right:
are you telling me that on recent models the windows driver
doesn't issue 0x5d?
>>> @Denis: can give an ack if this is the case?
>>>
>>> As for Derek's comment, he has a PR for his project where he removes
>>> the name match for Ally devices with ample time for it to be merged
>>> until kernel 6.19 is released. In addition, that specific software for
>>> full functionality relies on OOT drivers on the distros it ships with,
>>> so it is minimally affected in either case.
>> The part we are talking about depends on this driver (hid-asus)
>> and there are people on asus-linux community using inputplumber
>> for non-ally devices (the OOT driver is only for ally devices)
>> therefore it is very important to us (and various other distributions)
>> not to break that software in any way.
> This driver is only used for Ally devices. If you mean that people
> remap their keyboards using inputplumber I guess they could but I have
> not seen it.
I meant people remap keyboards using IP. I am sure there were
(and very probably still are) people doing that.
>> Weighting pros and cons of changing the name I am not sure
>> changing name brings any benefit? Or am I missing something here?
>> It's simply used by userspace so the hardware should be loading
>> regardless of the name...
> Users see the name of their devices in their settings menu. They
> should be accurate. Also, the early entry needs to be added anyway to
> prevent kicking devices.
If it's just aesthetics I don't see much reasons in changing the name.

"the early entry needs to be added anyway ...." has no meaning to me,
please rephrase. Sorry.

>> Along with IP and your tool and asusctl there is also openrgb,
>> and a newborn application for asus devices (I don't have contacts
>> with that dev nor I remember the name of the tool)
>> and I am not even that sure these are all asus-related
>> applications.
> My tool never checked for names, it briefly did for around a month
> after its creation for some devices until capability matches. Around
> 6.1 and 6.7 the kernel changed the names of most USB devices and that
> caused issues. It currently only uses name matches for VID/PID 0/0
> devices created by the kernel. Specifically, WMI and AT Keyboards. I
> am not sure there is a workaround for those. Asusctl also does not use
> names either.
But IP does, so I would like to hear confirmation from at least Derek
before the merge that there won't be future issues.

Interpret what I say here as a broad topic, not just name/PER_APP flag:
avoid changing data flow on older models...

>> Excercise EXTRA care touching this area as these are enough changes
>> to make it difficult to understand what exactly is the problem if
>> someone shows up with LEDs malfunctioning, laptop not entering sleep
>> anymore or something else entirely. Plus over time
>> ASUS has used various workarounds for windows problems
>> and I am not eager to find out what those are since there is only
>> a realistic way it's going to happen....
> These changes are not doing something extraordinary. It's just a minor cleanup.
>
>>> Moreover, that specific commit is needed for Xbox Ally devices anyway,
>>> as the kernel kicks one of the RGB endpoints because it does not
>>> register an input device (the check skipped by early return) so
>>> userspace becomes unable to control RGB on a stock kernel
>>> (hidraw/hiddev nodes are gone). For more context here, this specific
>>> endpoint implements the RGB Lamparray protocol for Windows dynamic
>>> lighting, and I think in an attempt to make it work properly in
>>> Windows, Asus made it so Windows has to first disable dynamic lighting
>>> for armoury crate RGB commands to work (the 0x5a ones over the 0xff31
>>> hid page).
>> Yes once ASUS introduces something new it sticks with that for
>> future models so it's expected more and more laptops will have
>> the same problem: I am not questioning if these patches are needed
>> as they very clearly are; I am questioning if everything that these
>> patches are doing are worth doing and aren't breaking/regressing
>> either tools or the flow of actions between the EC and these USB devices.
> Well, this series is needed to account for that. Sending the disable
> command is out of scope for now though.
Here I apologize for confusion: my comments were mostly about
older models: I absolutely don't want to break those, but if you find a way
to distinguish them from newer models that would give you more freedom with those.

No disable commands unless we find hard evidence those are strictly needed.

> Antheas
>
>>> Hopefully this clears things up
>>>
>>> Antheas
>>>
>>>>> Unrelated but I was b4ing this series on Ubuntu 24 and got BADSIG:
>>>>> DKIM/antheas.dev. Is there a reference for fixing this on my host?
>>>>> Perhaps it would help with spam
>>>> I see BADSIG very often these days from b4 (thanks to gmail expiring
>>>> things after 7 days or so, I recall hearing somewhere), I just ignore them
>>>> entirely.
>>>>
>>>> AFAIK, that has never caused any delay to any patch in pdx86 domain so if
>>>> that is what you thought is happening here, it's not the case.
>>>> If your patch does appear in the pdx86 patchwork, there's even less reason
>>>> to worry as I mostly pick patches to process using patchwork's list.
>>> Turns out I had to update my DNS records. It should be good now.
>>>
>>>> --
>>>>  i.
>> snipp
>>>>>> 2.51.2
>>>>>>
>>>>>>

Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling

[Antheas Kapenekakis]

On Thu, 13 Nov 2025 at 02:14, Denis Benato <benato.denis96@gmail.com> wrote:
>
>
> On 11/12/25 23:08, Antheas Kapenekakis wrote:
> > On Wed, 12 Nov 2025 at 20:51, Denis Benato <benato.denis96@gmail.com> wrote:
> >>
> >> On 11/12/25 14:41, Antheas Kapenekakis wrote:
> >>> On Wed, 12 Nov 2025 at 14:22, Ilpo Järvinen
> >>> <ilpo.jarvinen@linux.intel.com> wrote:
> >>>> On Wed, 12 Nov 2025, Antheas Kapenekakis wrote:
> >>>>
> >>>>> On Sat, 1 Nov 2025 at 11:47, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >>>>>> This is a two part series which does the following:
> >>>>>>   - Clean-up init sequence
> >>>>>>   - Unify 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.
> >>>>>>
> >>>>>> For more context, see cover letter of V1. Since V5, I removed some patches
> >>>>>> to make this easier to merge.
> >>>>> Small bump for this.
> >>>> I looked at v8 earlier but then got an impression some of Denis' comments
> >>>> against v7 were not taken into account in v8, which implies there will be
> >>>> delay until I've time to delve into the details (I need to understand
> >>>> things pretty deeply in such a case, which does take lots of time).
> >>>>
> >>>> Alternatively, if Denis says v8 is acceptable, then I don't need to spend
> >>>> so much time on it, but somehow I've a feeling he isn't happy with v8
> >>>> but just hasn't voiced it again...
> >>>>
> >>>> Please do realize that ignoring reviewer feedback without a very very good
> >>>> reason always risks adding delay or friction into getting things
> >>>> upstreamed. Especially, when the review comes from a person who has been
> >>>> around for me to learn to trust their reviews or from a maintainer of the
> >>>> code in question.
> >>> Sure, sorry if it came out this way. Dennis had two comments on the V7
> >>> version of the series.
> >>>
> >>> The first one was that asusctl has a hang bug, which he hasn't had
> >>> time to look into yet. This should have been fixed by dropping the
> >>> HID_QUIRK_INPUT_PER_APP. I retested the series and that QUIRK was a
> >>> bit of a NOOP that does not need to be added in the future.
> >> So it is supposed to not regress it now, correct?
> >>> The second is he is concerned with dropping the 0x5d/0x5e inits. Luke
> >>> said (back in March) that it is fine to drop 0x5e because it is only
> >>> used for ANIME displays. However, for 0x5d, it is hard to verify some
> >>> of the older laptops because they have only been tested with 0x5d and
> >>> we do not have the hardware in question to test.
> >>>
> >>> For this series, I re-added "initialize LED endpoint early for old
> >>> NKEY keyboards" that re-adds 0x5d for the keyboards that cannot be
> >>> tested again so this comment should be resolved too. With that in
> >>> mind, we do end up with an additional quirk/command that may be
> >>> unneeded and will remain there forever, but since it was a point of
> >>> contention, it is not worth arguing over.
> >>>
> >>> So both comments should be resolved
> >> The driver should also not late-initialize anything.
> >>
> >> Windows doesn't do it and the official asus application
> >> can freely send LEDs changing commands to either WMI or USB
> >> so I don't see any reason to do things differently [than windows]
> >> and not prepare every USB endpoint to receive commands,
> >> this has not been addressed unless I confused v7 and v8?
> > Yes, it's been added on v8. 0x5d is init for the laptops it is
> > problematic for. Not because it does not work, but because it has not
> > been verified to work for those laptops.
> I am not sure I am reading this right:
> are you telling me that on recent models the windows driver
> doesn't issue 0x5d?

Try to add spaces in your replies. This is hard to follow.

Do not conflate driver with software. 0x5a (over the application
0xff310076) has traditionally been used by a driver in Windows to
control the backlight level, as it is done in this driver. 0x5d (over
the application 0xff310079) is only used by laptops with RGB by
Armoury crate. But this driver does not do RGB. No device
functionality relies on it being sent for any device I've seen. The
device remembers its Windows settings, incl. the backlight color, in
the absence of a driver.

Laptops without RGB such as the Duo series which I would like to add
support for next only have a 0x5a endpoint. But, they are sent garbage
inits for 0x5d and 0x5e currently. This should be fixed.

Moreso, it seems that Armoury crate on the Xbox Ally series uses
exclusively 0x5a commands and if you use 0x5d it ignores them (perhaps
RGB still works though). With the previous generation, commands worked
for either report id.

> >>> @Denis: can give an ack if this is the case?
> >>>
> >>> As for Derek's comment, he has a PR for his project where he removes
> >>> the name match for Ally devices with ample time for it to be merged
> >>> until kernel 6.19 is released. In addition, that specific software for
> >>> full functionality relies on OOT drivers on the distros it ships with,
> >>> so it is minimally affected in either case.
> >> The part we are talking about depends on this driver (hid-asus)
> >> and there are people on asus-linux community using inputplumber
> >> for non-ally devices (the OOT driver is only for ally devices)
> >> therefore it is very important to us (and various other distributions)
> >> not to break that software in any way.
> > This driver is only used for Ally devices. If you mean that people
> > remap their keyboards using inputplumber I guess they could but I have
> > not seen it.
> I meant people remap keyboards using IP. I am sure there were
> (and very probably still are) people doing that.
> >> Weighting pros and cons of changing the name I am not sure
> >> changing name brings any benefit? Or am I missing something here?
> >> It's simply used by userspace so the hardware should be loading
> >> regardless of the name...
> > Users see the name of their devices in their settings menu. They
> > should be accurate. Also, the early entry needs to be added anyway to
> > prevent kicking devices.
> If it's just aesthetics I don't see much reasons in changing the name.
>
> "the early entry needs to be added anyway ...." has no meaning to me,
> please rephrase. Sorry.

Early exit-

> >> Along with IP and your tool and asusctl there is also openrgb,
> >> and a newborn application for asus devices (I don't have contacts
> >> with that dev nor I remember the name of the tool)
> >> and I am not even that sure these are all asus-related
> >> applications.
> > My tool never checked for names, it briefly did for around a month
> > after its creation for some devices until capability matches. Around
> > 6.1 and 6.7 the kernel changed the names of most USB devices and that
> > caused issues. It currently only uses name matches for VID/PID 0/0
> > devices created by the kernel. Specifically, WMI and AT Keyboards. I
> > am not sure there is a workaround for those. Asusctl also does not use
> > names either.
> But IP does, so I would like to hear confirmation from at least Derek
> before the merge that there won't be future issues.
>
> Interpret what I say here as a broad topic, not just name/PER_APP flag:
> avoid changing data flow on older models...

In [1] Derek removes the name matches

There are no other name matches concerning this driver in it.

The data flow is not changed in this series; you should go through the
patches once again if you think that. The only difference is 0x5e is
not sent, and 0x5d is not sent for newer devices.

[1] https://github.com/ShadowBlip/InputPlumber/pull/453

> >> Excercise EXTRA care touching this area as these are enough changes
> >> to make it difficult to understand what exactly is the problem if
> >> someone shows up with LEDs malfunctioning, laptop not entering sleep
> >> anymore or something else entirely. Plus over time
> >> ASUS has used various workarounds for windows problems
> >> and I am not eager to find out what those are since there is only
> >> a realistic way it's going to happen....
> > These changes are not doing something extraordinary. It's just a minor cleanup.
> >
> >>> Moreover, that specific commit is needed for Xbox Ally devices anyway,
> >>> as the kernel kicks one of the RGB endpoints because it does not
> >>> register an input device (the check skipped by early return) so
> >>> userspace becomes unable to control RGB on a stock kernel
> >>> (hidraw/hiddev nodes are gone). For more context here, this specific
> >>> endpoint implements the RGB Lamparray protocol for Windows dynamic
> >>> lighting, and I think in an attempt to make it work properly in
> >>> Windows, Asus made it so Windows has to first disable dynamic lighting
> >>> for armoury crate RGB commands to work (the 0x5a ones over the 0xff31
> >>> hid page).
> >> Yes once ASUS introduces something new it sticks with that for
> >> future models so it's expected more and more laptops will have
> >> the same problem: I am not questioning if these patches are needed
> >> as they very clearly are; I am questioning if everything that these
> >> patches are doing are worth doing and aren't breaking/regressing
> >> either tools or the flow of actions between the EC and these USB devices.
> > Well, this series is needed to account for that. Sending the disable
> > command is out of scope for now though.
> Here I apologize for confusion: my comments were mostly about
> older models: I absolutely don't want to break those, but if you find a way
> to distinguish them from newer models that would give you more freedom with those.

Yes, we know their specific PIDs, so if you see the patch that adds
the 0x5d init, it is only added for those models.

> No disable commands unless we find hard evidence those are strictly needed.

They are needed for the Xbox Ally series, but since this driver does
not do RGB it is out of scope.

> > Antheas
> >
> >>> Hopefully this clears things up
> >>>
> >>> Antheas
> >>>
> >>>>> Unrelated but I was b4ing this series on Ubuntu 24 and got BADSIG:
> >>>>> DKIM/antheas.dev. Is there a reference for fixing this on my host?
> >>>>> Perhaps it would help with spam
> >>>> I see BADSIG very often these days from b4 (thanks to gmail expiring
> >>>> things after 7 days or so, I recall hearing somewhere), I just ignore them
> >>>> entirely.
> >>>>
> >>>> AFAIK, that has never caused any delay to any patch in pdx86 domain so if
> >>>> that is what you thought is happening here, it's not the case.
> >>>> If your patch does appear in the pdx86 patchwork, there's even less reason
> >>>> to worry as I mostly pick patches to process using patchwork's list.
> >>> Turns out I had to update my DNS records. It should be good now.
> >>>
> >>>> --
> >>>>  i.
> >> snipp
> >>>>>> 2.51.2
> >>>>>>
> >>>>>>
>

Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling

[luke]

> On 13 Nov 2025, at 21:44, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> 
> On Thu, 13 Nov 2025 at 02:14, Denis Benato <benato.denis96@gmail.com> wrote:
>> 
>> 
>> On 11/12/25 23:08, Antheas Kapenekakis wrote:
>>> On Wed, 12 Nov 2025 at 20:51, Denis Benato <benato.denis96@gmail.com> wrote:
>>>> 
>>>> On 11/12/25 14:41, Antheas Kapenekakis wrote:
>>>>> On Wed, 12 Nov 2025 at 14:22, Ilpo Järvinen
>>>>> <ilpo.jarvinen@linux.intel.com> wrote:
>>>>>> On Wed, 12 Nov 2025, Antheas Kapenekakis wrote:
>>>>>> 
>>>>>>> On Sat, 1 Nov 2025 at 11:47, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>>>>>>>> This is a two part series which does the following:
>>>>>>>>  - Clean-up init sequence
>>>>>>>>  - Unify 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.
>>>>>>>> 
>>>>>>>> For more context, see cover letter of V1. Since V5, I removed some patches
>>>>>>>> to make this easier to merge.
>>>>>>> Small bump for this.
>>>>>> I looked at v8 earlier but then got an impression some of Denis' comments
>>>>>> against v7 were not taken into account in v8, which implies there will be
>>>>>> delay until I've time to delve into the details (I need to understand
>>>>>> things pretty deeply in such a case, which does take lots of time).
>>>>>> 
>>>>>> Alternatively, if Denis says v8 is acceptable, then I don't need to spend
>>>>>> so much time on it, but somehow I've a feeling he isn't happy with v8
>>>>>> but just hasn't voiced it again...
>>>>>> 
>>>>>> Please do realize that ignoring reviewer feedback without a very very good
>>>>>> reason always risks adding delay or friction into getting things
>>>>>> upstreamed. Especially, when the review comes from a person who has been
>>>>>> around for me to learn to trust their reviews or from a maintainer of the
>>>>>> code in question.
>>>>> Sure, sorry if it came out this way. Dennis had two comments on the V7
>>>>> version of the series.
>>>>> 
>>>>> The first one was that asusctl has a hang bug, which he hasn't had
>>>>> time to look into yet. This should have been fixed by dropping the
>>>>> HID_QUIRK_INPUT_PER_APP. I retested the series and that QUIRK was a
>>>>> bit of a NOOP that does not need to be added in the future.
>>>> So it is supposed to not regress it now, correct?
>>>>> The second is he is concerned with dropping the 0x5d/0x5e inits. Luke
>>>>> said (back in March) that it is fine to drop 0x5e because it is only
>>>>> used for ANIME displays. However, for 0x5d, it is hard to verify some
>>>>> of the older laptops because they have only been tested with 0x5d and
>>>>> we do not have the hardware in question to test.
>>>>> 
>>>>> For this series, I re-added "initialize LED endpoint early for old
>>>>> NKEY keyboards" that re-adds 0x5d for the keyboards that cannot be
>>>>> tested again so this comment should be resolved too. With that in
>>>>> mind, we do end up with an additional quirk/command that may be
>>>>> unneeded and will remain there forever, but since it was a point of
>>>>> contention, it is not worth arguing over.
>>>>> 
>>>>> So both comments should be resolved
>>>> The driver should also not late-initialize anything.
>>>> 
>>>> Windows doesn't do it and the official asus application
>>>> can freely send LEDs changing commands to either WMI or USB
>>>> so I don't see any reason to do things differently [than windows]
>>>> and not prepare every USB endpoint to receive commands,
>>>> this has not been addressed unless I confused v7 and v8?
>>> Yes, it's been added on v8. 0x5d is init for the laptops it is
>>> problematic for. Not because it does not work, but because it has not
>>> been verified to work for those laptops.
>> I am not sure I am reading this right:
>> are you telling me that on recent models the windows driver
>> doesn't issue 0x5d?
> 
> Try to add spaces in your replies. This is hard to follow.
> 
> Do not conflate driver with software. 0x5a (over the application
> 0xff310076) has traditionally been used by a driver in Windows to
> control the backlight level, as it is done in this driver. 0x5d (over
> the application 0xff310079) is only used by laptops with RGB by
> Armoury crate. But this driver does not do RGB. No device
> functionality relies on it being sent for any device I've seen. The
> device remembers its Windows settings, incl. the backlight color, in
> the absence of a driver.
> 
> Laptops without RGB such as the Duo series which I would like to add
> support for next only have a 0x5a endpoint. But, they are sent garbage
> inits for 0x5d and 0x5e currently. This should be fixed.
> 
> Moreso, it seems that Armoury crate on the Xbox Ally series uses
> exclusively 0x5a commands and if you use 0x5d it ignores them (perhaps
> RGB still works though). With the previous generation, commands worked
> for either report id.
> 
>>>>> @Denis: can give an ack if this is the case?
>>>>> 
>>>>> As for Derek's comment, he has a PR for his project where he removes
>>>>> the name match for Ally devices with ample time for it to be merged
>>>>> until kernel 6.19 is released. In addition, that specific software for
>>>>> full functionality relies on OOT drivers on the distros it ships with,
>>>>> so it is minimally affected in either case.
>>>> The part we are talking about depends on this driver (hid-asus)
>>>> and there are people on asus-linux community using inputplumber
>>>> for non-ally devices (the OOT driver is only for ally devices)
>>>> therefore it is very important to us (and various other distributions)
>>>> not to break that software in any way.
>>> This driver is only used for Ally devices. If you mean that people
>>> remap their keyboards using inputplumber I guess they could but I have
>>> not seen it.
>> I meant people remap keyboards using IP. I am sure there were
>> (and very probably still are) people doing that.
>>>> Weighting pros and cons of changing the name I am not sure
>>>> changing name brings any benefit? Or am I missing something here?
>>>> It's simply used by userspace so the hardware should be loading
>>>> regardless of the name...
>>> Users see the name of their devices in their settings menu. They
>>> should be accurate. Also, the early entry needs to be added anyway to
>>> prevent kicking devices.
>> If it's just aesthetics I don't see much reasons in changing the name.
>> 
>> "the early entry needs to be added anyway ...." has no meaning to me,
>> please rephrase. Sorry.
> 
> Early exit-
> 
>>>> Along with IP and your tool and asusctl there is also openrgb,
>>>> and a newborn application for asus devices (I don't have contacts
>>>> with that dev nor I remember the name of the tool)
>>>> and I am not even that sure these are all asus-related
>>>> applications.
>>> My tool never checked for names, it briefly did for around a month
>>> after its creation for some devices until capability matches. Around
>>> 6.1 and 6.7 the kernel changed the names of most USB devices and that
>>> caused issues. It currently only uses name matches for VID/PID 0/0
>>> devices created by the kernel. Specifically, WMI and AT Keyboards. I
>>> am not sure there is a workaround for those. Asusctl also does not use
>>> names either.
>> But IP does, so I would like to hear confirmation from at least Derek
>> before the merge that there won't be future issues.
>> 
>> Interpret what I say here as a broad topic, not just name/PER_APP flag:
>> avoid changing data flow on older models...
> 
> In [1] Derek removes the name matches
> 
> There are no other name matches concerning this driver in it.
> 
> The data flow is not changed in this series; you should go through the
> patches once again if you think that. The only difference is 0x5e is
> not sent, and 0x5d is not sent for newer devices.
> 
> [1] https://github.com/ShadowBlip/InputPlumber/pull/453
> 
>>>> Excercise EXTRA care touching this area as these are enough changes
>>>> to make it difficult to understand what exactly is the problem if
>>>> someone shows up with LEDs malfunctioning, laptop not entering sleep
>>>> anymore or something else entirely. Plus over time
>>>> ASUS has used various workarounds for windows problems
>>>> and I am not eager to find out what those are since there is only
>>>> a realistic way it's going to happen....
>>> These changes are not doing something extraordinary. It's just a minor cleanup.
>>> 
>>>>> Moreover, that specific commit is needed for Xbox Ally devices anyway,
>>>>> as the kernel kicks one of the RGB endpoints because it does not
>>>>> register an input device (the check skipped by early return) so
>>>>> userspace becomes unable to control RGB on a stock kernel
>>>>> (hidraw/hiddev nodes are gone). For more context here, this specific
>>>>> endpoint implements the RGB Lamparray protocol for Windows dynamic
>>>>> lighting, and I think in an attempt to make it work properly in
>>>>> Windows, Asus made it so Windows has to first disable dynamic lighting
>>>>> for armoury crate RGB commands to work (the 0x5a ones over the 0xff31
>>>>> hid page).
>>>> Yes once ASUS introduces something new it sticks with that for
>>>> future models so it's expected more and more laptops will have
>>>> the same problem: I am not questioning if these patches are needed
>>>> as they very clearly are; I am questioning if everything that these
>>>> patches are doing are worth doing and aren't breaking/regressing
>>>> either tools or the flow of actions between the EC and these USB devices.
>>> Well, this series is needed to account for that. Sending the disable
>>> command is out of scope for now though.
>> Here I apologize for confusion: my comments were mostly about
>> older models: I absolutely don't want to break those, but if you find a way
>> to distinguish them from newer models that would give you more freedom with those.
> 
> Yes, we know their specific PIDs, so if you see the patch that adds
> the 0x5d init, it is only added for those models.

I’m only half keeping up to date on this. I do recall however that the 0x5D init was definitely required for the first ASUS laptop I worked on, and old GX502 - the PID for keyboard is 0x1866 and I think that was the last of that generation MCU. All the previous MCU also required it.

I saw some messages in perhaps another thread where it was mentioned that 0x5E init should be removed? That I agreed with that?
I know there are AniMe and Slash versions that use that init, and they are on the same MCU as the keyboard. I had expected that just one init (on 0x5A or whatever) would work but it doesn’t - what I don’t recall is if an incomplete init affected the keyboard features.

In all reality unless the full set of init is causing issues it’s best to leave them in. If it is then I guess this driver is going to become a little more complex and have a few more quirks.

Unfortunately I didn’t keep good records of my findings on this so it’s just my remembered observations that you’ll have to take at my word.

It would be a good idea for you both to perhaps collaborate with Sergei from ghelper, he has put a huge amount of effort in to that tool and due to it being windows he gets a hell of a lot more use and bug reports/data than this driver does. There’s no shame in looking to others for inspiration, ideas, or guidance.

Cheers,
Luke.

> 
>> No disable commands unless we find hard evidence those are strictly needed.
> 
> They are needed for the Xbox Ally series, but since this driver does
> not do RGB it is out of scope.
> 
>>> Antheas
>>> 
>>>>> Hopefully this clears things up
>>>>> 
>>>>> Antheas
>>>>> 
>>>>>>> Unrelated but I was b4ing this series on Ubuntu 24 and got BADSIG:
>>>>>>> DKIM/antheas.dev. Is there a reference for fixing this on my host?
>>>>>>> Perhaps it would help with spam
>>>>>> I see BADSIG very often these days from b4 (thanks to gmail expiring
>>>>>> things after 7 days or so, I recall hearing somewhere), I just ignore them
>>>>>> entirely.
>>>>>> 
>>>>>> AFAIK, that has never caused any delay to any patch in pdx86 domain so if
>>>>>> that is what you thought is happening here, it's not the case.
>>>>>> If your patch does appear in the pdx86 patchwork, there's even less reason
>>>>>> to worry as I mostly pick patches to process using patchwork's list.
>>>>> Turns out I had to update my DNS records. It should be good now.
>>>>> 
>>>>>> --
>>>>>> i.
>>>> snipp
>>>>>>>> 2.51.2

Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling

[Antheas Kapenekakis]

On Thu, 13 Nov 2025 at 21:23, <luke@ljones.dev> wrote:
>
>
> > On 13 Nov 2025, at 21:44, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >
> > On Thu, 13 Nov 2025 at 02:14, Denis Benato <benato.denis96@gmail.com> wrote:
> >>
> >>
> >> On 11/12/25 23:08, Antheas Kapenekakis wrote:
> >>> On Wed, 12 Nov 2025 at 20:51, Denis Benato <benato.denis96@gmail.com> wrote:
> >>>>
> >>>> On 11/12/25 14:41, Antheas Kapenekakis wrote:
> >>>>> On Wed, 12 Nov 2025 at 14:22, Ilpo Järvinen
> >>>>> <ilpo.jarvinen@linux.intel.com> wrote:
> >>>>>> On Wed, 12 Nov 2025, Antheas Kapenekakis wrote:
> >>>>>>
> >>>>>>> On Sat, 1 Nov 2025 at 11:47, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >>>>>>>> This is a two part series which does the following:
> >>>>>>>>  - Clean-up init sequence
> >>>>>>>>  - Unify 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.
> >>>>>>>>
> >>>>>>>> For more context, see cover letter of V1. Since V5, I removed some patches
> >>>>>>>> to make this easier to merge.
> >>>>>>> Small bump for this.
> >>>>>> I looked at v8 earlier but then got an impression some of Denis' comments
> >>>>>> against v7 were not taken into account in v8, which implies there will be
> >>>>>> delay until I've time to delve into the details (I need to understand
> >>>>>> things pretty deeply in such a case, which does take lots of time).
> >>>>>>
> >>>>>> Alternatively, if Denis says v8 is acceptable, then I don't need to spend
> >>>>>> so much time on it, but somehow I've a feeling he isn't happy with v8
> >>>>>> but just hasn't voiced it again...
> >>>>>>
> >>>>>> Please do realize that ignoring reviewer feedback without a very very good
> >>>>>> reason always risks adding delay or friction into getting things
> >>>>>> upstreamed. Especially, when the review comes from a person who has been
> >>>>>> around for me to learn to trust their reviews or from a maintainer of the
> >>>>>> code in question.
> >>>>> Sure, sorry if it came out this way. Dennis had two comments on the V7
> >>>>> version of the series.
> >>>>>
> >>>>> The first one was that asusctl has a hang bug, which he hasn't had
> >>>>> time to look into yet. This should have been fixed by dropping the
> >>>>> HID_QUIRK_INPUT_PER_APP. I retested the series and that QUIRK was a
> >>>>> bit of a NOOP that does not need to be added in the future.
> >>>> So it is supposed to not regress it now, correct?
> >>>>> The second is he is concerned with dropping the 0x5d/0x5e inits. Luke
> >>>>> said (back in March) that it is fine to drop 0x5e because it is only
> >>>>> used for ANIME displays. However, for 0x5d, it is hard to verify some
> >>>>> of the older laptops because they have only been tested with 0x5d and
> >>>>> we do not have the hardware in question to test.
> >>>>>
> >>>>> For this series, I re-added "initialize LED endpoint early for old
> >>>>> NKEY keyboards" that re-adds 0x5d for the keyboards that cannot be
> >>>>> tested again so this comment should be resolved too. With that in
> >>>>> mind, we do end up with an additional quirk/command that may be
> >>>>> unneeded and will remain there forever, but since it was a point of
> >>>>> contention, it is not worth arguing over.
> >>>>>
> >>>>> So both comments should be resolved
> >>>> The driver should also not late-initialize anything.
> >>>>
> >>>> Windows doesn't do it and the official asus application
> >>>> can freely send LEDs changing commands to either WMI or USB
> >>>> so I don't see any reason to do things differently [than windows]
> >>>> and not prepare every USB endpoint to receive commands,
> >>>> this has not been addressed unless I confused v7 and v8?
> >>> Yes, it's been added on v8. 0x5d is init for the laptops it is
> >>> problematic for. Not because it does not work, but because it has not
> >>> been verified to work for those laptops.
> >> I am not sure I am reading this right:
> >> are you telling me that on recent models the windows driver
> >> doesn't issue 0x5d?
> >
> > Try to add spaces in your replies. This is hard to follow.
> >
> > Do not conflate driver with software. 0x5a (over the application
> > 0xff310076) has traditionally been used by a driver in Windows to
> > control the backlight level, as it is done in this driver. 0x5d (over
> > the application 0xff310079) is only used by laptops with RGB by
> > Armoury crate. But this driver does not do RGB. No device
> > functionality relies on it being sent for any device I've seen. The
> > device remembers its Windows settings, incl. the backlight color, in
> > the absence of a driver.
> >
> > Laptops without RGB such as the Duo series which I would like to add
> > support for next only have a 0x5a endpoint. But, they are sent garbage
> > inits for 0x5d and 0x5e currently. This should be fixed.
> >
> > Moreso, it seems that Armoury crate on the Xbox Ally series uses
> > exclusively 0x5a commands and if you use 0x5d it ignores them (perhaps
> > RGB still works though). With the previous generation, commands worked
> > for either report id.
> >
> >>>>> @Denis: can give an ack if this is the case?
> >>>>>
> >>>>> As for Derek's comment, he has a PR for his project where he removes
> >>>>> the name match for Ally devices with ample time for it to be merged
> >>>>> until kernel 6.19 is released. In addition, that specific software for
> >>>>> full functionality relies on OOT drivers on the distros it ships with,
> >>>>> so it is minimally affected in either case.
> >>>> The part we are talking about depends on this driver (hid-asus)
> >>>> and there are people on asus-linux community using inputplumber
> >>>> for non-ally devices (the OOT driver is only for ally devices)
> >>>> therefore it is very important to us (and various other distributions)
> >>>> not to break that software in any way.
> >>> This driver is only used for Ally devices. If you mean that people
> >>> remap their keyboards using inputplumber I guess they could but I have
> >>> not seen it.
> >> I meant people remap keyboards using IP. I am sure there were
> >> (and very probably still are) people doing that.
> >>>> Weighting pros and cons of changing the name I am not sure
> >>>> changing name brings any benefit? Or am I missing something here?
> >>>> It's simply used by userspace so the hardware should be loading
> >>>> regardless of the name...
> >>> Users see the name of their devices in their settings menu. They
> >>> should be accurate. Also, the early entry needs to be added anyway to
> >>> prevent kicking devices.
> >> If it's just aesthetics I don't see much reasons in changing the name.
> >>
> >> "the early entry needs to be added anyway ...." has no meaning to me,
> >> please rephrase. Sorry.
> >
> > Early exit-
> >
> >>>> Along with IP and your tool and asusctl there is also openrgb,
> >>>> and a newborn application for asus devices (I don't have contacts
> >>>> with that dev nor I remember the name of the tool)
> >>>> and I am not even that sure these are all asus-related
> >>>> applications.
> >>> My tool never checked for names, it briefly did for around a month
> >>> after its creation for some devices until capability matches. Around
> >>> 6.1 and 6.7 the kernel changed the names of most USB devices and that
> >>> caused issues. It currently only uses name matches for VID/PID 0/0
> >>> devices created by the kernel. Specifically, WMI and AT Keyboards. I
> >>> am not sure there is a workaround for those. Asusctl also does not use
> >>> names either.
> >> But IP does, so I would like to hear confirmation from at least Derek
> >> before the merge that there won't be future issues.
> >>
> >> Interpret what I say here as a broad topic, not just name/PER_APP flag:
> >> avoid changing data flow on older models...
> >
> > In [1] Derek removes the name matches
> >
> > There are no other name matches concerning this driver in it.
> >
> > The data flow is not changed in this series; you should go through the
> > patches once again if you think that. The only difference is 0x5e is
> > not sent, and 0x5d is not sent for newer devices.
> >
> > [1] https://github.com/ShadowBlip/InputPlumber/pull/453
> >
> >>>> Excercise EXTRA care touching this area as these are enough changes
> >>>> to make it difficult to understand what exactly is the problem if
> >>>> someone shows up with LEDs malfunctioning, laptop not entering sleep
> >>>> anymore or something else entirely. Plus over time
> >>>> ASUS has used various workarounds for windows problems
> >>>> and I am not eager to find out what those are since there is only
> >>>> a realistic way it's going to happen....
> >>> These changes are not doing something extraordinary. It's just a minor cleanup.
> >>>
> >>>>> Moreover, that specific commit is needed for Xbox Ally devices anyway,
> >>>>> as the kernel kicks one of the RGB endpoints because it does not
> >>>>> register an input device (the check skipped by early return) so
> >>>>> userspace becomes unable to control RGB on a stock kernel
> >>>>> (hidraw/hiddev nodes are gone). For more context here, this specific
> >>>>> endpoint implements the RGB Lamparray protocol for Windows dynamic
> >>>>> lighting, and I think in an attempt to make it work properly in
> >>>>> Windows, Asus made it so Windows has to first disable dynamic lighting
> >>>>> for armoury crate RGB commands to work (the 0x5a ones over the 0xff31
> >>>>> hid page).
> >>>> Yes once ASUS introduces something new it sticks with that for
> >>>> future models so it's expected more and more laptops will have
> >>>> the same problem: I am not questioning if these patches are needed
> >>>> as they very clearly are; I am questioning if everything that these
> >>>> patches are doing are worth doing and aren't breaking/regressing
> >>>> either tools or the flow of actions between the EC and these USB devices.
> >>> Well, this series is needed to account for that. Sending the disable
> >>> command is out of scope for now though.
> >> Here I apologize for confusion: my comments were mostly about
> >> older models: I absolutely don't want to break those, but if you find a way
> >> to distinguish them from newer models that would give you more freedom with those.
> >
> > Yes, we know their specific PIDs, so if you see the patch that adds
> > the 0x5d init, it is only added for those models.
>
> I’m only half keeping up to date on this. I do recall however that the 0x5D init was definitely required for the first ASUS laptop I worked on, and old GX502 - the PID for keyboard is 0x1866 and I think that was the last of that generation MCU. All the previous MCU also required it.

You recall if it was needed to enable the RGB commands or was it only
for keyboard shortcuts? If it is needed for keyboard shortcuts it is
correct for it to stay. If RGB does not turn on where it has been
enabled before, it should also stay.

> I saw some messages in perhaps another thread where it was mentioned that 0x5E init should be removed? That I agreed with that?
> I know there are AniMe and Slash versions that use that init, and they are on the same MCU as the keyboard. I had expected that just one init (on 0x5A or whatever) would work but it doesn’t - what I don’t recall is if an incomplete init affected the keyboard features.

Well, the way these devices work is that there are three hiddev
devices, usually nested within the same hid endpoint under up to three
collections. Each has one report ID. 0x5a is for brightness controls,
0x5d is for RGB, and 0x5e is for anime. For the first two, I know the
usages are 76 and 79 (see above). I am not sure what the usage for
anime is because I do not have a hid descriptor for that device.

In order to start talking to one of the hiddev devices, you are
supposed to start with an init. The init is bidirectional, so after
reading it back software knows it is talking to an Asus device (as it
is done in this series). Likewise, even though it is not the case for
all MCUs, the MCUs themselves can use it to verify they are talking to
an Asus application (as you said) and reject commands if it is not
sent.

For this reason, I think it is a good idea before asusctl starts
controlling RGB, to always start with a 0x5d init to verify it is
talking to an Asus device. And before Anime, with a 0x5e init (if the
specific application for it is available). So since Dennis you are the
new maintainer, you should try to work that in. Sending it twice does
not hurt, even if not ideal.

Similarly, because this driver does not do Anime currently, there is
no reason for it to send 0x5e. It also does not do RGB, so there is no
reason to send 0x5D (unless not sending it causes issues). For the RGB
patch I did, I delayed the init purposely until userspace interacts
with the sysfs RGB endpoint, partly to interfere with userspace
software less as well. So if the user does not use the sysfs RGB e.g.
asusctl is completely unaffected.

> In all reality unless the full set of init is causing issues it’s best to leave them in. If it is then I guess this driver is going to become a little more complex and have a few more quirks.
>
> Unfortunately I didn’t keep good records of my findings on this so it’s just my remembered observations that you’ll have to take at my word.
>
> It would be a good idea for you both to perhaps collaborate with Sergei from ghelper, he has put a huge amount of effort in to that tool and due to it being windows he gets a hell of a lot more use and bug reports/data than this driver does. There’s no shame in looking to others for inspiration, ideas, or guidance.

Good idea. From a quick look, indeed slash/anime is 0x5e. We could
interact with him more in the future.

Although looking into it, to find the correct endpoint he does a dirty
check for report length being more than 128[1]. SIgh

I think it would be productive to try to merge this for 6.19. So
Dennis can you try to be a bit more cooperative?

I already have 6 more patches for duo keyboards. Although the keyboard
brightness button on those seems to not work (?)[2]. I am waiting on a
reply for that. Perhaps it uses a slightly different ID code. However,
it seems that brightness works even when disconnecting and connecting
the keyboard. Which is great.

Antheas

[1] https://github.com/seerge/g-helper/blob/610b11749b4da97346012e5d47f0a9bbc93b94af/app/AnimeMatrix/Communication/Platform/WindowsUsbProvider.cs#L37
[2] https://github.com/bazzite-org/kernel-bazzite/issues/35

> Cheers,
> Luke.
>
> >
> >> No disable commands unless we find hard evidence those are strictly needed.
> >
> > They are needed for the Xbox Ally series, but since this driver does
> > not do RGB it is out of scope.
> >
> >>> Antheas
> >>>
> >>>>> Hopefully this clears things up
> >>>>>
> >>>>> Antheas
> >>>>>
> >>>>>>> Unrelated but I was b4ing this series on Ubuntu 24 and got BADSIG:
> >>>>>>> DKIM/antheas.dev. Is there a reference for fixing this on my host?
> >>>>>>> Perhaps it would help with spam
> >>>>>> I see BADSIG very often these days from b4 (thanks to gmail expiring
> >>>>>> things after 7 days or so, I recall hearing somewhere), I just ignore them
> >>>>>> entirely.
> >>>>>>
> >>>>>> AFAIK, that has never caused any delay to any patch in pdx86 domain so if
> >>>>>> that is what you thought is happening here, it's not the case.
> >>>>>> If your patch does appear in the pdx86 patchwork, there's even less reason
> >>>>>> to worry as I mostly pick patches to process using patchwork's list.
> >>>>> Turns out I had to update my DNS records. It should be good now.
> >>>>>
> >>>>>> --
> >>>>>> i.
> >>>> snipp
> >>>>>>>> 2.51.2
>
>
>

Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling

[luke]

> On 14 Nov 2025, at 10:17, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> 
> On Thu, 13 Nov 2025 at 21:23, <luke@ljones.dev> wrote:
>> 
>> 
>>> On 13 Nov 2025, at 21:44, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>>> 
>>> On Thu, 13 Nov 2025 at 02:14, Denis Benato <benato.denis96@gmail.com> wrote:
>>>> 
>>>> 
>>>> On 11/12/25 23:08, Antheas Kapenekakis wrote:
>>>>> On Wed, 12 Nov 2025 at 20:51, Denis Benato <benato.denis96@gmail.com> wrote:
>>>>>> 
>>>>>> On 11/12/25 14:41, Antheas Kapenekakis wrote:
>>>>>>> On Wed, 12 Nov 2025 at 14:22, Ilpo Järvinen
>>>>>>> <ilpo.jarvinen@linux.intel.com> wrote:
>>>>>>>> On Wed, 12 Nov 2025, Antheas Kapenekakis wrote:
>>>>>>>> 
>>>>>>>>> On Sat, 1 Nov 2025 at 11:47, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>>>>>>>>>> This is a two part series which does the following:
>>>>>>>>>> - Clean-up init sequence
>>>>>>>>>> - Unify 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.
>>>>>>>>>> 
>>>>>>>>>> For more context, see cover letter of V1. Since V5, I removed some patches
>>>>>>>>>> to make this easier to merge.
>>>>>>>>> Small bump for this.
>>>>>>>> I looked at v8 earlier but then got an impression some of Denis' comments
>>>>>>>> against v7 were not taken into account in v8, which implies there will be
>>>>>>>> delay until I've time to delve into the details (I need to understand
>>>>>>>> things pretty deeply in such a case, which does take lots of time).
>>>>>>>> 
>>>>>>>> Alternatively, if Denis says v8 is acceptable, then I don't need to spend
>>>>>>>> so much time on it, but somehow I've a feeling he isn't happy with v8
>>>>>>>> but just hasn't voiced it again...
>>>>>>>> 
>>>>>>>> Please do realize that ignoring reviewer feedback without a very very good
>>>>>>>> reason always risks adding delay or friction into getting things
>>>>>>>> upstreamed. Especially, when the review comes from a person who has been
>>>>>>>> around for me to learn to trust their reviews or from a maintainer of the
>>>>>>>> code in question.
>>>>>>> Sure, sorry if it came out this way. Dennis had two comments on the V7
>>>>>>> version of the series.
>>>>>>> 
>>>>>>> The first one was that asusctl has a hang bug, which he hasn't had
>>>>>>> time to look into yet. This should have been fixed by dropping the
>>>>>>> HID_QUIRK_INPUT_PER_APP. I retested the series and that QUIRK was a
>>>>>>> bit of a NOOP that does not need to be added in the future.
>>>>>> So it is supposed to not regress it now, correct?
>>>>>>> The second is he is concerned with dropping the 0x5d/0x5e inits. Luke
>>>>>>> said (back in March) that it is fine to drop 0x5e because it is only
>>>>>>> used for ANIME displays. However, for 0x5d, it is hard to verify some
>>>>>>> of the older laptops because they have only been tested with 0x5d and
>>>>>>> we do not have the hardware in question to test.
>>>>>>> 
>>>>>>> For this series, I re-added "initialize LED endpoint early for old
>>>>>>> NKEY keyboards" that re-adds 0x5d for the keyboards that cannot be
>>>>>>> tested again so this comment should be resolved too. With that in
>>>>>>> mind, we do end up with an additional quirk/command that may be
>>>>>>> unneeded and will remain there forever, but since it was a point of
>>>>>>> contention, it is not worth arguing over.
>>>>>>> 
>>>>>>> So both comments should be resolved
>>>>>> The driver should also not late-initialize anything.
>>>>>> 
>>>>>> Windows doesn't do it and the official asus application
>>>>>> can freely send LEDs changing commands to either WMI or USB
>>>>>> so I don't see any reason to do things differently [than windows]
>>>>>> and not prepare every USB endpoint to receive commands,
>>>>>> this has not been addressed unless I confused v7 and v8?
>>>>> Yes, it's been added on v8. 0x5d is init for the laptops it is
>>>>> problematic for. Not because it does not work, but because it has not
>>>>> been verified to work for those laptops.
>>>> I am not sure I am reading this right:
>>>> are you telling me that on recent models the windows driver
>>>> doesn't issue 0x5d?
>>> 
>>> Try to add spaces in your replies. This is hard to follow.
>>> 
>>> Do not conflate driver with software. 0x5a (over the application
>>> 0xff310076) has traditionally been used by a driver in Windows to
>>> control the backlight level, as it is done in this driver. 0x5d (over
>>> the application 0xff310079) is only used by laptops with RGB by
>>> Armoury crate. But this driver does not do RGB. No device
>>> functionality relies on it being sent for any device I've seen. The
>>> device remembers its Windows settings, incl. the backlight color, in
>>> the absence of a driver.
>>> 
>>> Laptops without RGB such as the Duo series which I would like to add
>>> support for next only have a 0x5a endpoint. But, they are sent garbage
>>> inits for 0x5d and 0x5e currently. This should be fixed.
>>> 
>>> Moreso, it seems that Armoury crate on the Xbox Ally series uses
>>> exclusively 0x5a commands and if you use 0x5d it ignores them (perhaps
>>> RGB still works though). With the previous generation, commands worked
>>> for either report id.
>>> 
>>>>>>> @Denis: can give an ack if this is the case?
>>>>>>> 
>>>>>>> As for Derek's comment, he has a PR for his project where he removes
>>>>>>> the name match for Ally devices with ample time for it to be merged
>>>>>>> until kernel 6.19 is released. In addition, that specific software for
>>>>>>> full functionality relies on OOT drivers on the distros it ships with,
>>>>>>> so it is minimally affected in either case.
>>>>>> The part we are talking about depends on this driver (hid-asus)
>>>>>> and there are people on asus-linux community using inputplumber
>>>>>> for non-ally devices (the OOT driver is only for ally devices)
>>>>>> therefore it is very important to us (and various other distributions)
>>>>>> not to break that software in any way.
>>>>> This driver is only used for Ally devices. If you mean that people
>>>>> remap their keyboards using inputplumber I guess they could but I have
>>>>> not seen it.
>>>> I meant people remap keyboards using IP. I am sure there were
>>>> (and very probably still are) people doing that.
>>>>>> Weighting pros and cons of changing the name I am not sure
>>>>>> changing name brings any benefit? Or am I missing something here?
>>>>>> It's simply used by userspace so the hardware should be loading
>>>>>> regardless of the name...
>>>>> Users see the name of their devices in their settings menu. They
>>>>> should be accurate. Also, the early entry needs to be added anyway to
>>>>> prevent kicking devices.
>>>> If it's just aesthetics I don't see much reasons in changing the name.
>>>> 
>>>> "the early entry needs to be added anyway ...." has no meaning to me,
>>>> please rephrase. Sorry.
>>> 
>>> Early exit-
>>> 
>>>>>> Along with IP and your tool and asusctl there is also openrgb,
>>>>>> and a newborn application for asus devices (I don't have contacts
>>>>>> with that dev nor I remember the name of the tool)
>>>>>> and I am not even that sure these are all asus-related
>>>>>> applications.
>>>>> My tool never checked for names, it briefly did for around a month
>>>>> after its creation for some devices until capability matches. Around
>>>>> 6.1 and 6.7 the kernel changed the names of most USB devices and that
>>>>> caused issues. It currently only uses name matches for VID/PID 0/0
>>>>> devices created by the kernel. Specifically, WMI and AT Keyboards. I
>>>>> am not sure there is a workaround for those. Asusctl also does not use
>>>>> names either.
>>>> But IP does, so I would like to hear confirmation from at least Derek
>>>> before the merge that there won't be future issues.
>>>> 
>>>> Interpret what I say here as a broad topic, not just name/PER_APP flag:
>>>> avoid changing data flow on older models...
>>> 
>>> In [1] Derek removes the name matches
>>> 
>>> There are no other name matches concerning this driver in it.
>>> 
>>> The data flow is not changed in this series; you should go through the
>>> patches once again if you think that. The only difference is 0x5e is
>>> not sent, and 0x5d is not sent for newer devices.
>>> 
>>> [1] https://github.com/ShadowBlip/InputPlumber/pull/453
>>> 
>>>>>> Excercise EXTRA care touching this area as these are enough changes
>>>>>> to make it difficult to understand what exactly is the problem if
>>>>>> someone shows up with LEDs malfunctioning, laptop not entering sleep
>>>>>> anymore or something else entirely. Plus over time
>>>>>> ASUS has used various workarounds for windows problems
>>>>>> and I am not eager to find out what those are since there is only
>>>>>> a realistic way it's going to happen....
>>>>> These changes are not doing something extraordinary. It's just a minor cleanup.
>>>>> 
>>>>>>> Moreover, that specific commit is needed for Xbox Ally devices anyway,
>>>>>>> as the kernel kicks one of the RGB endpoints because it does not
>>>>>>> register an input device (the check skipped by early return) so
>>>>>>> userspace becomes unable to control RGB on a stock kernel
>>>>>>> (hidraw/hiddev nodes are gone). For more context here, this specific
>>>>>>> endpoint implements the RGB Lamparray protocol for Windows dynamic
>>>>>>> lighting, and I think in an attempt to make it work properly in
>>>>>>> Windows, Asus made it so Windows has to first disable dynamic lighting
>>>>>>> for armoury crate RGB commands to work (the 0x5a ones over the 0xff31
>>>>>>> hid page).

Ah… this was an annoyance. In the hid-asus driver I did I ended up defaulting the LED control to the lamp-array style because it enabled faster per-led control. Then on sleep/resume it applied static colour matching first LED to keep some consistency. It works fine and there is no noticeable delay between switching to/from since the LampArray commands are instant (no set/apply required).

Anyhow, that driver created proper new LED device just for LED control. But it could only do that by taking the Ally HID off of the current driver and managing the whole lot. The end result I thought was much cleaner and separated the actual endpoints out to specific functions instead of how the current driver takes *all* the endpoints and tries to work off usage pages or report ID only.

For example use endpoint 0x83 for configuration (of gamepad) and LED. 0x87 is typically keyboard press events etc.

It did make me wonder if a newer cleaner driver for new MCU 0x19b6 onwards would have worked better instead of trying to shoehorn stuff in to the current driver constantly. It’s dead easy to bring up a new driver for this as an experiment. Maybe both you and Denis could investigate doing so?


>>>>>> Yes once ASUS introduces something new it sticks with that for
>>>>>> future models so it's expected more and more laptops will have
>>>>>> the same problem: I am not questioning if these patches are needed
>>>>>> as they very clearly are; I am questioning if everything that these
>>>>>> patches are doing are worth doing and aren't breaking/regressing
>>>>>> either tools or the flow of actions between the EC and these USB devices.
>>>>> Well, this series is needed to account for that. Sending the disable
>>>>> command is out of scope for now though.
>>>> Here I apologize for confusion: my comments were mostly about
>>>> older models: I absolutely don't want to break those, but if you find a way
>>>> to distinguish them from newer models that would give you more freedom with those.
>>> 
>>> Yes, we know their specific PIDs, so if you see the patch that adds
>>> the 0x5d init, it is only added for those models.
>> 
>> I’m only half keeping up to date on this. I do recall however that the 0x5D init was definitely required for the first ASUS laptop I worked on, and old GX502 - the PID for keyboard is 0x1866 and I think that was the last of that generation MCU. All the previous MCU also required it.
> 
> You recall if it was needed to enable the RGB commands or was it only
> for keyboard shortcuts? If it is needed for keyboard shortcuts it is
> correct for it to stay. If RGB does not turn on where it has been
> enabled before, it should also stay.

It was for shortcuts and the ROG buttons above the keyboard. There may have been some laptops using the same MCU that required it to enable LEDs too.

> 
>> I saw some messages in perhaps another thread where it was mentioned that 0x5E init should be removed? That I agreed with that?
>> I know there are AniMe and Slash versions that use that init, and they are on the same MCU as the keyboard. I had expected that just one init (on 0x5A or whatever) would work but it doesn’t - what I don’t recall is if an incomplete init affected the keyboard features.
> 
> Well, the way these devices work is that there are three hiddev
> devices, usually nested within the same hid endpoint under up to three
> collections. Each has one report ID. 0x5a is for brightness controls,
> 0x5d is for RGB, and 0x5e is for anime. For the first two, I know the
> usages are 76 and 79 (see above). I am not sure what the usage for
> anime is because I do not have a hid descriptor for that device.
> 
> In order to start talking to one of the hiddev devices, you are
> supposed to start with an init. The init is bidirectional, so after
> reading it back software knows it is talking to an Asus device (as it
> is done in this series). Likewise, even though it is not the case for
> all MCUs, the MCUs themselves can use it to verify they are talking to
> an Asus application (as you said) and reject commands if it is not
> sent.

Yes, I know. 
Before I stopped on all this I built up a rather large (untidy) collection of dumps for various things here https://gitlab.com/asus-linux/reverse-engineering/-/tree/master

> 
> For this reason, I think it is a good idea before asusctl starts
> controlling RGB, to always start with a 0x5d init to verify it is
> talking to an Asus device. And before Anime, with a 0x5e init (if the
> specific application for it is available). So since Dennis you are the
> new maintainer, you should try to work that in. Sending it twice does
> not hurt, even if not ideal.
> 
> Similarly, because this driver does not do Anime currently, there is
> no reason for it to send 0x5e. It also does not do RGB, so there is no
> reason to send 0x5D (unless not sending it causes issues). For the RGB
> patch I did, I delayed the init purposely until userspace interacts
> with the sysfs RGB endpoint, partly to interfere with userspace
> software less as well. So if the user does not use the sysfs RGB e.g.
> asusctl is completely unaffected.
> 
>> In all reality unless the full set of init is causing issues it’s best to leave them in. If it is then I guess this driver is going to become a little more complex and have a few more quirks.
>> 
>> Unfortunately I didn’t keep good records of my findings on this so it’s just my remembered observations that you’ll have to take at my word.
>> 
>> It would be a good idea for you both to perhaps collaborate with Sergei from ghelper, he has put a huge amount of effort in to that tool and due to it being windows he gets a hell of a lot more use and bug reports/data than this driver does. There’s no shame in looking to others for inspiration, ideas, or guidance.
> 
> Good idea. From a quick look, indeed slash/anime is 0x5e. We could
> interact with him more in the future.
> 
> Although looking into it, to find the correct endpoint he does a dirty
> check for report length being more than 128[1]. SIgh

Sergei would appreciate any friendly hints for sure. He’s a very nice guy.

> 
> I think it would be productive to try to merge this for 6.19. So
> Dennis can you try to be a bit more cooperative?

He’s not being intentionally recalcitrant. You both have vested interest in the outcomes of this review process and from what I’ve seen he has provided some excellent feedback.  If something isn’t going the way you want it doesn’t mean it’s personal. You will both converge on acceptable solutions through good faith and communication.

I’ll try to get a look in early on the next patch version and help a little if I can - it would be good to get this work in kernel and you both build off it.

> 
> I already have 6 more patches for duo keyboards. Although the keyboard
> brightness button on those seems to not work (?)[2]. I am waiting on a
> reply for that. Perhaps it uses a slightly different ID code. However,
> it seems that brightness works even when disconnecting and connecting
> the keyboard. Which is great.

Do the keys emit any codes? Maybe checking the raw output before it all gets filtered by the driver could help (like printing the raw array as hex) in asus_raw_event(). If there is nothing it could be emitting WMI events. ASUS did a dirty on some laptops and left the default WMI (I probably misremember, but ACPI event at least) in the ACPI, but made them emit nothing and used HID for brightness control instead.

That was this patch: https://github.com/torvalds/linux/commit/a720dee5e039238a44c0142dfccdc0e35c1125f7

Seems likely that because it appears to be a single button brightness cycle it could be a new code. In any case, debug printing the raw array as hex will show it if it’s being emitted.

While I remember, if you ever start playing with per-key RGB I mapped a lot of laptops https://gitlab.com/asus-linux/reverse-engineering/-/blob/master/keyboard/per_key_raw_bytes.ods?ref_type=heads - something to note is that each packet takes 1ms, but due to kernel internals it may attempt a burst of a few, or there could be up to 5ms delay. So a full sequence per row can be 8-20ms or more.

Oh, small reminder: if any patch changes dramatically from what I reviewed my tags should be removed.

> 
> Antheas
> 
> [1] https://github.com/seerge/g-helper/blob/610b11749b4da97346012e5d47f0a9bbc93b94af/app/AnimeMatrix/Communication/Platform/WindowsUsbProvider.cs#L37
> [2] https://github.com/bazzite-org/kernel-bazzite/issues/35
> 
>> Cheers,
>> Luke.
>> 
>>> 
>>>> No disable commands unless we find hard evidence those are strictly needed.
>>> 
>>> They are needed for the Xbox Ally series, but since this driver does
>>> not do RGB it is out of scope.
>>> 
>>>>> Antheas
>>>>> 
>>>>>>> Hopefully this clears things up
>>>>>>> 
>>>>>>> Antheas
>>>>>>> 
>>>>>>>>> Unrelated but I was b4ing this series on Ubuntu 24 and got BADSIG:
>>>>>>>>> DKIM/antheas.dev. Is there a reference for fixing this on my host?
>>>>>>>>> Perhaps it would help with spam
>>>>>>>> I see BADSIG very often these days from b4 (thanks to gmail expiring
>>>>>>>> things after 7 days or so, I recall hearing somewhere), I just ignore them
>>>>>>>> entirely.
>>>>>>>> 
>>>>>>>> AFAIK, that has never caused any delay to any patch in pdx86 domain so if
>>>>>>>> that is what you thought is happening here, it's not the case.
>>>>>>>> If your patch does appear in the pdx86 patchwork, there's even less reason
>>>>>>>> to worry as I mostly pick patches to process using patchwork's list.
>>>>>>> Turns out I had to update my DNS records. It should be good now.
>>>>>>> 
>>>>>>>> --
>>>>>>>> i.
>>>>>> snipp
>>>>>>>>>> 2.51.2
>> 
>> 
>> 
> 

Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling

[Antheas Kapenekakis]

On Thu, 13 Nov 2025 at 23:09, <luke@ljones.dev> wrote:
>
>
>
> > On 14 Nov 2025, at 10:17, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >
> > On Thu, 13 Nov 2025 at 21:23, <luke@ljones.dev> wrote:
> >>
> >>
> >>> On 13 Nov 2025, at 21:44, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >>>
> >>> On Thu, 13 Nov 2025 at 02:14, Denis Benato <benato.denis96@gmail.com> wrote:
> >>>>
> >>>>
> >>>> On 11/12/25 23:08, Antheas Kapenekakis wrote:
> >>>>> On Wed, 12 Nov 2025 at 20:51, Denis Benato <benato.denis96@gmail.com> wrote:
> >>>>>>
> >>>>>> On 11/12/25 14:41, Antheas Kapenekakis wrote:
> >>>>>>> On Wed, 12 Nov 2025 at 14:22, Ilpo Järvinen
> >>>>>>> <ilpo.jarvinen@linux.intel.com> wrote:
> >>>>>>>> On Wed, 12 Nov 2025, Antheas Kapenekakis wrote:
> >>>>>>>>
> >>>>>>>>> On Sat, 1 Nov 2025 at 11:47, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >>>>>>>>>> This is a two part series which does the following:
> >>>>>>>>>> - Clean-up init sequence
> >>>>>>>>>> - Unify 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.
> >>>>>>>>>>
> >>>>>>>>>> For more context, see cover letter of V1. Since V5, I removed some patches
> >>>>>>>>>> to make this easier to merge.
> >>>>>>>>> Small bump for this.
> >>>>>>>> I looked at v8 earlier but then got an impression some of Denis' comments
> >>>>>>>> against v7 were not taken into account in v8, which implies there will be
> >>>>>>>> delay until I've time to delve into the details (I need to understand
> >>>>>>>> things pretty deeply in such a case, which does take lots of time).
> >>>>>>>>
> >>>>>>>> Alternatively, if Denis says v8 is acceptable, then I don't need to spend
> >>>>>>>> so much time on it, but somehow I've a feeling he isn't happy with v8
> >>>>>>>> but just hasn't voiced it again...
> >>>>>>>>
> >>>>>>>> Please do realize that ignoring reviewer feedback without a very very good
> >>>>>>>> reason always risks adding delay or friction into getting things
> >>>>>>>> upstreamed. Especially, when the review comes from a person who has been
> >>>>>>>> around for me to learn to trust their reviews or from a maintainer of the
> >>>>>>>> code in question.
> >>>>>>> Sure, sorry if it came out this way. Dennis had two comments on the V7
> >>>>>>> version of the series.
> >>>>>>>
> >>>>>>> The first one was that asusctl has a hang bug, which he hasn't had
> >>>>>>> time to look into yet. This should have been fixed by dropping the
> >>>>>>> HID_QUIRK_INPUT_PER_APP. I retested the series and that QUIRK was a
> >>>>>>> bit of a NOOP that does not need to be added in the future.
> >>>>>> So it is supposed to not regress it now, correct?
> >>>>>>> The second is he is concerned with dropping the 0x5d/0x5e inits. Luke
> >>>>>>> said (back in March) that it is fine to drop 0x5e because it is only
> >>>>>>> used for ANIME displays. However, for 0x5d, it is hard to verify some
> >>>>>>> of the older laptops because they have only been tested with 0x5d and
> >>>>>>> we do not have the hardware in question to test.
> >>>>>>>
> >>>>>>> For this series, I re-added "initialize LED endpoint early for old
> >>>>>>> NKEY keyboards" that re-adds 0x5d for the keyboards that cannot be
> >>>>>>> tested again so this comment should be resolved too. With that in
> >>>>>>> mind, we do end up with an additional quirk/command that may be
> >>>>>>> unneeded and will remain there forever, but since it was a point of
> >>>>>>> contention, it is not worth arguing over.
> >>>>>>>
> >>>>>>> So both comments should be resolved
> >>>>>> The driver should also not late-initialize anything.
> >>>>>>
> >>>>>> Windows doesn't do it and the official asus application
> >>>>>> can freely send LEDs changing commands to either WMI or USB
> >>>>>> so I don't see any reason to do things differently [than windows]
> >>>>>> and not prepare every USB endpoint to receive commands,
> >>>>>> this has not been addressed unless I confused v7 and v8?
> >>>>> Yes, it's been added on v8. 0x5d is init for the laptops it is
> >>>>> problematic for. Not because it does not work, but because it has not
> >>>>> been verified to work for those laptops.
> >>>> I am not sure I am reading this right:
> >>>> are you telling me that on recent models the windows driver
> >>>> doesn't issue 0x5d?
> >>>
> >>> Try to add spaces in your replies. This is hard to follow.
> >>>
> >>> Do not conflate driver with software. 0x5a (over the application
> >>> 0xff310076) has traditionally been used by a driver in Windows to
> >>> control the backlight level, as it is done in this driver. 0x5d (over
> >>> the application 0xff310079) is only used by laptops with RGB by
> >>> Armoury crate. But this driver does not do RGB. No device
> >>> functionality relies on it being sent for any device I've seen. The
> >>> device remembers its Windows settings, incl. the backlight color, in
> >>> the absence of a driver.
> >>>
> >>> Laptops without RGB such as the Duo series which I would like to add
> >>> support for next only have a 0x5a endpoint. But, they are sent garbage
> >>> inits for 0x5d and 0x5e currently. This should be fixed.
> >>>
> >>> Moreso, it seems that Armoury crate on the Xbox Ally series uses
> >>> exclusively 0x5a commands and if you use 0x5d it ignores them (perhaps
> >>> RGB still works though). With the previous generation, commands worked
> >>> for either report id.
> >>>
> >>>>>>> @Denis: can give an ack if this is the case?
> >>>>>>>
> >>>>>>> As for Derek's comment, he has a PR for his project where he removes
> >>>>>>> the name match for Ally devices with ample time for it to be merged
> >>>>>>> until kernel 6.19 is released. In addition, that specific software for
> >>>>>>> full functionality relies on OOT drivers on the distros it ships with,
> >>>>>>> so it is minimally affected in either case.
> >>>>>> The part we are talking about depends on this driver (hid-asus)
> >>>>>> and there are people on asus-linux community using inputplumber
> >>>>>> for non-ally devices (the OOT driver is only for ally devices)
> >>>>>> therefore it is very important to us (and various other distributions)
> >>>>>> not to break that software in any way.
> >>>>> This driver is only used for Ally devices. If you mean that people
> >>>>> remap their keyboards using inputplumber I guess they could but I have
> >>>>> not seen it.
> >>>> I meant people remap keyboards using IP. I am sure there were
> >>>> (and very probably still are) people doing that.
> >>>>>> Weighting pros and cons of changing the name I am not sure
> >>>>>> changing name brings any benefit? Or am I missing something here?
> >>>>>> It's simply used by userspace so the hardware should be loading
> >>>>>> regardless of the name...
> >>>>> Users see the name of their devices in their settings menu. They
> >>>>> should be accurate. Also, the early entry needs to be added anyway to
> >>>>> prevent kicking devices.
> >>>> If it's just aesthetics I don't see much reasons in changing the name.
> >>>>
> >>>> "the early entry needs to be added anyway ...." has no meaning to me,
> >>>> please rephrase. Sorry.
> >>>
> >>> Early exit-
> >>>
> >>>>>> Along with IP and your tool and asusctl there is also openrgb,
> >>>>>> and a newborn application for asus devices (I don't have contacts
> >>>>>> with that dev nor I remember the name of the tool)
> >>>>>> and I am not even that sure these are all asus-related
> >>>>>> applications.
> >>>>> My tool never checked for names, it briefly did for around a month
> >>>>> after its creation for some devices until capability matches. Around
> >>>>> 6.1 and 6.7 the kernel changed the names of most USB devices and that
> >>>>> caused issues. It currently only uses name matches for VID/PID 0/0
> >>>>> devices created by the kernel. Specifically, WMI and AT Keyboards. I
> >>>>> am not sure there is a workaround for those. Asusctl also does not use
> >>>>> names either.
> >>>> But IP does, so I would like to hear confirmation from at least Derek
> >>>> before the merge that there won't be future issues.
> >>>>
> >>>> Interpret what I say here as a broad topic, not just name/PER_APP flag:
> >>>> avoid changing data flow on older models...
> >>>
> >>> In [1] Derek removes the name matches
> >>>
> >>> There are no other name matches concerning this driver in it.
> >>>
> >>> The data flow is not changed in this series; you should go through the
> >>> patches once again if you think that. The only difference is 0x5e is
> >>> not sent, and 0x5d is not sent for newer devices.
> >>>
> >>> [1] https://github.com/ShadowBlip/InputPlumber/pull/453
> >>>
> >>>>>> Excercise EXTRA care touching this area as these are enough changes
> >>>>>> to make it difficult to understand what exactly is the problem if
> >>>>>> someone shows up with LEDs malfunctioning, laptop not entering sleep
> >>>>>> anymore or something else entirely. Plus over time
> >>>>>> ASUS has used various workarounds for windows problems
> >>>>>> and I am not eager to find out what those are since there is only
> >>>>>> a realistic way it's going to happen....
> >>>>> These changes are not doing something extraordinary. It's just a minor cleanup.
> >>>>>
> >>>>>>> Moreover, that specific commit is needed for Xbox Ally devices anyway,
> >>>>>>> as the kernel kicks one of the RGB endpoints because it does not
> >>>>>>> register an input device (the check skipped by early return) so
> >>>>>>> userspace becomes unable to control RGB on a stock kernel
> >>>>>>> (hidraw/hiddev nodes are gone). For more context here, this specific
> >>>>>>> endpoint implements the RGB Lamparray protocol for Windows dynamic
> >>>>>>> lighting, and I think in an attempt to make it work properly in
> >>>>>>> Windows, Asus made it so Windows has to first disable dynamic lighting
> >>>>>>> for armoury crate RGB commands to work (the 0x5a ones over the 0xff31
> >>>>>>> hid page).
>
> Ah… this was an annoyance. In the hid-asus driver I did I ended up defaulting the LED control to the lamp-array style because it enabled faster per-led control. Then on sleep/resume it applied static colour matching first LED to keep some consistency. It works fine and there is no noticeable delay between switching to/from since the LampArray commands are instant (no set/apply required).
>
> Anyhow, that driver created proper new LED device just for LED control. But it could only do that by taking the Ally HID off of the current driver and managing the whole lot. The end result I thought was much cleaner and separated the actual endpoints out to specific functions instead of how the current driver takes *all* the endpoints and tries to work off usage pages or report ID only.
>
> For example use endpoint 0x83 for configuration (of gamepad) and LED. 0x87 is typically keyboard press events etc.
>
> It did make me wonder if a newer cleaner driver for new MCU 0x19b6 onwards would have worked better instead of trying to shoehorn stuff in to the current driver constantly. It’s dead easy to bring up a new driver for this as an experiment. Maybe both you and Denis could investigate doing so?

Yeah, this driver is not ideal for the Ally devices. They are not
keyboards. At least for now we can make sure that endpoint is not
ejected so that userspace programs work.

At this point its a party, disable this do that, then flip around for
the RGB to work.

I barely have time to cleanup and upstream the TDP stuff, less so
having the luxury of doing the controller in kernel as well.

>
> >>>>>> Yes once ASUS introduces something new it sticks with that for
> >>>>>> future models so it's expected more and more laptops will have
> >>>>>> the same problem: I am not questioning if these patches are needed
> >>>>>> as they very clearly are; I am questioning if everything that these
> >>>>>> patches are doing are worth doing and aren't breaking/regressing
> >>>>>> either tools or the flow of actions between the EC and these USB devices.
> >>>>> Well, this series is needed to account for that. Sending the disable
> >>>>> command is out of scope for now though.
> >>>> Here I apologize for confusion: my comments were mostly about
> >>>> older models: I absolutely don't want to break those, but if you find a way
> >>>> to distinguish them from newer models that would give you more freedom with those.
> >>>
> >>> Yes, we know their specific PIDs, so if you see the patch that adds
> >>> the 0x5d init, it is only added for those models.
> >>
> >> I’m only half keeping up to date on this. I do recall however that the 0x5D init was definitely required for the first ASUS laptop I worked on, and old GX502 - the PID for keyboard is 0x1866 and I think that was the last of that generation MCU. All the previous MCU also required it.
> >
> > You recall if it was needed to enable the RGB commands or was it only
> > for keyboard shortcuts? If it is needed for keyboard shortcuts it is
> > correct for it to stay. If RGB does not turn on where it has been
> > enabled before, it should also stay.
>
> It was for shortcuts and the ROG buttons above the keyboard. There may have been some laptops using the same MCU that required it to enable LEDs too.

Ok, so we should keep it for those then. This series keeps it for those.

> >
> >> I saw some messages in perhaps another thread where it was mentioned that 0x5E init should be removed? That I agreed with that?
> >> I know there are AniMe and Slash versions that use that init, and they are on the same MCU as the keyboard. I had expected that just one init (on 0x5A or whatever) would work but it doesn’t - what I don’t recall is if an incomplete init affected the keyboard features.
> >
> > Well, the way these devices work is that there are three hiddev
> > devices, usually nested within the same hid endpoint under up to three
> > collections. Each has one report ID. 0x5a is for brightness controls,
> > 0x5d is for RGB, and 0x5e is for anime. For the first two, I know the
> > usages are 76 and 79 (see above). I am not sure what the usage for
> > anime is because I do not have a hid descriptor for that device.
> >
> > In order to start talking to one of the hiddev devices, you are
> > supposed to start with an init. The init is bidirectional, so after
> > reading it back software knows it is talking to an Asus device (as it
> > is done in this series). Likewise, even though it is not the case for
> > all MCUs, the MCUs themselves can use it to verify they are talking to
> > an Asus application (as you said) and reject commands if it is not
> > sent.
>
> Yes, I know.
> Before I stopped on all this I built up a rather large (untidy) collection of dumps for various things here https://gitlab.com/asus-linux/reverse-engineering/-/tree/master
>
> >
> > For this reason, I think it is a good idea before asusctl starts
> > controlling RGB, to always start with a 0x5d init to verify it is
> > talking to an Asus device. And before Anime, with a 0x5e init (if the
> > specific application for it is available). So since Dennis you are the
> > new maintainer, you should try to work that in. Sending it twice does
> > not hurt, even if not ideal.
> >
> > Similarly, because this driver does not do Anime currently, there is
> > no reason for it to send 0x5e. It also does not do RGB, so there is no
> > reason to send 0x5D (unless not sending it causes issues). For the RGB
> > patch I did, I delayed the init purposely until userspace interacts
> > with the sysfs RGB endpoint, partly to interfere with userspace
> > software less as well. So if the user does not use the sysfs RGB e.g.
> > asusctl is completely unaffected.
> >
> >> In all reality unless the full set of init is causing issues it’s best to leave them in. If it is then I guess this driver is going to become a little more complex and have a few more quirks.
> >>
> >> Unfortunately I didn’t keep good records of my findings on this so it’s just my remembered observations that you’ll have to take at my word.
> >>
> >> It would be a good idea for you both to perhaps collaborate with Sergei from ghelper, he has put a huge amount of effort in to that tool and due to it being windows he gets a hell of a lot more use and bug reports/data than this driver does. There’s no shame in looking to others for inspiration, ideas, or guidance.
> >
> > Good idea. From a quick look, indeed slash/anime is 0x5e. We could
> > interact with him more in the future.
> >
> > Although looking into it, to find the correct endpoint he does a dirty
> > check for report length being more than 128[1]. SIgh
>
> Sergei would appreciate any friendly hints for sure. He’s a very nice guy.
>
> >
> > I think it would be productive to try to merge this for 6.19. So
> > Dennis can you try to be a bit more cooperative?
>
> He’s not being intentionally recalcitrant. You both have vested interest in the outcomes of this review process and from what I’ve seen he has provided some excellent feedback.  If something isn’t going the way you want it doesn’t mean it’s personal. You will both converge on acceptable solutions through good faith and communication.
>
> I’ll try to get a look in early on the next patch version and help a little if I can - it would be good to get this work in kernel and you both build off it.

I do not have any comments to go through for a next revision yet. I
tried to fix Denis' comments in this revision, I have not received
something actionable from him for this revision yet.

> >
> > I already have 6 more patches for duo keyboards. Although the keyboard
> > brightness button on those seems to not work (?)[2]. I am waiting on a
> > reply for that. Perhaps it uses a slightly different ID code. However,
> > it seems that brightness works even when disconnecting and connecting
> > the keyboard. Which is great.
>
> Do the keys emit any codes? Maybe checking the raw output before it all gets filtered by the driver could help (like printing the raw array as hex) in asus_raw_event(). If there is nothing it could be emitting WMI events. ASUS did a dirty on some laptops and left the default WMI (I probably misremember, but ACPI event at least) in the ACPI, but made them emit nothing and used HID for brightness control instead.
>
> That was this patch: https://github.com/torvalds/linux/commit/a720dee5e039238a44c0142dfccdc0e35c1125f7

This series fixes that problem. It makes all HID and WMI RGB stuff go
under a single device under asus-wmi and be controlled together.
Solves a lot of problems. For the Z13, it has two devices for
brightness: lightbar and keyboard. The duo has a detacheable keyboard
that switches from USB to bluetooth. The devices on that patch have
both WMI and HID. And while you say that only one controls RGB, there
is probably something little like a lid light that is controlled by
WMI/HID and was missed by that patch. Also makes us not require a
quirk.

It also means that a WMI event would then control the USB backlight too.

> Seems likely that because it appears to be a single button brightness cycle it could be a new code. In any case, debug printing the raw array as hex will show it if it’s being emitted.

Yeah, its not an unmapped event because it did not show up in the log.
It is probably mapped to something. I am waiting for the user to tell
me the event.

> While I remember, if you ever start playing with per-key RGB I mapped a lot of laptops https://gitlab.com/asus-linux/reverse-engineering/-/blob/master/keyboard/per_key_raw_bytes.ods?ref_type=heads - something to note is that each packet takes 1ms, but due to kernel internals it may attempt a burst of a few, or there could be up to 5ms delay. So a full sequence per row can be 8-20ms or more.
>
> Oh, small reminder: if any patch changes dramatically from what I reviewed my tags should be removed.

I removed some reviewed-by when i rewrote the first part of the
series. The latter part is mostly unchanged, although perhaps after
switching to the work queue, there was a leftover reviewed-by that was
kept.

Antheas

> >
> > Antheas
> >
> > [1] https://github.com/seerge/g-helper/blob/610b11749b4da97346012e5d47f0a9bbc93b94af/app/AnimeMatrix/Communication/Platform/WindowsUsbProvider.cs#L37
> > [2] https://github.com/bazzite-org/kernel-bazzite/issues/35
> >
> >> Cheers,
> >> Luke.
> >>
> >>>
> >>>> No disable commands unless we find hard evidence those are strictly needed.
> >>>
> >>> They are needed for the Xbox Ally series, but since this driver does
> >>> not do RGB it is out of scope.
> >>>
> >>>>> Antheas
> >>>>>
> >>>>>>> Hopefully this clears things up
> >>>>>>>
> >>>>>>> Antheas
> >>>>>>>
> >>>>>>>>> Unrelated but I was b4ing this series on Ubuntu 24 and got BADSIG:
> >>>>>>>>> DKIM/antheas.dev. Is there a reference for fixing this on my host?
> >>>>>>>>> Perhaps it would help with spam
> >>>>>>>> I see BADSIG very often these days from b4 (thanks to gmail expiring
> >>>>>>>> things after 7 days or so, I recall hearing somewhere), I just ignore them
> >>>>>>>> entirely.
> >>>>>>>>
> >>>>>>>> AFAIK, that has never caused any delay to any patch in pdx86 domain so if
> >>>>>>>> that is what you thought is happening here, it's not the case.
> >>>>>>>> If your patch does appear in the pdx86 patchwork, there's even less reason
> >>>>>>>> to worry as I mostly pick patches to process using patchwork's list.
> >>>>>>> Turns out I had to update my DNS records. It should be good now.
> >>>>>>>
> >>>>>>>> --
> >>>>>>>> i.
> >>>>>> snipp
> >>>>>>>>>> 2.51.2
> >>
> >>
> >>
> >
>
>