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

[David Lechner <dlechner@baylibre.com>]

On 11/9/25 8:11 AM, Ajith Anandhan wrote:
> Add driver for the Texas Instruments ADS1120, a precision 16-bit
> analog-to-digital converter with an SPI interface.
> 

...

> +#define ADS1120_CFG0_GAIN_MASK		GENMASK(3, 1)
> +#define ADS1120_CFG0_GAIN_1		0
> +#define ADS1120_CFG0_GAIN_2		1
> +#define ADS1120_CFG0_GAIN_4		2
> +#define ADS1120_CFG0_GAIN_8		3
> +#define ADS1120_CFG0_GAIN_16		4
> +#define ADS1120_CFG0_GAIN_32		5
> +#define ADS1120_CFG0_GAIN_64		6
> +#define ADS1120_CFG0_GAIN_128		7

We could avoid these macros by just doing ilog2(gain).

> +
> +#define ADS1120_CFG0_PGA_BYPASS		BIT(0)
> +
> +/* Config Register 1 bit definitions */
> +#define ADS1120_CFG1_DR_MASK		GENMASK(7, 5)
> +#define ADS1120_CFG1_DR_20SPS		0
> +#define ADS1120_CFG1_DR_45SPS		1
> +#define ADS1120_CFG1_DR_90SPS		2
> +#define ADS1120_CFG1_DR_175SPS		3
> +#define ADS1120_CFG1_DR_330SPS		4
> +#define ADS1120_CFG1_DR_600SPS		5
> +#define ADS1120_CFG1_DR_1000SPS		6

I would avoid writing macros for values we don't use yet. For example,
it may make more sense to have a lookup table when it comes to actually
implementing something that uses this.

Same applies to the rest below.

