* Re: [PATCH 1/1] leds: st1202: add hardware-accelerated blink support
2026-07-13 15:20 ` [PATCH 1/1] " Manuel Fombuena
@ 2026-07-13 15:37 ` sashiko-bot
2026-07-13 19:51 ` Manuel Fombuena
2026-07-23 12:13 ` Lee Jones
1 sibling, 1 reply; 5+ messages in thread
From: sashiko-bot @ 2026-07-13 15:37 UTC (permalink / raw)
To: Manuel Fombuena; +Cc: linux-leds, lee
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 <fombuena@outlook.com>
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] leds: st1202: add hardware-accelerated blink support
2026-07-13 15:20 ` [PATCH 1/1] " Manuel Fombuena
2026-07-13 15:37 ` sashiko-bot
@ 2026-07-23 12:13 ` Lee Jones
1 sibling, 0 replies; 5+ messages in thread
From: Lee Jones @ 2026-07-23 12:13 UTC (permalink / raw)
To: Manuel Fombuena; +Cc: pavel, vicentiu.galanopulo, linux-leds, linux-kernel
On Mon, 13 Jul 2026, Manuel Fombuena wrote:
> Implement blink_set() to enable hardware-accelerated blinking via the
> timer trigger. The LED1202 pattern engine is used to produce a two-step
> sequence: full brightness for delay_on, off for delay_off, repeating
> indefinitely.
>
> Requested delays are clamped to the hardware range [22ms, 5610ms] and
> rounded up to the nearest 22ms step. A zero delay is replaced with the
> default of 500ms independently for each of delay_on and delay_off.
>
> The LED1202 pattern sequencer is global and its timing registers are
> shared across all channels, so only one blink configuration can be
> active at a time. Other active channels have their PWM slots zeroed for
> both pattern steps so they remain dark rather than outputting unintended
> values when the sequencer runs. The target channel's ILED register is
> set to full brightness and the channel is enabled, since the timer
> trigger deactivates the current trigger before calling blink_set which
> would otherwise leave the channel disabled.
>
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
> Assisted-by: Claude:claude-sonnet-4-6
Should we avoid using non-standard metadata tags such as 'Assisted-by' in the
commit message to adhere to standard upstream practices?
> ---
> drivers/leds/leds-st1202.c | 82 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 82 insertions(+)
>
> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 168df5ecf27b..fc784a854a33 100644
> --- .../leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -15,6 +15,7 @@
> #include <linux/slab.h>
> #include <linux/string.h>
>
> +#define ST1202_BLINK_DEFAULT_DELAY 500
> #define ST1202_CHAN_DISABLE_ALL 0x00
> #define ST1202_CHAN_ENABLE_HIGH 0x03
> #define ST1202_CHAN_ENABLE_LOW 0x02
> @@ -275,6 +276,86 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
> return 0;
> }
>
> +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;
> +
> + on = *delay_on ? *delay_on : ST1202_BLINK_DEFAULT_DELAY;
> + off = *delay_off ? *delay_off : ST1202_BLINK_DEFAULT_DELAY;
Use the short form here:
on = *delay_on: ST1202_BLINK_DEFAULT_DELAY;
> + on = clamp_val(on, ST1202_MILLIS_PATTERN_DUR_MIN, ST1202_MILLIS_PATTERN_DUR_MAX);
> + off = clamp_val(off, ST1202_MILLIS_PATTERN_DUR_MIN, ST1202_MILLIS_PATTERN_DUR_MAX);
> + on = roundup(on, ST1202_MILLIS_PATTERN_DUR_MIN);
> + off = roundup(off, ST1202_MILLIS_PATTERN_DUR_MIN);
Should we perform the 'roundup' before 'clamp_val' to ensure that rounding the
value up does not push it beyond 'ST1202_MILLIS_PATTERN_DUR_MAX'?
> +
> + guard(mutex)(&chip->lock);
> +
> + ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
SHFT is weird - why save that very short char and harm readability?
> + if (ret)
> + return ret;
> +
> + /* Zero out PWM for all other active channels to prevent them from blinking */
> + for (int i = 0; i < ST1202_MAX_LEDS; i++) {
Does zeroing out the pattern PWM slots for other active channels permanently
overwrite their configurations or is there a mechanism to restore their state
once blinking is disabled?
> + 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;
> + }
> +
> + 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));
Should this function be named 'st1202_milliseconds_to_prescaler' instead,
since we are converting a millisecond value into a register value? Also,
could we correct the spelling of 'prescaler' and 'milliseconds' to ensure the
code is clean and passes spell checks?
> + 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;
> + }
> +
> + 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;
> +
> + ret = __st1202_channel_set(chip, led->led_num, true);
> + if (ret)
> + return ret;
> +
> + ret = st1202_write_reg(chip, ST1202_CONFIG_REG,
> + ST1202_CONFIG_REG_PATSR | ST1202_CONFIG_REG_PATS |
> + ST1202_CONFIG_REG_SHFT);
> + if (ret)
> + return ret;
> +
> + *delay_on = on;
> + *delay_off = off;
> +
> + return 0;
> +}
> +
> static int st1202_dt_init(struct st1202_chip *chip)
> {
> struct device *dev = &chip->client->dev;
> @@ -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;
> }
> --
> 2.55.0
>
--
Lee Jones
^ permalink raw reply [flat|nested] 5+ messages in thread