Re: [PATCH v2 1/2] ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Xingyu Wu <xingyu.wu@starfivetech.com>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	Claudiu Beznea <Claudiu.Beznea@microchip.com>,
	Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor.dooley@microchip.com>
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	alsa-devel@alsa-project.org, linux-sound@vger.kernel.org
Subject: Re: [PATCH v2 1/2] ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller
Date: Thu, 21 Mar 2024 09:24:14 +0100	[thread overview]
Message-ID: <9d9efb8a-0b3c-4e7a-8673-07cd3b1f5f87@linaro.org> (raw)
In-Reply-To: <20240320090239.168743-2-xingyu.wu@starfivetech.com>

On 20/03/2024 10:02, Xingyu Wu wrote:
> Add bindings for the Multi-Channel I2S controller of Cadence.
> 
> The Multi-Channel I2S (I2S-MC) implements a function of the
> 8-channel I2S bus interfasce. Each channel can become receiver
> or transmitter. Four I2S instances are used on the StarFive
> JH8100 SoC. One instance of them is limited to 2 channels, two
> instance are limited to 4 channels, and the other one can use
> most 8 channels. Add a unique property about
> 'starfive,i2s-max-channels' to distinguish each instance.
> 
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> ---
>  .../bindings/sound/cdns,i2s-mc.yaml           | 110 ++++++++++++++++++
>  1 file changed, 110 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml b/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> new file mode 100644
> index 000000000000..0a1b0122a246
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> @@ -0,0 +1,110 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/cdns,i2s-mc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cadence multi-channel I2S controller
> +
> +description:
> +  The Cadence I2S Controller implements a function of the multi-channel
> +  (up to 8-channel) bus. It combines functions of a transmitter and a receiver.
> +
> +maintainers:
> +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - cdns,i2s-mc

Why did this appear? Who asked for this? Usually these blocks are not
usable on their own.

> +      - starfive,jh8100-i2s
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    description:
> +      The interrupt line number for the I2S controller. Add this
> +      parameter if the I2S controller that you are using does not
> +      using DMA.

That's still wrong. You already got comment on this. Either you have
interrupt or not. You do not add interrupts, based on your choice or not
of having DMA. Drop the comment.

> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Bit clock
> +      - description: Main ICG clock
> +      - description: Inner master clock
> +
> +  clock-names:
> +    items:
> +      - const: bclk
> +      - const: icg
> +      - const: mclk_inner
> +
> +  resets:
> +    maxItems: 1
> +
> +  dmas:
> +    items:
> +      - description: TX DMA Channel
> +      - description: RX DMA Channel
> +    minItems: 1
> +
> +  dma-names:
> +    items:
> +      - const: tx
> +      - const: rx
> +    minItems: 1
> +
> +  "#sound-dai-cells":
> +    const: 0
> +
> +allOf:
> +  - $ref: dai-common.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: starfive,jh8100-i2s
> +    then:
> +      properties:
> +        starfive,i2s-max-channels:
> +          description:
> +            Number of I2S max stereo channels supported on the StarFive
> +            JH8100 SoC.
> +          $ref: /schemas/types.yaml#/definitions/uint32

Properties must be defined in top-level. You can disallow the property
for other variants, but I claim you won't have here other variants.

https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L212

> +          enum: [2, 4, 8]
> +      required:
> +        - starfive,i2s-max-channels
> +
> +required:

required goes after properties.

> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - resets
> +
> +oneOf:
> +  - required:
> +      - dmas
> +      - dma-names
> +  - required:
> +      - interrupts

This won't work. Provide both interrupts and dmas, and then test your DTS.

> +
> +unevaluatedProperties: false
> +

Best regards,
Krzysztof