Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Sun, 1 May 2022 17:09:48 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Cosmin Tanislav <demonsingur@gmail.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        linux-iio@vger.kernel.org, linux-gpio@vger.kernel.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        Cosmin Tanislav <cosmin.tanislav@analog.com>
Subject: Re: [PATCH v3 1/2] dt-bindings: iio: adc: add AD4130
Message-ID: <20220501170906.02281a82@jic23-huawei>
In-Reply-To: <20220419150828.191933-2-cosmin.tanislav@analog.com>
References: <20220419150828.191933-1-cosmin.tanislav@analog.com>
        <20220419150828.191933-2-cosmin.tanislav@analog.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, 19 Apr 2022 18:08:27 +0300
Cosmin Tanislav <demonsingur@gmail.com> wrote:

> AD4130-8 is an ultra-low power, high precision, measurement solution for
> low bandwidth battery operated applications.
>=20
> The fully integrated AFE (Analog Front-End) includes a multiplexer for up
> to 16 single-ended or 8 differential inputs, PGA (Programmable Gain
> Amplifier), 24-bit Sigma-Delta ADC, on-chip reference and oscillator,
> selectable filter options, smart sequencer, sensor biasing and excitation
> options, diagnostics, and a FIFO buffer.
>=20
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
Hi Cosmin,

A few things inline to add to fixing the lack of detail Rob highlighted.

Thanks,

Jonathan

> ---
>  .../bindings/iio/adc/adi,ad4130.yaml          | 264 ++++++++++++++++++
>  1 file changed, 264 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4130.=
yaml
>=20
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml b/=
Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
> new file mode 100644
> index 000000000000..32996b62cd20
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
> @@ -0,0 +1,264 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2022 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4130.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD4130 ADC device driver
> +
> +maintainers:
> +  - Cosmin Tanislav <cosmin.tanislav@analog.com>
> +
> +description: |
> +  Bindings for the Analog Devices AD4130 ADC. Datasheet can be found her=
e:
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/=
AD4130-8.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad4130-8-16-lfcsp
> +      - adi,ad4130-8-16-wlcsp
> +      - adi,ad4130-8-24-lfcsp
> +      - adi,ad4130-8-24-wlcsp
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +    description: phandle to the master clock (mclk)
> +
> +  clock-names:
> +    items:
> +      - const: mclk
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-names:
> +    description: |
> +      Specify which interrupt pin should be configured as Data Ready / F=
IFO
> +      interrupt.
> +      Default if not supplied is dout-int.
> +    enum:
> +      - dout-int
> +      - clk
> +      - p2
> +      - dout
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  refin1-supply:
> +    description: refin1 supply. Can be used as reference for conversion.
> +
> +  refin2-supply:
> +    description: refin2 supply. Can be used as reference for conversion.
> +
> +  avdd-supply:
> +    description: AVDD voltage supply. Can be used as reference for conve=
rsion.
> +
> +  iovdd-supply:
> +    description: IOVDD voltage supply. Used for the chip interface.
> +
> +  spi-max-frequency:
> +    maximum: 5000000
> +
> +  adi,int-clk-out:
> +    description: Specify if the internal clock should be exposed on the =
CLK pin.
> +    type: boolean
> +
> +  adi,ext-clk-freq:
> +    description: Specify the frequency of the external clock.

Units?  Even better if we can map this to one of the standard unit types and
include the unit in the name.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [76800, 153600]
> +    default: 76800
> +
> +  adi,bipolar:
> +    description: Specify if the device should be used in bipolar mode.
> +    type: boolean
> +
> +  adi,vbias-pins:
> +    description: Analog inputs to apply a voltage bias of (AVDD =E2=88=
=92 AVSS) / 2 to.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    maxItems: 16
> +    items:
> +      minimum: 0
> +      maximum: 15
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +patternProperties:
> +  "^channel@([0-9]|1[0-5])$":
> +    type: object
> +    $ref: adc.yaml
> +
> +    properties:
> +      reg:
> +        description: |
> +          The channel number.

This isn't used explicitly in the driver. I'm wondering
if perhaps it should be rather than using the order in which the
child nodes are found...

The driver would then need to cope with potential holes however
(or just reject a binding where they occur?).=20

