Re: [PATCH v6 02/12] iio: adc: Add basic support for AD4170

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 18 Jun 2025 14:35:38 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:

> From: Ana-Maria Cusco <ana-maria.cusco@analog.com>
> 
> The AD4170 is a multichannel, low noise, 24-bit precision sigma-delta
> analog to digital converter. The AD4170 design offers a flexible data
> acquisition solution with crosspoint multiplexed analog inputs,
> configurable ADC voltage reference inputs, ultra-low noise integrated PGA,
> digital filtering, wide range of configurable output data rates, internal
> oscillator and temperature sensor, four GPIOs, and integrated features for
> interfacing with load cell weigh scales, RTD, and thermocouple sensors.
> 
> Add basic support for the AD4170 ADC with the following features:
> - Single-shot read.
> - Analog front end PGA configuration.
> - Differential and pseudo-differential input configuration.
> 
> Signed-off-by: Ana-Maria Cusco <ana-maria.cusco@analog.com>
> Co-developed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

One comment directing you back to some stuff that came in after you'd
posted this but was in the v5 thread.  The other is a random musing on
whether this should just use spi_write_then_read() to simplify things
at the cost of a few extra data copies.  For that I'm not that fussed
so it is entirely up to you.

Jonathan

> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 6516ccb4d63b..d99c35ff9a1c 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_AD4000) += ad4000.o
>  obj-$(CONFIG_AD4030) += ad4030.o
>  obj-$(CONFIG_AD4080) += ad4080.o
>  obj-$(CONFIG_AD4130) += ad4130.o
> +obj-$(CONFIG_AD4170) += ad4170.o
>  obj-$(CONFIG_AD4695) += ad4695.o
>  obj-$(CONFIG_AD4851) += ad4851.o
>  obj-$(CONFIG_AD7091R) += ad7091r-base.o
> diff --git a/drivers/iio/adc/ad4170.c b/drivers/iio/adc/ad4170.c
> new file mode 100644
> index 000000000000..58716ad6e7fc
> --- /dev/null
> +++ b/drivers/iio/adc/ad4170.c


> +static int ad4170_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> +	struct ad4170_state *st = context;
> +	unsigned int size;
> +	int ret;
> +
> +	ret = ad4170_get_reg_size(st, reg, &size);
> +	if (ret)
> +		return ret;
> +
> +	put_unaligned_be16(reg, st->tx_buf);
> +	switch (size) {
> +	case 3:
> +		put_unaligned_be24(val, &st->tx_buf[AD4170_SPI_INST_PHASE_LEN]);
> +		break;
> +	case 2:
> +		put_unaligned_be16(val, &st->tx_buf[AD4170_SPI_INST_PHASE_LEN]);
> +		break;
> +	case 1:
> +		st->tx_buf[AD4170_SPI_INST_PHASE_LEN] = val;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return spi_write(st->spi, st->tx_buf, AD4170_SPI_INST_PHASE_LEN + size);

If we did use the below suggestion and switch the read path to spi_write_then_read()
we could do the same here (with zero size write). 

Perhaps neither is a change worth doing though.  Up to you.


> +}
> +
> +static int ad4170_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +	struct ad4170_state *st = context;
> +	struct spi_transfer t[] = {
> +		{
> +			.tx_buf = st->tx_buf,
> +			.len = AD4170_SPI_INST_PHASE_LEN,
> +		},
> +		{
> +			.rx_buf = st->rx_buf,
> +		},

I wonder....  Would we better off just using spi_write_then_read() here
given they are all fairly small register sizes.  The big advantage
is we can skip messing around with dma safe buffers as spi_write_then_read
always bounces the data through some buffers the SPI core creates for this
purpose.

> +	};
> +	unsigned int size;
> +	int ret;
> +
> +	ret = ad4170_get_reg_size(st, reg, &size);
> +	if (ret)
> +		return ret;
> +
> +	put_unaligned_be16(AD4170_REG_READ_MASK | reg, st->tx_buf);
> +	t[1].len = size;
> +
> +	ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
> +	if (ret)
> +		return ret;
> +
> +	switch (size) {
> +	case 3:
> +		*val = get_unaligned_be24(st->rx_buf);
> +		return 0;
> +	case 2:
> +		*val = get_unaligned_be16(st->rx_buf);
> +		return 0;
> +	case 1:
> +		*val = st->rx_buf[0];
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}



> +static int ad4170_parse_adc_channel_type(struct device *dev,
> +					 struct fwnode_handle *child,
> +					 struct iio_chan_spec *chan)
> +{
> +	const char *propname, *propname2;
> +	int ret, ret2;
> +	u32 pins[2];
> +
> +	/* Parse pseudo-differential channel configuration */
> +	propname = "single-channel";
> +	propname2 = "common-mode-channel";
> +	ret = fwnode_property_read_u32(child, propname, &pins[0]);
> +	ret2 = fwnode_property_read_u32(child, propname2, &pins[1]);
> +	if (!ret && ret2)
> +		return dev_err_probe(dev, ret,
ret isn't appropriate to use here as it's 0 so you'll report success.
> +				     "When %s is defined, %s must be defined too\n",
> +				     propname, propname2);
> +
> +	if (!ret && !ret2) {

See feedback following from Dan's email in v5 thread that came in just after you'd
posted this.

Andy was keen that we do this differently and I think his suggestion makes sense.

> +		chan->differential = false;
> +		chan->channel = pins[0];
> +		chan->channel2 = pins[1];
> +		return 0;
> +	}
> +	/* Failed to parse pseudo-diff chan props so try diff chan */
> +
> +	/* Parse differential channel configuration */
> +	propname2 = "diff-channels";
> +	ret = fwnode_property_read_u32_array(child, propname2, pins,
> +					     ARRAY_SIZE(pins));
> +	if (!ret) {
> +		chan->differential = true;
> +		chan->channel = pins[0];
> +		chan->channel2 = pins[1];
> +		return 0;
> +	}
> +	return dev_err_probe(dev, ret, "Channel must define one of %s or %s.\n",
> +			     propname, propname2);
> +}