Re: [PATCH 01/12] dt-bindings: clock: mediatek: document clk bindings for mediatek mt7986 SoC

[Sam Shih <sam.shih@xxxxxxxxxxxx>]

Hi,

On Mon, 2021-07-26 at 17:20 +0800, Chen-Yu Tsai wrote:
> Furthermore, based on the driver patch and the fact that they share
> the
> same compatible string, it seems you shouldn't need to have two
> compatible
> strings for two identical hardware blocks. The need for separate
> entries
> to have different clock names is an implementation detail. Please
> consider
> using and supporting clock-output-names.
> 
> Also, please check out the MT8195 clock driver series [1]. I'm
> guessing
> a lot of the comments apply to this one as well.
> 
> Regards
> ChenYu
> 
> [1] 
> https://urldefense.com/v3/__https://lore.kernel.org/linux-mediatek/20210616224743.5109-1-chun-jie.chen@xxxxxxxxxxxx/T/*t__;Iw!!CTRNKA9wMg0ARbw!29pb4TJiGHLvLbYJgDB2Dhf8Mpw5VU8zV-W3NrMan_RPQrtWT2EdRTyyjWpu0nZE$
>  
> 

I have organized your comments in "Mediatek MT8195 clock support"
series into the following list, reply to your here:

> dt-binding: Move the not-to-be-exposed clock to driver directory or
> simply left out
Okay, thanks for your comment, I will update this in the next patch set

> describe some of the clock relations between the various clock
> controllers
I have checked the files in
"Documentation/devicetree/bindings/arm/mediatek", It seems that all
MediaTek SoC clocks are simply described by each controller, like
"mediatek,infracfg.txt" and "mediatek,topckgen.txt", and those document
only include compatible strings information and examples.
Can we insert the clock relationship of MT7986 directlly in common
documents?
Or we should add a new "mediatek,mt7986-clock.yaml" and move compatible
strings information and example to this file, and insert clock
relationship descriptions to this file? Wouldn’t it be strange to skip
existing files and create a new one?

> external oscillator's case, the oscillator is described in the device
> tree
Yes, we have "clkxtal" in the DT, which stands for external oscillator,
All clocks in apmixedsys use "clkxtal" as the parent clock

> Dual license please (and the dts files).
Okay, thanks for your comment, I will update this in the next patch set

> Why are this and other 1:1 factor clks needed? They look like
> placeholders. Please remove them.
Okay, thanks for your comment, I will update this in the next patch set

> Merge duplicate parent instances
We have considered this in the MT7986 basic clock driver, but I will
check again. If corrections are needed, I will make changes in the next
patch set.

> Leaking clk_data if some function return fail
Okay, thanks for your comment, I will update this in the next patch set

> This file contains four drivers. They do not have depend on each
> other, and do not need to be in the same file. Please split them into
> differen files and preferably different patches
Okay, thanks for your comment, I will separate those clock drivers in
the next patch set

> Is there any particular reason for arch_initcall
We have considered this in MT7986 basic clock driver, and use
CLK_OF_DECLARE instead of arch_initcall.

Another question:
Should the clock patches in "Add basic SoC support for MediaTek mt7986"
need to be separated into another patch series, such as MT8195
"Mediatek MT8195 clock support" ? 


Regards
Sam