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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux