Re: [RFC PATCH 2/3] iio: adc: Add support for TI ADS1120

From: Ajith Anandhan

Date: Fri Nov 07 2025 - 09:40:23 EST

On 10/30/25 11:24 PM, Jonathan Cameron wrote:
On Thu, 30 Oct 2025 22:04:10 +0530
Ajith Anandhan <ajithanandhan0406@xxxxxxxxx> wrote:

Add driver for the Texas Instruments ADS1120, a precision 16-bit
analog-to-digital converter with an SPI interface.

The driver provides:
- 4 single-ended voltage input channels
- Programmable gain amplifier (1 to 128)
- Configurable data rates (20 to 1000 SPS)
- Single-shot conversion mode

Link: https://www.ti.com/lit/gpn/ads1120
Datasheet:

Signed-off-by: Ajith Anandhan <ajithanandhan0406@xxxxxxxxx>
Hi Ajith

Various comments inline. Mostly superficial stuff but the DMA safety
of SPI buffers needs fixing. There is a useful talk from this given
by Wolfram Sang if you want to understand more about this
https://www.youtube.com/watch?v=JDwaMClvV-s

Thanks,

Jonathan


Thank you for the detailed review and the helpful video reference!
diff --git a/drivers/iio/adc/ti-ads1120.c b/drivers/iio/adc/ti-ads1120.c
new file mode 100644
index 000000000..07a6fb309
--- /dev/null
+++ b/drivers/iio/adc/ti-ads1120.c
@@ -0,0 +1,509 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TI ADS1120 4-channel, 2kSPS, 16-bit delta-sigma ADC
+ *
+ * Datasheet: https://www.ti.com/lit/gpn/ads1120
+ *
+ * Copyright (C) 2025 Ajith Anandhan <ajithanandhan0406@xxxxxxxxx>
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
Figure out what you are actually using. There is ongoing effort to not include
kernel.h in drivers because there is usually a small set of more appropriate
fine grained includes.

Sure. I'll replace kernel.h with the specific includes needed.
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/spi/spi.h>
+
+#include <linux/iio/iio.h>
+
+/* Commands */
+#define ADS1120_CMD_RESET 0x06
+#define ADS1120_CMD_START 0x08
+#define ADS1120_CMD_PWRDWN 0x02
+#define ADS1120_CMD_RDATA 0x10
+#define ADS1120_CMD_RREG 0x20
+#define ADS1120_CMD_WREG 0x40
+
+/* Registers */
+#define ADS1120_REG_CONFIG0 0x00
+#define ADS1120_REG_CONFIG1 0x01
+#define ADS1120_REG_CONFIG2 0x02
+#define ADS1120_REG_CONFIG3 0x03
+
+/* Config Register 0 bit definitions */
+#define ADS1120_MUX_MASK GENMASK(7, 4)
+#define ADS1120_MUX_AIN0_AVSS 0x80
+#define ADS1120_MUX_AIN1_AVSS 0x90
+#define ADS1120_MUX_AIN2_AVSS 0xa0
+#define ADS1120_MUX_AIN3_AVSS 0xb0
+
+#define ADS1120_GAIN_MASK GENMASK(3, 1)
Better to name these so it's obvious which register they are in.
#define ADS1120_CFG0_GAIN_MSK or similar.
Saves anyone checking every time that it's being written to
the appropriate register.
I'll rename all masks to include register names.
+#define ADS1120_GAIN_1 0x00
+#define ADS1120_GAIN_2 0x02
+#define ADS1120_GAIN_4 0x04
+#define ADS1120_GAIN_8 0x06
+#define ADS1120_GAIN_16 0x08
+#define ADS1120_GAIN_32 0x0a
+#define ADS1120_GAIN_64 0x0c
+#define ADS1120_GAIN_128 0x0e
Same as called out for DR below. (applies in other places
as well).
Sure.
+
+#define ADS1120_PGA_BYPASS BIT(0)
+
+/* Config Register 1 bit definitions */
+#define ADS1120_DR_MASK GENMASK(7, 5)
+#define ADS1120_DR_20SPS 0x00
+#define ADS1120_DR_45SPS 0x20
+#define ADS1120_DR_90SPS 0x40
+#define ADS1120_DR_175SPS 0x60
+#define ADS1120_DR_330SPS 0x80
+#define ADS1120_DR_600SPS 0xa0
+#define ADS1120_DR_1000SPS 0xc0
Define these as values of the field (So not shifted)

