[PATCH 0/3] iio:adc:ad7476: Fix, regulator support and binding doc.

[PATCH 0/3] iio:adc:ad7476: Fix, regulator support and binding doc.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

I was only actually aiming to add a missing binding document for this one,
but whilst testing it came across some other things that needed doing.

Jonathan Cameron (3):
  iio:adc:ad7476: Fix remove handling
  iio:adc:ad7476: Handle the different regulators used by various parts.
  dt-bindings:iio:adc:adi,ad7474: Add missing binding document

 .../bindings/iio/adc/adi,ad7476.yaml          | 139 ++++++++++++++++++
 drivers/iio/adc/ad7476.c                      |  99 +++++++++----
 2 files changed, 211 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7476.yaml

-- 
2.31.1

[PATCH 1/3] iio:adc:ad7476: Fix remove handling

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This driver was in an odd half way state between devm based cleanup
and manual cleanup (most of which was missing).
I would guess something went wrong with a rebase or similar.
Anyhow, this basially finishes the job as a precusor to improving
the regulator handling.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Fixes: 4bb2b8f94ace3 ("iio: adc: ad7476: implement devm_add_action_or_reset")
Cc: Michael Hennerich <michael.hennerich@analog.com>
---
 drivers/iio/adc/ad7476.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
index 17402714b387..9e9ff07cf972 100644
--- a/drivers/iio/adc/ad7476.c
+++ b/drivers/iio/adc/ad7476.c
@@ -321,25 +321,15 @@ static int ad7476_probe(struct spi_device *spi)
 	spi_message_init(&st->msg);
 	spi_message_add_tail(&st->xfer, &st->msg);
 
-	ret = iio_triggered_buffer_setup(indio_dev, NULL,
-			&ad7476_trigger_handler, NULL);
+	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, NULL,
+					      &ad7476_trigger_handler, NULL);
 	if (ret)
-		goto error_disable_reg;
+		return ret;
 
 	if (st->chip_info->reset)
 		st->chip_info->reset(st);
 
