[PATCH v2 0/2] staging: iio: cdc: ad7746: add dt properties for capacitive channel setup

[PATCH v2 0/2] staging: iio: cdc: ad7746: add dt properties for capacitive channel setup

[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.

Changelog v1 -> v2:
- Correctly set I2C device address on the device tree documentation.
- Change EXCA and EXCB dt properties from u32 enum to boolean.
- Update capacitive excitation dt property to enumerate possible per mille
  values instead of using indexed voltage levels.
- Merge last two patches to avoid special handling of remaining
  platform_data properties.

Lucas Stankus (2):
  dt-bindings: staging: iio: cdc: ad7746: add binding documentation for
    AD7746
  staging: iio: cdc: ad7746: use dt for capacitive channel setup.

 .../bindings/iio/cdc/adi,ad7746.yaml          | 73 +++++++++++++++++++
 drivers/staging/iio/cdc/ad7746.c              | 42 +++++------
 drivers/staging/iio/cdc/ad7746.h              | 28 -------
 3 files changed, 92 insertions(+), 51 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 v2 1/2] 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>
---

A minor note about the adi,excitation-vdd-permille property. Jonathan
suggested the name to be adi,excitation-vdd-milicent, but I was unsure of
the milicent naming. With a quick search I found out that the common way to
call a thousandth is 'per mille'[1], but I didn't find any use of it in the
kernel documentation. Any thoughts about it?

[1] https://en.wikipedia.org/wiki/Per_mille

 .../bindings/iio/cdc/adi,ad7746.yaml          | 73 +++++++++++++++++++
 1 file changed, 73 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..a2a7eee674ba
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml
@@ -0,0 +1,73 @@
+# 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:
+    maxItems: 1
+
+  adi,excitation-vdd-permille:
+    description: |
+      Set VDD per mille to be used as the excitation voltage.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [125, 250, 375, 500]
+
+  adi,exca-output-en:
+    description: Enables the EXCA pin as the excitation output.
+    type: boolean
+
+  adi,exca-output-invert:
+    description: Inverts the excitation output in the EXCA pin.
+    type: boolean
+
+  adi,excb-output-en:
+    description: Enables the EXCB pin as the excitation output.
+    type: boolean
+
+  adi,excb-output-invert:
+    description: Inverts the excitation output in the EXCB pin.
+    type: boolean
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      ad7746: cdc@48 {
+        compatible = "adi,ad7746";
+        reg = <0x48>;
+        adi,excitation-vdd-permille = <125>;
+
+        adi,exca-output-en;
+        adi,exca-output-invert;
+        adi,excb-output-en;
+        adi,excb-output-invert;
+      };
+    };
+...
-- 
2.31.1

[PATCH v2 2/2] staging: iio: cdc: ad7746: use dt for capacitive channel setup.

[Lucas Stankus]

Ditch platform_data fields in favor of device tree properties for
configuring EXCA and EXCB output pins and setting the capacitive channel
excitation level.

As this covers all current use cases for the platform_data struct, remove
ad7746.h header file since it's no longer needed.

Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
---

Changes in v2:
Previously this change set was composed of two patches, but it lead to
regressions between the two. Merging them avoids the unwanted behaviour
without having to deal with the special cases.

 drivers/staging/iio/cdc/ad7746.c | 42 +++++++++++++++-----------------
 drivers/staging/iio/cdc/ad7746.h | 28 ---------------------
 2 files changed, 19 insertions(+), 51 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 dfd71e99e872..c0e84c1cf4f1 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
  */
@@ -66,7 +64,7 @@
 #define AD7746_EXCSETUP_NEXCB		BIT(4)
 #define AD7746_EXCSETUP_EXCA		BIT(3)
 #define AD7746_EXCSETUP_NEXCA		BIT(2)
-#define AD7746_EXCSETUP_EXCLVL(x)	(((x) & 0x3) << 0)
+#define AD7746_EXCSETUP_EXCLVL(x)	(((x) >> 7) & 0x3)
 
 /* Config Register Bit Designations (AD7746_REG_CFG) */
 #define AD7746_CONF_VTFS_SHIFT		6
@@ -676,10 +674,11 @@ 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 *dev = &client->dev;
 	struct ad7746_chip_info *chip;
 	struct iio_dev *indio_dev;
 	unsigned char regval = 0;
+	unsigned int vdd_permille;
 	int ret = 0;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
