Re: [PATCH v3 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 2 potential
issues to consider:
- [Low] dt-bindings: i3c: silvaco: unwieldy per-compatible clock
  conditionals
- [Low] dt-bindings: i3c: silvaco: single-item enum and description in
  if/then
--

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

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

This commit adds the amlogic,a9-i3c and amlogic,a9-ao-i3c compatible
strings and their per-variant clock constraints to the Silvaco I3C master
Device Tree binding schema.

> diff --git a/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml b/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
> index 853092f7522d3..d892d42bee65c 100644
> --- a/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
> +++ b/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
> @@ -13,6 +13,8 @@ properties:
>    compatible:
>      oneOf:
>        - enum:
> +          - amlogic,a9-i3c
> +          - amlogic,a9-ao-i3c
>            - nuvoton,npcm845-i3c
>            - silvaco,i3c-master-v1
>        - items:
> @@ -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
> @@ -70,15 +72,35 @@ allOf:
>          compatible:
>            contains:
>              enum:
> +              - amlogic,a9-i3c
>                - nxp,imx94-i3c
>                - nxp,imx95-i3c
>      then:
>        properties:
>          clocks:
> +          minItems: 2
>            maxItems: 2
>          clock-names:
> +          minItems: 2
>            maxItems: 2

[Severity: Low]
Should the Amlogic A9 variants be split into a separate binding schema
instead of adding more per-compatible conditionals here?

Neither amlogic,a9-i3c nor amlogic,a9-ao-i3c uses the silvaco,i3c-master-v1
fallback, and accommodating the 1-clock AO variant weakens the top-level
clocks and clock-names minItems to 1 so that 3-clock, 2-clock, and 1-clock
variants all have to be constrained via separate if/then blocks in allOf.

> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - 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: Low]
Does this new if/then block pass dt_binding_check?

The dtschema meta-schema requires enum arrays to have at least two items,
so matching the single amlogic,a9-ao-i3c string under contains needs const
rather than a single-entry enum.  Also, property descriptions belong in the
top-level properties block rather than inside an if/then conditional.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-a9_i3c-v3-0-804b69b9095b@amlogic.com?part=2

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