Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Mike Looijmans]

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@xxxxxxxxxxxxxxxxx
W: www.topic.nl

Please consider the environment before printing this e-mail
On 06-03-2023 13:11, Andy Shevchenko wrote:
> On Sat, Mar 04, 2023 at 05:57:51PM +0000, Jonathan Cameron wrote:
> > On Tue, 28 Feb 2023 07:31:51 +0100
> > Mike Looijmans <mike.looijmans@xxxxxxxx> wrote:
> ...
>
> > > +	for (i = 0; i < 4; i++) {
> > > +		if (BIT(i) == gain) {
> > > +			ads1100_set_config_bits(data, ADS1100_PGA_MASK, i);
> > > +			return 0;
> > > +		}
> > > +	}
> > Andy's suggestion of something like..
> > 	if (!gain)
> > 		return -EINVAL;
> > 	i = ffs(gain);
> > 	if (i >= 4 || BIT(i) != gain)
> > 		return -EINVAL;
> >
> > 	ads...
> >
> > Is perhaps nicer than the loop.
> Even better:
>
> 	if (!gain || !is_power_of_2(gain))
> 		return -EINVAL;
>
> 	i = ffs(gain);
> 	if (i >= 4)
> 		return -EINVAL;
I'd guess that "is_power_of_2" is about the same complexity as "ffs".

If we want smaller code, in retrospect, I'd vote for omitting that 
power-of-two check altogether.

The IIO device reports this for a 3v3 Vdd:
# cat /sys/bus/iio/devices/iio\:device1/scale_available
0.100708007 0.050354003 0.025177001 0.012588500
# echo 0.012588500 > /sys/bus/iio/devices/iio:device1/scale

That last statement results in val2=12588 in this call.
The whole point of this exercise is that the value '0.012588' 
corresponds to a gain of '8' here. There's already quite a bit of 
rounding going on, since writing to the scale turns the value into 
"micro" scale.

Also, looking at the "avail" table. the values for "7" and "8" are much 
closer together than "3" and "4".

And the correct formula for the inverse of "gain = BIT(i);" is "i = 
ffs(gain) - 1;" because ffs(1) == 1

So I propose this code:

    if (gain <= 0 || gain > 8)
        return -EINVAL;

    regval = ffs(gain) - 1;
    ads1100_set_config_bits(data, ADS1100_PGA_MASK, regval);


--
Mike Looijmans