Received-SPF: pass (google.com: domain of linux-kernel+bounces-136154-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <cover.1712585500.git.marcelo.schmitt@analog.com> <7c877c865f0b7da28d9f1f177b3b2692b0ae20b9.1712585500.git.marcelo.schmitt@analog.com>
In-Reply-To: <7c877c865f0b7da28d9f1f177b3b2692b0ae20b9.1712585500.git.marcelo.schmitt@analog.com>
From: David Lechner <dlechner@baylibre.com>
Date: Mon, 8 Apr 2024 21:57:02 -0500
Message-ID: <CAMknhBGKNZhGbD7pQ0Z7SMCWqxqGux0LcO_wW0XGP4hLTOwNBg@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] dt-bindings: iio: adc: Add AD4000
To: Marcelo Schmitt <marcelo.schmitt@analog.com>
Cc: lars@metafoo.de, Michael.Hennerich@analog.com, jic23@kernel.org, 
	robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, 
	marcelo.schmitt1@gmail.com, linux-iio@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 8, 2024 at 9:32=E2=80=AFAM Marcelo Schmitt
<marcelo.schmitt@analog.com> wrote:
>
> Add device tree documentation for AD4000 family of ADC devices.
>
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-s=
heets/ad4000-4004-4008.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-s=
heets/ad4001-4005.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-s=
heets/ad4002-4006-4010.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-s=
heets/ad4003-4007-4011.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-s=
heets/ad4020-4021-4022.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-s=
heets/adaq4001.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-s=
heets/adaq4003.pdf
>

Suggested-by: David Lechner <dlechner@baylibre.com>

(if you still use mostly my suggestions in the end)

> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad4000.yaml          | 201 ++++++++++++++++++
>  MAINTAINERS                                   |   7 +
>  2 files changed, 208 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.=
yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/=
Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> new file mode 100644
> index 000000000000..ca06afb5149e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> @@ -0,0 +1,201 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4000.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD4000 and similar Analog to Digital Converters
> +
> +maintainers:
> +  - Marcelo Schmitt <marcelo.schmitt@analog.com>
> +
> +description: |
> +  Analog Devices AD4000 family of Analog to Digital Converters with SPI =
support.
> +  Specifications can be found at:
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/=
ad4000-4004-4008.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/=
ad4001-4005.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/=
ad4002-4006-4010.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/=
ad4003-4007-4011.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/=
ad4020-4021-4022.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/=
adaq4001.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/=
adaq4003.pdf
> +
> +$ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad4000
> +      - adi,ad4001
> +      - adi,ad4002
> +      - adi,ad4003
> +      - adi,ad4004
> +      - adi,ad4005
> +      - adi,ad4006
> +      - adi,ad4007
> +      - adi,ad4008
> +      - adi,ad4010
> +      - adi,ad4011
> +      - adi,ad4020
> +      - adi,ad4021
> +      - adi,ad4022
> +      - adi,adaq4001
> +      - adi,adaq4003
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 102040816 # for VIO > 2.7 V, 81300813 for VIO > 1.7 V
> +
> +  spi-cpha: true
> +
> +  adi,spi-mode:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ single, chain ]

It sounds like there are more possible wiring configurations for these
chips that I thought when suggesting reusing this binding from AD7944
so we probably need more options here. (see my reply to the cover
letter for the complete context of these remarks)

We identified A) an additional wiring configuration where SDI of the
ADC chip is wired to SDO of the SPI controller and B) a potential need
to pin mux between wiring modes to work around SPI controller
limitations perhaps we could omit the adi,spi-mode property and just
use the standard pinctrl properties.

  pinctrl-names:
    description: |
      Names for possible ways the SDI line of the controller is wired.

      * default: The SDI line of the ADC is connected to the SDO line of th=
e
        SPI controller.  CNV line of the ADC is connected to CS of the SPI
        controller.
      * single: The datasheet calls this "3-wire mode".  (NOTE: The datashe=
et's
        definition of 3-wire mode is NOT at all related to the standard
        spi-3wire property!)  In this mode, SDI is tied to VIO, and the CNV=
 line
        can be connected to the CS line of the SPI controller (typical) or =
to a
        GPIO, in which case the CS line of the controller is unused.  The S=
DO
        line of the SPI controller is not connected.
      * multi: The datasheet calls this "4-wire mode" and is used when mult=
iple
        chips are connected in parallel.  In this mode, the ADC SDI line is=
 tied
        to the CS line on the SPI controller and the CNV line is connected =
to
        a GPIO.  The SDO line of the SPI controller is not connected.
      * chain: The datasheet calls this "chain mode".  This mode is used to=
 save
        on wiring when multiple ADCs are used.  In this mode, the SDI line =
of
        one chip is tied to the SDO of the next chip in the chain and the S=