@@ -703,28 +702,25 @@ 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;
-		}
-
-		if (pdata->excb_en) {
-			if (pdata->excb_inv_en)
-				regval |= AD7746_EXCSETUP_NEXCB;
-			else
-				regval |= AD7746_EXCSETUP_EXCB;
-		}
+	if (device_property_read_bool(dev, "adi,exca-output-en")) {
+		if (device_property_read_bool(dev, "adi,exca-output-invert"))
+			regval |= AD7746_EXCSETUP_NEXCA;
+		else
+			regval |= AD7746_EXCSETUP_EXCA;
+	}
 
-		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);
+	if (device_property_read_bool(dev, "adi,excb-output-en")) {
+		if (device_property_read_bool(dev, "adi,excb-output-invert"))
+			regval |= AD7746_EXCSETUP_NEXCB;
+		else
+			regval |= AD7746_EXCSETUP_EXCB;
 	}
 
+	ret = device_property_read_u32(dev, "adi,excitation-vdd-permille",
+				       &vdd_permille);
+	if (!ret)
+		regval |= AD7746_EXCSETUP_EXCLVL(vdd_permille);
+
 	ret = i2c_smbus_write_byte_data(chip->client,
 					AD7746_REG_EXC_SETUP, regval);
 	if (ret < 0)
diff --git a/drivers/staging/iio/cdc/ad7746.h b/drivers/staging/iio/cdc/ad7746.h
deleted file mode 100644
index 8bdbd732dbbd..000000000000
--- a/drivers/staging/iio/cdc/ad7746.h
+++ /dev/null
@@ -1,28 +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 */
-	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 v2 1/2] dt-bindings: staging: iio: cdc: ad7746: add binding documentation for AD7746

[Rob Herring]

On Sun, Apr 18, 2021 at 07:49:51PM -0300, Lucas Stankus wrote:
> Add device tree binding documentation for AD7746 cdc in YAML format.
> 
> Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> ---
> 
> A minor note about the adi,excitation-vdd-permille property. Jonathan
> suggested the name to be adi,excitation-vdd-milicent, but I was unsure of
> the milicent naming. With a quick search I found out that the common way to
> call a thousandth is 'per mille'[1], but I didn't find any use of it in the
> kernel documentation. Any thoughts about it?

Seems okay to me.

> [1] https://en.wikipedia.org/wiki/Per_mille
> 
>  .../bindings/iio/cdc/adi,ad7746.yaml          | 73 +++++++++++++++++++
>  1 file changed, 73 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..a2a7eee674ba
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml
> @@ -0,0 +1,73 @@
> +# 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:
> +    maxItems: 1
> +
> +  adi,excitation-vdd-permille:
> +    description: |
> +      Set VDD per mille to be used as the excitation voltage.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [125, 250, 375, 500]
> +
> +  adi,exca-output-en:
> +    description: Enables the EXCA pin as the excitation output.
> +    type: boolean
> +
> +  adi,exca-output-invert:
> +    description: Inverts the excitation output in the EXCA pin.
> +    type: boolean

'invert' assumes I know what the non-inverted signal is. Sometimes that 
makes sense, but if you can define in terms of the inverse that would be 
better. For example, for a normally active low signal, name the property 
'foo-active-high'.

> +
> +  adi,excb-output-en:
> +    description: Enables the EXCB pin as the excitation output.
> +    type: boolean
> +
> +  adi,excb-output-invert:
> +    description: Inverts the excitation output in the EXCB pin.
> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      ad7746: cdc@48 {
> +        compatible = "adi,ad7746";
> +        reg = <0x48>;
> +        adi,excitation-vdd-permille = <125>;
> +
> +        adi,exca-output-en;
> +        adi,exca-output-invert;
> +        adi,excb-output-en;
> +        adi,excb-output-invert;
> +      };
> +    };
> +...
> -- 
> 2.31.1
> 

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

[Lucas Stankus]

