* Re: [PATCH v3 01/10] ASoC: dt-bindings: renesas,fsi: add support multiple clocks
[not found] ` <20260511204528.14632C2BCB0@smtp.kernel.org>
@ 2026-05-12 6:42 ` Bui Duc Phuc
2026-05-14 15:17 ` Rob Herring
0 siblings, 1 reply; 2+ messages in thread
From: Bui Duc Phuc @ 2026-05-12 6:42 UTC (permalink / raw)
To: sashiko
Cc: devicetree, wsa+renesas, robh, krzk+dt, conor+dt,
Geert Uytterhoeven, Kuninori Morimoto
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 <sashiko-bot@kernel.org> 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 <phucduc.bui@gmail.com>
>
> 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/20260510084303.122426-1-phucduc.bui@gmail.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH v3 01/10] ASoC: dt-bindings: renesas,fsi: add support multiple clocks
2026-05-12 6:42 ` [PATCH v3 01/10] ASoC: dt-bindings: renesas,fsi: add support multiple clocks Bui Duc Phuc
@ 2026-05-14 15:17 ` Rob Herring
0 siblings, 0 replies; 2+ messages in thread
From: Rob Herring @ 2026-05-14 15:17 UTC (permalink / raw)
To: Bui Duc Phuc
Cc: sashiko, devicetree, wsa+renesas, krzk+dt, conor+dt,
Geert Uytterhoeven, Kuninori Morimoto
On Tue, May 12, 2026 at 01:42:06PM +0700, Bui Duc Phuc wrote:
> 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
Between this and just giving up on enforcing an order, I pick the
latter.
Rob
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-05-14 15:17 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260510084303.122426-2-phucduc.bui@gmail.com>
[not found] ` <20260511204528.14632C2BCB0@smtp.kernel.org>
2026-05-12 6:42 ` [PATCH v3 01/10] ASoC: dt-bindings: renesas,fsi: add support multiple clocks Bui Duc Phuc
2026-05-14 15:17 ` Rob Herring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox