Re: [PATCH v1 2/5] ASoC: dt-bindings: snps,designware-i2s: Add StarFive JH7110 SoC support

[Krzysztof Kozlowski]

On 02/08/2023 10:42, Xingyu Wu wrote:
> Add the StarFive JH7110 (TX0/TX1/RX channel) SoC support in the bindings
> of Designware I2S controller. The I2S controller needs two reset items''

Thank you for your patch. There is something to discuss/improve.

>  
>    resets:
>      items:
>        - description: Optional controller resets
> +      - description: controller reset of Sampling rate
> +    minItems: 1
>  
>    dmas:
>      items:
> @@ -51,6 +75,17 @@ properties:
>        - const: rx
>      minItems: 1
>  
> +  starfive,syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: phandle to System Register Controller sys_syscon node.
> +          - description: I2S-rx enabled control offset of SYS_SYSCONSAIF__SYSCFG register.
> +          - description: I2S-rx enabled control mask
> +    description:
> +      The phandle to System Register Controller syscon node and the I2S-rx(ADC)
> +      enabled control offset and mask of SYS_SYSCONSAIF__SYSCFG register.
> +
>  allOf:
>    - $ref: dai-common.yaml#
>    - if:
> @@ -66,6 +101,66 @@ allOf:
>        properties:
>          "#sound-dai-cells":
>            const: 0

You need to constrain clocks and resets also for all other existing
variants.

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: snps,designware-i2s
> +    then:
> +      properties:
> +        clocks:
> +          maxItems: 1
> +        clock-names:
> +          maxItems: 1
> +        resets:
> +          maxItems: 1
> +    else:
> +      properties:
> +        resets:
> +          minItems: 2
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: starfive,jh7110-i2stx0
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 5

Also maxItems

> +        clock-names:
> +          minItems: 5

Also maxItems

What about resets? 1 or 2 items?

> +      required:
> +        - resets
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: starfive,jh7110-i2stx1
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 9
> +        clock-names:
> +          minItems: 9

resets?

> +      required:
> +        - resets
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: starfive,jh7110-i2srx
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 9
> +        clock-names:
> +          minItems: 9

resets?

> +      required:
> +        - resets
> +        - starfive,syscon
> +    else:
> +      properties:
> +        starfive,syscon: false
>  
>  required:
>    - compatible

Best regards,
Krzysztof