Re: [PATCH v7 1/1] leds: st1202: Add hardware-accelerated blink support

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

New issues:
- [High] st1202_blink_set alters the hardware state of sibling LED channels by zeroing out their PWM configurations, permanently turning off solid-ON LEDs and desynchronizing their physical state from the LED subsystem.
- [High] Hardware blink implementation incorrectly intercepts 0-delay values, breaking timer trigger off/on states.
- [Medium] st1202_blink_set hardcodes maximum brightness, completely ignoring the user's requested blink brightness.

Pre-existing issues:
- [High] st1202_brightness_set is incorrectly assigned to the non-blocking brightness_set callback, violating the atomic context requirement.
--

--- Patch [1]: [PATCH v7 1/1] leds: st1202: Add hardware-accelerated blink support ---
commit 88e3d78cc1a73cce68a7f3ccf668fb1f654b398e
Author: Manuel Fombuena <[email protected]>

leds: st1202: Add hardware-accelerated blink support

This commit implements blink_set() to enable hardware-accelerated blinking
using the LED1202 pattern engine. Requested delays are clamped to hardware
ranges, and active channels have their PWM slots zeroed to prevent unintended
blinking when the sequencer runs.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 168df5ecf27b7..ead1441558497 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c

[ ... ]

> +static int st1202_blink_set(struct led_classdev *led_cdev,
> +			unsigned long *delay_on, unsigned long *delay_off)
> +{
> +	struct st1202_led *led = cdev_to_st1202_led(led_cdev);
> +	struct st1202_chip *chip = led->chip;
> +	unsigned long on, off;
> +	int ret;
> +
> +	if (!*delay_on)
> +		*delay_on = ST1202_BLINK_DEFAULT_DELAY;
> +	if (!*delay_off)
> +		*delay_off = ST1202_BLINK_DEFAULT_DELAY;

[Severity: High]
By silently overriding a requested zero delay to 500ms and returning success,
does this break the core's fallback contract?

When a user requests a steady state via the timer trigger, the core expects
the driver to return -EINVAL if it cannot natively support the zero-delay
condition. This expectation allows a software fallback via
led_set_software_blink().

> +
> +	on = *delay_on;

[ ... ]

> +	/* 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;
> +	}

[Severity: High]
Does this loop permanently turn off solid-ON sibling LEDs and desynchronize
their physical state from the LED subsystem?

The original PWM states of these independent channels are destructed here
without being tracked or restored later.

> +
> +	ret = st1202_pwm_pattern_write(chip, led->led_num, 0, ST1202_PATTERN_PWM_FULL);

[ ... ]

> +	ret = st1202_write_reg(chip, ST1202_PATTERN_REP, U8_MAX);
> +	if (ret)
> +		return ret;
> +
> +	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, U8_MAX);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Does this unconditionally hardcode the blink setting to maximum intensity?

This appears to completely ignore the user-configured led_cdev->blink_brightness
or fallback led_cdev->brightness states.

> +
> +	ret = __st1202_channel_set(chip, led->led_num, true);

[ ... ]

> @@ -301,6 +389,7 @@ static int st1202_dt_init(struct st1202_chip *chip)
>  		led->led_cdev.pattern_set = st1202_led_pattern_set;
>  		led->led_cdev.pattern_clear = st1202_led_pattern_clear;
>  		led->led_cdev.default_trigger = "pattern";
> +		led->led_cdev.blink_set = st1202_blink_set;
>  		led->led_cdev.brightness_set = st1202_brightness_set;

[Severity: High]
This is a pre-existing issue, but can this cause 'scheduling while atomic'
panics?

By assigning st1202_brightness_set() to the non-blocking brightness_set
callback, the LED core will invoke it directly from softirq/timer contexts.

Since st1202_brightness_set() performs sleeping I2C transfers and
immediately calls:

st1202_brightness_set()
    guard(mutex)(&chip->lock);

is this safe to run in an atomic context, or should this be mapped to
brightness_set_blocking instead?

>  		led->led_cdev.brightness_get = st1202_brightness_get;
>  	}

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