Re: [PATCH 1/3] iio: adc: ad7923: fix channel readings for some variants

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 9 Sep 2022 17:14:11 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> Some of the supported devices have 4 or 2 LSB trailing bits that should
> not be taken into account. Hence we need to shift these bits out which
> fits perfectly on the scan type shift property. This change fixes both
> raw and buffered reads.

Hi Nuno,

Seems that all the values of shift are 12 - realbits.
If that's the case, can we reduce the noise this patch creates by just
updating AD7923_V_CHAN() to set .shift = 12 - (bits) ?

I guess that's not as flexible if anyone adds support for a device
with different shifts, but I suspect that may never happen.

Given we want a fix to be minimal (and hence as likely as possible to backport
cleanly) I think that approach would be a cleaner choice.

Thanks,

Jonathan
> 
> Fixes: f2f7a449707e ("iio:adc:ad7923: Add support for the ad7904/ad7914/ad7924")
> Fixes: 851644a60d20 ("iio: adc: ad7923: Add support for the ad7908/ad7918/ad7928")
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  drivers/iio/adc/ad7923.c | 46 +++++++++++++++++++++-------------------
>  1 file changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c
> index edad1f30121d..910cf05e75cd 100644
> --- a/drivers/iio/adc/ad7923.c
> +++ b/drivers/iio/adc/ad7923.c
> @@ -80,7 +80,7 @@ enum ad7923_id {
>  	AD7928
>  };
>  
> -#define AD7923_V_CHAN(index, bits)					\
> +#define AD7923_V_CHAN(index, bits, _shift)				\
>  	{								\
>  		.type = IIO_VOLTAGE,					\
>  		.indexed = 1,						\
> @@ -93,38 +93,39 @@ enum ad7923_id {
>  			.sign = 'u',					\
>  			.realbits = (bits),				\
>  			.storagebits = 16,				\
> +			.shift = (_shift),				\
>  			.endianness = IIO_BE,				\
>  		},							\
>  	}
>  
> -#define DECLARE_AD7923_CHANNELS(name, bits) \
> +#define DECLARE_AD7923_CHANNELS(name, bits, shift) \
>  const struct iio_chan_spec name ## _channels[] = { \
> -	AD7923_V_CHAN(0, bits), \
> -	AD7923_V_CHAN(1, bits), \
> -	AD7923_V_CHAN(2, bits), \
> -	AD7923_V_CHAN(3, bits), \
> +	AD7923_V_CHAN(0, bits, shift), \
> +	AD7923_V_CHAN(1, bits, shift), \
> +	AD7923_V_CHAN(2, bits, shift), \
> +	AD7923_V_CHAN(3, bits, shift), \
>  	IIO_CHAN_SOFT_TIMESTAMP(4), \
>  }
>  
> -#define DECLARE_AD7908_CHANNELS(name, bits) \
> +#define DECLARE_AD7908_CHANNELS(name, bits, shift) \
>  const struct iio_chan_spec name ## _channels[] = { \
> -	AD7923_V_CHAN(0, bits), \
> -	AD7923_V_CHAN(1, bits), \
> -	AD7923_V_CHAN(2, bits), \
> -	AD7923_V_CHAN(3, bits), \
> -	AD7923_V_CHAN(4, bits), \
> -	AD7923_V_CHAN(5, bits), \
> -	AD7923_V_CHAN(6, bits), \
> -	AD7923_V_CHAN(7, bits), \
> +	AD7923_V_CHAN(0, bits, shift), \
> +	AD7923_V_CHAN(1, bits, shift), \
> +	AD7923_V_CHAN(2, bits, shift), \
> +	AD7923_V_CHAN(3, bits, shift), \
> +	AD7923_V_CHAN(4, bits, shift), \
> +	AD7923_V_CHAN(5, bits, shift), \
> +	AD7923_V_CHAN(6, bits, shift), \
> +	AD7923_V_CHAN(7, bits, shift), \
>  	IIO_CHAN_SOFT_TIMESTAMP(8), \
>  }
>  
> -static DECLARE_AD7923_CHANNELS(ad7904, 8);
> -static DECLARE_AD7923_CHANNELS(ad7914, 10);
> -static DECLARE_AD7923_CHANNELS(ad7924, 12);
> -static DECLARE_AD7908_CHANNELS(ad7908, 8);
> -static DECLARE_AD7908_CHANNELS(ad7918, 10);
> -static DECLARE_AD7908_CHANNELS(ad7928, 12);
> +static DECLARE_AD7923_CHANNELS(ad7904, 8, 4);
> +static DECLARE_AD7923_CHANNELS(ad7914, 10, 2);
> +static DECLARE_AD7923_CHANNELS(ad7924, 12, 0);
> +static DECLARE_AD7908_CHANNELS(ad7908, 8, 4);
> +static DECLARE_AD7908_CHANNELS(ad7918, 10, 2);
> +static DECLARE_AD7908_CHANNELS(ad7928, 12, 0);
>  
>  static const struct ad7923_chip_info ad7923_chip_info[] = {
>  	[AD7904] = {
> @@ -268,7 +269,8 @@ static int ad7923_read_raw(struct iio_dev *indio_dev,
>  			return ret;
>  
>  		if (chan->address == EXTRACT(ret, 12, 4))
> -			*val = EXTRACT(ret, 0, 12);
> +			*val = EXTRACT(ret, chan->scan_type.shift,
> +				       chan->scan_type.realbits);
>  		else
>  			return -EIO;
>