Re: [PATCH v2 13/13] iio: adc: ad4080: add driver support

[Nuno Sá]

Hi Antoniu,

I still have doubts regarding the new backend API mostly because it's still
unclear how it should all work. I had some questions in the first version of the
series that were not clarified so I'll likely repeat myself... Please don't rush
into a v3 without having this sorted out.

Anyways, see below...
 
On Fri, 2025-04-11 at 15:36 +0300, Antoniu Miclaus wrote:
> Add support for AD4080 high-speed, low noise, low distortion,
> 20-bit, Easy Drive, successive approximation register (SAR)
> analog-to-digital converter (ADC).
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> ---
> changes in v2:
>  - set num_lanes during probe.
>  - rename bitslitp to iio_backend_data_alignment_enable
>  - do ad4080_lvds_sync_write only once during probe.
>  - use ETIMEDOUT
>  - rename to `cnv` instead of `adc-clk`
>  - update Kconfig help section
>  - drop extra blank line.
>  - replace `/**` with `/*` for comments
>  - drop redundant return 0.
>  - use `dev_dbg` during while(--timeout) procedure
>  - use while (--timeout && !sync_en)
>  - return -ETIME where applicable and check missing return codes.
>  - regmap_update_bits used where applicable
>  - use defines instead of GENMASK inline.
>  - return FIELD_GET()
>  - st->filter_enabled = mode and dropping the if else statement.
>  - remove redundant brackets.
>  - use OVERSAMPLING_RATIO attribute and drop custom ABI for it
>  - use already existing filter_type attribute
>  - fix indentation
>  - remove comma on 'terminators'
>  - use dev_err_probe instead of dev_err
>  - check missing return values.
>  - rework num_lanes property parse.
>  - keep ad4080_chip_info since the driver will be extended for more parts
>    in the future (also stated in cover letter).
>  drivers/iio/adc/Kconfig  |  14 +
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad4080.c | 653 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 668 insertions(+)
>  create mode 100644 drivers/iio/adc/ad4080.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 27413516216c..17df328f5322 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -47,6 +47,20 @@ config AD4030
>  	  To compile this driver as a module, choose M here: the module will
> be
>  	  called ad4030.
>  
> +config AD4080
> +	tristate "Analog Devices AD4080 high speed ADC"
> +	depends on SPI
> +	select REGMAP_SPI
> +	select IIO_BACKEND
> +	help
> +	  Say yes here to build support for Analog Devices AD4080
> +	  high speed, low noise, low distortion, 20-bit, Easy Drive,
> +	  successive approximation register (SAR) analog-to-digital
> +	  converter (ADC). Supports iio_backended devices for AD4080.
> +
> +	  To compile this driver as a module, choose M here: the module will
> be
> +	  called ad4080.
> +
>  config AD4130
>  	tristate "Analog Device AD4130 ADC Driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 9f26d5eca822..e6efed5b4e7a 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
>  obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
>  obj-$(CONFIG_AD4000) += ad4000.o
>  obj-$(CONFIG_AD4030) += ad4030.o
> +obj-$(CONFIG_AD4080) += ad4080.o
>  obj-$(CONFIG_AD4130) += ad4130.o
>  obj-$(CONFIG_AD4695) += ad4695.o
>  obj-$(CONFIG_AD4851) += ad4851.o
> diff --git a/drivers/iio/adc/ad4080.c b/drivers/iio/adc/ad4080.c
> new file mode 100644
> index 000000000000..3a0b1ad13765
> --- /dev/null
> +++ b/drivers/iio/adc/ad4080.c
> @@ -0,0 +1,653 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD4080 SPI ADC driver
> + *
> + * Copyright 2025 Analog Devices Inc.
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
> +
> 

...

