On Fri, 19 Apr 2024 17:36:51 +0200
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@xxxxxxxxxx> wrote:
> From: Nuno Sa <nuno.sa@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
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);
> +}
> +
