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

[Armin Wolf <W_Armin@gmx.de>]

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