Re: [PATCH 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() destructively zeroes the PWM pattern registers for all other active LED channels, forcing them dark and breaking their steady states.
- [Medium] Blink brightness is hardcoded to maximum (`U8_MAX`), ignoring user-configured levels.
- [Low] The commit message description is not capitalized, violating LED subsystem naming conventions.

Pre-existing issues:
- [High] The sleepable st1202_brightness_set() function is incorrectly assigned to the non-blocking brightness_set callback.
--

--- Patch [1]: [PATCH 1/1] leds: st1202: add hardware-accelerated blink support ---
commit 2db7bf5ac4afb8af5d3c0f74d2f24a829da7bc4f
Author: Manuel Fombuena <[email protected]>

leds: st1202: add hardware-accelerated blink support

This patch implements blink_set() to enable hardware-accelerated blinking via
the timer trigger. It uses the LED1202 pattern engine to sequence full
brightness and off periods, sharing timing registers globally.

> leds: st1202: add hardware-accelerated blink support

[Severity: Low]
This isn't a bug, but the subsystem guidelines request capitalizing the
description after the subsystem prefix. Should this be "Add
hardware-accelerated blink support"?

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 168df5ecf27b7..fc784a854a33b 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)
> +{
[ ... ]
> +	/* Zero out PWM for all other active channels to prevent them from blinking */
> +	for (int i = 0; i < ST1202_MAX_LEDS; i++) {
> +		if (!chip->leds[i].is_active || i == led->led_num)
> +			continue;
> +		ret = st1202_pwm_pattern_write(chip, i, 0, LED_OFF);
> +		if (ret)
> +			return ret;
> +		ret = st1202_pwm_pattern_write(chip, i, 1, LED_OFF);
> +		if (ret)
> +			return ret;
> +	}

[Severity: High]
Does this code force all other active LEDs on the same chip to go dark and
lose their configured states?

When hardware blinking is enabled on one LED, writing LED_OFF to the pattern 0
and 1 PWM registers for other active LEDs seems like it would permanently clear
their steady states (for instance, if they were previously configured to be
constantly ON).

> +
> +	ret = st1202_pwm_pattern_write(chip, led->led_num, 0, ST1202_PATTERN_PWM_FULL);
> +	if (ret)
> +		return ret;
> +	ret = st1202_pwm_pattern_write(chip, led->led_num, 1, LED_OFF);
> +	if (ret)
> +		return ret;
> +
> +	ret = st1202_write_reg(chip, ST1202_PATTERN_DUR,
> +				st1202_prescalar_to_miliseconds(on));
> +	if (ret)
> +		return ret;
> +	ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + 1,
> +				st1202_prescalar_to_miliseconds(off));
> +	if (ret)
> +		return ret;
> +
> +	for (int patt = 2; patt < ST1202_MAX_PATTERNS; patt++) {
> +		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);
> +		if (ret)
> +			return ret;
> +	}

[Severity: High]
Does clearing the global durations for patterns 2 through 7 here break custom
patterns that might be running on any other LEDs?

> +
> +	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);

[Severity: Medium]
Is it intentional to hardcode the blink brightness to maximum (U8_MAX)?

When the timer trigger starts a blink, it sets led_cdev->blink_brightness to
the user's desired intensity. Should this register write use that configured
level instead of always causing the LED to blink at full intensity?

[ ... ]
> @@ -301,6 +382,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;
>  		led->led_cdev.brightness_get = st1202_brightness_get;

[Severity: High]
This is a pre-existing issue, but should st1202_brightness_set() be assigned
to brightness_set?

The st1202_brightness_set() function acquires a mutex and performs I2C writes
which can sleep:

st1202_brightness_set() {
    guard(mutex)(&chip->lock);
    ...
}

The LED core uses the brightness_set callback in atomic contexts like software
timer interrupts. Should this driver only implement and assign
brightness_set_blocking to prevent scheduling while atomic bugs?

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