Re: [PATCH 2/3] dt-bindings: i3c: Add Amlogic A9 I3C controller

[Junyi Zhao]

Hi Krzysztof，
	thanks for your review.

On 2026/10/19 14:00, Krzysztof Kozlowski wrote:
> [ EXTERNAL EMAIL ]
>
> On 19/10/2026 07:34, Junyi Zhao via B4 Relay wrote:
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - amlogic,i3c-a9
> > +      - amlogic,i3c-ao-a9
>
> What is ao?

AO is the Always-On domain on Amlogic SoCs (remains powered in suspend).
I've documented it in the binding description.

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    minItems: 1
> > +    maxItems: 2
> > +
> > +  clock-names:
> > +    minItems: 1
> > +    maxItems: 2
>
> Names go here

will do.

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - clock-names
> > +
> > +unevaluatedProperties: false
>
> This goes before example

will do.

> > +
> > +if:
>
> And this in allOf, see example schema or other bindings.

will do.

> > +  properties:
> > +    compatible:
> > +      const: amlogic,i3c-a9
> > +then:
> > +  properties:
> > +    clocks:
> > +      items:
> > +        - description: APB peripheral clock
> > +        - description: I3C fast clock
> > +    clock-names:
> > +      items:
> > +        - const: pclk
> > +        - const: fast_clk
>
> That's just fast
>
> minItems: 1

Will renam to "fast" and updated the driver/DTS accordingly,
and will add minItems: 1

> > +else:
> > +  properties:
> > +    clocks:
> > +      maxItems: 1
> > +    clock-names:
> > +      items:
> > +        - const: fast_clk
>
> maxItems: 1

will add maxItems: 1

> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    i3c@3a7000 {
> > +        compatible = "amlogic,i3c-a9";
> > +        reg = <0x3a7000 0x10000>;
> > +        interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>;
> > +        #address-cells = <3>;
> > +        #size-cells = <0>;
> > +        clocks = <&clkc 15>,
> > +                 <&clkc 16>;
> > +        clock-names = "pclk", "fast_clk";
> > +        status = "disabled";
> > +    };
> > +
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    i3c@22000 {
>
> Drop, one example is enough.

will delete one.

> > +        compatible = "amlogic,i3c-ao-a9";
> > +        reg = <0x22000 0x10000>;
> > +        interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> > +        #address-cells = <3>;
> > +        #size-cells = <0>;
> > +        clocks = <&clkc 17>;
> > +        clock-names = "fast_clk";
> > +        status = "disabled";
> > +    };
> > +...
>
>
> Best regards,
> Krzysztof