Re: [PATCH 1/1] leds: st1202: add hardware-accelerated blink support

Manuel Fombuena <[email protected]> Mon, 13 Jul 2026 20:51:40 +0100 (BST)
Newsgroups org.kernel.vger.linux-leds,dev.linux.lists.sashiko-reviews
Message-ID <GV1PR08MB8497264D9FF91843D6F2FE8AC5FA2@GV1PR08MB8497.eurprd08.prod.outlook.com>
> 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.

This is an inherent hardware constraint. The LED1202 uses a single global
pattern sequencer with shared timing registers, making it impossible for
two channels to run independent blink configurations simultaneously. When
blink_set() is called, the shared timing is reconfigured for the requested
delays and other channels' PWM slots are set to LED_OFF so they remain
dark rather than outputting unintended values. This behaviour is described
in the commit message.

> - [Medium] Blink brightness is hardcoded to maximum (`U8_MAX`), ignoring 
user-configured levels.

led_cdev->blink_brightness is set inside led_set_software_blink(), which
is the fallback path taken when blink_set() is absent or returns non-zero.
Since st1202_blink_set() returns 0 on success, led_set_software_blink() is
never reached and blink_brightness is not updated by the core before our
callback is invoked. Using it would risk reading 0 or a stale value from
a previous software blink, causing the LED to blink invisibly. U8_MAX is
intentional.

> - [Low] The commit message description is not capitalized, violating LED 
subsystem naming conventions.

Noted, it would be fixed in v2 if it was necessary to send it because of
code changes.
 
> Pre-existing issues:
> - [High] The sleepable st1202_brightness_set() function is incorrectly 
assigned to the non-blocking brightness_set callback.

This is a pre-existing issue outside the scope of this patch and will be
addressed in a follow-up submission.

--
Manuel Fombuena