Re: [PATCH v3 02/10] iio: adc: Add basic support for AD4170

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 13 May 2025 09:34:00 -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
> aquisition 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>

A few minor things inline.

J

> diff --git a/drivers/iio/adc/ad4170.c b/drivers/iio/adc/ad4170.c
> new file mode 100644
> index 000000000000..bf19b31095ee
> --- /dev/null
> +++ b/drivers/iio/adc/ad4170.c

> +
> +/*
> + * Verifies whether the channel configuration is valid by checking the provided
> + * input type, polarity, and voltage references result in a sane input range.
> + * Returns negative error code on failure.
> + */
> +static int ad4170_get_input_range(struct ad4170_state *st,
> +				  struct iio_chan_spec const *chan,
> +				  unsigned int ch_reg, unsigned int ref_sel)
> +{
> +	bool bipolar = chan->scan_type.sign == 's';
> +	struct device *dev = &st->spi->dev;
> +	int refp, refn, ain_voltage, ret;
> +
> +	switch (ref_sel) {
> +	case AD4170_REF_REFIN1:
> +		if (st->vrefs_uv[AD4170_REFIN1P_SUP] == -ENODEV ||
> +		    st->vrefs_uv[AD4170_REFIN1N_SUP] == -ENODEV)
> +			return dev_err_probe(dev, -ENODEV,
> +					     "REFIN± selected but not provided\n");
> +
> +		refp = st->vrefs_uv[AD4170_REFIN1P_SUP];
> +		refn = st->vrefs_uv[AD4170_REFIN1N_SUP];
> +		break;
> +	case AD4170_REF_REFIN2:
> +		if (st->vrefs_uv[AD4170_REFIN2P_SUP] == -ENODEV ||
> +		    st->vrefs_uv[AD4170_REFIN2N_SUP] == -ENODEV)
> +			return dev_err_probe(dev, -ENODEV,
> +					     "REFIN2± selected but not provided\n");
> +
> +		refp = st->vrefs_uv[AD4170_REFIN2P_SUP];
> +		refn = st->vrefs_uv[AD4170_REFIN2N_SUP];
> +		break;
> +	case AD4170_REF_AVDD:
> +		refp = st->vrefs_uv[AD4170_AVDD_SUP];
> +		refn = st->vrefs_uv[AD4170_AVSS_SUP];
> +		break;
> +	case AD4170_REF_REFOUT:
> +		/* REFOUT is 2.5 V relative to AVSS */
> +		refp = st->vrefs_uv[AD4170_AVSS_SUP] + AD4170_INT_REF_2_5V;
> +		refn = st->vrefs_uv[AD4170_AVSS_SUP];
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Find out the analog input range from the channel type, polarity, and
> +	 * voltage reference selection.
> +	 * AD4170 channels are either differential or pseudo-differential.
> +	 * Diff input voltage range: −VREF/gain to +VREF/gain (datasheet page 6)
> +	 * Pseudo-diff input voltage range: 0 to VREF/gain (datasheet page 6)
> +	 */
> +	if (chan->differential) {
> +		if (!bipolar)
> +			return dev_err_probe(&st->spi->dev, -EINVAL,

dev

> +					     "Channel %u differential unipolar\n",
> +					     ch_reg);
> +
> +		/*
> +		 * Differential bipolar channel.
> +		 * avss-supply is never above 0V.
> +		 * Assuming refin1n-supply not above 0V.
> +		 * Assuming refin2n-supply not above 0V.
> +		 */
> +		return refp + abs(refn);
> +	}
> +	/*
> +	 * Some configurations can lead to invalid setups.
> +	 * For example, if AVSS = -2.5V, REF_SELECT set to REFOUT (REFOUT/AVSS),
> +	 * and pseudo-diff channel configuration set, then the input range
> +	 * should go from 0V to +VREF (single-ended - datasheet pg 10), but
> +	 * REFOUT/AVSS range would be -2.5V to 0V.
> +	 * Check the positive reference is higher than 0V for pseudo-diff
> +	 * channels.
> +	 */
> +	if (refp <= 0)
> +		return dev_err_probe(&st->spi->dev, -EINVAL,

dev

> +				     "REF+ <= GND for pseudo-diff chan %u\n",
> +				     ch_reg);
> +
> +	if (bipolar)
> +		return refp;
> +
> +	/*
> +	 * Pseudo-differential unipolar channel.
> +	 * Input expected to swing from IN- to +VREF.
> +	 */
> +	ret = ad4170_get_ain_voltage_uv(st, chan->channel2, &ain_voltage);
> +	if (ret)
> +		return ret;
> +
> +	if (refp - ain_voltage <= 0)
> +		return dev_err_probe(&st->spi->dev, -EINVAL,

dev

> +				     "Negative input >= REF+ for pseudo-diff chan %u\n",
> +				     ch_reg);
> +
> +	return refp - ain_voltage;
> +}



> +static int ad4170_parse_reference(struct ad4170_state *st,
> +				  struct fwnode_handle *child,
> +				  struct ad4170_setup *setup)
> +{
> +	struct device *dev = &st->spi->dev;
> +	int ret;
> +	u8 aux;
> +
> +	/* Optional positive reference buffering, if omitted we use the default */

I'd drop the "if omitted" part as the next line makes that clear.

> +	aux = AD4170_REF_BUF_FULL; /* Default to full precharge buffer enabled. */
> +	ret = fwnode_property_read_u8(child, "adi,buffered-positive", &aux);
> +	if (!ret) {
> +		if (aux < AD4170_REF_BUF_PRE || aux > AD4170_REF_BUF_BYPASS)

Given default is within these limits (I assume!), can simplified as:

	aux = AD4170_REF_BUF_FULL;
	fwnode_property_read_u8(child, "adi,buffered-positive", &aux);
	if (aux < AD4170_REF_BUF_PRE || aux > AD4170_REF_BUF_BYPASS)
		return dev_err_probe(dev, -EINVAL,
				     "Invalid adi,buffered-positive: %u\n", aux);

	setup->afe |= FIELD_PREP(AD4170_AFE_REF_BUF_P_MSK, aux);


> +					     aux);
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Invalid adi,buffered-positive: %u\n",
> +					     aux);
> +	}
> +	setup->afe |= FIELD_PREP(AD4170_AFE_REF_BUF_P_MSK, aux);
> +
> +	/* Optional negative reference buffering, if omitted we use the default */
> +	aux = AD4170_REF_BUF_FULL; /* Default to full precharge buffer enabled. */

