[PATCH 01/15] iio: adc: ad4030: Fix _scale for when oversampling is enabled

Marcelo Schmitt posted 15 patches 1 month ago
There is a newer version of this series
[PATCH 01/15] iio: adc: ad4030: Fix _scale for when oversampling is enabled
Posted by Marcelo Schmitt 1 month ago
Previously, the AD4030 driver was using the number of scan realbits for the
voltage channel to derive the scale to millivolts. Though, when sample
averaging is enabled (oversampling_ratio > 1), the number of scan realbits
for the channel is set to 30 and doesn't match the amount of conversion
precision bits. Due to that, the calculated channel scale did not correctly
scale raw sample data to millivolt units in those cases. Use chip specific
precision bits to derive the correct channel _scale on every and all
channel configuration.

Fixes: dc78e71d7c15 ("iio: adc: ad4030: remove some duplicate code")
Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
This was probalby buggy since 
commit 949abd1ca5a4 ("iio: adc: ad4030: add averaging support")
but I decided to set the fixes tag with dc78e71d7c15 because this patch will
not apply cleanly over 949abd1ca5a4.

 drivers/iio/adc/ad4030.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
index 1bc2f9a22470..82784593f976 100644
--- a/drivers/iio/adc/ad4030.c
+++ b/drivers/iio/adc/ad4030.c
@@ -394,7 +394,14 @@ static int ad4030_get_chan_scale(struct iio_dev *indio_dev,
 	else
 		*val = st->vref_uv / MILLI;
 
-	*val2 = scan_type->realbits;
+	/*
+	 * Even though the sample data comes in a 30-bit chunk when the ADC
+	 * is averaging samples, the conversion precision is still 16-bit or
+	 * 24-bit depending on the device. Thus, instead of scan_type->realbits,
+	 * use chip specific precision bits to derive the correct scale to mV.
+	 */
+	*val2 = scan_type->realbits == 30 ? st->chip->precision_bits
+					  : scan_type->realbits;
 
 	return IIO_VAL_FRACTIONAL_LOG2;
 }
-- 
2.39.2
Re: [PATCH 01/15] iio: adc: ad4030: Fix _scale for when oversampling is enabled
Posted by Jonathan Cameron 1 month ago
On Fri, 29 Aug 2025 21:40:24 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:

> Previously, the AD4030 driver was using the number of scan realbits for the
> voltage channel to derive the scale to millivolts. Though, when sample
> averaging is enabled (oversampling_ratio > 1), the number of scan realbits
> for the channel is set to 30 and doesn't match the amount of conversion
> precision bits. Due to that, the calculated channel scale did not correctly
> scale raw sample data to millivolt units in those cases. Use chip specific
> precision bits to derive the correct channel _scale on every and all
> channel configuration.
> 
> Fixes: dc78e71d7c15 ("iio: adc: ad4030: remove some duplicate code")
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

Hi Marcelo