On Tue, Apr 20, 2021 at 4:37 PM Rob Herring <robh@kernel.org> wrote:
>
> On Sun, Apr 18, 2021 at 07:49:51PM -0300, Lucas Stankus wrote:
> > Add device tree binding documentation for AD7746 cdc in YAML format.
> >
> > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> > ---
> >
> > A minor note about the adi,excitation-vdd-permille property. Jonathan
> > suggested the name to be adi,excitation-vdd-milicent, but I was unsure of
> > the milicent naming. With a quick search I found out that the common way to
> > call a thousandth is 'per mille'[1], but I didn't find any use of it in the
> > kernel documentation. Any thoughts about it?
>
> Seems okay to me.
>
> > [1] https://en.wikipedia.org/wiki/Per_mille
> >
> >  .../bindings/iio/cdc/adi,ad7746.yaml          | 73 +++++++++++++++++++
> >  1 file changed, 73 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..a2a7eee674ba
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml
> > @@ -0,0 +1,73 @@
> > +# 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:
> > +    maxItems: 1
> > +
> > +  adi,excitation-vdd-permille:
> > +    description: |
> > +      Set VDD per mille to be used as the excitation voltage.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [125, 250, 375, 500]
> > +
> > +  adi,exca-output-en:
> > +    description: Enables the EXCA pin as the excitation output.
> > +    type: boolean
> > +
> > +  adi,exca-output-invert:
> > +    description: Inverts the excitation output in the EXCA pin.
> > +    type: boolean
>
> 'invert' assumes I know what the non-inverted signal is. Sometimes that
> makes sense, but if you can define in terms of the inverse that would be
> better. For example, for a normally active low signal, name the property
> 'foo-active-high'.

Thanks for the feedback!
From reading the data sheet I couldn't quite grasp what would be an inverted
excitation output, but I don't have a lot of experience with CDCs. Maybe someone
with more experience could help out with a better suggestion.

>
> > +
> > +  adi,excb-output-en:
> > +    description: Enables the EXCB pin as the excitation output.
> > +    type: boolean
> > +
> > +  adi,excb-output-invert:
> > +    description: Inverts the excitation output in the EXCB pin.
> > +    type: boolean
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      ad7746: cdc@48 {
> > +        compatible = "adi,ad7746";
> > +        reg = <0x48>;
> > +        adi,excitation-vdd-permille = <125>;
> > +
> > +        adi,exca-output-en;
> > +        adi,exca-output-invert;
> > +        adi,excb-output-en;
> > +        adi,excb-output-invert;
> > +      };
> > +    };
> > +...
> > --
> > 2.31.1
> >

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

[Lucas Stankus]

On Tue, Apr 20, 2021 at 4:37 PM Rob Herring <robh@kernel.org> wrote:
>
> On Sun, Apr 18, 2021 at 07:49:51PM -0300, Lucas Stankus wrote:
> > Add device tree binding documentation for AD7746 cdc in YAML format.
> >
> > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> > ---
> >
> > A minor note about the adi,excitation-vdd-permille property. Jonathan
> > suggested the name to be adi,excitation-vdd-milicent, but I was unsure of
> > the milicent naming. With a quick search I found out that the common way to
> > call a thousandth is 'per mille'[1], but I didn't find any use of it in the
> > kernel documentation. Any thoughts about it?
>
> Seems okay to me.
>
> > [1] https://en.wikipedia.org/wiki/Per_mille
> >
> >  .../bindings/iio/cdc/adi,ad7746.yaml          | 73 +++++++++++++++++++
> >  1 file changed, 73 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..a2a7eee674ba
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml
> > @@ -0,0 +1,73 @@
> > +# 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:
> > +    maxItems: 1
> > +
> > +  adi,excitation-vdd-permille:
> > +    description: |
> > +      Set VDD per mille to be used as the excitation voltage.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [125, 250, 375, 500]
> > +
> > +  adi,exca-output-en:
> > +    description: Enables the EXCA pin as the excitation output.
> > +    type: boolean
> > +
> > +  adi,exca-output-invert:
> > +    description: Inverts the excitation output in the EXCA pin.
> > +    type: boolean
>
> 'invert' assumes I know what the non-inverted signal is. Sometimes that
> makes sense, but if you can define in terms of the inverse that would be
> better. For example, for a normally active low signal, name the property
> 'foo-active-high'.

Thanks for the feedback!
From reading the data sheet I couldn't quite grasp what would be an inverted
excitation output, but I don't have a lot of experience with CDCs. Maybe someone
with more experience could help out with a better suggestion.

