Re: [PATCH v5 5/5] leds: is31f132xx: Fix missing brightness_steps for is31f13236

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The commit being fixed (a18983b95a61) broke the driver's lockless design by introducing multi-register PWM updates.
- [High] The commit being fixed (a18983b95a61) introduced an initialization order regression where hardware is reset after LED class devices are registered, wiping their default states.
- [Low] The commit subject contains typos in both the driver and chip names ('1' instead of 'l').
--

commit 368d92722167efb41b11021742d19656c04d1740
Author: Jun Yan <jerrysteve1101@gmail.com>

leds: is31f132xx: Fix missing brightness_steps for is31f13236

This commit adds the missing brightness_steps configuration for the
is31fl3236 chip to restore proper brightness control.

> leds: is31f132xx: Fix missing brightness_steps for is31f13236

This isn't a bug, but there's a typo in the subject line where the number
'1' is used instead of the letter 'l' in the driver and chip names
(is31f132xx and is31f13236).

> Add missing brightness_steps for is31fl3236 to fix brightness control.
>
> Fixes: a18983b95a61 ("leds: is31f132xx: Add support for is31fl3293")

Since this patch fixes commit a18983b95a61, are there a couple of other
regressions introduced by that commit that also need addressing?

The driver lacks a lock for brightness updates, relying on the assumption
that intervening update register writes from other channels are harmless.
While this was true for older 8-bit chips that use a single PWM write, the
IS31FL3293 requires two writes (LSB and MSB) per brightness update.

If two LED channels update concurrently from different per-LED workqueue
threads, an update register write from one channel could interleave between
the LSB and MSB writes of the other channel.

Could this cause the hardware to latch a torn brightness value?

By swapping the order of is31fl32xx_parse_dt() and is31fl32xx_init_regs()
in the probe function in commit a18983b95a61, the driver now registers the
LED class devices before initializing the hardware.

During registration, the subsystem evaluates properties like default-state
and sets the initial brightness via I2C writes. However, immediately after
device tree parsing, is31fl32xx_init_regs() is invoked and triggers a
hardware reset, which silently wipes out the PWM values applied during
registration.

Does this initialization order wipe out the default states entirely?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508131139.1523597-1-jerrysteve1101@gmail.com?part=5