Re: [PATCH 0/4] HID: lamparray: fixes from testing on Acer Predator PT14-52T

[Aaron Erhardt]

Am 09.09.26 um 18:52 schrieb Cristian Mazzotta:
> These are four fixes on top of the v5 series, posted 2026-09-03:
> https://lore.kernel.org/linux-input/20260903073602.3815258-1-aer@xxxxxxxxxxxxxxxxxxx/
> 
> Tested on an Acer Predator PT14-52T, which has two LampArray devices: a
> USB keyboard (05AF:767A) and an I2C ENE controller (0CF2:5130).
> 
> With v5 as posted, the keyboard does not probe. LampCount reads back 0,
> and once that is fixed the level counts read back 0 as well, which
> divides by zero in led_mc_calc_color_components() during probe and takes
> the USB hub worker down with it. Patches 1 and 2 cover the reads and the
> validation; this is the same problem Aaron and Werner discussed in this
> thread, with a backtrace from hardware that hits it.
> 
> Patch 3 makes use_leds_uapi transfer control in both directions.
> Autonomous mode is currently only set at probe, so writing 1 re-registers
> the LED class device without taking the hardware back, and updates sent
> afterwards are ignored.
> 
> Patch 4 blanks the lamps across suspend. On this machine, the lighting
> accounts for 9.21W of the 12.35W s2idle power draw against 3.14W with
> the lamps blanked from this driver, so this is most of the suspend power
> on a laptop with RGB. This was also tested with one of the 2 lights on
> the ENE controller still on; this patch does not control both zones.
> 
> Please treat these as input for v6 rather than a separate series;
> squashing them in is fine by me, and I will rebase and retest against v6
> when it arrives.

Thanks for your patches! I have reviewed and tested them and the only
thing I noticed is that wrapping the resume/suspend methods with pm_ptr
is missing in the final patch. Otherwise everything LGTM.

I will follow up with v6 soon, adding you with "Co-developed-by" on
the relevant commits.

Best regards
Aaron

> 
> Two things I have not fixed:
> 
> - The ENE controller exposes two LampArrayAttributesReport collections.
>   lamparray_parse_update_report() has no first-match guard, so the second
>   overwrites the first and only one zone is claimed; the other stays in
>   firmware control. This is the multi-zone question from Werner's reply.
>   I really believe that multi-zone support should exist, but I have not
>   included it because I haven't started it; uni is taking away a lot of
>   my time currently.

Ack. I think this is better suited for a separate patch series, unless it
is trivial to add.

> 
> - hid_device_io_start() in lamparray_register() may no longer be needed:
>   hid_hw_raw_request() is synchronous and does not go through
>   hid_input_report(), so it does not need driver_input_lock released.
>   That would also address the second [High] item from the Sashiko review.

Ack. I will look into this for v6.

> 
> Cristian Mazzotta (4):
>   HID: lamparray: read attribute reports synchronously
>   HID: lamparray: raise log level of fatal probe errors
>   HID: lamparray: transfer control when use_leds_uapi changes
>   HID: lamparray: blank lamps across suspend and restore on resume
> 
>  drivers/hid/hid-generic.c     |  27 ++++++
>  drivers/hid/hid-lamparray.c   | 162 +++++++++++++++++++++++++++++-----
>  include/linux/hid-lamparray.h |  35 ++++++++
>  3 files changed, 200 insertions(+), 24 deletions(-)
> 
> 
> base-commit: 9b298109e37e5caf4b6800198c4907a5a6bf00ae