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
