[PATCH 21/24] dt-bindings:iio:cdc:adi,ad7150 binding doc

[PATCH 21/24] dt-bindings:iio:cdc:adi,ad7150 binding doc

[Jonathan Cameron]

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

Binding covering the ad7150, ad7151 and ad7156 capacitance to digital
convertors.  The only difference between these is how many channels they
have (1 or 2)

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Robh+dt@kernel.org
Cc: devicetree@vger.kernel.org
---
 .../bindings/iio/cdc/adi,ad7150.yaml          | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/cdc/adi,ad7150.yaml b/Documentation/devicetree/bindings/iio/cdc/adi,ad7150.yaml
new file mode 100644
index 000000000000..2155d3f5666c
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/cdc/adi,ad7150.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/cdc/adi,ad7150.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog device AD7150 and similar capacitance to digital convertors.
+
+maintainers:
+  - Jonathan Cameron <jic23@kernel.org>
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7150
+      - adi,ad7151
+      - adi,ad7156
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+  interrupts: true
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad7150
+              - adi,ad7156
+    then:
+      properties:
+        interrupts:
+          minItems: 2
+          maxItems: 2
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: adi,ad7151
+    then:
+      properties:
+        interrupts:
+          minItems: 1
+          maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        cdc@48 {
+            compatible = "adi,ad7150";
+            reg = <0x48>;
+            interrupts = <25 2>, <26 2>;
+            interrupt-parent = <&gpio>;
+        };
+    };
+...
-- 
2.30.0

Re: [PATCH 21/24] dt-bindings:iio:cdc:adi,ad7150 binding doc

[Lars-Peter Clausen]

On 2/7/21 4:46 PM, Jonathan Cameron wrote:
> +required:
> +  - compatible
> +  - reg

Is vdd-supply really optional the way it is implemented in the driver?

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        cdc@48 {
> +            compatible = "adi,ad7150";
> +            reg = <0x48>;
> +            interrupts = <25 2>, <26 2>;

I wonder if we should use the symbolic constants for the IRQ type to 
make the example more clear. E.g.

interrupts = <25 IRQ_TYPE_EDGE_FALLING>, ...

> +            interrupt-parent = <&gpio>;
> +        };
> +    };
> +...

Re: [PATCH 21/24] dt-bindings:iio:cdc:adi,ad7150 binding doc

[Jonathan Cameron]

On Sun, 7 Feb 2021 17:00:24 +0100
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 2/7/21 4:46 PM, Jonathan Cameron wrote:
> > +required:
> > +  - compatible
> > +  - reg  
> 
> Is vdd-supply really optional the way it is implemented in the driver?

Well sort of.  Obviously VDD isn't optional in the sense that the
device needs power, but it is in the binding because a stub regulator
should be fine.  For those regulator_enable() is a noop on assumption
they are already on.

> 
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        cdc@48 {
> > +            compatible = "adi,ad7150";
> > +            reg = <0x48>;
> > +            interrupts = <25 2>, <26 2>;  
> 
> I wonder if we should use the symbolic constants for the IRQ type to 
> make the example more clear. E.g.
> 
> interrupts = <25 IRQ_TYPE_EDGE_FALLING>, ...

Sure. I'll update in v2.

> 
> > +            interrupt-parent = <&gpio>;
> > +        };
> > +    };
> > +...  
> 
> 

RE: [PATCH 21/24] dt-bindings:iio:cdc:adi,ad7150 binding doc

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Lars-Peter Clausen [mailto:lars@metafoo.de]
> Sent: Monday, February 8, 2021 5:00 AM
> To: Jonathan Cameron <jic23@kernel.org>; linux-iio@vger.kernel.org
> Cc: Michael Hennerich <Michael.Hennerich@analog.com>; Song Bao Hua (Barry Song)
> <song.bao.hua@hisilicon.com>; robh+dt@kernel.org; Jonathan Cameron
> <jonathan.cameron@huawei.com>; devicetree@vger.kernel.org
> Subject: Re: [PATCH 21/24] dt-bindings:iio:cdc:adi,ad7150 binding doc
> 
> On 2/7/21 4:46 PM, Jonathan Cameron wrote:
> > +required:
> > +  - compatible
> > +  - reg
> 
> Is vdd-supply really optional the way it is implemented in the driver?
> 
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        cdc@48 {
> > +            compatible = "adi,ad7150";
> > +            reg = <0x48>;
> > +            interrupts = <25 2>, <26 2>;

One question, here we have two interrupts, but the driver is reading
one interrupt only, do we need to call
of_irq_get(dev, index)
or
of_irq_get_byname()?


> 
> I wonder if we should use the symbolic constants for the IRQ type to
> make the example more clear. E.g.
> 
> interrupts = <25 IRQ_TYPE_EDGE_FALLING>, ...
> 
> > +            interrupt-parent = <&gpio>;
> > +        };
> > +    };
> > +...
> 
Thanks
Barry

