Re: [PATCH 1/5] iio: adc: ad9467: support multiple channels calibration

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 4 Jul 2024 11:25:21 +0200
Nuno Sa <nuno.sa@analog.com> wrote:

> The calibration process mixes the support for calibrating multiple
> channels with only having one channel. Some paths do have 'num_channels'
> into account while others don't.
> 
> As of now, the driver only supports devices with one channel so the
> above is not really a problem. That said, we'll add support for devices
> with more than one channel, hence let's properly make the calibration
> process to work with it.
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
Hi Nuno,

Not suggesting you change it here, but one place where I think
the existing code readability could be improved.

>  static int ad9467_calibrate(struct ad9467_state *st)
>  {
> -	unsigned int point, val, inv_val, cnt, inv_cnt = 0;
> +	unsigned int point, val, inv_val, cnt, inv_cnt = 0, c;

Comment on existing code. I'm not keen on mix of assignment and non
assignment in a single line of local variable declarations.
It can get hard to spot what is assigned!

>  	/*
>  	 * Half of the bitmap is for the inverted signal. The number of test
>  	 * points is the same though...
> @@ -526,11 +546,26 @@ static int ad9467_calibrate(struct ad9467_state *st)
>  		if (ret)
>  			return ret;
>  
> -		ret = iio_backend_chan_status(st->back, 0, &stat);
> -		if (ret)
> -			return ret;
> +		for (c = 0; c < st->info->num_channels; c++) {
> +			ret = iio_backend_chan_status(st->back, c, &stat);
> +			if (ret)
> +				return ret;
>  
> -		__assign_bit(point + invert * test_points, st->calib_map, stat);
> +			/*
> +			 * A point is considered valid if all channels report no
> +			 * error. If one reports an error, then we consider the
> +			 * point as invalid and we can break the loop right away.
> +			 */
> +			if (stat) {
> +				dev_dbg(dev, "Invalid point(%u, inv:%u) for CH:%u\n",
> +					point, invert, c);
> +				break;
> +			}
> +
> +			if (c == st->info->num_channels - 1)
> +				__clear_bit(point + invert * test_points,
> +					    st->calib_map);
> +		}
>  	}
>  
>  	if (!invert) {
>