Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 19 Feb 2020 10:17:32 -0600
From:   Rob Herring <robh@kernel.org>
To:     Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Cc:     Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        alsa-devel@alsa-project.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] ASoC: dt-bindings: renesas,rsnd: switch to yaml base
 Documentation
Message-ID: <20200219161732.GB25095@bogus>
References: <87d0ahzr9d.wl-kuninori.morimoto.gx@renesas.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87d0ahzr9d.wl-kuninori.morimoto.gx@renesas.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Feb 14, 2020 at 02:12:39PM +0900, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> This patch switches from .txt base to .yaml base Document.
> It is still keeping detail explanations at .txt
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> v1 -> v2
> 	- fixup dtc error
> 
>  .../bindings/sound/renesas,rsnd.txt           | 518 -----------------
>  .../bindings/sound/renesas,rsnd.yaml          | 529 ++++++++++++++++++
>  2 files changed, 529 insertions(+), 518 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/renesas,rsnd.yaml


> diff --git a/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml b/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml
> new file mode 100644
> index 000000000000..0958255c8542
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml
> @@ -0,0 +1,529 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/renesas,rsnd.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas R-Car Sound Driver Device Tree Bindings
> +
> +maintainers:
> +  - Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> +
> +properties:
> +  $nodename:
> +    pattern: "^sound@.*"
> +
> +  compatible:
> +    oneOf:
> +      # for Gen1 SoC
> +      - items:
> +        - enum:

nit: Should be indented 2 more spaces.

> +          - renesas,rcar_sound-r8a7778 # R-Car M1A
> +          - renesas,rcar_sound-r8a7779 # R-Car H1
> +        - enum:
> +          - renesas,rcar_sound-gen1
> +      # for Gen2 SoC
> +      - items:
> +        - enum:
> +          - renesas,rcar_sound-r8a7743   # RZ/G1M
> +          - renesas,rcar_sound-r8a7744   # RZ/G1N
> +          - renesas,rcar_sound-r8a7745   # RZ/G1E
> +          - renesas,rcar_sound-r8a77470  # RZ/G1C
> +          - renesas,rcar_sound-r8a7790   # R-Car H2
> +          - renesas,rcar_sound-r8a7791   # R-Car M2-W
> +          - renesas,rcar_sound-r8a7793   # R-Car M2-N
> +          - renesas,rcar_sound-r8a7794   # R-Car E2
> +        - enum:
> +          - renesas,rcar_sound-gen2
> +      # for Gen3 SoC
> +      - items:
> +        - enum:
> +          - renesas,rcar_sound-r8a774a1  # RZ/G2M
> +          - renesas,rcar_sound-r8a774b1  # RZ/G2N
> +          - renesas,rcar_sound-r8a774c0  # RZ/G2E
> +          - renesas,rcar_sound-r8a7795   # R-Car H3
> +          - renesas,rcar_sound-r8a7796   # R-Car M3-W
> +          - renesas,rcar_sound-r8a77965  # R-Car M3-N
> +          - renesas,rcar_sound-r8a77990  # R-Car E3
> +          - renesas,rcar_sound-r8a77995  # R-Car D3
> +        - enum:
> +          - renesas,rcar_sound-gen3
> +      # for Generic
> +      - items:
> +        - enum:
> +          - renesas,rcar_sound-gen1
> +          - renesas,rcar_sound-gen2
> +          - renesas,rcar_sound-gen3
> +
> +  reg:
> +    minItems: 3 # Gen1
> +    maxItems: 5 # Gen2/Gen3
> +
> +  reg-names:
> +    description: |
> +      Should contain the register names.
> +      scu/adg/ssi              if generation1
> +      scu/adg/ssiu/ssi/audmapp if generation2/generation3
> +    minItems: 3 # Gen1
> +    maxItems: 5 # Gen2/Gen3
> +    allOf:
> +      - items:
> +          enum:
> +            - scu
> +            - adg
> +            - ssiu
> +            - ssi
> +            - audmapp

This should be 2 'oneOf' entries listing out the exact names and index.

