diff mbox series

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

Message ID 20230802084301.134122-3-xingyu.wu@starfivetech.com
State Superseded
Headers show
Series Add I2S support for the StarFive JH7110 SoC | expand

Commit Message

Xingyu Wu Aug. 2, 2023, 8:42 a.m. UTC
Add the StarFive JH7110 (TX0/TX1/RX channel) SoC support in the bindings
of Designware I2S controller. The I2S controller needs two reset items
to work properly on the JH7110 SoC. And TX0 channel as master mode needs
5 clock items and TX1/RX channels as slave mode need 9 clock items on
the JH7110 SoC. The RX channel needs System Register Controller property
to enable it and other platforms do not need it.

Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
---
 .../bindings/sound/snps,designware-i2s.yaml   | 101 +++++++++++++++++-
 1 file changed, 98 insertions(+), 3 deletions(-)

Comments

Krzysztof Kozlowski Aug. 5, 2023, 9:02 p.m. UTC | #1
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
Xingyu Wu Aug. 7, 2023, 9:03 a.m. UTC | #2
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
Krzysztof Kozlowski Aug. 7, 2023, 9:17 a.m. UTC | #3
On 07/08/2023 11:03, Xingyu Wu wrote:
>>>> +  - 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.

Ah, its fine. Clocks seem to be also constrained.

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

It won't be easy to read... probably the binding should be split.
Anyway, it's fine as is, except the maxItems above.

Best regards,
Krzysztof
Xingyu Wu Aug. 7, 2023, 9:33 a.m. UTC | #4
On 2023/8/7 17:17, Krzysztof Kozlowski wrote:
> On 07/08/2023 11:03, Xingyu Wu wrote:
>>>>> +  - 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.
> 
> Ah, its fine. Clocks seem to be also constrained.

OK, I will keep it 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?
> 
> It won't be easy to read... probably the binding should be split.
> Anyway, it's fine as is, except the maxItems above.
> 

So I will keep it and just add the maxItems in next version.

Thanks,
Xingyu Wu
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/sound/snps,designware-i2s.yaml b/Documentation/devicetree/bindings/sound/snps,designware-i2s.yaml
index a970fd264b21..a5ab7f3e49b2 100644
--- a/Documentation/devicetree/bindings/sound/snps,designware-i2s.yaml
+++ b/Documentation/devicetree/bindings/sound/snps,designware-i2s.yaml
@@ -17,6 +17,9 @@  properties:
           - const: snps,designware-i2s
       - enum:
           - snps,designware-i2s
+          - starfive,jh7110-i2stx0
+          - starfive,jh7110-i2stx1
+          - starfive,jh7110-i2srx
 
   reg:
     maxItems: 1
@@ -29,15 +32,36 @@  properties:
     maxItems: 1
 
   clocks:
-    description: Sampling rate reference clock
-    maxItems: 1
+    items:
+      - description: Sampling rate reference clock
+      - description: APB clock
+      - description: Audio master clock
+      - description: Inner audio master clock source
+      - description: External audio master clock source
+      - description: Bit clock
+      - description: Left/right channel clock
+      - description: External bit clock
+      - description: External left/right channel clock
+    minItems: 1
 
   clock-names:
-    const: i2sclk
+    items:
+      - const: i2sclk
+      - const: apb
+      - const: mclk
+      - const: mclk_inner
+      - const: mclk_ext
+      - const: bclk
+      - const: lrck
+      - const: bclk_ext
+      - const: lrck_ext
+    minItems: 1
 
   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
+  - 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
+        clock-names:
+          minItems: 5
+      required:
+        - resets
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: starfive,jh7110-i2stx1
+    then:
+      properties:
+        clocks:
+          minItems: 9
+        clock-names:
+          minItems: 9
+      required:
+        - resets
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: starfive,jh7110-i2srx
+    then:
+      properties:
+        clocks:
+          minItems: 9
+        clock-names:
+          minItems: 9
+      required:
+        - resets
+        - starfive,syscon
+    else:
+      properties:
+        starfive,syscon: false
 
 required:
   - compatible