Re: [PATCH v3 01/10] ASoC: dt-bindings: renesas,fsi: add support multiple clocks

Hi all,

Based on the Sashiko AI review, I am thinking of adding the following
constraints specifically for renesas,fsi2-r8a7740 to address the
reported issues.

I think this may also help balance both Geert's and Krzysztof's
comments from the previous v2 review.

Does this approach look reasonable to you?

---------------------------------------------

-allOf:
-  - $ref: dai-common.yaml#
-
 properties:
   $nodename:
     pattern: "^sound@.*"
@@ -94,6 +91,78 @@ required:

 unevaluatedProperties: false

+allOf:
+  - $ref: dai-common.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,fsi2-r8a7740
+    then:
+      properties:
+        clock-names:
+          oneOf:
+            - items: # FSIA & FSIB is slave
+                - const: fck
+                - const: spu
+            - items: # FSIA slave & FSIB master use internal clock
+                - const: fck
+                - const: spu
+                - const: ickb
+                - const: divb
+            - items: # FSIA slave & FSIB master use external clock
+                - const: fck
+                - const: spu
+                - const: ickb
+                - const: xckb
+            - items: # FSIB slave & FSIA master use internal clock
+                - const: fck
+                - const: spu
+                - const: icka
+                - const: diva
+            - items: # FSIB slave & FSIA master use external clock
+                - const: fck
+                - const: spu
+                - const: icka
+                - const: xcka
+            - items: # FSIA master ex-clk  & FSIB master ex-clk
+                - const: fck
+                - const: spu
+                - const: icka
+                - const: xcka
+                - const: ickb
+                - const: xckb
+            - items: # FSIA master in-xlk  & FSIB master in-clk
+                - const: fck
+                - const: spu
+                - const: icka
+                - const: diva
+                - const: ickb
+                - const: divb
+            - items: # FSIA master in-clk  & FSIB master ex-clk
+                - const: fck
+                - const: spu
+                - const: icka
+                - const: diva
+                - const: ickb
+                - const: xckb
+            - items: # FSIA master ex-clk  & FSIB master in-clk
+                - const: fck
+                - const: spu
+                - const: icka
+                - const: xcka
+                - const: ickb
+                - const: divb
+            - items: # FSIA & FSIB  full clock support
+                - const: fck
+                - const: spu
+                - const: icka
+                - const: xcka
+                - const: diva
+                - const: ickb
+                - const: xckb
+                - const: divb
+
 examples:
   - |
     #include <dt-bindings/clock/r8a7740-clock.h>
@@ -102,7 +171,11 @@ examples:
             compatible = "renesas,fsi2-r8a7740", "renesas,sh_fsi2";
             reg = <0xfe1f0000 0x400>;
             interrupts = <GIC_SPI 9 0x4>;
-            clocks = <&mstp3_clks R8A7740_CLK_FSI>;
+            clocks = <&mstp3_clks R8A7740_CLK_FSI>, <&spu_clk>,
+                    <&fsia_clk>, <&fsiack_clk>, <&fsidiva_clk>,
+                    <&fsib_clk>, <&fsibck_clk>, <&fsidivb_clk>;
+            clock-names = "fck", "spu", "icka", "xcka", "diva",
+                          "ickb", "xckb", "divb";
             power-domains = <&pd_a4mp>;

-----------------------------------------------------------------------------------------


Best regard,
Phuc




On Tue, May 12, 2026 at 3:45 AM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The schema fails to enforce the mandatory SPU clock for the r8a7740 hardware variant, and the example provides a functionally broken configuration.
> - [Medium] Mismatch between strictly positional `clocks` descriptions and unordered `clock-names`, combined with missing `clock-names` enforcement.
> --
>
> commit 3adcf4902a5e62b71566ef5381f002fd5338a399
> Author: bui duc phuc <[email protected]>
>
>     ASoC: dt-bindings: renesas,fsi: add support multiple clocks
>
> This commit updates the FSI bindings to support multiple clocks to properly
> describe the hardware clock tree. It adds the SPU bus/bridge clock required
> for the r8a7740 SoC, as well as definitions for functional clocks, internal
> dividers, and external clock inputs.
>
> > diff --git a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> > index df91991699a72..c50e7115b21a0 100644
> > --- a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> > +++ b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> > @@ -38,7 +38,32 @@ properties:
> >      maxItems: 1
> >
> >    clocks:
> > -    maxItems: 1
> > +    minItems: 1
> > +    items:
> > +      - description: Main FSI module clock
> > +      - description: |
> > +          SPU bus/bridge clock. On R8A7740, this clock must be enabled to allow
> > +          register access as the FSI block is connected behind the SPU bus.
>
> Should we add an allOf block with an if-then condition to enforce the
> presence of the SPU clock for the renesas,fsi2-r8a7740 hardware variant?
>
> Without it, incomplete device trees might silently pass schema validation but
> fail catastrophically at runtime, as the commit message notes that missing
> this clock causes a system hang.
>
> The examples section of this file demonstrates this:
>
>     sh_fsi2: sound@fe1f0000 {
>             compatible = "renesas,fsi2-r8a7740", "renesas,sh_fsi2";
>             ...
>             clocks = <&mstp3_clks R8A7740_CLK_FSI>;
>
> This example only supplies a single clock and omits both the required SPU
> clock and the clock-names property, which contradicts the commit message.
>
> > +      - description: CPG DIV6 functional clocks for FSI port A
> > +      - description: CPG DIV6 functional clocks for FSI port B
> > +      - description: Internal FSI dividers for port A used for audio clock generation
> > +      - description: Internal FSI dividers for port B used for audio clock generation
> > +      - description: External clock inputs for FSI port A provided by the board
> > +      - description: External clock inputs for FSI port B provided by the board
> > +
> > +  clock-names:
> > +    minItems: 1
> > +    maxItems: 8
> > +    items:
> > +      enum:
> > +        - fck  # Main FSI module clock
> > +        - spu  # optional SPU bus/bridge clock
>
> Does defining clock-names with an enum correctly map sparse clocks?
>
> Because clocks defines an items array with eight fixed descriptions, it
> acts as a strict positional tuple. If a board provides the Main clock and
> the icka clock but omits the optional spu clock, icka shifts to index
> 1. The dt-schema tooling will then incorrectly map it to the SPU description.
>
> Additionally, using an enum here allows arbitrary ordering and does not
> enforce uniqueItems: true.
>
> Could we add clock-names to the required list when multiple clocks are
> provided? Since it is not required, a device tree could specify multiple clocks
> without names, leaving the driver unable to reliably map the phandles to their
> respective internal functions.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1