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@xxxxxxxxxxxxxxxxxxx/
>>
>> 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@xxxxxxxxxxxxxxxxxxx).
> 
> 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
>>
>