Re: [PATCH v6 6/9] leds: st1202: fix brightness having no effect while pattern mode is active

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

New issues:
- [Medium] The newly added loop in `st1202_brightness_set` ignores I2C error returns, continuing blindly on bus failures.

Pre-existing issues:
- [High] The `st1202_brightness_set` function is incorrectly registered as the non-blocking `brightness_set` callback, but it sleeps by acquiring a mutex and performing I2C operations.
--

commit d9b4d02ea7dfd72ef99f875246743455506af12f
Author: Manuel Fombuena <[email protected]>

leds: st1202: fix brightness having no effect while pattern mode is active

This commit sets all 8 PWM slots for a channel to full duty cycle before
writing ILED to ensure brightness changes take effect when a hardware
pattern is running. It enables static brightness control without halting
the global sequencer.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index b8a5712829009..4909985ade04b 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -136,6 +136,8 @@ static void st1202_brightness_set(struct led_classdev *led_cdev,
>  
>  	guard(mutex)(&chip->lock);

[Severity: High]
This is a pre-existing issue, but does st1202_brightness_set() sleep in an
atomic context?

This function acquires a mutex and performs I2C operations which can sleep,
but it appears to be registered as the non-blocking brightness_set callback
in st1202_dt_init():

led->led_cdev.brightness_set = st1202_brightness_set;

Activating a software trigger (like 'timer' or 'heartbeat') executes
brightness_set() from a timer softirq context, where acquiring a mutex or
performing synchronous I2C operations is illegal and causes a system crash.

While not introduced by this patch, adding a loop inside this atomic
section that executes 16 additional synchronous I2C writes significantly
prolongs the illegal blocking duration.

> +	for (int patt = 0; patt < ST1202_MAX_PATTERNS; patt++)
> +		st1202_pwm_pattern_write(chip, led->led_num, patt, ST1202_PATTERN_PWM_FULL);

[Severity: Medium]
Is it safe to ignore the return value of st1202_pwm_pattern_write() here?

If an I2C transaction fails (e.g., due to a temporary bus error), this loop
does not break and unconditionally attempts all 8 iterations. Because the
underlying st1202_write_reg() internally calls dev_err() on failure, a
single bus error will cause 16 consecutive error messages to flood the
kernel log, while pointlessly blocking to attempt writes that are
guaranteed to fail.

Should the loop check the return value and abort early on failure?

>  	st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
>  }

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