diff mbox series

[1/2] dt-bindings: iio: adc: add ad7944 ADCs

Message ID 20240206-ad7944-mainline-v1-1-bf115fa9474f@baylibre.com (mailing list archive)
State Changes Requested
Headers show
Series iio: adc: ad7944: new driver | expand

Commit Message

David Lechner Feb. 6, 2024, 5:25 p.m. UTC
This adds a new binding for the Analog Devices, Inc. AD7944, AD7985, and
AD7986 ADCs.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 .../devicetree/bindings/iio/adc/adi,ad7944.yaml    | 231 +++++++++++++++++++++
 MAINTAINERS                                        |   8 +
 2 files changed, 239 insertions(+)

Comments

David Lechner Feb. 6, 2024, 5:34 p.m. UTC | #1
On Tue, Feb 6, 2024 at 11:26 AM David Lechner <dlechner@baylibre.com> wrote:
>
> This adds a new binding for the Analog Devices, Inc. AD7944, AD7985, and
> AD7986 ADCs.
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7944.yaml    | 231 +++++++++++++++++++++
>  MAINTAINERS                                        |   8 +
>  2 files changed, 239 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
> new file mode 100644
> index 000000000000..a023adbeba42
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml

...


+  adi,reference:
+    $ref: /schemas/types.yaml#/definitions/string
+    enum: [ internal, internal-buffer, external ]
+    default: internal

...

> +allOf:
> +  # ref-supply is only used for external reference voltage
> +  - if:
> +      not:
> +        required:
> +          - adi,reference
> +    then:
> +      properties:
> +        ref-supply: false
> +    else:
> +      if:
> +        properties:
> +          adi,reference:
> +            const: external
> +      then:
> +        required:
> +          - ref-supply
> +      else:
> +        properties:
> +          ref-supply: false

This seems like something that could potentially be improved in the
dtschema tooling. Since adi,reference has a default of "internal", I
would expect:

     if:
       properties:
         adi,reference:
           const: external
     then:
       required:
         - ref-supply
     else:
       properties:
         ref-supply: false

to be sufficient here. However, currently, if the adi,reference
property is omitted from the dts/dtb, the condition here evaluates to
true and unexpectedly (incorrectly?) the validator requires the
ref-supply property.
Conor Dooley Feb. 7, 2024, 5:27 p.m. UTC | #2
On Tue, Feb 06, 2024 at 11:34:13AM -0600, David Lechner wrote:
> On Tue, Feb 6, 2024 at 11:26 AM David Lechner <dlechner@baylibre.com> wrote:
> >
> > This adds a new binding for the Analog Devices, Inc. AD7944, AD7985, and
> > AD7986 ADCs.
> >
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > ---
> >  .../devicetree/bindings/iio/adc/adi,ad7944.yaml    | 231 +++++++++++++++++++++
> >  MAINTAINERS                                        |   8 +
> >  2 files changed, 239 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
> > new file mode 100644
> > index 000000000000..a023adbeba42
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
> 
> ...
> 
> 
> +  adi,reference:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ internal, internal-buffer, external ]
> +    default: internal
> 
> ...
> 
> > +allOf:
> > +  # ref-supply is only used for external reference voltage
> > +  - if:
> > +      not:
> > +        required:
> > +          - adi,reference
> > +    then:
> > +      properties:
> > +        ref-supply: false
> > +    else:
> > +      if:
> > +        properties:
> > +          adi,reference:
> > +            const: external
> > +      then:
> > +        required:
> > +          - ref-supply
> > +      else:
> > +        properties:
> > +          ref-supply: false
> 
> This seems like something that could potentially be improved in the
> dtschema tooling. Since adi,reference has a default of "internal", I
> would expect:

Oh god, Rob will probably have to remind us how this works. I talked
with him about trying to do some conditional stuff like this a while
back, but I was not able to find any logs for IRC from then.

>      if:
>        properties:
>          adi,reference:
>            const: external
>      then:
>        required:
>          - ref-supply
>      else:
>        properties:
>          ref-supply: false
> 
> to be sufficient here. However, currently, if the adi,reference
> property is omitted from the dts/dtb, the condition here evaluates to
> true and unexpectedly (incorrectly?) the validator requires the
> ref-supply property.

