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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Sun, Nov 09, 2025 at 07:41:19PM +0530, Ajith Anandhan 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.

Unneeded sentence in the commit message (may be located in the comment block,
though).

...

Many are still missing... Please, follow IWYU principle.

> +#include <linux/bitfield.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>

> +#include <linux/device.h>

Not see why you need this and not

dev_printk.h
device/devres.h

instead.

> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +#include <linux/unaligned.h>

...

> +/* Internal reference voltage in millivolts */
> +#define ADS1120_VREF_INTERNAL_MV	2048

_mV

*Yes, it's okay to use small letter in this case (it's all about proper units).

...

> +struct ads1120_state {
> +	struct spi_device	*spi;
> +	struct regmap		*regmap;

I'm not sure why do you need separate regmap and spi transactions at the same
time. The commit message also kept silent about this. Needs a justification.

In any case the spi device can be derived from regmap, so definitely you don't
need both.

> +	/*
> +	 * Protects chip configuration and ADC reads to ensure
> +	 * consistent channel/gain settings during conversions.
> +	 */
> +	struct mutex		lock;

No header for this type.

> +	int vref_mv;

_mV

> +	/* DMA-safe buffer for SPI transfers */
> +	u8 data[4] __aligned(IIO_DMA_MINALIGN);

No header for this type and __aligned attribute.

> +};

...

> +	struct spi_transfer xfer[2] = {

You may leave []

> +		{
> +			.tx_buf = st->data,
> +			.len = 1,
> +		}, {
> +			.rx_buf = st->data,
> +			.len = 2,
> +		}
> +	};
> +
> +	*val = sign_extend32(get_unaligned_be16(st->data), 15);

No header for this API.

> +	return 0;
> +}

...

> +static int ads1120_read_measurement(struct ads1120_state *st,
> +				    const struct iio_chan_spec *chan, int *val)
> +{
> +	int ret;
> +
> +	ret = ads1120_set_mux(st, chan->address);
> +	if (ret)
> +		return ret;
> +
> +	ret = ads1120_write_cmd(st, ADS1120_CMD_START);
> +	if (ret)
> +		return ret;

Needs a comment explaining this rather big delay.

> +	msleep(ADS1120_CONV_TIME_MS);
> +
> +	return ads1120_read_raw_adc(st, val);
> +}

...

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

Wondering if there is a correlation between count == 2 and this 2. If it has
1:1 relationship, perhaps use count directly here?

> +}

...

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

> +	.cache_type = REGCACHE_FLAT,

Why not MAPPLE? Or scattered FLAT?

> +};

...

> +static int ads1120_init(struct ads1120_state *st)
> +{
> +	int ret;

	struct device *dev = ... // from regmap

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

		return dev_err_probe(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) |

I would do it on a single line...

> +			   FIELD_PREP(ADS1120_CFG0_GAIN_MASK,
> +				      ADS1120_CFG0_GAIN_1) |

...and this despite being long, But it's up to you and maintainers.
Same for all similar cases.

> +			   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;
> +
> +	st->vref_mv = ADS1120_VREF_INTERNAL_MV;
> +
> +	return 0;
> +}

...

> +static int ads1120_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ads1120_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	st->regmap = devm_regmap_init(dev, &ads1120_regmap_bus, st,
> +				      &ads1120_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> +					"Failed to initialize regmap\n");
> +
> +	indio_dev->name = "ads1120";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ads1120_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ads1120_channels);

No header for ARRAY_SIZE().

> +	indio_dev->info = &ads1120_info;
> +
> +	ret = ads1120_init(st);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +					"Failed to initialize device\n");

Besides broken indentation this may be a single line.

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

-- 
With Best Regards,
Andy Shevchenko