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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing schema constraints for `renesas,fsi2-sh73a0`.
- [Medium] Device tree schema mismatch between positional `clocks` descriptions and flexible `clock-names`.
--

commit f87e156c63c67f6faedd026ab0b5578543349e89
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: dt-bindings: renesas,fsi: add support multiple clocks

This commit updates the FSI binding to support multiple clocks to properly
describe the hardware clock tree, including the SPU bus/bridge clock and
various CPG DIV6 functional clocks. It documents the clocks supported by
the FSI driver for the sh73a0 and r8a7740 variants.

> diff --git a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> index df91991699a72..803945b7f82f0 100644
> --- a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> +++ b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> @@ -9,9 +9,6 @@ title: Renesas FIFO-buffered Serial Interface (FSI)
>  maintainers:
>    - Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>  
> -allOf:
> -  - $ref: dai-common.yaml#
> -
>  properties:
>    $nodename:
>      pattern: "^sound@.*"

[ ... ]

> @@ -38,7 +35,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.
> +      - description: CPG DIV6 functional clocks for FSI port A
> +      - description: CPG DIV6 functional clocks for FSI port B
> +      - description: FSI dividers for port A used for audio clock generation
> +      - description: 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
> +        - icka # optional CPG DIV6 functional clocks for FSI port A
> +        - ickb # optional CPG DIV6 functional clocks for FSI port B
> +        - diva # optional FSI dividers for port A used for audio clock generation
> +        - divb # optional FSI dividers for port B used for audio clock generation
> +        - xcka # optional External clock inputs for FSI port A provided by the board
> +        - xckb # optional External clock inputs for FSI port B provided by the board

[ ... ]

> @@ -69,6 +91,30 @@ required:
>  
>  unevaluatedProperties: false
>  
> +allOf:
> +  - $ref: dai-common.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: renesas,fsi2-r8a7740

[Severity: Medium]
The commit message states that both the sh73a0 and r8a7740 variants define
the SPU DIV6 clock control register, but is the renesas,fsi2-sh73a0
compatible string missing from this conditional block?

If the condition restricts the clock validation rules entirely to
renesas,fsi2-r8a7740, the sh73a0 variant would be left without any
enforcement of the required fck and spu clocks, allowing improperly
configured device trees to silently pass schema validation but fail
at runtime.

> +    then:
> +      required:
> +        - clock-names
> +
> +      properties:
> +        clock-names:
> +          minItems: 2
> +          items:
> +            - const: fck
> +            - const: spu
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]

[Severity: Medium]
Does the use of an unordered enum list for clock-names create a structural
mismatch with the strictly positional descriptions defined in the clocks
array above?

Because dt-schema correlates array properties by index, a valid unordered
list of optional clocks might be mismatched with incorrect descriptions.
Additionally, defining sequential items with identical enum lists permits
invalid configurations with duplicate clock entries to pass validation.

> +
>  examples:
>    - |
>      #include <dt-bindings/clock/r8a7740-clock.h>
> @@ -77,7 +123,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";

For instance, this example places xcka at index 3. Would this violate the
strict positional schema since index 3 in the clocks array is explicitly
described as "CPG DIV6 functional clocks for FSI port B"?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609113836.45079-1-phucduc.bui@gmail.com?part=1