| Message ID | 20230802084301.134122-3-xingyu.wu@starfivetech.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add I2S support for the StarFive JH7110 SoC | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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
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
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
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 --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
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(-)