Re: [PATCH v2 00/10] leds: st1202: fix multiple bugs in pattern engine and brightness handling

[Lee Jones]

Can you take a look through these Sashiko reviews please:

https://sashiko.dev/#/patchset/WA0P291MB03855101311F0506B6448A8EC50D2%40WA0P291MB0385.POLP291.PROD.OUTLOOK.COM

> This series fixes several bugs in the LED1202 driver related to hardware
> pattern programming and brightness control. The issues were found during
> testing on a Linksys MX4200v2 router running OpenWrt.
> 
> --- Pattern sequence not stopped before reprogramming ---
> 
> The LED1202 datasheet (section 4.8) states that writes to PAT_REP and
> pattern duration registers are only applied after the sequence completes
> or is stopped. When running in infinite loop mode the sequence never
> completes on its own, so these writes are silently ignored by the
> hardware.
> 
>   Patch 1: Stop the running sequence by clearing PATS in the
>   configuration register at the start of both pattern_clear() and
>   pattern_set(), ensuring the hardware accepts new values immediately.
> 
>   Patch 2: Validate pattern input before stopping the sequence. An
>   out-of-range duration value should be rejected without disrupting a
>   running pattern, so input validation is moved ahead of the sequence
>   stop.
> 
> --- pattern_clear() leaving hardware in inconsistent state ---
> 
> Several independent bugs in pattern_clear() left the hardware in a state
> that affected subsequent pattern or brightness operations.
> 
>   Patch 3: Fix the duration prescaler formula. The original formula
>   (value / ST1202_MILLIS_PATTERN_DUR_MIN - 1) produced an off-by-one
>   result, writing the minimum valid duration (22ms, register value 0x01)
>   instead of the skip marker (0x00) for unused slots.
> 
>   Patch 4: Write the skip marker (0x00) directly to unused duration
>   registers in pattern_clear() rather than going through
>   st1202_duration_pattern_write(), which operates on millisecond values
>   and cannot express register value zero.
> 
>   Patch 5: Reset PAT_REP to its power-on default of 1 in pattern_clear().
>   A stale value — most critically 0xFF from a previous infinite loop —
>   would be picked up by a subsequent pattern_set() call in the window
>   between pattern_clear() returning and pattern_set() writing its own
>   value.
> 
>   Patch 6: Restore Pattern0 PWM to full brightness (0x0FFF) after
>   clearing. pattern_clear() zeroes all PWM slots as part of the clear,
>   but leaves Pattern0 at zero, so a subsequent direct brightness write
>   has no visible effect until Pattern0 PWM is restored.
> 
> --- Spurious pattern sequence start during setup ---
> 
>   Patch 7: Remove the write of PATS|PATSR to the configuration register
>   in st1202_setup(). This accidentally started a pattern sequence before
>   any pattern data was programmed, producing undefined output on startup.
> 
> --- Brightness control broken while pattern mode is active ---
> 
>   Patch 8: Exit pattern mode in brightness_set() before writing the ILED
>   register. With PATS set, the LED output is determined by the pattern
>   engine regardless of the ILED value, making direct brightness writes
>   have no visible effect while a pattern is active.
> 
>   Patch 9: Disable the hardware channel in brightness_set() when value
>   is zero. Previously only the ILED DAC was zeroed while the channel
>   remained enabled, causing residual current through the enabled channel
>   and a visible dim glow on the LED.
> 
> --- Documentation ---
> 
>   Patch 10: Correct and extend the hw_pattern documentation. The maximum
>   pattern duration was stated as 5660ms but the correct value derived
>   from the prescaler formula is 5610ms. The repeat field documentation
>   is also corrected and the brightness range is made explicit.
> 
> --- Testing ---
> 
> Tested on LED1202 hardware via I2C. Register state verified with i2cget
> at each step. Correct LED behaviour confirmed across pattern cycling,
> infinite repeat, pattern_clear, and direct brightness control with
> trigger=none.
> 
> --- Changes in v2 ---
> 
>   Patch 3: Fix commit message wording — programmed durations were ~22ms
>   too short, not too long.
>   Patch 7: Fix Signed-off-by typo.
>   Patch 10: Add missing quotes around commit subject in Fixes: tag.
> 
> v1: https://lore.kernel.org/all/WA0P291MB03850F4E9B4EC7389FE89779C5052@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> 
> Manuel Fombuena (10):
>   leds: st1202: stop pattern sequence before reprogramming
>   leds: st1202: validate pattern input before stopping the sequence
>   leds: st1202: fix pattern duration register calculation
>   leds: st1202: fix pattern_clear to explicitly mark unused slots as
>     skip
>   leds: st1202: reset PAT_REP to POR default in pattern_clear
>   leds: st1202: restore Pattern0 PWM to full on after clearing pattern
>   leds: st1202: fix spurious pattern sequence start in setup
>   leds: st1202: fix brightness having no effect while pattern mode is
>     active
>   leds: st1202: disable channel when brightness is set to zero
>   leds: st1202: correct and extend hw_pattern documentation
> 
>  Documentation/leds/leds-st1202.rst |  9 ++-
>  drivers/leds/leds-st1202.c         | 95 ++++++++++++++++++------------
>  2 files changed, 62 insertions(+), 42 deletions(-)
> 
> -- 
> 2.54.0
> 

-- 
Lee Jones