DI of
        the last chip in the chain is tied to GND.  Only the first chip in =
the
        chain is connected to the SPI bus.  The CNV line of all chips are t=
ied
        together.  The CS line of the SPI controller can be used as the CNV=
 line
        only if it is active high.

      If one name is specified, it is assumed the chip is hard-wired in thi=
s
      configuration.

      If two names are specified, it is assumed that a pinmux can switch be=
tween
      the two wiring configurations.  The first is the default mode for rea=
ding
      and writing registers on the chip and the second is the mode for read=
ing
      the conversion data from the chip.
    oneOf:
      - items:
          - enum:
            - default
            - single
            - multi
            - chain
      - items:
          - const: default
          - enum:
            - single
            - multi
            - chain

  pinctrl-0:
    maxItems: 1

  pinctrl-1:
    maxItems: 1


> +    description: |
> +      This property indicates the SPI wiring configuration.
> +
> +      When this property is omitted, it is assumed that the device is us=
ing what
> +      the datasheet calls "4-wire mode". This is the conventional SPI mo=
de used
> +      when there are multiple devices on the same bus. In this mode, the=
 CNV
> +      line is used to initiate the conversion and the SDI line is connec=
ted to
> +      CS on the SPI controller.
> +
> +      When this property is present, it indicates that the device is usi=
ng one
> +      of the following alternative wiring configurations:
> +
> +      * single: The datasheet calls this "3-wire mode". (NOTE: The datas=
heet's
> +        definition of 3-wire mode is NOT at all related to the standard
> +        spi-3wire property!) This mode is often used when the ADC is the=
 only
> +        device on the bus. In this mode, SDI is tied to VIO, and the CNV=
 line
> +        can be connected to the CS line of the SPI controller or to a GP=
IO, in
> +        which case the CS line of the controller is unused.
> +      * chain: The datasheet calls this "chain mode". This mode is used =
to save
> +        on wiring when multiple ADCs are used. In this mode, the SDI lin=
e of
> +        one chip is tied to the SDO of the next chip in the chain and th=
e SDI of
> +        the last chip in the chain is tied to GND. Only the first chip i=
n the
> +        chain is connected to the SPI bus. The CNV line of all chips are=
 tied
> +        together. The CS line of the SPI controller can be used as the C=
NV line
> +        only if it is active high.
> +
> +  '#daisy-chained-devices': true
> +
> +  vdd-supply:
> +    description: A 1.8V supply that powers the chip (VDD).
> +
> +  vio-supply:
> +    description:
> +      A 1.8V to 5.5V supply for the digital inputs and outputs (VIO).
> +
> +  ref-supply:
> +    description:
> +      A 2.5 to 5V supply for the external reference voltage (REF).
> +
> +  cnv-gpios:
> +    description:
> +      The Convert Input (CNV). This input has multiple functions. It ini=
tiates
> +      the conversions and selects the SPI mode of the device (chain or C=
S). In
> +      'single' mode, this property is omitted if the CNV pin is connecte=
d to the
> +      CS line of the SPI controller. If 'single' mode is selected and th=
is GPIO
> +      is provided, it must be active low.

Since the conversion is triggered on the low to high transition of
CNV, I think it only makes sense to have it active high and not active
low.

> +    maxItems: 1
> +
> +  adi,high-z-input:
> +    type: boolean
> +    description:
> +      High-Z mode allows the amplifier and RC filter in front of the ADC=
 to be
> +      chosen based on the signal bandwidth of interest, rather than the =
settling
> +      requirements of the switched capacitor SAR ADC inputs.
> +
> +  adi,gain-milli:
> +    description: |
> +      The hardware gain applied to the ADC input (in milli units).
> +      The gain provided by the ADC input scaler is defined by the hardwa=
re
> +      connections between chip pins OUT+, R1K-, R1K1-, R1K+, R1K1+, and =
OUT-.
> +      If not present, default to 1000 (no actual gain applied).
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [454, 909, 1000, 1900]
> +    default: 1000

Same suggestion as in V1 - we should make it clear that this property
only applies to ADAQ chips (in the description and also a -if: for the
bindings validator). Also, looking at the datasheet, it looks like
there are a lot more pins on the ADAQ chips, so I think there are more
properties missing here.

Some trivial ones:

vs-pos-supply (VS+ pin, 0 to 11V supply) and vs-neg-supply (VS- pin,
-11 to 0V supply)

pd-amp-gpios (active low) and pd-ref-gpios (active low) for optional
runtime power management.

Also the datasheet says the ADAQ chips supports "Single-ended to
differential conversion". So it seems like we might need some extra
properties to describe that case (a flag for indicating single-ended
wiring and an optional voltage supply to describe what is connected to
the negative input if it isn't tied to GND)