linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] iio: adc: ad7291: convert to device tree
@ 2020-03-17 13:56 Michael Auchter
  2020-03-17 13:56 ` [PATCH 2/2] dt-bindings: iio: adc: ad7291: add binding Michael Auchter
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Michael Auchter @ 2020-03-17 13:56 UTC (permalink / raw)
  To: Lars-Peter Clausen, Michael Hennerich, Stefan Popa,
	Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler,
	Liam Girdwood, Mark Brown
  Cc: Michael Auchter, linux-kernel, linux-iio

There are no in-tree users of the platform data for this driver, so
remove it and convert the driver to use device tree instead.

Signed-off-by: Michael Auchter <michael.auchter@ni.com>
---
 drivers/iio/adc/ad7291.c             | 33 ++++++++++++++++------------
 include/linux/platform_data/ad7291.h | 13 -----------
 2 files changed, 19 insertions(+), 27 deletions(-)
 delete mode 100644 include/linux/platform_data/ad7291.h

diff --git a/drivers/iio/adc/ad7291.c b/drivers/iio/adc/ad7291.c
index b2b137fed246..536e31862309 100644
--- a/drivers/iio/adc/ad7291.c
+++ b/drivers/iio/adc/ad7291.c
@@ -20,8 +20,6 @@
 #include <linux/iio/sysfs.h>
 #include <linux/iio/events.h>
 
-#include <linux/platform_data/ad7291.h>
-
 /*
  * Simplified handling
  *
@@ -465,7 +463,6 @@ static const struct iio_info ad7291_info = {
 static int ad7291_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
-	struct ad7291_platform_data *pdata = client->dev.platform_data;
 	struct ad7291_chip_info *chip;
 	struct iio_dev *indio_dev;
 	int ret;
@@ -475,16 +472,6 @@ static int ad7291_probe(struct i2c_client *client,
 		return -ENOMEM;
 	chip = iio_priv(indio_dev);
 
-	if (pdata && pdata->use_external_ref) {
-		chip->reg = devm_regulator_get(&client->dev, "vref");
-		if (IS_ERR(chip->reg))
-			return PTR_ERR(chip->reg);
-
-		ret = regulator_enable(chip->reg);
-		if (ret)
-			return ret;
-	}
-
 	mutex_init(&chip->state_lock);
 	/* this is only used for device removal purposes */
 	i2c_set_clientdata(client, indio_dev);
@@ -495,8 +482,19 @@ static int ad7291_probe(struct i2c_client *client,
 			AD7291_T_SENSE_MASK | /* Tsense always enabled */
 			AD7291_ALERT_POLARITY; /* set irq polarity low level */
 
-	if (pdata && pdata->use_external_ref)
+	chip->reg = devm_regulator_get_optional(&client->dev, "vref");
+	if (IS_ERR(chip->reg)) {
+		if (PTR_ERR(chip->reg) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		chip->reg = NULL;
+	} else {
+		ret = regulator_enable(chip->reg);
+		if (ret)
+			return ret;
+
 		chip->command |= AD7291_EXT_REF;
+	}
 
 	indio_dev->name = id->name;
 	indio_dev->channels = ad7291_channels;
@@ -569,9 +567,16 @@ static const struct i2c_device_id ad7291_id[] = {
 
 MODULE_DEVICE_TABLE(i2c, ad7291_id);
 
+static const struct of_device_id ad7291_of_match[] = {
+	{ .compatible = "adi,ad7291", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ad7291_of_match);
+
 static struct i2c_driver ad7291_driver = {
 	.driver = {
 		.name = KBUILD_MODNAME,
+		.of_match_table = of_match_ptr(ad7291_of_match),
 	},
 	.probe = ad7291_probe,
 	.remove = ad7291_remove,
diff --git a/include/linux/platform_data/ad7291.h b/include/linux/platform_data/ad7291.h
deleted file mode 100644
index b1fd1530c9a5..000000000000
--- a/include/linux/platform_data/ad7291.h
+++ /dev/null
@@ -1,13 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __IIO_AD7291_H__
-#define __IIO_AD7291_H__
-
-/**
- * struct ad7291_platform_data - AD7291 platform data
- * @use_external_ref: Whether to use an external or internal reference voltage
- */
-struct ad7291_platform_data {
-	bool use_external_ref;
-};
-
-#endif
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] dt-bindings: iio: adc: ad7291: add binding
  2020-03-17 13:56 [PATCH 1/2] iio: adc: ad7291: convert to device tree Michael Auchter
@ 2020-03-17 13:56 ` Michael Auchter
  2020-03-17 14:16 ` [PATCH 1/2] iio: adc: ad7291: convert to device tree Lars-Peter Clausen
  2020-03-17 14:51 ` [PATCH v2 " Michael Auchter
  2 siblings, 0 replies; 11+ messages in thread
From: Michael Auchter @ 2020-03-17 13:56 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Rob Herring, Mark Rutland
  Cc: Michael Auchter, linux-iio, devicetree, linux-kernel

Add device-tree binding for ADI AD7291 ADC.

Signed-off-by: Michael Auchter <michael.auchter@ni.com>
---
 .../bindings/iio/adc/adi,ad7291.yaml          | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7291.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7291.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7291.yaml
new file mode 100644
index 000000000000..93aa29413049
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7291.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7291.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AD7291 8-Channel, I2C, 12-Bit SAR ADC with Temperature Sensor
+
+maintainers:
+  - Michael Auchter <michael.auchter@ni.com>
+
+description: |
+  Analog Devices AD7291 8-Channel I2C 12-Bit SAR ADC with Temperature Sensor
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7291.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7291
+
+  reg:
+    maxItems: 1
+
+  vref-supply:
+    description: |
+      The regulator supply for ADC reference voltage.
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      ad7291: adc@0 {
+        compatible = "adi,ad7291";
+        reg = <0>;
+        vref-supply = <&adc_vref>;
+      };
+    };
+
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] iio: adc: ad7291: convert to device tree
  2020-03-17 13:56 [PATCH 1/2] iio: adc: ad7291: convert to device tree Michael Auchter
  2020-03-17 13:56 ` [PATCH 2/2] dt-bindings: iio: adc: ad7291: add binding Michael Auchter
@ 2020-03-17 14:16 ` Lars-Peter Clausen
  2020-03-17 14:51 ` [PATCH v2 " Michael Auchter
  2 siblings, 0 replies; 11+ messages in thread
From: Lars-Peter Clausen @ 2020-03-17 14:16 UTC (permalink / raw)
  To: Michael Auchter, Michael Hennerich, Stefan Popa,
	Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler,
	Liam Girdwood, Mark Brown
  Cc: linux-kernel, linux-iio

On 3/17/20 2:56 PM, Michael Auchter wrote:
> There are no in-tree users of the platform data for this driver, so
> remove it and convert the driver to use device tree instead.
> 
> Signed-off-by: Michael Auchter <michael.auchter@ni.com>

Hi,

Thanks for the patch, looks good for the most part. One comment inline.

> 
> diff --git a/drivers/iio/adc/ad7291.c b/drivers/iio/adc/ad7291.c
> index b2b137fed246..536e31862309 100644
> --- a/drivers/iio/adc/ad7291.c
> +++ b/drivers/iio/adc/ad7291.c
> @@ -495,8 +482,19 @@ static int ad7291_probe(struct i2c_client *client,
>   			AD7291_T_SENSE_MASK | /* Tsense always enabled */
>   			AD7291_ALERT_POLARITY; /* set irq polarity low level */
>   
> -	if (pdata && pdata->use_external_ref)
> +	chip->reg = devm_regulator_get_optional(&client->dev, "vref");
> +	if (IS_ERR(chip->reg)) {
> +		if (PTR_ERR(chip->reg) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;

We should only continue if the error is ENODEV, which means that no 
regulator was specified. Any other error means it was specified but 
there was an issue requesting it. See for example ad7266.c

> +
> +		chip->reg = NULL;
> +	} else {
> +		ret = regulator_enable(chip->reg);
> +		if (ret)
> +			return ret;
> +
>   		chip->command |= AD7291_EXT_REF;
> +	}

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 1/2] iio: adc: ad7291: convert to device tree
  2020-03-17 13:56 [PATCH 1/2] iio: adc: ad7291: convert to device tree Michael Auchter
  2020-03-17 13:56 ` [PATCH 2/2] dt-bindings: iio: adc: ad7291: add binding Michael Auchter
  2020-03-17 14:16 ` [PATCH 1/2] iio: adc: ad7291: convert to device tree Lars-Peter Clausen
@ 2020-03-17 14:51 ` Michael Auchter
  2020-03-17 14:51   ` [PATCH v2 2/2] dt-bindings: iio: adc: ad7291: add binding Michael Auchter
                     ` (3 more replies)
  2 siblings, 4 replies; 11+ messages in thread