But I was trying to do something like you are here, and was also
surprised that this evaluated to true when the property was not present.

I ended up doing something completely different, so I have no example to
show you of how things ended up being.
Jonathan Cameron Feb. 10, 2024, 5:40 p.m. UTC | #3
On Tue,  6 Feb 2024 11:25:59 -0600
David Lechner <dlechner@baylibre.com> wrote:

> This adds a new binding for the Analog Devices, Inc. AD7944, AD7985, and
> AD7986 ADCs.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>

Hi David,

Some tricky corners...
3-wire here for example doesn't mean what I at least expected it to.

> ---
>  .../devicetree/bindings/iio/adc/adi,ad7944.yaml    | 231 +++++++++++++++++++++
>  MAINTAINERS                                        |   8 +
>  2 files changed, 239 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
> new file mode 100644
> index 000000000000..a023adbeba42
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
> @@ -0,0 +1,231 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7944.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices PulSAR LFCSP Analog to Digital Converters
> +
> +maintainers:
> +  - Michael Hennerich <Michael.Hennerich@analog.com>
> +  - Nuno Sá <nuno.sa@analog.com

I hope Nuno + Michael will ack this. Bit mean to drop them in it otherwise
(funny though :)

> +
> +description: |
> +  A family of pin-compatible single channel differential analog to digital
> +  converters with SPI support in a LFCSP package.
> +
> +  * https://www.analog.com/en/products/ad7944.html
> +  * https://www.analog.com/en/products/ad7985.html
> +  * https://www.analog.com/en/products/ad7986.html
> +
> +$ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad7944
> +      - adi,ad7985
> +      - adi,ad7986
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 111111111

So 9ns for 3-write and 4-wire, but I think it's 11ns for chained.
Maybe it's not worth constraining that.

> +
> +  spi-cpha: true
> +
> +  adi,spi-mode:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ 3-wire, 4-wire, chain ]
> +    default: 4-wire
> +    description:
> +      This chip can operate in a 3-wire mode where SDI is tied to VIO, a 4-wire
> +      mode where SDI acts as the CS line, or a chain mode where SDI of one chip
> +      is tied to the SDO of the next chip in the chain and the SDI of the last
> +      chip in the chain is tied to GND.

there is a standard property in spi-controller.yaml for 3-wire. Does that cover
the selection between 3-wire and 4-wire here?  Seems like this might behave
differently from that (and so perhaps we shouldn't use 3-wire as the description
to avoid confusion, normally 3-wire is a half duplex link I think).

Chain mode is more fun.  We've had that before and I'm trying to remember what
the bindings look like. Devices like ad7280a do a different form of chaining.

Anyhow, main thing here is we need to be careful that the terms don't overlap
with other possible interpretations.

I think what this really means is:

3-wire - no chip select, exclusive use of the SPI bus (yuk)
4-write - conventional SPI with CS
chained - the 3 wire mode really but with some timing effects?

Can we figure out if chained is going on at runtime?







> +
> +  avdd-supply:
> +    description: A 2.5V supply that powers the analog circuitry.
> +
> +  dvdd-supply:
> +    description: A 2.5V supply that powers the digital circuitry.
> +
> +  vio-supply:
> +    description:
> +      A 1.8V to 2.7V supply for the digital inputs and outputs.
> +
> +  bvdd-supply:
> +    description:
> +      A voltage supply for the buffered power. When using an external reference
> +      without an internal buffer (PDREF high, REFIN low), this should be
> +      connected to the same supply as ref-supply. Otherwise, when using an
> +      internal reference or an external reference with an internal buffer, this
> +      is connected to a 5V supply.
> +
> +  ref-supply:
> +    description:
> +      Voltage regulator for the reference voltage (REF). This property is
> +      omitted when using an internal reference.
> +
> +  refin-supply:
> +    description:
> +      Voltage regulator for the reference buffer input (REFIN). When using an
> +      external buffer with internal reference, this should be connected to a
> +      1.2V external reference voltage supply.
> +
> +  adi,reference:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ internal, internal-buffer, external ]