Similar refactor to above applies here and dropping the obvious what happens
if omitted comment.

> +	ret = fwnode_property_read_u8(child, "adi,buffered-negative", &aux);
> +	if (!ret) {
> +		if (aux < AD4170_REF_BUF_PRE || aux > AD4170_REF_BUF_BYPASS)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Invalid adi,buffered-negative: %u\n",
> +					     aux);
> +	}
> +	setup->afe |= FIELD_PREP(AD4170_AFE_REF_BUF_M_MSK, aux);
> +
> +	/* Optional voltage reference selection, if omitted we use the default */
> +	aux = AD4170_REF_REFOUT; /* Default reference selection. */

And here.

> +	ret = fwnode_property_read_u8(child, "adi,reference-select", &aux);
> +	if (!ret) {
> +		if (aux > AD4170_REF_AVDD)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Invalid reference selected %u\n",
> +					     aux);
> +	}
> +	setup->afe |= FIELD_PREP(AD4170_AFE_REF_SELECT_MSK, aux);
> +
> +	return 0;
> +}
> +
> +static int ad4170_parse_adc_channel_type(struct device *dev,
> +					 struct fwnode_handle *child,
> +					 struct iio_chan_spec *chan)
> +{
> +	u32 pins[2];
> +	int ret, ret2;
> +
> +	/* Parse pseudo-differential channel configuration */
> +	ret = fwnode_property_read_u32(child, "single-channel", &pins[0]);
> +	ret2 = fwnode_property_read_u32(child, "common-mode-channel", &pins[1]);
> +	if (!ret && ret2)
> +		return dev_err_probe(dev, ret,
> +			"single-ended channels must define common-mode-channel\n");

ret == 0 so that will report success.

Move the ret2 logic down into this (!ret) statement that comes next then you
can just use ret and avoid this sort of issue. (Likely smatch would have
caught this but better to never have a bug report + fix :)


> +	if (!ret) {
> +		chan->differential = false;
> +		chan->channel = pins[0];
> +		chan->channel2 = pins[1];
> +		return 0;
> +	}
> +
> +	/* Parse differential channel configuration */
> +	ret = fwnode_property_read_u32_array(child, "diff-channels", 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 diff-channels or single-channel.\n");
> +}

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

> +
> +	init_completion(&st->completion);
> +
> +	if (spi->irq) {
> +		ret = devm_request_irq(&st->spi->dev, st->spi->irq,

Use dev and spi->irq.




> +				       &ad4170_irq_handler, IRQF_ONESHOT,
> +				       indio_dev->name, indio_dev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}