On Sun, 2024-04-28 at 18:32 +0100, Jonathan Cameron wrote:
> On Fri, 26 Apr 2024 17:42:16 +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>
>
> One trivial comment.
>
> I'd have picked up the whole series, but it feels too big to do on a Sunday
> when you only posted on Friday. Hence, lets let it sit for at least
> a few more days to see if others have comments.
Yeah, I kind of waited till the last moment to see if you had any important
comment (on the first version open discussions) that could affect v2 :).
>
> It might not make this cycle as a result. I've picked up the 2 fixes
> today to increase the chances those make it.
>
> Jonathan
>
>
> > static int ad9467_read_raw(struct iio_dev *indio_dev,
> > struct iio_chan_spec const *chan,
> > int *val, int *val2, long m)
> > @@ -345,7 +606,9 @@ static int ad9467_write_raw(struct iio_dev *indio_dev,
> > {
> > struct ad9467_state *st = iio_priv(indio_dev);
> > const struct ad9467_chip_info *info = st->info;
> > + unsigned long sample_rate;
> > long r_clk;
> > + int ret;
> >
> > switch (mask) {
> > case IIO_CHAN_INFO_SCALE:
> > @@ -358,7 +621,23 @@ static int ad9467_write_raw(struct iio_dev *indio_dev,
> > return -EINVAL;
> > }
> >
> > - return clk_set_rate(st->clk, r_clk);
> > + sample_rate = clk_get_rate(st->clk);
> > + /*
> > + * clk_set_rate() would also do this but since we would
> > still
> > + * need it for avoiding an unnecessary calibration, do it
> > now.
> > + */
> > + if (sample_rate == r_clk)
> > + return 0;
> > +
> > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> > + ret = clk_set_rate(st->clk, r_clk);
> > + if (ret)
> > + return ret;
> > +
> > + guard(mutex)(&st->lock);
> > + ret = ad9467_calibrate(st);
> return ad9467_calibrate(st);
> > + }
> unreachable();
>
> not totally elegant but I think the early return makes more sense and we
> should
> just use an unreachable() to squash the resulting compiler warning.
>
As you might remember I'm not a big fan of the unreachable() but also no strong
feelings about it :). Do you expect a v3 for this or is this something you can
fix up while applying (assuming no other things pop by)?
- Nuno Sá
>