> +        items:
> +          minimum: 0
> +          maximum: 15
> +
> +      diff-channels:
> +        description: |
> +          Besides the analog inputs available, internal inputs can be us=
ed.
> +          16: Internal temperature sensor.
> +          17: AVSS
> +          18: Internal reference
> +          19: DGND
> +          20: (AVDD =E2=88=92 AVSS)/6+
> +          21: (AVDD =E2=88=92 AVSS)/6-
> +          22: (IOVDD =E2=88=92 DGND)/6+
> +          23: (IOVDD =E2=88=92 DGND)/6-
> +          24: (ALDO =E2=88=92 AVSS)/6+
> +          25: (ALDO =E2=88=92 AVSS)/6-
> +          26: (DLDO =E2=88=92 DGND)/6+
> +          27: (DLDO =E2=88=92 DGND)/6-
> +          28: V_MV_P
> +          29: V_MV_M
> +        items:
> +          minimum: 0
> +          maximum: 29
> +
> +      adi,reference-select:
> +        description: |
> +          Select the reference source to use when converting on the
> +          specific channel. Valid values are:
> +          0: REFIN1(+)/REFIN1(=E2=88=92)
> +          1: REFIN2(+)/REFIN2(=E2=88=92)
> +          2: REFOUT/AVSS (Internal reference)
> +          3: AVDD/AVSS
> +          If not specified, REFIN1 is used.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1, 2, 3]
> +        default: 0
> +
> +      adi,excitation-pin-0:
> +        description: |
> +          Analog input to apply excitation current to while the channel
> +          is active.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        minimum: 0
> +        maximum: 15
> +        default: 0
> +
> +      adi,excitation-pin-1:
> +        description: |
> +          Analog input to apply excitation current to while this channel
> +          is active.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        minimum: 0
> +        maximum: 15
> +        default: 0
> +
> +      adi,excitation-current-0-nanoamps:
> +        description: |
> +          Excitation current in nanoamps to be applied to pin specified =
in
> +          adi,excitation-pin-0 while this channel is active.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000]
> +        default: 0
> +
> +      adi,excitation-current-1-nanoamps:
> +        description: |
> +          Excitation current in nanoamps to be applied to pin specified =
in
> +          adi,excitation-pin-1 while this channel is active.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000]
> +        default: 0
> +
> +      adi,burnout-current-nanoamps:
> +        description: |
> +          Burnout current in nanoamps to be applied for this channel.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 500, 2000, 4000]
> +        default: 0
> +
> +      adi,buffered-positive:
> +        description: Enable buffered mode for positive input.
> +        type: boolean
> +
> +      adi,buffered-negative:
> +        description: Enable buffered mode for negative input.
> +        type: boolean
> +
> +    required:
> +      - reg
> +      - diff-channels
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    spi {
> +      #address-cells =3D <1>;
> +      #size-cells =3D <0>;
> +
> +      adc@0 {
> +        compatible =3D "adi,ad4130-8-24-wlcsp";
> +        reg =3D <0>;
> +
> +        #address-cells =3D <1>;
> +        #size-cells =3D <0>;
> +
> +        spi-max-frequency =3D <5000000>;
> +        interrupts =3D <27 IRQ_TYPE_EDGE_FALLING>;
> +        interrupt-parent =3D <&gpio>;
> +
> +        channel@0 {
> +          reg =3D <0>;
> +
> +          adi,reference-select =3D <2>;
> +
> +          /* AIN8, AIN9 */
> +          diff-channels =3D <8 9>;
> +        };
> +
> +        channel@1 {
> +          reg =3D <1>;
> +
> +          adi,reference-select =3D <2>;
> +
> +          /* AIN10, AIN11 */
> +          diff-channels =3D <10 11>;
> +        };
> +
> +        channel@2 {
> +          reg =3D <2>;
> +
> +          adi,reference-select =3D <2>;
> +
> +          /* Temperature Sensor, DGND */
> +          diff-channels =3D <16 19>;
> +        };
> +
> +        channel@3 {
> +          reg =3D <3>;
> +
> +          adi,reference-select =3D <2>;
> +
> +          /* Internal reference, DGND */
> +          diff-channels =3D <18 19>;
> +        };
> +
> +        channel@4 {
> +          reg =3D <4>;
> +
> +          adi,reference-select =3D <2>;
> +
> +          /* DGND, DGND */
> +          diff-channels =3D <19 19>;
> +        };
> +      };
> +    };