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

[Jonathan Cameron <jic23@kernel.org>]

On Sun,  9 Nov 2025 19:41:19 +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 supports:
> - Differential and single-ended input channels
> - Configurable gain (1-128 for differential, 1-4 for single-ended)
> - Internal 2.048V reference
> - Single-shot conversion mode
> 
> Also update MAINTAINER document.
> 
> Signed-off-by: Ajith Anandhan <ajithanandhan0406@gmail.com>
Hi Ajith

A few comments from me to add to Andy's review. 

Jonathan

> 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..1e1871b74
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1120.c

> +
> +/* 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),		\
I note that scale is the same for all channels. So why have separate
attributes?

> +	.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,					\

We very rarely allow extend)name in new drivers.
It is a real pain for userspace code to deal with, so we now
put that info behind the get_label() callback.

It's also fairly rare that we put diagnostic channels out.  Might
be better to push those to debugfs and keep the main interface
less confusing.

Shorted is sometimes done as a differential channel with itself
(doesn't matter which one). It's made a little more complex here
as it is specified as shorting both to (AVDD + AVDSS) / 2 .


> +	.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"),
> +};

> +
> +/* 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;
> +
> +	/* 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));

return spi_sync_transfer()

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

> +
> +static const struct regmap_config ads1120_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = ADS1120_REG_CONFIG3,
> +	.cache_type = REGCACHE_FLAT,

Andy covered this already but unless you have strong reason for
something else just use REGCACHE_MAPLE.  If you do have a reason
add a comment here to stop it being changed by someone else.

> +};

> +
> +static int ads1120_probe(struct spi_device *spi)
> +{

> +	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)
> +		return dev_err_probe(dev, ret,
> +					"Failed to initialize device\n");
Align as.
		return dev_err_probe(dev, ret,
				     "Failed to initialize device\n");

Same for all other similar cases.


> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}