#define ADS1120_DR_20SPS 0
#define ADS1120_DR_45SPS 1
etc.
Then use FIELD_PREP(ADS1120_DR_MASK, ADIS1120_DR_20SPS)
and similar to set them.
I'll use the FIELD_PREP and fix the values.

+
+#define ADS1120_MODE_MASK GENMASK(4, 3)
+#define ADS1120_MODE_NORMAL 0x00
No other values for a 2 bit field? Define all values even
if you only use one for now. Makes it easier to review the
values
I'll define all the available values.
+
+#define ADS1120_CM_SINGLE 0x00
+#define ADS1120_CM_CONTINUOUS BIT(2)
+
+#define ADS1120_TS_EN BIT(1)
+#define ADS1120_BCS_EN BIT(0)
+
+/* Config Register 2 bit definitions */
+#define ADS1120_VREF_MASK GENMASK(7, 6)
+#define ADS1120_VREF_INTERNAL 0x00
+#define ADS1120_VREF_EXT_REFP0 0x40
+#define ADS1120_VREF_EXT_AIN0 0x80
+#define ADS1120_VREF_AVDD 0xc0
+
+#define ADS1120_REJECT_MASK GENMASK(5, 4)
+#define ADS1120_REJECT_OFF 0x00
+#define ADS1120_REJECT_50_60 0x10
+#define ADS1120_REJECT_50 0x20
+#define ADS1120_REJECT_60 0x30
+
+#define ADS1120_PSW_EN BIT(3)
+
+#define ADS1120_IDAC_MASK GENMASK(2, 0)
+
+/* Config Register 3 bit definitions */
+#define ADS1120_IDAC1_MASK GENMASK(7, 5)
+#define ADS1120_IDAC2_MASK GENMASK(4, 2)
+#define ADS1120_DRDYM_EN BIT(1)
+
+/* Default configuration values */
+#define ADS1120_DEFAULT_GAIN 1
+#define ADS1120_DEFAULT_DATARATE 175
These should be just handled by writing the registers in init.
The defines in of themselves don't help over seeing the default
set in code.
Sure.I'll write it at init.

