Received-SPF: pass (google.com: domain of linux-kernel+bounces-152301-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <0194ddb8-6e93-4a77-9fdd-93fc79a4e03e@salutedevices.com>
Date: Sat, 20 Apr 2024 17:48:30 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH v3 4/6] dt-bindings: clock: meson: document A1 SoC
 audio clock controller driver
Content-Language: en-US
To: Krzysztof Kozlowski <krzk@kernel.org>, Neil Armstrong
	<neil.armstrong@linaro.org>, Jerome Brunet <jbrunet@baylibre.com>, Michael
 Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, Rob
 Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor
 Dooley <conor+dt@kernel.org>, Kevin Hilman <khilman@baylibre.com>, Martin
 Blumenstingl <martin.blumenstingl@googlemail.com>, Philipp Zabel
	<p.zabel@pengutronix.de>, Jiucheng Xu <jiucheng.xu@amlogic.com>,
	<linux-amlogic@lists.infradead.org>, <linux-clk@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>
References: <20240419125812.983409-1-jan.dakinevich@salutedevices.com>
 <20240419125812.983409-5-jan.dakinevich@salutedevices.com>
 <07b1ca57-49a0-4151-99bf-caac053eaa01@kernel.org>
From: Jan Dakinevich <jan.dakinevich@salutedevices.com>
In-Reply-To: <07b1ca57-49a0-4151-99bf-caac053eaa01@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit


On 4/19/24 17:06, Krzysztof Kozlowski wrote:
> 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@salutedevices.com>
> 
> 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?

Yes exactly. Pins could be routed in any combination or could be not
routed at all. It is determined by schematics and that how external
codecs are configured.

> 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.
> 

You mean, all optional clocks should be mentioned here. Right?

>> +        };
>> +
>> +        clkc_audio_vad: clock-controller@fe054800 {
> 
> Just keep one example. It's basically almost the same.
> 

The worth of this duplication is to show how a clock from second
controller (<&clkc_audio_vad AUD_CLKID_VAD_AUDIOTOP>) is used by first
one. May be it would be better to keep it... What do you think?

> 
> 
> Best regards,
> Krzysztof
> 

-- 
Best regards
Jan Dakinevich