I'm a bit lost on this one - but think we can get rid of it in favour of using
the fact someone wired up the supplies to indicate their intent?

> +    default: internal
> +    description: |
> +      This property is used to specify the reference voltage source.
> +
> +      * internal: PDREF is wired low. The internal 4.096V reference voltage is
> +        used. The REF pin outputs 4.096V and REFIN outputs 1.2V.

So if neither refin-supply or ref-supply is present then this is the one to use.

> +      * internal-buffer: PDREF is wired high. REFIN is supplied with 1.2V. The
> +        buffered internal 4.096V reference voltage is used. The REF pin outputs
> +        4.096V.

So if refin-supply is supplied this is the expected choice?

> +      * external: PDREF is wired high and REFIN is wired low. The supply
> +        connnected the REF pin is used as the reference voltage.

So if a ref-supply is provided this is expected choice?

If we are going to rule you supplying refin and ref supplies. 

> +
> +  cnv-gpios:
> +    description:
> +      The Convert Input (CNV). This input has multiple functions. It initiates
> +      the conversions and selects the SPI mode of the device (chain or CS). In
> +      3-wire mode, this property is omitted if the CNV pin is connected to the
> +      CS line of the SPI controller.
> +    maxItems: 1

ah, that's exciting - so in 3-wire mode, we basically put the CS on a different pin...

Mark, perhaps you can suggest how to handle this complex family of spi variants?

Jonathan
David Lechner Feb. 11, 2024, 5:49 p.m. UTC | #4
On Sat, Feb 10, 2024 at 11:40 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue,  6 Feb 2024 11:25:59 -0600
> David Lechner <dlechner@baylibre.com> wrote:
>
> > This adds a new binding for the Analog Devices, Inc. AD7944, AD7985, and
> > AD7986 ADCs.
> >
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
>
> Hi David,
>
> Some tricky corners...
> 3-wire here for example doesn't mean what I at least expected it to.
>
> > ---
> >  .../devicetree/bindings/iio/adc/adi,ad7944.yaml    | 231 +++++++++++++++++++++
> >  MAINTAINERS                                        |   8 +
> >  2 files changed, 239 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
> > new file mode 100644
> > index 000000000000..a023adbeba42
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
> > @@ -0,0 +1,231 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/adi,ad7944.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices PulSAR LFCSP Analog to Digital Converters
> > +
> > +maintainers:
> > +  - Michael Hennerich <Michael.Hennerich@analog.com>
> > +  - Nuno Sá <nuno.sa@analog.com
>
> I hope Nuno + Michael will ack this. Bit mean to drop them in it otherwise
> (funny though :)

Nothing mean here. This is according to a prior (off-list) agreement.

>
> > +
> > +description: |
> > +  A family of pin-compatible single channel differential analog to digital
> > +  converters with SPI support in a LFCSP package.
> > +
> > +  * https://www.analog.com/en/products/ad7944.html
> > +  * https://www.analog.com/en/products/ad7985.html
> > +  * https://www.analog.com/en/products/ad7986.html
> > +
> > +$ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,ad7944
> > +      - adi,ad7985
> > +      - adi,ad7986
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-max-frequency:
> > +    maximum: 111111111
>
> So 9ns for 3-write and 4-wire, but I think it's 11ns for chained.
> Maybe it's not worth constraining that.

I didn't think it was worth it either, so left it out. Easy enough to
add though.

>
> > +
> > +  spi-cpha: true
> > +
> > +  adi,spi-mode:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: [ 3-wire, 4-wire, chain ]
> > +    default: 4-wire
> > +    description:
> > +      This chip can operate in a 3-wire mode where SDI is tied to VIO, a 4-wire
> > +      mode where SDI acts as the CS line, or a chain mode where SDI of one chip
> > +      is tied to the SDO of the next chip in the chain and the SDI of the last
> > +      chip in the chain is tied to GND.
>
> there is a standard property in spi-controller.yaml for 3-wire. Does that cover
> the selection between 3-wire and 4-wire here?  Seems like this might behave
> differently from that (and so perhaps we shouldn't use 3-wire as the description
> to avoid confusion, normally 3-wire is a half duplex link I think).

