Re: [PATCH v7 3/3] leds: Add LED1202 I2C driver

[Vicentiu Galanopulo <vicentiu.galanopulo@remote-tech.co.uk>]

On Thu, Nov 21, 2024 at 06:10:00PM +0100, Vicentiu Galanopulo wrote:
> On Mon, Nov 18, 2024 at 09:13:32PM +0100, Christophe JAILLET wrote:
> > Le 18/11/2024 à 15:54, Vicentiu Galanopulo a écrit :
> > > The output current can be adjusted separately for each channel by 8-bit
> > > analog (current sink input) and 12-bit digital (PWM) dimming control. The
> > > LED1202 implements 12 low-side current generators with independent dimming
> > > control.
> > > Internal volatile memory allows the user to store up to 8 different patterns,
> > > each pattern is a particular output configuration in terms of PWM
> > > duty-cycle (on 4096 steps). Analog dimming (on 256 steps) is per channel but
> > > common to all patterns. Each device tree LED node will have a corresponding
> > > entry in /sys/class/leds with the label name. The brightness property
> > > corresponds to the per channel analog dimming, while the patterns[1-8] to the
> > > PWM dimming control.
> > > 
> > > Signed-off-by: Vicentiu Galanopulo <vicentiu.galanopulo@remote-tech.co.uk>
> > ...

Sorry for resending this, I pasted wrong in the previous one and didn't notice


> > > +{
> > > +	return value/ST1202_MILLIS_PATTERN_DUR_MIN - 1;
> > 
> > Can value be 0?
> > If yes, should the return value be clamped?
> 
 No, it's called in st1202_duration_pattern_write, which is mostly a wrapper,
 called in 2 places:
 line 225: ret = st1202_duration_pattern_write(chip, patt, ST1202_MILLIS_PATTERN_DUR_MIN);
 line 258: ret = st1202_duration_pattern_write(chip, patt, pattern[patt].delta_t); 
 
 for line 258, I make sure in line 250:
                          if (pattern[patt].delta_t < ST1202_MILLIS_PATTERN_DUR_MIN ||
 				pattern[patt].delta_t > ST1202_MILLIS_PATTERN_DUR_MAX)
 			           return -EINVAL;
 that it respects the range provided by the datasheet and supported by the chip.
 
>  
> > 
> > Is it needed?
> > (There is no i2c_get_clientdata())
> 
 Indeed, not needed so removed.
> 
>  
> > > +
> > 
> > CJ
> 
 Thank you very much for thanking the time to review!
 
 Kindest regards,
 Vicentiu