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

[Aaron Erhardt <aer@tuxedocomputers.com>]

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.

Another question that came to my mind while doing some tests on what will soon
be posted as v6 is the following: Should we really set everything to zero as a
default? Up to v4, the LEDs were set to full brightness by default which would
be quite annoying in dark environments or when the LEDs of the device are
really bright. Turning everything off like the current implementation on the
other side makes things look like they don't work until a userspace program
takes over control. So I was wondering whether it would be better to just leave
everything in autonomous mode until userspace starts writing to the LED. I'm
not sure yet what the best solution is, so I would like to have some feedback
on that design if possible.

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