[PATCH v5 0/2] HID: generic: add LampArray support via hid-lamparray helper

[PATCH v5 0/2] HID: generic: add LampArray support via hid-lamparray helper

[Aaron Erhardt]

Add a new hid-lamparray helper module and integrate it with the
hid-generic driver.

While more complex lamparray handling should be done in userspace via
hidraw, providing a small module to add basic lamparray support makes it
possible for userspace software to interact with lamparrays by simply
using well-known APIs of the LED subsystem. One use-case would be to
enable desktop environments to support keyboard backlight control out of
the box for HID lamparray devices without having to implement the whole
HID protocol themselves.

This patch is based on previous discussions:
https://lore.kernel.org/all/1fb08a74-62c7-4d0c-ba5d-648e23082dcb@tuxedocomputers.com/

The helper provides basic support for devices exposing a
Lighting/LampArray application collection (usage page 0x59) and
registers a single-zone RGB LED representation via the LED
subsystem.

hid-generic now checks for LampArray support after hid_parse() and
optionally registers a lamparray instance. Failures in the helper
do not abort device probe to keep the driver logic otherwise unchanged.

LampArray resources are released on driver remove.

This commit was successfully tested on the Microsoft MacroPad reference
implementation (https://github.com/microsoft/RP2040MacropadHidSample
1d6c3ad) and in combination with the tuxedo_nb04_wmi driver, albeit
only fully functional with a recent fix posted to the LKML
(https://lore.kernel.org/all/20260826081149.235487-2-aer@tuxedocomputers.com).

v5:
- Proper hardware detection (no quirks necessary anymore)
- Add documentation for new sysfs knob
- Pass limits of the device to sysfs (intesities & brightness)
- More flexible Kconfig (use tristate)
- Improved locking
- Several memory leak and (de-)initialization fixes
- Don't read current color values from hardware (the HID spec does not
  offer this option)
- Remove redundant report dump functionality
v4:
- Restrict CONFIG_HID_LAMPARRAY to built-in configurations only to fix
  additional randconfig build errors
v3:
- Squash V1 and V2 into one patch
v2:
- Fix Kconfig to avoid build errors when LEDS_CLASS_MULTICOLOR is
  disabled

Aaron Erhardt (2):
  HID: lamparray: add new LampArray helper module
  HID: generic: add LampArray support via hid-lamparray helper

 .../ABI/testing/sysfs-driver-hid-lamparray    |  16 +
 drivers/hid/Kconfig                           |  18 +
 drivers/hid/Makefile                          |   2 +
 drivers/hid/hid-generic.c                     |  38 +
 drivers/hid/hid-lamparray.c                   | 812 ++++++++++++++++++
 include/linux/hid-lamparray.h                 |  88 ++
 6 files changed, 974 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-lamparray
 create mode 100644 drivers/hid/hid-lamparray.c
 create mode 100644 include/linux/hid-lamparray.h

-- 
2.43.0

[PATCH v5 1/2] HID: lamparray: add new LampArray helper module

[Aaron Erhardt]

Add a new hid-lamparray helper module that provides basic support for
devices exposing a Lighting/LampArray application collection (usage
page 0x59) and registers a single-zone RGB LED representation via the
LED subsystem.

The module can be used as a library in HID drivers to add support for
the HID LampArray protocol. While the API is quite basic as of now
it could be extended in the future.

Co-developed-by: Tim Guttzeit <tgu@tuxedocomputers.com>
Signed-off-by: Tim Guttzeit <tgu@tuxedocomputers.com>
Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
---
 .../ABI/testing/sysfs-driver-hid-lamparray    |  16 +
 drivers/hid/Kconfig                           |  17 +
 drivers/hid/Makefile                          |   2 +
 drivers/hid/hid-lamparray.c                   | 812 ++++++++++++++++++
 include/linux/hid-lamparray.h                 |  88 ++
 5 files changed, 935 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-lamparray
 create mode 100644 drivers/hid/hid-lamparray.c
 create mode 100644 include/linux/hid-lamparray.h

diff --git a/Documentation/ABI/testing/sysfs-driver-hid-lamparray b/Documentation/ABI/testing/sysfs-driver-hid-lamparray
new file mode 100644
index 000000000000..795be6c4c368
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-hid-lamparray
@@ -0,0 +1,16 @@
+What:		/sys/bus/hid/devices/<bus>:<vid>:<pid>.<n>/use_leds_uapi
+Date:		August 2026
+KernelVersion:	7.3
+Contact:	aer@tuxedocomputers.com
+Description:
+        If a driver uses the hid-lamparray module and a device supporting
+        LampArray is found, one multicolor LED class device is registered under
+        /sys/class/leds/rgb:<function> to expose the single-zone RGB control.
+        Every device gets an incremental unique id.
+
+        Additionally, the use_leds_uapi sysfs attribute to control the LED class
+        device is attached directly to the HID device at
+        /sys/bus/hid/devices/<bus>:<vid>:<pid>.<n>/use_leds_uapi. Writing 0 to
+        use_leds_uapi unregisters the LED class device. The last state is kept
+        cached. Writing 1 registers it again and restores the cached state to
+        hardware.
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index aa7fa11a0197..4afd80a67b39 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -92,6 +92,23 @@ config HID_GENERIC
 
 	If unsure, say Y.
 
+config HID_LAMPARRAY
+	tristate "HID LampArray helper"
+	depends on HID
+	depends on LEDS_CLASS_MULTICOLOR
+	default n
+	help
+	  Helper for HID devices exposing a Lighting/LampArray collection.
+	  Treats LampArray devices as a single-zone device and exposes a sysfs
+	  interface for changing color and intensity values. Also exposes a
+	  sysfs flag to be disabled e.g. by a userspace driver.
+
+	  This can be used as library in existing drivers. The generic HID
+	  driver is extended by default to handle lamp array devices if this
+	  option is enabled.
+
+	  If unsure, say N.
+
 config HID_HAPTIC
 	bool "Haptic touchpad support"
 	default n
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 48a863b245ee..f95630fa8bd8 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -13,6 +13,8 @@ obj-$(CONFIG_UHID)		+= uhid.o
 
 obj-$(CONFIG_HID_GENERIC)	+= hid-generic.o
 
+obj-$(CONFIG_HID_LAMPARRAY)	+= hid-lamparray.o
+
 hid-$(CONFIG_HIDRAW)		+= hidraw.o
 
 hid-logitech-y		:= hid-lg.o
diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
new file mode 100644
index 000000000000..9a438aa2d305
--- /dev/null
+++ b/drivers/hid/hid-lamparray.c
@@ -0,0 +1,812 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * hid-lamparray.c - HID LampArray helper module (single-zone RGB)
+ *
+ * Helper module for HID drivers supporting devices that expose a Lighting and
+ * Illumination (LampArray) application collection (usage page 0x59).
+ *
+ * The module provides a minimal integration with the LED subsystem and treats
+ * the device as a single zone: all lamps share one RGB value and a global
+ * brightness level. It does not implement multi-zone layouts or hardware
+ * effects.
+ *
+ * If enabled and a device supporting LampArray is found, one multicolor LED
+ * class device is registered under /sys/class/leds/<HID-ID>:rgb:LampArray to
+ * expose the single-zone RGB control.
+ *
+ * The use_leds_uapi sysfs attribute is attached directly to the HID device
+ * under /sys/bus/hid/devices/<HID-ID>/use_leds_uapi. Writing 0 to use_leds_uapi
+ * unregisters the LED class device. The last state is kept cached. Writing 1
+ * registers it again and restores the cached state to hardware. State is cached
+ * as last known RGB + brightness.
+ *
+ * The module does not bind to devices on its own. Instead, a HID driver may
+ * query support via lamparray_is_supported_device() after hid_parse() and
+ * create an instance using lamparray_register().
+ *
+ * Copyright (C) 2026 Tim Guttzeit <tgu@tuxedocomputers.com>
+ * Copyright (C) 2026 Aaron Erhardt <aer@tuxedocomputers.com>
+ */
+
+#include <dt-bindings/leds/common.h>
+#include <linux/limits.h>
+#include <linux/minmax.h>
+#include <linux/hid.h>
+#include <linux/leds.h>
+#include <linux/sysfs.h>
+#include <linux/hid-lamparray.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/container_of.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/xarray.h>
+
+/* Constants */
+
+/* HID usages (LampArray, etc.) */
+#define HID_LIGHTING_ILLUMINATION_USAGE_PAGE	0x0059
+
+/* HID usage types */
+#define HID_APPLICATION_COLLECTION_USAGE_TYPE	0x0001
+#define HID_LAMPARRAY_ATTRIBUTES_REPORT	0x0002
+#define HID_LAMP_ATTRIBUTES_RESPONSE_REPORT	0x0022
+#define HID_LAMP_RANGE_UPDATE_REPORT		0x0060
+#define HID_LAMPARRAY_CONTROL_REPORT		0x0070
+
+/* HID attributes */
+#define HID_LAIP_LAMP_COUNT			0x0003
+#define HID_LAIP_LAMPARRAY_KIND			0x0007
+#define HID_LAIP_RED_LEVEL_COUNT		0x0028
+#define HID_LAIP_GREEN_LEVEL_COUNT		0x0029
+#define HID_LAIP_BLUE_LEVEL_COUNT		0x002a
+#define HID_LAIP_INTENSITY_LEVEL_COUNT		0x002b
+#define HID_LAIP_RED_UPDATE_CHANNEL		0x0051
+#define HID_LAIP_GREEN_UPDATE_CHANNEL		0x0052
+#define HID_LAIP_BLUE_UPDATE_CHANNEL		0x0053
+#define HID_LAIP_INTENSITY_UPDATE_CHANNEL	0x0054
+#define HID_LAIP_LAMP_ID_START			0x0061
+#define HID_LAIP_LAMP_ID_END			0x0062
+#define HID_LAIP_AUTONOMOUS_MODE		0x0071
+
+/* LampArrayKind values */
+#define HID_LAMPARRAY_KIND_KEYBOARD		0x0001
+
+/* Helper struct for fields and their indices */
+struct hid_field_value {
+	struct hid_field *field;
+	int index;
+};
+
+/* Helper struct for color fields */
+struct lamparray_color_fields {
+	struct hid_field_value red;
+	struct hid_field_value green;
+	struct hid_field_value blue;
+	struct hid_field_value intensity;
+};
+
+/* Device state */
+struct lamparray_device {
+	struct hid_device *hdev;
+
+	struct lamparray_color_fields color_levels;
+	struct lamparray_color_fields color_update;
+
+	struct hid_field_value autonomous_field;
+	struct hid_field_value range_start;
+	struct hid_field_value range_end;
+	struct hid_field_value lamp_count;
+	struct hid_field_value lamparray_kind;
+
+	u16 lamp_count_value;
+	u32 lamparray_kind_value;
+
+	struct led_classdev_mc mc_cdev;
+	struct mc_subled subleds[3];
+
+	struct mutex dev_lock; /* Protects cached state and HID access */
+	struct mutex sysfs_lock; /* Protects sysfs LED (de-)initialization */
+
+	u8 max_r;
+	u8 max_g;
+	u8 max_b;
+	u8 max_brightness;
+
+	u8 last_r;
+	u8 last_g;
+	u8 last_b;
+	u8 last_brightness;
+
+	bool use_leds_uapi;
+	bool led_registered;
+};
+
+/*
+ * Opaque handle exposed to callers via the header.
+ * Keep the actual state in lamparray_device, but return a stable pointer.
+ */
+struct lamparray {
+	struct lamparray_device ldev;
+};
+
+/*
+ * Mapping for hid_device pointers to their lamparray data.
+ * Since there is not guarantee of how the driver using this library
+ * will use its drvdata, the only safe way to retrieve the lamparray
+ * data from a HID device pointer is using this mapping.
+ */
+static DEFINE_XARRAY(lamparray_by_hdev);
+
+/* HID helper functions */
+
+static int get_field_value(struct hid_field_value *field_value)
+{
+	return field_value->field->value[field_value->index];
+}
+
+static u8 get_field_value_as_u8(struct hid_field_value *field_value)
+{
+	return clamp_val(get_field_value(field_value), 0, U8_MAX);
+}
+
+static void set_field_value(struct hid_field_value *field_value, int value)
+{
+	field_value->field->value[field_value->index] = value;
+}
+
+static bool lamparray_color_fields_is_complete(struct lamparray_color_fields *color_fields)
+{
+	return color_fields->red.field && color_fields->green.field &&
+	       color_fields->blue.field && color_fields->intensity.field;
+}
+
+static int lamparray_read_attributes_report(struct lamparray_device *ldev)
+{
+	struct hid_device *hdev = ldev->hdev;
+	struct hid_report *report;
+
+	if (!ldev->lamp_count.field) {
+		hid_dbg(hdev, "No LampCount field found\n");
+		return -ENODEV;
+	}
+
+	if (!ldev->lamparray_kind.field) {
+		hid_dbg(hdev, "No LampArrayKind field found\n");
+		return -ENODEV;
+	}
+
+	report = ldev->lamp_count.field->report;
+
+	if (!report) {
+		hid_dbg(hdev, "LampCount field has no report\n");
+		return -ENODEV;
+	}
+
+	mutex_lock(&ldev->dev_lock);
+
+	/* Update values */
+	hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
+	hid_hw_wait(hdev);
+
+	ldev->lamp_count_value = get_field_value(&ldev->lamp_count);
+
+	if (ldev->lamp_count_value == 0) {
+		mutex_unlock(&ldev->dev_lock);
+		hid_dbg(hdev, "LampCount is %d (invalid)\n", ldev->lamp_count_value);
+		return -EINVAL;
+	}
+
+	ldev->lamparray_kind_value = get_field_value(&ldev->lamparray_kind);
+
+	mutex_unlock(&ldev->dev_lock);
+
+	return 0;
+}
+
+static int lamparray_parse_update_report(struct lamparray_device *ldev)
+{
+	struct hid_device *hdev = ldev->hdev;
+	struct hid_report_enum *re;
+	struct hid_report *report;
+	struct hid_field *field;
+	int i, j;
+	int ret = 0;
+
+	mutex_lock(&ldev->dev_lock);
+
+	re = &hdev->report_enum[HID_FEATURE_REPORT];
+
+	list_for_each_entry(report, &re->report_list, list) {
+		for (i = 0; i < report->maxfield; i++) {
+			field = report->field[i];
+			if (!field)
+				continue;
+
+			if (!field->usage || !field->maxusage)
+				continue;
+
+			for (j = 0; j < field->maxusage; j++) {
+				u32 usage = field->usage[j].hid;
+				u32 collection_idx = field->usage[j].collection_index;
+				u32 collection_usage = hdev->collection[collection_idx].usage;
+
+				u16 page = (usage & HID_USAGE_PAGE) >> 16;
+				u16 id = usage & HID_USAGE;
+				u16 collection_usage_id = collection_usage & U16_MAX;
+
+				if (page != HID_LIGHTING_ILLUMINATION_USAGE_PAGE)
+					continue;
+
+				if (collection_usage_id == HID_LAMPARRAY_ATTRIBUTES_REPORT) {
+					switch (id) {
+					case HID_LAIP_LAMP_COUNT:
+						ldev->lamp_count.field = field;
+						ldev->lamp_count.index = j;
+						break;
+					case HID_LAIP_LAMPARRAY_KIND:
+						ldev->lamparray_kind.field = field;
+						ldev->lamparray_kind.index = j;
+						break;
+					}
+				} else if (collection_usage_id ==
+					   HID_LAMP_ATTRIBUTES_RESPONSE_REPORT) {
+					switch (id) {
+					case HID_LAIP_RED_LEVEL_COUNT:
+						ldev->color_levels.red.field = field;
+						ldev->color_levels.red.index = j;
+						break;
+					case HID_LAIP_GREEN_LEVEL_COUNT:
+						ldev->color_levels.green.field = field;
+						ldev->color_levels.green.index = j;
+						break;
+					case HID_LAIP_BLUE_LEVEL_COUNT:
+						ldev->color_levels.blue.field = field;
+						ldev->color_levels.blue.index = j;
+						break;
+					case HID_LAIP_INTENSITY_LEVEL_COUNT:
+						ldev->color_levels.intensity.field = field;
+						ldev->color_levels.intensity.index = j;
+						break;
+					}
+				} else if (collection_usage_id == HID_LAMP_RANGE_UPDATE_REPORT) {
+					switch (id) {
+					case HID_LAIP_RED_UPDATE_CHANNEL:
+						ldev->color_update.red.field = field;
+						ldev->color_update.red.index = j;
+						break;
+					case HID_LAIP_GREEN_UPDATE_CHANNEL:
+						ldev->color_update.green.field = field;
+						ldev->color_update.green.index = j;
+						break;
+					case HID_LAIP_BLUE_UPDATE_CHANNEL:
+						ldev->color_update.blue.field = field;
+						ldev->color_update.blue.index = j;
+						break;
+					case HID_LAIP_INTENSITY_UPDATE_CHANNEL:
+						ldev->color_update.intensity.field = field;
+						ldev->color_update.intensity.index = j;
+						break;
+					case HID_LAIP_LAMP_ID_START:
+						ldev->range_start.field = field;
+						ldev->range_start.index = j;
+						break;
+					case HID_LAIP_LAMP_ID_END:
+						ldev->range_end.field = field;
+						ldev->range_end.index = j;
+						break;
+					default:
+						break;
+					}
+				} else if (collection_usage_id == HID_LAMPARRAY_CONTROL_REPORT &&
+					   id == HID_LAIP_AUTONOMOUS_MODE) {
+					ldev->autonomous_field.field = field;
+					ldev->autonomous_field.index = j;
+				}
+			}
+		}
+	}
+
+	if (!ldev->autonomous_field.field ||
+	    !lamparray_color_fields_is_complete(&ldev->color_update))
+		ret = -ENODEV;
+
+	mutex_unlock(&ldev->dev_lock);
+
+	return ret;
+}
+
+static int lamparray_hw_set_autonomous(struct lamparray_device *ldev,
+				       bool enable)
+{
+	struct hid_device *hdev = ldev->hdev;
+	struct hid_field *field = ldev->autonomous_field.field;
+
+	if (!field)
+		return -ENODEV;
+
+	mutex_lock(&ldev->dev_lock);
+
+	set_field_value(&ldev->autonomous_field, !!enable);
+
+	hid_hw_request(hdev, field->report, HID_REQ_SET_REPORT);
+	hid_hw_wait(hdev);
+
+	mutex_unlock(&ldev->dev_lock);
+
+	return 0;
+}
+
+static int lamparray_hw_set_state(struct lamparray_device *ldev, u8 r, u8 g,
+				  u8 b, u8 intensity)
+{
+	struct hid_device *hdev = ldev->hdev;
+	struct hid_report *report;
+
+	if (!lamparray_color_fields_is_complete(&ldev->color_update))
+		return -ENODEV;
+
+	if (ldev->range_start.field && ldev->range_end.field) {
+		set_field_value(&ldev->range_start, 0);
+		set_field_value(&ldev->range_end, ldev->lamp_count_value - 1);
+	}
+
+	set_field_value(&ldev->color_update.red, r);
+	set_field_value(&ldev->color_update.green, g);
+	set_field_value(&ldev->color_update.blue, b);
+	set_field_value(&ldev->color_update.intensity, intensity);
+
+	report = ldev->color_update.red.field->report;
+	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
+	hid_hw_wait(hdev);
+
+	return 0;
+}
+
+/*
+ * Simple helper to read the color information of the first lamp.
+ * This does not read the state of the whole lamp array since this driver only
+ * exposes one LED anyway, so one color is sufficient here for now.
+ */
+static int lamparray_get_lamp_attributes(struct lamparray_device *ldev)
+{
+	struct hid_device *hdev = ldev->hdev;
+	struct hid_report *report;
+
+	if (!lamparray_color_fields_is_complete(&ldev->color_levels))
+		return -ENODEV;
+
+	/*
+	 * Get value of any lamp.
+	 */
+	report = ldev->color_levels.red.field->report;
+
+	mutex_lock(&ldev->dev_lock);
+
+	hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
+	hid_hw_wait(hdev);
+
+	ldev->max_r = get_field_value_as_u8(&ldev->color_levels.red);
+	ldev->max_g = get_field_value_as_u8(&ldev->color_levels.green);
+	ldev->max_b = get_field_value_as_u8(&ldev->color_levels.blue);
+	ldev->max_brightness = get_field_value_as_u8(&ldev->color_levels.intensity);
+
+	mutex_unlock(&ldev->dev_lock);
+
+	return 0;
+}
+
+/* Helper functions */
+
+static int lamparray_restore_state(struct lamparray_device *ldev)
+{
+	u8 r, g, b;
+	int ret;
+	enum led_brightness brightness;
+
+	mutex_lock(&ldev->dev_lock);
+
+	if (!ldev->use_leds_uapi) {
+		mutex_unlock(&ldev->dev_lock);
+		return 0;
+	}
+
+	r = ldev->last_r;
+	g = ldev->last_g;
+	b = ldev->last_b;
+	brightness = ldev->last_brightness;
+
+	ldev->mc_cdev.subled_info[0].intensity = r;
+	ldev->mc_cdev.subled_info[1].intensity = g;
+	ldev->mc_cdev.subled_info[2].intensity = b;
+	ldev->mc_cdev.led_cdev.brightness = brightness;
+
+	led_mc_calc_color_components(&ldev->mc_cdev, brightness);
+
+	ret = lamparray_hw_set_state(ldev, r, g, b, brightness);
+
+	mutex_unlock(&ldev->dev_lock);
+	return ret;
+}
+
+/* LEDs API */
+
+static int lamparray_led_brightness_set(struct led_classdev *cdev,
+					enum led_brightness brightness)
+{
+	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
+	struct lamparray_device *ldev =
+		container_of_const(mc, struct lamparray_device, mc_cdev);
+	u8 r, g, b;
+	int ret;
+
+	/*
+	 * Brightness is handled by the LampArray device if supported,
+	 * so we can pass the raw intensity values.
+	 */
+	r = mc->subled_info[0].intensity;
+	g = mc->subled_info[1].intensity;
+	b = mc->subled_info[2].intensity;
+
+	mc->led_cdev.brightness = brightness;
+	led_mc_calc_color_components(&ldev->mc_cdev, brightness);
+
+	mutex_lock(&ldev->dev_lock);
+	ret = lamparray_hw_set_state(ldev, r, g, b, brightness);
+	if (ret) {
+		mutex_unlock(&ldev->dev_lock);
+		hid_err(ldev->hdev, "Failed to send LampArray update: %d\n",
+			ret);
+		return ret;
+	}
+
+	ldev->last_r = r;
+	ldev->last_g = g;
+	ldev->last_b = b;
+	ldev->last_brightness = brightness;
+	mutex_unlock(&ldev->dev_lock);
+
+	return 0;
+}
+
+static enum led_brightness
+lamparray_led_brightness_get(struct led_classdev *cdev)
+{
+	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
+	struct lamparray_device *ldev =
+	    container_of_const(mc, struct lamparray_device, mc_cdev);
+
+	return ldev->last_brightness;
+}
+
+static int lamparray_register_led(struct lamparray_device *ldev)
+{
+	struct device *dev = &ldev->hdev->dev;
+	struct led_classdev *cdev = &ldev->mc_cdev.led_cdev;
+	int ret;
+
+	mutex_lock(&ldev->sysfs_lock);
+
+	if (ldev->led_registered) {
+		mutex_unlock(&ldev->sysfs_lock);
+		return 0;
+	}
+
+	if (!cdev->name) {
+		/* Fallback value */
+		const char *function = LED_FUNCTION_STATUS;
+
+		/* Some heuristics for choosing a better LED function. */
+		if (ldev->lamparray_kind_value == HID_LAMPARRAY_KIND_KEYBOARD)
+			function = LED_FUNCTION_KBD_BACKLIGHT;
+
+		cdev->name = kasprintf(GFP_KERNEL, "rgb:%s", function);
+		if (!cdev->name) {
+			mutex_unlock(&ldev->sysfs_lock);
+			return -ENOMEM;
+		}
+	}
+
+	mutex_lock(&ldev->dev_lock);
+	/* Setup */
+	cdev->max_brightness = ldev->max_brightness;
+	cdev->brightness_set_blocking = lamparray_led_brightness_set;
+	cdev->brightness_get = lamparray_led_brightness_get;
+	cdev->flags |= LED_RETAIN_AT_SHUTDOWN;
+
+	ldev->subleds[0].color_index = LED_COLOR_ID_RED;
+	ldev->subleds[0].max_intensity = ldev->max_r;
+	ldev->subleds[1].color_index = LED_COLOR_ID_GREEN;
+	ldev->subleds[1].max_intensity = ldev->max_g;
+	ldev->subleds[2].color_index = LED_COLOR_ID_BLUE;
+	ldev->subleds[2].max_intensity = ldev->max_b;
+
+	/* Set values */
+	ldev->subleds[0].intensity = ldev->last_r;
+	ldev->subleds[1].intensity = ldev->last_g;
+	ldev->subleds[2].intensity = ldev->last_b;
+	cdev->brightness = ldev->last_brightness;
+
+	ldev->mc_cdev.subled_info = ldev->subleds;
+	ldev->mc_cdev.num_colors = ARRAY_SIZE(ldev->subleds);
+
+	/* Ensure subled_info[].brightness matches intensity + brightness */
+	led_mc_calc_color_components(&ldev->mc_cdev, ldev->last_brightness);
+	mutex_unlock(&ldev->dev_lock);
+
+	ret = led_classdev_multicolor_register(dev, &ldev->mc_cdev);
+	if (ret) {
+		mutex_unlock(&ldev->sysfs_lock);
+		return ret;
+	}
+
+	ldev->led_registered = true;
+	mutex_unlock(&ldev->sysfs_lock);
+
+	return 0;
+}
+
+static void lamparray_unregister_led(struct lamparray_device *ldev)
+{
+	bool was_registered;
+	struct led_classdev *cdev = &ldev->mc_cdev.led_cdev;
+
+	mutex_lock(&ldev->sysfs_lock);
+	was_registered = ldev->led_registered;
+	ldev->led_registered = false;
+
+	if (was_registered)
+		led_classdev_multicolor_unregister(&ldev->mc_cdev);
+
+	kfree(cdev->name);
+	cdev->name = NULL;
+
+	mutex_unlock(&ldev->sysfs_lock);
+}
+
+/* Sysfs */
+
+static struct lamparray_device *
+lamparray_ldev_from_sysfs_dev(struct device *dev)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+
+	return xa_load(&lamparray_by_hdev, (unsigned long)hdev);
+}
+
+static ssize_t use_leds_uapi_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct lamparray_device *ldev = lamparray_ldev_from_sysfs_dev(dev);
+
+	if (!ldev)
+		return -ENODEV;
+
+	return sysfs_emit(buf, "%d\n", ldev->use_leds_uapi);
+}
+
+static ssize_t use_leds_uapi_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct lamparray_device *ldev = lamparray_ldev_from_sysfs_dev(dev);
+	int val;
+	int old_val;
+	int ret;
+
+	if (!ldev)
+		return -ENODEV;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	if (val != 0 && val != 1)
+		return -EINVAL;
+
+	mutex_lock(&ldev->dev_lock);
+	old_val = ldev->use_leds_uapi;
+
+	if (val == old_val) {
+		mutex_unlock(&ldev->dev_lock);
+		return count;
+	}
+
+	ldev->use_leds_uapi = val;
+	mutex_unlock(&ldev->dev_lock);
+
+	if (val == 1) {
+		ret = lamparray_register_led(ldev);
+		if (ret) {
+			mutex_lock(&ldev->dev_lock);
+			ldev->use_leds_uapi = old_val;
+			mutex_unlock(&ldev->dev_lock);
+			return ret;
+		}
+		ret = lamparray_restore_state(ldev);
+		if (ret) {
+			hid_err(ldev->hdev, "Could not restore state: %d\n", ret);
+			return ret;
+		}
+
+	} else {
+		lamparray_unregister_led(ldev);
+	}
+
+	return count;
+}
+static DEVICE_ATTR_RW(use_leds_uapi);
+
+static int lamparray_register_sysfs(struct lamparray_device *ldev)
+{
+	struct device *dev = &ldev->hdev->dev;
+	int ret;
+
+	ret = sysfs_create_file(&dev->kobj, &dev_attr_use_leds_uapi.attr);
+	if (ret)
+		hid_err(ldev->hdev,
+			"Failed to create lamparray sysfs group: %d\n", ret);
+
+	return ret;
+}
+
+static void lamparray_remove_sysfs(struct lamparray_device *ldev)
+{
+	sysfs_remove_file(&ldev->hdev->dev.kobj, &dev_attr_use_leds_uapi.attr);
+}
+
+/* Public API */
+
+bool lamparray_is_supported_device(struct hid_device *hdev)
+{
+	unsigned int i;
+
+	hid_dbg(hdev, "lamparray: walking %u collections\n",
+		hdev->maxcollection);
+
+	for (i = 0; i < hdev->maxcollection; i++) {
+		struct hid_collection *col = &hdev->collection[i];
+		u16 page = (col->usage & HID_USAGE_PAGE) >> 16;
+		u16 code = col->usage & HID_USAGE;
+
+		hid_dbg(hdev,
+			"lamparray:  collection[%u]: type=%u level=%u usage=0x%08x page=0x%04x code=0x%04x\n",
+			i, col->type, col->level, col->usage, page, code);
+
+		if (col->type == HID_COLLECTION_APPLICATION &&
+		    page == HID_LIGHTING_ILLUMINATION_USAGE_PAGE &&
+		    code == HID_APPLICATION_COLLECTION_USAGE_TYPE) {
+			return true;
+		}
+	}
+	return false;
+}
+EXPORT_SYMBOL_GPL(lamparray_is_supported_device);
+
+struct lamparray *
+lamparray_register(struct hid_device *hdev,
+		   const struct lamparray_init_state *led_init_state)
+{
+	int ret;
+	struct lamparray *la;
+	struct lamparray_device *ldev;
+
+	if (!hdev)
+		return ERR_PTR(-ENODEV);
+
+	la = kzalloc_obj(*la, GFP_KERNEL);
+	if (!la)
+		return ERR_PTR(-ENOMEM);
+
+	ldev = &la->ldev;
+
+	mutex_init(&ldev->dev_lock);
+	mutex_init(&ldev->sysfs_lock);
+	ldev->hdev = hdev;
+	ldev->use_leds_uapi = true;
+	ldev->led_registered = false;
+
+	/* Make sure the driver lock gets released for probing. */
+	hid_device_io_start(hdev);
+
+	ret = lamparray_parse_update_report(ldev);
+	if (ret) {
+		hid_err(hdev, "No LampArray update report found: %d\n", ret);
+		goto err_free;
+	}
+
+	ret = lamparray_read_attributes_report(ldev);
+	if (ret) {
+		hid_err(hdev,
+			"Could not determine LampCount: %d\n",
+			ret);
+		goto err_free;
+	}
+
+	ret = lamparray_get_lamp_attributes(ldev);
+	if (ret) {
+		hid_err(hdev,
+			"Faulty device. Could not query lamp attributes.\n");
+		goto err_free;
+	}
+
+	/* Use black (all zeros) as default. */
+	if (led_init_state) {
+		ldev->last_r = min(led_init_state->r, ldev->max_r);
+		ldev->last_g = min(led_init_state->g, ldev->max_g);
+		ldev->last_b = min(led_init_state->b, ldev->max_b);
+		ldev->last_brightness = min(led_init_state->brightness,
+					    ldev->max_brightness);
+	}
+
+	ret = lamparray_register_led(ldev);
+	if (ret) {
+		hid_warn(hdev, "Failed to register LED UAPI: %d\n", ret);
+		ldev->use_leds_uapi = false;
+	}
+
+	ret = xa_err(xa_store(&lamparray_by_hdev, (unsigned long)hdev, ldev,
+			      GFP_KERNEL));
+	if (ret)
+		goto err_unregister_led;
+
+	ret = lamparray_register_sysfs(ldev);
+	if (ret)
+		goto err_xa_erase;
+
+	ret = lamparray_hw_set_autonomous(ldev, false);
+	if (ret) {
+		hid_err(hdev, "Could not disable autonomous mode: %d", ret);
+		goto err_remove_sysfs;
+	}
+
+	hid_info(hdev, "LampArray device registered\n");
+
+	ret = lamparray_restore_state(ldev);
+	if (ret) {
+		hid_err(hdev, "Failed to set default state: %d", ret);
+		goto err_remove_sysfs;
+	}
+
+	hid_device_io_stop(hdev);
+	return la;
+
+err_remove_sysfs:
+	lamparray_remove_sysfs(ldev);
+err_xa_erase:
+	xa_erase(&lamparray_by_hdev, (unsigned long)hdev);
+err_unregister_led:
+	lamparray_unregister_led(ldev);
+err_free:
+	hid_device_io_stop(hdev);
+	mutex_destroy(&ldev->dev_lock);
+	mutex_destroy(&ldev->sysfs_lock);
+	kfree(la);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(lamparray_register);
+
+void lamparray_unregister(struct lamparray *la)
+{
+	struct lamparray_device *ldev;
+
+	if (!la)
+		return;
+
+	ldev = &la->ldev;
+
+	lamparray_hw_set_autonomous(ldev, true);
+
+	lamparray_remove_sysfs(ldev);
+	xa_erase(&lamparray_by_hdev, (unsigned long)ldev->hdev);
+	lamparray_unregister_led(ldev);
+
+	mutex_destroy(&ldev->dev_lock);
+	mutex_destroy(&ldev->sysfs_lock);
+	kfree(la);
+}
+EXPORT_SYMBOL_GPL(lamparray_unregister);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Tim Guttzeit <tgu@tuxedocomputers.com>");
+MODULE_AUTHOR("Aaron Erhardt <aer@tuxedocomputers.com>");
+MODULE_DESCRIPTION("HID LampArray helper module (single-zone RGB)");
diff --git a/include/linux/hid-lamparray.h b/include/linux/hid-lamparray.h
new file mode 100644
index 000000000000..a77869728d12
--- /dev/null
+++ b/include/linux/hid-lamparray.h
@@ -0,0 +1,88 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef _HID_LAMPARRAY_H
+#define _HID_LAMPARRAY_H
+
+#include <linux/hid.h>
+#include <linux/err.h>
+#include <linux/types.h>
+
+struct lamparray;
+
+/*
+ * Optional initial LED state for lamparray_register().
+ * Used to define the initial state of a LampArray's LEDs.
+ */
+struct lamparray_init_state {
+	u8 r;
+	u8 g;
+	u8 b;
+	u8 brightness;
+};
+
+#if IS_ENABLED(CONFIG_HID_LAMPARRAY)
+
+/**
+ * lamparray_is_supported_device() - check whether a HID device supports LampArray
+ * @hdev: HID device to inspect
+ *
+ * Check whether the given HID device exposes a Lighting/LampArray application
+ * collection as defined by the HID Lighting specification.
+ *
+ * This helper can be used by HID drivers to determine whether LampArray
+ * functionality should be enabled for a device.
+ *
+ * Return: %true if LampArray support is detected, %false otherwise.
+ */
+bool lamparray_is_supported_device(struct hid_device *hdev);
+
+/**
+ * lamparray_register() - initialize LampArray support for a HID device
+ * @hdev: HID device
+ * @led_init_state: Optional LED state at init specification
+ *
+ * Allocate and initialize internal LampArray state for the given HID device.
+ * The function parses required HID reports and fields and registers the
+ * associated miscdevice and sysfs attributes.
+ *
+ * Registers a multicolor LED class device to expose the LampArray functionality
+ * via the LED subsystem. If specified, the desired initial LED state is
+ * applied. If led_init_state is NULL, a default state is applied (all LEDs off).
+ *
+ * Return: pointer to a LampArray handle on success, or ERR_PTR() on failure.
+ */
+struct lamparray *lamparray_register(struct hid_device *hdev,
+				     const struct lamparray_init_state *led_init_state);
+
+/**
+ * lamparray_unregister() - tear down LampArray support
+ * @la: LampArray handle returned by lamparray_register()
+ *
+ * Remove all resources associated with a LampArray instance.
+ *
+ * This unregisters the LED class device (if present), removes the miscdevice
+ * and sysfs interfaces and frees all internal state associated with @la.
+ */
+void lamparray_unregister(struct lamparray *la);
+
+#else /* !CONFIG_HID_LAMPARRAY */
+
+static inline bool lamparray_is_supported_device(struct hid_device *hdev)
+{
+	return false;
+}
+
+static inline struct lamparray *
+lamparray_register(struct hid_device *hdev,
+		   const struct lamparray_init_state *led_init_state)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline void lamparray_unregister(struct lamparray *la)
+{
+}
+
+#endif /* CONFIG_HID_LAMPARRAY */
+
+#endif /* _HID_LAMPARRAY_H */
-- 
2.43.0

[PATCH v5 2/2] HID: generic: add LampArray support via hid-lamparray helper

[Aaron Erhardt]

The hid-generic driver now checks for LampArray support after
hid_parse() and optionally registers a lamparray instance. Failures in
the helper do not abort device probe to keep the device unchanged.

LampArray resources are released on driver remove.

This patch was successfully tested on the Microsoft MacroPad reference
implementation (https://github.com/microsoft/RP2040MacropadHidSample
1d6c3ad) and in combination with the tuxedo_nb04_wmi driver, albeit only
functional with a recent fix posted to the LKML
(https://lore.kernel.org/all/20260728115918.125349-2-aer@tuxedocomputers.com).

Co-developed-by: Tim Guttzeit <tgu@tuxedocomputers.com>
Signed-off-by: Tim Guttzeit <tgu@tuxedocomputers.com>
Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
---
 drivers/hid/Kconfig       |  1 +
 drivers/hid/hid-generic.c | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 4afd80a67b39..a0fcd89c2bb0 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -82,6 +82,7 @@ config UHID
 
 config HID_GENERIC
 	tristate "Generic HID driver"
+	depends on HID_LAMPARRAY if HID_LAMPARRAY
 	default HID
 	help
 	Support for generic devices on the HID bus. This includes most
diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c
index c2de916747de..c3d2283198e8 100644
--- a/drivers/hid/hid-generic.c
+++ b/drivers/hid/hid-generic.c
@@ -20,6 +20,7 @@
 #include <asm/byteorder.h>
 
 #include <linux/hid.h>
+#include <linux/hid-lamparray.h>
 
 static struct hid_driver hid_generic;
 
@@ -60,6 +61,7 @@ static int hid_generic_probe(struct hid_device *hdev,
 			     const struct hid_device_id *id)
 {
 	int ret;
+	struct lamparray *la;
 
 	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
 
@@ -67,6 +69,31 @@ static int hid_generic_probe(struct hid_device *hdev,
 	if (ret)
 		return ret;
 
+	/*
+	 * Optional: attach LampArray support if present.
+	 * Never fail probe on LampArray errors; keep device functional.
+	 */
+	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && lamparray_is_supported_device(hdev)) {
+		/*
+		 * Use HID_CONNECT_DRIVER to claim driver to make sure
+		 * requests are processed. Needed for performing
+		 * hid_hw_request()/hid_hw_wait() to communicate with the
+		 * LampArray device.
+		 */
+		ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT | HID_CONNECT_DRIVER);
+		if (ret)
+			return ret;
+
+		la = lamparray_register(hdev, NULL);
+		if (IS_ERR(la)) {
+			hid_hw_stop(hdev);
+			hid_warn(hdev, "LampArray init failed: %ld\n", PTR_ERR(la));
+		} else {
+			hid_set_drvdata(hdev, la);
+			return 0;
+		}
+	}
+
 	return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 }
 
@@ -78,6 +105,16 @@ static int hid_generic_reset_resume(struct hid_device *hdev)
 	return 0;
 }
 
