Message-ID: <599074b26d59134f4697d6440ab82b7d6a855fe4.camel@gmail.com>
Date: Tue, 29 Apr 2025 09:46:10 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Antoniu Miclaus <antoniu.miclaus@...log.com>, jic23@...nel.org, 
	robh@...nel.org, conor+dt@...nel.org, linux-iio@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 11/11] iio: adc: ad4080: add driver support

Hi Antoniu,

On Fri, 2025-04-25 at 14:25 +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@...log.com>
> ---
> changes in v3:
>  - name the defines to make associacion with register name.
>  - drop redundant defines.
>  - add trailing comma when needed.
>  - drop explicit masking and use field_prep instead
>  - add fsleep during sync process.
>  - do not wrap where 80 chars is not exceeded.
>  - use space for { }
>  - drop channel definition macro
>  - return dev_info on chip id mismatch.
>  - flip expression to `if (!st->lvds_cnv_en)`
>  - rework num_lanes property parse.
>  - update the driver based on the new edits on the backend interface related
> to
>    this part and the last disscussions in v2.
>  drivers/iio/adc/Kconfig  |  14 +
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad4080.c | 618 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 633 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..b51893253941
> --- /dev/null
> +++ b/drivers/iio/adc/ad4080.c
> @@ -0,0 +1,618 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD4080 SPI ADC driver
> + *
> + * Copyright 2025 Analog Devices Inc.
> + */
> +
> 

...

> +
> +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_type)
> +			*val = DIV_ROUND_CLOSEST(clk_get_rate(st->clk),
> dec_rate);
> +		else
> +			*val = clk_get_rate(st->clk);

Kind of a nit comment (and likely personal preference) but I would rather get
the clock rate during probe. Most of the times, the clock never changes so I
rather prefer doing the above when actually needed. Or is this one those cases
were it actually changes?

> +		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;

Why not handle this in 'default'?

> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		return ad4080_set_dec_rate(indio_dev, chan, val);

IIRC, you mentioned at some point that after changing the sampling frequency we
should align the interface again. Isn't this setting affecting it?

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4080_lvds_sync_write(struct ad4080_state *st)
> +{
> +	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_ADC_DATA_INTF_CONFIG_A_RESERVED_CONFIG_A_MSK |
> +				  
> AD4080_ADC_DATA_INTF_CONFIG_A_INTF_CHK_EN_MSK);
> +	else
> +		ret = regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +				  
> AD4080_ADC_DATA_INTF_CONFIG_A_RESERVED_CONFIG_A_MSK |
> +				  
> AD4080_ADC_DATA_INTF_CONFIG_A_INTF_CHK_EN_MSK |
> +				  
> AD4080_ADC_DATA_INTF_CONFIG_A_SPI_LVDS_LANES_MSK);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_backend_data_alignment_enable(st->back);
> +	if (ret)
> +		return ret;

So it looks like you never disable the internal alignment. Is that on purpose? I
also failed to reply to your comments in the previous version so I'll bring them
back:

"In this particular case, yes, one backend would fit for the sync process. But
taking into account that these two features are part also from the common core
of the AXI ADC, in other cases they might be used separately."

Not sure if that is true... I guess these are in the default register map but
are they always implemented for all the designs? They might just be no-ops for
some designs. Example, I do not think bitslip is always implemented. But even in
that case, the goal here is to align the interface for data sampling. So,
really, something like:

iio_backend_interface_data_align(st->back, u32 timeout);

looks fairly generic to me (given that the process is for the complete interface
at once. IOW, there's no specific timing points or stuff like that that we need
to probe independently)... So doing the polling on the backend side seems
reasonable to me (and there you can use regmap_poll APIs).

And as Jonathan puts it, this is all in kernel APIs so we can easily change it
afterwards :)

"The CNV signal is mainly used for sampling (an input pin according to the
datasheet - 
conversion is initiated on the rising edge of the convert signal).
We use it only for determining the sampling frequency."

Ok, from your previous versions I got the impression this pin could also be used
for aligning the interface.

> +
> +	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");
> +
> +		fsleep(500);
> +	} 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_ADC_DATA_INTF_CONFIG_A_RESERVED_CONFIG_A_MSK);
> +		else
> +			return regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +					   
> AD4080_ADC_DATA_INTF_CONFIG_A_RESERVED_CONFIG_A_MSK |
> +					   
> AD4080_ADC_DATA_INTF_CONFIG_A_SPI_LVDS_LANES_MSK);

redundant else

> +	} 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_ADC_DATA_INTF_CONFIG_A_RESERVED_CONFIG_A_MSK);
> +			if (ret)
> +				return ret;
> +		} else {
> +			ret = regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +					  
> AD4080_ADC_DATA_INTF_CONFIG_A_RESERVED_CONFIG_A_MSK |
> +					  
> AD4080_ADC_DATA_INTF_CONFIG_A_SPI_LVDS_LANES_MSK);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		return -ETIMEDOUT;
> +	}
> +}
> 

Powered by blists - more mailing lists