Re: [PATCH v1 07/11] leds: st1202: Program the channel current for hardware patterns

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] `st1202_led_pattern_clear` and `st1202_led_pattern_set` manage the global sequencer state and shared duration registers on a per-channel basis, corrupting other active patterns.
--

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

leds: st1202: Program the channel current for hardware patterns

This commit modifies the hardware pattern setup to properly program the
analog current register (ILED) so that programmed patterns are visible. It
also ensures the current is released when a pattern is cleared to avoid
latching the LED on.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 5558ac9612a15..5042fe88fbff7 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -221,6 +221,10 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
>  
>  	guard(mutex)(&chip->lock);
>  
> +	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, LED_OFF);
> +	if (ret != 0)
> +		return ret;
> +
>  	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_PHASE_SHIFT);
>  	if (ret != 0)
>  		return ret;

[Severity: High]
This is a pre-existing issue, but does writing ST1202_CONFIG_REG_PHASE_SHIFT
here abruptly halt the shared global sequencer for all channels?

Since st1202_led_pattern_clear() and st1202_led_pattern_set() are executed on
a per-channel basis via led_cdev.pattern_clear, clearing the PATS and PATSR
bits in the shared ST1202_CONFIG_REG appears to corrupt the state machine and
stop any hardware patterns currently executing on other LEDs on the same chip.

Additionally, further down in st1202_led_pattern_clear(), there is a loop
that writes 0 to ST1202_PATTERN_DUR + pattern. Can this zero out the chip-wide
global duration registers and destroy the pattern sequence timings for all
other active channels?

[ ... ]

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