>
> > +
> > +  adi,excb-output-en:
> > +    description: Enables the EXCB pin as the excitation output.
> > +    type: boolean
> > +
> > +  adi,excb-output-invert:
> > +    description: Inverts the excitation output in the EXCB pin.
> > +    type: boolean
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      ad7746: cdc@48 {
> > +        compatible = "adi,ad7746";
> > +        reg = <0x48>;
> > +        adi,excitation-vdd-permille = <125>;
> > +
> > +        adi,exca-output-en;
> > +        adi,exca-output-invert;
> > +        adi,excb-output-en;
> > +        adi,excb-output-invert;
> > +      };
> > +    };
> > +...
> > --
> > 2.31.1
> >

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

[Lars-Peter Clausen]

On 4/22/21 9:16 PM, Lucas Stankus wrote:
> On Tue, Apr 20, 2021 at 4:37 PM Rob Herring <robh@kernel.org> wrote:
>> On Sun, Apr 18, 2021 at 07:49:51PM -0300, Lucas Stankus wrote:
>>> Add device tree binding documentation for AD7746 cdc in YAML format.
>>>
>>> Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
>>> ---
>>>
>>> A minor note about the adi,excitation-vdd-permille property. Jonathan
>>> suggested the name to be adi,excitation-vdd-milicent, but I was unsure of
>>> the milicent naming. With a quick search I found out that the common way to
>>> call a thousandth is 'per mille'[1], but I didn't find any use of it in the
>>> kernel documentation. Any thoughts about it?
>> Seems okay to me.
>>
>>> [1] https://en.wikipedia.org/wiki/Per_mille
>>>
>>>   .../bindings/iio/cdc/adi,ad7746.yaml          | 73 +++++++++++++++++++
>>>   1 file changed, 73 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..a2a7eee674ba
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml
>>> @@ -0,0 +1,73 @@
>>> +# 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:
>>> +    maxItems: 1
>>> +
>>> +  adi,excitation-vdd-permille:
>>> +    description: |
>>> +      Set VDD per mille to be used as the excitation voltage.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [125, 250, 375, 500]
>>> +
>>> +  adi,exca-output-en:
>>> +    description: Enables the EXCA pin as the excitation output.
>>> +    type: boolean
>>> +
>>> +  adi,exca-output-invert:
>>> +    description: Inverts the excitation output in the EXCA pin.
>>> +    type: boolean
>> 'invert' assumes I know what the non-inverted signal is. Sometimes that
>> makes sense, but if you can define in terms of the inverse that would be
>> better. For example, for a normally active low signal, name the property
>> 'foo-active-high'.
> Thanks for the feedback!
>  From reading the data sheet I couldn't quite grasp what would be an inverted
> excitation output, but I don't have a lot of experience with CDCs. Maybe someone
> with more experience could help out with a better suggestion.

This is the only thing I could find 
https://www.analog.com/media/en/technical-documentation/application-notes/AN-1585.pdf.

The datasheet also says that only at most one of them should be 
inverted. So maybe only a single property which selects which of the two 
is inverted.

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

[Jonathan Cameron]

On Tue, 20 Apr 2021 14:37:46 -0500
Rob Herring <robh@kernel.org> wrote:

> On Sun, Apr 18, 2021 at 07:49:51PM -0300, Lucas Stankus wrote:
> > Add device tree binding documentation for AD7746 cdc in YAML format.
> > 
> > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> > ---
> > 
> > A minor note about the adi,excitation-vdd-permille property. Jonathan
> > suggested the name to be adi,excitation-vdd-milicent, but I was unsure of
> > the milicent naming. With a quick search I found out that the common way to
> > call a thousandth is 'per mille'[1], but I didn't find any use of it in the
> > kernel documentation. Any thoughts about it?  
> 
> Seems okay to me.

