Re: [PATCH v7 3/9] leds: st1202: fix pattern duration prescaler and pattern_clear skip marker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver improperly maps the single global ST1202 hardware pattern sequencer to independent per-LED operations, causing destructive interference between channels.
- [High] `st1202_led_pattern_set` does not clear trailing unused pattern slots, potentially executing stale delay steps if not preceded by `pattern_clear`.
--

commit b7d0b63932c36211ff0c19e9d1915d909b53394b
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: fix pattern duration prescaler and pattern_clear skip marker

This commit fixes the formula used to calculate the pattern duration
prescaler by removing an erroneous subtraction, updating the maximum
duration constant accordingly. It also explicitly writes 0 to the duration
registers in st1202_led_pattern_clear() so that unused slots are correctly
marked as skipped.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index a750c95ef3f89..b4adade49898e 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
[ ... ]
> @@ -209,7 +209,7 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
>  		if (ret != 0)
>  			return ret;
>  
> -		ret = st1202_duration_pattern_write(chip, patt, ST1202_MILLIS_PATTERN_DUR_MIN);
> +		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);

[Severity: High]
This isn't a bug introduced by this patch, but could zeroing out the duration
registers here freeze the sequence for other LEDs?

The ST1202 hardware uses a single, globally shared pattern sequencer with
shared step duration registers (ST1202_PATTERN_DUR). When
st1202_led_pattern_clear() is called for one LED, zeroing out these shared
duration registers would stop or corrupt the active pattern on any other LEDs
that are relying on the global sequencer.

Could this cause cross-channel interference, and should the driver manage the
sequencer state globally rather than on a per-LED basis?

[Severity: High]
This is also a pre-existing issue. Is st1202_led_pattern_set() leaving
trailing unused pattern slots active?

While st1202_led_pattern_clear() explicitly writes 0 to all 8 slots to mark
them as skipped, st1202_led_pattern_set() only updates the duration registers
up to the provided len. 

If a previous pattern was longer, won't the trailing duration registers still
contain non-zero values? This might cause the hardware sequencer to execute
those stale delay steps instead of skipping them, artificially lengthening
the new pattern's cycle time.

>  		if (ret != 0)
>  			return ret;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/GV1PR08MB8497C0B898789BB73ACE6EE3C5F52@GV1PR08MB8497.eurprd08.prod.outlook.com?part=3