Re: [RFC PATCH v3 4/6] dt-bindings: clock: meson: document A1 SoC audio clock controller driver

[Krzysztof Kozlowski]

On 19/04/2024 14:58, Jan Dakinevich wrote:
> Add device tree bindings for A1 SoC audio clock and reset controllers.
> 
> Signed-off-by: Jan Dakinevich <jan.dakinevich@xxxxxxxxxxxxxxxxx>

This is still RFC, so not ready.

Limited, incomplete review follows. Full review will be provided when
the work is ready.

Drop "driver" references, e.g. from subject. Bindings are about hardware.


...

> +
> +  clocks:
> +    maxItems: 26
> +    items:
> +      - description: input main peripheral bus clock
> +      - description: input dds_in
> +      - description: input fixed pll div2
> +      - description: input fixed pll div3
> +      - description: input hifi_pll
> +      - description: input oscillator (usually at 24MHz)
> +    additionalItems:
> +      oneOf:
> +        - description: slv_sclk[0-9] - slave bit clocks provided by external components
> +        - description: slv_lrclk[0-9]- slave sample clocks provided by external components

What does it mean the clocks are optional? Pins could be not routed?
It's really rare case that clocks within the SoC are optional, so every
such case is questionable.


> +
> +  clock-names:
> +    maxItems: 26
> +    items:
> +      - const: pclk
> +      - const: dds_in
> +      - const: fclk_div2
> +      - const: fclk_div3
> +      - const: hifi_pll
> +      - const: xtal
> +    additionalItems:
> +      oneOf:
> +        - pattern: "^slv_sclk[0-9]$"
> +        - pattern: "^slv_lrclk[0-9]$"
> +
> +required:
> +  - compatible
> +  - '#clock-cells'
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: amlogic,a1-audio-clkc
> +    then:
> +      required:
> +        - '#reset-cells'
> +    else:
> +      properties:
> +        '#reset-cells': false
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/amlogic,a1-pll-clkc.h>
> +    #include <dt-bindings/clock/amlogic,a1-peripherals-clkc.h>
> +    #include <dt-bindings/clock/amlogic,a1-audio-clkc.h>
> +    audio {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        clkc_audio: clock-controller@fe050000 {
> +                compatible = "amlogic,a1-audio-clkc";
> +                reg = <0x0 0xfe050000 0x0 0xb0>;
> +                #clock-cells = <1>;
> +                #reset-cells = <1>;
> +                clocks = <&clkc_audio_vad AUD_CLKID_VAD_AUDIOTOP>,
> +                         <&clkc_periphs CLKID_DDS_IN>,
> +                         <&clkc_pll CLKID_FCLK_DIV2>,
> +                         <&clkc_pll CLKID_FCLK_DIV3>,
> +                         <&clkc_pll CLKID_HIFI_PLL>,
> +                         <&xtal>;
> +                clock-names = "pclk",
> +                              "dds_in",
> +                              "fclk_div2",
> +                              "fclk_div3",
> +                              "hifi_pll",
> +                              "xtal";

Make it complete - list all clocks.

> +        };
> +
> +        clkc_audio_vad: clock-controller@fe054800 {

Just keep one example. It's basically almost the same.



Best regards,
Krzysztof