+static void hid_generic_remove(struct hid_device *hdev)
+{
+	struct lamparray *la = hid_get_drvdata(hdev);
+
+	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && la)
+		lamparray_unregister(la);
+
+	hid_hw_stop(hdev);
+}
+
 static const struct hid_device_id hid_table[] = {
 	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_ANY, HID_ANY_ID, HID_ANY_ID) },
 	{ }
@@ -90,6 +127,7 @@ static struct hid_driver hid_generic = {
 	.match = hid_generic_match,
 	.probe = hid_generic_probe,
 	.reset_resume = hid_generic_reset_resume,
+	.remove = hid_generic_remove,
 };
 module_hid_driver(hid_generic);
 
-- 
2.43.0

Re: [PATCH v5 2/2] HID: generic: add LampArray support via hid-lamparray helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The generic HID driver depends on HID_LAMPARRAY if HID_LAMPARRAY, which forces the core HID generic driver to be a module if the optional LampArray helper is a module.
--

commit de92bcafc34d337da4d9893ee22e49ef91520c6c
Author: Aaron Erhardt <aer@tuxedocomputers.com>

HID: generic: add LampArray support via hid-lamparray helper

The hid-generic driver now optionally registers a lamparray instance after
hid_parse(). LampArray resources are released on driver remove.

> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 4afd80a67b394..a0fcd89c2bb02 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -82,6 +82,7 @@ config UHID
>  
>  config HID_GENERIC
>  	tristate "Generic HID driver"
> +	depends on HID_LAMPARRAY if HID_LAMPARRAY

[Severity: High]
Does this restriction inadvertently force the core generic HID driver to be a
module when the optional LampArray helper is built as a module?

If a system relies on hid-generic for keyboard input during early boot
(e.g., to enter a LUKS decryption password or interact with a panic
shell) and expects the driver to be built-in, forcing it to compile
as a module could result in a total loss of early boot keyboard support.

Could this be avoided by dropping the "depends on" restriction and instead
using IS_REACHABLE(CONFIG_HID_LAMPARRAY) in both the header (for stubs)
and the C file? This would allow a built-in generic driver to gracefully
degrade to stubs while the optional helper remains a module.

>  	default HID
>  	help
>  	Support for generic devices on the HID bus. This includes most
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903073602.3815258-1-aer@tuxedocomputers.com?part=2

Re: [PATCH v5 1/2] HID: lamparray: add new LampArray helper module

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Division by zero during LampArray LED registration if the device reports zero intensity levels.
- [High] `lamparray_register` forcibly releases the HID core's `driver_input_lock` during driver probe.
- [Medium] Concurrent sysfs writes can cause the internal state flag (`use_leds_uapi`) to completely desynchronize from the actual LED registration state due to prematurely dropping `dev_lock`.
- [Medium] The hardcoded LED name violates the LED subsystem naming conventions and will cause a sysfs `-EEXIST` error when multiple identical devices are connected.
- [Medium] The driver assumes all color and range fields reside within the same HID report, leading to incomplete state updates if they are split.
- [Medium] Hardware is left in a non-autonomous state on the `lamparray_register` error path if `lamparray_restore_state` fails.
- [Medium] Lamp Range Update reports are defined as Output reports in the HID specification, but the driver only searches Feature reports.
- [Medium] Incorrect teardown ordering in `lamparray_unregister` allows concurrent hardware writes in autonomous mode.
- [Low] The driver prints a success log message during initialization, which explicitly violates the subsystem's logging rules.
--

commit f90eeb3c7fb481eba03a666b2a5bcb8dd92f3e9c
Author: Aaron Erhardt <aer@tuxedocomputers.com>
Subject: HID: lamparray: add new LampArray helper module

This commit adds a new hid-lamparray helper module for devices exposing a
Lighting/LampArray application collection (usage page 0x59). It registers a
single-zone RGB LED representation via the LED subsystem.

> diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
> new file mode 100644
> index 0000000000000..9a438aa2d305e
> --- /dev/null
> +++ b/drivers/hid/hid-lamparray.c

[ ... ]

