Re: [PATCH v7 4/9] leds: st1202: set all pattern PWM slots to full after clearing pattern

[sashiko-bot@kernel.org]

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

New issues:
- [High] Trailing unused pattern slots retain 100% duty cycle configurations, causing unintended maximum brightness flashes during shorter sequences.

Pre-existing issues:
- [Critical] The driver assigns a sleeping function to the non-blocking `brightness_set` callback, causing kernel panics in atomic contexts.
- [High] Single-channel pattern operations uncontrollably zero out shared pattern durations, breaking active patterns on all other LEDs.
- [High] Pattern brightness values are written to 12-bit registers without scaling, causing extremely dim LED patterns.
- [High] The `brightness_set_blocking` callback fails to update the brightness level.
--

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

leds: st1202: set all pattern PWM slots to full after clearing pattern

This commit updates pattern_clear() to set all 8 PWM slots for a channel
to ST1202_PATTERN_PWM_FULL (4095). This ensures that the channel responds
to the ILED value in static mode and doesn't remain dark if the global
sequencer is started by another channel.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index b4adade49898e..3203b3768ec52 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
[ ... ]
> @@ -205,7 +206,7 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
>  		return ret;
>  
>  	for (int patt = 0; patt < ST1202_MAX_PATTERNS; patt++) {
> -		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, LED_OFF);
> +		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, ST1202_PATTERN_PWM_FULL);

[Severity: High]
If a pattern is later configured with len < 8 in st1202_led_pattern_set(),
will this cause the trailing unused slots to inherit a 100% duty cycle instead
of 0? 

If another channel concurrently sets an 8-step pattern (configuring the
globally shared step durations to non-zero values for all 8 steps), wouldn't
the first channel's LED flash at maximum brightness during the trailing unused
steps?

[Severity: High]
This is a pre-existing issue, but does the following line uncontrollably
zero out chip-wide shared duration registers for a single channel's pattern
clear, breaking active patterns on all other channels?

drivers/leds/leds-st1202.c:st1202_led_pattern_clear()
		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);

[Severity: Critical]
This is a pre-existing issue, but does assigning a sleeping function
to the non-blocking brightness_set callback cause a panic when invoked
from atomic contexts (like hardware triggers or timer interrupts)?

drivers/leds/leds-st1202.c:st1202_brightness_set()
	guard(mutex)(&chip->lock);
	st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);

[Severity: High]
This is a pre-existing issue, but are pattern brightness values written
to 12-bit registers without scaling?

drivers/leds/leds-st1202.c:st1202_led_pattern_set()
		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, pattern[patt].brightness);

Since ST1202 uses 12-bit PWM values (0 to 4095), writing the 8-bit
brightness value (0 to 255) directly into these registers without scaling
appears to cause extremely dim LED patterns.

[Severity: High]
This is a pre-existing issue, but does the brightness_set_blocking callback
fail to update the brightness level?

drivers/leds/leds-st1202.c:st1202_led_set()
	return st1202_channel_set(led->chip, led->led_num, !!value);

It looks like this discards the actual requested brightness value and acts
merely as an on/off boolean toggle, skipping writing the actual brightness
value to the ST1202_ILED_REG0 register.

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