+
+struct ads1120_state {
+ struct spi_device *spi;
+ struct mutex lock;
Locks should always have comments to say what data they protect.
I'll add the appropriate comments.

+ /*
+ * Used to correctly align data.
+ * Ensure natural alignment for potential future timestamp support.
You don't do that except by coincidence of IIO_DMA_MINALIGN being large
enough. So this comment is misleading - Drop it.
Sure.

+ */
+ u8 data[4] __aligned(IIO_DMA_MINALIGN);
Unless everything after this is used for DMA buffers you have defeated
the point of the __aligned 'trick'. We need to ensure nothing that isn't
used for DMA and can potentially be modified in parallel with this
is not in the cache line. Probably a case of just moving data to the
end of the structure.
I'll move the data buffer to the end of the struct.

+
+ u8 config[4];
+ int current_channel;
+ int gain;
+ int datarate;
+ int conv_time_ms;
+};
+
+struct ads1120_datarate {
+ int rate;
+ int conv_time_ms;
+ u8 reg_value;
+};
+
+static const struct ads1120_datarate ads1120_datarates[] = {
+ { 20, 51, ADS1120_DR_20SPS },
As above, use the field values not the shifted ones.
Agreed.

+ { 45, 24, ADS1120_DR_45SPS },
+ { 90, 13, ADS1120_DR_90SPS },
+ { 175, 7, ADS1120_DR_175SPS },
+ { 330, 4, ADS1120_DR_330SPS },
+ { 600, 3, ADS1120_DR_600SPS },
+ { 1000, 2, ADS1120_DR_1000SPS },
+};
+static int ads1120_write_cmd(struct ads1120_state *st, u8 cmd)
+{
+ st->data[0] = cmd;
+ return spi_write(st->spi, st->data, 1);
+}
+
+static int ads1120_write_reg(struct ads1120_state *st, u8 reg, u8 value)
+{
+ u8 buf[2];
+
+ if (reg > ADS1120_REG_CONFIG3)
+ return -EINVAL;
+
+ buf[0] = ADS1120_CMD_WREG | (reg << 2);
+ buf[1] = value;
+
+ return spi_write(st->spi, buf, 2);
sizeof(buf);

However DMA safety thing applies here as well so you can't just use
a buffer in the stack. Can use spi_write_then_read() though as that bounce
buffers.
I prefer to use the global buffer for now.

+}
+
+static int ads1120_set_channel(struct ads1120_state *st, int channel)
+{
+ u8 mux_val;
+ u8 config0;
+
+ if (channel < 0 || channel > 3)
+ return -EINVAL;
+
+ /* Map channel to AINx/AVSS single-ended input */
+ mux_val = ADS1120_MUX_AIN0_AVSS + (channel << 4);
+
+ config0 = (st->config[0] & ~ADS1120_MUX_MASK) | mux_val;
+ st->config[0] = config0;
FIELD_MODIFY() after the defines are field values (not shifted version)
and that << 4 is gone.
I'll use the FIELD_MODIFY with proper arguments.

+
+ return ads1120_write_reg(st, ADS1120_REG_CONFIG0, config0);
+}
+
+static int ads1120_set_gain(struct ads1120_state *st, int gain_val)
+{
+ u8 gain_bits;
+ u8 config0;
+ int i;
+
+ /* Find gain in supported values */
+ for (i = 0; i < ARRAY_SIZE(ads1120_gain_values); i++) {
+ if (ads1120_gain_values[i] == gain_val) {
+ gain_bits = i << 1;
gain_bits = BIT(i);

+ goto found;
Avoid this goto by the following simplifying code flow.

I'll refactor it.


break;
+ }
+ }
if (i == ARRAY_SIZE(ads1120_gain_values)
return -EINVAL;

config0 = ...

+ return -EINVAL;
+
+found:
+ config0 = (st->config[0] & ~ADS1120_GAIN_MASK) | gain_bits;
+ st->config[0] = config0;
FIELD_MODIFY()
Sure.

+ st->gain = gain_val;
Similar to below - I'd not store this explicitly. Where it is used
is not a fast path so add loop to look it up there.

It's much easier to be sure there is no getting out of sync with
only one store of information, even if it does bloat the code a it.
MSTM. I'll correct it.

+
+ return ads1120_write_reg(st, ADS1120_REG_CONFIG0, config0);
+}
+
+static int ads1120_set_datarate(struct ads1120_state *st, int rate)
+{
+ u8 config1;
+ int i;
+
+ /* Find data rate in supported values */
+ for (i = 0; i < ARRAY_SIZE(ads1120_datarates); i++) {
+ if (ads1120_datarates[i].rate == rate) {
Flip logic to reduce indent
if (ads1120_datarates[i].rate != rate)
continue;

config1 =...
I'll follow the same.
+ config1 = (st->config[1] & ~ADS1120_DR_MASK) |
+ ads1120_datarates[i].reg_value;
FIELD_MODIFY() once reg_value is the field value not a shifted version of it.
And operate directly on st->config[1]
Agreed.

+ st->config[1] = config1;
+ st->datarate = rate;
+ st->conv_time_ms = ads1120_datarates[i].conv_time_ms;
+
+ return ads1120_write_reg(st, ADS1120_REG_CONFIG1,
+ config1);
+ }
+ }
+
+ return -EINVAL;
+}
+
+static int ads1120_read_raw_adc(struct ads1120_state *st, int *val)
+{
+ u8 rx_buf[4];
+ u8 tx_buf[4] = { ADS1120_CMD_RDATA, 0xff, 0xff, 0xff };
+ int ret;
+ struct spi_transfer xfer = {
+ .tx_buf = tx_buf,
+ .rx_buf = rx_buf,
+ .len = 4,
+ };
+
+ ret = spi_sync_transfer(st->spi, &xfer, 1);
SPI requires DMA safe buffers. 2 ways to make that true
here.
1. Put a buffer at the end of iio_priv() structure and mark it
__aligned(IIO_DMA_MINALIGN);
2. Allocate on the heap here.
(Can't use spi_write_then read() here if those '0xff's are required values).
I have chose to move(option 1) the buffer to the end of structure.
+ if (ret)
+ return ret;
+
+ /*
+ * Data format: 16-bit two's complement MSB first
+ * rx_buf[0] is echo of command
+ * rx_buf[1] is MSB of data
+ * rx_buf[2] is LSB of data
+ */
+ *val = (s16)((rx_buf[1] << 8) | rx_buf[2]);
*val = sign_extend32(get_unaligned_be16(&rx_buf[1]), 15);
or something along those lines.
I'll fix.

+
+ return 0;
+}
+
+static int ads1120_read_measurement(struct ads1120_state *st, int channel,
+ int *val)
+{
+ int ret;
+
+ ret = ads1120_set_channel(st, channel);
+ if (ret)
+ return ret;
+
+ /* Start single-shot conversion */
This all seems fairly standard so not sure what your RFC question was
looking for feedback on wrt to how you did single conversions?

I was indeed concerned about using the polling(adding wait) method to read adc values.

That's the reason i have asked it in the cover letter.


+ ret = ads1120_write_cmd(st, ADS1120_CMD_START);
+ if (ret)
+ return ret;
+
+ /* Wait for conversion to complete */
+ msleep(st->conv_time_ms);
+
+ /* Read the result */
+ ret = ads1120_read_raw_adc(st, val);
+ if (ret)
+ return ret;
+
+ st->current_channel = channel;
+
+ return 0;
+}
+
+static int ads1120_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct ads1120_state *st = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ mutex_lock(&st->lock);
+ ret = ads1120_read_measurement(st, chan->channel, val);
guard() as below.
Sure.

+ mutex_unlock(&st->lock);
+ if (ret)
+ return ret;
+ return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_SCALE:
+ *val = st->gain;
+ return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ *val = st->datarate;
+ return IIO_VAL_INT;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ads1120_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct ads1120_state *st = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ mutex_lock(&st->lock);
include cleanup.h and use

guard(mutex)(&st->lock;
return ads1120_set_gain(st, val);
here. Similar for other cases.
I'll include cleanup.h and use guard instead of handling mutex directly.
+ ret = ads1120_set_gain(st, val);
+ mutex_unlock(&st->lock);
+ return ret;
+
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ mutex_lock(&st->lock);
+ ret = ads1120_set_datarate(st, val);
+ mutex_unlock(&st->lock);
+ return ret;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ads1120_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long mask)
+{
+ static const int datarates[] = { 20, 45, 90, 175, 330, 600, 1000 };
I'd put this up alongside the register defines. Much easier to see it aligns
with those that way.
I'll fix.
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ *vals = ads1120_gain_values;
+ *type = IIO_VAL_INT;
+ *length = ARRAY_SIZE(ads1120_gain_values);
+ return IIO_AVAIL_LIST;
+
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ *vals = datarates;
+ *type = IIO_VAL_INT;
+ *length = ARRAY_SIZE(datarates);
+ return IIO_AVAIL_LIST;
+
+ default:
+ return -EINVAL;
+ }
+}
...

+
+static int ads1120_init(struct ads1120_state *st)
+{
+ int ret;
+
+ /* Reset device to default state */
I think that is is obvious enough from the function name that I'd
drop this comment.
I'll remove the comment.

+ ret = ads1120_reset(st);
+ if (ret) {
+ dev_err(&st->spi->dev, "Failed to reset device\n");
+ return ret;
return dev_err_probe()
Noted.

+ }
+
+ /*
+ * Configure Register 0:
+ * - Input MUX: AIN0/AVSS (updated per channel read)
+ * - Gain: 1
+ * - PGA bypassed (lower power consumption)
+ */
+ st->config[0] = ADS1120_MUX_AIN0_AVSS | ADS1120_GAIN_1 |
+ ADS1120_PGA_BYPASS;
+ ret = ads1120_write_reg(st, ADS1120_REG_CONFIG0, st->config[0]);
+ if (ret)
+ return ret;
+
+ /*
+ * Configure Register 1:
+ * - Data rate: 175 SPS
+ * - Mode: Normal
+ * - Conversion mode: Single-shot
+ * - Temperature sensor: Disabled
+ * - Burnout current: Disabled
If you want to make this more self documenting you can use
the definition changes above and
st->config[1] = FIELD_PREP(ADS1120_CFG1_DR_MASK, ADS1120_CFG1_DR_175_SPS) |
FIELD_PREP(ADS1120_CFG1_MODE_MASK, ADS1120_CFG1_MODE_NORMAL) |
FIELD_PREP(ADS1120_CFG1_CONTINOUS, 0) |
FIELD_PREP(ADS1120_CFG1_TEMP, 0) |
FIELD_PREP(ADS1120_CFG1_BCS, 0);
So provide field writes with 0 rather than setting them by their absence.
I'll use FIELD_PREP and add the fields with 0 to show their disable status.



+ */
+ st->config[1] = ADS1120_DR_175SPS | ADS1120_MODE_NORMAL |
+ ADS1120_CM_SINGLE;
+ ret = ads1120_write_reg(st, ADS1120_REG_CONFIG1, st->config[1]);
+ if (ret)
+ return ret;
+
+ /*
+ * Configure Register 2:
+ * - Voltage reference: AVDD
+ * - 50/60Hz rejection: Off
+ * - Power switch: Off
+ * - IDAC: Off
+ */
+ st->config[2] = ADS1120_VREF_AVDD | ADS1120_REJECT_OFF;
Currently config[2] and config[3] are unused outside this function.
Might make sense to use local variables for now.
I'll use local variables for config[2] and config[3].

+ ret = ads1120_write_reg(st, ADS1120_REG_CONFIG2, st->config[2]);
+ if (ret)
+ return ret;
+
+ /*
+ * Configure Register 3:
+ * - IDAC1: Disabled
+ * - IDAC2: Disabled
+ * - DRDY mode: DOUT/DRDY pin behavior
+ */
+ st->config[3] = ADS1120_DRDYM_EN;
+ ret = ads1120_write_reg(st, ADS1120_REG_CONFIG3, st->config[3]);
+ if (ret)
+ return ret;
+
+ /* Set default operating parameters */
+ st->gain = ADS1120_DEFAULT_GAIN;
+ st->datarate = ADS1120_DEFAULT_DATARATE;
+ st->conv_time_ms = 7; /* For 175 SPS */
I'd set these alongside where you do the writes.
Where possible just retrieve the value from what is cached in the config[]
registers rather than having two ways to get to related info.
Sure, I'll fix.

+ st->current_channel = -1;
+
+ return 0;
+}
+
+static int ads1120_probe(struct spi_device *spi)
+{
There are enough uses of spi->dev that I'd add a local variable
struct device *dev = &spi->dev;

I'll add local var to handle it.
+ struct iio_dev *indio_dev;
+ struct ads1120_state *st;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ st = iio_priv(indio_dev);
+ st->spi = spi;
+
+ mutex_init(&st->lock);
ret = devm_mutex_init(&spi->dev, st->lock);
if (ret)
return ret;

This helper is so easy to use it makes sense to use it here even though
the lock debugging it enables is unlikely to be particularly useful.
Agreed.

+
+ indio_dev->name = "ads1120";
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->channels = ads1120_channels;
+ indio_dev->num_channels = ARRAY_SIZE(ads1120_channels);
+ indio_dev->info = &ads1120_info;
+
+ ret = ads1120_init(st);
+ if (ret) {
+ dev_err(&spi->dev, "Failed to initialize device: %d\n", ret);
+ return ret;
For errors in code only called form probe path use
return dev_err_probe(&spi->dev, ret, "Failed to initialize device\n");

Whilst this particular path presumably doesn't defer it is still a useful
helper and pretty prints the return value + takes a few lines less than what
you currently have.
Agreed. I'll follow the same.

+ }
+
+ return devm_iio_device_register(&spi->dev, indio_dev);
+}

I’ll post v2 with these fixes shortly.

BR,

Ajith.