> @@ -176,14 +176,12 @@ static int lamparray_parse_update_report(struct lamparray_device *ldev)
>  	int i, j;
>  	int ret = 0;
>  
>  	mutex_lock(&ldev->dev_lock);
>  
>  	re = &hdev->report_enum[HID_FEATURE_REPORT];
>  
>  	list_for_each_entry(report, &re->report_list, list) {
>  		for (i = 0; i < report->maxfield; i++) {

[Severity: Medium]
This restricts the search strictly to feature reports.

Since the HID specification defines Lamp Range Update reports as output
reports, does this mean the driver will fail to probe compliant devices
because it cannot find their output reports?

[ ... ]

> @@ -340,11 +340,9 @@ static int lamparray_hw_set_state(struct lamparray_device *ldev, u8 r, u8 g,
>  	set_field_value(&ldev->color_update.red, r);
>  	set_field_value(&ldev->color_update.green, g);
>  	set_field_value(&ldev->color_update.blue, b);
>  	set_field_value(&ldev->color_update.intensity, intensity);
>  
>  	report = ldev->color_update.red.field->report;
>  	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
>  	hid_hw_wait(hdev);

[Severity: Medium]
This hardware request is dispatched exclusively for the report containing
the 'red' field.

If a device descriptor splits the color channels across multiple report IDs,
wouldn't this leave the other colors unsent and result in incomplete state
updates?

[ ... ]

> @@ -386,10 +386,9 @@ static int lamparray_get_lamp_attributes(struct lamparray_device *ldev)
>  	hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
>  	hid_hw_wait(hdev);
>  
>  	ldev->max_r = get_field_value_as_u8(&ldev->color_levels.red);
>  	ldev->max_g = get_field_value_as_u8(&ldev->color_levels.green);
>  	ldev->max_b = get_field_value_as_u8(&ldev->color_levels.blue);
>  	ldev->max_brightness = get_field_value_as_u8(&ldev->color_levels.intensity);

[Severity: High]
This reads the maximum brightness directly from the device's GET_REPORT
response without validating it against zero.

Later, when led_mc_calc_color_components() is called to apply the brightness,
this max_brightness value acts as a divisor. If a device reports zero
intensity levels, does this trigger a kernel panic due to division by zero?

[ ... ]

> @@ -497,10 +497,8 @@ static int lamparray_register_led(struct lamparray_device *ldev)
>  		if (ldev->lamparray_kind_value == HID_LAMPARRAY_KIND_KEYBOARD)
>  			function = LED_FUNCTION_KBD_BACKLIGHT;
>  
>  		cdev->name = kasprintf(GFP_KERNEL, "rgb:%s", function);
>  		if (!cdev->name) {
>  			mutex_unlock(&ldev->sysfs_lock);
>  			return -ENOMEM;
>  		}

[Severity: Medium]
This hardcodes the LED name based solely on the function.

If multiple identical LampArray devices are connected, won't this cause a
deterministic sysfs naming collision (-EEXIST) that prevents the second
device's LED interface from registering?

[ ... ]

> @@ -608,13 +608,10 @@ static ssize_t use_leds_uapi_store(struct device *dev,
>  
>  	if (val == old_val) {
>  		mutex_unlock(&ldev->dev_lock);
>  		return count;
>  	}
>  
>  	ldev->use_leds_uapi = val;
>  	mutex_unlock(&ldev->dev_lock);
>  
>  	if (val == 1) {
>  		ret = lamparray_register_led(ldev);

[Severity: Medium]
The protective dev_lock is dropped here before executing the actual
registration side effects.

Does this leave a race window where concurrent sysfs writes could cause the
internal use_leds_uapi state to completely desynchronize from the actual LED
registration state?

[ ... ]

> @@ -706,10 +706,8 @@ struct lamparray *
>  	ldev->use_leds_uapi = true;
>  	ldev->led_registered = false;
>  
>  	/* Make sure the driver lock gets released for probing. */
>  	hid_device_io_start(hdev);

[Severity: High]
Calling hid_device_io_start() here forcibly releases the HID core's
driver_input_lock during the probe sequence.

Could this allow incoming hardware interrupt events to race with the
unfinished probe() initialization of the parent driver, potentially leading
to uninitialized memory accesses or a use-after-free?

[ ... ]

> @@ -772,16 +772,13 @@ struct lamparray *
>  	ret = lamparray_hw_set_autonomous(ldev, false);
>  	if (ret) {
>  		hid_err(hdev, "Could not disable autonomous mode: %d", ret);
>  		goto err_remove_sysfs;
>  	}
>  
>  	hid_info(hdev, "LampArray device registered\n");

[Severity: Low]
This prints a success log message during normal device initialization.
Should this be removed to comply with the subsystem's logging rules against
printing success messages?

>  
>  	ret = lamparray_restore_state(ldev);
>  	if (ret) {
>  		hid_err(hdev, "Failed to set default state: %d", ret);
>  		goto err_remove_sysfs;
>  	}

[Severity: Medium]
If lamparray_restore_state() fails, this error path routes to
err_remove_sysfs. However, this bypasses restoring the hardware back to
autonomous mode via lamparray_hw_set_autonomous().

Does this leave the hardware in a frozen, non-autonomous state if the
initial state restoration times out or fails?

[ ... ]

> @@ -794,11 +794,9 @@ void lamparray_unregister(struct lamparray *la)
>  	if (!la)
>  		return;
>  
>  	ldev = &la->ldev;
>  
>  	lamparray_hw_set_autonomous(ldev, true);
>  
>  	lamparray_remove_sysfs(ldev);
>  	xa_erase(&lamparray_by_hdev, (unsigned long)ldev->hdev);
>  	lamparray_unregister_led(ldev);

[Severity: Medium]
This sets the hardware to autonomous mode before unregistering the userspace
interfaces (the LED class device and sysfs attributes).

Could userspace write to the sysfs brightness attribute simultaneously while
the driver is being unbound, sending manual LED commands while the hardware
is configured to operate autonomously?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903073602.3815258-1-aer@tuxedocomputers.com?part=1

Re: [PATCH v5 1/2] HID: lamparray: add new LampArray helper module

[Werner Sembach]

Am 03.09.26 um 09:35 schrieb Aaron Erhardt:
> Add a new hid-lamparray helper module that provides basic support for
> devices exposing a Lighting/LampArray application collection (usage
> page 0x59) and registers a single-zone RGB LED representation via the
> LED subsystem.
>
> The module can be used as a library in HID drivers to add support for
> the HID LampArray protocol. While the API is quite basic as of now
> it could be extended in the future.
>
> Co-developed-by: Tim Guttzeit <tgu@tuxedocomputers.com>
> Signed-off-by: Tim Guttzeit <tgu@tuxedocomputers.com>
> Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
> ---
>   .../ABI/testing/sysfs-driver-hid-lamparray    |  16 +
>   drivers/hid/Kconfig                           |  17 +
>   drivers/hid/Makefile                          |   2 +
>   drivers/hid/hid-lamparray.c                   | 812 ++++++++++++++++++
>   include/linux/hid-lamparray.h                 |  88 ++
>   5 files changed, 935 insertions(+)
>   create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-lamparray
>   create mode 100644 drivers/hid/hid-lamparray.c
>   create mode 100644 include/linux/hid-lamparray.h
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-lamparray b/Documentation/ABI/testing/sysfs-driver-hid-lamparray
> new file mode 100644
> index 000000000000..795be6c4c368
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-hid-lamparray
> @@ -0,0 +1,16 @@
> +What:		/sys/bus/hid/devices/<bus>:<vid>:<pid>.<n>/use_leds_uapi
> +Date:		August 2026
> +KernelVersion:	7.3
> +Contact:	aer@tuxedocomputers.com
> +Description:
> +        If a driver uses the hid-lamparray module and a device supporting
> +        LampArray is found, one multicolor LED class device is registered under
> +        /sys/class/leds/rgb:<function> to expose the single-zone RGB control.
> +        Every device gets an incremental unique id.
> +
> +        Additionally, the use_leds_uapi sysfs attribute to control the LED class
> +        device is attached directly to the HID device at
> +        /sys/bus/hid/devices/<bus>:<vid>:<pid>.<n>/use_leds_uapi. Writing 0 to
> +        use_leds_uapi unregisters the LED class device. The last state is kept
> +        cached. Writing 1 registers it again and restores the cached state to
> +        hardware.
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index aa7fa11a0197..4afd80a67b39 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -92,6 +92,23 @@ config HID_GENERIC
>   
>   	If unsure, say Y.
>   
> +config HID_LAMPARRAY
> +	tristate "HID LampArray helper"
> +	depends on HID
> +	depends on LEDS_CLASS_MULTICOLOR
> +	default n
> +	help
> +	  Helper for HID devices exposing a Lighting/LampArray collection.
> +	  Treats LampArray devices as a single-zone device and exposes a sysfs
> +	  interface for changing color and intensity values. Also exposes a
> +	  sysfs flag to be disabled e.g. by a userspace driver.
> +
> +	  This can be used as library in existing drivers. The generic HID
> +	  driver is extended by default to handle lamp array devices if this
> +	  option is enabled.
> +
> +	  If unsure, say N.
> +
>   config HID_HAPTIC
>   	bool "Haptic touchpad support"
>   	default n
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 48a863b245ee..f95630fa8bd8 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -13,6 +13,8 @@ obj-$(CONFIG_UHID)		+= uhid.o
>   
>   obj-$(CONFIG_HID_GENERIC)	+= hid-generic.o
>   
> +obj-$(CONFIG_HID_LAMPARRAY)	+= hid-lamparray.o
> +
>   hid-$(CONFIG_HIDRAW)		+= hidraw.o
>   
>   hid-logitech-y		:= hid-lg.o
> diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
> new file mode 100644
> index 000000000000..9a438aa2d305
> --- /dev/null
> +++ b/drivers/hid/hid-lamparray.c
> @@ -0,0 +1,812 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * hid-lamparray.c - HID LampArray helper module (single-zone RGB)
> + *
> + * Helper module for HID drivers supporting devices that expose a Lighting and
> + * Illumination (LampArray) application collection (usage page 0x59).
> + *
> + * The module provides a minimal integration with the LED subsystem and treats
> + * the device as a single zone: all lamps share one RGB value and a global
> + * brightness level. It does not implement multi-zone layouts or hardware
> + * effects.
> + *
> + * If enabled and a device supporting LampArray is found, one multicolor LED
> + * class device is registered under /sys/class/leds/<HID-ID>:rgb:LampArray to
> + * expose the single-zone RGB control.
> + *
> + * The use_leds_uapi sysfs attribute is attached directly to the HID device
> + * under /sys/bus/hid/devices/<HID-ID>/use_leds_uapi. Writing 0 to use_leds_uapi
> + * unregisters the LED class device. The last state is kept cached. Writing 1
> + * registers it again and restores the cached state to hardware. State is cached
> + * as last known RGB + brightness.
> + *
> + * The module does not bind to devices on its own. Instead, a HID driver may
> + * query support via lamparray_is_supported_device() after hid_parse() and
> + * create an instance using lamparray_register().
> + *
> + * Copyright (C) 2026 Tim Guttzeit <tgu@tuxedocomputers.com>
> + * Copyright (C) 2026 Aaron Erhardt <aer@tuxedocomputers.com>
> + */
> +
> +#include <dt-bindings/leds/common.h>
> +#include <linux/limits.h>
> +#include <linux/minmax.h>
> +#include <linux/hid.h>
> +#include <linux/leds.h>
> +#include <linux/sysfs.h>
> +#include <linux/hid-lamparray.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/container_of.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/xarray.h>
> +
> +/* Constants */
> +
> +/* HID usages (LampArray, etc.) */
> +#define HID_LIGHTING_ILLUMINATION_USAGE_PAGE	0x0059
> +
> +/* HID usage types */
> +#define HID_APPLICATION_COLLECTION_USAGE_TYPE	0x0001
> +#define HID_LAMPARRAY_ATTRIBUTES_REPORT	0x0002
> +#define HID_LAMP_ATTRIBUTES_RESPONSE_REPORT	0x0022
> +#define HID_LAMP_RANGE_UPDATE_REPORT		0x0060
> +#define HID_LAMPARRAY_CONTROL_REPORT		0x0070
> +
> +/* HID attributes */
> +#define HID_LAIP_LAMP_COUNT			0x0003
> +#define HID_LAIP_LAMPARRAY_KIND			0x0007
> +#define HID_LAIP_RED_LEVEL_COUNT		0x0028
> +#define HID_LAIP_GREEN_LEVEL_COUNT		0x0029
> +#define HID_LAIP_BLUE_LEVEL_COUNT		0x002a
> +#define HID_LAIP_INTENSITY_LEVEL_COUNT		0x002b
> +#define HID_LAIP_RED_UPDATE_CHANNEL		0x0051
> +#define HID_LAIP_GREEN_UPDATE_CHANNEL		0x0052
> +#define HID_LAIP_BLUE_UPDATE_CHANNEL		0x0053
> +#define HID_LAIP_INTENSITY_UPDATE_CHANNEL	0x0054
> +#define HID_LAIP_LAMP_ID_START			0x0061
> +#define HID_LAIP_LAMP_ID_END			0x0062
> +#define HID_LAIP_AUTONOMOUS_MODE		0x0071
> +
> +/* LampArrayKind values */
> +#define HID_LAMPARRAY_KIND_KEYBOARD		0x0001
> +
> +/* Helper struct for fields and their indices */
> +struct hid_field_value {
> +	struct hid_field *field;
> +	int index;
> +};
> +
> +/* Helper struct for color fields */
> +struct lamparray_color_fields {
> +	struct hid_field_value red;
> +	struct hid_field_value green;
> +	struct hid_field_value blue;
> +	struct hid_field_value intensity;
> +};
> +
> +/* Device state */
> +struct lamparray_device {
> +	struct hid_device *hdev;
> +
> +	struct lamparray_color_fields color_levels;
> +	struct lamparray_color_fields color_update;
> +
> +	struct hid_field_value autonomous_field;
> +	struct hid_field_value range_start;
> +	struct hid_field_value range_end;
> +	struct hid_field_value lamp_count;
> +	struct hid_field_value lamparray_kind;
> +
> +	u16 lamp_count_value;
> +	u32 lamparray_kind_value;
> +
> +	struct led_classdev_mc mc_cdev;
> +	struct mc_subled subleds[3];
> +
> +	struct mutex dev_lock; /* Protects cached state and HID access */
> +	struct mutex sysfs_lock; /* Protects sysfs LED (de-)initialization */
> +
> +	u8 max_r;
> +	u8 max_g;
> +	u8 max_b;
> +	u8 max_brightness;
> +
> +	u8 last_r;
> +	u8 last_g;
> +	u8 last_b;
> +	u8 last_brightness;
> +
> +	bool use_leds_uapi;
> +	bool led_registered;
> +};
> +
> +/*
> + * Opaque handle exposed to callers via the header.
> + * Keep the actual state in lamparray_device, but return a stable pointer.
> + */
> +struct lamparray {
> +	struct lamparray_device ldev;
> +};
> +
> +/*
> + * Mapping for hid_device pointers to their lamparray data.
> + * Since there is not guarantee of how the driver using this library
> + * will use its drvdata, the only safe way to retrieve the lamparray
> + * data from a HID device pointer is using this mapping.
> + */
> +static DEFINE_XARRAY(lamparray_by_hdev);
> +
> +/* HID helper functions */
> +
> +static int get_field_value(struct hid_field_value *field_value)
> +{
> +	return field_value->field->value[field_value->index];
> +}
> +
> +static u8 get_field_value_as_u8(struct hid_field_value *field_value)
> +{
> +	return clamp_val(get_field_value(field_value), 0, U8_MAX);
> +}
> +
> +static void set_field_value(struct hid_field_value *field_value, int value)
> +{
> +	field_value->field->value[field_value->index] = value;
> +}
> +
> +static bool lamparray_color_fields_is_complete(struct lamparray_color_fields *color_fields)
> +{
> +	return color_fields->red.field && color_fields->green.field &&
> +	       color_fields->blue.field && color_fields->intensity.field;
> +}
> +
> +static int lamparray_read_attributes_report(struct lamparray_device *ldev)
> +{
> +	struct hid_device *hdev = ldev->hdev;
> +	struct hid_report *report;
> +
> +	if (!ldev->lamp_count.field) {
> +		hid_dbg(hdev, "No LampCount field found\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!ldev->lamparray_kind.field) {
> +		hid_dbg(hdev, "No LampArrayKind field found\n");
> +		return -ENODEV;
> +	}
> +
> +	report = ldev->lamp_count.field->report;
> +
> +	if (!report) {
> +		hid_dbg(hdev, "LampCount field has no report\n");
> +		return -ENODEV;
> +	}
> +
> +	mutex_lock(&ldev->dev_lock);
> +
> +	/* Update values */
> +	hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
> +	hid_hw_wait(hdev);
> +
> +	ldev->lamp_count_value = get_field_value(&ldev->lamp_count);
> +
> +	if (ldev->lamp_count_value == 0) {
> +		mutex_unlock(&ldev->dev_lock);
> +		hid_dbg(hdev, "LampCount is %d (invalid)\n", ldev->lamp_count_value);
> +		return -EINVAL;
> +	}
> +
> +	ldev->lamparray_kind_value = get_field_value(&ldev->lamparray_kind);
> +
> +	mutex_unlock(&ldev->dev_lock);
> +
> +	return 0;
> +}
> +
> +static int lamparray_parse_update_report(struct lamparray_device *ldev)
> +{
> +	struct hid_device *hdev = ldev->hdev;
> +	struct hid_report_enum *re;
> +	struct hid_report *report;
> +	struct hid_field *field;
> +	int i, j;
> +	int ret = 0;
> +
> +	mutex_lock(&ldev->dev_lock);
> +
> +	re = &hdev->report_enum[HID_FEATURE_REPORT];
> +
> +	list_for_each_entry(report, &re->report_list, list) {
> +		for (i = 0; i < report->maxfield; i++) {
> +			field = report->field[i];
> +			if (!field)
> +				continue;
> +
> +			if (!field->usage || !field->maxusage)
> +				continue;
> +
> +			for (j = 0; j < field->maxusage; j++) {
> +				u32 usage = field->usage[j].hid;
> +				u32 collection_idx = field->usage[j].collection_index;
> +				u32 collection_usage = hdev->collection[collection_idx].usage;
> +
> +				u16 page = (usage & HID_USAGE_PAGE) >> 16;
> +				u16 id = usage & HID_USAGE;
> +				u16 collection_usage_id = collection_usage & U16_MAX;
> +
> +				if (page != HID_LIGHTING_ILLUMINATION_USAGE_PAGE)
> +					continue;
> +
> +				if (collection_usage_id == HID_LAMPARRAY_ATTRIBUTES_REPORT) {
> +					switch (id) {
> +					case HID_LAIP_LAMP_COUNT:
> +						ldev->lamp_count.field = field;
> +						ldev->lamp_count.index = j;
> +						break;
> +					case HID_LAIP_LAMPARRAY_KIND:
> +						ldev->lamparray_kind.field = field;
> +						ldev->lamparray_kind.index = j;
> +						break;
> +					}
> +				} else if (collection_usage_id ==
> +					   HID_LAMP_ATTRIBUTES_RESPONSE_REPORT) {
> +					switch (id) {
> +					case HID_LAIP_RED_LEVEL_COUNT:
> +						ldev->color_levels.red.field = field;
> +						ldev->color_levels.red.index = j;
> +						break;
> +					case HID_LAIP_GREEN_LEVEL_COUNT:
> +						ldev->color_levels.green.field = field;
> +						ldev->color_levels.green.index = j;
> +						break;
> +					case HID_LAIP_BLUE_LEVEL_COUNT:
> +						ldev->color_levels.blue.field = field;
> +						ldev->color_levels.blue.index = j;
> +						break;
> +					case HID_LAIP_INTENSITY_LEVEL_COUNT:
> +						ldev->color_levels.intensity.field = field;
> +						ldev->color_levels.intensity.index = j;
> +						break;
> +					}
> +				} else if (collection_usage_id == HID_LAMP_RANGE_UPDATE_REPORT) {
> +					switch (id) {
> +					case HID_LAIP_RED_UPDATE_CHANNEL:
> +						ldev->color_update.red.field = field;
> +						ldev->color_update.red.index = j;
> +						break;
> +					case HID_LAIP_GREEN_UPDATE_CHANNEL:
> +						ldev->color_update.green.field = field;
> +						ldev->color_update.green.index = j;
> +						break;
> +					case HID_LAIP_BLUE_UPDATE_CHANNEL:
> +						ldev->color_update.blue.field = field;
> +						ldev->color_update.blue.index = j;
> +						break;
> +					case HID_LAIP_INTENSITY_UPDATE_CHANNEL:
> +						ldev->color_update.intensity.field = field;
> +						ldev->color_update.intensity.index = j;
> +						break;
> +					case HID_LAIP_LAMP_ID_START:
> +						ldev->range_start.field = field;
> +						ldev->range_start.index = j;
> +						break;
> +					case HID_LAIP_LAMP_ID_END:
> +						ldev->range_end.field = field;
> +						ldev->range_end.index = j;
> +						break;
> +					default:
> +						break;
> +					}
> +				} else if (collection_usage_id == HID_LAMPARRAY_CONTROL_REPORT &&
> +					   id == HID_LAIP_AUTONOMOUS_MODE) {
> +					ldev->autonomous_field.field = field;
> +					ldev->autonomous_field.index = j;
> +				}
> +			}
> +		}
> +	}
> +
> +	if (!ldev->autonomous_field.field ||
> +	    !lamparray_color_fields_is_complete(&ldev->color_update))
> +		ret = -ENODEV;
> +
> +	mutex_unlock(&ldev->dev_lock);
> +
> +	return ret;
> +}
> +
> +static int lamparray_hw_set_autonomous(struct lamparray_device *ldev,
> +				       bool enable)
> +{
> +	struct hid_device *hdev = ldev->hdev;
> +	struct hid_field *field = ldev->autonomous_field.field;
> +
> +	if (!field)
> +		return -ENODEV;
> +
> +	mutex_lock(&ldev->dev_lock);
> +
> +	set_field_value(&ldev->autonomous_field, !!enable);
> +
> +	hid_hw_request(hdev, field->report, HID_REQ_SET_REPORT);
> +	hid_hw_wait(hdev);
> +
> +	mutex_unlock(&ldev->dev_lock);
> +
> +	return 0;
> +}
> +
> +static int lamparray_hw_set_state(struct lamparray_device *ldev, u8 r, u8 g,
> +				  u8 b, u8 intensity)
> +{
> +	struct hid_device *hdev = ldev->hdev;
> +	struct hid_report *report;
> +
> +	if (!lamparray_color_fields_is_complete(&ldev->color_update))
> +		return -ENODEV;
> +
> +	if (ldev->range_start.field && ldev->range_end.field) {
> +		set_field_value(&ldev->range_start, 0);
> +		set_field_value(&ldev->range_end, ldev->lamp_count_value - 1);
> +	}
> +
> +	set_field_value(&ldev->color_update.red, r);
> +	set_field_value(&ldev->color_update.green, g);
> +	set_field_value(&ldev->color_update.blue, b);
> +	set_field_value(&ldev->color_update.intensity, intensity);
> +
> +	report = ldev->color_update.red.field->report;
> +	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> +	hid_hw_wait(hdev);
> +
> +	return 0;
> +}
> +
> +/*
> + * Simple helper to read the color information of the first lamp.
> + * This does not read the state of the whole lamp array since this driver only
> + * exposes one LED anyway, so one color is sufficient here for now.
> + */

Just now thought of checking this: We probably should still iterate over all 
lamps to make sure all behave the same (as a first heuristic, later we could 
think of grouping the lamps based on weather or not they behave the same and 
register a led device for each group).

Also we should check via IsProgrammable and no max value being 0 if it even is 
an RGB LED.

> +static int lamparray_get_lamp_attributes(struct lamparray_device *ldev)
> +{
> +	struct hid_device *hdev = ldev->hdev;
> +	struct hid_report *report;
> +
> +	if (!lamparray_color_fields_is_complete(&ldev->color_levels))
> +		return -ENODEV;
> +
> +	/*
> +	 * Get value of any lamp.
> +	 */
> +	report = ldev->color_levels.red.field->report;
> +
> +	mutex_lock(&ldev->dev_lock);
> +
> +	hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
> +	hid_hw_wait(hdev);
> +
> +	ldev->max_r = get_field_value_as_u8(&ldev->color_levels.red);
> +	ldev->max_g = get_field_value_as_u8(&ldev->color_levels.green);
> +	ldev->max_b = get_field_value_as_u8(&ldev->color_levels.blue);
> +	ldev->max_brightness = get_field_value_as_u8(&ldev->color_levels.intensity);
> +
> +	mutex_unlock(&ldev->dev_lock);
> +
> +	return 0;
> +}
> +
> +/* Helper functions */
> +
> +static int lamparray_restore_state(struct lamparray_device *ldev)
> +{
> +	u8 r, g, b;
> +	int ret;
> +	enum led_brightness brightness;
> +
> +	mutex_lock(&ldev->dev_lock);
> +
> +	if (!ldev->use_leds_uapi) {
> +		mutex_unlock(&ldev->dev_lock);
> +		return 0;
> +	}
> +
> +	r = ldev->last_r;
> +	g = ldev->last_g;
> +	b = ldev->last_b;
> +	brightness = ldev->last_brightness;
> +
> +	ldev->mc_cdev.subled_info[0].intensity = r;
> +	ldev->mc_cdev.subled_info[1].intensity = g;
> +	ldev->mc_cdev.subled_info[2].intensity = b;
> +	ldev->mc_cdev.led_cdev.brightness = brightness;
> +
> +	led_mc_calc_color_components(&ldev->mc_cdev, brightness);
> +
> +	ret = lamparray_hw_set_state(ldev, r, g, b, brightness);
> +
> +	mutex_unlock(&ldev->dev_lock);
> +	return ret;
> +}
> +
> +/* LEDs API */
> +
> +static int lamparray_led_brightness_set(struct led_classdev *cdev,
> +					enum led_brightness brightness)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> +	struct lamparray_device *ldev =
> +		container_of_const(mc, struct lamparray_device, mc_cdev);
> +	u8 r, g, b;
> +	int ret;
> +
> +	/*
> +	 * Brightness is handled by the LampArray device if supported,
> +	 * so we can pass the raw intensity values.
> +	 */
> +	r = mc->subled_info[0].intensity;
> +	g = mc->subled_info[1].intensity;
> +	b = mc->subled_info[2].intensity;
> +
> +	mc->led_cdev.brightness = brightness;
> +	led_mc_calc_color_components(&ldev->mc_cdev, brightness);
> +
> +	mutex_lock(&ldev->dev_lock);
> +	ret = lamparray_hw_set_state(ldev, r, g, b, brightness);
> +	if (ret) {
> +		mutex_unlock(&ldev->dev_lock);
> +		hid_err(ldev->hdev, "Failed to send LampArray update: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ldev->last_r = r;
> +	ldev->last_g = g;
> +	ldev->last_b = b;
> +	ldev->last_brightness = brightness;
> +	mutex_unlock(&ldev->dev_lock);
> +
> +	return 0;
> +}
> +
> +static enum led_brightness
> +lamparray_led_brightness_get(struct led_classdev *cdev)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> +	struct lamparray_device *ldev =
> +	    container_of_const(mc, struct lamparray_device, mc_cdev);
> +
> +	return ldev->last_brightness;
> +}
> +
> +static int lamparray_register_led(struct lamparray_device *ldev)
> +{
> +	struct device *dev = &ldev->hdev->dev;
> +	struct led_classdev *cdev = &ldev->mc_cdev.led_cdev;
> +	int ret;
> +
> +	mutex_lock(&ldev->sysfs_lock);
> +
> +	if (ldev->led_registered) {
> +		mutex_unlock(&ldev->sysfs_lock);
> +		return 0;
> +	}
> +
> +	if (!cdev->name) {
> +		/* Fallback value */
> +		const char *function = LED_FUNCTION_STATUS;
> +
> +		/* Some heuristics for choosing a better LED function. */
> +		if (ldev->lamparray_kind_value == HID_LAMPARRAY_KIND_KEYBOARD)
> +			function = LED_FUNCTION_KBD_BACKLIGHT;
> +
> +		cdev->name = kasprintf(GFP_KERNEL, "rgb:%s", function);
> +		if (!cdev->name) {
> +			mutex_unlock(&ldev->sysfs_lock);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	mutex_lock(&ldev->dev_lock);
> +	/* Setup */
> +	cdev->max_brightness = ldev->max_brightness;
> +	cdev->brightness_set_blocking = lamparray_led_brightness_set;
> +	cdev->brightness_get = lamparray_led_brightness_get;
> +	cdev->flags |= LED_RETAIN_AT_SHUTDOWN;
> +
> +	ldev->subleds[0].color_index = LED_COLOR_ID_RED;
> +	ldev->subleds[0].max_intensity = ldev->max_r;
> +	ldev->subleds[1].color_index = LED_COLOR_ID_GREEN;
> +	ldev->subleds[1].max_intensity = ldev->max_g;
> +	ldev->subleds[2].color_index = LED_COLOR_ID_BLUE;
> +	ldev->subleds[2].max_intensity = ldev->max_b;
> +
> +	/* Set values */
> +	ldev->subleds[0].intensity = ldev->last_r;
> +	ldev->subleds[1].intensity = ldev->last_g;
> +	ldev->subleds[2].intensity = ldev->last_b;
> +	cdev->brightness = ldev->last_brightness;
> +
> +	ldev->mc_cdev.subled_info = ldev->subleds;
> +	ldev->mc_cdev.num_colors = ARRAY_SIZE(ldev->subleds);
> +
> +	/* Ensure subled_info[].brightness matches intensity + brightness */
> +	led_mc_calc_color_components(&ldev->mc_cdev, ldev->last_brightness);
> +	mutex_unlock(&ldev->dev_lock);
> +
> +	ret = led_classdev_multicolor_register(dev, &ldev->mc_cdev);
> +	if (ret) {
> +		mutex_unlock(&ldev->sysfs_lock);
> +		return ret;
> +	}
> +
> +	ldev->led_registered = true;
> +	mutex_unlock(&ldev->sysfs_lock);
> +
> +	return 0;
> +}
> +
> +static void lamparray_unregister_led(struct lamparray_device *ldev)
> +{
> +	bool was_registered;
> +	struct led_classdev *cdev = &ldev->mc_cdev.led_cdev;
> +
> +	mutex_lock(&ldev->sysfs_lock);
> +	was_registered = ldev->led_registered;
> +	ldev->led_registered = false;
> +
> +	if (was_registered)
> +		led_classdev_multicolor_unregister(&ldev->mc_cdev);
> +
> +	kfree(cdev->name);
> +	cdev->name = NULL;
> +
> +	mutex_unlock(&ldev->sysfs_lock);
> +}
> +
> +/* Sysfs */
> +
> +static struct lamparray_device *
> +lamparray_ldev_from_sysfs_dev(struct device *dev)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +
> +	return xa_load(&lamparray_by_hdev, (unsigned long)hdev);
> +}
> +
> +static ssize_t use_leds_uapi_show(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct lamparray_device *ldev = lamparray_ldev_from_sysfs_dev(dev);
> +
> +	if (!ldev)
> +		return -ENODEV;
> +
> +	return sysfs_emit(buf, "%d\n", ldev->use_leds_uapi);
> +}
> +
> +static ssize_t use_leds_uapi_store(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct lamparray_device *ldev = lamparray_ldev_from_sysfs_dev(dev);
> +	int val;
> +	int old_val;
> +	int ret;
> +
> +	if (!ldev)
> +		return -ENODEV;
> +
> +	ret = kstrtoint(buf, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val != 0 && val != 1)
> +		return -EINVAL;
> +
> +	mutex_lock(&ldev->dev_lock);
> +	old_val = ldev->use_leds_uapi;
> +
> +	if (val == old_val) {
> +		mutex_unlock(&ldev->dev_lock);
> +		return count;
> +	}
> +
> +	ldev->use_leds_uapi = val;
> +	mutex_unlock(&ldev->dev_lock);
> +
> +	if (val == 1) {
> +		ret = lamparray_register_led(ldev);
> +		if (ret) {
> +			mutex_lock(&ldev->dev_lock);
> +			ldev->use_leds_uapi = old_val;
> +			mutex_unlock(&ldev->dev_lock);
> +			return ret;
> +		}
> +		ret = lamparray_restore_state(ldev);
> +		if (ret) {
> +			hid_err(ldev->hdev, "Could not restore state: %d\n", ret);
> +			return ret;
> +		}
> +
> +	} else {
> +		lamparray_unregister_led(ldev);
> +	}
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(use_leds_uapi);
> +
> +static int lamparray_register_sysfs(struct lamparray_device *ldev)
> +{
> +	struct device *dev = &ldev->hdev->dev;
> +	int ret;
> +
> +	ret = sysfs_create_file(&dev->kobj, &dev_attr_use_leds_uapi.attr);
> +	if (ret)
> +		hid_err(ldev->hdev,
> +			"Failed to create lamparray sysfs group: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static void lamparray_remove_sysfs(struct lamparray_device *ldev)
> +{
> +	sysfs_remove_file(&ldev->hdev->dev.kobj, &dev_attr_use_leds_uapi.attr);
> +}
> +
> +/* Public API */
> +
> +bool lamparray_is_supported_device(struct hid_device *hdev)
> +{
> +	unsigned int i;
> +
> +	hid_dbg(hdev, "lamparray: walking %u collections\n",
> +		hdev->maxcollection);
> +
> +	for (i = 0; i < hdev->maxcollection; i++) {
> +		struct hid_collection *col = &hdev->collection[i];
> +		u16 page = (col->usage & HID_USAGE_PAGE) >> 16;
> +		u16 code = col->usage & HID_USAGE;
> +
> +		hid_dbg(hdev,
> +			"lamparray:  collection[%u]: type=%u level=%u usage=0x%08x page=0x%04x code=0x%04x\n",
> +			i, col->type, col->level, col->usage, page, code);
> +
> +		if (col->type == HID_COLLECTION_APPLICATION &&
> +		    page == HID_LIGHTING_ILLUMINATION_USAGE_PAGE &&
> +		    code == HID_APPLICATION_COLLECTION_USAGE_TYPE) {
> +			return true;
> +		}
> +	}
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(lamparray_is_supported_device);
> +
> +struct lamparray *
> +lamparray_register(struct hid_device *hdev,
> +		   const struct lamparray_init_state *led_init_state)
> +{
> +	int ret;
> +	struct lamparray *la;
> +	struct lamparray_device *ldev;
> +
> +	if (!hdev)
> +		return ERR_PTR(-ENODEV);
> +
> +	la = kzalloc_obj(*la, GFP_KERNEL);
> +	if (!la)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ldev = &la->ldev;
> +
> +	mutex_init(&ldev->dev_lock);
> +	mutex_init(&ldev->sysfs_lock);
> +	ldev->hdev = hdev;
> +	ldev->use_leds_uapi = true;
> +	ldev->led_registered = false;
> +
> +	/* Make sure the driver lock gets released for probing. */
> +	hid_device_io_start(hdev);
> +
> +	ret = lamparray_parse_update_report(ldev);
> +	if (ret) {
> +		hid_err(hdev, "No LampArray update report found: %d\n", ret);
> +		goto err_free;
> +	}
> +
> +	ret = lamparray_read_attributes_report(ldev);
> +	if (ret) {
> +		hid_err(hdev,
> +			"Could not determine LampCount: %d\n",
> +			ret);
> +		goto err_free;
> +	}
> +
> +	ret = lamparray_get_lamp_attributes(ldev);
> +	if (ret) {
> +		hid_err(hdev,
> +			"Faulty device. Could not query lamp attributes.\n");
> +		goto err_free;
> +	}
> +
> +	/* Use black (all zeros) as default. */
> +	if (led_init_state) {
> +		ldev->last_r = min(led_init_state->r, ldev->max_r);
> +		ldev->last_g = min(led_init_state->g, ldev->max_g);
> +		ldev->last_b = min(led_init_state->b, ldev->max_b);
> +		ldev->last_brightness = min(led_init_state->brightness,
> +					    ldev->max_brightness);
> +	}
> +
> +	ret = lamparray_register_led(ldev);
> +	if (ret) {
> +		hid_warn(hdev, "Failed to register LED UAPI: %d\n", ret);
> +		ldev->use_leds_uapi = false;
> +	}
> +
> +	ret = xa_err(xa_store(&lamparray_by_hdev, (unsigned long)hdev, ldev,
> +			      GFP_KERNEL));
> +	if (ret)
> +		goto err_unregister_led;
> +
> +	ret = lamparray_register_sysfs(ldev);
> +	if (ret)
> +		goto err_xa_erase;
> +
> +	ret = lamparray_hw_set_autonomous(ldev, false);
> +	if (ret) {
> +		hid_err(hdev, "Could not disable autonomous mode: %d", ret);
> +		goto err_remove_sysfs;
> +	}
> +
> +	hid_info(hdev, "LampArray device registered\n");
> +
> +	ret = lamparray_restore_state(ldev);
> +	if (ret) {
> +		hid_err(hdev, "Failed to set default state: %d", ret);
> +		goto err_remove_sysfs;
> +	}
> +
> +	hid_device_io_stop(hdev);
> +	return la;
> +
> +err_remove_sysfs:
> +	lamparray_remove_sysfs(ldev);
> +err_xa_erase:
> +	xa_erase(&lamparray_by_hdev, (unsigned long)hdev);
> +err_unregister_led:
> +	lamparray_unregister_led(ldev);
> +err_free:
> +	hid_device_io_stop(hdev);
> +	mutex_destroy(&ldev->dev_lock);
> +	mutex_destroy(&ldev->sysfs_lock);
> +	kfree(la);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(lamparray_register);
> +
> +void lamparray_unregister(struct lamparray *la)
> +{
> +	struct lamparray_device *ldev;
> +
> +	if (!la)
> +		return;
> +
> +	ldev = &la->ldev;
> +
> +	lamparray_hw_set_autonomous(ldev, true);
> +
> +	lamparray_remove_sysfs(ldev);
> +	xa_erase(&lamparray_by_hdev, (unsigned long)ldev->hdev);
> +	lamparray_unregister_led(ldev);
> +
> +	mutex_destroy(&ldev->dev_lock);
> +	mutex_destroy(&ldev->sysfs_lock);
> +	kfree(la);
> +}
> +EXPORT_SYMBOL_GPL(lamparray_unregister);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Tim Guttzeit <tgu@tuxedocomputers.com>");
> +MODULE_AUTHOR("Aaron Erhardt <aer@tuxedocomputers.com>");
> +MODULE_DESCRIPTION("HID LampArray helper module (single-zone RGB)");
> diff --git a/include/linux/hid-lamparray.h b/include/linux/hid-lamparray.h
> new file mode 100644
> index 000000000000..a77869728d12
> --- /dev/null
> +++ b/include/linux/hid-lamparray.h
> @@ -0,0 +1,88 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef _HID_LAMPARRAY_H
> +#define _HID_LAMPARRAY_H
> +
> +#include <linux/hid.h>
> +#include <linux/err.h>
> +#include <linux/types.h>
> +
> +struct lamparray;
> +
> +/*
> + * Optional initial LED state for lamparray_register().
> + * Used to define the initial state of a LampArray's LEDs.
> + */
> +struct lamparray_init_state {
> +	u8 r;
> +	u8 g;
> +	u8 b;
> +	u8 brightness;
> +};
> +
> +#if IS_ENABLED(CONFIG_HID_LAMPARRAY)
> +
> +/**
> + * lamparray_is_supported_device() - check whether a HID device supports LampArray
> + * @hdev: HID device to inspect
> + *
> + * Check whether the given HID device exposes a Lighting/LampArray application
> + * collection as defined by the HID Lighting specification.
> + *
> + * This helper can be used by HID drivers to determine whether LampArray
> + * functionality should be enabled for a device.
> + *
> + * Return: %true if LampArray support is detected, %false otherwise.
> + */
> +bool lamparray_is_supported_device(struct hid_device *hdev);
> +
> +/**
> + * lamparray_register() - initialize LampArray support for a HID device
> + * @hdev: HID device
> + * @led_init_state: Optional LED state at init specification
> + *
> + * Allocate and initialize internal LampArray state for the given HID device.
> + * The function parses required HID reports and fields and registers the
> + * associated miscdevice and sysfs attributes.
> + *
> + * Registers a multicolor LED class device to expose the LampArray functionality
> + * via the LED subsystem. If specified, the desired initial LED state is
> + * applied. If led_init_state is NULL, a default state is applied (all LEDs off).
> + *
> + * Return: pointer to a LampArray handle on success, or ERR_PTR() on failure.
> + */
> +struct lamparray *lamparray_register(struct hid_device *hdev,
> +				     const struct lamparray_init_state *led_init_state);
> +
> +/**
> + * lamparray_unregister() - tear down LampArray support
> + * @la: LampArray handle returned by lamparray_register()
> + *
> + * Remove all resources associated with a LampArray instance.
> + *
> + * This unregisters the LED class device (if present), removes the miscdevice
> + * and sysfs interfaces and frees all internal state associated with @la.
> + */
> +void lamparray_unregister(struct lamparray *la);
> +
> +#else /* !CONFIG_HID_LAMPARRAY */
> +
> +static inline bool lamparray_is_supported_device(struct hid_device *hdev)
> +{
> +	return false;
> +}
> +
> +static inline struct lamparray *
> +lamparray_register(struct hid_device *hdev,
> +		   const struct lamparray_init_state *led_init_state)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static inline void lamparray_unregister(struct lamparray *la)
> +{
> +}
> +
> +#endif /* CONFIG_HID_LAMPARRAY */
> +
> +#endif /* _HID_LAMPARRAY_H */

Re: [PATCH v5 1/2] HID: lamparray: add new LampArray helper module

[Aaron Erhardt]

Am 03.09.26 um 22:07 schrieb Werner Sembach:
> 
> Am 03.09.26 um 09:35 schrieb Aaron Erhardt:
>> Add a new hid-lamparray helper module that provides basic support for
>> devices exposing a Lighting/LampArray application collection (usage
>> page 0x59) and registers a single-zone RGB LED representation via the
>> LED subsystem.
>>
>> The module can be used as a library in HID drivers to add support for
>> the HID LampArray protocol. While the API is quite basic as of now
>> it could be extended in the future.
>>
>> Co-developed-by: Tim Guttzeit <tgu@tuxedocomputers.com>
>> Signed-off-by: Tim Guttzeit <tgu@tuxedocomputers.com>
>> Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
>> ---
>>   .../ABI/testing/sysfs-driver-hid-lamparray    |  16 +
>>   drivers/hid/Kconfig                           |  17 +
>>   drivers/hid/Makefile                          |   2 +
>>   drivers/hid/hid-lamparray.c                   | 812 ++++++++++++++++++
>>   include/linux/hid-lamparray.h                 |  88 ++
>>   5 files changed, 935 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-lamparray
>>   create mode 100644 drivers/hid/hid-lamparray.c
>>   create mode 100644 include/linux/hid-lamparray.h
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-lamparray b/Documentation/ABI/testing/sysfs-driver-hid-lamparray
>> new file mode 100644
>> index 000000000000..795be6c4c368
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-driver-hid-lamparray
>> @@ -0,0 +1,16 @@
>> +What:        /sys/bus/hid/devices/<bus>:<vid>:<pid>.<n>/use_leds_uapi
>> +Date:        August 2026
>> +KernelVersion:    7.3
>> +Contact:    aer@tuxedocomputers.com
>> +Description:
>> +        If a driver uses the hid-lamparray module and a device supporting
>> +        LampArray is found, one multicolor LED class device is registered under
>> +        /sys/class/leds/rgb:<function> to expose the single-zone RGB control.
>> +        Every device gets an incremental unique id.
>> +
>> +        Additionally, the use_leds_uapi sysfs attribute to control the LED class
>> +        device is attached directly to the HID device at
>> +        /sys/bus/hid/devices/<bus>:<vid>:<pid>.<n>/use_leds_uapi. Writing 0 to
>> +        use_leds_uapi unregisters the LED class device. The last state is kept
>> +        cached. Writing 1 registers it again and restores the cached state to
>> +        hardware.
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index aa7fa11a0197..4afd80a67b39 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -92,6 +92,23 @@ config HID_GENERIC
>>         If unsure, say Y.
>>   +config HID_LAMPARRAY
>> +    tristate "HID LampArray helper"
>> +    depends on HID
>> +    depends on LEDS_CLASS_MULTICOLOR
>> +    default n
>> +    help
>> +      Helper for HID devices exposing a Lighting/LampArray collection.
>> +      Treats LampArray devices as a single-zone device and exposes a sysfs
>> +      interface for changing color and intensity values. Also exposes a
>> +      sysfs flag to be disabled e.g. by a userspace driver.
>> +
>> +      This can be used as library in existing drivers. The generic HID
>> +      driver is extended by default to handle lamp array devices if this
>> +      option is enabled.
>> +
>> +      If unsure, say N.
>> +
>>   config HID_HAPTIC
>>       bool "Haptic touchpad support"
>>       default n
>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> index 48a863b245ee..f95630fa8bd8 100644
>> --- a/drivers/hid/Makefile
>> +++ b/drivers/hid/Makefile
>> @@ -13,6 +13,8 @@ obj-$(CONFIG_UHID)        += uhid.o
>>     obj-$(CONFIG_HID_GENERIC)    += hid-generic.o
>>   +obj-$(CONFIG_HID_LAMPARRAY)    += hid-lamparray.o
>> +
>>   hid-$(CONFIG_HIDRAW)        += hidraw.o
>>     hid-logitech-y        := hid-lg.o
>> diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
>> new file mode 100644
>> index 000000000000..9a438aa2d305
>> --- /dev/null
>> +++ b/drivers/hid/hid-lamparray.c
>> @@ -0,0 +1,812 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * hid-lamparray.c - HID LampArray helper module (single-zone RGB)
>> + *
>> + * Helper module for HID drivers supporting devices that expose a Lighting and
>> + * Illumination (LampArray) application collection (usage page 0x59).
>> + *
>> + * The module provides a minimal integration with the LED subsystem and treats
>> + * the device as a single zone: all lamps share one RGB value and a global
>> + * brightness level. It does not implement multi-zone layouts or hardware
>> + * effects.
>> + *
>> + * If enabled and a device supporting LampArray is found, one multicolor LED
>> + * class device is registered under /sys/class/leds/<HID-ID>:rgb:LampArray to
>> + * expose the single-zone RGB control.
>> + *
>> + * The use_leds_uapi sysfs attribute is attached directly to the HID device
>> + * under /sys/bus/hid/devices/<HID-ID>/use_leds_uapi. Writing 0 to use_leds_uapi
>> + * unregisters the LED class device. The last state is kept cached. Writing 1
>> + * registers it again and restores the cached state to hardware. State is cached
>> + * as last known RGB + brightness.
>> + *
>> + * The module does not bind to devices on its own. Instead, a HID driver may
>> + * query support via lamparray_is_supported_device() after hid_parse() and
>> + * create an instance using lamparray_register().
>> + *
>> + * Copyright (C) 2026 Tim Guttzeit <tgu@tuxedocomputers.com>
>> + * Copyright (C) 2026 Aaron Erhardt <aer@tuxedocomputers.com>
>> + */
>> +
>> +#include <dt-bindings/leds/common.h>
>> +#include <linux/limits.h>
>> +#include <linux/minmax.h>
>> +#include <linux/hid.h>
>> +#include <linux/leds.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/hid-lamparray.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/container_of.h>
>> +#include <linux/led-class-multicolor.h>
>> +#include <linux/xarray.h>
>> +
>> +/* Constants */
>> +
>> +/* HID usages (LampArray, etc.) */
>> +#define HID_LIGHTING_ILLUMINATION_USAGE_PAGE    0x0059
>> +
>> +/* HID usage types */
>> +#define HID_APPLICATION_COLLECTION_USAGE_TYPE    0x0001
>> +#define HID_LAMPARRAY_ATTRIBUTES_REPORT    0x0002
>> +#define HID_LAMP_ATTRIBUTES_RESPONSE_REPORT    0x0022
>> +#define HID_LAMP_RANGE_UPDATE_REPORT        0x0060
>> +#define HID_LAMPARRAY_CONTROL_REPORT        0x0070
>> +
>> +/* HID attributes */
>> +#define HID_LAIP_LAMP_COUNT            0x0003
>> +#define HID_LAIP_LAMPARRAY_KIND            0x0007
>> +#define HID_LAIP_RED_LEVEL_COUNT        0x0028
>> +#define HID_LAIP_GREEN_LEVEL_COUNT        0x0029
>> +#define HID_LAIP_BLUE_LEVEL_COUNT        0x002a
>> +#define HID_LAIP_INTENSITY_LEVEL_COUNT        0x002b
>> +#define HID_LAIP_RED_UPDATE_CHANNEL        0x0051
>> +#define HID_LAIP_GREEN_UPDATE_CHANNEL        0x0052
>> +#define HID_LAIP_BLUE_UPDATE_CHANNEL        0x0053
>> +#define HID_LAIP_INTENSITY_UPDATE_CHANNEL    0x0054
>> +#define HID_LAIP_LAMP_ID_START            0x0061
>> +#define HID_LAIP_LAMP_ID_END            0x0062
>> +#define HID_LAIP_AUTONOMOUS_MODE        0x0071
>> +
>> +/* LampArrayKind values */
>> +#define HID_LAMPARRAY_KIND_KEYBOARD        0x0001
>> +
>> +/* Helper struct for fields and their indices */
>> +struct hid_field_value {
>> +    struct hid_field *field;
>> +    int index;
>> +};
>> +
>> +/* Helper struct for color fields */
>> +struct lamparray_color_fields {
>> +    struct hid_field_value red;
>> +    struct hid_field_value green;
>> +    struct hid_field_value blue;
>> +    struct hid_field_value intensity;
>> +};
>> +
>> +/* Device state */
>> +struct lamparray_device {
>> +    struct hid_device *hdev;
>> +
>> +    struct lamparray_color_fields color_levels;
>> +    struct lamparray_color_fields color_update;
>> +
>> +    struct hid_field_value autonomous_field;
>> +    struct hid_field_value range_start;
>> +    struct hid_field_value range_end;
>> +    struct hid_field_value lamp_count;
>> +    struct hid_field_value lamparray_kind;
>> +
>> +    u16 lamp_count_value;
>> +    u32 lamparray_kind_value;
>> +
>> +    struct led_classdev_mc mc_cdev;
>> +    struct mc_subled subleds[3];
>> +
>> +    struct mutex dev_lock; /* Protects cached state and HID access */
>> +    struct mutex sysfs_lock; /* Protects sysfs LED (de-)initialization */
>> +
>> +    u8 max_r;
>> +    u8 max_g;
>> +    u8 max_b;
>> +    u8 max_brightness;
>> +
>> +    u8 last_r;
>> +    u8 last_g;
>> +    u8 last_b;
>> +    u8 last_brightness;
>> +
>> +    bool use_leds_uapi;
>> +    bool led_registered;
>> +};
>> +
>> +/*
>> + * Opaque handle exposed to callers via the header.
>> + * Keep the actual state in lamparray_device, but return a stable pointer.
>> + */
>> +struct lamparray {
>> +    struct lamparray_device ldev;
>> +};
>> +
>> +/*
>> + * Mapping for hid_device pointers to their lamparray data.
>> + * Since there is not guarantee of how the driver using this library
>> + * will use its drvdata, the only safe way to retrieve the lamparray
>> + * data from a HID device pointer is using this mapping.
>> + */
>> +static DEFINE_XARRAY(lamparray_by_hdev);
>> +
>> +/* HID helper functions */
>> +
>> +static int get_field_value(struct hid_field_value *field_value)
>> +{
>> +    return field_value->field->value[field_value->index];
>> +}
>> +
>> +static u8 get_field_value_as_u8(struct hid_field_value *field_value)
>> +{
>> +    return clamp_val(get_field_value(field_value), 0, U8_MAX);
>> +}
>> +
>> +static void set_field_value(struct hid_field_value *field_value, int value)
>> +{
>> +    field_value->field->value[field_value->index] = value;
>> +}
>> +
>> +static bool lamparray_color_fields_is_complete(struct lamparray_color_fields *color_fields)
>> +{
>> +    return color_fields->red.field && color_fields->green.field &&
>> +           color_fields->blue.field && color_fields->intensity.field;
>> +}
>> +
>> +static int lamparray_read_attributes_report(struct lamparray_device *ldev)
>> +{
>> +    struct hid_device *hdev = ldev->hdev;
>> +    struct hid_report *report;
>> +
>> +    if (!ldev->lamp_count.field) {
>> +        hid_dbg(hdev, "No LampCount field found\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    if (!ldev->lamparray_kind.field) {
>> +        hid_dbg(hdev, "No LampArrayKind field found\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    report = ldev->lamp_count.field->report;
>> +
>> +    if (!report) {
>> +        hid_dbg(hdev, "LampCount field has no report\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    mutex_lock(&ldev->dev_lock);
>> +
>> +    /* Update values */
>> +    hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
>> +    hid_hw_wait(hdev);
>> +
>> +    ldev->lamp_count_value = get_field_value(&ldev->lamp_count);
>> +
>> +    if (ldev->lamp_count_value == 0) {
>> +        mutex_unlock(&ldev->dev_lock);
>> +        hid_dbg(hdev, "LampCount is %d (invalid)\n", ldev->lamp_count_value);
>> +        return -EINVAL;
>> +    }
>> +
>> +    ldev->lamparray_kind_value = get_field_value(&ldev->lamparray_kind);
>> +
>> +    mutex_unlock(&ldev->dev_lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static int lamparray_parse_update_report(struct lamparray_device *ldev)
>> +{
>> +    struct hid_device *hdev = ldev->hdev;
>> +    struct hid_report_enum *re;
>> +    struct hid_report *report;
>> +    struct hid_field *field;
>> +    int i, j;
>> +    int ret = 0;
>> +
>> +    mutex_lock(&ldev->dev_lock);
>> +
>> +    re = &hdev->report_enum[HID_FEATURE_REPORT];
>> +
>> +    list_for_each_entry(report, &re->report_list, list) {
>> +        for (i = 0; i < report->maxfield; i++) {
>> +            field = report->field[i];
>> +            if (!field)
>> +                continue;
>> +
>> +            if (!field->usage || !field->maxusage)
>> +                continue;
>> +
>> +            for (j = 0; j < field->maxusage; j++) {
>> +                u32 usage = field->usage[j].hid;
>> +                u32 collection_idx = field->usage[j].collection_index;
>> +                u32 collection_usage = hdev->collection[collection_idx].usage;
>> +
>> +                u16 page = (usage & HID_USAGE_PAGE) >> 16;
>> +                u16 id = usage & HID_USAGE;
>> +                u16 collection_usage_id = collection_usage & U16_MAX;
>> +
>> +                if (page != HID_LIGHTING_ILLUMINATION_USAGE_PAGE)
>> +                    continue;
>> +
>> +                if (collection_usage_id == HID_LAMPARRAY_ATTRIBUTES_REPORT) {
>> +                    switch (id) {
>> +                    case HID_LAIP_LAMP_COUNT:
>> +                        ldev->lamp_count.field = field;
>> +                        ldev->lamp_count.index = j;
>> +                        break;
>> +                    case HID_LAIP_LAMPARRAY_KIND:
>> +                        ldev->lamparray_kind.field = field;
>> +                        ldev->lamparray_kind.index = j;
>> +                        break;
>> +                    }
>> +                } else if (collection_usage_id ==
>> +                       HID_LAMP_ATTRIBUTES_RESPONSE_REPORT) {
>> +                    switch (id) {
>> +                    case HID_LAIP_RED_LEVEL_COUNT:
>> +                        ldev->color_levels.red.field = field;
>> +                        ldev->color_levels.red.index = j;
>> +                        break;
>> +                    case HID_LAIP_GREEN_LEVEL_COUNT:
>> +                        ldev->color_levels.green.field = field;
>> +                        ldev->color_levels.green.index = j;
>> +                        break;
>> +                    case HID_LAIP_BLUE_LEVEL_COUNT:
>> +                        ldev->color_levels.blue.field = field;
>> +                        ldev->color_levels.blue.index = j;
>> +                        break;
>> +                    case HID_LAIP_INTENSITY_LEVEL_COUNT:
>> +                        ldev->color_levels.intensity.field = field;
>> +                        ldev->color_levels.intensity.index = j;
>> +                        break;
>> +                    }
>> +                } else if (collection_usage_id == HID_LAMP_RANGE_UPDATE_REPORT) {
>> +                    switch (id) {
>> +                    case HID_LAIP_RED_UPDATE_CHANNEL:
>> +                        ldev->color_update.red.field = field;
>> +                        ldev->color_update.red.index = j;
>> +                        break;
>> +                    case HID_LAIP_GREEN_UPDATE_CHANNEL:
>> +                        ldev->color_update.green.field = field;
>> +                        ldev->color_update.green.index = j;
>> +                        break;
>> +                    case HID_LAIP_BLUE_UPDATE_CHANNEL:
>> +                        ldev->color_update.blue.field = field;
>> +                        ldev->color_update.blue.index = j;
>> +                        break;
>> +                    case HID_LAIP_INTENSITY_UPDATE_CHANNEL:
>> +                        ldev->color_update.intensity.field = field;
>> +                        ldev->color_update.intensity.index = j;
>> +                        break;
>> +                    case HID_LAIP_LAMP_ID_START:
>> +                        ldev->range_start.field = field;
>> +                        ldev->range_start.index = j;
>> +                        break;
>> +                    case HID_LAIP_LAMP_ID_END:
>> +                        ldev->range_end.field = field;
>> +                        ldev->range_end.index = j;
>> +                        break;
>> +                    default:
>> +                        break;
>> +                    }
>> +                } else if (collection_usage_id == HID_LAMPARRAY_CONTROL_REPORT &&
>> +                       id == HID_LAIP_AUTONOMOUS_MODE) {
>> +                    ldev->autonomous_field.field = field;
>> +                    ldev->autonomous_field.index = j;
>> +                }
>> +            }
>> +        }
>> +    }
>> +
>> +    if (!ldev->autonomous_field.field ||
>> +        !lamparray_color_fields_is_complete(&ldev->color_update))
>> +        ret = -ENODEV;
>> +
>> +    mutex_unlock(&ldev->dev_lock);
>> +
>> +    return ret;
>> +}
>> +
>> +static int lamparray_hw_set_autonomous(struct lamparray_device *ldev,
>> +                       bool enable)
>> +{
>> +    struct hid_device *hdev = ldev->hdev;
>> +    struct hid_field *field = ldev->autonomous_field.field;
>> +
>> +    if (!field)
>> +        return -ENODEV;
>> +
>> +    mutex_lock(&ldev->dev_lock);
>> +
>> +    set_field_value(&ldev->autonomous_field, !!enable);
>> +
>> +    hid_hw_request(hdev, field->report, HID_REQ_SET_REPORT);
>> +    hid_hw_wait(hdev);
>> +
>> +    mutex_unlock(&ldev->dev_lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static int lamparray_hw_set_state(struct lamparray_device *ldev, u8 r, u8 g,
>> +                  u8 b, u8 intensity)
>> +{
>> +    struct hid_device *hdev = ldev->hdev;
>> +    struct hid_report *report;
>> +
>> +    if (!lamparray_color_fields_is_complete(&ldev->color_update))
>> +        return -ENODEV;
>> +
>> +    if (ldev->range_start.field && ldev->range_end.field) {
>> +        set_field_value(&ldev->range_start, 0);
>> +        set_field_value(&ldev->range_end, ldev->lamp_count_value - 1);
>> +    }
>> +
>> +    set_field_value(&ldev->color_update.red, r);
>> +    set_field_value(&ldev->color_update.green, g);
>> +    set_field_value(&ldev->color_update.blue, b);
>> +    set_field_value(&ldev->color_update.intensity, intensity);
>> +
>> +    report = ldev->color_update.red.field->report;
>> +    hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
>> +    hid_hw_wait(hdev);
>> +
>> +    return 0;
>> +}
>> +
>> +/*
>> + * Simple helper to read the color information of the first lamp.
>> + * This does not read the state of the whole lamp array since this driver only
>> + * exposes one LED anyway, so one color is sufficient here for now.
>> + */
> 
> Just now thought of checking this: We probably should still iterate over all lamps to make sure all behave the same (as a first heuristic, later we could think of grouping the lamps based on weather or not they behave the same and register a led device for each group).
> 

That's a good point. I'll consider this in v6 once more feedback has come in.

The only downside is that iterating over all lamps requires a lot of back and forth between the driver and the device.
One request for each lamp and in reality, there won't be many LampArray devices with different properties per lamp.
But it is probably still worth to make sure so we don't send bogus requests to the device.

> Also we should check via IsProgrammable and no max value being 0 if it even is an RGB LED.
> 
Ack.

As one of the AI bots already figured out, santizing IntensityLevelCount (aka max_brightness) is absolutely necessary to prevent division by zero.
Such lamps should obviously be ignored.

>> +static int lamparray_get_lamp_attributes(struct lamparray_device *ldev)
>> +{
>> +    struct hid_device *hdev = ldev->hdev;
>> +    struct hid_report *report;
>> +
>> +    if (!lamparray_color_fields_is_complete(&ldev->color_levels))
>> +        return -ENODEV;
>> +
>> +    /*
>> +     * Get value of any lamp.
>> +     */
>> +    report = ldev->color_levels.red.field->report;
>> +
>> +    mutex_lock(&ldev->dev_lock);
>> +
>> +    hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
>> +    hid_hw_wait(hdev);
>> +
>> +    ldev->max_r = get_field_value_as_u8(&ldev->color_levels.red);
>> +    ldev->max_g = get_field_value_as_u8(&ldev->color_levels.green);
>> +    ldev->max_b = get_field_value_as_u8(&ldev->color_levels.blue);
>> +    ldev->max_brightness = get_field_value_as_u8(&ldev->color_levels.intensity);
>> +
>> +    mutex_unlock(&ldev->dev_lock);
>> +
>> +    return 0;
>> +}
>> +
>> +/* Helper functions */
>> +
>> +static int lamparray_restore_state(struct lamparray_device *ldev)
>> +{
>> +    u8 r, g, b;
>> +    int ret;
>> +    enum led_brightness brightness;
>> +
>> +    mutex_lock(&ldev->dev_lock);
>> +
>> +    if (!ldev->use_leds_uapi) {
>> +        mutex_unlock(&ldev->dev_lock);
>> +        return 0;
>> +    }
>> +
>> +    r = ldev->last_r;
>> +    g = ldev->last_g;
>> +    b = ldev->last_b;
>> +    brightness = ldev->last_brightness;
>> +
>> +    ldev->mc_cdev.subled_info[0].intensity = r;
>> +    ldev->mc_cdev.subled_info[1].intensity = g;
>> +    ldev->mc_cdev.subled_info[2].intensity = b;
>> +    ldev->mc_cdev.led_cdev.brightness = brightness;
>> +
>> +    led_mc_calc_color_components(&ldev->mc_cdev, brightness);
>> +
>> +    ret = lamparray_hw_set_state(ldev, r, g, b, brightness);
>> +
>> +    mutex_unlock(&ldev->dev_lock);
>> +    return ret;
>> +}
>> +
>> +/* LEDs API */
>> +
>> +static int lamparray_led_brightness_set(struct led_classdev *cdev,
>> +                    enum led_brightness brightness)
>> +{
>> +    struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
>> +    struct lamparray_device *ldev =
>> +        container_of_const(mc, struct lamparray_device, mc_cdev);
>> +    u8 r, g, b;
>> +    int ret;
>> +
>> +    /*
>> +     * Brightness is handled by the LampArray device if supported,
>> +     * so we can pass the raw intensity values.
>> +     */
>> +    r = mc->subled_info[0].intensity;
>> +    g = mc->subled_info[1].intensity;
>> +    b = mc->subled_info[2].intensity;
>> +
>> +    mc->led_cdev.brightness = brightness;
>> +    led_mc_calc_color_components(&ldev->mc_cdev, brightness);
>> +
>> +    mutex_lock(&ldev->dev_lock);
>> +    ret = lamparray_hw_set_state(ldev, r, g, b, brightness);
>> +    if (ret) {
>> +        mutex_unlock(&ldev->dev_lock);
>> +        hid_err(ldev->hdev, "Failed to send LampArray update: %d\n",
>> +            ret);
>> +        return ret;
>> +    }
>> +
>> +    ldev->last_r = r;
>> +    ldev->last_g = g;
>> +    ldev->last_b = b;
>> +    ldev->last_brightness = brightness;
>> +    mutex_unlock(&ldev->dev_lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static enum led_brightness
>> +lamparray_led_brightness_get(struct led_classdev *cdev)
>> +{
>> +    struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
>> +    struct lamparray_device *ldev =
>> +        container_of_const(mc, struct lamparray_device, mc_cdev);
>> +
>> +    return ldev->last_brightness;
>> +}
>> +
>> +static int lamparray_register_led(struct lamparray_device *ldev)
>> +{
>> +    struct device *dev = &ldev->hdev->dev;
>> +    struct led_classdev *cdev = &ldev->mc_cdev.led_cdev;
>> +    int ret;
>> +
>> +    mutex_lock(&ldev->sysfs_lock);
>> +
>> +    if (ldev->led_registered) {
>> +        mutex_unlock(&ldev->sysfs_lock);
>> +        return 0;
>> +    }
>> +
>> +    if (!cdev->name) {
>> +        /* Fallback value */
>> +        const char *function = LED_FUNCTION_STATUS;
>> +
>> +        /* Some heuristics for choosing a better LED function. */
>> +        if (ldev->lamparray_kind_value == HID_LAMPARRAY_KIND_KEYBOARD)
>> +            function = LED_FUNCTION_KBD_BACKLIGHT;
>> +
>> +        cdev->name = kasprintf(GFP_KERNEL, "rgb:%s", function);
>> +        if (!cdev->name) {
>> +            mutex_unlock(&ldev->sysfs_lock);
>> +            return -ENOMEM;
>> +        }
>> +    }
>> +
>> +    mutex_lock(&ldev->dev_lock);
>> +    /* Setup */
>> +    cdev->max_brightness = ldev->max_brightness;
>> +    cdev->brightness_set_blocking = lamparray_led_brightness_set;
>> +    cdev->brightness_get = lamparray_led_brightness_get;
>> +    cdev->flags |= LED_RETAIN_AT_SHUTDOWN;
>> +
>> +    ldev->subleds[0].color_index = LED_COLOR_ID_RED;
>> +    ldev->subleds[0].max_intensity = ldev->max_r;
>> +    ldev->subleds[1].color_index = LED_COLOR_ID_GREEN;
>> +    ldev->subleds[1].max_intensity = ldev->max_g;
>> +    ldev->subleds[2].color_index = LED_COLOR_ID_BLUE;
>> +    ldev->subleds[2].max_intensity = ldev->max_b;
>> +
>> +    /* Set values */
>> +    ldev->subleds[0].intensity = ldev->last_r;
>> +    ldev->subleds[1].intensity = ldev->last_g;
>> +    ldev->subleds[2].intensity = ldev->last_b;
>> +    cdev->brightness = ldev->last_brightness;
>> +
>> +    ldev->mc_cdev.subled_info = ldev->subleds;
>> +    ldev->mc_cdev.num_colors = ARRAY_SIZE(ldev->subleds);
>> +
>> +    /* Ensure subled_info[].brightness matches intensity + brightness */
>> +    led_mc_calc_color_components(&ldev->mc_cdev, ldev->last_brightness);
>> +    mutex_unlock(&ldev->dev_lock);
>> +
>> +    ret = led_classdev_multicolor_register(dev, &ldev->mc_cdev);
>> +    if (ret) {
>> +        mutex_unlock(&ldev->sysfs_lock);
>> +        return ret;
>> +    }
>> +
>> +    ldev->led_registered = true;
>> +    mutex_unlock(&ldev->sysfs_lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static void lamparray_unregister_led(struct lamparray_device *ldev)
>> +{
>> +    bool was_registered;
>> +    struct led_classdev *cdev = &ldev->mc_cdev.led_cdev;
>> +
>> +    mutex_lock(&ldev->sysfs_lock);
>> +    was_registered = ldev->led_registered;
>> +    ldev->led_registered = false;
>> +
>> +    if (was_registered)
>> +        led_classdev_multicolor_unregister(&ldev->mc_cdev);
>> +
>> +    kfree(cdev->name);
>> +    cdev->name = NULL;
>> +
>> +    mutex_unlock(&ldev->sysfs_lock);
>> +}
>> +
>> +/* Sysfs */
>> +
>> +static struct lamparray_device *
>> +lamparray_ldev_from_sysfs_dev(struct device *dev)
>> +{
>> +    struct hid_device *hdev = to_hid_device(dev);
>> +
>> +    return xa_load(&lamparray_by_hdev, (unsigned long)hdev);
>> +}
>> +
>> +static ssize_t use_leds_uapi_show(struct device *dev,
>> +                  struct device_attribute *attr, char *buf)
>> +{
>> +    struct lamparray_device *ldev = lamparray_ldev_from_sysfs_dev(dev);
>> +
>> +    if (!ldev)
>> +        return -ENODEV;
>> +
>> +    return sysfs_emit(buf, "%d\n", ldev->use_leds_uapi);
>> +}
>> +
>> +static ssize_t use_leds_uapi_store(struct device *dev,
>> +                   struct device_attribute *attr,
>> +                   const char *buf, size_t count)
>> +{
>> +    struct lamparray_device *ldev = lamparray_ldev_from_sysfs_dev(dev);
>> +    int val;
>> +    int old_val;
>> +    int ret;
>> +
>> +    if (!ldev)
>> +        return -ENODEV;
>> +
>> +    ret = kstrtoint(buf, 0, &val);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (val != 0 && val != 1)
>> +        return -EINVAL;
>> +
>> +    mutex_lock(&ldev->dev_lock);
>> +    old_val = ldev->use_leds_uapi;
>> +
>> +    if (val == old_val) {
>> +        mutex_unlock(&ldev->dev_lock);
>> +        return count;
>> +    }
>> +
>> +    ldev->use_leds_uapi = val;
>> +    mutex_unlock(&ldev->dev_lock);
>> +
>> +    if (val == 1) {
>> +        ret = lamparray_register_led(ldev);
>> +        if (ret) {
>> +            mutex_lock(&ldev->dev_lock);
>> +            ldev->use_leds_uapi = old_val;
>> +            mutex_unlock(&ldev->dev_lock);
>> +            return ret;
>> +        }
>> +        ret = lamparray_restore_state(ldev);
>> +        if (ret) {
>> +            hid_err(ldev->hdev, "Could not restore state: %d\n", ret);
>> +            return ret;
>> +        }
>> +
>> +    } else {
>> +        lamparray_unregister_led(ldev);
>> +    }
>> +
>> +    return count;
>> +}
>> +static DEVICE_ATTR_RW(use_leds_uapi);
>> +
>> +static int lamparray_register_sysfs(struct lamparray_device *ldev)
>> +{
>> +    struct device *dev = &ldev->hdev->dev;
>> +    int ret;
>> +
>> +    ret = sysfs_create_file(&dev->kobj, &dev_attr_use_leds_uapi.attr);
>> +    if (ret)
>> +        hid_err(ldev->hdev,
>> +            "Failed to create lamparray sysfs group: %d\n", ret);
>> +
>> +    return ret;
>> +}
>> +
>> +static void lamparray_remove_sysfs(struct lamparray_device *ldev)
>> +{
>> +    sysfs_remove_file(&ldev->hdev->dev.kobj, &dev_attr_use_leds_uapi.attr);
>> +}
>> +
>> +/* Public API */
>> +
>> +bool lamparray_is_supported_device(struct hid_device *hdev)
>> +{
>> +    unsigned int i;
>> +
>> +    hid_dbg(hdev, "lamparray: walking %u collections\n",
>> +        hdev->maxcollection);
>> +
>> +    for (i = 0; i < hdev->maxcollection; i++) {
>> +        struct hid_collection *col = &hdev->collection[i];
>> +        u16 page = (col->usage & HID_USAGE_PAGE) >> 16;
>> +        u16 code = col->usage & HID_USAGE;
>> +
>> +        hid_dbg(hdev,
>> +            "lamparray:  collection[%u]: type=%u level=%u usage=0x%08x page=0x%04x code=0x%04x\n",
>> +            i, col->type, col->level, col->usage, page, code);
>> +
>> +        if (col->type == HID_COLLECTION_APPLICATION &&
>> +            page == HID_LIGHTING_ILLUMINATION_USAGE_PAGE &&
>> +            code == HID_APPLICATION_COLLECTION_USAGE_TYPE) {
>> +            return true;
>> +        }
>> +    }
>> +    return false;
>> +}
>> +EXPORT_SYMBOL_GPL(lamparray_is_supported_device);
>> +
>> +struct lamparray *
>> +lamparray_register(struct hid_device *hdev,
>> +           const struct lamparray_init_state *led_init_state)
>> +{
>> +    int ret;
>> +    struct lamparray *la;
>> +    struct lamparray_device *ldev;
>> +
>> +    if (!hdev)
>> +        return ERR_PTR(-ENODEV);
>> +
>> +    la = kzalloc_obj(*la, GFP_KERNEL);
>> +    if (!la)
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    ldev = &la->ldev;
>> +
>> +    mutex_init(&ldev->dev_lock);
>> +    mutex_init(&ldev->sysfs_lock);
>> +    ldev->hdev = hdev;
>> +    ldev->use_leds_uapi = true;
>> +    ldev->led_registered = false;
>> +
>> +    /* Make sure the driver lock gets released for probing. */
>> +    hid_device_io_start(hdev);
>> +
>> +    ret = lamparray_parse_update_report(ldev);
>> +    if (ret) {
>> +        hid_err(hdev, "No LampArray update report found: %d\n", ret);
>> +        goto err_free;
>> +    }
>> +
>> +    ret = lamparray_read_attributes_report(ldev);
>> +    if (ret) {
>> +        hid_err(hdev,
>> +            "Could not determine LampCount: %d\n",
>> +            ret);
>> +        goto err_free;
>> +    }
>> +
>> +    ret = lamparray_get_lamp_attributes(ldev);
>> +    if (ret) {
>> +        hid_err(hdev,
>> +            "Faulty device. Could not query lamp attributes.\n");
>> +        goto err_free;
>> +    }
>> +
>> +    /* Use black (all zeros) as default. */
>> +    if (led_init_state) {
>> +        ldev->last_r = min(led_init_state->r, ldev->max_r);
>> +        ldev->last_g = min(led_init_state->g, ldev->max_g);
>> +        ldev->last_b = min(led_init_state->b, ldev->max_b);
>> +        ldev->last_brightness = min(led_init_state->brightness,
>> +                        ldev->max_brightness);
>> +    }
>> +
>> +    ret = lamparray_register_led(ldev);
>> +    if (ret) {
>> +        hid_warn(hdev, "Failed to register LED UAPI: %d\n", ret);
>> +        ldev->use_leds_uapi = false;
>> +    }
>> +
>> +    ret = xa_err(xa_store(&lamparray_by_hdev, (unsigned long)hdev, ldev,
>> +                  GFP_KERNEL));
>> +    if (ret)
>> +        goto err_unregister_led;
>> +
>> +    ret = lamparray_register_sysfs(ldev);
>> +    if (ret)
>> +        goto err_xa_erase;
>> +
>> +    ret = lamparray_hw_set_autonomous(ldev, false);
>> +    if (ret) {
>> +        hid_err(hdev, "Could not disable autonomous mode: %d", ret);
>> +        goto err_remove_sysfs;
>> +    }
>> +
>> +    hid_info(hdev, "LampArray device registered\n");
>> +
>> +    ret = lamparray_restore_state(ldev);
>> +    if (ret) {
>> +        hid_err(hdev, "Failed to set default state: %d", ret);
>> +        goto err_remove_sysfs;
>> +    }
>> +
>> +    hid_device_io_stop(hdev);
>> +    return la;
>> +
>> +err_remove_sysfs:
>> +    lamparray_remove_sysfs(ldev);
>> +err_xa_erase:
>> +    xa_erase(&lamparray_by_hdev, (unsigned long)hdev);
>> +err_unregister_led:
>> +    lamparray_unregister_led(ldev);
>> +err_free:
>> +    hid_device_io_stop(hdev);
>> +    mutex_destroy(&ldev->dev_lock);
>> +    mutex_destroy(&ldev->sysfs_lock);
>> +    kfree(la);
>> +    return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(lamparray_register);
>> +
>> +void lamparray_unregister(struct lamparray *la)
>> +{
>> +    struct lamparray_device *ldev;
>> +
>> +    if (!la)
>> +        return;
>> +
>> +    ldev = &la->ldev;
>> +
>> +    lamparray_hw_set_autonomous(ldev, true);
>> +
>> +    lamparray_remove_sysfs(ldev);
>> +    xa_erase(&lamparray_by_hdev, (unsigned long)ldev->hdev);
>> +    lamparray_unregister_led(ldev);
>> +
>> +    mutex_destroy(&ldev->dev_lock);
>> +    mutex_destroy(&ldev->sysfs_lock);
>> +    kfree(la);
>> +}
>> +EXPORT_SYMBOL_GPL(lamparray_unregister);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Tim Guttzeit <tgu@tuxedocomputers.com>");
>> +MODULE_AUTHOR("Aaron Erhardt <aer@tuxedocomputers.com>");
>> +MODULE_DESCRIPTION("HID LampArray helper module (single-zone RGB)");
>> diff --git a/include/linux/hid-lamparray.h b/include/linux/hid-lamparray.h
>> new file mode 100644
>> index 000000000000..a77869728d12
>> --- /dev/null
>> +++ b/include/linux/hid-lamparray.h
>> @@ -0,0 +1,88 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +
>> +#ifndef _HID_LAMPARRAY_H
>> +#define _HID_LAMPARRAY_H
>> +
>> +#include <linux/hid.h>
>> +#include <linux/err.h>
>> +#include <linux/types.h>
>> +
>> +struct lamparray;
>> +
>> +/*
>> + * Optional initial LED state for lamparray_register().
>> + * Used to define the initial state of a LampArray's LEDs.
>> + */
>> +struct lamparray_init_state {
>> +    u8 r;
>> +    u8 g;
>> +    u8 b;
>> +    u8 brightness;
>> +};
>> +
>> +#if IS_ENABLED(CONFIG_HID_LAMPARRAY)
>> +
>> +/**
>> + * lamparray_is_supported_device() - check whether a HID device supports LampArray
>> + * @hdev: HID device to inspect
>> + *
>> + * Check whether the given HID device exposes a Lighting/LampArray application
>> + * collection as defined by the HID Lighting specification.
>> + *
>> + * This helper can be used by HID drivers to determine whether LampArray
>> + * functionality should be enabled for a device.
>> + *
>> + * Return: %true if LampArray support is detected, %false otherwise.
>> + */
>> +bool lamparray_is_supported_device(struct hid_device *hdev);
>> +
>> +/**
>> + * lamparray_register() - initialize LampArray support for a HID device
>> + * @hdev: HID device
>> + * @led_init_state: Optional LED state at init specification
>> + *
>> + * Allocate and initialize internal LampArray state for the given HID device.
>> + * The function parses required HID reports and fields and registers the
>> + * associated miscdevice and sysfs attributes.
>> + *
>> + * Registers a multicolor LED class device to expose the LampArray functionality
>> + * via the LED subsystem. If specified, the desired initial LED state is
>> + * applied. If led_init_state is NULL, a default state is applied (all LEDs off).
>> + *
>> + * Return: pointer to a LampArray handle on success, or ERR_PTR() on failure.
>> + */
>> +struct lamparray *lamparray_register(struct hid_device *hdev,
>> +                     const struct lamparray_init_state *led_init_state);
>> +
>> +/**
>> + * lamparray_unregister() - tear down LampArray support
>> + * @la: LampArray handle returned by lamparray_register()
>> + *
>> + * Remove all resources associated with a LampArray instance.
>> + *
>> + * This unregisters the LED class device (if present), removes the miscdevice
>> + * and sysfs interfaces and frees all internal state associated with @la.
>> + */
>> +void lamparray_unregister(struct lamparray *la);
>> +
>> +#else /* !CONFIG_HID_LAMPARRAY */
>> +
>> +static inline bool lamparray_is_supported_device(struct hid_device *hdev)
>> +{
>> +    return false;
>> +}
>> +
>> +static inline struct lamparray *
>> +lamparray_register(struct hid_device *hdev,
>> +           const struct lamparray_init_state *led_init_state)
>> +{
>> +    return ERR_PTR(-EOPNOTSUPP);
>> +}
>> +
>> +static inline void lamparray_unregister(struct lamparray *la)
>> +{
>> +}
>> +
>> +#endif /* CONFIG_HID_LAMPARRAY */
>> +
>> +#endif /* _HID_LAMPARRAY_H */

Re: [PATCH v5 0/2] HID: generic: add LampArray support via hid-lamparray helper

[Armin Wolf]

Am 03.09.26 um 09:35 schrieb Aaron Erhardt:

> Add a new hid-lamparray helper module and integrate it with the
> hid-generic driver.
>
> While more complex lamparray handling should be done in userspace via
> hidraw, providing a small module to add basic lamparray support makes it
> possible for userspace software to interact with lamparrays by simply
> using well-known APIs of the LED subsystem. One use-case would be to
> enable desktop environments to support keyboard backlight control out of
> the box for HID lamparray devices without having to implement the whole
> HID protocol themselves.
>
> This patch is based on previous discussions:
> https://lore.kernel.org/all/1fb08a74-62c7-4d0c-ba5d-648e23082dcb@tuxedocomputers.com/
>
> The helper provides basic support for devices exposing a
> Lighting/LampArray application collection (usage page 0x59) and
> registers a single-zone RGB LED representation via the LED
> subsystem.
>
> hid-generic now checks for LampArray support after hid_parse() and
> optionally registers a lamparray instance. Failures in the helper
> do not abort device probe to keep the driver logic otherwise unchanged.
>
> LampArray resources are released on driver remove.
>
> This commit was successfully tested on the Microsoft MacroPad reference
> implementation (https://github.com/microsoft/RP2040MacropadHidSample
> 1d6c3ad) and in combination with the tuxedo_nb04_wmi driver, albeit
> only fully functional with a recent fix posted to the LKML
> (https://lore.kernel.org/all/20260826081149.235487-2-aer@tuxedocomputers.com).

Nice work, it works on my ASUS Prime B650-Plus. However the behavior of the brightness
attribute is a bit strange:

- manually setting "brightness" does not change anything (max. is 1)
- setting RGB to "0 0 0" causes "brightness" to become 0
- setting RGB to a non-zero value causes "brightness" to become 1

Any idea why this happens? I can check if the same problems also exists under Windows,
if requested.

Thanks,
Armin Wolf

> v5:
> - Proper hardware detection (no quirks necessary anymore)
> - Add documentation for new sysfs knob
> - Pass limits of the device to sysfs (intesities & brightness)
> - More flexible Kconfig (use tristate)
> - Improved locking
> - Several memory leak and (de-)initialization fixes
> - Don't read current color values from hardware (the HID spec does not
>    offer this option)
> - Remove redundant report dump functionality
> v4:
> - Restrict CONFIG_HID_LAMPARRAY to built-in configurations only to fix
>    additional randconfig build errors
> v3:
> - Squash V1 and V2 into one patch
> v2:
> - Fix Kconfig to avoid build errors when LEDS_CLASS_MULTICOLOR is
>    disabled
>
> Aaron Erhardt (2):
>    HID: lamparray: add new LampArray helper module
>    HID: generic: add LampArray support via hid-lamparray helper
>
>   .../ABI/testing/sysfs-driver-hid-lamparray    |  16 +
>   drivers/hid/Kconfig                           |  18 +
>   drivers/hid/Makefile                          |   2 +
>   drivers/hid/hid-generic.c                     |  38 +
>   drivers/hid/hid-lamparray.c                   | 812 ++++++++++++++++++
>   include/linux/hid-lamparray.h                 |  88 ++
>   6 files changed, 974 insertions(+)
>   create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-lamparray
>   create mode 100644 drivers/hid/hid-lamparray.c
>   create mode 100644 include/linux/hid-lamparray.h
>

Re: [PATCH v5 1/2] HID: lamparray: add new LampArray helper module

[Armin Wolf]

Am 03.09.26 um 09:35 schrieb Aaron Erhardt:

> Add a new hid-lamparray helper module that provides basic support for
> devices exposing a Lighting/LampArray application collection (usage
> page 0x59) and registers a single-zone RGB LED representation via the
> LED subsystem.
>
> The module can be used as a library in HID drivers to add support for
> the HID LampArray protocol. While the API is quite basic as of now
> it could be extended in the future.
>
> Co-developed-by: Tim Guttzeit <tgu@tuxedocomputers.com>
> Signed-off-by: Tim Guttzeit <tgu@tuxedocomputers.com>
> Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
> ---
>   .../ABI/testing/sysfs-driver-hid-lamparray    |  16 +
>   drivers/hid/Kconfig                           |  17 +
>   drivers/hid/Makefile                          |   2 +
>   drivers/hid/hid-lamparray.c                   | 812 ++++++++++++++++++
>   include/linux/hid-lamparray.h                 |  88 ++
>   5 files changed, 935 insertions(+)
>   create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-lamparray
>   create mode 100644 drivers/hid/hid-lamparray.c
>   create mode 100644 include/linux/hid-lamparray.h
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-lamparray b/Documentation/ABI/testing/sysfs-driver-hid-lamparray
> new file mode 100644
> index 000000000000..795be6c4c368
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-hid-lamparray
> @@ -0,0 +1,16 @@
> +What:		/sys/bus/hid/devices/<bus>:<vid>:<pid>.<n>/use_leds_uapi
> +Date:		August 2026
> +KernelVersion:	7.3
> +Contact:	aer@tuxedocomputers.com
> +Description:
> +        If a driver uses the hid-lamparray module and a device supporting
> +        LampArray is found, one multicolor LED class device is registered under
> +        /sys/class/leds/rgb:<function> to expose the single-zone RGB control.
> +        Every device gets an incremental unique id.
> +
> +        Additionally, the use_leds_uapi sysfs attribute to control the LED class
> +        device is attached directly to the HID device at
> +        /sys/bus/hid/devices/<bus>:<vid>:<pid>.<n>/use_leds_uapi. Writing 0 to
> +        use_leds_uapi unregisters the LED class device. The last state is kept
> +        cached. Writing 1 registers it again and restores the cached state to
> +        hardware.
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index aa7fa11a0197..4afd80a67b39 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -92,6 +92,23 @@ config HID_GENERIC
>   
>   	If unsure, say Y.
>   
> +config HID_LAMPARRAY
> +	tristate "HID LampArray helper"
> +	depends on HID
> +	depends on LEDS_CLASS_MULTICOLOR
> +	default n
> +	help
> +	  Helper for HID devices exposing a Lighting/LampArray collection.
> +	  Treats LampArray devices as a single-zone device and exposes a sysfs
> +	  interface for changing color and intensity values. Also exposes a
> +	  sysfs flag to be disabled e.g. by a userspace driver.
> +
> +	  This can be used as library in existing drivers. The generic HID
> +	  driver is extended by default to handle lamp array devices if this
> +	  option is enabled.
> +
> +	  If unsure, say N.
> +
>   config HID_HAPTIC
>   	bool "Haptic touchpad support"
>   	default n
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 48a863b245ee..f95630fa8bd8 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -13,6 +13,8 @@ obj-$(CONFIG_UHID)		+= uhid.o
>   
>   obj-$(CONFIG_HID_GENERIC)	+= hid-generic.o
>   
> +obj-$(CONFIG_HID_LAMPARRAY)	+= hid-lamparray.o
> +
>   hid-$(CONFIG_HIDRAW)		+= hidraw.o
>   
>   hid-logitech-y		:= hid-lg.o
> diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
> new file mode 100644
> index 000000000000..9a438aa2d305
> --- /dev/null
> +++ b/drivers/hid/hid-lamparray.c
> @@ -0,0 +1,812 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * hid-lamparray.c - HID LampArray helper module (single-zone RGB)
> + *
> + * Helper module for HID drivers supporting devices that expose a Lighting and
> + * Illumination (LampArray) application collection (usage page 0x59).
> + *
> + * The module provides a minimal integration with the LED subsystem and treats
> + * the device as a single zone: all lamps share one RGB value and a global
> + * brightness level. It does not implement multi-zone layouts or hardware
> + * effects.
> + *
> + * If enabled and a device supporting LampArray is found, one multicolor LED
> + * class device is registered under /sys/class/leds/<HID-ID>:rgb:LampArray to
> + * expose the single-zone RGB control.
> + *
> + * The use_leds_uapi sysfs attribute is attached directly to the HID device
> + * under /sys/bus/hid/devices/<HID-ID>/use_leds_uapi. Writing 0 to use_leds_uapi
> + * unregisters the LED class device. The last state is kept cached. Writing 1
> + * registers it again and restores the cached state to hardware. State is cached
> + * as last known RGB + brightness.
> + *
> + * The module does not bind to devices on its own. Instead, a HID driver may
> + * query support via lamparray_is_supported_device() after hid_parse() and
> + * create an instance using lamparray_register().
> + *
> + * Copyright (C) 2026 Tim Guttzeit <tgu@tuxedocomputers.com>
> + * Copyright (C) 2026 Aaron Erhardt <aer@tuxedocomputers.com>
> + */
> +
> +#include <dt-bindings/leds/common.h>
> +#include <linux/limits.h>
> +#include <linux/minmax.h>
> +#include <linux/hid.h>
> +#include <linux/leds.h>
> +#include <linux/sysfs.h>
> +#include <linux/hid-lamparray.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/container_of.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/xarray.h>
> +
> +/* Constants */
> +
> +/* HID usages (LampArray, etc.) */
> +#define HID_LIGHTING_ILLUMINATION_USAGE_PAGE	0x0059
> +
> +/* HID usage types */
> +#define HID_APPLICATION_COLLECTION_USAGE_TYPE	0x0001
> +#define HID_LAMPARRAY_ATTRIBUTES_REPORT	0x0002
> +#define HID_LAMP_ATTRIBUTES_RESPONSE_REPORT	0x0022
> +#define HID_LAMP_RANGE_UPDATE_REPORT		0x0060
> +#define HID_LAMPARRAY_CONTROL_REPORT		0x0070
> +
> +/* HID attributes */
> +#define HID_LAIP_LAMP_COUNT			0x0003
> +#define HID_LAIP_LAMPARRAY_KIND			0x0007
> +#define HID_LAIP_RED_LEVEL_COUNT		0x0028
> +#define HID_LAIP_GREEN_LEVEL_COUNT		0x0029
> +#define HID_LAIP_BLUE_LEVEL_COUNT		0x002a
> +#define HID_LAIP_INTENSITY_LEVEL_COUNT		0x002b
> +#define HID_LAIP_RED_UPDATE_CHANNEL		0x0051
> +#define HID_LAIP_GREEN_UPDATE_CHANNEL		0x0052
> +#define HID_LAIP_BLUE_UPDATE_CHANNEL		0x0053
> +#define HID_LAIP_INTENSITY_UPDATE_CHANNEL	0x0054
> +#define HID_LAIP_LAMP_ID_START			0x0061
> +#define HID_LAIP_LAMP_ID_END			0x0062
> +#define HID_LAIP_AUTONOMOUS_MODE		0x0071
> +
> +/* LampArrayKind values */
> +#define HID_LAMPARRAY_KIND_KEYBOARD		0x0001
> +
> +/* Helper struct for fields and their indices */
> +struct hid_field_value {
> +	struct hid_field *field;
> +	int index;
> +};
> +
> +/* Helper struct for color fields */
> +struct lamparray_color_fields {
> +	struct hid_field_value red;
> +	struct hid_field_value green;
> +	struct hid_field_value blue;
> +	struct hid_field_value intensity;
> +};
> +
> +/* Device state */
> +struct lamparray_device {
> +	struct hid_device *hdev;
> +
> +	struct lamparray_color_fields color_levels;
> +	struct lamparray_color_fields color_update;
> +
> +	struct hid_field_value autonomous_field;
> +	struct hid_field_value range_start;
> +	struct hid_field_value range_end;
> +	struct hid_field_value lamp_count;
> +	struct hid_field_value lamparray_kind;
> +
> +	u16 lamp_count_value;
> +	u32 lamparray_kind_value;
> +
> +	struct led_classdev_mc mc_cdev;
> +	struct mc_subled subleds[3];
> +
> +	struct mutex dev_lock; /* Protects cached state and HID access */
> +	struct mutex sysfs_lock; /* Protects sysfs LED (de-)initialization */
> +
> +	u8 max_r;
> +	u8 max_g;
> +	u8 max_b;
> +	u8 max_brightness;
> +
> +	u8 last_r;
> +	u8 last_g;
> +	u8 last_b;
> +	u8 last_brightness;
> +
> +	bool use_leds_uapi;
> +	bool led_registered;
> +};
> +
> +/*
> + * Opaque handle exposed to callers via the header.
> + * Keep the actual state in lamparray_device, but return a stable pointer.
> + */
> +struct lamparray {
> +	struct lamparray_device ldev;
> +};
> +
> +/*
> + * Mapping for hid_device pointers to their lamparray data.
> + * Since there is not guarantee of how the driver using this library
> + * will use its drvdata, the only safe way to retrieve the lamparray
> + * data from a HID device pointer is using this mapping.
> + */
> +static DEFINE_XARRAY(lamparray_by_hdev);
> +
> +/* HID helper functions */
> +
> +static int get_field_value(struct hid_field_value *field_value)
> +{
> +	return field_value->field->value[field_value->index];
> +}
> +
> +static u8 get_field_value_as_u8(struct hid_field_value *field_value)
> +{
> +	return clamp_val(get_field_value(field_value), 0, U8_MAX);
> +}
> +
> +static void set_field_value(struct hid_field_value *field_value, int value)
> +{
> +	field_value->field->value[field_value->index] = value;
> +}
> +
> +static bool lamparray_color_fields_is_complete(struct lamparray_color_fields *color_fields)
> +{
> +	return color_fields->red.field && color_fields->green.field &&
> +	       color_fields->blue.field && color_fields->intensity.field;
> +}
> +
> +static int lamparray_read_attributes_report(struct lamparray_device *ldev)
> +{
> +	struct hid_device *hdev = ldev->hdev;
> +	struct hid_report *report;
> +
> +	if (!ldev->lamp_count.field) {
> +		hid_dbg(hdev, "No LampCount field found\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!ldev->lamparray_kind.field) {
> +		hid_dbg(hdev, "No LampArrayKind field found\n");
> +		return -ENODEV;
> +	}
> +
> +	report = ldev->lamp_count.field->report;
> +
> +	if (!report) {
> +		hid_dbg(hdev, "LampCount field has no report\n");
> +		return -ENODEV;
> +	}
> +
> +	mutex_lock(&ldev->dev_lock);
> +
> +	/* Update values */
> +	hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
> +	hid_hw_wait(hdev);
> +
> +	ldev->lamp_count_value = get_field_value(&ldev->lamp_count);
> +
> +	if (ldev->lamp_count_value == 0) {
> +		mutex_unlock(&ldev->dev_lock);
> +		hid_dbg(hdev, "LampCount is %d (invalid)\n", ldev->lamp_count_value);
> +		return -EINVAL;
> +	}
> +
> +	ldev->lamparray_kind_value = get_field_value(&ldev->lamparray_kind);
> +
> +	mutex_unlock(&ldev->dev_lock);
> +
> +	return 0;
> +}
> +
> +static int lamparray_parse_update_report(struct lamparray_device *ldev)
> +{
> +	struct hid_device *hdev = ldev->hdev;
> +	struct hid_report_enum *re;
> +	struct hid_report *report;
> +	struct hid_field *field;
> +	int i, j;
> +	int ret = 0;
> +
> +	mutex_lock(&ldev->dev_lock);
> +
> +	re = &hdev->report_enum[HID_FEATURE_REPORT];
> +
> +	list_for_each_entry(report, &re->report_list, list) {
> +		for (i = 0; i < report->maxfield; i++) {
> +			field = report->field[i];
> +			if (!field)
> +				continue;
> +
> +			if (!field->usage || !field->maxusage)
> +				continue;
> +
> +			for (j = 0; j < field->maxusage; j++) {
> +				u32 usage = field->usage[j].hid;
> +				u32 collection_idx = field->usage[j].collection_index;
> +				u32 collection_usage = hdev->collection[collection_idx].usage;
> +
> +				u16 page = (usage & HID_USAGE_PAGE) >> 16;
> +				u16 id = usage & HID_USAGE;
> +				u16 collection_usage_id = collection_usage & U16_MAX;
> +
> +				if (page != HID_LIGHTING_ILLUMINATION_USAGE_PAGE)
> +					continue;
> +
> +				if (collection_usage_id == HID_LAMPARRAY_ATTRIBUTES_REPORT) {
> +					switch (id) {
> +					case HID_LAIP_LAMP_COUNT:
> +						ldev->lamp_count.field = field;
> +						ldev->lamp_count.index = j;
> +						break;
> +					case HID_LAIP_LAMPARRAY_KIND:
> +						ldev->lamparray_kind.field = field;
> +						ldev->lamparray_kind.index = j;
> +						break;
> +					}
> +				} else if (collection_usage_id ==
> +					   HID_LAMP_ATTRIBUTES_RESPONSE_REPORT) {
> +					switch (id) {
> +					case HID_LAIP_RED_LEVEL_COUNT:
> +						ldev->color_levels.red.field = field;
> +						ldev->color_levels.red.index = j;
> +						break;
> +					case HID_LAIP_GREEN_LEVEL_COUNT:
> +						ldev->color_levels.green.field = field;
> +						ldev->color_levels.green.index = j;
> +						break;
> +					case HID_LAIP_BLUE_LEVEL_COUNT:
> +						ldev->color_levels.blue.field = field;
> +						ldev->color_levels.blue.index = j;
> +						break;
> +					case HID_LAIP_INTENSITY_LEVEL_COUNT:
> +						ldev->color_levels.intensity.field = field;
> +						ldev->color_levels.intensity.index = j;
> +						break;
> +					}
> +				} else if (collection_usage_id == HID_LAMP_RANGE_UPDATE_REPORT) {
> +					switch (id) {
> +					case HID_LAIP_RED_UPDATE_CHANNEL:
> +						ldev->color_update.red.field = field;
> +						ldev->color_update.red.index = j;
> +						break;
> +					case HID_LAIP_GREEN_UPDATE_CHANNEL:
> +						ldev->color_update.green.field = field;
> +						ldev->color_update.green.index = j;
> +						break;
> +					case HID_LAIP_BLUE_UPDATE_CHANNEL:
> +						ldev->color_update.blue.field = field;
> +						ldev->color_update.blue.index = j;
> +						break;
> +					case HID_LAIP_INTENSITY_UPDATE_CHANNEL:
> +						ldev->color_update.intensity.field = field;
> +						ldev->color_update.intensity.index = j;
> +						break;
> +					case HID_LAIP_LAMP_ID_START:
> +						ldev->range_start.field = field;
> +						ldev->range_start.index = j;
> +						break;
> +					case HID_LAIP_LAMP_ID_END:
> +						ldev->range_end.field = field;
> +						ldev->range_end.index = j;
> +						break;
> +					default:
> +						break;
> +					}
> +				} else if (collection_usage_id == HID_LAMPARRAY_CONTROL_REPORT &&
> +					   id == HID_LAIP_AUTONOMOUS_MODE) {
> +					ldev->autonomous_field.field = field;
> +					ldev->autonomous_field.index = j;
> +				}
> +			}
> +		}
> +	}
> +
> +	if (!ldev->autonomous_field.field ||
> +	    !lamparray_color_fields_is_complete(&ldev->color_update))
> +		ret = -ENODEV;
> +
> +	mutex_unlock(&ldev->dev_lock);
> +
> +	return ret;
> +}
> +
> +static int lamparray_hw_set_autonomous(struct lamparray_device *ldev,
> +				       bool enable)
> +{
> +	struct hid_device *hdev = ldev->hdev;
> +	struct hid_field *field = ldev->autonomous_field.field;
> +
> +	if (!field)
> +		return -ENODEV;
> +
> +	mutex_lock(&ldev->dev_lock);
> +
> +	set_field_value(&ldev->autonomous_field, !!enable);
> +
> +	hid_hw_request(hdev, field->report, HID_REQ_SET_REPORT);
> +	hid_hw_wait(hdev);
> +
> +	mutex_unlock(&ldev->dev_lock);
> +
> +	return 0;
> +}
> +
> +static int lamparray_hw_set_state(struct lamparray_device *ldev, u8 r, u8 g,
> +				  u8 b, u8 intensity)
> +{
> +	struct hid_device *hdev = ldev->hdev;
> +	struct hid_report *report;
> +
> +	if (!lamparray_color_fields_is_complete(&ldev->color_update))
> +		return -ENODEV;
> +
> +	if (ldev->range_start.field && ldev->range_end.field) {
> +		set_field_value(&ldev->range_start, 0);
> +		set_field_value(&ldev->range_end, ldev->lamp_count_value - 1);
> +	}
> +
> +	set_field_value(&ldev->color_update.red, r);
> +	set_field_value(&ldev->color_update.green, g);
> +	set_field_value(&ldev->color_update.blue, b);
> +	set_field_value(&ldev->color_update.intensity, intensity);
> +
> +	report = ldev->color_update.red.field->report;
> +	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> +	hid_hw_wait(hdev);
> +
> +	return 0;
> +}
> +
> +/*
> + * Simple helper to read the color information of the first lamp.
> + * This does not read the state of the whole lamp array since this driver only
> + * exposes one LED anyway, so one color is sufficient here for now.
> + */
> +static int lamparray_get_lamp_attributes(struct lamparray_device *ldev)
> +{
> +	struct hid_device *hdev = ldev->hdev;
> +	struct hid_report *report;
> +
> +	if (!lamparray_color_fields_is_complete(&ldev->color_levels))
> +		return -ENODEV;
> +
> +	/*
> +	 * Get value of any lamp.
> +	 */
> +	report = ldev->color_levels.red.field->report;
> +
> +	mutex_lock(&ldev->dev_lock);
> +
> +	hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
> +	hid_hw_wait(hdev);
> +
> +	ldev->max_r = get_field_value_as_u8(&ldev->color_levels.red);
> +	ldev->max_g = get_field_value_as_u8(&ldev->color_levels.green);
> +	ldev->max_b = get_field_value_as_u8(&ldev->color_levels.blue);
> +	ldev->max_brightness = get_field_value_as_u8(&ldev->color_levels.intensity);
> +
> +	mutex_unlock(&ldev->dev_lock);
> +
> +	return 0;
> +}
> +
> +/* Helper functions */
> +
> +static int lamparray_restore_state(struct lamparray_device *ldev)
> +{
> +	u8 r, g, b;
> +	int ret;
> +	enum led_brightness brightness;
> +
> +	mutex_lock(&ldev->dev_lock);
> +
> +	if (!ldev->use_leds_uapi) {
> +		mutex_unlock(&ldev->dev_lock);
> +		return 0;
> +	}
> +
> +	r = ldev->last_r;
> +	g = ldev->last_g;
> +	b = ldev->last_b;
> +	brightness = ldev->last_brightness;
> +
> +	ldev->mc_cdev.subled_info[0].intensity = r;
> +	ldev->mc_cdev.subled_info[1].intensity = g;
> +	ldev->mc_cdev.subled_info[2].intensity = b;
> +	ldev->mc_cdev.led_cdev.brightness = brightness;
> +
> +	led_mc_calc_color_components(&ldev->mc_cdev, brightness);

Hi,

since you are not using the brightness of the subleds, this call to
led_mc_calc_color_components() is unnecessary.

> +
> +	ret = lamparray_hw_set_state(ldev, r, g, b, brightness);
> +
> +	mutex_unlock(&ldev->dev_lock);
> +	return ret;
> +}
> +
> +/* LEDs API */
> +
> +static int lamparray_led_brightness_set(struct led_classdev *cdev,
> +					enum led_brightness brightness)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> +	struct lamparray_device *ldev =
> +		container_of_const(mc, struct lamparray_device, mc_cdev);
> +	u8 r, g, b;
> +	int ret;
> +
> +	/*
> +	 * Brightness is handled by the LampArray device if supported,
> +	 * so we can pass the raw intensity values.
> +	 */
> +	r = mc->subled_info[0].intensity;
> +	g = mc->subled_info[1].intensity;
> +	b = mc->subled_info[2].intensity;
> +
> +	mc->led_cdev.brightness = brightness;
> +	led_mc_calc_color_components(&ldev->mc_cdev, brightness);

Same here, the assignment of mc->led_cdev.brightness is also already preformed
by the LED core itself.

Thanks,
Armin Wolf

> +	mutex_lock(&ldev->dev_lock);
> +	ret = lamparray_hw_set_state(ldev, r, g, b, brightness);
> +	if (ret) {
> +		mutex_unlock(&ldev->dev_lock);
> +		hid_err(ldev->hdev, "Failed to send LampArray update: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ldev->last_r = r;
> +	ldev->last_g = g;
> +	ldev->last_b = b;
> +	ldev->last_brightness = brightness;
> +	mutex_unlock(&ldev->dev_lock);
> +
> +	return 0;
> +}
> +
> +static enum led_brightness
> +lamparray_led_brightness_get(struct led_classdev *cdev)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> +	struct lamparray_device *ldev =
> +	    container_of_const(mc, struct lamparray_device, mc_cdev);
> +
> +	return ldev->last_brightness;
> +}
> +
> +static int lamparray_register_led(struct lamparray_device *ldev)
> +{
> +	struct device *dev = &ldev->hdev->dev;
> +	struct led_classdev *cdev = &ldev->mc_cdev.led_cdev;
> +	int ret;
> +
> +	mutex_lock(&ldev->sysfs_lock);
> +
> +	if (ldev->led_registered) {
> +		mutex_unlock(&ldev->sysfs_lock);
> +		return 0;
> +	}
> +
> +	if (!cdev->name) {
> +		/* Fallback value */
> +		const char *function = LED_FUNCTION_STATUS;
> +
> +		/* Some heuristics for choosing a better LED function. */
> +		if (ldev->lamparray_kind_value == HID_LAMPARRAY_KIND_KEYBOARD)
> +			function = LED_FUNCTION_KBD_BACKLIGHT;
> +
> +		cdev->name = kasprintf(GFP_KERNEL, "rgb:%s", function);
> +		if (!cdev->name) {
> +			mutex_unlock(&ldev->sysfs_lock);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	mutex_lock(&ldev->dev_lock);
> +	/* Setup */
> +	cdev->max_brightness = ldev->max_brightness;
> +	cdev->brightness_set_blocking = lamparray_led_brightness_set;
> +	cdev->brightness_get = lamparray_led_brightness_get;
> +	cdev->flags |= LED_RETAIN_AT_SHUTDOWN;
> +
> +	ldev->subleds[0].color_index = LED_COLOR_ID_RED;
> +	ldev->subleds[0].max_intensity = ldev->max_r;
> +	ldev->subleds[1].color_index = LED_COLOR_ID_GREEN;
> +	ldev->subleds[1].max_intensity = ldev->max_g;
> +	ldev->subleds[2].color_index = LED_COLOR_ID_BLUE;
> +	ldev->subleds[2].max_intensity = ldev->max_b;
> +
> +	/* Set values */
> +	ldev->subleds[0].intensity = ldev->last_r;
> +	ldev->subleds[1].intensity = ldev->last_g;
> +	ldev->subleds[2].intensity = ldev->last_b;
> +	cdev->brightness = ldev->last_brightness;
> +
> +	ldev->mc_cdev.subled_info = ldev->subleds;
> +	ldev->mc_cdev.num_colors = ARRAY_SIZE(ldev->subleds);
> +
> +	/* Ensure subled_info[].brightness matches intensity + brightness */
> +	led_mc_calc_color_components(&ldev->mc_cdev, ldev->last_brightness);
> +	mutex_unlock(&ldev->dev_lock);
> +
> +	ret = led_classdev_multicolor_register(dev, &ldev->mc_cdev);
> +	if (ret) {
> +		mutex_unlock(&ldev->sysfs_lock);
> +		return ret;
> +	}
> +
> +	ldev->led_registered = true;
> +	mutex_unlock(&ldev->sysfs_lock);
> +
> +	return 0;
> +}
> +
> +static void lamparray_unregister_led(struct lamparray_device *ldev)
> +{
> +	bool was_registered;
> +	struct led_classdev *cdev = &ldev->mc_cdev.led_cdev;
> +
> +	mutex_lock(&ldev->sysfs_lock);
> +	was_registered = ldev->led_registered;
> +	ldev->led_registered = false;
> +
> +	if (was_registered)
> +		led_classdev_multicolor_unregister(&ldev->mc_cdev);
> +
> +	kfree(cdev->name);
> +	cdev->name = NULL;
> +
> +	mutex_unlock(&ldev->sysfs_lock);
> +}
> +
> +/* Sysfs */
> +
> +static struct lamparray_device *
> +lamparray_ldev_from_sysfs_dev(struct device *dev)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +
> +	return xa_load(&lamparray_by_hdev, (unsigned long)hdev);
> +}
> +
> +static ssize_t use_leds_uapi_show(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct lamparray_device *ldev = lamparray_ldev_from_sysfs_dev(dev);
> +
> +	if (!ldev)
> +		return -ENODEV;
> +
> +	return sysfs_emit(buf, "%d\n", ldev->use_leds_uapi);
> +}
> +
> +static ssize_t use_leds_uapi_store(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct lamparray_device *ldev = lamparray_ldev_from_sysfs_dev(dev);
> +	int val;
> +	int old_val;
> +	int ret;
> +
> +	if (!ldev)
> +		return -ENODEV;
> +
> +	ret = kstrtoint(buf, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val != 0 && val != 1)
> +		return -EINVAL;
> +
> +	mutex_lock(&ldev->dev_lock);
> +	old_val = ldev->use_leds_uapi;
> +
> +	if (val == old_val) {
> +		mutex_unlock(&ldev->dev_lock);
> +		return count;
> +	}
> +
> +	ldev->use_leds_uapi = val;
> +	mutex_unlock(&ldev->dev_lock);
> +
> +	if (val == 1) {
> +		ret = lamparray_register_led(ldev);
> +		if (ret) {
> +			mutex_lock(&ldev->dev_lock);
> +			ldev->use_leds_uapi = old_val;
> +			mutex_unlock(&ldev->dev_lock);
> +			return ret;
> +		}
> +		ret = lamparray_restore_state(ldev);
> +		if (ret) {
> +			hid_err(ldev->hdev, "Could not restore state: %d\n", ret);
> +			return ret;
> +		}
> +
> +	} else {
> +		lamparray_unregister_led(ldev);
> +	}
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(use_leds_uapi);
> +
> +static int lamparray_register_sysfs(struct lamparray_device *ldev)
> +{
> +	struct device *dev = &ldev->hdev->dev;
> +	int ret;
> +
> +	ret = sysfs_create_file(&dev->kobj, &dev_attr_use_leds_uapi.attr);
> +	if (ret)
> +		hid_err(ldev->hdev,
> +			"Failed to create lamparray sysfs group: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static void lamparray_remove_sysfs(struct lamparray_device *ldev)
> +{
> +	sysfs_remove_file(&ldev->hdev->dev.kobj, &dev_attr_use_leds_uapi.attr);
> +}
> +
> +/* Public API */
> +
> +bool lamparray_is_supported_device(struct hid_device *hdev)
> +{
> +	unsigned int i;
> +
> +	hid_dbg(hdev, "lamparray: walking %u collections\n",
> +		hdev->maxcollection);
> +
> +	for (i = 0; i < hdev->maxcollection; i++) {
> +		struct hid_collection *col = &hdev->collection[i];
> +		u16 page = (col->usage & HID_USAGE_PAGE) >> 16;
> +		u16 code = col->usage & HID_USAGE;
> +
> +		hid_dbg(hdev,
> +			"lamparray:  collection[%u]: type=%u level=%u usage=0x%08x page=0x%04x code=0x%04x\n",
> +			i, col->type, col->level, col->usage, page, code);
> +
> +		if (col->type == HID_COLLECTION_APPLICATION &&
> +		    page == HID_LIGHTING_ILLUMINATION_USAGE_PAGE &&
> +		    code == HID_APPLICATION_COLLECTION_USAGE_TYPE) {
> +			return true;
> +		}
> +	}
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(lamparray_is_supported_device);
> +
> +struct lamparray *
> +lamparray_register(struct hid_device *hdev,
> +		   const struct lamparray_init_state *led_init_state)
> +{
> +	int ret;
> +	struct lamparray *la;
> +	struct lamparray_device *ldev;
> +
> +	if (!hdev)
> +		return ERR_PTR(-ENODEV);
> +
> +	la = kzalloc_obj(*la, GFP_KERNEL);
> +	if (!la)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ldev = &la->ldev;
> +
> +	mutex_init(&ldev->dev_lock);
> +	mutex_init(&ldev->sysfs_lock);
> +	ldev->hdev = hdev;
> +	ldev->use_leds_uapi = true;
> +	ldev->led_registered = false;
> +
> +	/* Make sure the driver lock gets released for probing. */
> +	hid_device_io_start(hdev);
> +
> +	ret = lamparray_parse_update_report(ldev);
> +	if (ret) {
> +		hid_err(hdev, "No LampArray update report found: %d\n", ret);
> +		goto err_free;
> +	}
> +
> +	ret = lamparray_read_attributes_report(ldev);
> +	if (ret) {
> +		hid_err(hdev,
> +			"Could not determine LampCount: %d\n",
> +			ret);
> +		goto err_free;
> +	}
> +
> +	ret = lamparray_get_lamp_attributes(ldev);
> +	if (ret) {
> +		hid_err(hdev,
> +			"Faulty device. Could not query lamp attributes.\n");
> +		goto err_free;
> +	}
> +
> +	/* Use black (all zeros) as default. */
> +	if (led_init_state) {
> +		ldev->last_r = min(led_init_state->r, ldev->max_r);
> +		ldev->last_g = min(led_init_state->g, ldev->max_g);
> +		ldev->last_b = min(led_init_state->b, ldev->max_b);
> +		ldev->last_brightness = min(led_init_state->brightness,
> +					    ldev->max_brightness);
> +	}
> +
> +	ret = lamparray_register_led(ldev);
> +	if (ret) {
> +		hid_warn(hdev, "Failed to register LED UAPI: %d\n", ret);
> +		ldev->use_leds_uapi = false;
> +	}
> +
> +	ret = xa_err(xa_store(&lamparray_by_hdev, (unsigned long)hdev, ldev,
> +			      GFP_KERNEL));
> +	if (ret)
> +		goto err_unregister_led;
> +
> +	ret = lamparray_register_sysfs(ldev);
> +	if (ret)
> +		goto err_xa_erase;
> +
> +	ret = lamparray_hw_set_autonomous(ldev, false);
> +	if (ret) {
> +		hid_err(hdev, "Could not disable autonomous mode: %d", ret);
> +		goto err_remove_sysfs;
> +	}
> +
> +	hid_info(hdev, "LampArray device registered\n");
> +
> +	ret = lamparray_restore_state(ldev);
> +	if (ret) {
> +		hid_err(hdev, "Failed to set default state: %d", ret);
> +		goto err_remove_sysfs;
> +	}
> +
> +	hid_device_io_stop(hdev);
> +	return la;
> +
> +err_remove_sysfs:
> +	lamparray_remove_sysfs(ldev);
> +err_xa_erase:
> +	xa_erase(&lamparray_by_hdev, (unsigned long)hdev);
> +err_unregister_led:
> +	lamparray_unregister_led(ldev);
> +err_free:
> +	hid_device_io_stop(hdev);
> +	mutex_destroy(&ldev->dev_lock);
> +	mutex_destroy(&ldev->sysfs_lock);
> +	kfree(la);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(lamparray_register);
> +
> +void lamparray_unregister(struct lamparray *la)
> +{
> +	struct lamparray_device *ldev;
> +
> +	if (!la)
> +		return;
> +
> +	ldev = &la->ldev;
> +
> +	lamparray_hw_set_autonomous(ldev, true);
> +
> +	lamparray_remove_sysfs(ldev);
> +	xa_erase(&lamparray_by_hdev, (unsigned long)ldev->hdev);
> +	lamparray_unregister_led(ldev);
> +
> +	mutex_destroy(&ldev->dev_lock);
> +	mutex_destroy(&ldev->sysfs_lock);
> +	kfree(la);
> +}
> +EXPORT_SYMBOL_GPL(lamparray_unregister);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Tim Guttzeit <tgu@tuxedocomputers.com>");
> +MODULE_AUTHOR("Aaron Erhardt <aer@tuxedocomputers.com>");
> +MODULE_DESCRIPTION("HID LampArray helper module (single-zone RGB)");
> diff --git a/include/linux/hid-lamparray.h b/include/linux/hid-lamparray.h
> new file mode 100644
> index 000000000000..a77869728d12
> --- /dev/null
> +++ b/include/linux/hid-lamparray.h
> @@ -0,0 +1,88 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef _HID_LAMPARRAY_H
> +#define _HID_LAMPARRAY_H
> +
> +#include <linux/hid.h>
> +#include <linux/err.h>
> +#include <linux/types.h>
> +
> +struct lamparray;
> +
> +/*
> + * Optional initial LED state for lamparray_register().
> + * Used to define the initial state of a LampArray's LEDs.
> + */
> +struct lamparray_init_state {
> +	u8 r;
> +	u8 g;
> +	u8 b;
> +	u8 brightness;
> +};
> +
> +#if IS_ENABLED(CONFIG_HID_LAMPARRAY)
> +
> +/**
> + * lamparray_is_supported_device() - check whether a HID device supports LampArray
> + * @hdev: HID device to inspect
> + *
> + * Check whether the given HID device exposes a Lighting/LampArray application
> + * collection as defined by the HID Lighting specification.
> + *
> + * This helper can be used by HID drivers to determine whether LampArray
> + * functionality should be enabled for a device.
> + *
> + * Return: %true if LampArray support is detected, %false otherwise.
> + */
> +bool lamparray_is_supported_device(struct hid_device *hdev);
> +
> +/**
> + * lamparray_register() - initialize LampArray support for a HID device
> + * @hdev: HID device
> + * @led_init_state: Optional LED state at init specification
> + *
> + * Allocate and initialize internal LampArray state for the given HID device.
> + * The function parses required HID reports and fields and registers the
> + * associated miscdevice and sysfs attributes.
> + *
> + * Registers a multicolor LED class device to expose the LampArray functionality
> + * via the LED subsystem. If specified, the desired initial LED state is
> + * applied. If led_init_state is NULL, a default state is applied (all LEDs off).
> + *
> + * Return: pointer to a LampArray handle on success, or ERR_PTR() on failure.
> + */
> +struct lamparray *lamparray_register(struct hid_device *hdev,
> +				     const struct lamparray_init_state *led_init_state);
> +
> +/**
> + * lamparray_unregister() - tear down LampArray support
> + * @la: LampArray handle returned by lamparray_register()
> + *
> + * Remove all resources associated with a LampArray instance.
> + *
> + * This unregisters the LED class device (if present), removes the miscdevice
> + * and sysfs interfaces and frees all internal state associated with @la.
> + */
> +void lamparray_unregister(struct lamparray *la);
> +
> +#else /* !CONFIG_HID_LAMPARRAY */
> +
> +static inline bool lamparray_is_supported_device(struct hid_device *hdev)
> +{
> +	return false;
> +}
> +
> +static inline struct lamparray *
> +lamparray_register(struct hid_device *hdev,
> +		   const struct lamparray_init_state *led_init_state)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static inline void lamparray_unregister(struct lamparray *la)
> +{
> +}
> +
> +#endif /* CONFIG_HID_LAMPARRAY */
> +
> +#endif /* _HID_LAMPARRAY_H */

Re: [PATCH v5 1/2] HID: lamparray: add new LampArray helper module

[Aaron Erhardt]

Am 04.09.26 um 23:30 schrieb Armin Wolf:
> Am 03.09.26 um 09:35 schrieb Aaron Erhardt:
> 
>> Add a new hid-lamparray helper module that provides basic support for
>> devices exposing a Lighting/LampArray application collection (usage
>> page 0x59) and registers a single-zone RGB LED representation via the
>> LED subsystem.
>>
>> The module can be used as a library in HID drivers to add support for
>> the HID LampArray protocol. While the API is quite basic as of now
>> it could be extended in the future.
>>
>> Co-developed-by: Tim Guttzeit <tgu@tuxedocomputers.com>
>> Signed-off-by: Tim Guttzeit <tgu@tuxedocomputers.com>
>> Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
>> ---
>>   .../ABI/testing/sysfs-driver-hid-lamparray    |  16 +
>>   drivers/hid/Kconfig                           |  17 +
>>   drivers/hid/Makefile                          |   2 +
>>   drivers/hid/hid-lamparray.c                   | 812 ++++++++++++++++++
>>   include/linux/hid-lamparray.h                 |  88 ++
>>   5 files changed, 935 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-lamparray
>>   create mode 100644 drivers/hid/hid-lamparray.c
>>   create mode 100644 include/linux/hid-lamparray.h
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-lamparray b/Documentation/ABI/testing/sysfs-driver-hid-lamparray
>> new file mode 100644
>> index 000000000000..795be6c4c368
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-driver-hid-lamparray
>> @@ -0,0 +1,16 @@
>> +What:        /sys/bus/hid/devices/<bus>:<vid>:<pid>.<n>/use_leds_uapi
>> +Date:        August 2026
>> +KernelVersion:    7.3
>> +Contact:    aer@tuxedocomputers.com
>> +Description:
>> +        If a driver uses the hid-lamparray module and a device supporting
>> +        LampArray is found, one multicolor LED class device is registered under
>> +        /sys/class/leds/rgb:<function> to expose the single-zone RGB control.
>> +        Every device gets an incremental unique id.
>> +
>> +        Additionally, the use_leds_uapi sysfs attribute to control the LED class
>> +        device is attached directly to the HID device at
>> +        /sys/bus/hid/devices/<bus>:<vid>:<pid>.<n>/use_leds_uapi. Writing 0 to
>> +        use_leds_uapi unregisters the LED class device. The last state is kept
>> +        cached. Writing 1 registers it again and restores the cached state to
>> +        hardware.
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index aa7fa11a0197..4afd80a67b39 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -92,6 +92,23 @@ config HID_GENERIC
>>         If unsure, say Y.
>>   +config HID_LAMPARRAY
>> +    tristate "HID LampArray helper"
>> +    depends on HID
>> +    depends on LEDS_CLASS_MULTICOLOR
>> +    default n
>> +    help
>> +      Helper for HID devices exposing a Lighting/LampArray collection.
>> +      Treats LampArray devices as a single-zone device and exposes a sysfs
>> +      interface for changing color and intensity values. Also exposes a
>> +      sysfs flag to be disabled e.g. by a userspace driver.
>> +
>> +      This can be used as library in existing drivers. The generic HID
>> +      driver is extended by default to handle lamp array devices if this
>> +      option is enabled.
>> +
>> +      If unsure, say N.
>> +
>>   config HID_HAPTIC
>>       bool "Haptic touchpad support"
>>       default n
>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> index 48a863b245ee..f95630fa8bd8 100644
>> --- a/drivers/hid/Makefile
>> +++ b/drivers/hid/Makefile
>> @@ -13,6 +13,8 @@ obj-$(CONFIG_UHID)        += uhid.o
>>     obj-$(CONFIG_HID_GENERIC)    += hid-generic.o
>>   +obj-$(CONFIG_HID_LAMPARRAY)    += hid-lamparray.o
>> +
>>   hid-$(CONFIG_HIDRAW)        += hidraw.o
>>     hid-logitech-y        := hid-lg.o
>> diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
>> new file mode 100644
>> index 000000000000..9a438aa2d305
>> --- /dev/null
>> +++ b/drivers/hid/hid-lamparray.c
>> @@ -0,0 +1,812 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * hid-lamparray.c - HID LampArray helper module (single-zone RGB)
>> + *
>> + * Helper module for HID drivers supporting devices that expose a Lighting and
>> + * Illumination (LampArray) application collection (usage page 0x59).
>> + *
>> + * The module provides a minimal integration with the LED subsystem and treats
>> + * the device as a single zone: all lamps share one RGB value and a global
>> + * brightness level. It does not implement multi-zone layouts or hardware
>> + * effects.
>> + *
>> + * If enabled and a device supporting LampArray is found, one multicolor LED
>> + * class device is registered under /sys/class/leds/<HID-ID>:rgb:LampArray to
>> + * expose the single-zone RGB control.
>> + *
>> + * The use_leds_uapi sysfs attribute is attached directly to the HID device
>> + * under /sys/bus/hid/devices/<HID-ID>/use_leds_uapi. Writing 0 to use_leds_uapi
>> + * unregisters the LED class device. The last state is kept cached. Writing 1
>> + * registers it again and restores the cached state to hardware. State is cached
>> + * as last known RGB + brightness.
>> + *
>> + * The module does not bind to devices on its own. Instead, a HID driver may
>> + * query support via lamparray_is_supported_device() after hid_parse() and
>> + * create an instance using lamparray_register().
>> + *
>> + * Copyright (C) 2026 Tim Guttzeit <tgu@tuxedocomputers.com>
>> + * Copyright (C) 2026 Aaron Erhardt <aer@tuxedocomputers.com>
>> + */
>> +
>> +#include <dt-bindings/leds/common.h>
>> +#include <linux/limits.h>
>> +#include <linux/minmax.h>
>> +#include <linux/hid.h>
>> +#include <linux/leds.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/hid-lamparray.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/container_of.h>
>> +#include <linux/led-class-multicolor.h>
>> +#include <linux/xarray.h>
>> +
>> +/* Constants */
>> +
>> +/* HID usages (LampArray, etc.) */
>> +#define HID_LIGHTING_ILLUMINATION_USAGE_PAGE    0x0059
>> +
>> +/* HID usage types */
>> +#define HID_APPLICATION_COLLECTION_USAGE_TYPE    0x0001
>> +#define HID_LAMPARRAY_ATTRIBUTES_REPORT    0x0002
>> +#define HID_LAMP_ATTRIBUTES_RESPONSE_REPORT    0x0022
>> +#define HID_LAMP_RANGE_UPDATE_REPORT        0x0060
>> +#define HID_LAMPARRAY_CONTROL_REPORT        0x0070
>> +
>> +/* HID attributes */
>> +#define HID_LAIP_LAMP_COUNT            0x0003
>> +#define HID_LAIP_LAMPARRAY_KIND            0x0007
>> +#define HID_LAIP_RED_LEVEL_COUNT        0x0028
>> +#define HID_LAIP_GREEN_LEVEL_COUNT        0x0029
>> +#define HID_LAIP_BLUE_LEVEL_COUNT        0x002a
>> +#define HID_LAIP_INTENSITY_LEVEL_COUNT        0x002b
>> +#define HID_LAIP_RED_UPDATE_CHANNEL        0x0051
>> +#define HID_LAIP_GREEN_UPDATE_CHANNEL        0x0052
>> +#define HID_LAIP_BLUE_UPDATE_CHANNEL        0x0053
>> +#define HID_LAIP_INTENSITY_UPDATE_CHANNEL    0x0054
>> +#define HID_LAIP_LAMP_ID_START            0x0061
>> +#define HID_LAIP_LAMP_ID_END            0x0062
>> +#define HID_LAIP_AUTONOMOUS_MODE        0x0071
>> +
>> +/* LampArrayKind values */
>> +#define HID_LAMPARRAY_KIND_KEYBOARD        0x0001
>> +
>> +/* Helper struct for fields and their indices */
>> +struct hid_field_value {
>> +    struct hid_field *field;
>> +    int index;
>> +};
>> +
>> +/* Helper struct for color fields */
>> +struct lamparray_color_fields {
>> +    struct hid_field_value red;
>> +    struct hid_field_value green;
>> +    struct hid_field_value blue;
>> +    struct hid_field_value intensity;
>> +};
>> +
>> +/* Device state */
>> +struct lamparray_device {
>> +    struct hid_device *hdev;
>> +
>> +    struct lamparray_color_fields color_levels;
>> +    struct lamparray_color_fields color_update;
>> +
>> +    struct hid_field_value autonomous_field;
>> +    struct hid_field_value range_start;
>> +    struct hid_field_value range_end;
>> +    struct hid_field_value lamp_count;
>> +    struct hid_field_value lamparray_kind;
>> +
>> +    u16 lamp_count_value;
>> +    u32 lamparray_kind_value;
>> +
>> +    struct led_classdev_mc mc_cdev;
>> +    struct mc_subled subleds[3];
>> +
>> +    struct mutex dev_lock; /* Protects cached state and HID access */
>> +    struct mutex sysfs_lock; /* Protects sysfs LED (de-)initialization */
>> +
>> +    u8 max_r;
>> +    u8 max_g;
>> +    u8 max_b;
>> +    u8 max_brightness;
>> +
>> +    u8 last_r;
>> +    u8 last_g;
>> +    u8 last_b;
>> +    u8 last_brightness;
>> +
>> +    bool use_leds_uapi;
>> +    bool led_registered;
>> +};
>> +
>> +/*
>> + * Opaque handle exposed to callers via the header.
>> + * Keep the actual state in lamparray_device, but return a stable pointer.
>> + */
>> +struct lamparray {
>> +    struct lamparray_device ldev;
>> +};
>> +
>> +/*
>> + * Mapping for hid_device pointers to their lamparray data.
>> + * Since there is not guarantee of how the driver using this library
>> + * will use its drvdata, the only safe way to retrieve the lamparray
>> + * data from a HID device pointer is using this mapping.
>> + */
>> +static DEFINE_XARRAY(lamparray_by_hdev);
>> +
>> +/* HID helper functions */
>> +
>> +static int get_field_value(struct hid_field_value *field_value)
>> +{
>> +    return field_value->field->value[field_value->index];
>> +}
>> +
>> +static u8 get_field_value_as_u8(struct hid_field_value *field_value)
>> +{
>> +    return clamp_val(get_field_value(field_value), 0, U8_MAX);
>> +}
>> +
>> +static void set_field_value(struct hid_field_value *field_value, int value)
>> +{
>> +    field_value->field->value[field_value->index] = value;
>> +}
>> +
>> +static bool lamparray_color_fields_is_complete(struct lamparray_color_fields *color_fields)
>> +{
>> +    return color_fields->red.field && color_fields->green.field &&
>> +           color_fields->blue.field && color_fields->intensity.field;
>> +}
>> +
>> +static int lamparray_read_attributes_report(struct lamparray_device *ldev)
>> +{
>> +    struct hid_device *hdev = ldev->hdev;
>> +    struct hid_report *report;
>> +
>> +    if (!ldev->lamp_count.field) {
>> +        hid_dbg(hdev, "No LampCount field found\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    if (!ldev->lamparray_kind.field) {
>> +        hid_dbg(hdev, "No LampArrayKind field found\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    report = ldev->lamp_count.field->report;
>> +
>> +    if (!report) {
>> +        hid_dbg(hdev, "LampCount field has no report\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    mutex_lock(&ldev->dev_lock);
>> +
>> +    /* Update values */
>> +    hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
>> +    hid_hw_wait(hdev);
>> +
>> +    ldev->lamp_count_value = get_field_value(&ldev->lamp_count);
>> +
>> +    if (ldev->lamp_count_value == 0) {
>> +        mutex_unlock(&ldev->dev_lock);
>> +        hid_dbg(hdev, "LampCount is %d (invalid)\n", ldev->lamp_count_value);
>> +        return -EINVAL;
>> +    }
>> +
>> +    ldev->lamparray_kind_value = get_field_value(&ldev->lamparray_kind);
>> +
>> +    mutex_unlock(&ldev->dev_lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static int lamparray_parse_update_report(struct lamparray_device *ldev)
>> +{
>> +    struct hid_device *hdev = ldev->hdev;
>> +    struct hid_report_enum *re;
>> +    struct hid_report *report;
>> +    struct hid_field *field;
>> +    int i, j;
>> +    int ret = 0;
>> +
>> +    mutex_lock(&ldev->dev_lock);
>> +
>> +    re = &hdev->report_enum[HID_FEATURE_REPORT];
>> +
>> +    list_for_each_entry(report, &re->report_list, list) {
>> +        for (i = 0; i < report->maxfield; i++) {
>> +            field = report->field[i];
>> +            if (!field)
>> +                continue;
>> +
>> +            if (!field->usage || !field->maxusage)
>> +                continue;
>> +
>> +            for (j = 0; j < field->maxusage; j++) {
>> +                u32 usage = field->usage[j].hid;
>> +                u32 collection_idx = field->usage[j].collection_index;
>> +                u32 collection_usage = hdev->collection[collection_idx].usage;
>> +
>> +                u16 page = (usage & HID_USAGE_PAGE) >> 16;
>> +                u16 id = usage & HID_USAGE;
>> +                u16 collection_usage_id = collection_usage & U16_MAX;
>> +
>> +                if (page != HID_LIGHTING_ILLUMINATION_USAGE_PAGE)
>> +                    continue;
>> +
>> +                if (collection_usage_id == HID_LAMPARRAY_ATTRIBUTES_REPORT) {
>> +                    switch (id) {
>> +                    case HID_LAIP_LAMP_COUNT:
>> +                        ldev->lamp_count.field = field;
>> +                        ldev->lamp_count.index = j;
>> +                        break;
>> +                    case HID_LAIP_LAMPARRAY_KIND:
>> +                        ldev->lamparray_kind.field = field;
>> +                        ldev->lamparray_kind.index = j;
>> +                        break;
>> +                    }
>> +                } else if (collection_usage_id ==
>> +                       HID_LAMP_ATTRIBUTES_RESPONSE_REPORT) {
>> +                    switch (id) {
>> +                    case HID_LAIP_RED_LEVEL_COUNT:
>> +                        ldev->color_levels.red.field = field;
>> +                        ldev->color_levels.red.index = j;
>> +                        break;
>> +                    case HID_LAIP_GREEN_LEVEL_COUNT:
>> +                        ldev->color_levels.green.field = field;
>> +                        ldev->color_levels.green.index = j;
>> +                        break;
>> +                    case HID_LAIP_BLUE_LEVEL_COUNT:
>> +                        ldev->color_levels.blue.field = field;
>> +                        ldev->color_levels.blue.index = j;
>> +                        break;
>> +                    case HID_LAIP_INTENSITY_LEVEL_COUNT:
>> +                        ldev->color_levels.intensity.field = field;
>> +                        ldev->color_levels.intensity.index = j;
>> +                        break;
>> +                    }
>> +                } else if (collection_usage_id == HID_LAMP_RANGE_UPDATE_REPORT) {
>> +                    switch (id) {
>> +                    case HID_LAIP_RED_UPDATE_CHANNEL:
>> +                        ldev->color_update.red.field = field;
>> +                        ldev->color_update.red.index = j;
>> +                        break;
>> +                    case HID_LAIP_GREEN_UPDATE_CHANNEL:
>> +                        ldev->color_update.green.field = field;
>> +                        ldev->color_update.green.index = j;
>> +                        break;
>> +                    case HID_LAIP_BLUE_UPDATE_CHANNEL:
>> +                        ldev->color_update.blue.field = field;
>> +                        ldev->color_update.blue.index = j;
>> +                        break;
>> +                    case HID_LAIP_INTENSITY_UPDATE_CHANNEL:
>> +                        ldev->color_update.intensity.field = field;
>> +                        ldev->color_update.intensity.index = j;
>> +                        break;
>> +                    case HID_LAIP_LAMP_ID_START:
>> +                        ldev->range_start.field = field;
>> +                        ldev->range_start.index = j;
>> +                        break;
>> +                    case HID_LAIP_LAMP_ID_END:
>> +                        ldev->range_end.field = field;
>> +                        ldev->range_end.index = j;
>> +                        break;
>> +                    default:
>> +                        break;
>> +                    }
>> +                } else if (collection_usage_id == HID_LAMPARRAY_CONTROL_REPORT &&
>> +                       id == HID_LAIP_AUTONOMOUS_MODE) {
>> +                    ldev->autonomous_field.field = field;
>> +                    ldev->autonomous_field.index = j;
>> +                }
>> +            }
>> +        }
>> +    }
>> +
>> +    if (!ldev->autonomous_field.field ||
>> +        !lamparray_color_fields_is_complete(&ldev->color_update))
>> +        ret = -ENODEV;
>> +
>> +    mutex_unlock(&ldev->dev_lock);
>> +
>> +    return ret;
>> +}
>> +
>> +static int lamparray_hw_set_autonomous(struct lamparray_device *ldev,
>> +                       bool enable)
>> +{
>> +    struct hid_device *hdev = ldev->hdev;
>> +    struct hid_field *field = ldev->autonomous_field.field;
>> +
>> +    if (!field)
>> +        return -ENODEV;
>> +
>> +    mutex_lock(&ldev->dev_lock);
>> +
>> +    set_field_value(&ldev->autonomous_field, !!enable);
>> +
>> +    hid_hw_request(hdev, field->report, HID_REQ_SET_REPORT);
>> +    hid_hw_wait(hdev);
>> +
>> +    mutex_unlock(&ldev->dev_lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static int lamparray_hw_set_state(struct lamparray_device *ldev, u8 r, u8 g,
>> +                  u8 b, u8 intensity)
>> +{
>> +    struct hid_device *hdev = ldev->hdev;
>> +    struct hid_report *report;
>> +
>> +    if (!lamparray_color_fields_is_complete(&ldev->color_update))
>> +        return -ENODEV;
>> +
>> +    if (ldev->range_start.field && ldev->range_end.field) {
>> +        set_field_value(&ldev->range_start, 0);
>> +        set_field_value(&ldev->range_end, ldev->lamp_count_value - 1);
>> +    }
>> +
>> +    set_field_value(&ldev->color_update.red, r);
>> +    set_field_value(&ldev->color_update.green, g);
>> +    set_field_value(&ldev->color_update.blue, b);
>> +    set_field_value(&ldev->color_update.intensity, intensity);
>> +
>> +    report = ldev->color_update.red.field->report;
>> +    hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
>> +    hid_hw_wait(hdev);
>> +
>> +    return 0;
>> +}
>> +
>> +/*
>> + * Simple helper to read the color information of the first lamp.
>> + * This does not read the state of the whole lamp array since this driver only
>> + * exposes one LED anyway, so one color is sufficient here for now.
>> + */
>> +static int lamparray_get_lamp_attributes(struct lamparray_device *ldev)
>> +{
>> +    struct hid_device *hdev = ldev->hdev;
>> +    struct hid_report *report;
>> +
>> +    if (!lamparray_color_fields_is_complete(&ldev->color_levels))
>> +        return -ENODEV;
>> +
>> +    /*
>> +     * Get value of any lamp.
>> +     */
>> +    report = ldev->color_levels.red.field->report;
>> +
>> +    mutex_lock(&ldev->dev_lock);
>> +
>> +    hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
>> +    hid_hw_wait(hdev);
>> +
>> +    ldev->max_r = get_field_value_as_u8(&ldev->color_levels.red);
>> +    ldev->max_g = get_field_value_as_u8(&ldev->color_levels.green);
>> +    ldev->max_b = get_field_value_as_u8(&ldev->color_levels.blue);
>> +    ldev->max_brightness = get_field_value_as_u8(&ldev->color_levels.intensity);
>> +
>> +    mutex_unlock(&ldev->dev_lock);
>> +
>> +    return 0;
>> +}
>> +
>> +/* Helper functions */
>> +
>> +static int lamparray_restore_state(struct lamparray_device *ldev)
>> +{
>> +    u8 r, g, b;
>> +    int ret;
>> +    enum led_brightness brightness;
>> +
>> +    mutex_lock(&ldev->dev_lock);
>> +
>> +    if (!ldev->use_leds_uapi) {
>> +        mutex_unlock(&ldev->dev_lock);
>> +        return 0;
>> +    }
>> +
>> +    r = ldev->last_r;
>> +    g = ldev->last_g;
>> +    b = ldev->last_b;
>> +    brightness = ldev->last_brightness;
>> +
>> +    ldev->mc_cdev.subled_info[0].intensity = r;
>> +    ldev->mc_cdev.subled_info[1].intensity = g;
>> +    ldev->mc_cdev.subled_info[2].intensity = b;
>> +    ldev->mc_cdev.led_cdev.brightness = brightness;
>> +
>> +    led_mc_calc_color_components(&ldev->mc_cdev, brightness);
> 
> Hi,
> 
> since you are not using the brightness of the subleds, this call to
> led_mc_calc_color_components() is unnecessary.
> 

Thanks for pointing this out. I will remove it in the next iteration.

I was aware that this is not necessary for the LampArray functionality, but
assumed it would be useful for updating the values in the LED subsystem. I
somehow thought there would be a file with scaled values which would need 
to be updates sperately since the LampArray device will do the scaling by
itself. After checking, this was quite obviously wrong.
>> +
>> +    ret = lamparray_hw_set_state(ldev, r, g, b, brightness);
>> +
>> +    mutex_unlock(&ldev->dev_lock);
>> +    return ret;
>> +}
>> +
>> +/* LEDs API */
>> +
>> +static int lamparray_led_brightness_set(struct led_classdev *cdev,
>> +                    enum led_brightness brightness)
>> +{
>> +    struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
>> +    struct lamparray_device *ldev =
>> +        container_of_const(mc, struct lamparray_device, mc_cdev);
>> +    u8 r, g, b;
>> +    int ret;
>> +
>> +    /*
>> +     * Brightness is handled by the LampArray device if supported,
>> +     * so we can pass the raw intensity values.
>> +     */
>> +    r = mc->subled_info[0].intensity;
>> +    g = mc->subled_info[1].intensity;
>> +    b = mc->subled_info[2].intensity;
>> +
>> +    mc->led_cdev.brightness = brightness;
>> +    led_mc_calc_color_components(&ldev->mc_cdev, brightness);
> 
> Same here, the assignment of mc->led_cdev.brightness is also already preformed
> by the LED core itself.
> 
> Thanks,
> Armin Wolf
> 
>> +    mutex_lock(&ldev->dev_lock);
>> +    ret = lamparray_hw_set_state(ldev, r, g, b, brightness);
>> +    if (ret) {
>> +        mutex_unlock(&ldev->dev_lock);
>> +        hid_err(ldev->hdev, "Failed to send LampArray update: %d\n",
>> +            ret);
>> +        return ret;
>> +    }
>> +
>> +    ldev->last_r = r;
>> +    ldev->last_g = g;
>> +    ldev->last_b = b;
>> +    ldev->last_brightness = brightness;
>> +    mutex_unlock(&ldev->dev_lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static enum led_brightness
>> +lamparray_led_brightness_get(struct led_classdev *cdev)
>> +{
>> +    struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
>> +    struct lamparray_device *ldev =
>> +        container_of_const(mc, struct lamparray_device, mc_cdev);
>> +
>> +    return ldev->last_brightness;
>> +}
>> +
>> +static int lamparray_register_led(struct lamparray_device *ldev)
>> +{
>> +    struct device *dev = &ldev->hdev->dev;
>> +    struct led_classdev *cdev = &ldev->mc_cdev.led_cdev;
>> +    int ret;
>> +
>> +    mutex_lock(&ldev->sysfs_lock);
>> +
>> +    if (ldev->led_registered) {
>> +        mutex_unlock(&ldev->sysfs_lock);
>> +        return 0;
>> +    }
>> +
>> +    if (!cdev->name) {
>> +        /* Fallback value */
>> +        const char *function = LED_FUNCTION_STATUS;
>> +
>> +        /* Some heuristics for choosing a better LED function. */
>> +        if (ldev->lamparray_kind_value == HID_LAMPARRAY_KIND_KEYBOARD)
>> +            function = LED_FUNCTION_KBD_BACKLIGHT;
>> +
>> +        cdev->name = kasprintf(GFP_KERNEL, "rgb:%s", function);
>> +        if (!cdev->name) {
>> +            mutex_unlock(&ldev->sysfs_lock);
>> +            return -ENOMEM;
>> +        }
>> +    }
>> +
>> +    mutex_lock(&ldev->dev_lock);
>> +    /* Setup */
>> +    cdev->max_brightness = ldev->max_brightness;
>> +    cdev->brightness_set_blocking = lamparray_led_brightness_set;
>> +    cdev->brightness_get = lamparray_led_brightness_get;
>> +    cdev->flags |= LED_RETAIN_AT_SHUTDOWN;
>> +
>> +    ldev->subleds[0].color_index = LED_COLOR_ID_RED;
>> +    ldev->subleds[0].max_intensity = ldev->max_r;
>> +    ldev->subleds[1].color_index = LED_COLOR_ID_GREEN;
>> +    ldev->subleds[1].max_intensity = ldev->max_g;
>> +    ldev->subleds[2].color_index = LED_COLOR_ID_BLUE;
>> +    ldev->subleds[2].max_intensity = ldev->max_b;
>> +
>> +    /* Set values */
>> +    ldev->subleds[0].intensity = ldev->last_r;
>> +    ldev->subleds[1].intensity = ldev->last_g;
>> +    ldev->subleds[2].intensity = ldev->last_b;
>> +    cdev->brightness = ldev->last_brightness;
>> +
>> +    ldev->mc_cdev.subled_info = ldev->subleds;
>> +    ldev->mc_cdev.num_colors = ARRAY_SIZE(ldev->subleds);
>> +
>> +    /* Ensure subled_info[].brightness matches intensity + brightness */
>> +    led_mc_calc_color_components(&ldev->mc_cdev, ldev->last_brightness);
>> +    mutex_unlock(&ldev->dev_lock);
>> +
>> +    ret = led_classdev_multicolor_register(dev, &ldev->mc_cdev);
>> +    if (ret) {
>> +        mutex_unlock(&ldev->sysfs_lock);
>> +        return ret;
>> +    }
>> +
>> +    ldev->led_registered = true;
>> +    mutex_unlock(&ldev->sysfs_lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static void lamparray_unregister_led(struct lamparray_device *ldev)
>> +{
>> +    bool was_registered;
>> +    struct led_classdev *cdev = &ldev->mc_cdev.led_cdev;
>> +
>> +    mutex_lock(&ldev->sysfs_lock);
>> +    was_registered = ldev->led_registered;
>> +    ldev->led_registered = false;
>> +
>> +    if (was_registered)
>> +        led_classdev_multicolor_unregister(&ldev->mc_cdev);
>> +
>> +    kfree(cdev->name);
>> +    cdev->name = NULL;
>> +
>> +    mutex_unlock(&ldev->sysfs_lock);
>> +}
>> +
>> +/* Sysfs */
>> +
>> +static struct lamparray_device *
>> +lamparray_ldev_from_sysfs_dev(struct device *dev)
>> +{
>> +    struct hid_device *hdev = to_hid_device(dev);
>> +
>> +    return xa_load(&lamparray_by_hdev, (unsigned long)hdev);
>> +}
>> +
>> +static ssize_t use_leds_uapi_show(struct device *dev,
>> +                  struct device_attribute *attr, char *buf)
>> +{
>> +    struct lamparray_device *ldev = lamparray_ldev_from_sysfs_dev(dev);
>> +
>> +    if (!ldev)
>> +        return -ENODEV;
>> +
>> +    return sysfs_emit(buf, "%d\n", ldev->use_leds_uapi);
>> +}
>> +
>> +static ssize_t use_leds_uapi_store(struct device *dev,
>> +                   struct device_attribute *attr,
>> +                   const char *buf, size_t count)
>> +{
>> +    struct lamparray_device *ldev = lamparray_ldev_from_sysfs_dev(dev);
>> +    int val;
>> +    int old_val;
>> +    int ret;
>> +
>> +    if (!ldev)
>> +        return -ENODEV;
>> +
>> +    ret = kstrtoint(buf, 0, &val);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (val != 0 && val != 1)
>> +        return -EINVAL;
>> +
>> +    mutex_lock(&ldev->dev_lock);
>> +    old_val = ldev->use_leds_uapi;
>> +
>> +    if (val == old_val) {
>> +        mutex_unlock(&ldev->dev_lock);
>> +        return count;
>> +    }
>> +
>> +    ldev->use_leds_uapi = val;
>> +    mutex_unlock(&ldev->dev_lock);
>> +
>> +    if (val == 1) {
>> +        ret = lamparray_register_led(ldev);
>> +        if (ret) {
>> +            mutex_lock(&ldev->dev_lock);
>> +            ldev->use_leds_uapi = old_val;
>> +            mutex_unlock(&ldev->dev_lock);
>> +            return ret;
>> +        }
>> +        ret = lamparray_restore_state(ldev);
>> +        if (ret) {
>> +            hid_err(ldev->hdev, "Could not restore state: %d\n", ret);
>> +            return ret;
>> +        }
>> +
>> +    } else {
>> +        lamparray_unregister_led(ldev);
>> +    }
>> +
>> +    return count;
>> +}
>> +static DEVICE_ATTR_RW(use_leds_uapi);
>> +
>> +static int lamparray_register_sysfs(struct lamparray_device *ldev)
>> +{
>> +    struct device *dev = &ldev->hdev->dev;
>> +    int ret;
>> +
>> +    ret = sysfs_create_file(&dev->kobj, &dev_attr_use_leds_uapi.attr);
>> +    if (ret)
>> +        hid_err(ldev->hdev,
>> +            "Failed to create lamparray sysfs group: %d\n", ret);
>> +
>> +    return ret;
>> +}
>> +
>> +static void lamparray_remove_sysfs(struct lamparray_device *ldev)
>> +{
>> +    sysfs_remove_file(&ldev->hdev->dev.kobj, &dev_attr_use_leds_uapi.attr);
>> +}
>> +
>> +/* Public API */
>> +
>> +bool lamparray_is_supported_device(struct hid_device *hdev)
>> +{
>> +    unsigned int i;
>> +
>> +    hid_dbg(hdev, "lamparray: walking %u collections\n",
>> +        hdev->maxcollection);
>> +
>> +    for (i = 0; i < hdev->maxcollection; i++) {
>> +        struct hid_collection *col = &hdev->collection[i];
>> +        u16 page = (col->usage & HID_USAGE_PAGE) >> 16;
>> +        u16 code = col->usage & HID_USAGE;
>> +
>> +        hid_dbg(hdev,
>> +            "lamparray:  collection[%u]: type=%u level=%u usage=0x%08x page=0x%04x code=0x%04x\n",
>> +            i, col->type, col->level, col->usage, page, code);
>> +
>> +        if (col->type == HID_COLLECTION_APPLICATION &&
>> +            page == HID_LIGHTING_ILLUMINATION_USAGE_PAGE &&
>> +            code == HID_APPLICATION_COLLECTION_USAGE_TYPE) {
>> +            return true;
>> +        }
>> +    }
>> +    return false;
>> +}
>> +EXPORT_SYMBOL_GPL(lamparray_is_supported_device);
>> +
>> +struct lamparray *
>> +lamparray_register(struct hid_device *hdev,
>> +           const struct lamparray_init_state *led_init_state)
>> +{
>> +    int ret;
>> +    struct lamparray *la;
>> +    struct lamparray_device *ldev;
>> +
>> +    if (!hdev)
>> +        return ERR_PTR(-ENODEV);
>> +
>> +    la = kzalloc_obj(*la, GFP_KERNEL);
>> +    if (!la)
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    ldev = &la->ldev;
>> +
>> +    mutex_init(&ldev->dev_lock);
>> +    mutex_init(&ldev->sysfs_lock);
>> +    ldev->hdev = hdev;
>> +    ldev->use_leds_uapi = true;
>> +    ldev->led_registered = false;
>> +
>> +    /* Make sure the driver lock gets released for probing. */
>> +    hid_device_io_start(hdev);
>> +
>> +    ret = lamparray_parse_update_report(ldev);
>> +    if (ret) {
>> +        hid_err(hdev, "No LampArray update report found: %d\n", ret);
>> +        goto err_free;
>> +    }
>> +
>> +    ret = lamparray_read_attributes_report(ldev);
>> +    if (ret) {
>> +        hid_err(hdev,
>> +            "Could not determine LampCount: %d\n",
>> +            ret);
>> +        goto err_free;
>> +    }
>> +
>> +    ret = lamparray_get_lamp_attributes(ldev);
>> +    if (ret) {
>> +        hid_err(hdev,
>> +            "Faulty device. Could not query lamp attributes.\n");
>> +        goto err_free;
>> +    }
>> +
>> +    /* Use black (all zeros) as default. */
>> +    if (led_init_state) {
>> +        ldev->last_r = min(led_init_state->r, ldev->max_r);
>> +        ldev->last_g = min(led_init_state->g, ldev->max_g);
>> +        ldev->last_b = min(led_init_state->b, ldev->max_b);
>> +        ldev->last_brightness = min(led_init_state->brightness,
>> +                        ldev->max_brightness);
>> +    }
>> +
>> +    ret = lamparray_register_led(ldev);
>> +    if (ret) {
>> +        hid_warn(hdev, "Failed to register LED UAPI: %d\n", ret);
>> +        ldev->use_leds_uapi = false;
>> +    }
>> +
>> +    ret = xa_err(xa_store(&lamparray_by_hdev, (unsigned long)hdev, ldev,
>> +                  GFP_KERNEL));
>> +    if (ret)
>> +        goto err_unregister_led;
>> +
>> +    ret = lamparray_register_sysfs(ldev);
>> +    if (ret)
>> +        goto err_xa_erase;
>> +
>> +    ret = lamparray_hw_set_autonomous(ldev, false);
>> +    if (ret) {
>> +        hid_err(hdev, "Could not disable autonomous mode: %d", ret);
>> +        goto err_remove_sysfs;
>> +    }
>> +
>> +    hid_info(hdev, "LampArray device registered\n");
>> +
>> +    ret = lamparray_restore_state(ldev);
>> +    if (ret) {
>> +        hid_err(hdev, "Failed to set default state: %d", ret);
>> +        goto err_remove_sysfs;
>> +    }
>> +
>> +    hid_device_io_stop(hdev);
>> +    return la;
>> +
>> +err_remove_sysfs:
>> +    lamparray_remove_sysfs(ldev);
>> +err_xa_erase:
>> +    xa_erase(&lamparray_by_hdev, (unsigned long)hdev);
>> +err_unregister_led:
>> +    lamparray_unregister_led(ldev);
>> +err_free:
>> +    hid_device_io_stop(hdev);
>> +    mutex_destroy(&ldev->dev_lock);
>> +    mutex_destroy(&ldev->sysfs_lock);
>> +    kfree(la);
>> +    return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(lamparray_register);
>> +
>> +void lamparray_unregister(struct lamparray *la)
>> +{
>> +    struct lamparray_device *ldev;
>> +
>> +    if (!la)
>> +        return;
>> +
>> +    ldev = &la->ldev;
>> +
>> +    lamparray_hw_set_autonomous(ldev, true);
>> +
>> +    lamparray_remove_sysfs(ldev);
>> +    xa_erase(&lamparray_by_hdev, (unsigned long)ldev->hdev);
>> +    lamparray_unregister_led(ldev);
>> +
>> +    mutex_destroy(&ldev->dev_lock);
>> +    mutex_destroy(&ldev->sysfs_lock);
>> +    kfree(la);
>> +}
>> +EXPORT_SYMBOL_GPL(lamparray_unregister);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Tim Guttzeit <tgu@tuxedocomputers.com>");
>> +MODULE_AUTHOR("Aaron Erhardt <aer@tuxedocomputers.com>");
>> +MODULE_DESCRIPTION("HID LampArray helper module (single-zone RGB)");
>> diff --git a/include/linux/hid-lamparray.h b/include/linux/hid-lamparray.h
>> new file mode 100644
>> index 000000000000..a77869728d12
>> --- /dev/null
>> +++ b/include/linux/hid-lamparray.h
>> @@ -0,0 +1,88 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +
>> +#ifndef _HID_LAMPARRAY_H
>> +#define _HID_LAMPARRAY_H
>> +
>> +#include <linux/hid.h>
>> +#include <linux/err.h>
>> +#include <linux/types.h>
>> +
>> +struct lamparray;
>> +
>> +/*
>> + * Optional initial LED state for lamparray_register().
>> + * Used to define the initial state of a LampArray's LEDs.
>> + */
>> +struct lamparray_init_state {
>> +    u8 r;
>> +    u8 g;
>> +    u8 b;
>> +    u8 brightness;
>> +};
>> +
>> +#if IS_ENABLED(CONFIG_HID_LAMPARRAY)
>> +
>> +/**
>> + * lamparray_is_supported_device() - check whether a HID device supports LampArray
>> + * @hdev: HID device to inspect
>> + *
>> + * Check whether the given HID device exposes a Lighting/LampArray application
>> + * collection as defined by the HID Lighting specification.
>> + *
>> + * This helper can be used by HID drivers to determine whether LampArray
>> + * functionality should be enabled for a device.
>> + *
>> + * Return: %true if LampArray support is detected, %false otherwise.
>> + */
>> +bool lamparray_is_supported_device(struct hid_device *hdev);
>> +
>> +/**
>> + * lamparray_register() - initialize LampArray support for a HID device
>> + * @hdev: HID device
>> + * @led_init_state: Optional LED state at init specification
>> + *
>> + * Allocate and initialize internal LampArray state for the given HID device.
>> + * The function parses required HID reports and fields and registers the
>> + * associated miscdevice and sysfs attributes.
>> + *
>> + * Registers a multicolor LED class device to expose the LampArray functionality
>> + * via the LED subsystem. If specified, the desired initial LED state is
>> + * applied. If led_init_state is NULL, a default state is applied (all LEDs off).
>> + *
>> + * Return: pointer to a LampArray handle on success, or ERR_PTR() on failure.
>> + */
>> +struct lamparray *lamparray_register(struct hid_device *hdev,
>> +                     const struct lamparray_init_state *led_init_state);
>> +
>> +/**
>> + * lamparray_unregister() - tear down LampArray support
>> + * @la: LampArray handle returned by lamparray_register()
>> + *
>> + * Remove all resources associated with a LampArray instance.
>> + *
>> + * This unregisters the LED class device (if present), removes the miscdevice
>> + * and sysfs interfaces and frees all internal state associated with @la.
>> + */
>> +void lamparray_unregister(struct lamparray *la);
>> +
>> +#else /* !CONFIG_HID_LAMPARRAY */
>> +
>> +static inline bool lamparray_is_supported_device(struct hid_device *hdev)
>> +{
>> +    return false;
>> +}
>> +
>> +static inline struct lamparray *
>> +lamparray_register(struct hid_device *hdev,
>> +           const struct lamparray_init_state *led_init_state)
>> +{
>> +    return ERR_PTR(-EOPNOTSUPP);
>> +}
>> +
>> +static inline void lamparray_unregister(struct lamparray *la)
>> +{
>> +}
>> +
>> +#endif /* CONFIG_HID_LAMPARRAY */
>> +
>> +#endif /* _HID_LAMPARRAY_H */
> 

Re: [PATCH v5 0/2] HID: generic: add LampArray support via hid-lamparray helper

[Aaron Erhardt]

Am 04.09.26 um 22:49 schrieb Armin Wolf:
> Am 03.09.26 um 09:35 schrieb Aaron Erhardt:
> 
>> Add a new hid-lamparray helper module and integrate it with the
>> hid-generic driver.
>>
>> While more complex lamparray handling should be done in userspace via
>> hidraw, providing a small module to add basic lamparray support makes it
>> possible for userspace software to interact with lamparrays by simply
>> using well-known APIs of the LED subsystem. One use-case would be to
>> enable desktop environments to support keyboard backlight control out of
>> the box for HID lamparray devices without having to implement the whole
>> HID protocol themselves.
>>
>> This patch is based on previous discussions:
>> https://lore.kernel.org/all/1fb08a74-62c7-4d0c-ba5d-648e23082dcb@tuxedocomputers.com/
>>
>> The helper provides basic support for devices exposing a
>> Lighting/LampArray application collection (usage page 0x59) and
>> registers a single-zone RGB LED representation via the LED
>> subsystem.
>>
>> hid-generic now checks for LampArray support after hid_parse() and
>> optionally registers a lamparray instance. Failures in the helper
>> do not abort device probe to keep the driver logic otherwise unchanged.
>>
>> LampArray resources are released on driver remove.
>>
>> This commit was successfully tested on the Microsoft MacroPad reference
>> implementation (https://github.com/microsoft/RP2040MacropadHidSample
>> 1d6c3ad) and in combination with the tuxedo_nb04_wmi driver, albeit
>> only fully functional with a recent fix posted to the LKML
>> (https://lore.kernel.org/all/20260826081149.235487-2-aer@tuxedocomputers.com).
> 
> Nice work, it works on my ASUS Prime B650-Plus. However the behavior of the brightness
> attribute is a bit strange:
> 
> - manually setting "brightness" does not change anything (max. is 1)
> - setting RGB to "0 0 0" causes "brightness" to become 0
> - setting RGB to a non-zero value causes "brightness" to become 1
> 
> Any idea why this happens? I can check if the same problems also exists under Windows,
> if requested.
> 
> Thanks,
> Armin Wolf
> 

It is completely normal for LampArray devices to only offer two brightness
values (1 and 0) for turning the whole LED on and off. Since a lot of
userspace software seems to never use brightness (it is more convenient to
adjust the RGB channels directly), this was not even properly implemented in
the MacropadHidSample until recently:
https://github.com/microsoft/RP2040MacropadHidSample/commit/cfc29120a3910c5772976da29ecd57392dfd44d6

Thus, I think it is likely, that the implementation is broken and simply
ignores brightness. The driver just forwards this to the device.

However, the RGB values (aka. multi_intensity) should not interfere with the
brightness. Yet, I wasn't able to reproduce this on the Macropad. Capping the
brightness to 1 is normal on the other hand, at least if that's the
maxIntensity reported by your device.

So the only really odd thing for me would be the RGB values influencing the
brightness. Please provide more detailed feedback if you can since I can't
reproduce this on the hardware available to me.

If you want to investigate the LampArray capabilities of your device, you're
probably better off with userspace tooling like my lampctl fork:
https://github.com/tuxedo-aer/lampctl

You can adjust the hardcoded brightness here to see whether your device honors
the brightness value or not:
https://github.com/tuxedo-aer/lampctl/blob/main/crates/lamparray/src/hid.rs#L36

>> v5:
>> - Proper hardware detection (no quirks necessary anymore)
>> - Add documentation for new sysfs knob
>> - Pass limits of the device to sysfs (intesities & brightness)
>> - More flexible Kconfig (use tristate)
>> - Improved locking
>> - Several memory leak and (de-)initialization fixes
>> - Don't read current color values from hardware (the HID spec does not
>>    offer this option)
>> - Remove redundant report dump functionality
>> v4:
>> - Restrict CONFIG_HID_LAMPARRAY to built-in configurations only to fix
>>    additional randconfig build errors
>> v3:
>> - Squash V1 and V2 into one patch
>> v2:
>> - Fix Kconfig to avoid build errors when LEDS_CLASS_MULTICOLOR is
>>    disabled
>>
>> Aaron Erhardt (2):
>>    HID: lamparray: add new LampArray helper module
>>    HID: generic: add LampArray support via hid-lamparray helper
>>
>>   .../ABI/testing/sysfs-driver-hid-lamparray    |  16 +
>>   drivers/hid/Kconfig                           |  18 +
>>   drivers/hid/Makefile                          |   2 +
>>   drivers/hid/hid-generic.c                     |  38 +
>>   drivers/hid/hid-lamparray.c                   | 812 ++++++++++++++++++
>>   include/linux/hid-lamparray.h                 |  88 ++
>>   6 files changed, 974 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-lamparray
>>   create mode 100644 drivers/hid/hid-lamparray.c
>>   create mode 100644 include/linux/hid-lamparray.h
>>
> 

[PATCH 0/4] HID: lamparray: fixes from testing on Acer Predator PT14-52T

[Cristian Mazzotta]

These are four fixes on top of the v5 series, posted 2026-09-03:
https://lore.kernel.org/linux-input/20260903073602.3815258-1-aer@tuxedocomputers.com/

Tested on an Acer Predator PT14-52T, which has two LampArray devices: a
USB keyboard (05AF:767A) and an I2C ENE controller (0CF2:5130).

With v5 as posted, the keyboard does not probe. LampCount reads back 0,
and once that is fixed the level counts read back 0 as well, which
divides by zero in led_mc_calc_color_components() during probe and takes
the USB hub worker down with it. Patches 1 and 2 cover the reads and the
validation; this is the same problem Aaron and Werner discussed in this
thread, with a backtrace from hardware that hits it.

Patch 3 makes use_leds_uapi transfer control in both directions.
Autonomous mode is currently only set at probe, so writing 1 re-registers
the LED class device without taking the hardware back, and updates sent
afterwards are ignored.

Patch 4 blanks the lamps across suspend. On this machine, the lighting
accounts for 9.21W of the 12.35W s2idle power draw against 3.14W with
the lamps blanked from this driver, so this is most of the suspend power
on a laptop with RGB. This was also tested with one of the 2 lights on
the ENE controller still on; this patch does not control both zones.

Please treat these as input for v6 rather than a separate series;
squashing them in is fine by me, and I will rebase and retest against v6
when it arrives.

Two things I have not fixed:

- The ENE controller exposes two LampArrayAttributesReport collections.
  lamparray_parse_update_report() has no first-match guard, so the second
  overwrites the first and only one zone is claimed; the other stays in
  firmware control. This is the multi-zone question from Werner's reply.
  I really believe that multi-zone support should exist, but I have not
  included it because I haven't started it; uni is taking away a lot of
  my time currently.

- hid_device_io_start() in lamparray_register() may no longer be needed:
  hid_hw_raw_request() is synchronous and does not go through
  hid_input_report(), so it does not need driver_input_lock released.
  That would also address the second [High] item from the Sashiko review.

Cristian Mazzotta (4):
  HID: lamparray: read attribute reports synchronously
  HID: lamparray: raise log level of fatal probe errors
  HID: lamparray: transfer control when use_leds_uapi changes
  HID: lamparray: blank lamps across suspend and restore on resume

 drivers/hid/hid-generic.c     |  27 ++++++
 drivers/hid/hid-lamparray.c   | 162 +++++++++++++++++++++++++++++-----
 include/linux/hid-lamparray.h |  35 ++++++++
 3 files changed, 200 insertions(+), 24 deletions(-)


base-commit: 9b298109e37e5caf4b6800198c4907a5a6bf00ae
-- 
2.55.0

[PATCH 1/4] HID: lamparray: read attribute reports synchronously

[Cristian Mazzotta]

lamparray_read_attributes_report() and lamparray_get_lamp_attributes()
use hid_hw_request(HID_REQ_GET_REPORT) followed by hid_hw_wait(), then
read the results out of field->value[]. hid_hw_request() is asynchronous
and hid_hw_wait() only clears the output queue, so the values are still
read before the transfer has completed.

On an Acer Predator PT14-52T (USB keyboard 05AF:767A), this returns zero
for LampCount, and then zero for red, green, blue, and intensity counts.
The last one is evil: max_brightness is passed to
led_mc_calc_color_components() as a divisor during
lamparray_register_led(), resulting in a divide by zero during probe:

    Oops: divide error: 0000 [#1] SMP NOPTI
    RIP: 0010:led_mc_calc_color_components+0x58/0x70
    Call Trace:
        lamparray_register_led+0x119/0x1e0
        lamparray_register+0x502/0x820
        hid_generic_probe+0x5e/0xc0

The fault kills the kworker running hub_event() while it holds the USB
and HID device locks, which stalls further probing on that bus.

Use hid_hw_raw_request() with a hid_report_len()-sized buffer and hand
the result to hid_report_raw_event() so the HID core parses it into
field->value[] before the values are read.

Validate the level counts after reading them and fail with -EINVAL if
any is zero, so a device reporting no levels cannot reach
led_mc_calc_color_components() at all. The four level fields are also
required to share one report, since a single GET_REPORT is used to
fetch them.

Signed-off-by: Cristian Mazzotta <cmmazzo@icloud.com>
---
 drivers/hid/hid-lamparray.c | 73 ++++++++++++++++++++++++++++++-------
 1 file changed, 59 insertions(+), 14 deletions(-)

diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
index 9a438aa2d305..f169929aecd6 100644
--- a/drivers/hid/hid-lamparray.c
+++ b/drivers/hid/hid-lamparray.c
@@ -164,6 +164,9 @@ static int lamparray_read_attributes_report(struct lamparray_device *ldev)
 {
 	struct hid_device *hdev = ldev->hdev;
 	struct hid_report *report;
+	int ret;
+	u8 *buf;
+	size_t len;
 
 	if (!ldev->lamp_count.field) {
 		hid_dbg(hdev, "No LampCount field found\n");
@@ -182,25 +185,37 @@ static int lamparray_read_attributes_report(struct lamparray_device *ldev)
 		return -ENODEV;
 	}
 
+	len = hid_report_len(report);
+	buf = kmalloc(len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
 	mutex_lock(&ldev->dev_lock);
 
 	/* Update values */
-	hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
-	hid_hw_wait(hdev);
+	ret = hid_hw_raw_request(hdev, report->id, buf, len,
+		HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+	if (ret < 0) {
+		hid_dbg(hdev, "Failed to get LampCount value from device: %d\n", ret);
+		goto out;
+	}
 
-	ldev->lamp_count_value = get_field_value(&ldev->lamp_count);
+	hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf, len, ret, 0);
 
-	if (ldev->lamp_count_value == 0) {
-		mutex_unlock(&ldev->dev_lock);
-		hid_dbg(hdev, "LampCount is %d (invalid)\n", ldev->lamp_count_value);
-		return -EINVAL;
+	ldev->lamp_count_value = get_field_value(&ldev->lamp_count);
+	if (!ldev->lamp_count_value) {
+		hid_dbg(hdev, "LampCount is 0 (invalid)\n");
+		ret = -EINVAL;
+		goto out;
 	}
 
 	ldev->lamparray_kind_value = get_field_value(&ldev->lamparray_kind);
+	ret = 0;
 
+out:
 	mutex_unlock(&ldev->dev_lock);
-
-	return 0;
+	kfree(buf);
+	return ret;
 }
 
 static int lamparray_parse_update_report(struct lamparray_device *ldev)
@@ -371,28 +386,58 @@ static int lamparray_get_lamp_attributes(struct lamparray_device *ldev)
 {
 	struct hid_device *hdev = ldev->hdev;
 	struct hid_report *report;
+	int ret;
+	u8 *buf;
+	size_t len;
 
 	if (!lamparray_color_fields_is_complete(&ldev->color_levels))
 		return -ENODEV;
 
 	/*
-	 * Get value of any lamp.
+	 * All four fields must share the same report since the
+	 * attributes are fetched with a single GET_REPORT below.
 	 */
 	report = ldev->color_levels.red.field->report;
+	if (!report ||
+		ldev->color_levels.green.field->report != report ||
+		ldev->color_levels.blue.field->report != report ||
+		ldev->color_levels.intensity.field->report != report)
+		return -ENODEV;
+
+	len = hid_report_len(report);
+	buf = kmalloc(len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
 
 	mutex_lock(&ldev->dev_lock);
 
-	hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
-	hid_hw_wait(hdev);
+	/* Update values */
+	ret = hid_hw_raw_request(hdev, report->id, buf, len,
+		HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+	if (ret < 0) {
+		hid_dbg(hdev, "Failed to read LampAttributesResponseReport: %d\n", ret);
+		goto out;
+	}
+
+	hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf, len, ret, 0);
 
 	ldev->max_r = get_field_value_as_u8(&ldev->color_levels.red);
 	ldev->max_g = get_field_value_as_u8(&ldev->color_levels.green);
 	ldev->max_b = get_field_value_as_u8(&ldev->color_levels.blue);
 	ldev->max_brightness = get_field_value_as_u8(&ldev->color_levels.intensity);
 
-	mutex_unlock(&ldev->dev_lock);
+	if (!ldev->max_r || !ldev->max_g || !ldev->max_b || !ldev->max_brightness) {
+		hid_dbg(hdev, "LampArray device has no color levels\n");
+		ret = -EINVAL;
+		goto out;
+	}
 
-	return 0;
+	ret = 0;
+
+out:
+	mutex_unlock(&ldev->dev_lock);
+	kfree(buf);
+	return ret;
 }
 
 /* Helper functions */
-- 
2.55.0

[PATCH 2/4] HID: lamparray: raise log level of fatal probe errors

[Cristian Mazzotta]

The failure paths in lamparray_read_attributes_report() and
lamparray_get_lamp_attributes() use hid_dbg(), so on a device that
cannot be driven the driver declines to register with no output unless
dynamic debug is enabled for the module.

Use hid_warn() for these, since they leave the device without LED
support and the reason is useful to anyone diagnosing why a LampArray
device did not appear under /sys/class/leds.

Signed-off-by: Cristian Mazzotta <cmmazzo@icloud.com>
---
 drivers/hid/hid-lamparray.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
index f169929aecd6..70c596f292a2 100644
--- a/drivers/hid/hid-lamparray.c
+++ b/drivers/hid/hid-lamparray.c
@@ -169,19 +169,19 @@ static int lamparray_read_attributes_report(struct lamparray_device *ldev)
 	size_t len;
 
 	if (!ldev->lamp_count.field) {
-		hid_dbg(hdev, "No LampCount field found\n");
+		hid_warn(hdev, "No LampCount field found\n");
 		return -ENODEV;
 	}
 
 	if (!ldev->lamparray_kind.field) {
-		hid_dbg(hdev, "No LampArrayKind field found\n");
+		hid_warn(hdev, "No LampArrayKind field found\n");
 		return -ENODEV;
 	}
 
 	report = ldev->lamp_count.field->report;
 
 	if (!report) {
-		hid_dbg(hdev, "LampCount field has no report\n");
+		hid_warn(hdev, "LampCount field has no report\n");
 		return -ENODEV;
 	}
 
@@ -196,7 +196,7 @@ static int lamparray_read_attributes_report(struct lamparray_device *ldev)
 	ret = hid_hw_raw_request(hdev, report->id, buf, len,
 		HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
 	if (ret < 0) {
-		hid_dbg(hdev, "Failed to get LampCount value from device: %d\n", ret);
+		hid_warn(hdev, "Failed to get LampCount value from device: %d\n", ret);
 		goto out;
 	}
 
@@ -204,7 +204,7 @@ static int lamparray_read_attributes_report(struct lamparray_device *ldev)
 
 	ldev->lamp_count_value = get_field_value(&ldev->lamp_count);
 	if (!ldev->lamp_count_value) {
-		hid_dbg(hdev, "LampCount is 0 (invalid)\n");
+		hid_warn(hdev, "LampCount is 0 (invalid)\n");
 		ret = -EINVAL;
 		goto out;
 	}
@@ -415,7 +415,7 @@ static int lamparray_get_lamp_attributes(struct lamparray_device *ldev)
 	ret = hid_hw_raw_request(hdev, report->id, buf, len,
 		HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
 	if (ret < 0) {
-		hid_dbg(hdev, "Failed to read LampAttributesResponseReport: %d\n", ret);
+		hid_warn(hdev, "Failed to read LampAttributesResponseReport: %d\n", ret);
 		goto out;
 	}
 
@@ -427,7 +427,7 @@ static int lamparray_get_lamp_attributes(struct lamparray_device *ldev)
 	ldev->max_brightness = get_field_value_as_u8(&ldev->color_levels.intensity);
 
 	if (!ldev->max_r || !ldev->max_g || !ldev->max_b || !ldev->max_brightness) {
-		hid_dbg(hdev, "LampArray device has no color levels\n");
+		hid_warn(hdev, "LampArray device has no color levels\n");
 		ret = -EINVAL;
 		goto out;
 	}
-- 
2.55.0

[PATCH 3/4] HID: lamparray: transfer control when use_leds_uapi changes

[Cristian Mazzotta]

Autonomous mode is disabled once in lamparray_register() and never
changed again. Writing 0 to use_leds_uapi unregisters the LED class
device but leaves the device under host control, and writing 1
re-registers the class device without taking the device back, so lamp
updates sent afterwards are ignored.

On an Acer Predator PT14-52T (USB keyboard 05AF:767A) the device stays
with whatever last drove it: after writing 0, running a userspace tool
that sets a hardware effect, then writing 1, the LED class device
reports the cached values despite the hardware still running an effect.
Reading AutonomousMode back returns the last value written by the host
rather than the state the device is actually in, so the driver cannot
detect this.

Disable autonomous mode before registering the LED class device and
re-enable it after unregistering, so control is handed over in both
directions. Unregister the class device before handing the hardware
back, so userspace cannot write lamp updates to a device that is no
longer accepting them.

Failure to re-enable autonomous mode on the disable path is logged but
not propagated since the class device is already gone and the write
has otherwise succeeded. The restore failure path drops back to
hid_warn() for the same reason: it is recoverable, and the attribute
reverts to its previous value.

Signed-off-by: Cristian Mazzotta <cmmazzo@icloud.com>
---
 drivers/hid/hid-lamparray.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
index 70c596f292a2..bbde006d119b 100644
--- a/drivers/hid/hid-lamparray.c
+++ b/drivers/hid/hid-lamparray.c
@@ -659,25 +659,43 @@ static ssize_t use_leds_uapi_store(struct device *dev,
 	ldev->use_leds_uapi = val;
 	mutex_unlock(&ldev->dev_lock);
 
+	/*
+	 * Take the device out of autonomous mode before exposing the LED
+	 * class device, and hand it back afterwards, so that control is
+	 * transferred in both directions rather than only at probe.
+	 */
 	if (val == 1) {
+		ret = lamparray_hw_set_autonomous(ldev, false);
+		if (ret)
+			goto err_revert;
 		ret = lamparray_register_led(ldev);
 		if (ret) {
-			mutex_lock(&ldev->dev_lock);
-			ldev->use_leds_uapi = old_val;
-			mutex_unlock(&ldev->dev_lock);
-			return ret;
+			lamparray_hw_set_autonomous(ldev, true);
+			goto err_revert;
 		}
 		ret = lamparray_restore_state(ldev);
 		if (ret) {
-			hid_err(ldev->hdev, "Could not restore state: %d\n", ret);
-			return ret;
+			hid_warn(ldev->hdev, "Could not restore state: %d\n", ret);
+			lamparray_unregister_led(ldev);
+			lamparray_hw_set_autonomous(ldev, true);
+			goto err_revert;
 		}
-
 	} else {
 		lamparray_unregister_led(ldev);
+		ret = lamparray_hw_set_autonomous(ldev, true);
+		if (ret) {
+			hid_warn(ldev->hdev, "Could not enable autonomous mode: %d\n", ret);
+			return count;
+		}
 	}
 
 	return count;
+
+err_revert:
+	mutex_lock(&ldev->dev_lock);
+	ldev->use_leds_uapi = old_val;
+	mutex_unlock(&ldev->dev_lock);
+	return ret;
 }
 static DEVICE_ATTR_RW(use_leds_uapi);
 
-- 
2.55.0

[PATCH 4/4] HID: lamparray: blank lamps across suspend and restore on resume

[Cristian Mazzotta]

The helper installs no PM callbacks, so lamps keep their last state
across suspend. Firmware is not required to turn them off in low power
states, and on devices where it does not, they stay lit for the whole
suspend.

On an Acer Predator PT14-52T, system power draw during s2idle is 12.35W
with the lamps lit and 3.14W with them blanked, measured at the charger.

Add lamparray_suspend(), which writes zeroes to the color channels
while leaving the cached RGB and brightness untouched, and
lamparray_resume(), which restores that cache. Both return early when
use_leds_uapi is 0 so a userspace driver keeps full control. hid-generic
gains suspend and resume callbacks to drive them, and calls
lamparray_resume() from reset_resume as well.

Some devices return to firmware control across a hibernate transition
while still reporting AutonomousMode = 0, and ignore host lamp updates
until the value genuinely transitions. Force a 1 -> 0 on resume
before restoring state.

Signed-off-by: Cristian Mazzotta <cmmazzo@icloud.com>
---
 drivers/hid/hid-generic.c     | 27 +++++++++++++++++++
 drivers/hid/hid-lamparray.c   | 51 +++++++++++++++++++++++++++++++++++
 include/linux/hid-lamparray.h | 35 ++++++++++++++++++++++++
 3 files changed, 113 insertions(+)

diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c
index c3d2283198e8..dc1cca45d599 100644
--- a/drivers/hid/hid-generic.c
+++ b/drivers/hid/hid-generic.c
@@ -99,9 +99,34 @@ static int hid_generic_probe(struct hid_device *hdev,
 
 static int hid_generic_reset_resume(struct hid_device *hdev)
 {
+	struct lamparray *la = hid_get_drvdata(hdev);
+
 	if (hdev->claimed & HID_CLAIMED_INPUT)
 		hidinput_reset_resume(hdev);
 
+	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && la)
+		lamparray_resume(la);
+
+	return 0;
+}
+
+static int hid_generic_suspend(struct hid_device *hdev, pm_message_t message)
+{
+	struct lamparray *la = hid_get_drvdata(hdev);
+
+	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && la)
+		lamparray_suspend(la);
+
+	return 0;
+}
+
+static int hid_generic_resume(struct hid_device *hdev)
+{
+	struct lamparray *la = hid_get_drvdata(hdev);
+
+	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && la)
+		lamparray_resume(la);
+
 	return 0;
 }
 
@@ -127,6 +152,8 @@ static struct hid_driver hid_generic = {
 	.match = hid_generic_match,
 	.probe = hid_generic_probe,
 	.reset_resume = hid_generic_reset_resume,
+	.suspend = hid_generic_suspend,
+	.resume = hid_generic_resume,
 	.remove = hid_generic_remove,
 };
 module_hid_driver(hid_generic);
diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
index bbde006d119b..dec9d7883887 100644
--- a/drivers/hid/hid-lamparray.c
+++ b/drivers/hid/hid-lamparray.c
@@ -869,6 +869,57 @@ void lamparray_unregister(struct lamparray *la)
 }
 EXPORT_SYMBOL_GPL(lamparray_unregister);
 
+/*
+ * Blank all lamps on suspend rather than handing control back to the firmware,
+ * which may not turn them off in low power states. On an Acer Predator PT14-52T,
+ * system power draw during s2idle was ~12.35W with lamps lit, and ~2.84W with
+ * them blanked; the lighting accounted for ~77% of the power draw during suspend.
+ * Since writing zeroes is well defined on all lamparray devices, always do it.
+ * This is ignored if use_leds_uapi is 0; let userspace keep full control.
+ *
+ * Lamps are written to without holding the lock because PM will freeze userspace
+ * first, which makes concurrent writes impossible.
+ */
+int lamparray_suspend(struct lamparray *la)
+{
+	if (!la)
+		return 0;
+
+	struct lamparray_device *ldev = &la->ldev;
+
+	if (!ldev->use_leds_uapi)
+		return 0;
+
+	lamparray_hw_set_state(ldev, 0, 0, 0, ldev->last_brightness);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(lamparray_suspend);
+
+int lamparray_resume(struct lamparray *la)
+{
+	if (!la)
+		return 0;
+
+	struct lamparray_device *ldev = &la->ldev;
+
+	if (!ldev->use_leds_uapi)
+		return 0;
+
+	/*
+	 * After a S4 transition, some devices report
+	 * AutonomousMode = 0 while still ignoring host lamp updates.
+	 * Writing 0 again does nothing; forcing a 1 -> 0
+	 * will guarantee the device will update.
+	 */
+	lamparray_hw_set_autonomous(ldev, true);
+	lamparray_hw_set_autonomous(ldev, false);
+	lamparray_restore_state(ldev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(lamparray_resume);
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Tim Guttzeit <tgu@tuxedocomputers.com>");
 MODULE_AUTHOR("Aaron Erhardt <aer@tuxedocomputers.com>");
diff --git a/include/linux/hid-lamparray.h b/include/linux/hid-lamparray.h
index a77869728d12..a9f827743baa 100644
--- a/include/linux/hid-lamparray.h
+++ b/include/linux/hid-lamparray.h
@@ -65,6 +65,31 @@ struct lamparray *lamparray_register(struct hid_device *hdev,
  */
 void lamparray_unregister(struct lamparray *la);
 
+/**
+ * lamparray_suspend() - blank all lamps ahead of sleep transition
+ * @la: LampArray handle returned by lamparray_register()
+ *
+ * Writes zeroes to the rgb values only, keeping the brightness, unless the
+ * use_leds_uapi sysfs attribute is 0, in which case, it will return early
+ * before writing anything. The cached state is left untouched so
+ * lamparray_resume() can restore it.
+ *
+ * Return: 0
+ */
+int lamparray_suspend(struct lamparray *la);
+
+/**
+ * lamparray_resume() - restore host control and LampArray state
+ * @la: LampArray handle returned by lamparray_register()
+ *
+ * Disables autonomous mode (in case device returns to firmware control after suspend)
+ * and restores the cached state of the device. If the use_leds_uapi attribute is 0,
+ * it will return early and prevent any unwanted writing.
+ *
+ * Return: 0
+ */
+int lamparray_resume(struct lamparray *la);
+
 #else /* !CONFIG_HID_LAMPARRAY */
 
 static inline bool lamparray_is_supported_device(struct hid_device *hdev)
@@ -83,6 +108,16 @@ static inline void lamparray_unregister(struct lamparray *la)
 {
 }
 
+static inline int lamparray_suspend(struct lamparray *la)
+{
+	return 0;
+}
+
+static inline int lamparray_resume(struct lamparray *la)
+{
+	return 0;
+}
+
 #endif /* CONFIG_HID_LAMPARRAY */
 
 #endif /* _HID_LAMPARRAY_H */
-- 
2.55.0

Re: [PATCH v5 0/2] HID: generic: add LampArray support via hid-lamparray helper

[Armin Wolf]

Am 07.09.26 um 18:30 schrieb Aaron Erhardt:
> Am 04.09.26 um 22:49 schrieb Armin Wolf:
>> Am 03.09.26 um 09:35 schrieb Aaron Erhardt:
>>
>>> Add a new hid-lamparray helper module and integrate it with the
>>> hid-generic driver.
>>>
>>> While more complex lamparray handling should be done in userspace via
>>> hidraw, providing a small module to add basic lamparray support makes it
>>> possible for userspace software to interact with lamparrays by simply
>>> using well-known APIs of the LED subsystem. One use-case would be to
>>> enable desktop environments to support keyboard backlight control out of
>>> the box for HID lamparray devices without having to implement the whole
>>> HID protocol themselves.
>>>
>>> This patch is based on previous discussions:
>>> https://lore.kernel.org/all/1fb08a74-62c7-4d0c-ba5d-648e23082dcb@tuxedocomputers.com/
>>>
>>> The helper provides basic support for devices exposing a
>>> Lighting/LampArray application collection (usage page 0x59) and
>>> registers a single-zone RGB LED representation via the LED
>>> subsystem.
>>>
>>> hid-generic now checks for LampArray support after hid_parse() and
>>> optionally registers a lamparray instance. Failures in the helper
>>> do not abort device probe to keep the driver logic otherwise unchanged.
>>>
>>> LampArray resources are released on driver remove.
>>>
>>> This commit was successfully tested on the Microsoft MacroPad reference
>>> implementation (https://github.com/microsoft/RP2040MacropadHidSample
>>> 1d6c3ad) and in combination with the tuxedo_nb04_wmi driver, albeit
>>> only fully functional with a recent fix posted to the LKML
>>> (https://lore.kernel.org/all/20260826081149.235487-2-aer@tuxedocomputers.com).
>> Nice work, it works on my ASUS Prime B650-Plus. However the behavior of the brightness
>> attribute is a bit strange:
>>
>> - manually setting "brightness" does not change anything (max. is 1)
>> - setting RGB to "0 0 0" causes "brightness" to become 0
>> - setting RGB to a non-zero value causes "brightness" to become 1
>>
>> Any idea why this happens? I can check if the same problems also exists under Windows,
>> if requested.
>>
>> Thanks,
>> Armin Wolf
>>
> It is completely normal for LampArray devices to only offer two brightness
> values (1 and 0) for turning the whole LED on and off. Since a lot of
> userspace software seems to never use brightness (it is more convenient to
> adjust the RGB channels directly), this was not even properly implemented in
> the MacropadHidSample until recently:
> https://github.com/microsoft/RP2040MacropadHidSample/commit/cfc29120a3910c5772976da29ecd57392dfd44d6
>
> Thus, I think it is likely, that the implementation is broken and simply
> ignores brightness. The driver just forwards this to the device.
>
> However, the RGB values (aka. multi_intensity) should not interfere with the
> brightness. Yet, I wasn't able to reproduce this on the Macropad. Capping the
> brightness to 1 is normal on the other hand, at least if that's the
> maxIntensity reported by your device.
>
> So the only really odd thing for me would be the RGB values influencing the
> brightness. Please provide more detailed feedback if you can since I can't
> reproduce this on the hardware available to me.

I did some further tests, and it turned out that the RGB values indeed do not
influence the brightness value. It seems that i confused myself during testing xd.

So it seems that Asus copied the buggy Macropad code. Would it be possible to
send RGB = (0, 0, 0) when the user has selected brightness 0 to work around this
firmware bug?

Thanks,
Armin Wolf

> If you want to investigate the LampArray capabilities of your device, you're
> probably better off with userspace tooling like my lampctl fork:
> https://github.com/tuxedo-aer/lampctl
>
> You can adjust the hardcoded brightness here to see whether your device honors
> the brightness value or not:
> https://github.com/tuxedo-aer/lampctl/blob/main/crates/lamparray/src/hid.rs#L36
>
>>> v5:
>>> - Proper hardware detection (no quirks necessary anymore)
>>> - Add documentation for new sysfs knob
>>> - Pass limits of the device to sysfs (intesities & brightness)
>>> - More flexible Kconfig (use tristate)
>>> - Improved locking
>>> - Several memory leak and (de-)initialization fixes
>>> - Don't read current color values from hardware (the HID spec does not
>>>     offer this option)
>>> - Remove redundant report dump functionality
>>> v4:
>>> - Restrict CONFIG_HID_LAMPARRAY to built-in configurations only to fix
>>>     additional randconfig build errors
>>> v3:
>>> - Squash V1 and V2 into one patch
>>> v2:
>>> - Fix Kconfig to avoid build errors when LEDS_CLASS_MULTICOLOR is
>>>     disabled
>>>
>>> Aaron Erhardt (2):
>>>     HID: lamparray: add new LampArray helper module
>>>     HID: generic: add LampArray support via hid-lamparray helper
>>>
>>>    .../ABI/testing/sysfs-driver-hid-lamparray    |  16 +
>>>    drivers/hid/Kconfig                           |  18 +
>>>    drivers/hid/Makefile                          |   2 +
>>>    drivers/hid/hid-generic.c                     |  38 +
>>>    drivers/hid/hid-lamparray.c                   | 812 ++++++++++++++++++
>>>    include/linux/hid-lamparray.h                 |  88 ++
>>>    6 files changed, 974 insertions(+)
>>>    create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-lamparray
>>>    create mode 100644 drivers/hid/hid-lamparray.c
>>>    create mode 100644 include/linux/hid-lamparray.h
>>>

Re: [PATCH v5 0/2] HID: generic: add LampArray support via hid-lamparray helper

[Aaron Erhardt]

Am 10.09.26 um 00:36 schrieb Armin Wolf:
> 
> Am 07.09.26 um 18:30 schrieb Aaron Erhardt:
>> Am 04.09.26 um 22:49 schrieb Armin Wolf:
>>> Am 03.09.26 um 09:35 schrieb Aaron Erhardt:
>>>
>>>> Add a new hid-lamparray helper module and integrate it with the
>>>> hid-generic driver.
>>>>
>>>> While more complex lamparray handling should be done in userspace via
>>>> hidraw, providing a small module to add basic lamparray support makes it
>>>> possible for userspace software to interact with lamparrays by simply
>>>> using well-known APIs of the LED subsystem. One use-case would be to
>>>> enable desktop environments to support keyboard backlight control out of
>>>> the box for HID lamparray devices without having to implement the whole
>>>> HID protocol themselves.
>>>>
>>>> This patch is based on previous discussions:
>>>> https://lore.kernel.org/all/1fb08a74-62c7-4d0c-ba5d-648e23082dcb@tuxedocomputers.com/
>>>>
>>>> The helper provides basic support for devices exposing a
>>>> Lighting/LampArray application collection (usage page 0x59) and
>>>> registers a single-zone RGB LED representation via the LED
>>>> subsystem.
>>>>
>>>> hid-generic now checks for LampArray support after hid_parse() and
>>>> optionally registers a lamparray instance. Failures in the helper
>>>> do not abort device probe to keep the driver logic otherwise unchanged.
>>>>
>>>> LampArray resources are released on driver remove.
>>>>
>>>> This commit was successfully tested on the Microsoft MacroPad reference
>>>> implementation (https://github.com/microsoft/RP2040MacropadHidSample
>>>> 1d6c3ad) and in combination with the tuxedo_nb04_wmi driver, albeit
>>>> only fully functional with a recent fix posted to the LKML
>>>> (https://lore.kernel.org/all/20260826081149.235487-2-aer@tuxedocomputers.com).
>>> Nice work, it works on my ASUS Prime B650-Plus. However the behavior of the brightness
>>> attribute is a bit strange:
>>>
>>> - manually setting "brightness" does not change anything (max. is 1)
>>> - setting RGB to "0 0 0" causes "brightness" to become 0
>>> - setting RGB to a non-zero value causes "brightness" to become 1
>>>
>>> Any idea why this happens? I can check if the same problems also exists under Windows,
>>> if requested.
>>>
>>> Thanks,
>>> Armin Wolf
>>>
>> It is completely normal for LampArray devices to only offer two brightness
>> values (1 and 0) for turning the whole LED on and off. Since a lot of
>> userspace software seems to never use brightness (it is more convenient to
>> adjust the RGB channels directly), this was not even properly implemented in
>> the MacropadHidSample until recently:
>> https://github.com/microsoft/RP2040MacropadHidSample/commit/cfc29120a3910c5772976da29ecd57392dfd44d6
>>
>> Thus, I think it is likely, that the implementation is broken and simply
>> ignores brightness. The driver just forwards this to the device.
>>
>> However, the RGB values (aka. multi_intensity) should not interfere with the
>> brightness. Yet, I wasn't able to reproduce this on the Macropad. Capping the
>> brightness to 1 is normal on the other hand, at least if that's the
>> maxIntensity reported by your device.
>>
>> So the only really odd thing for me would be the RGB values influencing the
>> brightness. Please provide more detailed feedback if you can since I can't
>> reproduce this on the hardware available to me.
> 
> I did some further tests, and it turned out that the RGB values indeed do not
> influence the brightness value. It seems that i confused myself during testing xd.
> 
> So it seems that Asus copied the buggy Macropad code. Would it be possible to
> send RGB = (0, 0, 0) when the user has selected brightness 0 to work around this
> firmware bug?

Yes, I think that would be a reasonably small quirk that could be useful for a
wide range of devices. I will add this in the next iteration.

> 
> Thanks,
> Armin Wolf
> 
>> If you want to investigate the LampArray capabilities of your device, you're
>> probably better off with userspace tooling like my lampctl fork:
>> https://github.com/tuxedo-aer/lampctl
>>
>> You can adjust the hardcoded brightness here to see whether your device honors
>> the brightness value or not:
>> https://github.com/tuxedo-aer/lampctl/blob/main/crates/lamparray/src/hid.rs#L36
>>
>>>> v5:
>>>> - Proper hardware detection (no quirks necessary anymore)
>>>> - Add documentation for new sysfs knob
>>>> - Pass limits of the device to sysfs (intesities & brightness)
>>>> - More flexible Kconfig (use tristate)
>>>> - Improved locking
>>>> - Several memory leak and (de-)initialization fixes
>>>> - Don't read current color values from hardware (the HID spec does not
>>>>     offer this option)
>>>> - Remove redundant report dump functionality
>>>> v4:
>>>> - Restrict CONFIG_HID_LAMPARRAY to built-in configurations only to fix
>>>>     additional randconfig build errors
>>>> v3:
>>>> - Squash V1 and V2 into one patch
>>>> v2:
>>>> - Fix Kconfig to avoid build errors when LEDS_CLASS_MULTICOLOR is
>>>>     disabled
>>>>
>>>> Aaron Erhardt (2):
>>>>     HID: lamparray: add new LampArray helper module
>>>>     HID: generic: add LampArray support via hid-lamparray helper
>>>>
>>>>    .../ABI/testing/sysfs-driver-hid-lamparray    |  16 +
>>>>    drivers/hid/Kconfig                           |  18 +
>>>>    drivers/hid/Makefile                          |   2 +
>>>>    drivers/hid/hid-generic.c                     |  38 +
>>>>    drivers/hid/hid-lamparray.c                   | 812 ++++++++++++++++++
>>>>    include/linux/hid-lamparray.h                 |  88 ++
>>>>    6 files changed, 974 insertions(+)
>>>>    create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-lamparray
>>>>    create mode 100644 drivers/hid/hid-lamparray.c
>>>>    create mode 100644 include/linux/hid-lamparray.h
>>>>
> 

Re: [PATCH 0/4] HID: lamparray: fixes from testing on Acer Predator PT14-52T

[Aaron Erhardt]

Am 09.09.26 um 18:52 schrieb Cristian Mazzotta:
> These are four fixes on top of the v5 series, posted 2026-09-03:
> https://lore.kernel.org/linux-input/20260903073602.3815258-1-aer@tuxedocomputers.com/
> 
> Tested on an Acer Predator PT14-52T, which has two LampArray devices: a
> USB keyboard (05AF:767A) and an I2C ENE controller (0CF2:5130).
> 
> With v5 as posted, the keyboard does not probe. LampCount reads back 0,
> and once that is fixed the level counts read back 0 as well, which
> divides by zero in led_mc_calc_color_components() during probe and takes
> the USB hub worker down with it. Patches 1 and 2 cover the reads and the
> validation; this is the same problem Aaron and Werner discussed in this
> thread, with a backtrace from hardware that hits it.
> 
> Patch 3 makes use_leds_uapi transfer control in both directions.
> Autonomous mode is currently only set at probe, so writing 1 re-registers
> the LED class device without taking the hardware back, and updates sent
> afterwards are ignored.
> 
> Patch 4 blanks the lamps across suspend. On this machine, the lighting
> accounts for 9.21W of the 12.35W s2idle power draw against 3.14W with
> the lamps blanked from this driver, so this is most of the suspend power
> on a laptop with RGB. This was also tested with one of the 2 lights on
> the ENE controller still on; this patch does not control both zones.
> 
> Please treat these as input for v6 rather than a separate series;
> squashing them in is fine by me, and I will rebase and retest against v6
> when it arrives.

Thanks for your patches! I have reviewed and tested them and the only
thing I noticed is that wrapping the resume/suspend methods with pm_ptr
is missing in the final patch. Otherwise everything LGTM.

I will follow up with v6 soon, adding you with "Co-developed-by" on
the relevant commits.

Best regards
Aaron

> 
> Two things I have not fixed:
> 
> - The ENE controller exposes two LampArrayAttributesReport collections.
>   lamparray_parse_update_report() has no first-match guard, so the second
>   overwrites the first and only one zone is claimed; the other stays in
>   firmware control. This is the multi-zone question from Werner's reply.
>   I really believe that multi-zone support should exist, but I have not
>   included it because I haven't started it; uni is taking away a lot of
>   my time currently.

Ack. I think this is better suited for a separate patch series, unless it
is trivial to add.

> 
> - hid_device_io_start() in lamparray_register() may no longer be needed:
>   hid_hw_raw_request() is synchronous and does not go through
>   hid_input_report(), so it does not need driver_input_lock released.
>   That would also address the second [High] item from the Sashiko review.

Ack. I will look into this for v6.

> 
> Cristian Mazzotta (4):
>   HID: lamparray: read attribute reports synchronously
>   HID: lamparray: raise log level of fatal probe errors
>   HID: lamparray: transfer control when use_leds_uapi changes
>   HID: lamparray: blank lamps across suspend and restore on resume
> 
>  drivers/hid/hid-generic.c     |  27 ++++++
>  drivers/hid/hid-lamparray.c   | 162 +++++++++++++++++++++++++++++-----
>  include/linux/hid-lamparray.h |  35 ++++++++
>  3 files changed, 200 insertions(+), 24 deletions(-)
> 
> 
> base-commit: 9b298109e37e5caf4b6800198c4907a5a6bf00ae