Re: [PATCH v1 11/11] leds: st1202: Honour max_brightness when blinking

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Blinking one channel forcibly turns off all other active channels due to a shared global sequencer zeroing out their PWM slots.
- [High] st1202_led_set() passes the requested brightness directly to a u8 register write without capping it, causing silent integer truncation that can turn the LED off.
--

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

leds: st1202: Honour max_brightness when blinking

This commit updates the blinking path to respect the max-brightness property
defined in the device tree, capping the value to the width of the register.
It ensures the current is only raised after the channel is enabled and the
sequencer is started to prevent raising current for a failed blink.

>     The brightness and pattern paths both respect it. Blinking is the one
>     that does not, so a board that declares a lower ceiling holds it until
>     something enables the timer trigger.

[Severity: High]
This isn't a bug introduced by this patch, but does the brightness path
actually respect the maximum brightness properly?

Looking at st1202_led_set(), the value is directly passed to the register
write:

drivers/leds/leds-st1202.c:st1202_led_set() {
    ...
    ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
    if (ret)
        return ret;
    ...
}

Because st1202_write_reg() takes a u8 for the value, if a device tree
specifies a max-brightness greater than 255 (e.g., 256), a requested
brightness of 256 will silently truncate to 0 and turn the led off instead
of setting it to max.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index b93828fbbe26c..2114ef8a31194 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -385,10 +385,6 @@ static int st1202_blink_set(struct led_classdev *led_cdev,

[Severity: High]
This is a pre-existing issue, but does blinking one led forcibly break the
state of all other active leds?

Looking earlier in st1202_blink_set():

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

If a user configures one led to be continuously on and then configures a
second led to blink, won't this loop overwrite the first led's pwm slots
and turn it off during both sequencer phases?

If the driver left the other active leds' pwm slots at
ST1202_PATTERN_PWM_FULL, wouldn't they correctly remain solidly on without
blinking while the global sequencer runs?

>  	if (ret)
>  		return ret;
>  
> -	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, U8_MAX);
> -	if (ret)
> -		return ret;
> -
>  	ret = __st1202_channel_set(chip, led->led_num, true);
>  	if (ret)
>  		return ret;
> @@ -399,6 +395,11 @@ static int st1202_blink_set(struct led_classdev *led_cdev,
>  	if (ret)
>  		return ret;
>  
> +	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num,
> +				min_t(unsigned int, led_cdev->max_brightness, U8_MAX));
> +	if (ret)
> +		return ret;
> +
>  	*delay_on = on;
>  	*delay_off = off;
>

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