Re: [PATCH 2/2] iio: adc: Add driver for TI ADS1100 and ADS1000 chips

[Mike Looijmans]

Thanks for your quick feedback. Replies below (skipping signature added 
by borked mailserver)
I'll post a v2, want to do some testing first with the changes and I'll 
have hardware access by the end of next week.
No further comment from me means I agree and will change as requested.


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 17-02-2023 13:29, Andy Shevchenko wrote:
> On Fri, Feb 17, 2023 at 10:31:28AM +0100, Mike Looijmans wrote:
> > The ADS1100 is a 16-bit ADC (at 8 samples per second).
> > The ADS1000 is similar, but has a fixed data rate.
> Any Datasheet link available?

Yes, will add a friendly link.

> ...
>
> > +static const int ads1100_data_rate[] = {128, 32, 16, 8};
> > +static const int ads1100_data_rate_scale[] = {2048, 8192, 16384, 32768};
> > +static const int ads1100_gain[] = {1, 2, 4, 8};
> Do you need all of them as tables? They all can be derived from a single table
> or without any table at all (just three values).

I think the "data_rate" tables make the driver smaller in size, it's not 
a straight power-of-two list. I want them anyway for passing as "avail" 
sequences.

The "gain" is a simple power-of-two and might be replaced with code, but 
makes the "avail" more complex. Even in this case, the code to generate 
the list will be larger than the list itself, and I need the list in 
memory for the IIO_AVAIL function anyway.


> ...
>
> > +#ifdef CONFIG_PM
> Why?

I based this on the ADS1015 driver. Which appears to be outdated, I'll 
convert to DEFINE_RUNTIME_DEV_PM_OPS

+ int ret;
> > +	u8 buffer[2];
> __be16 buffer;
>
> > +
> > +	if (chan != 0)
> > +		return -EINVAL;
> > +
> > +	ret = i2c_master_recv(data->client, buffer, sizeof(buffer));
> > +	if (ret < 0) {
> > +		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	*val = (s16)(((u16)buffer[0] << 8) | buffer[1]);
> 	(s16)be16_to_cpu();
>
> But (s16) looks suspicious. Should you use sign_extend32()?

The chip always returns a 16-bit 2's complement value, even when only 12 
bits are actually used. I'll use sign_extend anyway, just to improve 
readability and take away doubts such as these.

--

Mike Looijmans