Re: [PATCH v3 0/9] leds: st1202: fix multiple bugs in pattern engine and brightness handling

[Manuel Fombuena]

On Wed, 17 Jun 2026, Lee Jones wrote:

> 
> I was going to comment on each patch, but here, take the whole review:
> 
> https://sashiko.dev/#/patchset/GV1PR08MB84976729EED25ECB484835A4C5112@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> 

The Sashiko review of v2 resulted in several changes in v3: patches 3
and 4 were squashed, patch 5 was dropped as unnecessary, a redundant
I2C write was removed from patch 4, two commit messages were corrected,
and a new patch was added for the reg bounds check. Pre-existing issues
flagged in the review are outside the scope of this series and will be
addressed separately.

The Sashiko review of v3 has surfaced two new issues not present in the
earlier review. Both have been fixed and will be included in v4:

  - Patch 7: pattern_set() did not re-enable the hardware channel before
    starting the sequencer. If brightness had previously been set to zero
    (disabling the channel), the pattern would run but produce no light.

  - Patch 8: reg was declared as signed int but populated via
    of_property_read_u32(). A device tree value >= 0x80000000 would be
    interpreted as negative and silently bypass the bounds check. reg is
    now declared as u32.

The remaining comments in both reviews are pre-existing issues outside
the scope of this series and will be addressed separately. For
completeness, a full summary of the v3 review comments and responses
follows.

Patch 1: stop pattern sequence before reprogramming
----------------------------------------------------

Comment 1: Does clearing PATS break multi-channel independence? Could
zeroing the channel PWM registers suffice instead of stopping the
sequencer?

Response: Simply zeroing the PWM makes the LED visually dark but leaves
the sequencer running with stale duration values. Since the datasheet
(§4.8) states that writes to duration registers are only applied after
the sequence completes or is stopped, a subsequent pattern_set() call
would silently fail to reprogram timing without stopping first. Stopping
the sequencer is required for correctness. The global sequencer
constraint is an inherent hardware limitation acknowledged in the commit
message.

Comment 2: pattern_set() overwrites globally shared duration registers,
silently corrupting timings for other active channels. (Pre-existing)

Response: Acknowledged. Tracked for a follow-up submission.

Comment 3: brightness_set() acquires a mutex and performs I2C
transactions, making it unsafe to assign to the non-blocking
brightness_set callback. (Pre-existing)

Response: Acknowledged. Tracked for a follow-up submission.


Patch 2: validate pattern input before stopping the sequence
------------------------------------------------------------

Comment 1: brightness_set() acquires a mutex and performs I2C
transactions, making it unsafe to assign to the non-blocking
brightness_set callback. (Pre-existing, repeated)

Response: Acknowledged. Tracked for a follow-up submission.


Patch 3: fix pattern duration prescaler and pattern_clear skip marker
---------------------------------------------------------------------

Comment 1: Writing 0 to shared duration registers in pattern_clear()
halts patterns on all other active LEDs. (Pre-existing, repeated)

Response: Acknowledged. This is an inherent hardware constraint — the
LED1202 has a single global pattern sequencer. Tracked for a follow-up
submission.

Comment 2: st1202_pwm_pattern_write() writes an unscaled 8-bit value
to a 12-bit register, limiting pattern duty cycle to ~6.2%. (Pre-existing)

Response: Acknowledged. Tracked for a follow-up submission.

Comment 3: brightness_set() sleeps in atomic context. (Pre-existing, 
repeated)

Response: Acknowledged. Tracked for a follow-up submission.


Patch 4: restore Pattern0 PWM to full on after clearing pattern
---------------------------------------------------------------

Comment 1: pattern_clear() and pattern_set() mutate global chip state,
halting patterns on other channels. (Pre-existing, repeated)

Response: Acknowledged. Tracked for a follow-up submission.

Comment 2: brightness_set() sleeps in atomic context. (Pre-existing, 
repeated)

Response: Acknowledged. Tracked for a follow-up submission.

Comment 3: brightness_set_blocking ignores the requested brightness
value and only toggles the channel on or off. (Pre-existing, repeated)

Response: Acknowledged. Tracked for a follow-up submission.

Comment 4: PWM scaling limits pattern duty cycle to ~6.2%. (Pre-existing, 
repeated)

Response: Acknowledged. Tracked for a follow-up submission.

Comment 5: Unused duration slots not cleared in pattern_set() when
programming a shorter pattern than the previous one. (Pre-existing, 
repeated)

Response: Acknowledged. Tracked for a follow-up submission.


Patch 5: fix spurious pattern sequence start in setup
-----------------------------------------------------

Comment 1: brightness_set() sleeps in atomic context. (Pre-existing, 
repeated)

Response: Acknowledged. Tracked for a follow-up submission.

Comment 2: brightness_set_blocking ignores the requested brightness
value. (Pre-existing, repeated)

Response: Acknowledged. Tracked for a follow-up submission.

Comment 3: Global duration registers clobbered by per-LED pattern
operations. (Pre-existing, repeated)

Response: Acknowledged. Tracked for a follow-up submission.

Comment 4: Unused trailing duration slots not cleared in pattern_set().
(Pre-existing, repeated)

Response: Acknowledged. Tracked for a follow-up submission.


Patch 6: fix brightness having no effect while pattern mode is active
---------------------------------------------------------------------

Comment 1: brightness_set() sleeps in atomic context. (Pre-existing, 
repeated)

Response: Acknowledged. Tracked for a follow-up submission.

Comment 2: Clearing PATS in brightness_set() halts patterns on all
other channels. Reviewer suggests an alternative: set all 8 PWM steps
for the target channel to full duty cycle instead of stopping the global
sequencer, so the channel appears at constant brightness without
disrupting other channels. (Pre-existing)

Response: Acknowledged. The alternative approach is noted as a potential
fix direction in the follow-up submission.


Patch 7: disable channel when brightness is set to zero
-------------------------------------------------------

Comment 1: brightness_set() sleeps in atomic context. (Pre-existing, 
repeated)

Response: Acknowledged. Tracked for a follow-up submission.

Comment 2: If brightness is set to 0 (channel disabled), a subsequent
pattern_set() starts the sequencer without re-enabling the channel,
so the pattern runs but emits no light.

Response: Real bug. pattern_set() does not call __st1202_channel_set()
to re-enable the channel, unlike blink_set() which does. To be fixed.

Comment 3: brightness_set_blocking ignores the requested brightness
value. (Pre-existing, repeated)

Response: Acknowledged. Tracked for a follow-up submission.


Patch 8: validate LED reg property against channel count
--------------------------------------------------------

Comment 1: reg is declared as signed int but populated via
of_property_read_u32(). A DT value >= 0x80000000 would be interpreted
as negative and bypass the reg >= ST1202_MAX_LEDS check, leading to a
negative out-of-bounds array access.

Response: Real bug in this patch. reg must be changed to u32. To be
fixed.

Comment 2: fwnode stored without incrementing the reference count,
potential use-after-free if the DT node is dynamically removed.
(Pre-existing, repeated)

Response: Acknowledged. Tracked for a follow-up submission.

Comment 3: brightness_set() sleeps in atomic context. (Pre-existing, 
repeated)

Response: Acknowledged. Tracked for a follow-up submission.


Patch 9: correct and extend hw_pattern documentation
-----------------------------------------------------

No issues raised.

--
Manuel Fombuena