I used "3-wire" because that is what the datasheet calls it. But yes,
I see the potential for confusion here since this "3-wire" is
completely unrelated to the standard "spi-3wire" property.

>
> Chain mode is more fun.  We've had that before and I'm trying to remember what
> the bindings look like. Devices like ad7280a do a different form of chaining.

If there isn't a clear precedent for how to write bindings for chained
devices, this may be something better left for when there is an actual
use case to be sure we get it right.

>
> Anyhow, main thing here is we need to be careful that the terms don't overlap
> with other possible interpretations.
>
> I think what this really means is:
>
> 3-wire - no chip select, exclusive use of the SPI bus (yuk)

This can actually be done two ways. One where there is no CS and we
use cnv-gpios to control the conversion. The other is where CS of the
SPI controller is connected to the CNV pin on the ADC and cnv-gpios is
omitted. This requires very creative use of spi xfers to get the right
signal but does work.

In any case to achieve max sample rate these chips need to use this
"3-wire" mode and have exclusive use of the bus whether is is using
proper CS or not.

So maybe it would be more clear to split this one into two modes?
3-wire with CS and 3-wire without CS?

> 4-write - conventional SPI with CS

Yes.

> chained - the 3 wire mode really but with some timing effects?

Correct. With the exception that the SPI CS line can't be used in
chain mode (unless maybe if you had an inverted CS signal since the
CNV pin has to be high during the data transfer).

>
> Can we figure out if chained is going on at runtime?

No. We would always need the devicetree to at least say how many chips
are in the chain. Also, in theory, each chip could have independent
supplies and therefore different reference voltages.

>
> > +
> > +  avdd-supply:
> > +    description: A 2.5V supply that powers the analog circuitry.
> > +
> > +  dvdd-supply:
> > +    description: A 2.5V supply that powers the digital circuitry.
> > +
> > +  vio-supply:
> > +    description:
> > +      A 1.8V to 2.7V supply for the digital inputs and outputs.
> > +
> > +  bvdd-supply:
> > +    description:
> > +      A voltage supply for the buffered power. When using an external reference
> > +      without an internal buffer (PDREF high, REFIN low), this should be
> > +      connected to the same supply as ref-supply. Otherwise, when using an
> > +      internal reference or an external reference with an internal buffer, this
> > +      is connected to a 5V supply.
> > +
> > +  ref-supply:
> > +    description:
> > +      Voltage regulator for the reference voltage (REF). This property is
> > +      omitted when using an internal reference.
> > +
> > +  refin-supply:
> > +    description:
> > +      Voltage regulator for the reference buffer input (REFIN). When using an
> > +      external buffer with internal reference, this should be connected to a
> > +      1.2V external reference voltage supply.
> > +
> > +  adi,reference:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: [ internal, internal-buffer, external ]
>
> I'm a bit lost on this one - but think we can get rid of it in favour of using
> the fact someone wired up the supplies to indicate their intent?

Yes, we can do as you suggest. I added this property since I thought
it made things a bit clearer, but apparently not.

>
> > +    default: internal
> > +    description: |
> > +      This property is used to specify the reference voltage source.
> > +
> > +      * internal: PDREF is wired low. The internal 4.096V reference voltage is
> > +        used. The REF pin outputs 4.096V and REFIN outputs 1.2V.
>
> So if neither refin-supply or ref-supply is present then this is the one to use.

Correct.

>
> > +      * internal-buffer: PDREF is wired high. REFIN is supplied with 1.2V. The
> > +        buffered internal 4.096V reference voltage is used. The REF pin outputs
> > +        4.096V.
>
> So if refin-supply is supplied this is the expected choice?

Correct.

>
> > +      * external: PDREF is wired high and REFIN is wired low. The supply
> > +        connnected the REF pin is used as the reference voltage.
>
> So if a ref-supply is provided this is expected choice?

