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

Ajith Anandhan
2025-11-07T14:40:23Z

On 10/30/25 11:24 PM, Jonathan Cameron wrote:
> On Thu, 30 Oct 2025 22:04:10 +0530
> Ajith Anandhan <ajithanandhan0406@gmail.com> 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@gmail.com>
> 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@gmail.com>
>> + */
>> +
>> +#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.