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

[Manuel Fombuena]

On Thu, 23 Jul 2026, Lee Jones wrote:

> > Signed-off-by: Manuel Fombuena <fombuena@xxxxxxxxxxx>
> > 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?

I'm going with the guidance found in
https://docs.kernel.org/process/coding-assistants.html which at present
still states it should be included. I read somewhere that there were
active discussions about this guideline and that the tag might be dropped
in the future, but I ignore the outcome. I will drop it if that is the
preference.

> > +	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;

Noted, will fix in v2.

> > +	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'?

Noted, will fix in v2.

> > +	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
>
> SHFT is weird - why save that very short char and harm readability?

ST1202_CONFIG_REG_SHFT is a pre-existing constant from the driver. I
intend to keep this submission focused on implementing blink support.
I have this issue and all the others identified during the previous
fix-focused submission registered for a subsequent submission.

> > +	/* 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?

Yes, it permanently overwrites them. The LED1202 has a single global
pattern sequencer with no snapshot-and-restore capability at the
hardware level. When blink is activated on one channel the sequencer is
reconfigured globally; other channels' PWM slots are intentionally set
to LED_OFF so they remain dark rather than blinking at the new timing.
This is described in the commit message.

> > +	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?

Agreed, but as with the above, this is pre-existing and registered for
a subsequent submission.

--
Manuel Fombuena