Re: [PATCH 8/8] iio: adc: ad9467: support digital interface calibration

["Nuno Sá" <noname.nuno@gmail.com>]

On Sat, 2024-04-20 at 16:34 +0100, Jonathan Cameron wrote:
> On Fri, 19 Apr 2024 17:36:51 +0200
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > To make sure that we have the best timings on the serial data interface
> > we should calibrate it. This means going through the device supported
> > values and see for which ones we get a successful result. To do that, we
> > use a prbs test pattern both in the IIO backend and in the frontend
> > devices. Then for each of the test points we see if there are any
> > errors. Note that the backend is responsible to look for those errors.
> > 
> > As calibrating the interface also requires that the data format is disabled
> > (the one thing being done in ad9467_setup()), ad9467_setup() was removed
> > and configuring the data fomat is now part of the calibration process.
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> Trivial comments inline.
> 
> Jonathan
> 
> > ---
> >  drivers/iio/adc/ad9467.c | 337 +++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 296 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > index 7db87ccc1ea4..44552dd6f4c6 100644
> > --- a/drivers/iio/adc/ad9467.c
> > +++ b/drivers/iio/adc/ad9467.c
> > @@ -4,6 +4,9 @@
> >   *
> >   * Copyright 2012-2020 Analog Devices Inc.
> >   */
> > +
> > +#include <linux/bitmap.h>
> > +#include <linux/bitops.h>
> >  #include <linux/cleanup.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> > @@ -100,6 +103,8 @@
> >  #define AD9467_DEF_OUTPUT_MODE		0x08
> >  #define AD9467_REG_VREF_MASK		0x0F
> >  
> > +#define AD9647_MAX_TEST_POINTS		32
> > +
> >  struct ad9467_chip_info {
> >  	const char		*name;
> >  	unsigned int		id;
> > @@ -110,6 +115,8 @@ struct ad9467_chip_info {
> >  	unsigned long		max_rate;
> >  	unsigned int		default_output_mode;
> >  	unsigned int		vref_mask;
> > +	unsigned int		num_lanes;
> > +	bool			has_dco;
> 
> What is dco?  Perhaps a comment, or expand the naming somewhat.
> 

data clock output.. will add a comment

> >  };
> 
> >  
> > +static void ad9467_dump_table(const unsigned long *err_map, unsigned int size,
> > +			      bool invert)
> > +{
> > +#ifdef DEBUG
> > +	unsigned int bit;
> > +
> > +	pr_debug("Dump calibration table:\n");
> 
> If it's useful, poke it in debugfs, otherwise, drop this code.

From our experience, it's useful to dump the table in the tuning process. But I guess
I can also just save the bitmap and dump it on demand (on debugfs).

> 
> > +	for (bit = 0; bit < size; bit++) {
> > +		if (bit == size / 2) {
> > +			if (!invert)
> > +				break;
> > +			pr_cont("\n");
> > +		}
> > +
> > +		pr_cont("%c", test_bit(bit, err_map) ? 'x' : 'o');
> > +	}
> > +#endif
> > +}
> > +
> 
> > +static int ad9467_calibrate_apply(const struct ad9467_state *st,
> > +				  unsigned int val)
> > +{
> > +	unsigned int lane;
> > +	int ret;
> > +
> > +	if (st->info->has_dco) {
> > +		int ret;
> Shadowing ret above.
> 

ups...

> > +
> > +		ret = ad9467_spi_write(st->spi, AN877_ADC_REG_OUTPUT_DELAY,
> > +				       val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return ad9467_spi_write(st->spi, AN877_ADC_REG_TRANSFER,
> > +					AN877_ADC_TRANSFER_SYNC);
> > +	}
> > +
> > +	for (lane = 0; lane < st->info->num_lanes; lane++) {
> > +		ret = iio_backend_iodelay_set(st->back, lane, val);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> > +
> > +static int ad9467_calibrate(const struct ad9467_state *st)
> 
> Some docs on the sequence or a reference would be good.

I can point into the datasheets..

- Nuno Sá