Correct.

>
> If we are going to rule you supplying refin and ref supplies.

Not sure what you mean here, but we can get rid of the adi,reference
property and just add a check to not allow both ref-supply and
refin-supply at the same time.

>
> > +
> > +  cnv-gpios:
> > +    description:
> > +      The Convert Input (CNV). This input has multiple functions. It initiates
> > +      the conversions and selects the SPI mode of the device (chain or CS). In
> > +      3-wire mode, this property is omitted if the CNV pin is connected to the
> > +      CS line of the SPI controller.
> > +    maxItems: 1
>
> ah, that's exciting - so in 3-wire mode, we basically put the CS on a different pin...

I explained this above already, but just to have it in context here as
well... In what the datasheet calls "3-wire" mode, we can either have
CS connected and no cnv-gpios or we can have no CS and have cnv-gpios
connected.

So the intention here was to make cnv-gpios required all other modes
but in 3-wire mode, make it optional.


>
> Mark, perhaps you can suggest how to handle this complex family of spi variants?
>
> Jonathan
>
Rob Herring (Arm) Feb. 15, 2024, 1:23 p.m. UTC | #5
On Tue, Feb 06, 2024 at 11:34:13AM -0600, David Lechner wrote:
> On Tue, Feb 6, 2024 at 11:26 AM David Lechner <dlechner@baylibre.com> wrote:
> >
> > This adds a new binding for the Analog Devices, Inc. AD7944, AD7985, and
> > AD7986 ADCs.
> >
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > ---
> >  .../devicetree/bindings/iio/adc/adi,ad7944.yaml    | 231 +++++++++++++++++++++
> >  MAINTAINERS                                        |   8 +
> >  2 files changed, 239 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
> > new file mode 100644
> > index 000000000000..a023adbeba42
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
> 
> ...
> 
> 
> +  adi,reference:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ internal, internal-buffer, external ]
> +    default: internal
> 
> ...
> 
> > +allOf:
> > +  # ref-supply is only used for external reference voltage
> > +  - if:
> > +      not:
> > +        required:
> > +          - adi,reference
> > +    then:
> > +      properties:
> > +        ref-supply: false
> > +    else:
> > +      if:
> > +        properties:
> > +          adi,reference:
> > +            const: external
> > +      then:
> > +        required:
> > +          - ref-supply
> > +      else:
> > +        properties:
> > +          ref-supply: false
> 
> This seems like something that could potentially be improved in the
> dtschema tooling. Since adi,reference has a default of "internal", I
> would expect:
> 
>      if:
>        properties:
>          adi,reference:
>            const: external

         required:
           - adi,reference

>      then:
>        required:
>          - ref-supply
>      else:
>        properties:
>          ref-supply: false
> 
> to be sufficient here. However, currently, if the adi,reference
> property is omitted from the dts/dtb, the condition here evaluates to
> true and unexpectedly (incorrectly?) the validator requires the
> ref-supply property.

That's just how json-schema works. With the above, it should work for 
you.

However, redesigning the binding would make things simpler. Just make 
'ref-supply' being present mean external ref. No 'ref-supply' is then 
internal. Then you just need a boolean for 'internal-buffer' mode and:

dependentSchemas:
  ref-supply:
    not:
      required: ['adi,internal-buffer-ref']

Rob
David Lechner Feb. 15, 2024, 9:49 p.m. UTC | #6
On Thu, Feb 15, 2024 at 7:23 AM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Feb 06, 2024 at 11:34:13AM -0600, David Lechner wrote:
> > On Tue, Feb 6, 2024 at 11:26 AM David Lechner <dlechner@baylibre.com> wrote:
> > >
> >
> >      if:
> >        properties:
> >          adi,reference:
> >            const: external
>
>          required:
>            - adi,reference
>
> >      then:
> >        required:
> >          - ref-supply
> >      else:
> >        properties:
> >          ref-supply: false
> >
> > to be sufficient here. However, currently, if the adi,reference
> > property is omitted from the dts/dtb, the condition here evaluates to
> > true and unexpectedly (incorrectly?) the validator requires the
> > ref-supply property.
>
> That's just how json-schema works. With the above, it should work for
> you.
>
> However, redesigning the binding would make things simpler. Just make
> 'ref-supply' being present mean external ref. No 'ref-supply' is then
> internal. Then you just need a boolean for 'internal-buffer' mode and:
>
> dependentSchemas:
>   ref-supply:
>     not:
>       required: ['adi,internal-buffer-ref']
>

