Re: [PATCH v7 9/9] leds: st1202: correct and extend hw_pattern documentation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Hardware pattern brightness values are not scaled to the 12-bit PWM range, resulting in extremely dim patterns. The documentation update masks this bug by incorrectly blessing the 0-255 range.
- [High] The driver lacks bounds checking for the `repeat` count, leading to silent integer truncation. The documentation update inappropriately enshrines this hardware artifact, breaking API consistency.
--

commit 77ce9e120b28b32a84e379b2ca92614b4ebbf1aa
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: correct and extend hw_pattern documentation

The commit updates the hardware pattern documentation for the ST1202 LED
driver. It adds a brightness range of 0-255 and clarifies that writing -1 or
255 causes infinite repeats. It also removes a redundant trailing sentence.

> diff --git a/Documentation/leds/leds-st1202.rst b/Documentation/leds/leds-st1202.rst
> index a2353549469ee..ed32eb3a27d45 100644
> --- a/Documentation/leds/leds-st1202.rst
> +++ b/Documentation/leds/leds-st1202.rst
> @@ -16,6 +16,7 @@ in terms of PWM duty-cycle and duration (ms).
>  To be compatible with the hardware pattern format, maximum 8 tuples of
>  brightness (PWM) and duration must be written to hw_pattern.
>  
> +- Brightness range: 0-255

[Severity: High]
This is a pre-existing issue, but documenting 0-255 here appears to mask an
underlying bug in the driver.

The 8-bit brightness value (0-255) from the hw_pattern sysfs input is passed
directly to the 12-bit PWM write function without scaling:

drivers/leds/leds-st1202.c:st1202_led_pattern_set() {
    ...
    ret = st1202_pwm_pattern_write(chip, led->led_num, patt, pattern[patt].brightness);
    ...
}

Should the driver be updated to scale the brightness properly (e.g.,
brightness * 4095 / 255) so the hardware pattern isn't permanently
restricted to ~6.2% of its maximum brightness?

>  - Min pattern duration: 22 ms
>  - Max pattern duration: 5610 ms
>  
> @@ -26,9 +27,7 @@ The format of the hardware pattern values should be:
>  ----------------------------
>  
>  Specify a pattern repeat number, which is common for all channels.
> -Default is 1; negative numbers and 0 are invalid.
> +Default is 1. Writing 0 is invalid. Writing -1 or 255 repeats the
> +pattern indefinitely.

[Severity: High]
This isn't a bug introduced by this patch, but updating the documentation
to bless 255 as an infinite repeat value breaks the generic ledtrig-pattern
ABI contract.

The driver currently lacks bounds checking and passes the unvalidated repeat
integer directly to the 8-bit register write:

drivers/leds/leds-st1202.c:st1202_led_pattern_set() {
    ...
    ret = st1202_write_reg(chip, ST1202_PATTERN_REP, repeat);
    ...
}

Because of the implicit truncation to uint8_t, writing 256 causes it to loop
0 times, and writing 255 triggers the hardware's infinite loop instead of
looping 255 times.

Could we fix the bounds checking and truncation bug in the driver instead of
enshrining this hardware limit in the documentation?

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