Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <9c9485c4-78d7-4be7-a529-708f4d0f2ae8@canonical.com>
Date:   Thu, 17 Feb 2022 16:44:42 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH 2/3] dt-bindings: iio: add ADE9078
Content-Language: en-US
To:     chegbeli <ciprian.hegbeli@analog.com>, jic23@kernel.org,
        robh+dt@kernel.org, linux-iio@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220217135140.5658-1-ciprian.hegbeli@analog.com>
 <20220217135140.5658-3-ciprian.hegbeli@analog.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
In-Reply-To: <20220217135140.5658-3-ciprian.hegbeli@analog.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 17/02/2022 14:51, chegbeli wrote:
> Added device tree bindings for the ADE9078
> 
> Signed-off-by: chegbeli <ciprian.hegbeli@analog.com>
> ---
>  .../bindings/iio/meter/adi,ade9078.yaml       | 153 ++++++++++++++++++
>  include/dt-bindings/iio/meter/adi,ade9078.h   |  21 +++
>  2 files changed, 174 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml
>  create mode 100644 include/dt-bindings/iio/meter/adi,ade9078.h
> 
> diff --git a/Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml b/Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml
> new file mode 100644
> index 000000000000..e27d52e06e32
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml
> @@ -0,0 +1,153 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2021 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bindings/iio/addac/adi,ade9078.yaml#

Did you test your schema with dt_binding_check? This should fail.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADE9078 High Performance, Polyphase Energy Metering driver
> +
> +mainterners:
> +  -Ciprian Hegbeli <ciprian.hegbeli@analog.com>

Space after '-'.

> +
> +description: |
> +  The ADE9078 1 is a highly accurate, fully integrated energy
> +  metering device. Interfacing with both current transformer
> +  (CT) and Rogowski coil sensors, the ADE9078 enables users to
> +  develop a 3-phase metrology platform, which achieves high
> +  performance for Class 1 up to Class 0.2 meters.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ade9078
> +
> +    reg:
> +      maxItems: 1
> +
> +    '#address-cells':
> +      const: 1
> +
> +    '#size-cells':
> +      const: 0
> +
> +    spi-max-frequency:
> +      maximum: 1000000
> +
> +    interrupts:
> +      maxItems: 2
> +
> +    reset-gpios:
> +      description: |
> +        Must be the device tree identifier of the RESET pin. As the line is
> +        active low, it should be marked GPIO_ACTIVE_LOW.
> +      maxItems: 1
> +
> +    interrupt-names:
> +      description: |
> +        Names to be attributed to the interrupts of the device. Should be "irq0"
> +        or "irq1"

Skip description but instead list items ( - const: irq0 ...)

> +
> +    adi,wf-cap-sel:
> +      description: |
> +        This bit selects whether the waveform buffer is filled with resampled
> +        data or fixed data rate data
> +        0 - WF_RESAMPLED_DATA
> +        1 - WF_FIXED_DATA_RATE
> +      maxItems: 1
> +      minimum: 0
> +      maximum: 1

1. You need a type definition.
2. This looks like bool.
3. maxItems seems wrong here.
4. Do not describe registers and their bits but a feature of the device.

This applies to fields below as well.

> +
> +    adi,wf-mode:
> +      description: |
> +        Fixed data rate waveforms filling and trigger based modes.
> +        0 - WFB_FULL_MODE (Stop when waveform buffer is full)
> +        1 - WFB_EN_TRIG_MODE (Continuous fill—stop only on enabled trigger events)
> +        2 - WFB_CENTER_EN_TRIG_MODE (Continuous filling—center capture around enabled trigger events)
> +        3 - WFB_SVAE_EN_TRIG_MODE (Continuous fill—save event address of enabled trigger events)
> +      maxItems: 1
> +      minimum: 0
> +      maximum: 3

Everything above + this looks like an enum, so use enum, instead of min/max.

> +
> +    adi,wf-src:
> +      description: |
> +        Waveform buffer source and DREADY, data ready update rate, selection.
> +        0 - WFB_SRC_SINC4 (Sinc4 output, at 16 kSPS)
> +        1 - Reserved
> +        2 - WFB_SRC_SINC4_IIR_LPF (Sinc4 + IIR LPF output, at 4 kSPS)
> +        3 - WFB_SRC_DSP (Current and voltage channel waveform samples,processed by the DSP
> +            (xI_PCF, xV_PCF) at 4 kSPS)
> +      maxItems: 1
> +      minimum: 0
> +      maximum: 3
> +
> +    adi,wf-in-en:
> +      description: |
> +        This setting determines whether the IN waveform samples are read out of
> +        the waveform buffer through SPI.
> +        0 - WFB_IN_DISABLE
> +        1 - WFB_IN_EN
> +      maxItems: 1
> +      minimum: 0
> +      maximum: 1

Also bool.

> +
> +  required:
> +    - compatible
> +    - reg
> +    - reset-gpios
> +    - interrupts
> +    - interrupt-names
> +    - adi,wf-cap-sel
> +    - adi,wf-mode
> +    - adi,wf-src
> +    - adi,wf-in-en
> +
> +patternProperties:
> +  "^phase@[0-3]$":
> +    type: object
> +    description: |
> +      Represents the external phases which are externally connected. Each phase
> +      has a current, voltage and power component
> +
> +    properties:
> +      reg:
> +        description: |
> +          The phase represented by a number
> +          0 - Phase A
> +          1 - unused
> +          2 - Phase B
> +          3 - unused
> +          4 - Phase C
> +        maxItems: 1
> +        minimum: 0
> +        maximum: 4
> +
> +    required:
> +      - reg
> +
> +examples:
> +  - |
> +    ade9078@0 {

Generic node name. Please pick something appropriate. Maybe "meter"?

> +	compatible = "adi,ade9078";

You have entirely broken indentation here. Use four spaces for DTS example.

> +	reg = <0>;
> +	spi-max-frequency = <7000000>;
> +
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	reset-gpios = <&gpio 4 GPIO_ACTIVE_LOW>;
> +	interrupts = <2 IRQ_TYPE_EDGE_FALLING>, <3 IRQ_TYPE_EDGE_FALLING>;

You clearly did not test it... this won't work without headers.

> +	interrupt-names = "irq0", "irq1";
> +	interrupt-parent = <&gpio>;


Best regards,
Krzysztof