Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Jonathan Cameron]

On Tue, 02 Jun 2026 14:40:47 +0200
"Javier Carrasco" <javier.carrasco.cruz@xxxxxxxxx> wrote:

> On Tue Jun 2, 2026 at 2:38 PM CEST, Jonathan Cameron wrote:
> >> >> +/*
> >> >> + * The gain selector encodes (PD_D4 << 2) | GAIN to identify each gain setting.
> >> >> + * Gains are multiplied by 8 to work with integers. The values in the iio-gts
> >> >> + * tables don't need corrections because the maximum value of the scale refers
> >> >> + * to GAIN = x1, and the rest of the values are obtained from the resulting
> >> >> + * linear function.
> >> >> + * TODO: add support for MILLI_GAIN_X165 and MILLI_GAIN_X660
> >> >> + */
> >> >> +#define VEML6031X00_SEL_MILLI_GAIN_X125  0x07
> >> >> +#define VEML6031X00_SEL_MILLI_GAIN_X250  0x04
> >> >> +#define VEML6031X00_SEL_MILLI_GAIN_X500  0x03
> >> >> +#define VEML6031X00_SEL_MILLI_GAIN_X1000 0x00
> >> >> +#define VEML6031X00_SEL_MILLI_GAIN_X2000 0x01  
> >> >
> >> > Not sure if these one-time use definitions improve or not the readability
> >> > of the code. Up to Jonathan.
> >> >  
> >>
> >> I prefer these definitions, and a similar pattern is used in multiple
> >> drivers in IIO, but I have no strong feelings about it.  
> >
> > Looking again at this, what do the numbers in the defines actually mean?
> > Seems a bit odd to have the base gain of 1 being called X125.
> > Maybe a comment on that would be useful.  I don't mind either way
> > on defines for this but if that number is useful to have I'd rather
> > have a define than a comment on each line.
> >  
> 
> I thought that MILLI_GAIN was already documenting what x125 is: 0.125 =
> 125 milli. More than the base gain of 1, it is the lowest gain you can
> configure.

Ah. The X location maybe what meant I didn't figure it out.
For similar the past we've done _0_125 .... _2_000 
which might be less confusing?

There isn't really a perfect answer for this stuff.

J
> 
> >>  
> >> >> +static const struct iio_gain_sel_pair veml6031x00_gain_sel[] = {
> >> >> +	GAIN_SCALE_GAIN(1, VEML6031X00_SEL_MILLI_GAIN_X125),
> >> >> +	GAIN_SCALE_GAIN(2, VEML6031X00_SEL_MILLI_GAIN_X250),
> >> >> +	GAIN_SCALE_GAIN(4, VEML6031X00_SEL_MILLI_GAIN_X500),
> >> >> +	GAIN_SCALE_GAIN(8, VEML6031X00_SEL_MILLI_GAIN_X1000),
> >> >> +	GAIN_SCALE_GAIN(16, VEML6031X00_SEL_MILLI_GAIN_X2000),
> >> >> +};  
> >> >
> >> > ...  
> 
>