> +static int ad4080_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long m)
> +{
> +	struct ad4080_state *st = iio_priv(indio_dev);
> +	int dec_rate;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return ad4080_get_scale(st, val, val2);
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (st->filter_type == SINC_5_COMP)
> +			dec_rate = st->dec_rate * 2;
> +		else
> +			dec_rate = st->dec_rate;
> +		if (st->filter_en)
> +			*val = DIV_ROUND_CLOSEST(clk_get_rate(st->clk),
> dec_rate);
> +		else
> +			*val = clk_get_rate(st->clk);

Are we expecting the clock rate to change at runtime. Typically, we can just
cache the rate during probe and do it "dynamically" if we ever need it.

> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*val = ad4080_get_dec_rate(indio_dev, chan);
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4080_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return -EINVAL;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return -EINVAL;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		return ad4080_set_dec_rate(indio_dev, chan, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4080_lvds_sync_write(struct ad4080_state *st)
> +{

This whole function seems to depend on using the LVDS signal for cnv. Why? If we
have CMOS don't we also need some kind of interface alignment/calibration? If
not, that's sensible enough that should be stated in a comment.

> +	unsigned int timeout = 100;
> +	bool sync_en;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +	if (st->num_lanes == 1)
> +		ret = regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +				   AD4080_RESERVED_CONFIG_A_MSK |
> +				   AD4080_INTF_CHK_EN_MSK);
> +	else
> +		ret = regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +				   AD4080_RESERVED_CONFIG_A_MSK |
> +				   AD4080_INTF_CHK_EN_MSK |
> +				   AD4080_SPI_LVDS_LANES_MSK);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_backend_data_alignment_enable(st->back);
> +	if (ret)
> +		return ret;
> +
> +	do {
> +		ret = iio_backend_sync_status_get(st->back, &sync_en);
> +		if (ret)
> +			return ret;
> +
> +		if (!sync_en)
> +			dev_dbg(&st->spi->dev, "Not Locked: Running Bit
> Slip\n");
> +	} while (--timeout && !sync_en);
> +
> +	if (timeout) {
> +		dev_info(&st->spi->dev, "Success: Pattern correct and
> Locked!\n");
> +		if (st->num_lanes == 1)
> +			return regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +					    AD4080_RESERVED_CONFIG_A_MSK);
> +		else
> +			return regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +					    AD4080_RESERVED_CONFIG_A_MSK |
> +					    AD4080_SPI_LVDS_LANES_MSK);
> +	} else {
> +		dev_info(&st->spi->dev, "LVDS Sync Timeout.\n");
> +		if (st->num_lanes == 1) {
> +			ret = regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +					   AD4080_RESERVED_CONFIG_A_MSK);
> +			if (ret)
> +				return ret;
> +		} else {
> +			ret = regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +					   AD4080_RESERVED_CONFIG_A_MSK |
> +					   AD4080_SPI_LVDS_LANES_MSK);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		return -ETIMEDOUT;

So, first the things that I don't really get (also the hdl docs need to be
improved FWIW):

* It seems that we can have data alignment or data capture synchronization
through an internal AMD FPGA technique called bit-slip or an external signal,
right? In here, it seems that we only use bit-slip and never disable it. Is that
really the goal?

* This whole process seems to be some kind of calibration at the interface
level, right?

* What's the point of the conv clock? Is it only used to get the sample rate? If
so, the hdl docs are misleading as it seems that it can be used instead of bit-
slip for data capture alignment?

Now, speaking about the backend API's, it looks like that
iio_backend_self_sync_enable() and iio_backend_data_alignment_enable() could be
merged into one API. From what I can tell, iio_backend_data_alignment_enable()
just enables the bit-slip process but that likely does nothing unless the
SELF_SYNC bit is also set to bit-slip, right? In fact, we could think about a
more generic (less flexible though) API like:

* iio_backend_intf_data_align(back, timeout_us), or
* iio_backend_intf_calib(back, timeout_us), or
* iio_backend_intf_data_capture_sync(back, timeout_us)

On the backend side, you could then enable bit-slip and do the status read (with
timeout) and return success or an error code.

The above seems to be pretty much what you're doing but in just one API that
makes sense to me.

Of course that the following questions still come to mind:

* Do we need to disable bit-slip after we're done (still fits into the one API
model)?
* Do we need a meaningful API to change between the syncing/alignment methods?
External signal vs bit-slip?

The above is key to better think of an API. Right now it feels that you're just
adding an API for every bit you want to control in this process...

If we end up needing more flexibility for this, we can also consider the
existing iio_backend_data_sample_trigger() API. I know is abusing a bit and a
stretch but in the end of the day the whole thing is related with aligning,
syncing, calibrating the interface for properly sampling data. Even bit-slip
(while not a traditional external trigger) looks like some kind of self-
adjusting, data-driven trigger mechanism to establish the correct starting point
for capturing data. So having two new enums like:

IIO_BACKEND_SAMPLE_TRIGGER_EXT_SIGNAL,
IIO_BACKEND_SAMPLE_TRIGGER_BIT_SLIP // or maybe a more generic name like
s/BIT_SLIP/INTERNAL

I do not think the above is that horrible :P... But I would really like to get
more opinions about this.

