Message-ID: <be3fc6e363947f938b6705d2304f075cfa116471.camel@gmail.com>
Date: Thu, 20 Feb 2025 15:21:58 +0000
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 13/14] iio: adc: ad4080: add driver support

Hi Antoniu,

A first, not in depth, review from my side...

On Thu, 2025-02-20 at 15:54 +0200, 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>
> ---
>  drivers/iio/adc/Kconfig  |  15 +
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad4080.c | 768 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 784 insertions(+)
>  create mode 100644 drivers/iio/adc/ad4080.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 27413516216c..b198a93c10b7 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -47,6 +47,21 @@ 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).
> +
> +	  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..71c443965e10
> --- /dev/null
> +++ b/drivers/iio/adc/ad4080.c
> @@ -0,0 +1,768 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD4080 SPI ADC driver
> + *
> + * Copyright 2025 Analog Devices Inc.
> + */

...

> 
> +static int ad4080_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct ad4080_state *st = iio_priv(indio_dev);
> +	unsigned long s_clk;
> +	int dec_rate = 1;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return -EINVAL;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		s_clk = clk_round_rate(st->clk, val);
> +
> +		if (st->filter_enabled) {
> +			if (st->filter_mode == SINC_5_COMP)
> +				dec_rate = ad4080_dec_rate_value[st-
> >dec_rate] * 2;
> +			else
> +				dec_rate = ad4080_dec_rate_value[st-
> >dec_rate];
> +		}
> +
> +		s_clk *= dec_rate;
> +
> +		if (s_clk < AD4080_MIN_SAMP_FREQ)
> +			s_clk = AD4080_MIN_SAMP_FREQ;
> +		if (s_clk > AD4080_MAX_SAMP_FREQ)
> +			s_clk = AD4080_MAX_SAMP_FREQ;
> +
> +		return clk_set_rate(st->clk, s_clk);

It seems to me that we could skip the dec_rate attribute. Given the available
values we have, can't we compute the available sampling frequencies during
.probe() after getting our ref clock? 
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t ad4080_lvds_sync_write(struct iio_dev *indio_dev,
> +				      uintptr_t private,
> +				      const struct iio_chan_spec *chan,
> +				      const char *buf, size_t len)
> +{
> +	struct ad4080_state *st = iio_priv(indio_dev);
> +	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_self_sync_enable(st->back);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_backend_num_lanes_set(st->back, st->num_lanes);
> +	if (ret)
> +		return ret;

The number of lanes is a DT property so it seems to me that we can do the above
only once during probe. Otherwise I would expect a comment on why we need to do
this again...


> +
> +	ret = iio_backend_bitslip_enable(st->back);
> +	if (ret)
> +		return ret;

AFAIU, bit slip seems to be something very specific to AMD/Xilinx isn't it? It
also looks like this is mostly about adjusting the alignment of incoming data at
the interface level for data integrity. 

So we could maybe rename the API for something more generic like
iio_backend_data_alignment_enable()? I'm open for suggestions here :)

> +
> +	do {
> +		ret = iio_backend_sync_status_get(st->back, &sync_en);
> +		if (ret)
> +			return ret;

Since this is about waiting on some alignment/synchronization process to
complete, we could also think on a better name and API for this. I think we
could pass a timeout parameter and do the waiting (with some regmap polling) in
the backend returning 0 on success. Thoughts?

> +
> +		if (!sync_en)
> +			dev_info(&st->spi->dev, "Not Locked: Running Bit
> Slip\n");
> +		else
> +			break;
> +	} while (--timeout);
> 

Now comes my question about this process. This looks like some kind of interface
calibration or am I completely wrong? I wonder if this is something that really
needs to be a control that userspace can randomly use? Or is this something we
should only do once during probe? Or every time before buffering? Or everytime
we change the sampling frequency? 

Gut feeling is that we should do this after changing the sampling frequency but
I can be totally wrong.

Other thing that I'm confused about is that during probe we do
iio_backend_self_sync_enable() and we can start buffering without running the
lvds_sync code. My point is that after running lvds_sync we are left with  
iio_backend_bitslip_enable() so I'm wondering how this all should work? Should
bitslip be always enabled or only for this process?

The Docs are also a bit confusing:

BITSLIP_ENABLE - Enables the sync process.
SELF_SYNC - Controls if the data capture synchronization is done through CNV
signal or bit-slip.

>From what I can tell we always set SELF_SYNC to 1 which is not clear what it is?
I guess bit slip?

> +	if (timeout) {
> +		dev_info(&st->spi->dev, "Success: Pattern correct and
> Locked!\n");
> +		if (st->num_lanes == 1)
> +			ret = regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +					   AD4080_RESERVED_CONFIG_A_MSK);
> +		else
> +			ret = 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);
> +		else
> +			ret = regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +					   AD4080_RESERVED_CONFIG_A_MSK |
> +					   AD4080_SPI_LVDS_LANES_MSK);
> +		ret = -ETIME;

ETIMEDOUT (might have a typo)

> +	}
> +
> +	return ret ? ret : len;
> +}
> +

...

> 
> +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;
> +
> +	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;
> +
> +	ret = ad4080_properties_parse(st);
> +	if (ret)
> +		return ret;
> +
> +	st->clk = devm_clk_get_enabled(&spi->dev, "adc-clk");
> +	if (IS_ERR(st->clk))
> +		return PTR_ERR(st->clk);

Is this the CNV pin? If so, cnv-clk could be a better name or since we only have
one clock, we don't really need a name...

- Nuno Sá
> 

Powered by blists - more mailing lists