> +
> +  "#sound-dai-cells":
> +    description: |
> +      it must be 0 if your system is using single DAI
> +      it must be 1 if your system is using multi  DAIs
> +    enum: [0, 1]
> +
> +  "#clock-cells":
> +    description: |
> +      it must be 0 if your system has audio_clkout
> +      it must be 1 if your system has audio_clkout0/1/2/3
> +    enum: [0, 1]
> +
> +  clock-frequency:
> +    description: for audio_clkout0/1/2/3
> +    $ref: /schemas/types.yaml#/definitions/uint32-array

This already has a common definition and this conflicts with that. 
'clock-frequency' is a single uint32 or uint64.

> +
> +  clkout-lr-asynchronous:
> +    description: audio_clkoutn is asynchronizes with lr-clock.
> +    $ref: /schemas/types.yaml#/definitions/flag
> +
> +  power-domains: true
> +
> +  resets:
> +    description: References to SSI resets
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +
> +  reset-names:
> +    description: List of valid reset names.
> +    allOf:

Don't need allOf here.

You do need to define how many items.

> +      - items:
> +          enum:
> +            - ssi-all
> +            - ssi.9
> +            - ssi.8
> +            - ssi.7
> +            - ssi.6
> +            - ssi.5
> +            - ssi.4
> +            - ssi.3
> +            - ssi.2
> +            - ssi.1
> +            - ssi.0

Looks like a pattern.

pattern: '^ssi(-all|\.[0-9])$'


> +
> +  clocks:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array

Already has a type definition. Need to define how many.

> +    description: References to SSI/SRC/MIX/CTU/DVC/AUDIO_CLK clocks.
> +
> +  clock-names:
> +    description: List of necessary clock names.
> +    allOf:
> +      - items:
> +          enum:
> +            - ssi-all
> +            - ssi.9
> +            - ssi.8
> +            - ssi.7
> +            - ssi.6
> +            - ssi.5
> +            - ssi.4
> +            - ssi.3
> +            - ssi.2
> +            - ssi.1
> +            - ssi.0
> +            - src.9
> +            - src.8
> +            - src.7
> +            - src.6
> +            - src.5
> +            - src.4
> +            - src.3
> +            - src.2
> +            - src.1
> +            - src.0
> +            - mix.1
> +            - mix.0
> +            - ctu.1
> +            - ctu.0
> +            - dvc.1
> +            - dvc.0
> +            - clk_a
> +            - clk_b
> +            - clk_c
> +            - clk_i

Looks like a couple of patterns.

> +
> +  # For OF-graph
> +  port:
> +    description: OF-Graph subnode
> +    type: object
> +    properties:
> +      reg:
> +        $ref: /schemas/types.yaml#/definitions/uint32

No unit-address for 'port', so you don't need 'reg' here.

> +      endpoint:
> +        type: object
> +        properties:
> +          remote-endpoint:
> +            $ref: /schemas/types.yaml#/definitions/phandle-array

Assume this already has a type.

> +          dai-format:
> +            $ref: "simple-card.yaml#/definitions/format"
> +          playback:
> +            $ref: /schemas/types.yaml#/definitions/phandle-array
> +          capture:
> +            $ref: /schemas/types.yaml#/definitions/phandle-array
> +        required:
> +            - remote-endpoint
> +
> +  # For multi OF-graph
> +  ports:
> +    description: multi OF-Graph subnode
> +    type: object
> +    patternProperties:
> +      "port(@.*)?":

^port(@[0-9a-f])?$"

Perhaps there's max number of ports that's less than 0xf?

> +        $ref: "#properties/port"

Would be more simple to just always have 'ports'.

> +
> +patternProperties:
> +  "^rcar_sound,dvc$":

Not a pattern, but a fixed string...

> +    description: DVC subnode.
> +    type: object
> +    patternProperties:
> +      "dvc-.":
> +        type: object
> +        properties:
> +          dmas:
> +            $ref: /schemas/types.yaml#/definitions/phandle-array

Already has a type, just need 'maxItems: 1'.

> +          dma-names:
> +            const: "tx"
> +        required:
> +          - dmas
> +          - dma-names
> +    additionalProperties: false
> +
> +  "^rcar_sound,mix$":

Not a pattern...

> +    description: MIX subnode.
> +    type: object
> +    patternProperties:
> +      "mix-.":

foomix-bar is valid?

> +        type: object

Aren't there properties in this node?

> +    additionalProperties: false

Stopping here because the rest looks like more of the same comments.

Rob