* [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 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: [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: 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