* 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; 3+ 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] 3+ 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 2026-05-15 7:21 ` Geert Uytterhoeven 0 siblings, 1 reply; 3+ 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] 3+ messages in thread
* Re: [PATCH v3 01/10] ASoC: dt-bindings: renesas,fsi: add support multiple clocks 2026-05-14 15:17 ` Rob Herring @ 2026-05-15 7:21 ` Geert Uytterhoeven 0 siblings, 0 replies; 3+ messages in thread From: Geert Uytterhoeven @ 2026-05-15 7:21 UTC (permalink / raw) To: Rob Herring Cc: Bui Duc Phuc, sashiko, devicetree, wsa+renesas, krzk+dt, conor+dt, Kuninori Morimoto Hi Rob, On Thu, 14 May 2026 at 17:17, Rob Herring <robh@kernel.org> wrote: > On Tue, May 12, 2026 at 01:42:06PM +0700, Bui Duc Phuc wrote: > > 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. The important part is that "fck" must be first, and that the others must be unique. Isn't there a way to express that? minItems: 2 maxItems: 8 uniqueItems: true items: - const: fck - enum: [ spu, icka, ickb, diva, divb, xcka, xckb ] or does the single enum line only work if it's the sole item, and do we need 7 copies here? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-05-15 7:21 UTC | newest]
Thread overview: 3+ 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
2026-05-15 7:21 ` Geert Uytterhoeven
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox