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

[Jonathan Cameron <jic23@kernel.org>]

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.

>  };

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

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

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

> +{
> +	DECLARE_BITMAP(err_map, AD9647_MAX_TEST_POINTS * 2);
> +	unsigned int point, val, inv_val, cnt, inv_cnt = 0;
> +	/*
> +	 * Half of the bitmap is for the inverted signal. The number of test
> +	 * points is the same though...
> +	 */
> +	unsigned int test_points = AD9647_MAX_TEST_POINTS;
> +	unsigned long sample_rate = clk_get_rate(st->clk);
> +	struct device *dev = &st->spi->dev;
> +	bool invert = false, stat;
> +	int ret;
> +
> +	ret = ad9647_calibrate_prepare(st);
> +	if (ret)
> +		return ret;
> +retune:
> +	ret = ad9647_calibrate_polarity_set(st, invert);
> +	if (ret)
> +		return ret;
> +
> +	for (point = 0; point < test_points; point++) {
> +		ret = ad9467_calibrate_apply(st, point);
> +		if (ret)
> +			return ret;
> +
> +		ret = ad9467_calibrate_status_check(st, &stat);
> +		if (ret < 0)
> +			return ret;
> +
> +		__assign_bit(point + invert * test_points, err_map, stat);
> +	}
> +
> +	if (!invert) {
> +		cnt = ad9467_find_optimal_point(err_map, 0, test_points, &val);
> +		/*
> +		 * We're happy if we find, at least, three good test points in
> +		 * a row.
> +		 */
> +		if (cnt < 3) {
> +			invert = true;
> +			goto retune;
> +		}
> +	} else {
> +		inv_cnt = ad9467_find_optimal_point(err_map, test_points,
> +						    test_points, &inv_val);
> +		if (!inv_cnt && !cnt)
> +			return -EIO;
> +	}
> +
> +	ad9467_dump_table(err_map, BITS_PER_TYPE(err_map), invert);
> +
> +	if (inv_cnt < cnt) {
> +		ret = ad9647_calibrate_polarity_set(st, false);
> +		if (ret)
> +			return ret;
> +	} else {
> +		/*
> +		 * polarity inverted is the last test to run. Hence, there's no
> +		 * need to re-do any configuration. We just need to "normalize"
> +		 * the selected value.
> +		 */
> +		val = inv_val - test_points;
> +	}
> +
> +	if (st->info->has_dco)
> +		dev_dbg(dev, "%sDCO 0x%X CLK %lu Hz\n", inv_cnt >= cnt ? "INVERT " : "",
> +			val, sample_rate);
> +	else
> +		dev_dbg(dev, "%sIDELAY 0x%x\n", inv_cnt >= cnt ? "INVERT " : "",
> +			val);
> +
> +	ret = ad9467_calibrate_apply(st, val);
> +	if (ret)
> +		return ret;
> +
> +	/* finally apply the optimal value */
> +	return ad9647_calibrate_stop(st);
> +}
> +