[PATCH 0/3] staging: iio: cdc: ad7746: remove platform_data in favor of device tree bindings

[PATCH 0/3] staging: iio: cdc: ad7746: remove platform_data in favor of device tree bindings

[Lucas Stankus]

This patch series aims to replace the platform_struct for the ad7746 driver
in favor of device tree bindings, creating the dt-bindings documentation in
the process.

Since the header file was only used to define the struct and the excitation
level values, it was possible to remove the file entirely.

Lucas Stankus (3):
  dt-bindings: staging: iio: cdc: ad7746: add binding documentation for
    AD7746
  staging: iio: cdc: ad7746: use dt bindings to set the EXCx pins output
  staging: iio: cdc: ad7746: use dt binding to set the excitation level

 .../bindings/iio/cdc/adi,ad7746.yaml          | 79 +++++++++++++++++++
 drivers/staging/iio/cdc/ad7746.c              | 43 +++++-----
 drivers/staging/iio/cdc/ad7746.h              | 28 -------
 3 files changed, 100 insertions(+), 50 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml
 delete mode 100644 drivers/staging/iio/cdc/ad7746.h

-- 
2.31.1

[PATCH 1/3] dt-bindings: staging: iio: cdc: ad7746: add binding documentation for AD7746

[Lucas Stankus]

Add device tree binding documentation for AD7746 cdc in YAML format.

Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
---
 .../bindings/iio/cdc/adi,ad7746.yaml          | 79 +++++++++++++++++++
 1 file changed, 79 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml

diff --git a/Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml b/Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml
new file mode 100644
index 000000000000..5de86f4374e1
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml
@@ -0,0 +1,79 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/cdc/adi,ad7746.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AD7746 24-Bit Capacitance-to-Digital Converter with Temperature Sensor
+
+maintainers:
+  - Michael Hennerich <michael.hennerich@analog.com>
+
+description: |
+  AD7746 24-Bit Capacitance-to-Digital Converter with Temperature Sensor
+
+  Specifications about the part can be found at:
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7291.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7745
+      - adi,ad7746
+      - adi,ad7747
+
+  reg:
+    description: |
+      Physiscal address of the EXC set-up register.
+    maxItems: 1
+
+  adi,excitation-voltage-level:
+    description: |
+      Select the reference excitation voltage level used by the device.
+      With VDD being the power supply voltage, valid values are:
+      0: +-VDD / 8
+      1: +-VDD / 4
+      2: +-VDD * 3 / 8
+      3: +-VDD / 2
+      If left empty option 3 is selected.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3]
+
+  adi,exca-output:
+    description: |
+      Sets the excitation output in the exca pin.
+      Valid values are:
+      0: Disables output in the EXCA pin.
+      1: Enables EXCA pin as the excitation output.
+      2: Enables EXCA pin as the inverted excitation output.
+      If left empty the output is disabled.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2]
+
+  adi,excb-output:
+    description: |
+      Analoguos to the adi,exca-output for the EXCB pin.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2]
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      ad7746: cdc@0 {
+        compatible = "adi,ad7746";
+        reg = <0>;
+        adi,excitation-voltage-level = <3>;
+        adi,exca-output = <0>;
+        adi,excb-output = <0>;
+      };
+    };
+...
-- 
2.31.1

Re: [PATCH 1/3] dt-bindings: staging: iio: cdc: ad7746: add binding documentation for AD7746

[Jonathan Cameron]

On Fri, 9 Apr 2021 15:50:10 -0300
Lucas Stankus <lucas.p.stankus@gmail.com> wrote:

> Add device tree binding documentation for AD7746 cdc in YAML format.
> 
> Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>

Hi Lucas,

Good to see progress on this one after all these years :)

I think we can do a bit better though by making the attributes
easy to comprehend without needing to refer to the documentation.
Always good to avoid magic numbers if we can.

Suggestions inline.

Jonathan

> ---
>  .../bindings/iio/cdc/adi,ad7746.yaml          | 79 +++++++++++++++++++
>  1 file changed, 79 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml b/Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml
> new file mode 100644
> index 000000000000..5de86f4374e1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml
> @@ -0,0 +1,79 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/cdc/adi,ad7746.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AD7746 24-Bit Capacitance-to-Digital Converter with Temperature Sensor
> +
> +maintainers:
> +  - Michael Hennerich <michael.hennerich@analog.com>
> +
> +description: |
> +  AD7746 24-Bit Capacitance-to-Digital Converter with Temperature Sensor
> +
> +  Specifications about the part can be found at:
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7291.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad7745
> +      - adi,ad7746
> +      - adi,ad7747
> +
> +  reg:
> +    description: |
> +      Physiscal address of the EXC set-up register.

