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

On 2023/8/6 5:02, Krzysztof Kozlowski wrote:

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
  

The resets of TX0/TX1/RX on JH7110 SoC are mentioned in 'else' here.

...

• • if:

•  properties:
  

•    compatible:
  

•      contains:
  

•        const: starfive,jh7110-i2stx0
  

• then:

•  properties:
  

•    clocks:
  

•      minItems: 5
  

Also maxItems

Will add.

...

•    clock-names:
  

•      minItems: 5
  

Also maxItems

Will add.

What about resets? 1 or 2 items?

Mentioned it in the 'else'. Or do you mean I should drop the 'else' and add the resets in here? And is the same for TX1 and RX?

...

•  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, Xingyu Wu