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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 26 Apr 2024 17:42:16 +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>

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.

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.

> +		return ret;
>  	default:
>  		return -EINVAL;
>  	}