reg in this case would be the i2c address.

> +    maxItems: 1
> +
> +  adi,excitation-voltage-level:

This isn't a level as such, it's a scale factor, or something like
that and the naming should reflect that + the values
should be real in some sense (multipliers so
perhaps something like adi,excitation-vdd-milicent ?
schema/property-units.yaml includes -percent but that doesn't
have enough precision.

enum [125, 250, 375, 500] 

> +    description: |
> +      Select the reference excitation voltage level used by the device.
> +      With VDD being the power supply voltage, valid values are:
> +      0: +-VDD / 8
> +      1: +-VDD / 4
> +      2: +-VDD * 3 / 8
> +      3: +-VDD / 2
> +      If left empty option 3 is selected.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1, 2, 3]
> +
> +  adi,exca-output:
> +    description: |
> +      Sets the excitation output in the exca pin.
> +      Valid values are:
> +      0: Disables output in the EXCA pin.
> +      1: Enables EXCA pin as the excitation output.
> +      2: Enables EXCA pin as the inverted excitation output.

Hmm. Various ways we could do this and avoid the need for
a enum representing several different things.  Perhaps

adi,exa-output-en
adi,exa-output-invert

(appropriate checks so we can only have invert of the channel
is enabled as otherwise it is less than meaningful)

> +      If left empty the output is disabled.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1, 2]
> +
> +  adi,excb-output:
> +    description: |
> +      Analoguos to the adi,exca-output for the EXCB pin.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1, 2]
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      ad7746: cdc@0 {
> +        compatible = "adi,ad7746";
> +        reg = <0>;

That's very unlikely as an i2c address.

> +        adi,excitation-voltage-level = <3>;
> +        adi,exca-output = <0>;
> +        adi,excb-output = <0>;
> +      };
> +    };
> +...

[PATCH 2/3] staging: iio: cdc: ad7746: use dt bindings to set the EXCx pins output

[Lucas Stankus]

Ditch platform_data fields in favor of device tree properties for
configuring EXCA and EXCB output.
This also removes the fields from the platform_data struct, since they're
not used anymore.

Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
---
 drivers/staging/iio/cdc/ad7746.c | 33 +++++++++++++++++---------------
 drivers/staging/iio/cdc/ad7746.h |  4 ----
 2 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index dfd71e99e872..63041b164dbe 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -677,8 +677,10 @@ static int ad7746_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	struct ad7746_platform_data *pdata = client->dev.platform_data;
+	struct device_node *np = client->dev.of_node;
 	struct ad7746_chip_info *chip;
 	struct iio_dev *indio_dev;
+	unsigned int exca_en, excb_en;
 	unsigned char regval = 0;
 	int ret = 0;
 
