[PATCH 0/3] ad7923 fixes and full range support

[PATCH 0/3] ad7923 fixes and full range support

[Nuno Sá]

This patchset adds important fixes for some of the variants supported by the
driver with respect with raw and buffered reads. On top of this way, adds
a new firmware property to be able to support all the scales supported by
the hardware.

Nuno Sá (3):
  iio: adc: ad7923: fix channel readings for some variants
  iio: adc: ad7923: support extended range
  dt-bindings: iio: adi,ad7923: add range-select property

 .../bindings/iio/adc/adi,ad7923.yaml          |  8 +++
 drivers/iio/adc/ad7923.c                      | 62 ++++++++++++-------
 2 files changed, 47 insertions(+), 23 deletions(-)

-- 
2.37.3

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

[Nuno Sá]

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(-)

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;
 
-- 
2.37.3

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

[Jonathan Cameron]

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;
>  

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

[Nuno Sá]

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á

[PATCH 2/3] iio: adc: ad7923: support extended range

[Nuno Sá]

By default the driver was always setting the RANGE bit which means that
the analog input goes from 0 to VREF. However, we might want to have 0
to 2xVREF. This change adds a new FW property to allow for the range
selection while keeping the default behavior if nothing is provided.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/ad7923.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c
index 910cf05e75cd..4bd0ae209d18 100644
--- a/drivers/iio/adc/ad7923.c
+++ b/drivers/iio/adc/ad7923.c
@@ -6,8 +6,10 @@
  * Copyright 2012 CS Systemes d'Information
  */
 
+#include <linux/bitfield.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
 #include <linux/spi/spi.h>
@@ -45,6 +47,8 @@
 /* val = value, dec = left shift, bits = number of bits of the mask */
 #define EXTRACT(val, dec, bits)		(((val) >> (dec)) & ((1 << (bits)) - 1))
 
+#define AD7923_RANGE_0_VREF	1
+
 struct ad7923_state {
 	struct spi_device		*spi;
 	struct spi_transfer		ring_xfer[5];
@@ -300,6 +304,7 @@ static void ad7923_regulator_disable(void *data)
 
 static int ad7923_probe(struct spi_device *spi)
 {
+	u32 ad7923_range = FIELD_PREP(AD7923_RANGE, AD7923_RANGE_0_VREF), val;
 	struct ad7923_state *st;
 	struct iio_dev *indio_dev;
 	const struct ad7923_chip_info *info;
@@ -311,8 +316,17 @@ static int ad7923_probe(struct spi_device *spi)
 
 	st = iio_priv(indio_dev);
 
+	ret = device_property_read_u32(&spi->dev, "adi,range-select", &val);
+	if (!ret) {
+		if (val > AD7923_RANGE_0_VREF)
+			return dev_err_probe(&spi->dev, -EINVAL,
+					     "Invalid range (%u) selected\n", val);
+
+		ad7923_range = FIELD_PREP(AD7923_RANGE, val);
+	}
+
 	st->spi = spi;
-	st->settings = AD7923_CODING | AD7923_RANGE |
+	st->settings = AD7923_CODING | ad7923_range |
 			AD7923_PM_MODE_WRITE(AD7923_PM_MODE_OPS);
 
 	info = &ad7923_chip_info[spi_get_device_id(spi)->driver_data];
-- 
2.37.3

[PATCH 3/3] dt-bindings: iio: adi,ad7923: add range-select property

[Nuno Sá]

Document the new property to select the desired analog input range.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml
index 40b0a887db57..9041020bdb81 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml
@@ -36,6 +36,14 @@ properties:
     description: |
       The regulator supply for ADC reference voltage.
 
+  adi,range-select:
+    description: Selects the analog input range.
+      0 - 0 to 2xVREF
+      1 - 0 to VREF
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1]
+    default: 1
+
   '#address-cells':
     const: 1
 
-- 
2.37.3

Re: [PATCH 3/3] dt-bindings: iio: adi,ad7923: add range-select property

[Jonathan Cameron]

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

> Document the new property to select the desired analog input range.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml
> index 40b0a887db57..9041020bdb81 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml
> @@ -36,6 +36,14 @@ properties:
>      description: |
>        The regulator supply for ADC reference voltage.
>  
> +  adi,range-select:
> +    description: Selects the analog input range.
> +      0 - 0 to 2xVREF
> +      1 - 0 to VREF
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]
> +    default: 1
> +

Would this be better as a flag / boolean called something like
adi,range-double?

>    '#address-cells':
>      const: 1
>  

Re: [PATCH 3/3] dt-bindings: iio: adi,ad7923: add range-select property

[Nuno Sá]

On Sun, 2022-09-11 at 12:28 +0100, Jonathan Cameron wrote:
> On Fri, 9 Sep 2022 17:14:13 +0200
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > Document the new property to select the desired analog input range.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > ---
> >  Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml | 8
> > ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml
> > b/Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml
> > index 40b0a887db57..9041020bdb81 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml
> > @@ -36,6 +36,14 @@ properties:
> >      description: |
> >        The regulator supply for ADC reference voltage.
> >  
> > +  adi,range-select:
> > +    description: Selects the analog input range.
> > +      0 - 0 to 2xVREF
> > +      1 - 0 to VREF
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1]
> > +    default: 1
> > +
> 
> Would this be better as a flag / boolean called something like
> adi,range-double?
> 

In my first draft I actually had something like 'adi,range-disable' but
that felt strange and hacky... Your boolean suggestion is much better
and I have no problems in changing (as the code will also be simpler).
> 

- Nuno Sá