Per Jonathan's suggestion, I plan to simplify the bindings like this
but just use the presence/absence of refin-supply as this boolean
value to simplify it even further.
Jonathan Cameron Feb. 16, 2024, 2:08 p.m. UTC | #7
> > > +  adi,spi-mode:
> > > +    $ref: /schemas/types.yaml#/definitions/string
> > > +    enum: [ 3-wire, 4-wire, chain ]
> > > +    default: 4-wire
> > > +    description:
> > > +      This chip can operate in a 3-wire mode where SDI is tied to VIO, a 4-wire
> > > +      mode where SDI acts as the CS line, or a chain mode where SDI of one chip
> > > +      is tied to the SDO of the next chip in the chain and the SDI of the last
> > > +      chip in the chain is tied to GND.  
> >
> > there is a standard property in spi-controller.yaml for 3-wire. Does that cover
> > the selection between 3-wire and 4-wire here?  Seems like this might behave
> > differently from that (and so perhaps we shouldn't use 3-wire as the description
> > to avoid confusion, normally 3-wire is a half duplex link I think).  
> 
> I used "3-wire" because that is what the datasheet calls it. But yes,
> I see the potential for confusion here since this "3-wire" is
> completely unrelated to the standard "spi-3wire" property.
Maybe we fall back on a comment that says something like.

"This is not the same as spi-3wire." :)

Whatever we end up with here, I'd like everyone to agree it's
obviously different enough from existing SPI bindings that there won't
be any confusion. 

> 
> >
> > Chain mode is more fun.  We've had that before and I'm trying to remember what
> > the bindings look like. Devices like ad7280a do a different form of chaining.  
> 
> If there isn't a clear precedent for how to write bindings for chained
> devices, this may be something better left for when there is an actual
> use case to be sure we get it right.

Agreed.  Let us kick that into the future.

> 
> >
> > Anyhow, main thing here is we need to be careful that the terms don't overlap
> > with other possible interpretations.
> >
> > I think what this really means is:
> >
> > 3-wire - no chip select, exclusive use of the SPI bus (yuk)  
> 
> This can actually be done two ways. One where there is no CS and we
> use cnv-gpios to control the conversion. The other is where CS of the
> SPI controller is connected to the CNV pin on the ADC and cnv-gpios is
> omitted. This requires very creative use of spi xfers to get the right
> signal but does work.
> 
> In any case to achieve max sample rate these chips need to use this
> "3-wire" mode and have exclusive use of the bus whether is is using
> proper CS or not.
> 
> So maybe it would be more clear to split this one into two modes?
> 3-wire with CS and 3-wire without CS?
OK.

