On Thu, 4 Jul 2024 11:25:21 +0200
Nuno Sa <nuno.sa@xxxxxxxxxx> 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@xxxxxxxxxx>
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) {
>
