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

[Junyi Zhao <junyi.zhao@amlogic.com>]

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