* [PATCH v2 0/1] leds: st1202: Add hardware-accelerated blink support @ 2026-07-23 22:23 Manuel Fombuena 2026-07-23 22:26 ` [PATCH v2 1/1] " Manuel Fombuena 0 siblings, 1 reply; 3+ messages in thread From: Manuel Fombuena @ 2026-07-23 22:23 UTC (permalink / raw) To: lee, pavel, vicentiu.galanopulo, linux-leds, linux-kernel This patch adds blink_set() to the ST1202 LED driver, enabling hardware-accelerated blinking via the timer trigger. A series of nine fixes to the pattern engine and brightness handling was recently applied to for-leds-next: https://lore.kernel.org/all/GV1PR08MB8497C0B898789BB73ACE6EE3C5F52@GV1PR08MB8497.eurprd08.prod.outlook.com/ With those fixes in place, the pattern engine can be used reliably to implement blink_set(): a two-step pattern (full brightness for delay_on, off for delay_off) is programmed and started in infinite repeat mode. Requested delays are clamped to the hardware range and rounded up to the nearest 22ms step. During review of the fix series, several pre-existing issues were identified in the driver — including brightness_set() being assigned to a non-blocking callback, the global sequencer affecting all channels on pattern operations, and missing brightness scaling in pattern_set(). These do not affect blink_set(): the callback is not invoked from atomic context, the function explicitly programs all other channels' PWM slots to zero before starting the sequencer, and channel brightness is set directly via the ILED register. The pre-existing issues will be addressed in a follow-up submission. Tested on LED1202 hardware via I2C on a Linksys MX4200v2 router running OpenWrt. Hardware blinking confirmed functional with the timer trigger. --- Changes in v2 --- Use short-form ternary for default delay substitution. Perform roundup before clamp_val to ensure rounding cannot exceed the hardware maximum. Capitalise commit subject per LED subsystem convention. Drop Assisted-by tag per maintainer suggestion. v1: https://lore.kernel.org/all/GV1PR08MB8497C5B8CEB2CE19743DFCFFC5FA2@GV1PR08MB8497.eurprd08.prod.outlook.com/ Manuel Fombuena (1): leds: st1202: Add hardware-accelerated blink support drivers/leds/leds-st1202.c | 82 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) -- 2.55.0 ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v2 1/1] leds: st1202: Add hardware-accelerated blink support 2026-07-23 22:23 [PATCH v2 0/1] leds: st1202: Add hardware-accelerated blink support Manuel Fombuena @ 2026-07-23 22:26 ` Manuel Fombuena 2026-07-23 22:41 ` sashiko-bot 0 siblings, 1 reply; 3+ messages in thread From: Manuel Fombuena @ 2026-07-23 22:26 UTC (permalink / raw) To: lee, pavel, vicentiu.galanopulo, linux-leds, linux-kernel 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 rounded up to the nearest 22ms step then clamped to the hardware range [22ms, 5610ms]. 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> --- 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..fff42fc16441 100644 --- a/drivers/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 ?: ST1202_BLINK_DEFAULT_DELAY; + off = *delay_off ?: ST1202_BLINK_DEFAULT_DELAY; + + on = roundup(on, ST1202_MILLIS_PATTERN_DUR_MIN); + off = roundup(off, ST1202_MILLIS_PATTERN_DUR_MIN); + 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); + + guard(mutex)(&chip->lock); + + ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT); + 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++) { + 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)); + 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/1] leds: st1202: Add hardware-accelerated blink support 2026-07-23 22:26 ` [PATCH v2 1/1] " Manuel Fombuena @ 2026-07-23 22:41 ` sashiko-bot 0 siblings, 0 replies; 3+ messages in thread From: sashiko-bot @ 2026-07-23 22:41 UTC (permalink / raw) To: Manuel Fombuena; +Cc: lee, linux-leds Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: New issues: - [High] `st1202_blink_set` breaks all other active LEDs by destructively zeroing their PWM registers and clobbering global sequencer durations. - [Medium] `st1202_blink_set` unconditionally forces maximum brightness, ignoring the user's configured brightness level. - [Medium] Arithmetic overflow in delay calculations leads to clamping extremely large delays to the minimum duration instead of the maximum. Pre-existing issues: - [High] This problem wasn't introduced by this patch, but the driver incorrectly registers a sleeping function (`st1202_brightness_set`) as the non-blocking `brightness_set` callback, leading to 'scheduling while atomic' panics. -- --- Patch [1]: [PATCH v2 1/1] leds: st1202: Add hardware-accelerated blink support --- commit 0c45ca2d77a60a4118919a5f1d1afac3386caf4b Author: Manuel Fombuena <fombuena@outlook.com> leds: st1202: Add hardware-accelerated blink support 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. > diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c > index 168df5ecf27b7..fff42fc164412 100644 > --- a/drivers/leds/leds-st1202.c > +++ b/drivers/leds/leds-st1202.c [ ... ] > @@ -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 ?: ST1202_BLINK_DEFAULT_DELAY; > + off = *delay_off ?: ST1202_BLINK_DEFAULT_DELAY; > + > + on = roundup(on, ST1202_MILLIS_PATTERN_DUR_MIN); > + off = roundup(off, ST1202_MILLIS_PATTERN_DUR_MIN); [Severity: Medium] Will this cause an integer overflow if userspace provides a very large delay near ULONG_MAX for delay_on or delay_off? The roundup() macro performs an addition of the value and the divisor minus one. If the delay is extremely large, this addition can wrap around to a small number, which then mathematically rounds to 0. The subsequent clamp_val() will then erroneously clamp this 0 to the minimum hardware delay (22ms) instead of the maximum. Should the inputs be clamped before rounding? > + 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); > + > + guard(mutex)(&chip->lock); > + > + ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT); > + 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++) { > + if (!chip->leds[i].is_active || i == led->led_num) > + continue; > + ret = st1202_pwm_pattern_write(chip, i, 0, LED_OFF); [Severity: High] Does this inadvertently break independent channels that are configured to be solid-ON? This blindly zeroes the PWM slots of all other active LEDs. Since the hardware sequencer is global, when it runs for the blinking LED, it applies the 0 PWM to the solid-ON LEDs during steps 0 and 1, forcing them dark. [ ... ] > + 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); [Severity: High] Are global duration registers unconditionally overwritten here without tracking sequencer ownership or concurrency? [ ... ] > + 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 this bypassing and corrupting the subsystem's tracked brightness state? This unconditionally forces maximum brightness by writing U8_MAX to the ST1202_ILED_REG0 register, completely ignoring any user's previously configured brightness level. Furthermore, since st1202_brightness_get() reads directly from this register, does this permanently corrupt the internal sysfs state if the user had configured a lower brightness before activating the blink trigger? [ ... ] > @@ -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; [Severity: High] This is a pre-existing issue, but does assigning st1202_brightness_set to the non-blocking brightness_set callback lead to scheduling while atomic panics? The LED subsystem expects brightness_set to never sleep, calling it directly from atomic softirq contexts for software-based timer triggers. Because st1202_brightness_set() acquires a mutex and performs I2C writes, it will sleep in an atomic context. Should this driver leave brightness_set unassigned and rely solely on brightness_set_blocking to safely defer writes to a workqueue? -- Sashiko AI review · https://sashiko.dev/#/patchset/GV1PR08MB8497BAA7ABE9F424FD6B6153C5C02@GV1PR08MB8497.eurprd08.prod.outlook.com?part=1 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-07-23 22:41 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-23 22:23 [PATCH v2 0/1] leds: st1202: Add hardware-accelerated blink support Manuel Fombuena 2026-07-23 22:26 ` [PATCH v2 1/1] " Manuel Fombuena 2026-07-23 22:41 ` sashiko-bot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.