-	ret = iio_device_register(indio_dev);
-	if (ret)
-		goto error_ring_unregister;
-	return 0;
-
-error_ring_unregister:
-	iio_triggered_buffer_cleanup(indio_dev);
-error_disable_reg:
-	regulator_disable(st->reg);
-
-	return ret;
+	return devm_iio_device_register(&spi->dev, indio_dev);
 }
 
 static const struct spi_device_id ad7476_id[] = {
-- 
2.31.1

Re: [PATCH 1/3] iio:adc:ad7476: Fix remove handling

[Alexandru Ardelean]

On Thu, Apr 1, 2021 at 8:47 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> This driver was in an odd half way state between devm based cleanup
> and manual cleanup (most of which was missing).
> I would guess something went wrong with a rebase or similar.
> Anyhow, this basially finishes the job as a precusor to improving

2 typos in this commit description

> the regulator handling.
>

I was pretty surprised about this patch [before reading through it].
Anyhow:

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Fixes: 4bb2b8f94ace3 ("iio: adc: ad7476: implement devm_add_action_or_reset")
> Cc: Michael Hennerich <michael.hennerich@analog.com>
> ---
>  drivers/iio/adc/ad7476.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
> index 17402714b387..9e9ff07cf972 100644
> --- a/drivers/iio/adc/ad7476.c
> +++ b/drivers/iio/adc/ad7476.c
> @@ -321,25 +321,15 @@ static int ad7476_probe(struct spi_device *spi)
>         spi_message_init(&st->msg);
>         spi_message_add_tail(&st->xfer, &st->msg);
>
> -       ret = iio_triggered_buffer_setup(indio_dev, NULL,
> -                       &ad7476_trigger_handler, NULL);
> +       ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, NULL,
> +                                             &ad7476_trigger_handler, NULL);
>         if (ret)
> -               goto error_disable_reg;
> +               return ret;
>
>         if (st->chip_info->reset)
>                 st->chip_info->reset(st);
>
> -       ret = iio_device_register(indio_dev);
> -       if (ret)
> -               goto error_ring_unregister;
> -       return 0;
> -
> -error_ring_unregister:
> -       iio_triggered_buffer_cleanup(indio_dev);
> -error_disable_reg:
> -       regulator_disable(st->reg);
> -
> -       return ret;
> +       return devm_iio_device_register(&spi->dev, indio_dev);
>  }
>
>  static const struct spi_device_id ad7476_id[] = {
> --
> 2.31.1
>

Re: [PATCH 1/3] iio:adc:ad7476: Fix remove handling

[Jonathan Cameron]

On Fri, 2 Apr 2021 10:34:42 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Thu, Apr 1, 2021 at 8:47 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > This driver was in an odd half way state between devm based cleanup
> > and manual cleanup (most of which was missing).
> > I would guess something went wrong with a rebase or similar.
> > Anyhow, this basially finishes the job as a precusor to improving  
> 
> 2 typos in this commit description

One day I'll learn how to type /spell or at least to remember to use
a spell checker on commit descriptions.
> 
> > the regulator handling.
> >  
> 
> I was pretty surprised about this patch [before reading through it].
Yup. This one definitely got a wtf followed by a groan that it had snuck
through.  Has that look of a rebase going horribly wrong to me and
I missed it completely in the original reviews :(


> Anyhow:
> 
> Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
Thanks,

Applied to the togreg branch of iio.git and marked for stable.
Bit late in cycle to do another fixes pull request so this one
can wait for the merge window.

> 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Fixes: 4bb2b8f94ace3 ("iio: adc: ad7476: implement devm_add_action_or_reset")
> > Cc: Michael Hennerich <michael.hennerich@analog.com>
> > ---
> >  drivers/iio/adc/ad7476.c | 18 ++++--------------
> >  1 file changed, 4 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
> > index 17402714b387..9e9ff07cf972 100644
> > --- a/drivers/iio/adc/ad7476.c
> > +++ b/drivers/iio/adc/ad7476.c
> > @@ -321,25 +321,15 @@ static int ad7476_probe(struct spi_device *spi)
> >         spi_message_init(&st->msg);
> >         spi_message_add_tail(&st->xfer, &st->msg);
> >
> > -       ret = iio_triggered_buffer_setup(indio_dev, NULL,
> > -                       &ad7476_trigger_handler, NULL);
> > +       ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, NULL,
> > +                                             &ad7476_trigger_handler, NULL);
> >         if (ret)
> > -               goto error_disable_reg;
> > +               return ret;
> >
> >         if (st->chip_info->reset)
> >                 st->chip_info->reset(st);
> >
> > -       ret = iio_device_register(indio_dev);
> > -       if (ret)
> > -               goto error_ring_unregister;
> > -       return 0;
> > -
> > -error_ring_unregister:
> > -       iio_triggered_buffer_cleanup(indio_dev);
> > -error_disable_reg:
> > -       regulator_disable(st->reg);
> > -
> > -       return ret;
> > +       return devm_iio_device_register(&spi->dev, indio_dev);
> >  }
> >
> >  static const struct spi_device_id ad7476_id[] = {
> > --
> > 2.31.1
> >  

[PATCH 2/3] iio:adc:ad7476: Handle the different regulators used by various parts.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Not all of the parts supported by this driver use single supply.
Hence we add chip_info fields to say what additional supplies exist
and in the case of vref, ensure that is used for the reference voltage
rather than vcc.

Tested using really simple QEMU model and some fixed regulators.

The devm_add_action_or_reset() callback is changed to take the
regulator as it's parameter so we can use one callback for all the
different regulators without having to store pointers to them in
the iio_priv() structure.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Michael Hennerich <michael.hennerich@analog.com>
---
 drivers/iio/adc/ad7476.c | 81 +++++++++++++++++++++++++++++++++-------
 1 file changed, 68 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
index 9e9ff07cf972..49f7d252b200 100644
--- a/drivers/iio/adc/ad7476.c
+++ b/drivers/iio/adc/ad7476.c
@@ -32,12 +32,14 @@ struct ad7476_chip_info {
 	/* channels used when convst gpio is defined */
 	struct iio_chan_spec		convst_channel[2];
 	void (*reset)(struct ad7476_state *);
+	bool				has_vref;
+	bool				has_vdrive;
 };
 
 struct ad7476_state {
 	struct spi_device		*spi;
 	const struct ad7476_chip_info	*chip_info;
-	struct regulator		*reg;
+	struct regulator		*ref_reg;
 	struct gpio_desc		*convst_gpio;
 	struct spi_transfer		xfer;
 	struct spi_message		msg;
@@ -53,12 +55,15 @@ struct ad7476_state {
 
 enum ad7476_supported_device_ids {
 	ID_AD7091R,
+	ID_AD7273,
+	ID_AD7274,
 	ID_AD7276,
 	ID_AD7277,
 	ID_AD7278,
 	ID_AD7466,
 	ID_AD7467,
 	ID_AD7468,
+	ID_AD7475,
 	ID_AD7495,
 	ID_AD7940,
 	ID_ADC081S,
@@ -146,7 +151,7 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		if (!st->chip_info->int_vref_uv) {
-			scale_uv = regulator_get_voltage(st->reg);
+			scale_uv = regulator_get_voltage(st->ref_reg);
 			if (scale_uv < 0)
 				return scale_uv;
 		} else {
@@ -194,6 +199,16 @@ static const struct ad7476_chip_info ad7476_chip_info_tbl[] = {
 		.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
 		.reset = ad7091_reset,
 	},
+	[ID_AD7273] = {
+		.channel[0] = AD7940_CHAN(10),
+		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+		.has_vref = true,
+	},
+	[ID_AD7274] = {
+		.channel[0] = AD7940_CHAN(12),
+		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+		.has_vref = true,
+	},
 	[ID_AD7276] = {
 		.channel[0] = AD7940_CHAN(12),
 		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
@@ -218,10 +233,17 @@ static const struct ad7476_chip_info ad7476_chip_info_tbl[] = {
 		.channel[0] = AD7476_CHAN(8),
 		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
 	},
+	[ID_AD7475] = {
+		.channel[0] = AD7476_CHAN(12),
+		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+		.has_vref = true,
+		.has_vdrive = true,
+	},
 	[ID_AD7495] = {
 		.channel[0] = AD7476_CHAN(12),
 		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
 		.int_vref_uv = 2500000,
+		.has_vdrive = true,
 	},
 	[ID_AD7940] = {
 		.channel[0] = AD7940_CHAN(14),
@@ -254,6 +276,7 @@ static const struct ad7476_chip_info ad7476_chip_info_tbl[] = {
 	[ID_LTC2314_14] = {
 		.channel[0] = AD7940_CHAN(14),
 		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+		.has_vref = true,
 	},
 };
 
@@ -263,15 +286,16 @@ static const struct iio_info ad7476_info = {
 
 static void ad7476_reg_disable(void *data)
 {
-	struct ad7476_state *st = data;
+	struct regulator *reg = data;
 
-	regulator_disable(st->reg);
+	regulator_disable(reg);
 }
 
 static int ad7476_probe(struct spi_device *spi)
 {
 	struct ad7476_state *st;
 	struct iio_dev *indio_dev;
+	struct regulator *reg;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
@@ -282,19 +306,50 @@ static int ad7476_probe(struct spi_device *spi)
 	st->chip_info =
 		&ad7476_chip_info_tbl[spi_get_device_id(spi)->driver_data];
 
-	st->reg = devm_regulator_get(&spi->dev, "vcc");
-	if (IS_ERR(st->reg))
-		return PTR_ERR(st->reg);
+	reg = devm_regulator_get(&spi->dev, "vcc");
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
 
-	ret = regulator_enable(st->reg);
+	ret = regulator_enable(reg);
 	if (ret)
 		return ret;
 
-	ret = devm_add_action_or_reset(&spi->dev, ad7476_reg_disable,
-				       st);
+	ret = devm_add_action_or_reset(&spi->dev, ad7476_reg_disable, reg);
 	if (ret)
 		return ret;
 
+	if (st->chip_info->has_vref) {
+		reg = devm_regulator_get(&spi->dev, "vref");
+		if (IS_ERR(reg))
+			return PTR_ERR(reg);
+
+		ret = regulator_enable(reg);
+		if (ret)
+			return ret;
+
+		ret = devm_add_action_or_reset(&spi->dev, ad7476_reg_disable,
+					       reg);
+		if (ret)
+			return ret;
+	}
+	/* Either vcc or vref as appropriate */
+	st->ref_reg = reg;
+
+	if (st->chip_info->has_vdrive) {
+		reg = devm_regulator_get(&spi->dev, "vdrive");
+		if (IS_ERR(reg))
+			return PTR_ERR(reg);
+
+		ret = regulator_enable(reg);
+		if (ret)
+			return ret;
+
+		ret = devm_add_action_or_reset(&spi->dev, ad7476_reg_disable,
+					       reg);
+		if (ret)
+			return ret;
+	}
+
 	st->convst_gpio = devm_gpiod_get_optional(&spi->dev,
 						  "adi,conversion-start",
 						  GPIOD_OUT_LOW);
@@ -335,15 +390,15 @@ static int ad7476_probe(struct spi_device *spi)
 static const struct spi_device_id ad7476_id[] = {
 	{"ad7091", ID_AD7091R},
 	{"ad7091r", ID_AD7091R},
-	{"ad7273", ID_AD7277},
-	{"ad7274", ID_AD7276},
+	{"ad7273", ID_AD7273},
+	{"ad7274", ID_AD7274},
 	{"ad7276", ID_AD7276},
 	{"ad7277", ID_AD7277},
 	{"ad7278", ID_AD7278},
 	{"ad7466", ID_AD7466},
 	{"ad7467", ID_AD7467},
 	{"ad7468", ID_AD7468},
-	{"ad7475", ID_AD7466},
+	{"ad7475", ID_AD7475},
 	{"ad7476", ID_AD7466},
 	{"ad7476a", ID_AD7466},
 	{"ad7477", ID_AD7467},
-- 
2.31.1

Re: [PATCH 2/3] iio:adc:ad7476: Handle the different regulators used by various parts.

[Lars-Peter Clausen]

On 4/1/21 7:17 PM, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Not all of the parts supported by this driver use single supply.
> Hence we add chip_info fields to say what additional supplies exist
> and in the case of vref, ensure that is used for the reference voltage
> rather than vcc.
>
> Tested using really simple QEMU model and some fixed regulators.
>
> The devm_add_action_or_reset() callback is changed to take the
> regulator as it's parameter so we can use one callback for all the
> different regulators without having to store pointers to them in
> the iio_priv() structure.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Michael Hennerich <michael.hennerich@analog.com>

Looks good.

Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>

There is one special case that is currently not handled. The AD7091R can 
either use its internal reference, or an external reference.

At the moment the driver sets neither has_vref nor int_vref_uv.

Re: [PATCH 2/3] iio:adc:ad7476: Handle the different regulators used by various parts.

[Jonathan Cameron]

On Fri, 2 Apr 2021 10:35:17 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 4/1/21 7:17 PM, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Not all of the parts supported by this driver use single supply.
> > Hence we add chip_info fields to say what additional supplies exist
> > and in the case of vref, ensure that is used for the reference voltage
> > rather than vcc.
> >
> > Tested using really simple QEMU model and some fixed regulators.
> >
> > The devm_add_action_or_reset() callback is changed to take the
> > regulator as it's parameter so we can use one callback for all the
> > different regulators without having to store pointers to them in
> > the iio_priv() structure.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Michael Hennerich <michael.hennerich@analog.com>  
> 
> Looks good.
> 
> Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>
> 
> There is one special case that is currently not handled. The AD7091R can 
> either use its internal reference, or an external reference.
> 
> At the moment the driver sets neither has_vref nor int_vref_uv.

Ah. I'd missed that one.  Let's fix that whilst we are here.
Enough stuff will change in V2 that I'll not keep that Reviewed-by tag.

Thanks,

Jonathan

[PATCH 3/3] dt-bindings:iio:adc:adi,ad7474: Add missing binding document

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This binding covers class of simple SPI ADCs which only provide
data output - they don't have MOSI pin.

The only real variation between them is over how many supplies they
use and which one is used for the reference.

Michael listed as maintainer for this one as it is his driver and
falls under the catch all MAINTAINERS entry for ADI devices.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Michael Hennerich <michael.hennerich@analog.com>
---
 .../bindings/iio/adc/adi,ad7476.yaml          | 139 ++++++++++++++++++
 1 file changed, 139 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7476.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7476.yaml
new file mode 100644
index 000000000000..465c85add31d
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7476.yaml
@@ -0,0 +1,139 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2019 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7476.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AD7476 and similar simple SPI ADCs from multiple manufacturers.
+
+maintainers:
+  - Michael Hennerich <michael.hennerich@analog.com>
+
+description: |
+  A lot of simple SPI ADCs have very straight forward interfaces.
+  They typically don't provide a MOSI pin, simply reading out data
+  on MISO when the clock toggles.
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7091
+      - adi,ad7091r
+      - adi,ad7273
+      - adi,ad7274
+      - adi,ad7276
+      - adi,ad7277
+      - adi,ad7278
+      - adi,ad7466
+      - adi,ad7467
+      - adi,ad7468
+      - adi,ad7475
+      - adi,ad7476
+      - adi,ad7476a
+      - adi,ad7477
+      - adi,ad7477a
+      - adi,ad7478
+      - adi,ad7478a
+      - adi,ad7495
+      - adi,ad7910
+      - adi,ad7920
+      - adi,ad7940
+      - ti,adc081s
+      - ti,adc101s
+      - ti,adc121s
+      - ti,ads7866
+      - ti,ads7867
+      - ti,ads7868
+      - lltc,ltc2314-14
+
+  reg:
+    maxItems: 1
+
+  vcc-supply:
+    description:
+      Main powersupply voltage for the chips, sometimes referred to as VDD on
+      datasheets.  If there is no separate vref-supply, then this is needed
+      to establish channel scaling.
+
+  vdrive-supply:
+    description:
+      Some devices have separate supply for their digital control side.
+
+  vref-supply:
+    description:
+      Some devices have a specific reference voltage supplied on a different pin
+      to the other supplies. Needed to be able to establish channel scaling.
+
+  spi-max-frequency: true
+
+  adi,conversion-start-gpios:
+    description: A GPIO used to trigger the start of a conversion
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad7091
+              - adi,ad7091r
+    then:
+      properties:
+        adi,conversion-start-gpios: true
+    else:
+      properties:
+        adi,conversion-start-gpios: false
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad7273
+              - adi,ad7274
+              - adi,ad7475
+              - lltc,ltc2314-14
+    then:
+      properties:
+        vref-supply: true
+      required:
+        - vref-supply
+    else:
+      properties:
+        vref-supply: false
+      required:
+        - vcc-supply
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad7475
+              - adi,ad7495
+    then:
+      properties:
+        vdrive-supply: true
+    else:
+      properties:
+        vdrive-supply: false
+
+examples:
+  - |
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      adc@0 {
+        compatible = "adi,ad7091";
+        reg = <0>;
+        spi-max-frequency = <5000000>;
+        vcc-supply = <&adc_vcc>;
+      };
+    };
+...
-- 
2.31.1