From: Michael Auchter @ 2020-03-17 14:51 UTC (permalink / raw)
  To: Lars-Peter Clausen, Michael Hennerich, Stefan Popa,
	Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler,
	Liam Girdwood, Mark Brown
  Cc: Michael Auchter, linux-kernel, linux-iio

There are no in-tree users of the platform data for this driver, so
remove it and convert the driver to use device tree instead.

Signed-off-by: Michael Auchter <michael.auchter@ni.com>
---

Changes since v1:
- Fix regulator handling as suggested by Lars

 drivers/iio/adc/ad7291.c             | 33 ++++++++++++++++------------
 include/linux/platform_data/ad7291.h | 13 -----------
 2 files changed, 19 insertions(+), 27 deletions(-)
 delete mode 100644 include/linux/platform_data/ad7291.h

diff --git a/drivers/iio/adc/ad7291.c b/drivers/iio/adc/ad7291.c
index b2b137fed246..43a201fb4f34 100644
--- a/drivers/iio/adc/ad7291.c
+++ b/drivers/iio/adc/ad7291.c
@@ -20,8 +20,6 @@
 #include <linux/iio/sysfs.h>
 #include <linux/iio/events.h>
 
-#include <linux/platform_data/ad7291.h>
-
 /*
  * Simplified handling
  *
@@ -465,7 +463,6 @@ static const struct iio_info ad7291_info = {
 static int ad7291_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
-	struct ad7291_platform_data *pdata = client->dev.platform_data;
 	struct ad7291_chip_info *chip;
 	struct iio_dev *indio_dev;
 	int ret;
@@ -475,16 +472,6 @@ static int ad7291_probe(struct i2c_client *client,
 		return -ENOMEM;
 	chip = iio_priv(indio_dev);
 
-	if (pdata && pdata->use_external_ref) {
-		chip->reg = devm_regulator_get(&client->dev, "vref");
-		if (IS_ERR(chip->reg))
-			return PTR_ERR(chip->reg);
-
-		ret = regulator_enable(chip->reg);
-		if (ret)
-			return ret;
-	}
-
 	mutex_init(&chip->state_lock);
 	/* this is only used for device removal purposes */
 	i2c_set_clientdata(client, indio_dev);
