On Fri, 9 Sep 2022 17:14:11 +0200
Nuno Sá <nuno.sa@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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;
>