I was assuming that when this said 'averaging' it actually meant
summing (there is a note about using the upper precision bits to get the same
scaling which is what we'd expect it were simply summing over X samples).

So given that we don't divide back down to get the original scaling I'm
not following how this works.

E.g. If we 'averaged' just 2 values of 3 then we'd go from a value of 3 to
one of 6.  Therefore I'd expect the scale to halve as each lsb represents
half the voltage it did when we weren't averaging those 2 samples.

I think that is what we'd see with the current code, so my reasoning is
clearly wrong, but why?

Jonathan

> ---
> This was probalby buggy since 
> commit 949abd1ca5a4 ("iio: adc: ad4030: add averaging support")
> but I decided to set the fixes tag with dc78e71d7c15 because this patch will
> not apply cleanly over 949abd1ca5a4.
> 
>  drivers/iio/adc/ad4030.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> index 1bc2f9a22470..82784593f976 100644
> --- a/drivers/iio/adc/ad4030.c
> +++ b/drivers/iio/adc/ad4030.c
> @@ -394,7 +394,14 @@ static int ad4030_get_chan_scale(struct iio_dev *indio_dev,
>  	else
>  		*val = st->vref_uv / MILLI;
>  
> -	*val2 = scan_type->realbits;
> +	/*
> +	 * Even though the sample data comes in a 30-bit chunk when the ADC
> +	 * is averaging samples, the conversion precision is still 16-bit or
> +	 * 24-bit depending on the device. Thus, instead of scan_type->realbits,
> +	 * use chip specific precision bits to derive the correct scale to mV.
> +	 */
> +	*val2 = scan_type->realbits == 30 ? st->chip->precision_bits
> +					  : scan_type->realbits;
>  
>  	return IIO_VAL_FRACTIONAL_LOG2;
>  }
Re: [PATCH 01/15] iio: adc: ad4030: Fix _scale for when oversampling is enabled
Posted by Marcelo Schmitt 1 month ago
Hi Jonathan,

Thanks for having a look at this.
Comment inline.

On 08/30, Jonathan Cameron wrote:
> On Fri, 29 Aug 2025 21:40:24 -0300
> Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:
> 
> > Previously, the AD4030 driver was using the number of scan realbits for the
> > voltage channel to derive the scale to millivolts. Though, when sample
> > averaging is enabled (oversampling_ratio > 1), the number of scan realbits
> > for the channel is set to 30 and doesn't match the amount of conversion
> > precision bits. Due to that, the calculated channel scale did not correctly
> > scale raw sample data to millivolt units in those cases. Use chip specific
> > precision bits to derive the correct channel _scale on every and all
> > channel configuration.
> > 
> > Fixes: dc78e71d7c15 ("iio: adc: ad4030: remove some duplicate code")
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> 
> Hi Marcelo
> 
> I was assuming that when this said 'averaging' it actually meant
> summing (there is a note about using the upper precision bits to get the same
> scaling which is what we'd expect it were simply summing over X samples).
> 
> So given that we don't divide back down to get the original scaling I'm
> not following how this works.
> 
> E.g. If we 'averaged' just 2 values of 3 then we'd go from a value of 3 to
> one of 6.  Therefore I'd expect the scale to halve as each lsb represents
> half the voltage it did when we weren't averaging those 2 samples.

This makes sense and thank you for explaining it to me.
I did some more test and debugging on the remote setup and found out the device
was not correctly configured for averaging data on my tests for v1. I need to
tweak a few more things in the driver to get both device registers and spi
transfer configuration good for offload with averaging mode. I'll reply with
more details if I find something unexpected, or drop this patch on v2.

Thanks,
Marcelo
Re: [PATCH 01/15] iio: adc: ad4030: Fix _scale for when oversampling is enabled
Posted by David Lechner 1 month ago
On 8/30/25 1:43 PM, Jonathan Cameron wrote:
> On Fri, 29 Aug 2025 21:40:24 -0300
> Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:
> 
>> Previously, the AD4030 driver was using the number of scan realbits for the
>> voltage channel to derive the scale to millivolts. Though, when sample
>> averaging is enabled (oversampling_ratio > 1), the number of scan realbits
>> for the channel is set to 30 and doesn't match the amount of conversion
>> precision bits. Due to that, the calculated channel scale did not correctly
>> scale raw sample data to millivolt units in those cases. Use chip specific
>> precision bits to derive the correct channel _scale on every and all
>> channel configuration.
>>
>> Fixes: dc78e71d7c15 ("iio: adc: ad4030: remove some duplicate code")
>> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> 
> Hi Marcelo
> 
> I was assuming that when this said 'averaging' it actually meant
> summing (there is a note about using the upper precision bits to get the same
> scaling which is what we'd expect it were simply summing over X samples).
> 
> So given that we don't divide back down to get the original scaling I'm
> not following how this works.

I had the same feeling. I have some hardware I can test later this week.
Re: [PATCH 01/15] iio: adc: ad4030: Fix _scale for when oversampling is enabled
Posted by Andy Shevchenko 1 month ago
On Sat, Aug 30, 2025 at 3:40 AM Marcelo Schmitt
<marcelo.schmitt@analog.com> wrote:
>
> Previously, the AD4030 driver was using the number of scan realbits for the
> voltage channel to derive the scale to millivolts. Though, when sample
> averaging is enabled (oversampling_ratio > 1), the number of scan realbits
> for the channel is set to 30 and doesn't match the amount of conversion
> precision bits. Due to that, the calculated channel scale did not correctly
> scale raw sample data to millivolt units in those cases. Use chip specific
> precision bits to derive the correct channel _scale on every and all
> channel configuration.
>
> Fixes: dc78e71d7c15 ("iio: adc: ad4030: remove some duplicate code")
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---
> This was probalby buggy since
> commit 949abd1ca5a4 ("iio: adc: ad4030: add averaging support")
> but I decided to set the fixes tag with dc78e71d7c15 because this patch will
> not apply cleanly over 949abd1ca5a4.

FWIW, you may add a few Fixes tags: The original one, and the one(s)
which changed
 drastically the code. In any case for the small conflicts we don't
care, just put the correct Fixes and if needed to backport, one
provides an updated patch specifically for backporting.


-- 
With Best Regards,
Andy Shevchenko