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
Signed-off-by: Ajith Anandhan <ajithanandhan0406@gmail.com>
---
drivers/iio/adc/Kconfig | 10 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ti-ads1120.c | 509 +++++++++++++++++++++++++++++++++++
3 files changed, 520 insertions(+)
create mode 100644 drivers/iio/adc/ti-ads1120.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 58a14e683..afb7895dd 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1655,6 +1655,16 @@ config TI_ADS1119
This driver can also be built as a module. If so, the module will be
called ti-ads1119.
+config TI_ADS1120
+ tristate "Texas Instruments ADS1120"
+ depends on SPI
+ help
+ Say yes here to build support for Texas Instruments ADS1120
+ 4-channel, 2kSPS, 16-bit delta-sigma ADC.
+
+ This driver can also be built as module. If so, the module
+ will be called ti-ads1120.
+
config TI_ADS124S08
tristate "Texas Instruments ADS124S08"
depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index d008f78dc..49c56b459 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -144,6 +144,7 @@ obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
obj-$(CONFIG_TI_ADS1100) += ti-ads1100.o
obj-$(CONFIG_TI_ADS1119) += ti-ads1119.o
+obj-$(CONFIG_TI_ADS1120) += ti-ads1120.o
obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
obj-$(CONFIG_TI_ADS1298) += ti-ads1298.o
obj-$(CONFIG_TI_ADS131E08) += ti-ads131e08.o
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>
+#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)
+#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
+
+#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 ADS1120_MODE_MASK GENMASK(4, 3)
+#define ADS1120_MODE_NORMAL 0x00
+
+#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
+
+struct ads1120_state {
+ struct spi_device *spi;
+ struct mutex lock;
+ /*
+ * Used to correctly align data.
+ * Ensure natural alignment for potential future timestamp support.
+ */
+ u8 data[4] __aligned(IIO_DMA_MINALIGN);
+
+ 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 },
+ { 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 const int ads1120_gain_values[] = { 1, 2, 4, 8, 16, 32, 64, 128 };
+
+#define ADS1120_CHANNEL(index) \
+{ \
+ .type = IIO_VOLTAGE, \
+ .indexed = 1, \
+ .channel = index, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+}
+
+static const struct iio_chan_spec ads1120_channels[] = {
+ ADS1120_CHANNEL(0),
+ ADS1120_CHANNEL(1),
+ ADS1120_CHANNEL(2),
+ ADS1120_CHANNEL(3),
+};
+
+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);
+}
+
+static int ads1120_reset(struct ads1120_state *st)
+{
+ int ret;
+
+ ret = ads1120_write_cmd(st, ADS1120_CMD_RESET);
+ if (ret)
+ return ret;
+
+ /*
+ * Datasheet specifies reset takes 50us + 32 * t(CLK)
+ * where t(CLK) = 1/4.096MHz. Use 200us to be safe.
+ */
+ usleep_range(200, 300);
+
+ return 0;
+}
+
+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;
+
+ 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;
+ goto found;
+ }
+ }
+ return -EINVAL;
+
+found:
+ config0 = (st->config[0] & ~ADS1120_GAIN_MASK) | gain_bits;
+ st->config[0] = config0;
+ st->gain = gain_val;
+
+ 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) {
+ config1 = (st->config[1] & ~ADS1120_DR_MASK) |
+ ads1120_datarates[i].reg_value;
+ 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);
+ 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]);
+
+ 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 */
+ 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);
+ 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);
+ 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 };
+
+ 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 const struct iio_info ads1120_info = {
+ .read_raw = ads1120_read_raw,
+ .write_raw = ads1120_write_raw,
+ .read_avail = ads1120_read_avail,
+};
+
+static int ads1120_init(struct ads1120_state *st)
+{
+ int ret;
+
+ /* Reset device to default state */
+ ret = ads1120_reset(st);
+ if (ret) {
+ dev_err(&st->spi->dev, "Failed to reset device\n");
+ return ret;
+ }
+
+ /*
+ * 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
+ */
+ 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;
+ 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 */
+ st->current_channel = -1;
+
+ return 0;
+}
+
+static int ads1120_probe(struct spi_device *spi)
+{
+ 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);
+
+ 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;
+ }
+
+ return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct of_device_id ads1120_of_match[] = {
+ { .compatible = "ti,ads1120" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, ads1120_of_match);
+
+static const struct spi_device_id ads1120_id[] = {
+ { "ads1120" },
+ { }
+};
+MODULE_DEVICE_TABLE(spi, ads1120_id);
+
+static struct spi_driver ads1120_driver = {
+ .driver = {
+ .name = "ads1120",
+ .of_match_table = ads1120_of_match,
+ },
+ .probe = ads1120_probe,
+ .id_table = ads1120_id,
+};
+module_spi_driver(ads1120_driver);
+
+MODULE_AUTHOR("Ajith Anandhan <ajithanandhan0406@gmail.com>");
+MODULE_DESCRIPTION("Texas Instruments ADS1120 ADC driver");
+MODULE_LICENSE("GPL");
--
2.34.1On 10/30/25 11:34 AM, Ajith Anandhan 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)
I don't think there is much point in having a configureble data rate
until we add buffered reads since direct mode is using single-shot
conversions. So we can save that for a later patch/series that also
implementes buffered reads based off of the DRDY interrupt.
Maybe I'm wrong though and being able to specify the conversion
time for a single sample is still important. For example, if it
has some filtering effect.
Otherwise, I would just set it to the "best" value for single-shot
mode (if there is one) and always use that one rate.
> - Single-shot conversion mode
>
> Link: https://www.ti.com/lit/gpn/ads1120
> Signed-off-by: Ajith Anandhan <ajithanandhan0406@gmail.com>
> ---
> drivers/iio/adc/Kconfig | 10 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ti-ads1120.c | 509 +++++++++++++++++++++++++++++++++++
> 3 files changed, 520 insertions(+)
> create mode 100644 drivers/iio/adc/ti-ads1120.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 58a14e683..afb7895dd 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1655,6 +1655,16 @@ config TI_ADS1119
> This driver can also be built as a module. If so, the module will be
> called ti-ads1119.
>
> +config TI_ADS1120
> + tristate "Texas Instruments ADS1120"
> + depends on SPI
> + help
> + Say yes here to build support for Texas Instruments ADS1120
> + 4-channel, 2kSPS, 16-bit delta-sigma ADC.
> +
> + This driver can also be built as module. If so, the module
> + will be called ti-ads1120.
> +
> config TI_ADS124S08
> tristate "Texas Instruments ADS124S08"
> depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d008f78dc..49c56b459 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -144,6 +144,7 @@ obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
> obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
> obj-$(CONFIG_TI_ADS1100) += ti-ads1100.o
> obj-$(CONFIG_TI_ADS1119) += ti-ads1119.o
> +obj-$(CONFIG_TI_ADS1120) += ti-ads1120.o
> obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
> obj-$(CONFIG_TI_ADS1298) += ti-ads1298.o
> obj-$(CONFIG_TI_ADS131E08) += ti-ads131e08.o
> 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
Prefer more specific GPL-2.0-only or GPL-2.0-or-later (your choice).
> +/*
> + * 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>
> +#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)
> +#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
> +
> +#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 ADS1120_MODE_MASK GENMASK(4, 3)
> +#define ADS1120_MODE_NORMAL 0x00
> +
> +#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
> +
> +struct ads1120_state {
> + struct spi_device *spi;
> + struct mutex lock;
> + /*
> + * Used to correctly align data.
> + * Ensure natural alignment for potential future timestamp support.
> + */
> + u8 data[4] __aligned(IIO_DMA_MINALIGN);
> +
> + u8 config[4];
> + int current_channel;
> + int gain;
Since inputs are multiplexed, we can make this gain a per-channel value, no?
It also sounds like that certain mux input have to have the PGA bypassed
which means they are limited to only 3 gain values. So these would have
a different scale_available attribute.
> + 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 },
> + { 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 const int ads1120_gain_values[] = { 1, 2, 4, 8, 16, 32, 64, 128 };
> +
> +#define ADS1120_CHANNEL(index) \
> +{ \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = index, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
is also need to actually make use of the function you implemented. ;-)
> +}
> +
> +static const struct iio_chan_spec ads1120_channels[] = {
> + ADS1120_CHANNEL(0),
> + ADS1120_CHANNEL(1),
> + ADS1120_CHANNEL(2),
> + ADS1120_CHANNEL(3),
The mux has 15 possible values, so I would expect 15 channels
to coorespond to all possible combinations. 8 differnetial
channels for the first 8, then these 4 single-ended channels.
Then a few more differential channels for the 3 diagnostic
inputs.
> +};
> +
> +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);
> +}
Can we use the regmap framework instead of writing our own?
> +
> +static int ads1120_reset(struct ads1120_state *st)
> +{
> + int ret;
> +
> + ret = ads1120_write_cmd(st, ADS1120_CMD_RESET);
> + if (ret)
> + return ret;
> +
> + /*
> + * Datasheet specifies reset takes 50us + 32 * t(CLK)
> + * where t(CLK) = 1/4.096MHz. Use 200us to be safe.
> + */
> + usleep_range(200, 300);
fsleep(200);
> +
> + return 0;
> +}
> +
> +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;
> +
> + 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;
> + goto found;
> + }
> + }
> + return -EINVAL;
> +
> +found:
> + config0 = (st->config[0] & ~ADS1120_GAIN_MASK) | gain_bits;
> + st->config[0] = config0;
> + st->gain = gain_val;
> +
> + 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) {
> + config1 = (st->config[1] & ~ADS1120_DR_MASK) |
> + ads1120_datarates[i].reg_value;
> + 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,
> + };
This should be split into two transfers (still only one SPI message).
Then we don't need to pad the buffers. Also, it seems to have one
more byte than needed. Only to to tx one byte then rx two bytes.
> +
> + ret = spi_sync_transfer(st->spi, &xfer, 1);
> + 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]);
> +
> + 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 */
> + 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;
> +
This could actually all be done in a single SPI message with 3
transers. The spi_transfer struct has delay fields that can
provide the delay instead of msleep().
> + 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);
> + 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);
> + 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 };
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + *vals = ads1120_gain_values;
> + *type = IIO_VAL_INT;
> + *length = ARRAY_SIZE(ads1120_gain_values);
Scale also depends on the reference voltage, so it isn't quite so simple.
> + 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 const struct iio_info ads1120_info = {
> + .read_raw = ads1120_read_raw,
> + .write_raw = ads1120_write_raw,
> + .read_avail = ads1120_read_avail,
> +};
> +
> +static int ads1120_init(struct ads1120_state *st)
> +{
> + int ret;
> +
> + /* Reset device to default state */
> + ret = ads1120_reset(st);
> + if (ret) {
> + dev_err(&st->spi->dev, "Failed to reset device\n");
> + return ret;
> + }
> +
> + /*
> + * Configure Register 0:
> + * - Input MUX: AIN0/AVSS (updated per channel read)
> + * - Gain: 1
> + * - PGA bypassed (lower power consumption)
Should extend the comment to say that if gain is set higher than 4,
this value is ignored, so it is safe to leave it set all the time.
> + */
> + 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
> + */
> + 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;
> + ret = ads1120_write_reg(st, ADS1120_REG_CONFIG2, st->config[2]);
> + if (ret)
> + return ret;
> +
> + /*
> + * Configure Register 3:
* - Internal voltage reference
* - No FIR filter
* - Power switch always open
> + * - IDAC1: Disabled
> + * - IDAC2: Disabled
> + * - DRDY mode: DOUT/DRDY pin behavior
> + */
> + st->config[3] = ADS1120_DRDYM_EN;
It doesn't make sense to enable the DRDY pin on the DOUT line unless
we know that it is wired up to an interrupt.
> + 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 */
> + st->current_channel = -1;
> +
> + return 0;
> +}
> +
> +static int ads1120_probe(struct spi_device *spi)
> +{
> + 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);
> +
> + 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;
> + }
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static const struct of_device_id ads1120_of_match[] = {
> + { .compatible = "ti,ads1120" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ads1120_of_match);
> +
> +static const struct spi_device_id ads1120_id[] = {
> + { "ads1120" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, ads1120_id);
> +
> +static struct spi_driver ads1120_driver = {
> + .driver = {
> + .name = "ads1120",
> + .of_match_table = ads1120_of_match,
> + },
> + .probe = ads1120_probe,
> + .id_table = ads1120_id,
> +};
> +module_spi_driver(ads1120_driver);
> +
> +MODULE_AUTHOR("Ajith Anandhan <ajithanandhan0406@gmail.com>");
> +MODULE_DESCRIPTION("Texas Instruments ADS1120 ADC driver");
> +MODULE_LICENSE("GPL");
On 10/31/25 2:43 AM, David Lechner wrote:
> On 10/30/25 11:34 AM, Ajith Anandhan 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)
> I don't think there is much point in having a configureble data rate
> until we add buffered reads since direct mode is using single-shot
> conversions. So we can save that for a later patch/series that also
> implementes buffered reads based off of the DRDY interrupt.
>
> Maybe I'm wrong though and being able to specify the conversion
> time for a single sample is still important. For example, if it
> has some filtering effect.
>
> Otherwise, I would just set it to the "best" value for single-shot
> mode (if there is one) and always use that one rate.
You're right. I'll remove the configurable data rate for now and
just use a fixed optimal rate (20 SPS as default since its highest
accuracy).
We'll add this back when implementing buffered reads with DRDY interrupt
support in a future patch series.
>
>> - Single-shot conversion mode
>>
>> Link: https://www.ti.com/lit/gpn/ads1120
>> Signed-off-by: Ajith Anandhan <ajithanandhan0406@gmail.com>
>> ---
>> drivers/iio/adc/Kconfig | 10 +
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/ti-ads1120.c | 509 +++++++++++++++++++++++++++++++++++
>> 3 files changed, 520 insertions(+)
>> create mode 100644 drivers/iio/adc/ti-ads1120.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 58a14e683..afb7895dd 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -1655,6 +1655,16 @@ config TI_ADS1119
>> This driver can also be built as a module. If so, the module will be
>> called ti-ads1119.
>>
>> +config TI_ADS1120
>> + tristate "Texas Instruments ADS1120"
>> + depends on SPI
>> + help
>> + Say yes here to build support for Texas Instruments ADS1120
>> + 4-channel, 2kSPS, 16-bit delta-sigma ADC.
>> +
>> + This driver can also be built as module. If so, the module
>> + will be called ti-ads1120.
>> +
>> config TI_ADS124S08
>> tristate "Texas Instruments ADS124S08"
>> depends on SPI
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index d008f78dc..49c56b459 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -144,6 +144,7 @@ obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
>> obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
>> obj-$(CONFIG_TI_ADS1100) += ti-ads1100.o
>> obj-$(CONFIG_TI_ADS1119) += ti-ads1119.o
>> +obj-$(CONFIG_TI_ADS1120) += ti-ads1120.o
>> obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
>> obj-$(CONFIG_TI_ADS1298) += ti-ads1298.o
>> obj-$(CONFIG_TI_ADS131E08) += ti-ads131e08.o
>> 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
> Prefer more specific GPL-2.0-only or GPL-2.0-or-later (your choice).
Noted.
>
>> +/*
>> + * 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>
>> +#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)
>> +#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
>> +
>> +#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 ADS1120_MODE_MASK GENMASK(4, 3)
>> +#define ADS1120_MODE_NORMAL 0x00
>> +
>> +#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
>> +
>> +struct ads1120_state {
>> + struct spi_device *spi;
>> + struct mutex lock;
>> + /*
>> + * Used to correctly align data.
>> + * Ensure natural alignment for potential future timestamp support.
>> + */
>> + u8 data[4] __aligned(IIO_DMA_MINALIGN);
>> +
>> + u8 config[4];
>> + int current_channel;
>> + int gain;
> Since inputs are multiplexed, we can make this gain a per-channel value, no?
Yes we can, However i want to keep the initial version simple so would
it be
fine to support per-channel gain configurationin upcoming patches?
>
> It also sounds like that certain mux input have to have the PGA bypassed
> which means they are limited to only 3 gain values. So these would have
> a different scale_available attribute.
Since, I'm gonna enable all the 15 channels. I believe we have to have
all
gains(for differential channels). Correct me if i'm wrong.
>
>> + 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 },
>> + { 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 const int ads1120_gain_values[] = { 1, 2, 4, 8, 16, 32, 64, 128 };
>> +
>> +#define ADS1120_CHANNEL(index) \
>> +{ \
>> + .type = IIO_VOLTAGE, \
>> + .indexed = 1, \
>> + .channel = index, \
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>> + BIT(IIO_CHAN_INFO_SCALE), \
>> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
>
> is also need to actually make use of the function you implemented. ;-)
Sure. I'll fix.
>
>> +}
>> +
>> +static const struct iio_chan_spec ads1120_channels[] = {
>> + ADS1120_CHANNEL(0),
>> + ADS1120_CHANNEL(1),
>> + ADS1120_CHANNEL(2),
>> + ADS1120_CHANNEL(3),
> The mux has 15 possible values, so I would expect 15 channels
> to coorespond to all possible combinations. 8 differnetial
> channels for the first 8, then these 4 single-ended channels.
> Then a few more differential channels for the 3 diagnostic
> inputs.
Sure, I'll add all the 15 channels.
>
>> +};
>> +
>> +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);
>> +}
> Can we use the regmap framework instead of writing our own?
I’d like to keep the first version simple so i will add the regmap
support in the
later patch since the single ended has less spi transaction to handle.
>
>> +
>> +static int ads1120_reset(struct ads1120_state *st)
>> +{
>> + int ret;
>> +
>> + ret = ads1120_write_cmd(st, ADS1120_CMD_RESET);
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * Datasheet specifies reset takes 50us + 32 * t(CLK)
>> + * where t(CLK) = 1/4.096MHz. Use 200us to be safe.
>> + */
>> + usleep_range(200, 300);
> fsleep(200);
I'll use fsleep instead of usleep_range.
>
>> +
>> + return 0;
>> +}
>> +
>> +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;
>> +
>> + 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;
>> + goto found;
>> + }
>> + }
>> + return -EINVAL;
>> +
>> +found:
>> + config0 = (st->config[0] & ~ADS1120_GAIN_MASK) | gain_bits;
>> + st->config[0] = config0;
>> + st->gain = gain_val;
>> +
>> + 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) {
>> + config1 = (st->config[1] & ~ADS1120_DR_MASK) |
>> + ads1120_datarates[i].reg_value;
>> + 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,
>> + };
> This should be split into two transfers (still only one SPI message).
> Then we don't need to pad the buffers. Also, it seems to have one
> more byte than needed. Only to to tx one byte then rx two bytes.
Sure, I'll fix.
>> +
>> + ret = spi_sync_transfer(st->spi, &xfer, 1);
>> + 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]);
>> +
>> + 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 */
>> + 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;
>> +
> This could actually all be done in a single SPI message with 3
> transers. The spi_transfer struct has delay fields that can
> provide the delay instead of msleep().
Good point. I'll defer this improvement to a subsequent patch.
>
>> + 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);
>> + 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);
>> + 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 };
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_SCALE:
>> + *vals = ads1120_gain_values;
>> + *type = IIO_VAL_INT;
>> + *length = ARRAY_SIZE(ads1120_gain_values);
> Scale also depends on the reference voltage, so it isn't quite so simple.
Yeah sure. I'll fix.
>
>> + 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 const struct iio_info ads1120_info = {
>> + .read_raw = ads1120_read_raw,
>> + .write_raw = ads1120_write_raw,
>> + .read_avail = ads1120_read_avail,
>> +};
>> +
>> +static int ads1120_init(struct ads1120_state *st)
>> +{
>> + int ret;
>> +
>> + /* Reset device to default state */
>> + ret = ads1120_reset(st);
>> + if (ret) {
>> + dev_err(&st->spi->dev, "Failed to reset device\n");
>> + return ret;
>> + }
>> +
>> + /*
>> + * Configure Register 0:
>> + * - Input MUX: AIN0/AVSS (updated per channel read)
>> + * - Gain: 1
>> + * - PGA bypassed (lower power consumption)
> Should extend the comment to say that if gain is set higher than 4,
> this value is ignored, so it is safe to leave it set all the time.
Noted.
>
>> + */
>> + 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
>> + */
>> + 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;
>> + ret = ads1120_write_reg(st, ADS1120_REG_CONFIG2, st->config[2]);
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * Configure Register 3:
> * - Internal voltage reference
> * - No FIR filter
> * - Power switch always open
>
>> + * - IDAC1: Disabled
>> + * - IDAC2: Disabled
>> + * - DRDY mode: DOUT/DRDY pin behavior
>> + */
>> + st->config[3] = ADS1120_DRDYM_EN;
> It doesn't make sense to enable the DRDY pin on the DOUT line unless
> we know that it is wired up to an interrupt.
Thanks for pointing it out. I'll address the this in V2 patch.
>> + 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 */
>> + st->current_channel = -1;
>> +
>> + return 0;
>> +}
>> +
>> +static int ads1120_probe(struct spi_device *spi)
>> +{
>> + 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);
>> +
>> + 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;
>> + }
>> +
>> + return devm_iio_device_register(&spi->dev, indio_dev);
>> +}
>> +
>> +static const struct of_device_id ads1120_of_match[] = {
>> + { .compatible = "ti,ads1120" },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, ads1120_of_match);
>> +
>> +static const struct spi_device_id ads1120_id[] = {
>> + { "ads1120" },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(spi, ads1120_id);
>> +
>> +static struct spi_driver ads1120_driver = {
>> + .driver = {
>> + .name = "ads1120",
>> + .of_match_table = ads1120_of_match,
>> + },
>> + .probe = ads1120_probe,
>> + .id_table = ads1120_id,
>> +};
>> +module_spi_driver(ads1120_driver);
>> +
>> +MODULE_AUTHOR("Ajith Anandhan <ajithanandhan0406@gmail.com>");
>> +MODULE_DESCRIPTION("Texas Instruments ADS1120 ADC driver");
>> +MODULE_LICENSE("GPL");
On 11/7/25 9:50 AM, Ajith Anandhan wrote:
> On 10/31/25 2:43 AM, David Lechner wrote:
>> On 10/30/25 11:34 AM, Ajith Anandhan wrote:
>>> Add driver for the Texas Instruments ADS1120, a precision 16-bit
>>> analog-to-digital converter with an SPI interface.
>>>
One note about the review process. Any suggestions you agree with, you
don't need to reply to specifically. You can trim out those parts in
your reply. It saves time for those reading the replies.
>>> +struct ads1120_state {
>>> + struct spi_device *spi;
>>> + struct mutex lock;
>>> + /*
>>> + * Used to correctly align data.
>>> + * Ensure natural alignment for potential future timestamp support.
>>> + */
>>> + u8 data[4] __aligned(IIO_DMA_MINALIGN);
>>> +
>>> + u8 config[4];
>>> + int current_channel;
>>> + int gain;
>> Since inputs are multiplexed, we can make this gain a per-channel value, no?
>
> Yes we can, However i want to keep the initial version simple so would it be
>
> fine to support per-channel gain configurationin upcoming patches?
Absolutely. I really appreciate splitting things up like that as it makes
it much easier to review.
>
>>
>> It also sounds like that certain mux input have to have the PGA bypassed
>> which means they are limited to only 3 gain values. So these would have
>> a different scale_available attribute.
>
> Since, I'm gonna enable all the 15 channels. I believe we have to have all
>
> gains(for differential channels). Correct me if i'm wrong.
Yes, that is how I understood the datasheet. Differential channels have all
gains. Single-ended channels and diagnostic channels only get the low gains
(1, 2, 4).
>>> +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);
>>> +}
>> Can we use the regmap framework instead of writing our own?
>
> I’d like to keep the first version simple so i will add the regmap support in the
>
> later patch since the single ended has less spi transaction to handle.
It would be less churn to implement the regmap right away. Typically
we try to avoid churn if we can help it. So this would be an exception
to the general suggestion that splitting things up is better.
On 11/7/25 9:48 PM, David Lechner wrote:
> On 11/7/25 9:50 AM, Ajith Anandhan wrote:
>> On 10/31/25 2:43 AM, David Lechner wrote:
>>> On 10/30/25 11:34 AM, Ajith Anandhan wrote:
>>>> Add driver for the Texas Instruments ADS1120, a precision 16-bit
>>>> analog-to-digital converter with an SPI interface.
>>>>
> One note about the review process. Any suggestions you agree with, you
> don't need to reply to specifically. You can trim out those parts in
> your reply. It saves time for those reading the replies.
>
>>>> +struct ads1120_state {
>>>> + struct spi_device *spi;
>>>> + struct mutex lock;
>>>> + /*
>>>> + * Used to correctly align data.
>>>> + * Ensure natural alignment for potential future timestamp support.
>>>> + */
>>>> + u8 data[4] __aligned(IIO_DMA_MINALIGN);
>>>> +
>>>> + u8 config[4];
>>>> + int current_channel;
>>>> + int gain;
>>> Since inputs are multiplexed, we can make this gain a per-channel value, no?
>> Yes we can, However i want to keep the initial version simple so would it be
>>
>> fine to support per-channel gain configurationin upcoming patches?
> Absolutely. I really appreciate splitting things up like that as it makes
> it much easier to review.
>
>>> It also sounds like that certain mux input have to have the PGA bypassed
>>> which means they are limited to only 3 gain values. So these would have
>>> a different scale_available attribute.
>> Since, I'm gonna enable all the 15 channels. I believe we have to have all
>>
>> gains(for differential channels). Correct me if i'm wrong.
> Yes, that is how I understood the datasheet. Differential channels have all
> gains. Single-ended channels and diagnostic channels only get the low gains
> (1, 2, 4).
>
>
>>>> +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);
>>>> +}
>>> Can we use the regmap framework instead of writing our own?
>> I’d like to keep the first version simple so i will add the regmap support in the
>>
>> later patch since the single ended has less spi transaction to handle.
> It would be less churn to implement the regmap right away. Typically
> we try to avoid churn if we can help it. So this would be an exception
> to the general suggestion that splitting things up is better.
Got it, I’ll add regmap support and address all review comments in the v2
patch series.
BR,
Ajith.
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
> 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.
> +#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.
> +#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).
> +
> +#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.
> +
> +#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
> +
> +#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.
> +
> +struct ads1120_state {
> + struct spi_device *spi;
> + struct mutex lock;
Locks should always have comments to say what data they protect.
> + /*
> + * 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.
> + */
> + 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.
> +
> + 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.
> + { 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.
> +}
> +
> +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.
> +
> + 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.
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()
> + 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.
> +
> + 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 =...
> + 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]
> + 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).
> + 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.
> +
> + 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?
> + 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.
> + 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.
> + 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.
> +
> + 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.
> + ret = ads1120_reset(st);
> + if (ret) {
> + dev_err(&st->spi->dev, "Failed to reset device\n");
> + return ret;
return dev_err_probe()
> + }
> +
> + /*
> + * 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.
> + */
> + 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.
> + 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.
> + 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;
> + 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.
> +
> + 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.
> + }
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> +}
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.
On Fri, 7 Nov 2025 20:10:15 +0530
Ajith Anandhan <ajithanandhan0406@gmail.com> wrote:
> 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
Hi Ajith,
A small process thing around efficiency.
Crop your reply to only include things where you are answering questions
or wish the discussion to focus. If you accept changes, just put that stuff
in the change log for the next version.
Save a lot of scrolling and makes it a lot less likely important stuff
will be lost in the noise!
> >> +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.
Ok. A bit more detail next time on what you want feedback on will
help focus things.
>
> >
> >> + ret = ads1120_write_cmd(st, ADS1120_CMD_START);
> >> + if (ret)
Thanks,
Jonathan
© 2016 - 2026 Red Hat, Inc.