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

[Xingyu Wu]

> 
> 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@xxxxxxxxxxxxxxxx>
> > ---
> >  .../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@xxxxxxxxxxxxxxxx>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - cdns,i2s-mc
> 
> Why did this appear? Who asked for this? Usually these blocks are not usable on their
> own.

I wonder if I should keep the original IP compatible. Do I not need it?

> 
> > +      - 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.

Do I keep this property and drop this description?

> 
> > +    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/e
> xample-schema.yaml#L212
> 

Will fix.

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

Will fix.

> 
> > +  - 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.

I provided both properties in the DTS and test by dtbs_check. Then it printed that:
'More than one condition true in one of shema: ...'

> 
> > +
> > +unevaluatedProperties: false
> > +
> 
> Best regards,
> Krzysztof


Best regards,
Xingyu Wu