Re: [PATCH v1 05/11] leds: st1202: Clear unused pattern slots in pattern_set()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Activating a blink on one channel forcibly turns off all other active LED channels during the blink cycle.
- [High] Clearing or blinking a pattern on one LED abruptly terminates or corrupts hardware patterns running on all other LEDs due to shared sequencer registers.
--

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

leds: st1202: Clear unused pattern slots in pattern_set()

This commit clears the PWM registers for unused slots in a pattern sequence,
setting them to LED_OFF. It fixes an issue where the channel's PWM for those
slots was left at ST1202_PATTERN_PWM_FULL, causing it to sit at full brightness
for the rest of the sequence. It leaves the shared duration registers alone to
avoid truncating longer patterns on other channels.

>     Zero this channel's PWM for the unused slots so it stays dark for the
>     remainder of the timeline, as st1202_blink_set() already does for the
>     channels it is not driving.

[Severity: High]
This isn't a bug introduced by this patch, but does st1202_blink_set()
corrupt the PWM state of independent channels?

When activating a blink on one channel, it forcibly turns off all other
active LED channels during the blink cycle by explicitly zeroing out PWM
slots 0 and 1 for all other active channels:

drivers/leds/leds-st1202.c:st1202_blink_set() {
...
	/* Zero out PWM for all other active channels to prevent them from blinking */
	for (int chan = 0; chan < ST1202_MAX_LEDS; chan++) {
		if (!chip->leds[chan].is_active || chan == led->led_num)
			continue;

		ret = st1202_pwm_pattern_write(chip, chan, 0, LED_OFF);
		if (ret)
			return ret;

		ret = st1202_pwm_pattern_write(chip, chan, 1, LED_OFF);
...
}

Because the ST1202 chip shares a single hardware sequencer, zeroing these
slots causes steady-state channels (which rely on full PWM across all slots)
to output zero and go completely dark while the sequencer loops.

>     The duration registers are deliberately left alone. All twelve channels
>     share them, so clearing them would truncate a longer pattern still
>     running elsewhere on the same device. A channel whose pattern is shorter
>     than the active timeline therefore keeps the longer period; that follows
>     from having a single hardware sequencer and is not something this can
>     correct.

[Severity: High]
This is a pre-existing issue, but does clearing a pattern on one LED abruptly
terminate or corrupt hardware patterns running on all other LEDs?

Calling st1202_led_pattern_clear() on one channel unconditionally clears the
sequencer enable bits (PATS and PATSR) in the chip-wide ST1202_CONFIG_REG,
and zeroes out the globally shared pattern duration registers
(ST1202_PATTERN_DUR):

drivers/leds/leds-st1202.c:st1202_led_pattern_clear() {
...
	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_PHASE_SHIFT);
	if (ret != 0)
		return ret;

	for (int pattern = 0; pattern < ST1202_MAX_PATTERNS; pattern++) {
		ret = st1202_pwm_pattern_write(chip, led->led_num, pattern,
						ST1202_PATTERN_PWM_FULL);
		if (ret != 0)
			return ret;

		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + pattern, 0);
...
}

Because the sequencer is a single shared hardware resource, per-channel
operations shouldn't unconditionally halt it or wipe its timeline if other
channels are actively using it. Hardware patterns on independent channels
might instantly fail or skip steps.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 90f69ac167147..9985b4b609891 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -263,6 +263,12 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
>  			return ret;
>  	}
>  
> +	for (int pattern = len; pattern < ST1202_MAX_PATTERNS; pattern++) {
> +		ret = st1202_pwm_pattern_write(chip, led->led_num, pattern, LED_OFF);
> +		if (ret != 0)
> +			return ret;
> +	}
> +
>  	ret = st1202_write_reg(chip, ST1202_PATTERN_REP, repeat);
>  	if (ret != 0)
>  		return ret;

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