linux-kernel - Re: [PATCH v3 10/13] dt-bindings: iio: Add AD7091R-8

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMknhBEFnTUm9FPK-74+GkJs2d80dS+Q9WeGTxd3Ajs-JJGcdA@mail.gmail.com>
Date:   Thu, 7 Dec 2023 17:56:48 -0600
From:   David Lechner <dlechner@...libre.com>
To:     Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc:     apw@...onical.com, joe@...ches.com, dwaipayanray1@...il.com,
        lukas.bulwahn@...il.com, paul.cercueil@...log.com,
        Michael.Hennerich@...log.com, lars@...afoo.de, jic23@...nel.org,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        conor+dt@...nel.org, dan.carpenter@...aro.org,
        marcelo.schmitt1@...il.com, linux-iio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 10/13] dt-bindings: iio: Add AD7091R-8

On Thu, Dec 7, 2023 at 12:42 PM Marcelo Schmitt
<marcelo.schmitt@...log.com> wrote:
>
> Add device tree documentation for AD7091R-8.
>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> ---
>  .../bindings/iio/adc/adi,ad7091r8.yaml        | 99 +++++++++++++++++++
>  1 file changed, 99 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
> new file mode 100644
> index 000000000000..02320778f225
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
> @@ -0,0 +1,99 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7091r8.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD7091R8 8-Channel 12-Bit ADC
> +
> +maintainers:
> +  - Marcelo Schmitt <marcelo.schmitt@...log.com>
> +
> +description: |
> +  Analog Devices AD7091R-8 8-Channel 12-Bit ADC
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/AD7091R-2_7091R-4_7091R-8.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad7091r2
> +      - adi,ad7091r4
> +      - adi,ad7091r8
> +
> +  reg:
> +    maxItems: 1
> +

Missing other supplies? Like vdd-supply and vdrive-supply?

> +  vref-supply: true

refin-supply might be a better name to match the datasheet pin name.

> +
> +  adi,conversion-start-gpios:

gpios usually don't get a vendor prefix do they?

convst-gpios could be a better name to match the pin name on the datasheet.

> +    description:
> +      GPIO connected to the CONVST pin.
> +      This logic input is used to initiate conversions on the analog
> +      input channels.
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1

A description of what the interrupt is attached to (ALERT/BUSY/GPO0
pin) would be helpful.

> +
> +patternProperties:
> +  "^channel@[0-7]$":
> +    $ref: adc.yaml
> +    type: object
> +    description: Represents the external channels which are connected to the ADC.
> +
> +    properties:
> +      reg:
> +        minimum: 0
> +        maximum: 7

Shouldn't this be:

        items:
          - minimum: 0
            maximum: 7

> +
> +    required:
> +      - reg

Missing `unevaluatedProperties: false` for channels?

Bigger picture: since no other properties besides `reg` are included
here, do we actually need channel nodes?

> +
> +required:
> +  - compatible
> +  - reg
> +  - adi,conversion-start-gpios
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +  # AD7091R-2 does not have ALERT/BUSY/GPO pin
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,ad7091r4
> +              - adi,ad7091r8
> +    then:
> +      properties:
> +        interrupts: true

Interrupts is already true. Maybe better to only match chips without
interrupts and set false?

> +    else:
> +      properties:
> +        interrupts: false
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@0 {
> +                compatible = "adi,ad7091r8";
> +                reg = <0x0>;
> +                spi-max-frequency = <45454545>;
> +                vref-supply = <&adc_vref>;
> +                adi,conversion-start-gpios = <&gpio 25 GPIO_ACTIVE_LOW>;
> +                reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>;
> +                interrupts = <22 IRQ_TYPE_EDGE_FALLING>;
> +                interrupt-parent = <&gpio>;
> +        };
> +    };
> +...
> --
> 2.42.0
>
>