Re: [PATCH v3 01/10] 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:
- [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