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