Re: [PATCH v2 2/2] dt-bindings: i3c: silvaco,i3c-master: Add Amlogic A9 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dt-bindings: i3c: silvaco: unwieldy conditionals and invalid fallback
--

commit 0af7894c213f5346efb5a1f1360dff5a7a1704bb
Author: Junyi Zhao <junyi.zhao@amlogic.com>

dt-bindings: i3c: silvaco,i3c-master: Add Amlogic A9 support

This commit adds Amlogic A9 I3C controller compatibles (amlogic,a9-i3c and
amlogic,a9-ao-i3c) with a silvaco,i3c-master-v1 fallback to the Silvaco I3C
master Device Tree binding schema and adjusts the clock count constraints for
each variant.

> diff --git a/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml b/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
> index 853092f7522d3..de1fdec654a26 100644
> --- a/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
> +++ b/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
> @@ -19,6 +19,8 @@ properties:
>            - enum:
>                - nxp,imx94-i3c
>                - nxp,imx95-i3c
> +              - amlogic,a9-i3c
> +              - amlogic,a9-ao-i3c
>            - const: silvaco,i3c-master-v1
>  
>    reg:
> @@ -28,14 +30,14 @@ properties:
>      maxItems: 1
>  
>    clocks:
> -    minItems: 2
> +    minItems: 1
>      items:
>        - description: system clock
>        - description: bus clock
>        - description: other (slower) events clock
>  
>    clock-names:
> -    minItems: 2
> +    minItems: 1
>      items:
>        - const: pclk
>        - const: fast_clk
> @@ -72,12 +74,30 @@ allOf:
>              enum:
>                - nxp,imx94-i3c
>                - nxp,imx95-i3c
> +              - amlogic,a9-i3c
>      then:
>        properties:
>          clocks:
> +          minItems: 2
>            maxItems: 2
>          clock-names:
> +          minItems: 2
>            maxItems: 2
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: amlogic,a9-ao-i3c
> +    then:
> +      properties:
> +        clocks:
> +          maxItems: 1
> +          description:
> +            The "ao" variant is the controller instance in the Always-On (AO)
> +            domain, which has no separate bus clock and derives the
> +            bus clock from an internal divider.
> +        clock-names:
> +          maxItems: 1

[Severity: Medium]
With the top-level clocks and clock-names minItems lowered to 1 and a third
if-then conditional block added under allOf to handle 1-clock, 2-clock, and
3-clock variants, the per-compatible conditionals in silvaco,i3c-master.yaml
are becoming unwieldy. Since the Amlogic variants (especially the AO domain
instance with its internal bus clock divider) differ significantly from the
base binding, would it be cleaner to split the Amlogic binding into a
separate schema document rather than piling more conditionals into this file?

Also, is silvaco,i3c-master-v1 an appropriate fallback compatible for
amlogic,a9-i3c and amlogic,a9-ao-i3c?

If a device tree node for amlogic,a9-ao-i3c binds via the generic
silvaco,i3c-master-v1 compatible (which uses svc_default_drvdata without a
vendor_init callback), svc_i3c_master_probe() looks up fast_clk in
master->clks and fails with -EINVAL because fast_clk is absent from the
Device Tree and the internal clock divider at offset 0x304 is never
registered. Similarly, amlogic,a9-i3c requires Amlogic-specific fast_clk rate
setup in aml_i3c_init() and hardware quirks (SVC_I3C_QUIRK_FIFO_EMPTY,
SVC_I3C_QUIRK_FALSE_SLVSTART, and SVC_I3C_QUIRK_DAA_CORRUPT) to operate,
much like nuvoton,npcm845-i3c which does not use silvaco,i3c-master-v1 as a
fallback.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-a9_i3c-v2-0-0c474bfccffa@amlogic.com?part=2

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic