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@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. > 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á