@@ -703,26 +705,27 @@ static int ad7746_probe(struct i2c_client *client,
 	indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	if (pdata) {
-		if (pdata->exca_en) {
-			if (pdata->exca_inv_en)
-				regval |= AD7746_EXCSETUP_NEXCA;
-			else
-				regval |= AD7746_EXCSETUP_EXCA;
-		}
+	ret = of_property_read_u32(np, "adi,exca-output", &exca_en);
+	if (!ret && exca_en) {
+		if (exca_en == 1)
+			regval |= AD7746_EXCSETUP_EXCA;
+		else
+			regval |= AD7746_EXCSETUP_NEXCA;
+	}
 
-		if (pdata->excb_en) {
-			if (pdata->excb_inv_en)
-				regval |= AD7746_EXCSETUP_NEXCB;
-			else
-				regval |= AD7746_EXCSETUP_EXCB;
-		}
+	ret = of_property_read_u32(np, "adi,excb-output", &excb_en);
+	if (!ret && excb_en) {
+		if (excb_en == 1)
+			regval |= AD7746_EXCSETUP_EXCB;
+		else
+			regval |= AD7746_EXCSETUP_NEXCB;
+	}
 
+	if (pdata) {
 		regval |= AD7746_EXCSETUP_EXCLVL(pdata->exclvl);
 	} else {
 		dev_warn(&client->dev, "No platform data? using default\n");
-		regval = AD7746_EXCSETUP_EXCA | AD7746_EXCSETUP_EXCB |
-			AD7746_EXCSETUP_EXCLVL(3);
+		regval = AD7746_EXCSETUP_EXCLVL(3);
 	}
 
 	ret = i2c_smbus_write_byte_data(chip->client,
diff --git a/drivers/staging/iio/cdc/ad7746.h b/drivers/staging/iio/cdc/ad7746.h
index 8bdbd732dbbd..6cae4ecf779e 100644
--- a/drivers/staging/iio/cdc/ad7746.h
+++ b/drivers/staging/iio/cdc/ad7746.h
@@ -19,10 +19,6 @@
 
 struct ad7746_platform_data {
 	unsigned char exclvl;	/*Excitation Voltage Level */
-	bool exca_en;		/* enables EXCA pin as the excitation output */
-	bool exca_inv_en;	/* enables /EXCA pin as the excitation output */
-	bool excb_en;		/* enables EXCB pin as the excitation output */
-	bool excb_inv_en;	/* enables /EXCB pin as the excitation output */
 };
 
 #endif /* IIO_CDC_AD7746_H_ */
-- 
2.31.1

Re: [PATCH 2/3] staging: iio: cdc: ad7746: use dt bindings to set the EXCx pins output

[Alexandru Ardelean]

On Fri, Apr 9, 2021 at 9:51 PM Lucas Stankus <lucas.p.stankus@gmail.com> wrote:
>
> Ditch platform_data fields in favor of device tree properties for
> configuring EXCA and EXCB output.
> This also removes the fields from the platform_data struct, since they're
> not used anymore.
>
> Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> ---
>  drivers/staging/iio/cdc/ad7746.c | 33 +++++++++++++++++---------------
>  drivers/staging/iio/cdc/ad7746.h |  4 ----
>  2 files changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index dfd71e99e872..63041b164dbe 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -677,8 +677,10 @@ static int ad7746_probe(struct i2c_client *client,
>                         const struct i2c_device_id *id)
>  {
>         struct ad7746_platform_data *pdata = client->dev.platform_data;
> +       struct device_node *np = client->dev.of_node;
>         struct ad7746_chip_info *chip;
>         struct iio_dev *indio_dev;
> +       unsigned int exca_en, excb_en;
>         unsigned char regval = 0;
>         int ret = 0;
>
> @@ -703,26 +705,27 @@ static int ad7746_probe(struct i2c_client *client,
>         indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
>         indio_dev->modes = INDIO_DIRECT_MODE;
>

[1]

> -       if (pdata) {
> -               if (pdata->exca_en) {
> -                       if (pdata->exca_inv_en)
> -                               regval |= AD7746_EXCSETUP_NEXCA;
> -                       else
> -                               regval |= AD7746_EXCSETUP_EXCA;
> -               }
> +       ret = of_property_read_u32(np, "adi,exca-output", &exca_en);

maybe a better idea would be to use:

device_property_read_u32(dev, .... )
where:
dev = client->dev.;

this would make the driver a bit more friendly with both OF and ACPI

> +       if (!ret && exca_en) {
> +               if (exca_en == 1)
> +                       regval |= AD7746_EXCSETUP_EXCA;
> +               else
> +                       regval |= AD7746_EXCSETUP_NEXCA;
> +       }
>
> -               if (pdata->excb_en) {
> -                       if (pdata->excb_inv_en)
> -                               regval |= AD7746_EXCSETUP_NEXCB;
> -                       else
> -                               regval |= AD7746_EXCSETUP_EXCB;
> -               }
> +       ret = of_property_read_u32(np, "adi,excb-output", &excb_en);
> +       if (!ret && excb_en) {
> +               if (excb_en == 1)
> +                       regval |= AD7746_EXCSETUP_EXCB;
> +               else
> +                       regval |= AD7746_EXCSETUP_NEXCB;
> +       }
>
> +       if (pdata) {
>                 regval |= AD7746_EXCSETUP_EXCLVL(pdata->exclvl);
>         } else {
>                 dev_warn(&client->dev, "No platform data? using default\n");
> -               regval = AD7746_EXCSETUP_EXCA | AD7746_EXCSETUP_EXCB |
> -                       AD7746_EXCSETUP_EXCLVL(3);

This logic is problematic now.
Because no matter what you're setting in the DT, it always gets
overridden here because there is no platform data.

Maybe a better idea is to do something like:
if (!pdata)
     regval = AD7746_EXCSETUP_EXCLVL(3);

but this should be placed somewhere around [1]


> +               regval = AD7746_EXCSETUP_EXCLVL(3);
>         }
>
>         ret = i2c_smbus_write_byte_data(chip->client,
> diff --git a/drivers/staging/iio/cdc/ad7746.h b/drivers/staging/iio/cdc/ad7746.h
> index 8bdbd732dbbd..6cae4ecf779e 100644
> --- a/drivers/staging/iio/cdc/ad7746.h
> +++ b/drivers/staging/iio/cdc/ad7746.h
> @@ -19,10 +19,6 @@
>
>  struct ad7746_platform_data {
>         unsigned char exclvl;   /*Excitation Voltage Level */
> -       bool exca_en;           /* enables EXCA pin as the excitation output */
> -       bool exca_inv_en;       /* enables /EXCA pin as the excitation output */
> -       bool excb_en;           /* enables EXCB pin as the excitation output */
> -       bool excb_inv_en;       /* enables /EXCB pin as the excitation output */
>  };
>
>  #endif /* IIO_CDC_AD7746_H_ */
> --
> 2.31.1
>

Re: [PATCH 2/3] staging: iio: cdc: ad7746: use dt bindings to set the EXCx pins output

[Alexandru Ardelean]

On Sat, Apr 10, 2021 at 7:12 PM Alexandru Ardelean
<ardeleanalex@gmail.com> wrote:
>
> On Fri, Apr 9, 2021 at 9:51 PM Lucas Stankus <lucas.p.stankus@gmail.com> wrote:
> >
> > Ditch platform_data fields in favor of device tree properties for
> > configuring EXCA and EXCB output.
> > This also removes the fields from the platform_data struct, since they're
> > not used anymore.
> >
> > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> > ---
> >  drivers/staging/iio/cdc/ad7746.c | 33 +++++++++++++++++---------------
> >  drivers/staging/iio/cdc/ad7746.h |  4 ----
> >  2 files changed, 18 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> > index dfd71e99e872..63041b164dbe 100644
> > --- a/drivers/staging/iio/cdc/ad7746.c
> > +++ b/drivers/staging/iio/cdc/ad7746.c
> > @@ -677,8 +677,10 @@ static int ad7746_probe(struct i2c_client *client,
> >                         const struct i2c_device_id *id)
> >  {
> >         struct ad7746_platform_data *pdata = client->dev.platform_data;
> > +       struct device_node *np = client->dev.of_node;
> >         struct ad7746_chip_info *chip;
> >         struct iio_dev *indio_dev;
> > +       unsigned int exca_en, excb_en;
> >         unsigned char regval = 0;
> >         int ret = 0;
> >
> > @@ -703,26 +705,27 @@ static int ad7746_probe(struct i2c_client *client,
> >         indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
> >         indio_dev->modes = INDIO_DIRECT_MODE;
> >
>
> [1]
>
> > -       if (pdata) {
> > -               if (pdata->exca_en) {
> > -                       if (pdata->exca_inv_en)
> > -                               regval |= AD7746_EXCSETUP_NEXCA;
> > -                       else
> > -                               regval |= AD7746_EXCSETUP_EXCA;
> > -               }
> > +       ret = of_property_read_u32(np, "adi,exca-output", &exca_en);
>
> maybe a better idea would be to use:
>
> device_property_read_u32(dev, .... )
> where:
> dev = client->dev.;
>
> this would make the driver a bit more friendly with both OF and ACPI
>
> > +       if (!ret && exca_en) {
> > +               if (exca_en == 1)
> > +                       regval |= AD7746_EXCSETUP_EXCA;
> > +               else
> > +                       regval |= AD7746_EXCSETUP_NEXCA;
> > +       }
> >
> > -               if (pdata->excb_en) {
> > -                       if (pdata->excb_inv_en)
> > -                               regval |= AD7746_EXCSETUP_NEXCB;
> > -                       else
> > -                               regval |= AD7746_EXCSETUP_EXCB;
> > -               }
> > +       ret = of_property_read_u32(np, "adi,excb-output", &excb_en);
> > +       if (!ret && excb_en) {
> > +               if (excb_en == 1)
> > +                       regval |= AD7746_EXCSETUP_EXCB;
> > +               else
> > +                       regval |= AD7746_EXCSETUP_NEXCB;
> > +       }
> >
> > +       if (pdata) {
> >                 regval |= AD7746_EXCSETUP_EXCLVL(pdata->exclvl);
> >         } else {
> >                 dev_warn(&client->dev, "No platform data? using default\n");
> > -               regval = AD7746_EXCSETUP_EXCA | AD7746_EXCSETUP_EXCB |
> > -                       AD7746_EXCSETUP_EXCLVL(3);
>
> This logic is problematic now.
> Because no matter what you're setting in the DT, it always gets
> overridden here because there is no platform data.
>
> Maybe a better idea is to do something like:
> if (!pdata)
>      regval = AD7746_EXCSETUP_EXCLVL(3);
>
> but this should be placed somewhere around [1]

[ i can see that this logic will get corrected in the next patch]
to add here a bit: the idea of a patch is that it should try to not
introduce any [even temporary] breakage, even when it's in a series;
if a driver was already broken, then it should get fixed via it's own patch;
but no patch should introduce any breakages [if possible]

>
>
> > +               regval = AD7746_EXCSETUP_EXCLVL(3);
> >         }
> >
> >         ret = i2c_smbus_write_byte_data(chip->client,
> > diff --git a/drivers/staging/iio/cdc/ad7746.h b/drivers/staging/iio/cdc/ad7746.h
> > index 8bdbd732dbbd..6cae4ecf779e 100644
> > --- a/drivers/staging/iio/cdc/ad7746.h
> > +++ b/drivers/staging/iio/cdc/ad7746.h
> > @@ -19,10 +19,6 @@
> >
> >  struct ad7746_platform_data {
> >         unsigned char exclvl;   /*Excitation Voltage Level */
> > -       bool exca_en;           /* enables EXCA pin as the excitation output */
> > -       bool exca_inv_en;       /* enables /EXCA pin as the excitation output */
> > -       bool excb_en;           /* enables EXCB pin as the excitation output */
> > -       bool excb_inv_en;       /* enables /EXCB pin as the excitation output */
> >  };
> >
> >  #endif /* IIO_CDC_AD7746_H_ */
> > --
> > 2.31.1
> >

Re: [PATCH 2/3] staging: iio: cdc: ad7746: use dt bindings to set the EXCx pins output

[Jonathan Cameron]

On Sat, 10 Apr 2021 19:15:39 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Sat, Apr 10, 2021 at 7:12 PM Alexandru Ardelean
> <ardeleanalex@gmail.com> wrote:
> >
> > On Fri, Apr 9, 2021 at 9:51 PM Lucas Stankus <lucas.p.stankus@gmail.com> wrote:  
> > >
> > > Ditch platform_data fields in favor of device tree properties for
> > > configuring EXCA and EXCB output.
> > > This also removes the fields from the platform_data struct, since they're
> > > not used anymore.
> > >
> > > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> > > ---
> > >  drivers/staging/iio/cdc/ad7746.c | 33 +++++++++++++++++---------------
> > >  drivers/staging/iio/cdc/ad7746.h |  4 ----
> > >  2 files changed, 18 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> > > index dfd71e99e872..63041b164dbe 100644
> > > --- a/drivers/staging/iio/cdc/ad7746.c
> > > +++ b/drivers/staging/iio/cdc/ad7746.c
> > > @@ -677,8 +677,10 @@ static int ad7746_probe(struct i2c_client *client,
> > >                         const struct i2c_device_id *id)
> > >  {
> > >         struct ad7746_platform_data *pdata = client->dev.platform_data;
> > > +       struct device_node *np = client->dev.of_node;
> > >         struct ad7746_chip_info *chip;
> > >         struct iio_dev *indio_dev;
> > > +       unsigned int exca_en, excb_en;
> > >         unsigned char regval = 0;
> > >         int ret = 0;
> > >
> > > @@ -703,26 +705,27 @@ static int ad7746_probe(struct i2c_client *client,
> > >         indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
> > >         indio_dev->modes = INDIO_DIRECT_MODE;
> > >  
> >
> > [1]
> >  
> > > -       if (pdata) {
> > > -               if (pdata->exca_en) {
> > > -                       if (pdata->exca_inv_en)
> > > -                               regval |= AD7746_EXCSETUP_NEXCA;
> > > -                       else
> > > -                               regval |= AD7746_EXCSETUP_EXCA;
> > > -               }
> > > +       ret = of_property_read_u32(np, "adi,exca-output", &exca_en);  
> >
> > maybe a better idea would be to use:
> >
> > device_property_read_u32(dev, .... )
> > where:
> > dev = client->dev.;
> >
> > this would make the driver a bit more friendly with both OF and ACPI
> >  
> > > +       if (!ret && exca_en) {
> > > +               if (exca_en == 1)
> > > +                       regval |= AD7746_EXCSETUP_EXCA;
> > > +               else
> > > +                       regval |= AD7746_EXCSETUP_NEXCA;
> > > +       }
> > >
> > > -               if (pdata->excb_en) {
> > > -                       if (pdata->excb_inv_en)
> > > -                               regval |= AD7746_EXCSETUP_NEXCB;
> > > -                       else
> > > -                               regval |= AD7746_EXCSETUP_EXCB;
> > > -               }
> > > +       ret = of_property_read_u32(np, "adi,excb-output", &excb_en);
> > > +       if (!ret && excb_en) {
> > > +               if (excb_en == 1)
> > > +                       regval |= AD7746_EXCSETUP_EXCB;
> > > +               else
> > > +                       regval |= AD7746_EXCSETUP_NEXCB;
> > > +       }
> > >
> > > +       if (pdata) {
> > >                 regval |= AD7746_EXCSETUP_EXCLVL(pdata->exclvl);
> > >         } else {
> > >                 dev_warn(&client->dev, "No platform data? using default\n");
> > > -               regval = AD7746_EXCSETUP_EXCA | AD7746_EXCSETUP_EXCB |
> > > -                       AD7746_EXCSETUP_EXCLVL(3);  
> >
> > This logic is problematic now.
> > Because no matter what you're setting in the DT, it always gets
> > overridden here because there is no platform data.
> >
> > Maybe a better idea is to do something like:
> > if (!pdata)
> >      regval = AD7746_EXCSETUP_EXCLVL(3);
> >
> > but this should be placed somewhere around [1]  
> 
> [ i can see that this logic will get corrected in the next patch]
> to add here a bit: the idea of a patch is that it should try to not
> introduce any [even temporary] breakage, even when it's in a series;
> if a driver was already broken, then it should get fixed via it's own patch;
> but no patch should introduce any breakages [if possible]

The two patches are small enough I'd be fine with them being merged into one
that avoiding any special handling.  Just add a note to the patch description
to say that it was done in one patch for this reason.

Jonathan

> 
> >
> >  
> > > +               regval = AD7746_EXCSETUP_EXCLVL(3);
> > >         }
> > >
> > >         ret = i2c_smbus_write_byte_data(chip->client,
> > > diff --git a/drivers/staging/iio/cdc/ad7746.h b/drivers/staging/iio/cdc/ad7746.h
> > > index 8bdbd732dbbd..6cae4ecf779e 100644
> > > --- a/drivers/staging/iio/cdc/ad7746.h
> > > +++ b/drivers/staging/iio/cdc/ad7746.h
> > > @@ -19,10 +19,6 @@
> > >
> > >  struct ad7746_platform_data {
> > >         unsigned char exclvl;   /*Excitation Voltage Level */
> > > -       bool exca_en;           /* enables EXCA pin as the excitation output */
> > > -       bool exca_inv_en;       /* enables /EXCA pin as the excitation output */
> > > -       bool excb_en;           /* enables EXCB pin as the excitation output */
> > > -       bool excb_inv_en;       /* enables /EXCB pin as the excitation output */
> > >  };
> > >
> > >  #endif /* IIO_CDC_AD7746_H_ */
> > > --
> > > 2.31.1
> > >  

Re: [PATCH 2/3] staging: iio: cdc: ad7746: use dt bindings to set the EXCx pins output

[Jonathan Cameron]

On Sat, 10 Apr 2021 13:05:14 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Saturday, April 10, 2021, Andy Shevchenko <andy.shevchenko@gmail.com>
> wrote:
> 
> >
> >
> > On Friday, April 9, 2021, Lucas Stankus <lucas.p.stankus@gmail.com> wrote:
> >  
> >> Ditch platform_data fields in favor of device tree properties for
> >> configuring EXCA and EXCB output.
> >> This also removes the fields from the platform_data struct, since they're
> >> not used anymore.  
> >
> >
> > As far as I read the old code it’s possible to leave pins untouched, not
> > anymore the case after this patch. What datasheet tells about it? Please
> > elaborate in the commit message and add a Datasheet: tag as a reference.
> >
> >
> >  

Default is to have them disabled, so if you switch to separate -en
vs -invert lack of either will correspond to the power on default
and simplify things somewhat.

> 
> Okay, I see now. But can you simple use switch case or so, because
> currently code is not so understandable from the first glance?
> 
> 
> 
> >  
> >> Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> >> ---
> >>  drivers/staging/iio/cdc/ad7746.c | 33 +++++++++++++++++---------------
> >>  drivers/staging/iio/cdc/ad7746.h |  4 ----
> >>  2 files changed, 18 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/staging/iio/cdc/ad7746.c
> >> b/drivers/staging/iio/cdc/ad7746.c
> >> index dfd71e99e872..63041b164dbe 100644
> >> --- a/drivers/staging/iio/cdc/ad7746.c
> >> +++ b/drivers/staging/iio/cdc/ad7746.c
> >> @@ -677,8 +677,10 @@ static int ad7746_probe(struct i2c_client *client,
> >>                         const struct i2c_device_id *id)
> >>  {
> >>         struct ad7746_platform_data *pdata = client->dev.platform_data;
> >> +       struct device_node *np = client->dev.of_node;
> >>         struct ad7746_chip_info *chip;
> >>         struct iio_dev *indio_dev;
> >> +       unsigned int exca_en, excb_en;
> >>         unsigned char regval = 0;
> >>         int ret = 0;
> >>
> >> @@ -703,26 +705,27 @@ static int ad7746_probe(struct i2c_client *client,
> >>         indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
> >>         indio_dev->modes = INDIO_DIRECT_MODE;
> >>
> >> -       if (pdata) {
> >> -               if (pdata->exca_en) {
> >> -                       if (pdata->exca_inv_en)
> >> -                               regval |= AD7746_EXCSETUP_NEXCA;
> >> -                       else
> >> -                               regval |= AD7746_EXCSETUP_EXCA;
> >> -               }
> >> +       ret = of_property_read_u32(np, "adi,exca-output", &exca_en);
> >> +       if (!ret && exca_en) {
> >> +               if (exca_en == 1)
> >> +                       regval |= AD7746_EXCSETUP_EXCA;
> >> +               else
> >> +                       regval |= AD7746_EXCSETUP_NEXCA;
> >> +       }
> >>
> >> -               if (pdata->excb_en) {
> >> -                       if (pdata->excb_inv_en)
> >> -                               regval |= AD7746_EXCSETUP_NEXCB;
> >> -                       else
> >> -                               regval |= AD7746_EXCSETUP_EXCB;
> >> -               }
> >> +       ret = of_property_read_u32(np, "adi,excb-output", &excb_en);
> >> +       if (!ret && excb_en) {
> >> +               if (excb_en == 1)
> >> +                       regval |= AD7746_EXCSETUP_EXCB;
> >> +               else
> >> +                       regval |= AD7746_EXCSETUP_NEXCB;
> >> +       }
> >>
> >> +       if (pdata) {
> >>                 regval |= AD7746_EXCSETUP_EXCLVL(pdata->exclvl);
> >>         } else {
> >>                 dev_warn(&client->dev, "No platform data? using
> >> default\n");
> >> -               regval = AD7746_EXCSETUP_EXCA | AD7746_EXCSETUP_EXCB |
> >> -                       AD7746_EXCSETUP_EXCLVL(3);
> >> +               regval = AD7746_EXCSETUP_EXCLVL(3);
> >>         }
> >>
> >>         ret = i2c_smbus_write_byte_data(chip->client,
> >> diff --git a/drivers/staging/iio/cdc/ad7746.h
> >> b/drivers/staging/iio/cdc/ad7746.h
> >> index 8bdbd732dbbd..6cae4ecf779e 100644
> >> --- a/drivers/staging/iio/cdc/ad7746.h
> >> +++ b/drivers/staging/iio/cdc/ad7746.h
> >> @@ -19,10 +19,6 @@
> >>
> >>  struct ad7746_platform_data {
> >>         unsigned char exclvl;   /*Excitation Voltage Level */
> >> -       bool exca_en;           /* enables EXCA pin as the excitation
> >> output */
> >> -       bool exca_inv_en;       /* enables /EXCA pin as the excitation
> >> output */
> >> -       bool excb_en;           /* enables EXCB pin as the excitation
> >> output */
> >> -       bool excb_inv_en;       /* enables /EXCB pin as the excitation
> >> output */
> >>  };
> >>
> >>  #endif /* IIO_CDC_AD7746_H_ */
> >> --
> >> 2.31.1
> >>
> >>  
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
> >  
> 

[PATCH 3/3] staging: iio: cdc: ad7746: use dt binding to set the excitation level

[Lucas Stankus]

Set device excitation level using properties from device tree binding
instead of using platform_data.
As this replaces the last instance where the platform_data struct was
used, remove ad7746.h header file since it's no longer needed.

Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
---
 drivers/staging/iio/cdc/ad7746.c | 16 ++++++----------
 drivers/staging/iio/cdc/ad7746.h | 24 ------------------------
 2 files changed, 6 insertions(+), 34 deletions(-)
 delete mode 100644 drivers/staging/iio/cdc/ad7746.h

diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index 63041b164dbe..3c75d147c3dd 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -18,8 +18,6 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
-#include "ad7746.h"
-
 /*
  * AD7746 Register Definition
  */
@@ -676,11 +674,10 @@ static const struct iio_info ad7746_info = {
 static int ad7746_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
-	struct ad7746_platform_data *pdata = client->dev.platform_data;
 	struct device_node *np = client->dev.of_node;
 	struct ad7746_chip_info *chip;
 	struct iio_dev *indio_dev;
-	unsigned int exca_en, excb_en;
+	unsigned int exca_en, excb_en, exclvl;
 	unsigned char regval = 0;
 	int ret = 0;
 
@@ -721,12 +718,11 @@ static int ad7746_probe(struct i2c_client *client,
 			regval |= AD7746_EXCSETUP_NEXCB;
 	}
 
-	if (pdata) {
-		regval |= AD7746_EXCSETUP_EXCLVL(pdata->exclvl);
-	} else {
-		dev_warn(&client->dev, "No platform data? using default\n");
-		regval = AD7746_EXCSETUP_EXCLVL(3);
-	}
+	ret = of_property_read_u32(np, "adi,excitation-voltage-level", &exclvl);
+	if (!ret)
+		regval |= AD7746_EXCSETUP_EXCLVL(exclvl);
+	else
+		regval |= AD7746_EXCSETUP_EXCLVL(3);
 
 	ret = i2c_smbus_write_byte_data(chip->client,
 					AD7746_REG_EXC_SETUP, regval);
diff --git a/drivers/staging/iio/cdc/ad7746.h b/drivers/staging/iio/cdc/ad7746.h
deleted file mode 100644
index 6cae4ecf779e..000000000000
--- a/drivers/staging/iio/cdc/ad7746.h
+++ /dev/null
@@ -1,24 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * AD7746 capacitive sensor driver supporting AD7745, AD7746 and AD7747
- *
- * Copyright 2011 Analog Devices Inc.
- */
-
-#ifndef IIO_CDC_AD7746_H_
-#define IIO_CDC_AD7746_H_
-
-/*
- * TODO: struct ad7746_platform_data needs to go into include/linux/iio
- */
-
-#define AD7466_EXCLVL_0		0 /* +-VDD/8 */
-#define AD7466_EXCLVL_1		1 /* +-VDD/4 */
-#define AD7466_EXCLVL_2		2 /* +-VDD * 3/8 */
-#define AD7466_EXCLVL_3		3 /* +-VDD/2 */
-
-struct ad7746_platform_data {
-	unsigned char exclvl;	/*Excitation Voltage Level */
-};
-
-#endif /* IIO_CDC_AD7746_H_ */
-- 
2.31.1

Re: [PATCH 0/3] staging: iio: cdc: ad7746: remove platform_data in favor of device tree bindings

[Alexandru Ardelean]

On Fri, Apr 9, 2021 at 9:50 PM Lucas Stankus <lucas.p.stankus@gmail.com> wrote:
>
> This patch series aims to replace the platform_struct for the ad7746 driver
> in favor of device tree bindings, creating the dt-bindings documentation in
> the process.
>
> Since the header file was only used to define the struct and the excitation
> level values, it was possible to remove the file entirely.

From my side: I need to get better at understanding IIO and how to
place some logic of devices into IIO,
I don't know if there is a better approach at converting the current
platform_data into DT/OF.
Maybe Jonathan [or someone else] has some better ideas.
Otherwise the overall approach looks fine from my side.

>
> Lucas Stankus (3):
>   dt-bindings: staging: iio: cdc: ad7746: add binding documentation for
>     AD7746
>   staging: iio: cdc: ad7746: use dt bindings to set the EXCx pins output
>   staging: iio: cdc: ad7746: use dt binding to set the excitation level
>
>  .../bindings/iio/cdc/adi,ad7746.yaml          | 79 +++++++++++++++++++
>  drivers/staging/iio/cdc/ad7746.c              | 43 +++++-----
>  drivers/staging/iio/cdc/ad7746.h              | 28 -------
>  3 files changed, 100 insertions(+), 50 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml
>  delete mode 100644 drivers/staging/iio/cdc/ad7746.h
>
> --
> 2.31.1
>