[PATCH 0/2] iio: adc: ad7944: new driver

[PATCH 0/2] iio: adc: ad7944: new driver

[David Lechner]

This is a new driver for the Analog Devices AD7944/AD7985/AD7986 family
of ADCs. These are fairly simple chips (e.g. no configuration registers).
The initial driver is only supporting the 4-wire SPI mode. We plan to
follow up with support for the 3-wire SPI mode.

This work is done on behalf of Analog Devices, Inc., hence the
MAINTAINERS are @analog.com folks.

This has been tested using an EVAL-AD7985FMCZ evaluation board with a
Xilinx ZedBoard.

---
David Lechner (2):
      dt-bindings: iio: adc: add ad7944 ADCs
      iio: adc: ad7944: add driver for AD7944/AD7985/AD7986

 .../devicetree/bindings/iio/adc/adi,ad7944.yaml    | 231 ++++++++++++
 MAINTAINERS                                        |   9 +
 drivers/iio/adc/Kconfig                            |  10 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/ad7944.c                           | 397 +++++++++++++++++++++
 5 files changed, 648 insertions(+)
---
base-commit: 81e8e40ea16329914f78ca1f454d04f570540ca8
change-id: 20240206-ad7944-mainline-17c968aa0967

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

[David Lechner]

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
@@ -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

-- 
2.43.0

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

[David Lechner]

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.

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2584 bytes --]

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.



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Rob Herring]

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

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

[David Lechner]

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.

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

[Jonathan Cameron]

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

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

[David Lechner]

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
>

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

[Jonathan Cameron]

> > > +  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
> >  

[PATCH 2/2] iio: adc: ad7944: add driver for AD7944/AD7985/AD7986

[David Lechner]

This adds a driver for the Analog Devices Inc. AD7944, AD7985, and
AD7986 ADCs. These are a family of pin-compatible ADCs that can sample
at rates up to 2.5 MSPS.

The initial driver adds support for sampling at lower rates using the
usual IIO triggered buffer and can handle all 3 possible reference
voltage configurations.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 MAINTAINERS              |   1 +
 drivers/iio/adc/Kconfig  |  10 ++
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ad7944.c | 397 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 409 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4f1e658e1e0d..83d8367595f1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -458,6 +458,7 @@ 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
+F:	drivers/iio/adc/ad7944.c
 
 ADAFRUIT MINI I2C GAMEPAD
 M:	Anshul Dalal <anshulusr@gmail.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 59ae1d17b50d..93fbe6f8e306 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -280,6 +280,16 @@ config AD7923
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad7923.
 
+config AD7944
+	tristate "Analog Devices AD7944 and similar ADCs driver"
+	depends on SPI
+	help
+	  Say yes here to build support for Analog Devices
+	  AD7944, AD7985, AD7986 ADCs.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad7944
+
 config AD7949
 	tristate "Analog Devices AD7949 and similar ADCs driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 5a26ab6f1109..52d803b92cd7 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_AD7780) += ad7780.o
 obj-$(CONFIG_AD7791) += ad7791.o
 obj-$(CONFIG_AD7793) += ad7793.o
 obj-$(CONFIG_AD7887) += ad7887.o
+obj-$(CONFIG_AD7944) += ad7944.o
 obj-$(CONFIG_AD7949) += ad7949.o
 obj-$(CONFIG_AD799X) += ad799x.o
 obj-$(CONFIG_AD9467) += ad9467.o
diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
new file mode 100644
index 000000000000..67b525fb8e59
--- /dev/null
+++ b/drivers/iio/adc/ad7944.c
@@ -0,0 +1,397 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices AD7944/85/86 PulSAR ADC family driver.
+ *
+ * Copyright 2024 Analog Devices, Inc.
+ * Copyright 2024 Baylibre, SAS
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define AD7944_INTERNAL_REF_MV		4096
+
+struct ad7944_timing_spec {
+	/* Normal mode minimum CNV pulse width in nanoseconds. */
+	unsigned int cnv_ns;
+	/* TURBO mode minimum CNV pulse width in nanoseconds. */
+	unsigned int turbo_cnv_ns;
+};
+
+struct ad7944_adc {
+	struct spi_device *spi;
+	/* Chip-specific timing specifications. */
+	const struct ad7944_timing_spec *t;
+	/* GPIO connected to CNV pin. */
+	struct gpio_desc *cnv;
+	/* Optional GPIO to enable turbo mode. */
+	struct gpio_desc *turbo;
+	/* Indicates TURBO is hard-wired to be always enabled. */
+	bool always_turbo;
+	/* Reference voltage (millivolts). */
+	unsigned int ref_mv;
+
+	/*
+	 * DMA (thus cache coherency maintenance) requires the
+	 * transfer buffers to live in their own cache lines.
+	 */
+	struct {
+		union {
+			u16 u16;
+			u32 u32;
+		} raw;
+		u64 timestamp __aligned(8);
+	 } sample __aligned(IIO_DMA_MINALIGN);
+};
+
+static const struct ad7944_timing_spec ad7944_timing_spec = {
+	.cnv_ns = 420,
+	.turbo_cnv_ns = 320,
+};
+
+static const struct ad7944_timing_spec ad7986_timing_spec = {
+	.cnv_ns = 500,
+	.turbo_cnv_ns = 400,
+};
+
+struct ad7944_chip_info {
+	const char *name;
+	const struct ad7944_timing_spec *t;
+	const struct iio_chan_spec channels[2];
+};
+
+#define AD7944_DEFINE_CHIP_INFO(_name, _t, _bits, _sign)		\
+static const struct ad7944_chip_info _name##_chip_info = {		\
+	.name = #_name,							\
+	.t = &_t##_timing_spec,						\
+	.channels = {							\
+		{							\
+			.type = IIO_VOLTAGE,				\
+			.indexed = 1,					\
+			.differential = 1,				\
+			.channel = 0,					\
+			.channel2 = 1,					\
+			.scan_index = 0,				\
+			.scan_type.sign = _sign,			\
+			.scan_type.realbits = _bits,			\
+			.scan_type.storagebits = _bits > 16 ? 32 : 16,	\
+			.scan_type.endianness = IIO_CPU,		\
+			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)	\
+					| BIT(IIO_CHAN_INFO_SCALE),	\
+		},							\
+		IIO_CHAN_SOFT_TIMESTAMP(1),				\
+	},								\
+}
+
+AD7944_DEFINE_CHIP_INFO(ad7944, ad7944, 14, 'u');
+AD7944_DEFINE_CHIP_INFO(ad7985, ad7944, 16, 'u');
+AD7944_DEFINE_CHIP_INFO(ad7986, ad7986, 18, 's');
+
+/**
+ * ad7944_4_wire_mode_conversion - Perform a 4-wire mode conversion and acquisition
+ * @adc: The ADC device structure
+ * @chan: The channel specification
+ * Return: 0 on success, a negative error code on failure
+ *
+ * Upon successful return adc->sample.raw will contain the conversion result.
+ */
+static int ad7944_4_wire_mode_conversion(struct ad7944_adc *adc,
+					 const struct iio_chan_spec *chan)
+{
+	unsigned int t_cnv_ns = adc->always_turbo ? adc->t->turbo_cnv_ns
+						  : adc->t->cnv_ns;
+	struct spi_transfer xfers[] = {
+		{
+			/*
+			 * NB: can get better performance from some SPI
+			 * controllers if we use the same bits_per_word
+			 * in every transfer.
+			 */
+			.bits_per_word = chan->scan_type.realbits,
+			/*
+			 * CS has to be high for full conversion time to avoid
+			 * triggering the busy indication.
+			 */
+			.cs_off = 1,
+			.delay = {
+				.value = t_cnv_ns,
+				.unit = SPI_DELAY_UNIT_NSECS,
+			},
+
+		},
+		{
+			.rx_buf = &adc->sample.raw,
+			.len = BITS_TO_BYTES(chan->scan_type.storagebits),
+			.bits_per_word = chan->scan_type.realbits,
+		},
+	};
+	int ret;
+
+	/*
+	 * In 4-wire mode, the CNV line is held high for the entire conversion
+	 * and acquisition process.
+	 */
+	gpiod_set_value_cansleep(adc->cnv, 1);
+
+	ret = spi_sync_transfer(adc->spi, xfers, ARRAY_SIZE(xfers));
+	if (ret)
+		return ret;
+
+	gpiod_set_value_cansleep(adc->cnv, 0);
+
+	return 0;
+}
+
+static int ad7944_single_conversion(struct ad7944_adc *adc,
+				    const struct iio_chan_spec *chan,
+				    int *val)
+{
+	int ret;
+
+	ret = ad7944_4_wire_mode_conversion(adc, chan);
+	if (ret)
+		return ret;
+
+	if (chan->scan_type.storagebits > 16)
+		*val = adc->sample.raw.u32;
+	else
+		*val = adc->sample.raw.u16;
+
+	if (chan->scan_type.sign == 's')
+		*val = sign_extend32(*val, chan->scan_type.realbits - 1);
+
+	return IIO_VAL_INT;
+}
+
+static int ad7944_read_raw(struct iio_dev *indio_dev,
+			   const struct iio_chan_spec *chan,
+			   int *val, int *val2, long info)
+{
+	struct ad7944_adc *adc = iio_priv(indio_dev);
+	int ret;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = ad7944_single_conversion(adc, chan, val);
+		iio_device_release_direct_mode(indio_dev);
+		return ret;
+
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			*val = adc->ref_mv;
+			*val2 = chan->scan_type.realbits;
+
+			return IIO_VAL_FRACTIONAL_LOG2;
+		default:
+			return -EINVAL;
+		}
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info ad7944_iio_info = {
+	.read_raw = &ad7944_read_raw,
+};
+
+static irqreturn_t ad7944_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ad7944_adc *adc = iio_priv(indio_dev);
+	int ret;
+
+	ret = ad7944_4_wire_mode_conversion(adc, &indio_dev->channels[0]);
+	if (ret)
+		goto out;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &adc->sample.raw,
+					   indio_dev->scan_timestamp);
+
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static const char * const ad7944_power_supplies[] = {
+	"avdd",	"dvdd",	"bvdd", "vio"
+};
+
+static void ad7944_ref_disable(void *ref)
+{
+	regulator_disable(ref);
+}
+
+static int ad7944_probe(struct spi_device *spi)
+{
+	const struct ad7944_chip_info *chip_info;
+	struct iio_dev *indio_dev;
+	struct ad7944_adc *adc;
+	struct regulator *ref;
+	const char *str_val;
+	int ret;
+
+	/* adi,spi-mode property defaults to "4-wire" if not present */
+	if (device_property_read_string(&spi->dev, "adi,spi-mode", &str_val) < 0)
+		str_val = "4-wire";
+
+	if (strcmp(str_val, "4-wire"))
+		return dev_err_probe(&spi->dev, -EINVAL,
+				     "only \"4-wire\" mode is currently supported\n");
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	adc = iio_priv(indio_dev);
+	adc->spi = spi;
+
+	chip_info = spi_get_device_match_data(spi);
+	if (!chip_info)
+		return dev_err_probe(&spi->dev, -EINVAL, "no chip info\n");
+
+	adc->t = chip_info->t;
+
+	/*
+	 * Some chips use unusual word sizes, so check now instead of waiting
+	 * for the first xfer.
+	 */
+	if (!spi_is_bpw_supported(spi, chip_info->channels[0].scan_type.realbits))
+		return dev_err_probe(&spi->dev, -EINVAL,
+				"SPI host does not support %d bits per word\n",
+				chip_info->channels[0].scan_type.realbits);
+
+	ret = devm_regulator_bulk_get_enable(&spi->dev,
+					     ARRAY_SIZE(ad7944_power_supplies),
+					     ad7944_power_supplies);
+	if (ret)
+		return dev_err_probe(&spi->dev, ret,
+				     "failed to get and enable supplies\n");
+
+	/* adi,reference property defaults to "internal" if not present */
+	if (device_property_read_string(&spi->dev, "adi,reference", &str_val) < 0)
+		str_val = "internal";
+
+	/* sort out what is being used for the reference voltage */
+	if (strcmp(str_val, "internal") == 0) {
+		/* internal reference is used */
+		adc->ref_mv = AD7944_INTERNAL_REF_MV;
+	} else if (strcmp(str_val, "internal-buffer") == 0) {
+		/* external 1.2V REFIN and internal buffer is used */
+		ret = devm_regulator_get_enable_optional(&spi->dev, "refin");
+		if (ret)
+			return dev_err_probe(&spi->dev, ret,
+					"failed to get and enable REFIN supply\n");
+
+		adc->ref_mv = AD7944_INTERNAL_REF_MV;
+	} else if (strcmp(str_val, "external") == 0) {
+		/* external reference is used */
+		ref = devm_regulator_get_optional(&spi->dev, "ref");
+		if (IS_ERR(ref))
+			return dev_err_probe(&spi->dev, PTR_ERR(ref),
+					     "failed to get REF supply\n");
+
+		ret = regulator_enable(ref);
+		if (ret)
+			return dev_err_probe(&spi->dev, ret,
+					     "failed to enable REF supply\n");
+
+		ret = devm_add_action_or_reset(&spi->dev,
+					       ad7944_ref_disable, ref);
+		if (ret)
+			return ret;
+
+		ret = regulator_get_voltage(ref);
+		if (ret < 0)
+			return dev_err_probe(&spi->dev, ret,
+					     "failed to get REF voltage\n");
+
+		adc->ref_mv = ret / 1000;
+	} else {
+		return dev_err_probe(&spi->dev, -EINVAL,
+				     "invalid adi,reference property: %s\n",
+				     str_val);
+	}
+
+	adc->cnv = devm_gpiod_get(&spi->dev, "cnv", GPIOD_OUT_LOW);
+	if (IS_ERR(adc->cnv))
+		return dev_err_probe(&spi->dev, PTR_ERR(adc->cnv),
+				     "failed to get CNV GPIO\n");
+
+	adc->turbo = devm_gpiod_get_optional(&spi->dev, "turbo", GPIOD_OUT_LOW);
+	if (IS_ERR(adc->turbo))
+		return dev_err_probe(&spi->dev, PTR_ERR(adc->turbo),
+				     "failed to get TURBO GPIO\n");
+
+	if (device_property_present(&spi->dev, "adi,always-turbo"))
+		adc->always_turbo = true;
+
+	if (adc->turbo && adc->always_turbo)
+		return dev_err_probe(&spi->dev, -EINVAL,
+			"cannot have both turbo-gpios and adi,always-turbo\n");
+
+	indio_dev->name = chip_info->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &ad7944_iio_info;
+	indio_dev->channels = chip_info->channels;
+	indio_dev->num_channels = ARRAY_SIZE(chip_info->channels);
+
+	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      ad7944_trigger_handler, NULL);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct of_device_id ad7944_of_match[] = {
+	{ .compatible = "adi,ad7944", .data = &ad7944_chip_info },
+	{ .compatible = "adi,ad7985", .data = &ad7985_chip_info },
+	{ .compatible = "adi,ad7986", .data = &ad7986_chip_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad7944_of_match);
+
+static const struct spi_device_id ad7944_spi_id[] = {
+	{ "ad7944", (kernel_ulong_t)&ad7944_chip_info },
+	{ "ad7985", (kernel_ulong_t)&ad7985_chip_info },
+	{ "ad7986", (kernel_ulong_t)&ad7986_chip_info },
+	{ }
+
+};
+MODULE_DEVICE_TABLE(spi, ad7944_spi_id);
+
+static struct spi_driver ad7944_driver = {
+	.driver = {
+		.name = "ad7944",
+		.of_match_table = ad7944_of_match,
+	},
+	.probe = ad7944_probe,
+	.id_table = ad7944_spi_id,
+};
+module_spi_driver(ad7944_driver);
+
+MODULE_AUTHOR("David Lechner <dlechner@baylibre.com>");
+MODULE_DESCRIPTION("Analog Devices AD7944 PulSAR ADC family driver");
+MODULE_LICENSE("GPL");

-- 
2.43.0

Re: [PATCH 2/2] iio: adc: ad7944: add driver for AD7944/AD7985/AD7986

[Nuno Sá]

Hi David,

The driver It's in pretty good shape... Just some comments from me
 
On Tue, 2024-02-06 at 11:26 -0600, David Lechner wrote:
> This adds a driver for the Analog Devices Inc. AD7944, AD7985, and
> AD7986 ADCs. These are a family of pin-compatible ADCs that can sample
> at rates up to 2.5 MSPS.
> 
> The initial driver adds support for sampling at lower rates using the
> usual IIO triggered buffer and can handle all 3 possible reference
> voltage configurations.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  MAINTAINERS              |   1 +
>  drivers/iio/adc/Kconfig  |  10 ++
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad7944.c | 397
> +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 409 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4f1e658e1e0d..83d8367595f1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -458,6 +458,7 @@ 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
> +F:	drivers/iio/adc/ad7944.c
>  
>  ADAFRUIT MINI I2C GAMEPAD
>  M:	Anshul Dalal <anshulusr@gmail.com>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 59ae1d17b50d..93fbe6f8e306 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -280,6 +280,16 @@ config AD7923
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad7923.
>  
> +config AD7944
> +	tristate "Analog Devices AD7944 and similar ADCs driver"
> +	depends on SPI
> +	help
> +	  Say yes here to build support for Analog Devices
> +	  AD7944, AD7985, AD7986 ADCs.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad7944
> +
>  config AD7949
>  	tristate "Analog Devices AD7949 and similar ADCs driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 5a26ab6f1109..52d803b92cd7 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_AD7780) += ad7780.o
>  obj-$(CONFIG_AD7791) += ad7791.o
>  obj-$(CONFIG_AD7793) += ad7793.o
>  obj-$(CONFIG_AD7887) += ad7887.o
> +obj-$(CONFIG_AD7944) += ad7944.o
>  obj-$(CONFIG_AD7949) += ad7949.o
>  obj-$(CONFIG_AD799X) += ad799x.o
>  obj-$(CONFIG_AD9467) += ad9467.o
> diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
> new file mode 100644
> index 000000000000..67b525fb8e59
> --- /dev/null
> +++ b/drivers/iio/adc/ad7944.c
> @@ -0,0 +1,397 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD7944/85/86 PulSAR ADC family driver.
> + *
> + * Copyright 2024 Analog Devices, Inc.
> + * Copyright 2024 Baylibre, SAS
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define AD7944_INTERNAL_REF_MV		4096
> +
> +struct ad7944_timing_spec {
> +	/* Normal mode minimum CNV pulse width in nanoseconds. */
> +	unsigned int cnv_ns;
> +	/* TURBO mode minimum CNV pulse width in nanoseconds. */
> +	unsigned int turbo_cnv_ns;
> +};
> +
> 

...

> +}
> +
> +static int ad7944_single_conversion(struct ad7944_adc *adc,
> +				    const struct iio_chan_spec *chan,
> +				    int *val)
> +{
> +	int ret;
> +
> +	ret = ad7944_4_wire_mode_conversion(adc, chan);
> +	if (ret)
> +		return ret;
> +
> +	if (chan->scan_type.storagebits > 16)
> +		*val = adc->sample.raw.u32;
> +	else
> +		*val = adc->sample.raw.u16;
> +

Will this work both in big vs little endian archs? I don't think so but maybe
I'm missing something. At a first glance, it seems we get big endian from spi so
shouldn't we have __be16 and __be32?
 
> +	if (chan->scan_type.sign == 's')
> +		*val = sign_extend32(*val, chan->scan_type.realbits - 1);
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int ad7944_read_raw(struct iio_dev *indio_dev,
> +			   const struct iio_chan_spec *chan,
> +			   int *val, int *val2, long info)
> +{
> +	struct ad7944_adc *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +

I'm not totally sure but I think Jonathan already merged his series for the
cleanup stuff for the claim direct mode. Maybe take a look and use it? Not a big
win in here but I guess we could still reduce some LOC.

> +		ret = ad7944_single_conversion(adc, chan, val);
> +		iio_device_release_direct_mode(indio_dev);
> +		return ret;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			*val = adc->ref_mv;
> +			*val2 = chan->scan_type.realbits;
> +
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info ad7944_iio_info = {
> +	.read_raw = &ad7944_read_raw,
> +};
> +
> +static irqreturn_t ad7944_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad7944_adc *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad7944_4_wire_mode_conversion(adc, &indio_dev->channels[0]);
> +	if (ret)
> +		goto out;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &adc->sample.raw,
> +					   indio_dev->scan_timestamp);
> +
> +out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const char * const ad7944_power_supplies[] = {
> +	"avdd",	"dvdd",	"bvdd", "vio"
> +};
> +
> +static void ad7944_ref_disable(void *ref)
> +{
> +	regulator_disable(ref);
> +}
> +
> +static int ad7944_probe(struct spi_device *spi)
> +{
> +	const struct ad7944_chip_info *chip_info;
> +	struct iio_dev *indio_dev;
> +	struct ad7944_adc *adc;
> +	struct regulator *ref;
> +	const char *str_val;
> +	int ret;
> +
> +	/* adi,spi-mode property defaults to "4-wire" if not present */
> +	if (device_property_read_string(&spi->dev, "adi,spi-mode", &str_val)
> < 0)
> +		str_val = "4-wire";
> +
> +	if (strcmp(str_val, "4-wire"))
> +		return dev_err_probe(&spi->dev, -EINVAL,
> +				     "only \"4-wire\" mode is currently
> supported\n");

Did you looked at spi core? I guess the chain mode is not available but IIRC spi
already has spi-3wire. So maybe you could just have a boolean property for the
chain mode and check both that and 3wire?

> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	adc = iio_priv(indio_dev);
> +	adc->spi = spi;
> +
> +	chip_info = spi_get_device_match_data(spi);
> +	if (!chip_info)
> +		return dev_err_probe(&spi->dev, -EINVAL, "no chip info\n");
> +
> +	adc->t = chip_info->t;
> +
> +	/*
> +	 * Some chips use unusual word sizes, so check now instead of waiting
> +	 * for the first xfer.
> +	 */
> +	if (!spi_is_bpw_supported(spi, chip_info-
> >channels[0].scan_type.realbits))
> +		return dev_err_probe(&spi->dev, -EINVAL,
> +				"SPI host does not support %d bits per
> word\n",
> +				chip_info->channels[0].scan_type.realbits);
> +
> +	ret = devm_regulator_bulk_get_enable(&spi->dev,
> +					    
> ARRAY_SIZE(ad7944_power_supplies),
> +					     ad7944_power_supplies);
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret,
> +				     "failed to get and enable supplies\n");
> +
> +	/* adi,reference property defaults to "internal" if not present */
> +	if (device_property_read_string(&spi->dev, "adi,reference", &str_val)
> < 0)
> +		str_val = "internal";
> +
> +	/* sort out what is being used for the reference voltage */
> +	if (strcmp(str_val, "internal") == 0) {

Maybe you can make the code neater with match_string() and some enum...

- Nuno Sá

Re: [PATCH 2/2] iio: adc: ad7944: add driver for AD7944/AD7985/AD7986

[David Lechner]

On Wed, Feb 7, 2024 at 4:07 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> Hi David,
>
> The driver It's in pretty good shape... Just some comments from me
>
> On Tue, 2024-02-06 at 11:26 -0600, David Lechner wrote:
> > This adds a driver for the Analog Devices Inc. AD7944, AD7985, and
> > AD7986 ADCs. These are a family of pin-compatible ADCs that can sample
> > at rates up to 2.5 MSPS.
> >
> > The initial driver adds support for sampling at lower rates using the
> > usual IIO triggered buffer and can handle all 3 possible reference
> > voltage configurations.
> >
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > ---
> >  MAINTAINERS              |   1 +
> >  drivers/iio/adc/Kconfig  |  10 ++
> >  drivers/iio/adc/Makefile |   1 +
> >  drivers/iio/adc/ad7944.c | 397
> > +++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 409 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 4f1e658e1e0d..83d8367595f1 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -458,6 +458,7 @@ 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
> > +F:   drivers/iio/adc/ad7944.c
> >
> >  ADAFRUIT MINI I2C GAMEPAD
> >  M:   Anshul Dalal <anshulusr@gmail.com>
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 59ae1d17b50d..93fbe6f8e306 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -280,6 +280,16 @@ config AD7923
> >         To compile this driver as a module, choose M here: the
> >         module will be called ad7923.
> >
> > +config AD7944
> > +     tristate "Analog Devices AD7944 and similar ADCs driver"
> > +     depends on SPI
> > +     help
> > +       Say yes here to build support for Analog Devices
> > +       AD7944, AD7985, AD7986 ADCs.
> > +
> > +       To compile this driver as a module, choose M here: the
> > +       module will be called ad7944
> > +
> >  config AD7949
> >       tristate "Analog Devices AD7949 and similar ADCs driver"
> >       depends on SPI
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 5a26ab6f1109..52d803b92cd7 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -29,6 +29,7 @@ obj-$(CONFIG_AD7780) += ad7780.o
> >  obj-$(CONFIG_AD7791) += ad7791.o
> >  obj-$(CONFIG_AD7793) += ad7793.o
> >  obj-$(CONFIG_AD7887) += ad7887.o
> > +obj-$(CONFIG_AD7944) += ad7944.o
> >  obj-$(CONFIG_AD7949) += ad7949.o
> >  obj-$(CONFIG_AD799X) += ad799x.o
> >  obj-$(CONFIG_AD9467) += ad9467.o
> > diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
> > new file mode 100644
> > index 000000000000..67b525fb8e59
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad7944.c
> > @@ -0,0 +1,397 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Analog Devices AD7944/85/86 PulSAR ADC family driver.
> > + *
> > + * Copyright 2024 Analog Devices, Inc.
> > + * Copyright 2024 Baylibre, SAS
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/module.h>
> > +#include <linux/property.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +
> > +#define AD7944_INTERNAL_REF_MV               4096
> > +
> > +struct ad7944_timing_spec {
> > +     /* Normal mode minimum CNV pulse width in nanoseconds. */
> > +     unsigned int cnv_ns;
> > +     /* TURBO mode minimum CNV pulse width in nanoseconds. */
> > +     unsigned int turbo_cnv_ns;
> > +};
> > +
> >
>
> ...
>
> > +}
> > +
> > +static int ad7944_single_conversion(struct ad7944_adc *adc,
> > +                                 const struct iio_chan_spec *chan,
> > +                                 int *val)
> > +{
> > +     int ret;
> > +
> > +     ret = ad7944_4_wire_mode_conversion(adc, chan);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (chan->scan_type.storagebits > 16)
> > +             *val = adc->sample.raw.u32;
> > +     else
> > +             *val = adc->sample.raw.u16;
> > +
>
> Will this work both in big vs little endian archs? I don't think so but maybe
> I'm missing something. At a first glance, it seems we get big endian from spi so
> shouldn't we have __be16 and __be32?

Yes, in Linux SPI words are always CPU-endian. It is the drivers that
use 8-bit transfers to read 16 bits that need to handle big-endian
swapping. But here we are using 14/16/18 bit transfers.

>
> > +     if (chan->scan_type.sign == 's')
> > +             *val = sign_extend32(*val, chan->scan_type.realbits - 1);
> > +
> > +     return IIO_VAL_INT;
> > +}
> > +
> > +static int ad7944_read_raw(struct iio_dev *indio_dev,
> > +                        const struct iio_chan_spec *chan,
> > +                        int *val, int *val2, long info)
> > +{
> > +     struct ad7944_adc *adc = iio_priv(indio_dev);
> > +     int ret;
> > +
> > +     switch (info) {
> > +     case IIO_CHAN_INFO_RAW:
> > +             ret = iio_device_claim_direct_mode(indio_dev);
> > +             if (ret)
> > +                     return ret;
> > +
>
> I'm not totally sure but I think Jonathan already merged his series for the
> cleanup stuff for the claim direct mode. Maybe take a look and use it? Not a big
> win in here but I guess we could still reduce some LOC.

Yes, if it is merged already, happy to make use of it here.

>
> > +             ret = ad7944_single_conversion(adc, chan, val);
> > +             iio_device_release_direct_mode(indio_dev);
> > +             return ret;
> > +
> > +     case IIO_CHAN_INFO_SCALE:
> > +             switch (chan->type) {
> > +             case IIO_VOLTAGE:
> > +                     *val = adc->ref_mv;
> > +                     *val2 = chan->scan_type.realbits;
> > +
> > +                     return IIO_VAL_FRACTIONAL_LOG2;
> > +             default:
> > +                     return -EINVAL;
> > +             }
> > +
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}
> > +
> > +static const struct iio_info ad7944_iio_info = {
> > +     .read_raw = &ad7944_read_raw,
> > +};
> > +
> > +static irqreturn_t ad7944_trigger_handler(int irq, void *p)
> > +{
> > +     struct iio_poll_func *pf = p;
> > +     struct iio_dev *indio_dev = pf->indio_dev;
> > +     struct ad7944_adc *adc = iio_priv(indio_dev);
> > +     int ret;
> > +
> > +     ret = ad7944_4_wire_mode_conversion(adc, &indio_dev->channels[0]);
> > +     if (ret)
> > +             goto out;
> > +
> > +     iio_push_to_buffers_with_timestamp(indio_dev, &adc->sample.raw,
> > +                                        indio_dev->scan_timestamp);
> > +
> > +out:
> > +     iio_trigger_notify_done(indio_dev->trig);
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static const char * const ad7944_power_supplies[] = {
> > +     "avdd", "dvdd", "bvdd", "vio"
> > +};
> > +
> > +static void ad7944_ref_disable(void *ref)
> > +{
> > +     regulator_disable(ref);
> > +}
> > +
> > +static int ad7944_probe(struct spi_device *spi)
> > +{
> > +     const struct ad7944_chip_info *chip_info;
> > +     struct iio_dev *indio_dev;
> > +     struct ad7944_adc *adc;
> > +     struct regulator *ref;
> > +     const char *str_val;
> > +     int ret;
> > +
> > +     /* adi,spi-mode property defaults to "4-wire" if not present */
> > +     if (device_property_read_string(&spi->dev, "adi,spi-mode", &str_val)
> > < 0)
> > +             str_val = "4-wire";
> > +
> > +     if (strcmp(str_val, "4-wire"))
> > +             return dev_err_probe(&spi->dev, -EINVAL,
> > +                                  "only \"4-wire\" mode is currently
> > supported\n");
>
> Did you looked at spi core? I guess the chain mode is not available but IIRC spi
> already has spi-3wire. So maybe you could just have a boolean property for the
> chain mode and check both that and 3wire?

I used the term "3-wire" because that is what the datasheet calls it,
but it is not the same as what the SPI core calls SPI_3WIRE. The
former is described in the DT bindings patch in this series and the
latter means that SDI and SDO are on the same pin, which is not the
case here.

>
> > +
> > +     indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +
> > +     adc = iio_priv(indio_dev);
> > +     adc->spi = spi;
> > +
> > +     chip_info = spi_get_device_match_data(spi);
> > +     if (!chip_info)
> > +             return dev_err_probe(&spi->dev, -EINVAL, "no chip info\n");
> > +
> > +     adc->t = chip_info->t;
> > +
> > +     /*
> > +      * Some chips use unusual word sizes, so check now instead of waiting
> > +      * for the first xfer.
> > +      */
> > +     if (!spi_is_bpw_supported(spi, chip_info-
> > >channels[0].scan_type.realbits))
> > +             return dev_err_probe(&spi->dev, -EINVAL,
> > +                             "SPI host does not support %d bits per
> > word\n",
> > +                             chip_info->channels[0].scan_type.realbits);
> > +
> > +     ret = devm_regulator_bulk_get_enable(&spi->dev,
> > +
> > ARRAY_SIZE(ad7944_power_supplies),
> > +                                          ad7944_power_supplies);
> > +     if (ret)
> > +             return dev_err_probe(&spi->dev, ret,
> > +                                  "failed to get and enable supplies\n");
> > +
> > +     /* adi,reference property defaults to "internal" if not present */
> > +     if (device_property_read_string(&spi->dev, "adi,reference", &str_val)
> > < 0)
> > +             str_val = "internal";
> > +
> > +     /* sort out what is being used for the reference voltage */
> > +     if (strcmp(str_val, "internal") == 0) {
>
> Maybe you can make the code neater with match_string() and some enum...

I did not know about this function. Sounds useful.

>
> - Nuno Sá
>

Re: [PATCH 2/2] iio: adc: ad7944: add driver for AD7944/AD7985/AD7986

[Nuno Sá]

On Wed, 2024-02-07 at 08:19 -0600, David Lechner wrote:
> On Wed, Feb 7, 2024 at 4:07 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > Hi David,
> > 
> > The driver It's in pretty good shape... Just some comments from me
> > 
> > On Tue, 2024-02-06 at 11:26 -0600, David Lechner wrote:
> > > This adds a driver for the Analog Devices Inc. AD7944, AD7985, and
> > > AD7986 ADCs. These are a family of pin-compatible ADCs that can sample
> > > at rates up to 2.5 MSPS.
> > > 
> > > The initial driver adds support for sampling at lower rates using the
> > > usual IIO triggered buffer and can handle all 3 possible reference
> > > voltage configurations.
> > > 
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > ---
> > >  MAINTAINERS              |   1 +
> > >  drivers/iio/adc/Kconfig  |  10 ++
> > >  drivers/iio/adc/Makefile |   1 +
> > >  drivers/iio/adc/ad7944.c | 397
> > > +++++++++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 409 insertions(+)
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 4f1e658e1e0d..83d8367595f1 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -458,6 +458,7 @@ 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
> > > +F:   drivers/iio/adc/ad7944.c
> > > 
> > >  ADAFRUIT MINI I2C GAMEPAD
> > >  M:   Anshul Dalal <anshulusr@gmail.com>
> > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > index 59ae1d17b50d..93fbe6f8e306 100644
> > > --- a/drivers/iio/adc/Kconfig
> > > +++ b/drivers/iio/adc/Kconfig
> > > @@ -280,6 +280,16 @@ config AD7923
> > >         To compile this driver as a module, choose M here: the
> > >         module will be called ad7923.
> > > 
> > > +config AD7944
> > > +     tristate "Analog Devices AD7944 and similar ADCs driver"
> > > +     depends on SPI
> > > +     help
> > > +       Say yes here to build support for Analog Devices
> > > +       AD7944, AD7985, AD7986 ADCs.
> > > +
> > > +       To compile this driver as a module, choose M here: the
> > > +       module will be called ad7944
> > > +
> > >  config AD7949
> > >       tristate "Analog Devices AD7949 and similar ADCs driver"
> > >       depends on SPI
> > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > > index 5a26ab6f1109..52d803b92cd7 100644
> > > --- a/drivers/iio/adc/Makefile
> > > +++ b/drivers/iio/adc/Makefile
> > > @@ -29,6 +29,7 @@ obj-$(CONFIG_AD7780) += ad7780.o
> > >  obj-$(CONFIG_AD7791) += ad7791.o
> > >  obj-$(CONFIG_AD7793) += ad7793.o
> > >  obj-$(CONFIG_AD7887) += ad7887.o
> > > +obj-$(CONFIG_AD7944) += ad7944.o
> > >  obj-$(CONFIG_AD7949) += ad7949.o
> > >  obj-$(CONFIG_AD799X) += ad799x.o
> > >  obj-$(CONFIG_AD9467) += ad9467.o
> > > diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
> > > new file mode 100644
> > > index 000000000000..67b525fb8e59
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/ad7944.c
> > > @@ -0,0 +1,397 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Analog Devices AD7944/85/86 PulSAR ADC family driver.
> > > + *
> > > + * Copyright 2024 Analog Devices, Inc.
> > > + * Copyright 2024 Baylibre, SAS
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/bitops.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/err.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/module.h>
> > > +#include <linux/property.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/spi/spi.h>
> > > +
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>
> > > +#include <linux/iio/trigger_consumer.h>
> > > +#include <linux/iio/triggered_buffer.h>
> > > +
> > > +#define AD7944_INTERNAL_REF_MV               4096
> > > +
> > > +struct ad7944_timing_spec {
> > > +     /* Normal mode minimum CNV pulse width in nanoseconds. */
> > > +     unsigned int cnv_ns;
> > > +     /* TURBO mode minimum CNV pulse width in nanoseconds. */
> > > +     unsigned int turbo_cnv_ns;
> > > +};
> > > +
> > > 
> > 
> > ...
> > 
> > > +}
> > > +
> > > +static int ad7944_single_conversion(struct ad7944_adc *adc,
> > > +                                 const struct iio_chan_spec *chan,
> > > +                                 int *val)
> > > +{
> > > +     int ret;
> > > +
> > > +     ret = ad7944_4_wire_mode_conversion(adc, chan);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     if (chan->scan_type.storagebits > 16)
> > > +             *val = adc->sample.raw.u32;
> > > +     else
> > > +             *val = adc->sample.raw.u16;
> > > +
> > 
> > Will this work both in big vs little endian archs? I don't think so but
> > maybe
> > I'm missing something. At a first glance, it seems we get big endian from
> > spi so
> > shouldn't we have __be16 and __be32?
> 
> Yes, in Linux SPI words are always CPU-endian. It is the drivers that
> use 8-bit transfers to read 16 bits that need to handle big-endian
> swapping. But here we are using 14/16/18 bit transfers.
> 

Right...

> > 
> > > +     if (chan->scan_type.sign == 's')
> > > +             *val = sign_extend32(*val, chan->scan_type.realbits - 1);
> > > +
> > > +     return IIO_VAL_INT;
> > > +}
> > > +
> > > +static int ad7944_read_raw(struct iio_dev *indio_dev,
> > > +                        const struct iio_chan_spec *chan,
> > > +                        int *val, int *val2, long info)
> > > +{
> > > +     struct ad7944_adc *adc = iio_priv(indio_dev);
> > > +     int ret;
> > > +
> > > +     switch (info) {
> > > +     case IIO_CHAN_INFO_RAW:
> > > +             ret = iio_device_claim_direct_mode(indio_dev);
> > > +             if (ret)
> > > +                     return ret;
> > > +
> > 
> > I'm not totally sure but I think Jonathan already merged his series for the
> > cleanup stuff for the claim direct mode. Maybe take a look and use it? Not a
> > big
> > win in here but I guess we could still reduce some LOC.
> 
> Yes, if it is merged already, happy to make use of it here.
> 
> > 
> > > +             ret = ad7944_single_conversion(adc, chan, val);
> > > +             iio_device_release_direct_mode(indio_dev);
> > > +             return ret;
> > > +
> > > +     case IIO_CHAN_INFO_SCALE:
> > > +             switch (chan->type) {
> > > +             case IIO_VOLTAGE:
> > > +                     *val = adc->ref_mv;
> > > +                     *val2 = chan->scan_type.realbits;
> > > +
> > > +                     return IIO_VAL_FRACTIONAL_LOG2;
> > > +             default:
> > > +                     return -EINVAL;
> > > +             }
> > > +
> > > +     default:
> > > +             return -EINVAL;
> > > +     }
> > > +}
> > > +
> > > +static const struct iio_info ad7944_iio_info = {
> > > +     .read_raw = &ad7944_read_raw,
> > > +};
> > > +
> > > +static irqreturn_t ad7944_trigger_handler(int irq, void *p)
> > > +{
> > > +     struct iio_poll_func *pf = p;
> > > +     struct iio_dev *indio_dev = pf->indio_dev;
> > > +     struct ad7944_adc *adc = iio_priv(indio_dev);
> > > +     int ret;
> > > +
> > > +     ret = ad7944_4_wire_mode_conversion(adc, &indio_dev->channels[0]);
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     iio_push_to_buffers_with_timestamp(indio_dev, &adc->sample.raw,
> > > +                                        indio_dev->scan_timestamp);
> > > +
> > > +out:
> > > +     iio_trigger_notify_done(indio_dev->trig);
> > > +
> > > +     return IRQ_HANDLED;
> > > +}
> > > +
> > > +static const char * const ad7944_power_supplies[] = {
> > > +     "avdd", "dvdd", "bvdd", "vio"
> > > +};
> > > +
> > > +static void ad7944_ref_disable(void *ref)
> > > +{
> > > +     regulator_disable(ref);
> > > +}
> > > +
> > > +static int ad7944_probe(struct spi_device *spi)
> > > +{
> > > +     const struct ad7944_chip_info *chip_info;
> > > +     struct iio_dev *indio_dev;
> > > +     struct ad7944_adc *adc;
> > > +     struct regulator *ref;
> > > +     const char *str_val;
> > > +     int ret;
> > > +
> > > +     /* adi,spi-mode property defaults to "4-wire" if not present */
> > > +     if (device_property_read_string(&spi->dev, "adi,spi-mode", &str_val)
> > > < 0)
> > > +             str_val = "4-wire";
> > > +
> > > +     if (strcmp(str_val, "4-wire"))
> > > +             return dev_err_probe(&spi->dev, -EINVAL,
> > > +                                  "only \"4-wire\" mode is currently
> > > supported\n");
> > 
> > Did you looked at spi core? I guess the chain mode is not available but IIRC
> > spi
> > already has spi-3wire. So maybe you could just have a boolean property for
> > the
> > chain mode and check both that and 3wire?
> 
> I used the term "3-wire" because that is what the datasheet calls it,
> but it is not the same as what the SPI core calls SPI_3WIRE. The
> former is described in the DT bindings patch in this series and the
> latter means that SDI and SDO are on the same pin, which is not the
> case here.
> 

Oh, I see... I could have looked at the bindings.

- Nuno Sá
> 

Re: [PATCH 2/2] iio: adc: ad7944: add driver for AD7944/AD7985/AD7986

[Jonathan Cameron]

> > >   
> > > > +     if (chan->scan_type.sign == 's')
> > > > +             *val = sign_extend32(*val, chan->scan_type.realbits - 1);
> > > > +
> > > > +     return IIO_VAL_INT;
> > > > +}
> > > > +
> > > > +static int ad7944_read_raw(struct iio_dev *indio_dev,
> > > > +                        const struct iio_chan_spec *chan,
> > > > +                        int *val, int *val2, long info)
> > > > +{
> > > > +     struct ad7944_adc *adc = iio_priv(indio_dev);
> > > > +     int ret;
> > > > +
> > > > +     switch (info) {
> > > > +     case IIO_CHAN_INFO_RAW:
> > > > +             ret = iio_device_claim_direct_mode(indio_dev);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +  
> > > 
> > > I'm not totally sure but I think Jonathan already merged his series for the
> > > cleanup stuff for the claim direct mode. Maybe take a look and use it? Not a
> > > big
> > > win in here but I guess we could still reduce some LOC.  
> > 
> > Yes, if it is merged already, happy to make use of it here.
It is in my tree, but I'd rather maintain some separation between
patch sets (incase I need to pull it out again for some reason).
Given the saving here is minor, we can just follow up with a patch
making the conversion after both are in place.

> >   
> > >   
> > > > +             ret = ad7944_single_conversion(adc, chan, val);
> > > > +             iio_device_release_direct_mode(indio_dev);

Re: [PATCH 2/2] iio: adc: ad7944: add driver for AD7944/AD7985/AD7986

[Jonathan Cameron]

On Tue,  6 Feb 2024 11:26:00 -0600
David Lechner <dlechner@baylibre.com> wrote:

> This adds a driver for the Analog Devices Inc. AD7944, AD7985, and
> AD7986 ADCs. These are a family of pin-compatible ADCs that can sample
> at rates up to 2.5 MSPS.
> 
> The initial driver adds support for sampling at lower rates using the
> usual IIO triggered buffer and can handle all 3 possible reference
> voltage configurations.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>


The one thing in here that will probably bite if this gets much use of
different boards is the use of non multiple of 8 word sizes.

Often we can get away with padding those with trailing clocks.
Any idea if that is safe here?

Jonathan

Re: [PATCH 2/2] iio: adc: ad7944: add driver for AD7944/AD7985/AD7986

[David Lechner]

On Sat, Feb 10, 2024 at 11:47 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue,  6 Feb 2024 11:26:00 -0600
> David Lechner <dlechner@baylibre.com> wrote:
>
> > This adds a driver for the Analog Devices Inc. AD7944, AD7985, and
> > AD7986 ADCs. These are a family of pin-compatible ADCs that can sample
> > at rates up to 2.5 MSPS.
> >
> > The initial driver adds support for sampling at lower rates using the
> > usual IIO triggered buffer and can handle all 3 possible reference
> > voltage configurations.
> >
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
>
>
> The one thing in here that will probably bite if this gets much use of
> different boards is the use of non multiple of 8 word sizes.
>
> Often we can get away with padding those with trailing clocks.
> Any idea if that is safe here?

We can probably get away with it on these chips. The ultimate goal
here, though, is to get these chips working a max sample rate which
only has a few 10s of nanoseconds of wiggle room between SPI
transfers. So I would rather have a bit more play in the timing than
try to support generic SPI controllers.

Re: [PATCH 2/2] iio: adc: ad7944: add driver for AD7944/AD7985/AD7986

[Jonathan Cameron]

On Sun, 11 Feb 2024 11:03:43 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On Sat, Feb 10, 2024 at 11:47 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue,  6 Feb 2024 11:26:00 -0600
> > David Lechner <dlechner@baylibre.com> wrote:
> >  
> > > This adds a driver for the Analog Devices Inc. AD7944, AD7985, and
> > > AD7986 ADCs. These are a family of pin-compatible ADCs that can sample
> > > at rates up to 2.5 MSPS.
> > >
> > > The initial driver adds support for sampling at lower rates using the
> > > usual IIO triggered buffer and can handle all 3 possible reference
> > > voltage configurations.
> > >
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>  
> >
> >
> > The one thing in here that will probably bite if this gets much use of
> > different boards is the use of non multiple of 8 word sizes.
> >
> > Often we can get away with padding those with trailing clocks.
> > Any idea if that is safe here?  
> 
> We can probably get away with it on these chips. The ultimate goal
> here, though, is to get these chips working a max sample rate which
> only has a few 10s of nanoseconds of wiggle room between SPI
> transfers. So I would rather have a bit more play in the timing than
> try to support generic SPI controllers.
> 
Would just be a case of providing a fallback. If you have a good spi
controller then you get better data rats.

Meh, can be added later when someone needs this. We've done that a few
times before.

Jonathan