Re: [PATCH v4] HID: generic: add LampArray support via hid-lamparray helper

[Aaron Erhardt]

Hi Cristian,

I just posted the patch with the updated driver.

Please test it and feel free to provide feedback :)

https://lore.kernel.org/linux-input/20260903073602.3815258-1-aer@xxxxxxxxxxxxxxxxxxx/T/#m1b8fdad3bb17ddf6f9139b373ef41c626e52e44f

Best regards
Aaron

Am 28.08.26 um 16:45 schrieb Cristian Mazzotta:
> Am 27.08.26 um 22:13 schrieb Werner Sembach:
>> I think it's best for Aaron sending his version first and then you
>> reacting on that on the LKML and getting in touch.
> 
> Sounds good, I'll hold off and review Aaron's version when it lands.
> 
>> Unregistering is important for userspace to know what happens
> 
> That's fair, I'll be sure to change that.
> 
> As for multi-zone, my I2C controller has more than one
> LampArrayAttributesReport, and has 2 zones in effect, so the original
> would only grab one of them. I'm not sure if my device is spec compliant,
> but my implementation is agnostic to the zone count; I'll come back to it
> with Aaron's version. Quirks get an entry for each zone too.
> 
> To clarify, each zone in my implementation gets its own dev class where
> /sys/class/leds/<hid-id>:rgb:lamparray-<zone_number>
> And use_leds_uapi is accessible from both
> /sys/class/leds/<hid-id>:rgb:lamparray-<zone_number>/device/use_leds_uapi
> and /sys/bus/hid/devices/<hid-id>/use_leds_uapi.
> 
> For Aaron when he gets to see this, here are some things I noticed while
> testing on real hardware:
> 
> 1) lamparray_read_lamp_count() reads
>    lamp_count_field->value[lamp_count_index] immediately after
>    hid_hw_request(HID_REQ_GET_REPORT). As mentioned above, that call is
>    asynchronous, so the read races the transfer and returns stale values.
>    On my devices it consistently returned 0. hid_hw_raw_request() with a
>    heap buffer works.
> 2) In lamparray_parse_update_report(), usage 0x0003 is not unique within
>    the Lighting page, and the switch has no guard, so lamp_count_field ends
>    up bound to whichever match the parser saw last rather than the actual
>    LampCount field. I added a simple guard so only the first one binds.
>    Whether the first match will always be correct is an open question
>    however.
> 3) Both of my devices need the autonomous-mode disable re-asserted on S4
>    resume; without it they come back in firmware-driven mode and ignore
>    SET_REPORT.
> 4) I don't believe this would be consistent with all LampArray devices,
>    but mine in particular would ignore any change to the lighting that
>    was within 4 values of its current state, so consecutive updates that
>    are too similar silently do nothing.
> 5) My hardware did not support reading back the current state of the lamps
>    at all, so GET_REPORT and in effect, lamparray_hw_get_state(), can't
>    work here. Plenty of LED drivers return the cached value when getting
>    the state, which might be the safer default.
> 
> Also let me know if you want me to send my HID descriptors from
> hid-recorder or similar.
> 
> In the meantime, I will definitely go look at OpenRGB's implementation.
> I may also work on a driver that will fix s2idle residency on my laptop;
> it's part of what brought me here and this is just step one.
> 
> Thanks a lot,
> Cristian Mazzotta
>