> +
> +#define ADS1120_CFG1_MODE_MASK		GENMASK(4, 3)
> +#define ADS1120_CFG1_MODE_NORMAL	0
> +#define ADS1120_CFG1_MODE_DUTY		1
> +#define ADS1120_CFG1_MODE_TURBO		2
> +
> +#define ADS1120_CFG1_CM_MASK		BIT(2)
> +#define ADS1120_CFG1_CM_SINGLE		0
> +#define ADS1120_CFG1_CM_CONTINUOUS	1
> +
> +#define ADS1120_CFG1_TS_EN		BIT(1)
> +#define ADS1120_CFG1_BCS_EN		BIT(0)
> +
> +/* Config Register 2 bit definitions */
> +#define ADS1120_CFG2_VREF_MASK		GENMASK(7, 6)
> +#define ADS1120_CFG2_VREF_INTERNAL	0
> +#define ADS1120_CFG2_VREF_EXT_REFP0	1
> +#define ADS1120_CFG2_VREF_EXT_AIN0	2
> +#define ADS1120_CFG2_VREF_AVDD		3
> +
> +#define ADS1120_CFG2_REJECT_MASK	GENMASK(5, 4)
> +#define ADS1120_CFG2_REJECT_OFF		0
> +#define ADS1120_CFG2_REJECT_50_60	1
> +#define ADS1120_CFG2_REJECT_50		2
> +#define ADS1120_CFG2_REJECT_60		3
> +
> +#define ADS1120_CFG2_PSW_EN		BIT(3)
> +
> +#define ADS1120_CFG2_IDAC_MASK		GENMASK(2, 0)
> +#define ADS1120_CFG2_IDAC_OFF		0
> +#define ADS1120_CFG2_IDAC_10UA		1
> +#define ADS1120_CFG2_IDAC_50UA		2
> +#define ADS1120_CFG2_IDAC_100UA		3
> +#define ADS1120_CFG2_IDAC_250UA		4
> +#define ADS1120_CFG2_IDAC_500UA		5
> +#define ADS1120_CFG2_IDAC_1000UA	6
> +#define ADS1120_CFG2_IDAC_1500UA	7
> +
> +/* Config Register 3 bit definitions */
> +#define ADS1120_CFG3_IDAC1_MASK		GENMASK(7, 5)
> +#define ADS1120_CFG3_IDAC1_DISABLED	0
> +#define ADS1120_CFG3_IDAC1_AIN0		1
> +#define ADS1120_CFG3_IDAC1_AIN1		2
> +#define ADS1120_CFG3_IDAC1_AIN2		3
> +#define ADS1120_CFG3_IDAC1_AIN3		4
> +#define ADS1120_CFG3_IDAC1_REFP0	5
> +#define ADS1120_CFG3_IDAC1_REFN0	6
> +
> +#define ADS1120_CFG3_IDAC2_MASK		GENMASK(4, 2)
> +#define ADS1120_CFG3_IDAC2_DISABLED	0
> +#define ADS1120_CFG3_IDAC2_AIN0		1
> +#define ADS1120_CFG3_IDAC2_AIN1		2
> +#define ADS1120_CFG3_IDAC2_AIN2		3
> +#define ADS1120_CFG3_IDAC2_AIN3		4
> +#define ADS1120_CFG3_IDAC2_REFP0	5
> +#define ADS1120_CFG3_IDAC2_REFN0	6
> +
> +#define ADS1120_CFG3_DRDYM_MASK		BIT(1)
> +#define ADS1120_CFG3_DRDYM_DRDY_ONLY	0
> +#define ADS1120_CFG3_DRDYM_BOTH		1
> +
> +/* Conversion time for 20 SPS */
> +#define ADS1120_CONV_TIME_MS		51
> +
> +/* Internal reference voltage in millivolts */
> +#define ADS1120_VREF_INTERNAL_MV	2048
> +
> +
> +struct ads1120_state {
> +	struct spi_device	*spi;
> +	struct regmap		*regmap;
> +	/*
> +	 * Protects chip configuration and ADC reads to ensure
> +	 * consistent channel/gain settings during conversions.
> +	 */
> +	struct mutex		lock;
> +
> +	int vref_mv;
> +
> +	/* DMA-safe buffer for SPI transfers */
> +	u8 data[4] __aligned(IIO_DMA_MINALIGN);
> +};
> +
> +
> +static const int ads1120_gain_values[] = { 1, 2, 4, 8, 16, 32, 64, 128 };
> +static const int ads1120_low_gain_values[] = { 1, 2, 4 };
> +
> +/* Differential channel macro */
> +#define ADS1120_DIFF_CHANNEL(index, chan1, chan2)		\
> +{								\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = chan1,					\
> +	.channel2 = chan2,					\
> +	.differential = 1,					\
> +	.address = index,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +			      BIT(IIO_CHAN_INFO_SCALE),		\
> +	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE), \
> +}
> +
> +/* Single-ended channel macro */
> +#define ADS1120_SINGLE_CHANNEL(index, chan)			\
> +{								\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = chan,					\
> +	.address = index,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +			      BIT(IIO_CHAN_INFO_SCALE),		\
> +	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE), \
> +}
> +
> +/* Diagnostic channel macro */
> +#define ADS1120_DIAG_CHANNEL(index, label)			\
> +{								\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = index,					\
> +	.address = index,					\
> +	.extend_name = label,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +			      BIT(IIO_CHAN_INFO_SCALE),		\
> +	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE), \
> +}
> +
> +static const struct iio_chan_spec ads1120_channels[] = {
> +	/* Differential inputs */
> +	ADS1120_DIFF_CHANNEL(ADS1120_CFG0_MUX_AIN0_AIN1, 0, 1),
> +	ADS1120_DIFF_CHANNEL(ADS1120_CFG0_MUX_AIN0_AIN2, 0, 2),
> +	ADS1120_DIFF_CHANNEL(ADS1120_CFG0_MUX_AIN0_AIN3, 0, 3),
> +	ADS1120_DIFF_CHANNEL(ADS1120_CFG0_MUX_AIN1_AIN2, 1, 2),
> +	ADS1120_DIFF_CHANNEL(ADS1120_CFG0_MUX_AIN1_AIN3, 1, 3),
> +	ADS1120_DIFF_CHANNEL(ADS1120_CFG0_MUX_AIN2_AIN3, 2, 3),
> +	ADS1120_DIFF_CHANNEL(ADS1120_CFG0_MUX_AIN1_AIN0, 1, 0),
> +	ADS1120_DIFF_CHANNEL(ADS1120_CFG0_MUX_AIN3_AIN2, 3, 2),
> +	/* Single-ended inputs */
> +	ADS1120_SINGLE_CHANNEL(ADS1120_CFG0_MUX_AIN0_AVSS, 0),
> +	ADS1120_SINGLE_CHANNEL(ADS1120_CFG0_MUX_AIN1_AVSS, 1),
> +	ADS1120_SINGLE_CHANNEL(ADS1120_CFG0_MUX_AIN2_AVSS, 2),
> +	ADS1120_SINGLE_CHANNEL(ADS1120_CFG0_MUX_AIN3_AVSS, 3),
> +	/* Diagnostic inputs */
> +	ADS1120_DIAG_CHANNEL(ADS1120_CFG0_MUX_REFP_REFN_4, "ref_div4"),
> +	ADS1120_DIAG_CHANNEL(ADS1120_CFG0_MUX_AVDD_AVSS_4, "avdd_div4"),
> +	ADS1120_DIAG_CHANNEL(ADS1120_CFG0_MUX_SHORTED, "shorted"),
> +};