Re: [PATCH 21/24] dt-bindings:iio:cdc:adi,ad7150 binding doc

[Jonathan Cameron]

On Mon, 8 Feb 2021 08:12:21 +0000
"Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com> wrote:

> > -----Original Message-----
> > From: Lars-Peter Clausen [mailto:lars@metafoo.de]
> > Sent: Monday, February 8, 2021 5:00 AM
> > To: Jonathan Cameron <jic23@kernel.org>; linux-iio@vger.kernel.org
> > Cc: Michael Hennerich <Michael.Hennerich@analog.com>; Song Bao Hua (Barry Song)
> > <song.bao.hua@hisilicon.com>; robh+dt@kernel.org; Jonathan Cameron
> > <jonathan.cameron@huawei.com>; devicetree@vger.kernel.org
> > Subject: Re: [PATCH 21/24] dt-bindings:iio:cdc:adi,ad7150 binding doc
> > 
> > On 2/7/21 4:46 PM, Jonathan Cameron wrote:  
> > > +required:
> > > +  - compatible
> > > +  - reg  
> > 
> > Is vdd-supply really optional the way it is implemented in the driver?
> >   
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    i2c {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        cdc@48 {
> > > +            compatible = "adi,ad7150";
> > > +            reg = <0x48>;
> > > +            interrupts = <25 2>, <26 2>;  
> 
> One question, here we have two interrupts, but the driver is reading
> one interrupt only, do we need to call
> of_irq_get(dev, index)
> or
> of_irq_get_byname()?

Driver reads both now (again).  It wasn't the cleanest transition in this series.
I dropped the second irq in patch 5 and brought it back in patch 12.

Whilst we could in theory support only one interrupt, that would make the driver
annoyingly fiddly for something I suspect no one would actually do.
We'd need to support events only on one of the two channels to make it work.

So I vote we take lazy option of saying it's either no interrupts, or both of
them for the two channel devices.  Nothing stops us relaxing that in future
and then using get_by_name.  If we do that and also require names are in order
INT1 INT2, INT1, INT2 but not INT2 INT1 and hence optional except in the case
of only one interrupt provided.

1) New DT, old driver - no interrupts but then there weren't any previously anyway.
2) New Driver old DT - fine, either both are specified or neither.

So there is a clean migration path if we ever find anyone who has wired up only
one of the interrupt lines and really does only want events on one or two channels
but still wants readings on both of them.

Jonathan


> 
> 
> > 
> > I wonder if we should use the symbolic constants for the IRQ type to
> > make the example more clear. E.g.
> > 
> > interrupts = <25 IRQ_TYPE_EDGE_FALLING>, ...
> >   
> > > +            interrupt-parent = <&gpio>;
> > > +        };
> > > +    };
> > > +...  
> >   
> Thanks
> Barry
> 

Re: [PATCH 21/24] dt-bindings:iio:cdc:adi,ad7150 binding doc

[Jonathan Cameron]

On Sun,  7 Feb 2021 15:46:20 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Binding covering the ad7150, ad7151 and ad7156 capacitance to digital
> convertors.  The only difference between these is how many channels they
> have (1 or 2)
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Robh+dt@kernel.org
> Cc: devicetree@vger.kernel.org
@Rob,

Any comments on this?  Lars requested that I use symbolic values
for the irq flags which I can do whilst applying - but otherwise
I don't plan to change anything else in here.

It's the only patch that needs tweaking and I don't really
want to repost all 24 just for that.

Thanks,

Jonathan

> ---
>  .../bindings/iio/cdc/adi,ad7150.yaml          | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/cdc/adi,ad7150.yaml b/Documentation/devicetree/bindings/iio/cdc/adi,ad7150.yaml
> new file mode 100644
> index 000000000000..2155d3f5666c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/cdc/adi,ad7150.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/cdc/adi,ad7150.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog device AD7150 and similar capacitance to digital convertors.
> +
> +maintainers:
> +  - Jonathan Cameron <jic23@kernel.org>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad7150
> +      - adi,ad7151
> +      - adi,ad7156
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +  interrupts: true
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,ad7150
> +              - adi,ad7156
> +    then:
> +      properties:
> +        interrupts:
> +          minItems: 2
> +          maxItems: 2
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: adi,ad7151
> +    then:
> +      properties:
> +        interrupts:
> +          minItems: 1
> +          maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        cdc@48 {
> +            compatible = "adi,ad7150";
> +            reg = <0x48>;
> +            interrupts = <25 2>, <26 2>;
> +            interrupt-parent = <&gpio>;
> +        };
> +    };
> +...