Re: [PATCH v1 02/11] leds: st1202: Spell out the phase-shift configuration bit

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Blinking an LED destructively zeroes out the PWM values for all other channels, permanently breaking steady-ON LEDs.
- [High] Hardware state clobbering: per-LED pattern operations corrupt global sequencer configuration and step durations.
--

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

leds: st1202: Spell out the phase-shift configuration bit

This commit renames ST1202_CONFIG_REG_SHFT to ST1202_CONFIG_REG_PHASE_SHIFT
to improve readability and properly document the phase-shift delay feature.
It also adjusts line wrapping in the register writes to adhere to column
limits without introducing functional changes.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 5a2202fb4c223..d8a04e96fb46c 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
[ ... ]
> @@ -210,7 +211,7 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
>  
>  	guard(mutex)(&chip->lock);
>  
> -	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
> +	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_PHASE_SHIFT);

[Severity: High]
This is a pre-existing issue, but by writing only the phase-shift bit to the
global ST1202_CONFIG_REG, doesn't this unconditionally clear the PATS and
PATSR bits? 

Since the ST1202 chip appears to use a single global sequencer, clearing
these bits would instantly halt the sequencer for all other active LEDs on
the chip. Additionally, when setting patterns in st1202_led_pattern_set(),
the shared global ST1202_PATTERN_DUR step durations are unconditionally
overwritten. 

Does the driver need a way to coordinate or reject conflicting requests for
the global sequencer instead of silently clobbering the state of other LEDs?

[ ... ]
> @@ -299,7 +301,7 @@ static int st1202_blink_set(struct led_classdev *led_cdev,
>  
>  	guard(mutex)(&chip->lock);
>  
> -	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
> +	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_PHASE_SHIFT);

[Severity: High]
This isn't a bug introduced by this patch, but does st1202_blink_set()
inadvertently break steady-ON LEDs? 

When a given LED starts blinking, the function iterates over all other active
channels and unconditionally zero-fills their PWM slots to prevent them from
blinking:

st1202_pwm_pattern_write(chip, chan, 0, LED_OFF);
st1202_pwm_pattern_write(chip, chan, 1, LED_OFF);

If those other channels were previously configured to be steady ON, this zero
fill causes them to turn completely off during patterns 0 and 1. When the
blinking LED is later disabled, the original full PWM slots for the other
channels are never restored, which seems to leave them permanently disabled.

[ ... ]

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