Usually we don't make macros for the index. When it comes to adding
buffer support, having the macros makes it harder to see which scan_index
belongs to which channel. And if buffer support is planned for later,
it would make sense to use .scan_index instead of .address to avoid
duplicating that info later.


> +static int ads1120_set_gain(struct ads1120_state *st, int gain_val)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ads1120_gain_values); i++) {
> +		if (ads1120_gain_values[i] == gain_val)
> +			break;
> +	}
> +
> +	if (i == ARRAY_SIZE(ads1120_gain_values))
> +		return -EINVAL;

Since there is only one gain register, we should store this in a
per-channel variable, then set the gain in the register in
ads1120_set_mux() instead (and it would make sense to rename
that function as well to something like ads1120_configure_channel()).

> +
> +	return regmap_update_bits(st->regmap, ADS1120_REG_CONFIG0,
> +				  ADS1120_CFG0_GAIN_MASK,
> +				  FIELD_PREP(ADS1120_CFG0_GAIN_MASK, i));
> +}
> +

...

> +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, gain;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		guard(mutex)(&st->lock);
> +		ret = ads1120_read_measurement(st, chan, val);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		/*
> +		 * Scale = Vref / (gain * 2^15)
> +		 * Return in format: val / 2^val2
> +		 */
> +		gain = ads1120_get_gain(st);
> +		if (gain < 0)
> +			return gain;
> +
> +		*val = st->vref_mv;
> +		*val2 = gain * 15;

I think this would need to be `*val2 = ilog2(gain) + 15`.

> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +



> +
> +/* Regmap read function for ADS1120 */
> +static int ads1120_regmap_read(void *context, const void *reg_buf,
> +			       size_t reg_size, void *val_buf, size_t val_size)
> +{
> +	struct ads1120_state *st = context;
> +	u8 reg = *(u8 *)reg_buf;
> +	u8 *val = val_buf;
> +	int ret;
> +	struct spi_transfer xfer[2] = {
> +		{
> +			.tx_buf = st->data,
> +			.len = 1,
> +		}, {
> +			.rx_buf = val,
> +			.len = val_size,
> +		}
> +	};
> +
> +	if (reg > ADS1120_REG_CONFIG3)
> +		return -EINVAL;

This check should not be needed because of .maxregister.

> +
> +	/* RREG command: 0010rr00 where rr is register address */
> +	st->data[0] = ADS1120_CMD_RREG | (reg << 2);
> +
> +	ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer));
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +/* Regmap write function for ADS1120 */
> +static int ads1120_regmap_write(void *context, const void *data, size_t count)
> +{
> +	struct ads1120_state *st = context;
> +	const u8 *buf = data;
> +
> +	if (count != 2)
> +		return -EINVAL;
> +
> +	/* WREG command: 0100rr00 where rr is register address */
> +	st->data[0] = ADS1120_CMD_WREG | (buf[0] << 2);
> +	st->data[1] = buf[1];
> +
> +	return spi_write(st->spi, st->data, 2);
> +}

I don't see anyting unusal about these read/write functions. We should
be able to use the existing spi_regmap with the proper configuration
instead of making a custom regmap_bus.