Sorry about that.  I realised later in the day I was talking garbage, but
forgot to send a reply to say that when I was next on my computer :(
permille was what I mean.

> 
> > [1] https://en.wikipedia.org/wiki/Per_mille
> > 
> >  .../bindings/iio/cdc/adi,ad7746.yaml          | 73 +++++++++++++++++++
> >  1 file changed, 73 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..a2a7eee674ba
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml
> > @@ -0,0 +1,73 @@
> > +# 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:
> > +    maxItems: 1
> > +
> > +  adi,excitation-vdd-permille:
> > +    description: |
> > +      Set VDD per mille to be used as the excitation voltage.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [125, 250, 375, 500]
> > +
> > +  adi,exca-output-en:
> > +    description: Enables the EXCA pin as the excitation output.
> > +    type: boolean
> > +
> > +  adi,exca-output-invert:
> > +    description: Inverts the excitation output in the EXCA pin.
> > +    type: boolean  
> 
> 'invert' assumes I know what the non-inverted signal is. Sometimes that 
> makes sense, but if you can define in terms of the inverse that would be 
> better. For example, for a normally active low signal, name the property 
> 'foo-active-high'.
> 
> > +
> > +  adi,excb-output-en:
> > +    description: Enables the EXCB pin as the excitation output.
> > +    type: boolean
> > +
> > +  adi,excb-output-invert:
> > +    description: Inverts the excitation output in the EXCB pin.
> > +    type: boolean
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      ad7746: cdc@48 {
> > +        compatible = "adi,ad7746";
> > +        reg = <0x48>;
> > +        adi,excitation-vdd-permille = <125>;
> > +
> > +        adi,exca-output-en;
> > +        adi,exca-output-invert;
> > +        adi,excb-output-en;
> > +        adi,excb-output-invert;
> > +      };
> > +    };
> > +...
> > -- 
> > 2.31.1
> >   

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

[Jonathan Cameron]

On Thu, 22 Apr 2021 22:56:38 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 4/22/21 9:16 PM, Lucas Stankus wrote:
> > On Tue, Apr 20, 2021 at 4:37 PM Rob Herring <robh@kernel.org> wrote:  
> >> On Sun, Apr 18, 2021 at 07:49:51PM -0300, Lucas Stankus wrote:  
> >>> Add device tree binding documentation for AD7746 cdc in YAML format.
> >>>
> >>> Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> >>> ---
> >>>
> >>> A minor note about the adi,excitation-vdd-permille property. Jonathan
> >>> suggested the name to be adi,excitation-vdd-milicent, but I was unsure of
> >>> the milicent naming. With a quick search I found out that the common way to
> >>> call a thousandth is 'per mille'[1], but I didn't find any use of it in the
> >>> kernel documentation. Any thoughts about it?  
> >> Seems okay to me.
> >>  
> >>> [1] https://en.wikipedia.org/wiki/Per_mille
> >>>
> >>>   .../bindings/iio/cdc/adi,ad7746.yaml          | 73 +++++++++++++++++++
> >>>   1 file changed, 73 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..a2a7eee674ba
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml
> >>> @@ -0,0 +1,73 @@
> >>> +# 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:
> >>> +    maxItems: 1
> >>> +
> >>> +  adi,excitation-vdd-permille:
> >>> +    description: |
> >>> +      Set VDD per mille to be used as the excitation voltage.
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    enum: [125, 250, 375, 500]
> >>> +
> >>> +  adi,exca-output-en:
> >>> +    description: Enables the EXCA pin as the excitation output.
> >>> +    type: boolean
> >>> +
> >>> +  adi,exca-output-invert:
> >>> +    description: Inverts the excitation output in the EXCA pin.
> >>> +    type: boolean  
> >> 'invert' assumes I know what the non-inverted signal is. Sometimes that
> >> makes sense, but if you can define in terms of the inverse that would be
> >> better. For example, for a normally active low signal, name the property
> >> 'foo-active-high'.  
> > Thanks for the feedback!
> >  From reading the data sheet I couldn't quite grasp what would be an inverted
> > excitation output, but I don't have a lot of experience with CDCs. Maybe someone
> > with more experience could help out with a better suggestion.  
> 
> This is the only thing I could find 
> https://www.analog.com/media/en/technical-documentation/application-notes/AN-1585.pdf.
> 
> The datasheet also says that only at most one of them should be 
> inverted. So maybe only a single property which selects which of the two 
> is inverted.
> 
> 

Nice. I was wondering what on earth that was for as well.  That note
makes it a little clearer!

I'd keep them as separate properties because there 'might' be another
circuit where inverting them both makes sense (though I have no idea
what it would look like). A reference to that note though to explain
why you might do this would be great to have the dt property description.

If anyone ever actually does do this, we'll need to add a description
of the analog front end somehow to actually support reporting in
meaningful units.

Let's not tackle that problem today though and it might be we'd actually
just use a generic AFE IIO consumer to do the scaling rather than needing
anything custom.

Jonathan

Re: [PATCH v2 2/2] staging: iio: cdc: ad7746: use dt for capacitive channel setup.

[Jonathan Cameron]