> +	}
> +}

...

> 
> +static const struct ad4080_chip_info ad4080_chip_info = {
> +	.name = "AD4080",
> +	.product_id = AD4080_CHIP_ID,
> +	.scale_table = ad4080_scale_table,
> +	.num_scales = ARRAY_SIZE(ad4080_scale_table),
> +	.num_channels = 1,
> +	.channels = ad4080_channels,
> +};
> +
> +static int ad4080_setup(struct iio_dev *indio_dev)
> +{
> +	struct ad4080_state *st = iio_priv(indio_dev);
> +	unsigned int id;
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4080_REG_INTERFACE_CONFIG_A,
> +			   AD4080_SW_RESET);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4080_REG_INTERFACE_CONFIG_A,
> +			   AD4080_SDO_ENABLE_MSK);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD4080_REG_CHIP_TYPE, &id);
> +	if (ret)
> +		return ret;
> +
> +	if (id != AD4080_CHIP_ID)
> +		return dev_err_probe(&st->spi->dev, -EINVAL,
> +				     "Unrecognized CHIP_ID 0x%X\n", id);
> +
> +	ret = regmap_set_bits(st->regmap, AD4080_REG_GPIO_CONFIG_A,
> +			      AD4080_GPO_1_EN);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4080_REG_GPIO_CONFIG_B,
> +			   FIELD_PREP(AD4080_GPIO_1_SEL, 3));
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_backend_num_lanes_set(st->back, st->num_lanes);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_backend_self_sync_enable(st->back);
> +	if (ret)
> +		return ret;
> +

AFAIU, the above is enabling bit-slip?
 
> +	if (st->lvds_cnv_en) {
> +		if (st->num_lanes) {
> +			ret = regmap_update_bits(st->regmap,
> +						
> AD4080_REG_ADC_DATA_INTF_CONFIG_B,
> +						 AD4080_LVDS_CNV_CLK_CNT_MSK,
> +						
> FIELD_PREP(AD4080_LVDS_CNV_CLK_CNT_MSK, 7));
> +			if (ret)
> +				return ret;
> +		}
> +
> +		ret = regmap_set_bits(st->regmap,
> +				      AD4080_REG_ADC_DATA_INTF_CONFIG_B,
> +				      AD4080_LVDS_CNV_EN_MSK);
> +		if (ret)
> +			return ret;
> +
> +		return ad4080_lvds_sync_write(st);
> +	}
> +
> +	return 0;
> +}
> +
> +static void ad4080_properties_parse(struct ad4080_state *st)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	st->lvds_cnv_en = device_property_read_bool(&st->spi->dev,
> +						    "adi,lvds-cnv-enable");
> +

nit: I would probably drop the enable part. The property is about stating that
the signal is LVDS instead of CMOS. And IIUC, this should also depend on the
existence of `st->clk`
 
> +	st->num_lanes = 1;
> +	ret = device_property_read_u32(&st->spi->dev, "adi,num_lanes", &val);
> +	if (!ret)
> +		st->num_lanes = val;
> +}
> +
> +static int ad4080_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct device *dev = &spi->dev;
> +	struct ad4080_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +
> +	ret = devm_regulator_bulk_get_enable(dev,
> +					    
> ARRAY_SIZE(ad4080_power_supplies),
> +					     ad4080_power_supplies);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to get and enable supplies\n");
> +
> +	st->regmap = devm_regmap_init_spi(spi, &ad4080_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return PTR_ERR(st->regmap);
> +
> +	st->info = spi_get_device_match_data(spi);
> +	if (!st->info)
> +		return -ENODEV;
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	st->info = spi_get_device_match_data(spi);
> +	if (!st->info)
> +		return -ENODEV;
> +

The above is duplicated...

> +	indio_dev->name = st->info->name;
> +	indio_dev->channels = st->info->channels;
> +	indio_dev->num_channels = st->info->num_channels;
> +	indio_dev->info = &ad4080_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ad4080_properties_parse(st);
> +
> +	st->clk = devm_clk_get_enabled(&spi->dev, "cnv");
> +	if (IS_ERR(st->clk))
> +		return PTR_ERR(st->clk);
> +

>From the datasheet it looks like this clock is optional? Moreover in the IP docs
we have the following:


"SELF_SYNC: Controls if the data capture synchronization is done through CNV
signal or bit-slip."

So I wonder, is the cnv clock meaning that we want to have capture
sync/alignment through that external signal instead of bit-slip?

- Nuno Sá