> +
> +static const struct regmap_bus ads1120_regmap_bus = {
> +	.read = ads1120_regmap_read,
> +	.write = ads1120_regmap_write,
> +	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
> +	.val_format_endian_default = REGMAP_ENDIAN_BIG,
> +};
> +
> +static const struct regmap_config ads1120_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = ADS1120_REG_CONFIG3,
> +	.cache_type = REGCACHE_FLAT,
> +};
> +
> +static int ads1120_init(struct ads1120_state *st)
> +{
> +	int ret;

It's just a few extra lines to turn on the avdd supply here before
resettting.

> +
> +	ret = ads1120_reset(st);
> +	if (ret)
> +		return dev_err_probe(&st->spi->dev, ret,
> +					"Failed to reset device\n");
> +
> +	/*
> +	 * Configure Register 0:
> +	 * - Input MUX: AIN0/AVSS
> +	 * - Gain: 1
> +	 * - PGA bypass enabled. When gain is set > 4, this bit is
> +	 *   automatically ignored by the hardware and PGA is enabled,
> +	 *   so it's safe to leave it set.
> +	 */
> +	ret = regmap_write(st->regmap, ADS1120_REG_CONFIG0,
> +			   FIELD_PREP(ADS1120_CFG0_MUX_MASK,
> +				      ADS1120_CFG0_MUX_AIN0_AVSS) |
> +			   FIELD_PREP(ADS1120_CFG0_GAIN_MASK,
> +				      ADS1120_CFG0_GAIN_1) |
> +			   ADS1120_CFG0_PGA_BYPASS);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Configure Register 1:
> +	 * - Data rate: 20 SPS (for single-shot mode)
> +	 * - Operating mode: Normal
> +	 * - Conversion mode: Single-shot
> +	 * - Temperature sensor: Disabled
> +	 * - Burnout current: Disabled
> +	 */
> +	ret = regmap_write(st->regmap, ADS1120_REG_CONFIG1,
> +			   FIELD_PREP(ADS1120_CFG1_DR_MASK,
> +				      ADS1120_CFG1_DR_20SPS) |
> +			   FIELD_PREP(ADS1120_CFG1_MODE_MASK,
> +				      ADS1120_CFG1_MODE_NORMAL) |
> +			   FIELD_PREP(ADS1120_CFG1_CM_MASK,
> +				      ADS1120_CFG1_CM_SINGLE) |
> +			   FIELD_PREP(ADS1120_CFG1_TS_EN, 0) |
> +			   FIELD_PREP(ADS1120_CFG1_BCS_EN, 0));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Configure Register 2:
> +	 * - Voltage reference: Internal 2.048V
> +	 * - 50/60Hz rejection: Off
> +	 * - Power switch: Disabled
> +	 * - IDAC current: Off
> +	 */
> +	ret = regmap_write(st->regmap, ADS1120_REG_CONFIG2,
> +			   FIELD_PREP(ADS1120_CFG2_VREF_MASK,
> +				      ADS1120_CFG2_VREF_INTERNAL) |
> +			   FIELD_PREP(ADS1120_CFG2_REJECT_MASK,
> +				      ADS1120_CFG2_REJECT_OFF) |
> +			   FIELD_PREP(ADS1120_CFG2_PSW_EN, 0) |
> +			   FIELD_PREP(ADS1120_CFG2_IDAC_MASK,
> +				      ADS1120_CFG2_IDAC_OFF));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Configure Register 3:
> +	 * - IDAC1: Disabled
> +	 * - IDAC2: Disabled
> +	 * - DRDY mode: Only reflects data ready status
> +	 */
> +	ret = regmap_write(st->regmap, ADS1120_REG_CONFIG3,
> +			   FIELD_PREP(ADS1120_CFG3_IDAC1_MASK,
> +				      ADS1120_CFG3_IDAC1_DISABLED) |
> +			   FIELD_PREP(ADS1120_CFG3_IDAC2_MASK,
> +				      ADS1120_CFG3_IDAC2_DISABLED) |
> +			   FIELD_PREP(ADS1120_CFG3_DRDYM_MASK,
> +				      ADS1120_CFG3_DRDYM_DRDY_ONLY));
> +	if (ret)
> +		return ret;
> +

If we want to wait for a later patch to support external reference,
I would consider to check for the devicetree properties here and
print a warning that they aren't supported if present. Maybe others
think that is too much noise though? Especially if you plan to add
it soon.

> +	st->vref_mv = ADS1120_VREF_INTERNAL_MV;
> +
> +	return 0;
> +}
> +