On Sun, 18 Apr 2021 19:50:10 -0300
Lucas Stankus <lucas.p.stankus@gmail.com> wrote:

> Ditch platform_data fields in favor of device tree properties for
> configuring EXCA and EXCB output pins and setting the capacitive channel
> excitation level.
> 
> As this covers all current use cases for the platform_data struct, remove
> ad7746.h header file since it's no longer needed.
> 
> Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
Hi Lucas,

Just one comment which is that the permille to register value
is a bit opaque so I think I'd prefer a simple lookup table even though it
is a bit more code.

Otherwise looks good to me.

Thanks,

Jonathan
> ---
> 
> Changes in v2:
> Previously this change set was composed of two patches, but it lead to
> regressions between the two. Merging them avoids the unwanted behaviour
> without having to deal with the special cases.
> 
>  drivers/staging/iio/cdc/ad7746.c | 42 +++++++++++++++-----------------
>  drivers/staging/iio/cdc/ad7746.h | 28 ---------------------
>  2 files changed, 19 insertions(+), 51 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 dfd71e99e872..c0e84c1cf4f1 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
>   */
> @@ -66,7 +64,7 @@
>  #define AD7746_EXCSETUP_NEXCB		BIT(4)
>  #define AD7746_EXCSETUP_EXCA		BIT(3)
>  #define AD7746_EXCSETUP_NEXCA		BIT(2)
> -#define AD7746_EXCSETUP_EXCLVL(x)	(((x) & 0x3) << 0)
> +#define AD7746_EXCSETUP_EXCLVL(x)	(((x) >> 7) & 0x3)

I'd like a comment on this maths, or just use a lookup table instead so
we don't have to think about it and also enforce exact matches.
The rounding this results in is a bit odd

>  
>  /* Config Register Bit Designations (AD7746_REG_CFG) */
>  #define AD7746_CONF_VTFS_SHIFT		6
> @@ -676,10 +674,11 @@ 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 *dev = &client->dev;
>  	struct ad7746_chip_info *chip;
>  	struct iio_dev *indio_dev;
>  	unsigned char regval = 0;
> +	unsigned int vdd_permille;
>  	int ret = 0;
>  
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> @@ -703,28 +702,25 @@ 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;
> -		}
> -
> -		if (pdata->excb_en) {
> -			if (pdata->excb_inv_en)
> -				regval |= AD7746_EXCSETUP_NEXCB;
> -			else
> -				regval |= AD7746_EXCSETUP_EXCB;
> -		}
> +	if (device_property_read_bool(dev, "adi,exca-output-en")) {
> +		if (device_property_read_bool(dev, "adi,exca-output-invert"))
> +			regval |= AD7746_EXCSETUP_NEXCA;
> +		else
> +			regval |= AD7746_EXCSETUP_EXCA;
> +	}
>  
> -		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);
> +	if (device_property_read_bool(dev, "adi,excb-output-en")) {
> +		if (device_property_read_bool(dev, "adi,excb-output-invert"))
> +			regval |= AD7746_EXCSETUP_NEXCB;
> +		else
> +			regval |= AD7746_EXCSETUP_EXCB;
>  	}
>  
> +	ret = device_property_read_u32(dev, "adi,excitation-vdd-permille",
> +				       &vdd_permille);
> +	if (!ret)
> +		regval |= AD7746_EXCSETUP_EXCLVL(vdd_permille);
> +
>  	ret = i2c_smbus_write_byte_data(chip->client,
>  					AD7746_REG_EXC_SETUP, regval);
>  	if (ret < 0)
> diff --git a/drivers/staging/iio/cdc/ad7746.h b/drivers/staging/iio/cdc/ad7746.h
> deleted file mode 100644
> index 8bdbd732dbbd..000000000000
> --- a/drivers/staging/iio/cdc/ad7746.h
> +++ /dev/null
> @@ -1,28 +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 */
> -	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_ */

Re: [PATCH v2 2/2] staging: iio: cdc: ad7746: use dt for capacitive channel setup.

[Lucas Stankus]

On Sat, Apr 24, 2021 at 9:10 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 18 Apr 2021 19:50:10 -0300
> Lucas Stankus <lucas.p.stankus@gmail.com> wrote:
>
> > Ditch platform_data fields in favor of device tree properties for
> > configuring EXCA and EXCB output pins and setting the capacitive channel
> > excitation level.
> >
> > As this covers all current use cases for the platform_data struct, remove
> > ad7746.h header file since it's no longer needed.
> >
> > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> Hi Lucas,
>
> Just one comment which is that the permille to register value
> is a bit opaque so I think I'd prefer a simple lookup table even though it
> is a bit more code.
>
> Otherwise looks good to me.
>
> Thanks,
>
> Jonathan

