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

[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