Re: [PATCH v6 3/9] leds: st1202: fix pattern duration prescaler and pattern_clear skip marker

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver exposes a single global hardware sequencer as a per-LED pattern, causing pattern operations on one channel to destructively overwrite and halt patterns on all other channels.
- [High] The `brightness_set` callback points to a sleeping function, while `brightness_set_blocking` fails to actually set the analog brightness.
- [Medium] `st1202_led_pattern_set()` fails to clear duration registers for unused pattern steps, causing stale steps to leak into new, shorter patterns.
- [Medium] Pattern brightness is written directly to 12-bit registers, but `max_brightness` is advertised as 8-bit (255), severely capping output.
--

commit 9be44f88b33b68ec231e7f9681b7161f1bb21c36
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: fix pattern duration prescaler and pattern_clear skip marker

The commit fixes the ST1202 LED driver where the pattern duration prescaler
calculation was off by one, causing durations to be too short. It corrects
the maximum duration constant to reflect the 8-bit register limit.
Additionally, it explicitly clears unused pattern slots by writing 0 to the
duration register to mark them as skipped.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index a750c95ef3f89..b4adade49898e 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
[ ... ]
> @@ -85,7 +85,7 @@ static int st1202_write_reg(struct st1202_chip *chip, int reg, uint8_t val)
>  
>  static uint8_t st1202_prescalar_to_miliseconds(unsigned int value)
>  {
> -	return value / ST1202_MILLIS_PATTERN_DUR_MIN - 1;
> +	return value / ST1202_MILLIS_PATTERN_DUR_MIN;
>  }
>  
>  static int st1202_pwm_pattern_write(struct st1202_chip *chip, int led_num,

[Severity: Medium]
This is a pre-existing issue, but is the brightness value getting scaled
properly to the hardware's 12-bit range?

In drivers/leds/leds-st1202.c:st1202_pwm_pattern_write():

    value_l = (u8)value;
    value_h = (u8)(value >> 8);

The user's requested pattern brightness is scaled to the advertised
max_brightness of 255, but the driver appears to pass this value directly
to the hardware's 12-bit PWM register (max 4095). 

Does this cause requesting maximum brightness to cap the PWM duty cycle
at roughly 6.2%, making patterns extremely dim?

[ ... ]
> @@ -209,7 +209,7 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
>  		if (ret != 0)
>  			return ret;
>  
> -		ret = st1202_duration_pattern_write(chip, patt, ST1202_MILLIS_PATTERN_DUR_MIN);
> +		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);
>  		if (ret != 0)
>  			return ret;
>  	}

[Severity: Medium]
This isn't a bug introduced by this patch, but while pattern_clear resets
all slots, does st1202_led_pattern_set() fail to clear duration registers
for unused pattern steps?

drivers/leds/leds-st1202.c:st1202_led_pattern_set() {
    for (int patt = 0; patt < len; patt++) {
        ret = st1202_pwm_pattern_write(chip, led->led_num, patt,
                                       pattern[patt].brightness);
        ...
        ret = st1202_duration_pattern_write(chip, patt,
                                            pattern[patt].delta_t);
    }
}

If a user programs a shorter pattern sequence after a longer one, the
loop updates only up to len steps, failing to zero out the durations of
the remaining ST1202_MAX_PATTERNS - len steps. 

Will the hardware sequencer continue to execute the stale un-cleared
trailing steps?

[Severity: High]
This is a pre-existing issue, but does clearing the pattern for one LED
destructively overwrite and halt patterns on all other channels?

In st1202_led_pattern_clear():

    ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);

And in st1202_duration_pattern_write():

    return st1202_write_reg(chip, (ST1202_PATTERN_DUR + pattern),
                            st1202_prescalar_to_miliseconds(value));

Since ST1202_PATTERN_DUR and ST1202_CONFIG_REG are globally shared
registers, it appears that programming or clearing a pattern on one LED
corrupts the duration registers and sequencer state of all other active
patterns. 

Is there a lack of isolation between independent LED devices on the same
chip?

[Severity: High]
This isn't a bug introduced by this patch, but are the brightness
callbacks mapped correctly in st1202_dt_init()?

drivers/leds/leds-st1202.c:st1202_dt_init() {
    ...
    led->led_cdev.brightness_set_blocking = st1202_led_set;
    led->led_cdev.brightness_set = st1202_brightness_set;
    ...
}

The brightness_set callback is strictly required to be non-blocking, but
it is mapped to st1202_brightness_set, which is an I2C-dependent sleeping
function. 

Could this cause a kernel panic (scheduling while atomic) if a kernel LED
trigger calls it from a softirq context?

Additionally, st1202_led_set() acts only as an on/off toggle:

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

Since this is assigned to brightness_set_blocking, it completely drops the
requested brightness magnitude. 

Will this break sysfs writes if blocking is required?

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