Thank you!

> > ---
> >
> > Changes in v2:
> > Previously this change set was composed of two patches, but it lead to
> > regressions between the two. Merging them avoids the unwanted behaviour
> > without having to deal with the special cases.
> >
> >  drivers/staging/iio/cdc/ad7746.c | 42 +++++++++++++++-----------------
> >  drivers/staging/iio/cdc/ad7746.h | 28 ---------------------
> >  2 files changed, 19 insertions(+), 51 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 dfd71e99e872..c0e84c1cf4f1 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
> >   */
> > @@ -66,7 +64,7 @@
> >  #define AD7746_EXCSETUP_NEXCB                BIT(4)
> >  #define AD7746_EXCSETUP_EXCA         BIT(3)
> >  #define AD7746_EXCSETUP_NEXCA                BIT(2)
> > -#define AD7746_EXCSETUP_EXCLVL(x)    (((x) & 0x3) << 0)
> > +#define AD7746_EXCSETUP_EXCLVL(x)    (((x) >> 7) & 0x3)
>
> I'd like a comment on this maths, or just use a lookup table instead so
> we don't have to think about it and also enforce exact matches.
> The rounding this results in is a bit odd

To be fair this macro is a bit hack-ish, the permille binding can be 125, 250,
500 or 750, so dividing it by 128 maps these values from 0 to 3, which are the
expected numbers for configuring the device.

I decided to use the bit shift because it was more concise than a lookup table,
but the latter might be more legible, I'll send a v3 with one instead.

>
> >
> >  /* Config Register Bit Designations (AD7746_REG_CFG) */
> >  #define AD7746_CONF_VTFS_SHIFT               6
> > @@ -676,10 +674,11 @@ 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 *dev = &client->dev;
> >       struct ad7746_chip_info *chip;
> >       struct iio_dev *indio_dev;
> >       unsigned char regval = 0;
> > +     unsigned int vdd_permille;
> >       int ret = 0;
> >
> >       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> > @@ -703,28 +702,25 @@ 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;
> > -             }
> > -
> > -             if (pdata->excb_en) {
> > -                     if (pdata->excb_inv_en)
> > -                             regval |= AD7746_EXCSETUP_NEXCB;
> > -                     else
> > -                             regval |= AD7746_EXCSETUP_EXCB;
> > -             }
> > +     if (device_property_read_bool(dev, "adi,exca-output-en")) {
> > +             if (device_property_read_bool(dev, "adi,exca-output-invert"))
> > +                     regval |= AD7746_EXCSETUP_NEXCA;
> > +             else
> > +                     regval |= AD7746_EXCSETUP_EXCA;
> > +     }
> >
> > -             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);
> > +     if (device_property_read_bool(dev, "adi,excb-output-en")) {
> > +             if (device_property_read_bool(dev, "adi,excb-output-invert"))
> > +                     regval |= AD7746_EXCSETUP_NEXCB;
> > +             else
> > +                     regval |= AD7746_EXCSETUP_EXCB;
> >       }
> >
> > +     ret = device_property_read_u32(dev, "adi,excitation-vdd-permille",
> > +                                    &vdd_permille);
> > +     if (!ret)
> > +             regval |= AD7746_EXCSETUP_EXCLVL(vdd_permille);
> > +
> >       ret = i2c_smbus_write_byte_data(chip->client,
> >                                       AD7746_REG_EXC_SETUP, regval);
> >       if (ret < 0)
> > diff --git a/drivers/staging/iio/cdc/ad7746.h b/drivers/staging/iio/cdc/ad7746.h
> > deleted file mode 100644
> > index 8bdbd732dbbd..000000000000
> > --- a/drivers/staging/iio/cdc/ad7746.h
> > +++ /dev/null
> > @@ -1,28 +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 */
> > -     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_ */
>

Re: [PATCH v2 2/2] staging: iio: cdc: ad7746: use dt for capacitive channel setup.

[Lucas Stankus]

