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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] st1202_blink_set() incorrectly zeroes out PWM slots for other active channels, breaking solid-on LEDs and causing cross-channel interference.
- [High] st1202_led_pattern_clear() and st1202_blink_set() destructively clear shared global sequencer duration registers, halting hardware patterns on other channels.
--

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

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

The patch clears unused pattern slots (2 to 7) in st1202_blink_set() by
zeroing them out for the channel. This prevents a blinking channel from
emitting unrequested output when another channel programs longer hardware
patterns and re-arms the global sequencer.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 1267671557f09..b93828fbbe26c 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -361,9 +361,11 @@ static int st1202_blink_set(struct led_classdev *led_cdev,

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

Earlier in drivers/leds/leds-st1202.c:st1202_blink_set(), the code zeros
out other active channels:

	/* 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);
		if (ret)
			return ret;
	}

Because solid-on LEDs rely on having all their PWM slots set to
ST1202_PATTERN_PWM_FULL, overwriting their slots 0 and 1 to LED_OFF causes
them to incorrectly turn dark during the first two steps of the global
sequencer's cycle when any LED is set to blink mode via sysfs.

>  	if (ret)
>  		return ret;
>  
> -	ret = st1202_pwm_pattern_write(chip, led->led_num, 1, LED_OFF);
> -	if (ret)
> -		return ret;
> +	for (int pattern = 1; pattern < ST1202_MAX_PATTERNS; pattern++) {
> +		ret = st1202_pwm_pattern_write(chip, led->led_num, pattern, LED_OFF);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	ret = st1202_duration_pattern_write(chip, 0, on);
>  	if (ret)

[Severity: High]
This is a pre-existing issue, but do st1202_led_pattern_clear() and
st1202_blink_set() destructively clear shared global sequencer duration
registers?

Later in drivers/leds/leds-st1202.c:st1202_blink_set(), the duration registers
are reset to zero for all slots:

	for (int pattern = 2; pattern < ST1202_MAX_PATTERNS; pattern++) {
		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + pattern, 0);
		if (ret)
			return ret;
	}

And similarly in 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;
	...
		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + pattern, 0);

Since the ST1202 features a single, globally shared sequencer and duration
register set, zeroing these registers for one channel via sysfs truncates the
shared timeline. Does this prematurely halt or break all other channels that
are simultaneously using hardware blinking or patterns?

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