I'm not sure if the standard SPI bindings have an option for
CS tied active?  If so we should reuse that bit of [psson;e/

> 
> > 4-write - conventional SPI with CS  
> 
> Yes.
> 
> > chained - the 3 wire mode really but with some timing effects?  
> 
> Correct. With the exception that the SPI CS line can't be used in
> chain mode (unless maybe if you had an inverted CS signal since the
> CNV pin has to be high during the data transfer).
> 
> >
> > Can we figure out if chained is going on at runtime?  
> 
> No. We would always need the devicetree to at least say how many chips
> are in the chain. Also, in theory, each chip could have independent
> supplies and therefore different reference voltages.
That's one I think we only bother supporting when we actually see it.
For previous chained devices I don't think we've ever needed to do
it because they tend to be used for 'more of the same' rather than
measuring different things.  Supplies so far have always been wired
to single regulator (or single control anyway).


> >
> > If we are going to rule you supplying refin and ref supplies.  
> 
> Not sure what you mean here, but we can get rid of the adi,reference
> property and just add a check to not allow both ref-supply and
> refin-supply at the same time.

I think that is simplest route.

> 
> >  
> > > +
> > > +  cnv-gpios:
> > > +    description:
> > > +      The Convert Input (CNV). This input has multiple functions. It initiates
> > > +      the conversions and selects the SPI mode of the device (chain or CS). In
> > > +      3-wire mode, this property is omitted if the CNV pin is connected to the
> > > +      CS line of the SPI controller.
> > > +    maxItems: 1  
> >
> > ah, that's exciting - so in 3-wire mode, we basically put the CS on a different pin...  
> 
> I explained this above already, but just to have it in context here as
> well... In what the datasheet calls "3-wire" mode, we can either have
> CS connected and no cnv-gpios or we can have no CS and have cnv-gpios
> connected.
> 
> So the intention here was to make cnv-gpios required all other modes
> but in 3-wire mode, make it optional.

Seems reasonable. Thanks for the various explanations. This chip is just odd :)