On Sat, Apr 24, 2021 at 9:10 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 18 Apr 2021 19:50:10 -0300
> Lucas Stankus <lucas.p.stankus@gmail.com> wrote:
>
> > Ditch platform_data fields in favor of device tree properties for
> > configuring EXCA and EXCB output pins and setting the capacitive channel
> > excitation level.
> >
> > As this covers all current use cases for the platform_data struct, remove
> > ad7746.h header file since it's no longer needed.
> >
> > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> Hi Lucas,
>
> Just one comment which is that the permille to register value
> is a bit opaque so I think I'd prefer a simple lookup table even though it
> is a bit more code.
>
> Otherwise looks good to me.
>
> Thanks,
>
> Jonathan

Thank you!

> > ---
> >
> > Changes in v2:
> > Previously this change set was composed of two patches, but it lead to
> > regressions between the two. Merging them avoids the unwanted behaviour
> > without having to deal with the special cases.
> >
> >  drivers/staging/iio/cdc/ad7746.c | 42 +++++++++++++++-----------------
> >  drivers/staging/iio/cdc/ad7746.h | 28 ---------------------
> >  2 files changed, 19 insertions(+), 51 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 dfd71e99e872..c0e84c1cf4f1 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
> >   */
> > @@ -66,7 +64,7 @@
> >  #define AD7746_EXCSETUP_NEXCB                BIT(4)
> >  #define AD7746_EXCSETUP_EXCA         BIT(3)
> >  #define AD7746_EXCSETUP_NEXCA                BIT(2)
> > -#define AD7746_EXCSETUP_EXCLVL(x)    (((x) & 0x3) << 0)
> > +#define AD7746_EXCSETUP_EXCLVL(x)    (((x) >> 7) & 0x3)
>
> I'd like a comment on this maths, or just use a lookup table instead so
> we don't have to think about it and also enforce exact matches.
> The rounding this results in is a bit odd

To be fair this macro is a bit hack-ish, the permille binding can be 125, 250,
500 or 750, so dividing it by 128 maps these values from 0 to 3, which are the
expected numbers for configuring the device.

I decided to use the bit shift because it was more concise than a lookup table,
but the latter might be more legible, I'll send a v3 with one instead.

>
> >
> >  /* Config Register Bit Designations (AD7746_REG_CFG) */
> >  #define AD7746_CONF_VTFS_SHIFT               6
> > @@ -676,10 +674,11 @@ 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 *dev = &client->dev;
> >       struct ad7746_chip_info *chip;
> >       struct iio_dev *indio_dev;
> >       unsigned char regval = 0;
> > +     unsigned int vdd_permille;
> >       int ret = 0;
> >
> >       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> > @@ -703,28 +702,25 @@ 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;
> > -             }
> > -
> > -             if (pdata->excb_en) {
> > -                     if (pdata->excb_inv_en)
> > -                             regval |= AD7746_EXCSETUP_NEXCB;
> > -                     else
> > -                             regval |= AD7746_EXCSETUP_EXCB;
> > -             }
> > +     if (device_property_read_bool(dev, "adi,exca-output-en")) {
> > +             if (device_property_read_bool(dev, "adi,exca-output-invert"))
> > +                     regval |= AD7746_EXCSETUP_NEXCA;
> > +             else
> > +                     regval |= AD7746_EXCSETUP_EXCA;
> > +     }
> >
> > -             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);
> > +     if (device_property_read_bool(dev, "adi,excb-output-en")) {
> > +             if (device_property_read_bool(dev, "adi,excb-output-invert"))
> > +                     regval |= AD7746_EXCSETUP_NEXCB;
> > +             else
> > +                     regval |= AD7746_EXCSETUP_EXCB;
> >       }
> >
> > +     ret = device_property_read_u32(dev, "adi,excitation-vdd-permille",
> > +                                    &vdd_permille);
> > +     if (!ret)
> > +             regval |= AD7746_EXCSETUP_EXCLVL(vdd_permille);
> > +
> >       ret = i2c_smbus_write_byte_data(chip->client,
> >                                       AD7746_REG_EXC_SETUP, regval);
> >       if (ret < 0)
> > diff --git a/drivers/staging/iio/cdc/ad7746.h b/drivers/staging/iio/cdc/ad7746.h
> > deleted file mode 100644
> > index 8bdbd732dbbd..000000000000
> > --- a/drivers/staging/iio/cdc/ad7746.h
> > +++ /dev/null
> > @@ -1,28 +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 */
> > -     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_ */
>