diff mbox series

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

Message ID 20240704-dev-iio-ad9467-new-devs-v1-1-f1adfee921f7@analog.com (mailing list archive)
State Accepted
Headers show
Series iio: adc: ad9467: support new devices | expand

Commit Message

Nuno Sa July 4, 2024, 9:25 a.m. UTC
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>
---
 drivers/iio/adc/ad9467.c | 117 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 76 insertions(+), 41 deletions(-)

Comments

Jonathan Cameron July 6, 2024, 11:16 a.m. UTC | #1
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) {
>
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index 41c1b519c573..1d8270a5bccb 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -352,6 +352,34 @@  static int ad9467_outputmode_set(struct ad9467_state *st, unsigned int mode)
 				AN877_ADC_TRANSFER_SYNC);
 }
 
+static int ad9467_testmode_set(struct ad9467_state *st, unsigned int chan,
+			       unsigned int test_mode)
+{
+	int ret;
+
+	if (st->info->num_channels > 1) {
+		/* so that the test mode is only applied to one channel */
+		ret = ad9467_spi_write(st, AN877_ADC_REG_CHAN_INDEX, BIT(chan));
+		if (ret)
+			return ret;
+	}
+
+	ret = ad9467_spi_write(st, AN877_ADC_REG_TEST_IO, test_mode);
+	if (ret)
+		return ret;
+
+	if (st->info->num_channels > 1) {
+		/* go to default state where all channels get write commands */
+		ret = ad9467_spi_write(st, AN877_ADC_REG_CHAN_INDEX,
+				       GENMASK(st->info->num_channels - 1, 0));
+		if (ret)
+			return ret;
+	}
+
+	return ad9467_spi_write(st, AN877_ADC_REG_TRANSFER,
+				AN877_ADC_TRANSFER_SYNC);
+}
+
 static int ad9647_calibrate_prepare(struct ad9467_state *st)
 {
 	struct iio_backend_data_fmt data = {
@@ -360,32 +388,30 @@  static int ad9647_calibrate_prepare(struct ad9467_state *st)
 	unsigned int c;
 	int ret;
 
-	ret = ad9467_spi_write(st, AN877_ADC_REG_TEST_IO,
-			       AN877_ADC_TESTMODE_PN9_SEQ);
-	if (ret)
-		return ret;
-
-	ret = ad9467_spi_write(st, AN877_ADC_REG_TRANSFER,
-			       AN877_ADC_TRANSFER_SYNC);
-	if (ret)
-		return ret;
-
 	ret = ad9467_outputmode_set(st, st->info->default_output_mode);
 	if (ret)
 		return ret;
 
 	for (c = 0; c < st->info->num_channels; c++) {
+		ret = ad9467_testmode_set(st, c, AN877_ADC_TESTMODE_PN9_SEQ);
+		if (ret)
+			return ret;
+
 		ret = iio_backend_data_format_set(st->back, c, &data);
 		if (ret)
 			return ret;
+
+		ret = iio_backend_test_pattern_set(st->back, c,
+						   IIO_BACKEND_ADI_PRBS_9A);
+		if (ret)
+			return ret;
+
+		ret = iio_backend_chan_enable(st->back, c);
+		if (ret)
+			return ret;
 	}
 
-	ret = iio_backend_test_pattern_set(st->back, 0,
-					   IIO_BACKEND_ADI_PRBS_9A);
-	if (ret)
-		return ret;
-
-	return iio_backend_chan_enable(st->back, 0);
+	return 0;
 }
 
 static int ad9647_calibrate_polarity_set(struct ad9467_state *st,
@@ -468,38 +494,32 @@  static int ad9647_calibrate_stop(struct ad9467_state *st)
 	unsigned int c, mode;
 	int ret;
 
-	ret = iio_backend_chan_disable(st->back, 0);
-	if (ret)
-		return ret;
-
-	ret = iio_backend_test_pattern_set(st->back, 0,
-					   IIO_BACKEND_NO_TEST_PATTERN);
-	if (ret)
-		return ret;
-
 	for (c = 0; c < st->info->num_channels; c++) {
+		ret = iio_backend_chan_disable(st->back, c);
+		if (ret)
+			return ret;
+
+		ret = iio_backend_test_pattern_set(st->back, c,
+						   IIO_BACKEND_NO_TEST_PATTERN);
+		if (ret)
+			return ret;
+
 		ret = iio_backend_data_format_set(st->back, c, &data);
 		if (ret)
 			return ret;
+
+		ret = ad9467_testmode_set(st, c, AN877_ADC_TESTMODE_OFF);
+		if (ret)
+			return ret;
 	}
 
 	mode = st->info->default_output_mode | AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT;
-	ret = ad9467_outputmode_set(st, mode);
-	if (ret)
-		return ret;
-
-	ret = ad9467_spi_write(st, AN877_ADC_REG_TEST_IO,
-			       AN877_ADC_TESTMODE_OFF);
-	if (ret)
-		return ret;
-
-	return ad9467_spi_write(st, AN877_ADC_REG_TRANSFER,
-			       AN877_ADC_TRANSFER_SYNC);
+	return ad9467_outputmode_set(st, mode);
 }
 
 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;
 	/*
 	 * 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) {