| Message ID | 20220909151413.1164754-2-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ad7923 fixes and full range support | expand |
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; >
On Sun, 2022-09-11 at 12:26 +0100, Jonathan Cameron wrote: > 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, Hi Jonathan, > > 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) ? > Yes, it should be pretty much the same... As I don't have any strong feelings I can do as you suggest. > I guess that's not as flexible if anyone adds support for a device > with different shifts, but I suspect that may never happen. > Or a device with realbits > 12. But yeah, I'm also fairly positive we won't see that happening... - Nuno Sá
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;
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. 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(-)