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

[Manuel Fombuena]

On Thu, 2026-08-06 at 14:45 +0100, Lee Jones wrote:
> On Fri, 24 Jul 2026, Manuel Fombuena wrote:
>
> > +	on = *delay_on ?: ST1202_BLINK_DEFAULT_DELAY;
> > +	off = *delay_off ?: ST1202_BLINK_DEFAULT_DELAY;
>
> Could we simplify this by initialising '*delay_on' and '*delay_off'
> first using standard 'if' statements, as is common in other
> 'blink_set' implementations?

Fixed in v6 using standard if statements.

> > +	on = min_t(unsigned long, on, ST1202_MILLIS_PATTERN_DUR_MAX);
> > +	off = min_t(unsigned long, off, ST1202_MILLIS_PATTERN_DUR_MAX);
>
> Since 'on', 'off', and the maximum duration are all of type
> 'unsigned long', should we use the simpler 'min()' macro here
> instead of 'min_t()'?

No longer applicable. v4 restructured the clamping and rounding,
removing min_t() entirely. v5 and v6 use clamp_val() before roundup().

> > +	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);
>
> Is the second 'clamp_val()' call redundant here?

Correct, and it was removed in v4 for the same reason. ST1202_MILLIS_
PATTERN_DUR_MAX is an exact multiple of ST1202_MILLIS_PATTERN_DUR_MIN,
so roundup() on a clamped value cannot exceed the maximum.

> > +	ret = st1202_write_reg(chip, ST1202_PATTERN_DUR,
> > +				st1202_prescalar_to_miliseconds(on));
>
> We know that neither of these words are spelt correctly, right?

Yes. Both are pre-existing in the driver and tracked for a follow-up
submission. In v5 and v6 the misspelled name no longer appears directly
in the new code. The calls go through st1202_duration_pattern_write().

> > +	for (int patt = 2; patt < ST1202_MAX_PATTERNS; patt++) {
>
> pattern

Fixed in v6.

--
Manuel Fombuena