@@ -495,8 +482,19 @@ static int ad7291_probe(struct i2c_client *client,
 			AD7291_T_SENSE_MASK | /* Tsense always enabled */
 			AD7291_ALERT_POLARITY; /* set irq polarity low level */
 
-	if (pdata && pdata->use_external_ref)
+	chip->reg = devm_regulator_get_optional(&client->dev, "vref");
+	if (!IS_ERR(chip->reg)) {
+		ret = regulator_enable(chip->reg);
+		if (ret)
+			return ret;
+
 		chip->command |= AD7291_EXT_REF;
+	} else {
+		if (PTR_ERR(chip->reg) != -ENODEV)
+			return PTR_ERR(chip->reg);
+
+		chip->reg = NULL;
+	}
 
 	indio_dev->name = id->name;
 	indio_dev->channels = ad7291_channels;
@@ -569,9 +567,16 @@ static const struct i2c_device_id ad7291_id[] = {
 
 MODULE_DEVICE_TABLE(i2c, ad7291_id);
 
+static const struct of_device_id ad7291_of_match[] = {
+	{ .compatible = "adi,ad7291", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ad7291_of_match);
+
 static struct i2c_driver ad7291_driver = {
 	.driver = {
 		.name = KBUILD_MODNAME,
+		.of_match_table = of_match_ptr(ad7291_of_match),
 	},
 	.probe = ad7291_probe,
 	.remove = ad7291_remove,
diff --git a/include/linux/platform_data/ad7291.h b/include/linux/platform_data/ad7291.h
deleted file mode 100644
index b1fd1530c9a5..000000000000
--- a/include/linux/platform_data/ad7291.h
+++ /dev/null
@@ -1,13 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __IIO_AD7291_H__
-#define __IIO_AD7291_H__
-
-/**
- * struct ad7291_platform_data - AD7291 platform data
- * @use_external_ref: Whether to use an external or internal reference voltage
- */
-struct ad7291_platform_data {
-	bool use_external_ref;
-};
-
-#endif
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 2/2] dt-bindings: iio: adc: ad7291: add binding
  2020-03-17 14:51 ` [PATCH v2 " Michael Auchter
@ 2020-03-17 14:51   ` Michael Auchter
  2020-03-30 21:16     ` Rob Herring
  2020-03-17 16:13   ` [PATCH v2 1/2] iio: adc: ad7291: convert to device tree Lars-Peter Clausen
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Michael Auchter @ 2020-03-17 14:51 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Rob Herring, Mark Rutland
  Cc: Michael Auchter, linux-iio, devicetree, linux-kernel

Add device-tree binding for ADI AD7291 ADC.

Signed-off-by: Michael Auchter <michael.auchter@ni.com>
---
 .../bindings/iio/adc/adi,ad7291.yaml          | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7291.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7291.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7291.yaml
new file mode 100644
index 000000000000..93aa29413049
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7291.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7291.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AD7291 8-Channel, I2C, 12-Bit SAR ADC with Temperature Sensor
+
+maintainers:
+  - Michael Auchter <michael.auchter@ni.com>
+
+description: |
+  Analog Devices AD7291 8-Channel I2C 12-Bit SAR ADC with Temperature Sensor
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7291.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7291
+
+  reg:
+    maxItems: 1
+
+  vref-supply:
+    description: |
+      The regulator supply for ADC reference voltage.
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      ad7291: adc@0 {
+        compatible = "adi,ad7291";
+        reg = <0>;
+        vref-supply = <&adc_vref>;
+      };
+    };
+
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] iio: adc: ad7291: convert to device tree
  2020-03-17 14:51 ` [PATCH v2 " Michael Auchter
  2020-03-17 14:51   ` [PATCH v2 2/2] dt-bindings: iio: adc: ad7291: add binding Michael Auchter
@ 2020-03-17 16:13   ` Lars-Peter Clausen
  2020-03-21 23:46   ` Andy Shevchenko
  2020-03-31 11:10   ` Ardelean, Alexandru
  3 siblings, 0 replies; 11+ messages in thread
From: Lars-Peter Clausen @ 2020-03-17 16:13 UTC (permalink / raw)
  To: Michael Auchter, Michael Hennerich, Stefan Popa,
	Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler,
	Liam Girdwood, Mark Brown
  Cc: linux-kernel, linux-iio

On 3/17/20 3:51 PM, Michael Auchter wrote:
> There are no in-tree users of the platform data for this driver, so
> remove it and convert the driver to use device tree instead.
> 
> Signed-off-by: Michael Auchter <michael.auchter@ni.com>

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] iio: adc: ad7291: convert to device tree
  2020-03-17 14:51 ` [PATCH v2 " Michael Auchter
  2020-03-17 14:51   ` [PATCH v2 2/2] dt-bindings: iio: adc: ad7291: add binding Michael Auchter
  2020-03-17 16:13   ` [PATCH v2 1/2] iio: adc: ad7291: convert to device tree Lars-Peter Clausen
@ 2020-03-21 23:46   ` Andy Shevchenko
  2020-03-30 20:12     ` Michael Auchter
  2020-03-31 11:10   ` Ardelean, Alexandru
  3 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2020-03-21 23:46 UTC (permalink / raw)
  To: Michael Auchter
  Cc: Lars-Peter Clausen, Michael Hennerich, Stefan Popa,
	Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler,
	Liam Girdwood, Mark Brown, Linux Kernel Mailing List, linux-iio

On Tue, Mar 17, 2020 at 4:53 PM Michael Auchter <michael.auchter@ni.com> wrote:
>
> There are no in-tree users of the platform data for this driver, so
> remove it and convert the driver to use device tree instead.

...

> +       chip->reg = devm_regulator_get_optional(&client->dev, "vref");
> +       if (!IS_ERR(chip->reg)) {

Why not to go with usual positive conditional?

> +               ret = regulator_enable(chip->reg);
> +               if (ret)
> +                       return ret;
> +
>                 chip->command |= AD7291_EXT_REF;
> +       } else {
> +               if (PTR_ERR(chip->reg) != -ENODEV)
> +                       return PTR_ERR(chip->reg);
> +
> +               chip->reg = NULL;
> +       }

...

> +static const struct of_device_id ad7291_of_match[] = {
> +       { .compatible = "adi,ad7291", },

> +       {},

No need for comma.

> +};

...

> +               .of_match_table = of_match_ptr(ad7291_of_match),

No need to use of_match_ptr(). Haven't you got a compiler warning in !OF case?

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Re: [PATCH v2 1/2] iio: adc: ad7291: convert to device tree
  2020-03-21 23:46   ` Andy Shevchenko
@ 2020-03-30 20:12     ` Michael Auchter
  2020-03-31 10:56       ` Andy Shevchenko
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Auchter @ 2020-03-30 20:12 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Lars-Peter Clausen, Michael Hennerich, Stefan Popa,
	Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler,
	Liam Girdwood, Mark Brown, Linux Kernel Mailing List, linux-iio

Hello Andy,

Thanks for the review!

On Sun, Mar 22, 2020 at 01:46:21AM +0200, Andy Shevchenko wrote:
> On Tue, Mar 17, 2020 at 4:53 PM Michael Auchter <michael.auchter@ni.com> wrote:
> >
> > There are no in-tree users of the platform data for this driver, so
> > remove it and convert the driver to use device tree instead.
> 
> ...
> 
> > +       chip->reg = devm_regulator_get_optional(&client->dev, "vref");
> > +       if (!IS_ERR(chip->reg)) {
> 
> Why not to go with usual positive conditional?

I took this pattern from ad7266.c which Lars pointed me to. I agree that
a positive conditional here would probably be more natural. I'll change
that if you'd prefer.

> > +               ret = regulator_enable(chip->reg);
> > +               if (ret)
> > +                       return ret;
> > +
> >                 chip->command |= AD7291_EXT_REF;
> > +       } else {
> > +               if (PTR_ERR(chip->reg) != -ENODEV)
> > +                       return PTR_ERR(chip->reg);
> > +
> > +               chip->reg = NULL;
> > +       }
> 
> ...
> 
> > +static const struct of_device_id ad7291_of_match[] = {
> > +       { .compatible = "adi,ad7291", },
> 
> > +       {},
> 
> No need for comma.

Indeed, I'll drop it.

> 
> > +};
> 
> ...
> 
> > +               .of_match_table = of_match_ptr(ad7291_of_match),
> 
> No need to use of_match_ptr(). Haven't you got a compiler warning in !OF case?

Hm, no warning as far as I can see with !OF... but agreed that this
doesn't make much sense as-is.

Is dropping of_match_ptr() the preferred route here? The driver doesn't
depend on OF, so it seems like keeping of_match_ptr and instead guarding
the ad7291_of_match table with #ifdef CONFIG_OF would be preferred. Of
course, maybe that's not worth it for saving some bytes from the final
image.

Let me know which route would be preferred.

Thanks again,
 Michael

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] dt-bindings: iio: adc: ad7291: add binding
  2020-03-17 14:51   ` [PATCH v2 2/2] dt-bindings: iio: adc: ad7291: add binding Michael Auchter
@ 2020-03-30 21:16     ` Rob Herring
  0 siblings, 0 replies; 11+ messages in thread
From: Rob Herring @ 2020-03-30 21:16 UTC (permalink / raw)
  To: Michael Auchter
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Mark Rutland, Michael Auchter, linux-iio,
	devicetree, linux-kernel

On Tue, 17 Mar 2020 09:51:13 -0500, Michael Auchter wrote:
> Add device-tree binding for ADI AD7291 ADC.
> 
> Signed-off-by: Michael Auchter <michael.auchter@ni.com>
> ---
>  .../bindings/iio/adc/adi,ad7291.yaml          | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7291.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Re: [PATCH v2 1/2] iio: adc: ad7291: convert to device tree
  2020-03-30 20:12     ` Michael Auchter
@ 2020-03-31 10:56       ` Andy Shevchenko
  0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2020-03-31 10:56 UTC (permalink / raw)
  To: Michael Auchter
  Cc: Lars-Peter Clausen, Michael Hennerich, Stefan Popa,
	Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler,
	Liam Girdwood, Mark Brown, Linux Kernel Mailing List, linux-iio

On Mon, Mar 30, 2020 at 11:12 PM Michael Auchter <michael.auchter@ni.com> wrote:
> On Sun, Mar 22, 2020 at 01:46:21AM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 17, 2020 at 4:53 PM Michael Auchter <michael.auchter@ni.com> wrote:

...

> > > +       chip->reg = devm_regulator_get_optional(&client->dev, "vref");
> > > +       if (!IS_ERR(chip->reg)) {
> >
> > Why not to go with usual positive conditional?
>
> I took this pattern from ad7266.c which Lars pointed me to. I agree that
> a positive conditional here would probably be more natural. I'll change
> that if you'd prefer.

Yes, please do.

...

> > > +               .of_match_table = of_match_ptr(ad7291_of_match),
> >
> > No need to use of_match_ptr(). Haven't you got a compiler warning in !OF case?
>
> Hm, no warning as far as I can see with !OF...

Have you used `make W=1 ...`? With it you should get a warning that
table defined but not used.

> but agreed that this
> doesn't make much sense as-is.
>
> Is dropping of_match_ptr() the preferred route here? The driver doesn't
> depend on OF, so it seems like keeping of_match_ptr and instead guarding
> the ad7291_of_match table with #ifdef CONFIG_OF would be preferred. Of
> course, maybe that's not worth it for saving some bytes from the final
> image.

You need either both (of_match_ptr() _and_ ugly ifdeffery, and note
you will need of.h for that) or none (mod_devicetable.h maybe needed,
though).

> Let me know which route would be preferred.

If we would like to use this in non-DT environment, then drop all that
OF-specific stuff.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] iio: adc: ad7291: convert to device tree
  2020-03-17 14:51 ` [PATCH v2 " Michael Auchter
                     ` (2 preceding siblings ...)
  2020-03-21 23:46   ` Andy Shevchenko
@ 2020-03-31 11:10   ` Ardelean, Alexandru
  3 siblings, 0 replies; 11+ messages in thread
From: Ardelean, Alexandru @ 2020-03-31 11:10 UTC (permalink / raw)
  To: broonie, lars, knaack.h, Hennerich, Michael, jic23, lgirdwood,
	pmeerw, michael.auchter
  Cc: linux-kernel, linux-iio

On Tue, 2020-03-17 at 09:51 -0500, Michael Auchter wrote:
> [External]
> 
> There are no in-tree users of the platform data for this driver, so
> remove it and convert the driver to use device tree instead.
> 

A few comments inline for this.
Sorry if this is a bit late, but since there will be a V3 based on the DT
bindings patch, it's still not too late.

> Signed-off-by: Michael Auchter <michael.auchter@ni.com>
> ---
> 
> Changes since v1:
> - Fix regulator handling as suggested by Lars
> 
>  drivers/iio/adc/ad7291.c             | 33 ++++++++++++++++------------
>  include/linux/platform_data/ad7291.h | 13 -----------
>  2 files changed, 19 insertions(+), 27 deletions(-)
>  delete mode 100644 include/linux/platform_data/ad7291.h
> 
> diff --git a/drivers/iio/adc/ad7291.c b/drivers/iio/adc/ad7291.c
> index b2b137fed246..43a201fb4f34 100644
> --- a/drivers/iio/adc/ad7291.c
> +++ b/drivers/iio/adc/ad7291.c
> @@ -20,8 +20,6 @@
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/events.h>
>  
> -#include <linux/platform_data/ad7291.h>
> -
>  /*
>   * Simplified handling
>   *
> @@ -465,7 +463,6 @@ static const struct iio_info ad7291_info = {
>  static int ad7291_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
> -	struct ad7291_platform_data *pdata = client->dev.platform_data;
>  	struct ad7291_chip_info *chip;
>  	struct iio_dev *indio_dev;
>  	int ret;
> @@ -475,16 +472,6 @@ static int ad7291_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  	chip = iio_priv(indio_dev);
>  
> -	if (pdata && pdata->use_external_ref) {
> -		chip->reg = devm_regulator_get(&client->dev, "vref");
> -		if (IS_ERR(chip->reg))
> -			return PTR_ERR(chip->reg);
> -
> -		ret = regulator_enable(chip->reg);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	mutex_init(&chip->state_lock);
>  	/* this is only used for device removal purposes */
>  	i2c_set_clientdata(client, indio_dev);
> @@ -495,8 +482,19 @@ static int ad7291_probe(struct i2c_client *client,
>  			AD7291_T_SENSE_MASK | /* Tsense always enabled */
>  			AD7291_ALERT_POLARITY; /* set irq polarity low level */
>  
> -	if (pdata && pdata->use_external_ref)
> +	chip->reg = devm_regulator_get_optional(&client->dev, "vref");
> +	if (!IS_ERR(chip->reg)) {
> +		ret = regulator_enable(chip->reg);
> +		if (ret)
> +			return ret;
> +
>  		chip->command |= AD7291_EXT_REF;
> +	} else {
> +		if (PTR_ERR(chip->reg) != -ENODEV)
> +			return PTR_ERR(chip->reg);
> +
> +		chip->reg = NULL;
> +	}
>  
>  	indio_dev->name = id->name;
>  	indio_dev->channels = ad7291_channels;
> @@ -569,9 +567,16 @@ static const struct i2c_device_id ad7291_id[] = {
>  
>  MODULE_DEVICE_TABLE(i2c, ad7291_id);
>  
> +static const struct of_device_id ad7291_of_match[] = {
> +	{ .compatible = "adi,ad7291", },
> +	{},

[2]
if updating [1], maybe remove the trailing comma for the null-terminator;
so,  {},  ->  {}
not a biggy, but there are chances that someone else might complain about it
before Jonathan gets a chance to look over this;

> +};
> +MODULE_DEVICE_TABLE(of, ad7291_of_match);
> +
>  static struct i2c_driver ad7291_driver = {
>  	.driver = {
>  		.name = KBUILD_MODNAME,
> +		.of_match_table = of_match_ptr(ad7291_of_match),

[1]
not sure if there was a comment about this, but I'd remove the of_match_ptr()
and just assign this directly;
i.e.  .of_match_table = ad7297_of_match,

there is some code that can also handle this OF-table for ACPI as well; I don't
know if this is sufficient for ACPI [with this patch], but it's a good idea to
prepare for that;


>  	},
>  	.probe = ad7291_probe,
>  	.remove = ad7291_remove,
> diff --git a/include/linux/platform_data/ad7291.h
> b/include/linux/platform_data/ad7291.h
> deleted file mode 100644
> index b1fd1530c9a5..000000000000
> --- a/include/linux/platform_data/ad7291.h
> +++ /dev/null
> @@ -1,13 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef __IIO_AD7291_H__
> -#define __IIO_AD7291_H__
> -
> -/**
> - * struct ad7291_platform_data - AD7291 platform data
> - * @use_external_ref: Whether to use an external or internal reference
> voltage
> - */
> -struct ad7291_platform_data {
> -	bool use_external_ref;
> -};
> -
> -#endif

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-03-31 11:11 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-17 13:56 [PATCH 1/2] iio: adc: ad7291: convert to device tree Michael Auchter
2020-03-17 13:56 ` [PATCH 2/2] dt-bindings: iio: adc: ad7291: add binding Michael Auchter
2020-03-17 14:16 ` [PATCH 1/2] iio: adc: ad7291: convert to device tree Lars-Peter Clausen
2020-03-17 14:51 ` [PATCH v2 " Michael Auchter
2020-03-17 14:51   ` [PATCH v2 2/2] dt-bindings: iio: adc: ad7291: add binding Michael Auchter
2020-03-30 21:16     ` Rob Herring
2020-03-17 16:13   ` [PATCH v2 1/2] iio: adc: ad7291: convert to device tree Lars-Peter Clausen
2020-03-21 23:46   ` Andy Shevchenko
2020-03-30 20:12     ` Michael Auchter
2020-03-31 10:56       ` Andy Shevchenko
2020-03-31 11:10   ` Ardelean, Alexandru

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).