> 
> 
> >
> > Mark, perhaps you can suggest how to handle this complex family of spi variants?
> >
> > Jonathan
> >
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
new file mode 100644
index 000000000000..a023adbeba42
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
@@ -0,0 +1,231 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7944.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices PulSAR LFCSP Analog to Digital Converters
+
+maintainers:
+  - Michael Hennerich <Michael.Hennerich@analog.com>
+  - Nuno Sá <nuno.sa@analog.com>
+
+description: |
+  A family of pin-compatible single channel differential analog to digital
+  converters with SPI support in a LFCSP package.
+
+  * https://www.analog.com/en/products/ad7944.html
+  * https://www.analog.com/en/products/ad7985.html
+  * https://www.analog.com/en/products/ad7986.html
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7944
+      - adi,ad7985
+      - adi,ad7986
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 111111111
+
+  spi-cpha: true
+
+  adi,spi-mode:
+    $ref: /schemas/types.yaml#/definitions/string
+    enum: [ 3-wire, 4-wire, chain ]
+    default: 4-wire
+    description:
+      This chip can operate in a 3-wire mode where SDI is tied to VIO, a 4-wire
+      mode where SDI acts as the CS line, or a chain mode where SDI of one chip
+      is tied to the SDO of the next chip in the chain and the SDI of the last
+      chip in the chain is tied to GND.
+
+  avdd-supply:
+    description: A 2.5V supply that powers the analog circuitry.
+
+  dvdd-supply:
+    description: A 2.5V supply that powers the digital circuitry.
+
+  vio-supply:
+    description:
+      A 1.8V to 2.7V supply for the digital inputs and outputs.
+
+  bvdd-supply:
+    description:
+      A voltage supply for the buffered power. When using an external reference
+      without an internal buffer (PDREF high, REFIN low), this should be
+      connected to the same supply as ref-supply. Otherwise, when using an
+      internal reference or an external reference with an internal buffer, this
+      is connected to a 5V supply.
+
+  ref-supply:
+    description:
+      Voltage regulator for the reference voltage (REF). This property is
+      omitted when using an internal reference.
+
+  refin-supply:
+    description:
+      Voltage regulator for the reference buffer input (REFIN). When using an
+      external buffer with internal reference, this should be connected to a
+      1.2V external reference voltage supply.
+
+  adi,reference:
+    $ref: /schemas/types.yaml#/definitions/string
+    enum: [ internal, internal-buffer, external ]
+    default: internal
+    description: |
+      This property is used to specify the reference voltage source.
+
+      * internal: PDREF is wired low. The internal 4.096V reference voltage is
+        used. The REF pin outputs 4.096V and REFIN outputs 1.2V.
+      * internal-buffer: PDREF is wired high. REFIN is supplied with 1.2V. The
+        buffered internal 4.096V reference voltage is used. The REF pin outputs
+        4.096V.
+      * external: PDREF is wired high and REFIN is wired low. The supply
+        connnected the REF pin is used as the reference voltage.
+
+  cnv-gpios:
+    description:
+      The Convert Input (CNV). This input has multiple functions. It initiates
+      the conversions and selects the SPI mode of the device (chain or CS). In
+      3-wire mode, this property is omitted if the CNV pin is connected to the
+      CS line of the SPI controller.
+    maxItems: 1
+
+  turbo-gpios:
+    description:
+      GPIO connected to the TURBO line. If omitted, it is assumed that the TURBO
+      line is hard-wired and the state is determined by the adi,always-turbo
+      property.
+    maxItems: 1
+
+  adi,always-turbo:
+    type: boolean
+    description:
+      When present, this property indicates that the TURBO line is hard-wired
+      and the state is always high. If neither this property nor turbo-gpios is
+      present, the TURBO line is assumed to be hard-wired and the state is
+      always low.
+
+  interrupts:
+    description:
+      The SDO pin can also function as a busy indicator. This node should be
+      connected to an interrupt that is triggered when the SDO line goes low
+      while the SDI line is high and the CNV line is low (3-wire mode) or the
+      SDI line is low and the CNV line is high (4-wire mode); or when the SDO
+      line goes high while the SDI and CNV lines are high (chain mode),
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - avdd-supply
+  - dvdd-supply
+  - vio-supply
+  - bvdd-supply
+
+allOf:
+  # ref-supply is only used for external reference voltage
+  - if:
+      not:
+        required:
+          - adi,reference
+    then:
+      properties:
+        ref-supply: false
+    else:
+      if:
+        properties:
+          adi,reference:
+            const: external
+      then:
+        required:
+          - ref-supply
+      else:
+        properties:
+          ref-supply: false
+  # refin-supply is only used for internal buffer reference voltage
+  - if:
+      not:
+        required:
+          - adi,reference
+    then:
+      properties:
+        refin-supply: false
+    else:
+      if:
+        properties:
+          adi,reference:
+            const: internal-buffer
+      then:
+        required:
+          - refin-supply
+      else:
+        properties:
+          refin-supply: false
+  # in 3-wire mode, cnv-gpios is optional, for other modes it is required
+  - if:
+      not:
+        required:
+          - adi,spi-mode
+    then:
+      required:
+        - cnv-gpios
+    else:
+      if:
+        properties:
+          adi,spi-mode:
+            enum: [ 4-wire, chain ]
+      then:
+        required:
+          - cnv-gpios
+  # chain mode doesn't work when TRUBO is enabled
+  - if:
+      properties:
+        adi,spi-mode:
+          const: chain
+      required:
+        - adi,spi-mode
+    then:
+      properties:
+        adi,always-turbo: false
+  # turbo-gpios and adi,always-turbo are mutually exclusive
+  - if:
+      required:
+        - turbo-gpios
+    then:
+      properties:
+        adi,always-turbo: false
+  - if:
+      required:
+        - adi,always-turbo
+    then:
+      properties:
+        turbo-gpios: false
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        adc@0 {
+            compatible = "adi,ad7944";
+            reg = <0>;
+            spi-cpha;
+            spi-max-frequency = <111111111>;
+            avdd-supply = <&supply_2_5V>;
+            dvdd-supply = <&supply_2_5V>;
+            vio-supply = <&supply_1_8V>;
+            bvdd-supply = <&supply_5V>;
+            cnv-gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
+            turbo-gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 00d354af10f5..4f1e658e1e0d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -451,6 +451,14 @@  W:	http://wiki.analog.com/AD7879
 W:	https://ez.analog.com/linux-software-drivers
 F:	drivers/input/touchscreen/ad7879.c
 
+AD7944 ADC DRIVER (AD7944/AD7985/AD7986)
+M:	Michael Hennerich <michael.hennerich@analog.com>
+M:	Nuno Sá <nuno.sa@analog.com>
+R:	David Lechner <dlechner@baylibre.com>
+S:	Supported
+W:	https://ez.analog.com/linux-software-drivers
+F:	Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
+
 ADAFRUIT MINI I2C GAMEPAD
 M:	Anshul Dalal <anshulusr@gmail.com>
 L:	linux-input@vger.kernel.org