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

[Manuel Fombuena]

On Thu, 2026-08-13 at 13:16 +0100, Lee Jones wrote:
> On Thu, 06 Aug 2026, Manuel Fombuena wrote:
> 

> > +	for (int i = 0; i < ST1202_MAX_LEDS; i++) {
> 
> Nit: Would it be nicer to use a better named variable, like 'led' or
> 'chan'?

'led' is already in scope as the target channel. Using 'chan' in v7.

> > +		if (!chip->leds[i].is_active || i == led->led_num)
> > +			continue;
> 
> Nit: It's kinder on the eye if you separate these blocks.
> 
> > +		ret = st1202_pwm_pattern_write(chip, i, 0,
> > LED_OFF);
> > +		if (ret)
> > +			return ret;
> 
> '\n'
> 
> > +		ret = st1202_pwm_pattern_write(chip, i, 1,
> > LED_OFF);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	ret = st1202_pwm_pattern_write(chip, led->led_num, 0,
> > ST1202_PATTERN_PWM_FULL);
> > +	if (ret)
> > +		return ret;
> 
> '\n'
> 
> > +	ret = st1202_pwm_pattern_write(chip, led->led_num, 1,
> > LED_OFF);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = st1202_duration_pattern_write(chip, 0, on);
> > +	if (ret)
> > +		return ret;
> 
> '\n'

All fixed in v7.
> > 
> > +
> > +	ret = st1202_write_reg(chip, ST1202_CONFIG_REG,
> > +				ST1202_CONFIG_REG_PATSR |
> > ST1202_CONFIG_REG_PATS |
> > +				ST1202_CONFIG_REG_SHFT);
> 
> I'm assuming that this alignment is a patch artefact?

The continuation is indented with four tabs (32 columns at 8-wide),
which aligns with the first argument of st1202_write_reg(). The
alignment is correct in the source; the patch email renders tabs
differently.

-- 
Manuel Fombuena