Re: [PATCH 5/7] ASoC: renesas: add MSIOF sound Documentation

[Krzysztof Kozlowski <krzk@kernel.org>]

On 09/04/2025 03:05, Kuninori Morimoto wrote:
> Renesas MSIOF (Clock-Synchronized Serial Interface with FIFO) can work as
> both SPI and I2S. MSIOF-I2S will use Audio Graph Card/Card2 driver which
> uses Of-Graph in DT.
> 

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

A nit, subject: drop second/last, redundant "Documentation". The
"dt-bindings" prefix is already stating that these are documentation.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18


> MSIOF-SPI/I2S are using same DT compatible properties.
> MSIOF-I2S         uses Of-Graph for Audio-Graph-Card/Card2,
> MSIOF-SPI doesn't use  Of-Graph.
> 
> Adds MSIOF-I2S documentation for Sound.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  .../bindings/sound/renesas,msiof.yaml         | 112 ++++++++++++++++++
>  1 file changed, 112 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/renesas,msiof.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/renesas,msiof.yaml b/Documentation/devicetree/bindings/sound/renesas,msiof.yaml
> new file mode 100644
> index 000000000000..5173e80698fb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/renesas,msiof.yaml
> @@ -0,0 +1,112 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/renesas,msiof.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas MSIOF I2S controller
> +
> +maintainers:
> +  - Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> +
> +# sharing with MSIOF SPI
> +# see
> +# ${LINUX}/Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        pattern: "renesas,.*-msiof"
> +  required:
> +    - compatible
> +    - port

Drop entire select.

> +
> +properties:
> +  compatible:
> +    items:
> +      - const: renesas,msiof-r8a779g0   # R-Car V4H


Use expected format of all soc compatibles. It has been always: SoC-module.

> +      - const: renesas,rcar-gen4-msiof  # generic R-Car Gen4

If you have duplicated compatibles then:
1. It rarely makes sense because you claim that two different devices
are using the same compatible. Different device, different compatible.
2. Or if this is really same device, then only one schema.

> +
> +  reg:
> +    minItems: 1
> +    maxItems: 2

Drop these two.

> +    oneOf:

Why is this flexible?

> +      - items:
> +          - description: CPU and DMA engine registers
> +      - items:
> +          - description: CPU registers
> +          - description: DMA engine registers
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  dmas:
> +    minItems: 2
> +    maxItems: 4

Why flexible?

> +
> +  dma-names:
> +    minItems: 2
> +    maxItems: 4
> +    items:
> +      enum: [ tx, rx ]

How would that work? tx rx tx rx? And then driver requests 'tx' (by
name) and what is supposed to be returned?

> +
> +  port:
> +    $ref: audio-graph-port.yaml#/definitions/port-base
> +    unevaluatedProperties: false
> +    patternProperties:
> +      "^endpoint(@[0-9a-f]+)?":
> +        $ref: audio-graph-port.yaml#/definitions/endpoint-base
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - power-domains
> +  - port
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/r8a779g0-cpg-mssr.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/power/r8a779g0-sysc.h>
> +
> +    dummy-codec {
> +      compatible = "test-codec";
> +
> +      port {
> +        codec_ep: endpoint {
> +          remote-endpoint = <&msiof1_snd_ep>;
> +        };
> +      };
> +    };

Drop, not related to the binding.

> +
> +    msiof1: serial@e6ea0000 {

serial means UART controller. You need name matching the class of the
device.
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +      compatible = "renesas,msiof-r8a779g0",
> +                   "renesas,rcar-gen4-msiof";
> +      reg = <0 0xe6ea0000 0 0x0064>;
> +      interrupts = <GIC_SPI 240 IRQ_TYPE_LEVEL_HIGH>;
> +      clocks = <&cpg CPG_MOD 619>;
> +      dmas = <&dmac0 0x43>, <&dmac0 0x42>,
> +             <&dmac1 0x43>, <&dmac1 0x42>;
> +      dma-names = "tx", "rx", "tx", "rx";

So test it now - get DMA by name 'tx'. What do you get?

Also schema should fail here.

> +      power-domains = <&sysc R8A779G0_PD_ALWAYS_ON>;
> +      resets = <&cpg 619>;
> +
> +      port {
> +        msiof1_snd_ep: endpoint {
> +          remote-endpoint = <&codec_ep>;
> +        };
> +      };
> +    };


Best regards,
Krzysztof