Hi Stephen, On 19 May 2017 at 10:12, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote: > On 05/18, Chunyan Zhang wrote: >> On 18 May 2017 at 03:43, Arnd Bergmann <arnd@xxxxxxxx> wrote: >> > On Mon, May 15, 2017 at 10:35 AM, Chunyan Zhang >> > <chunyan.zhang@xxxxxxxxxxxxxx> wrote: >> >> diff --git a/Documentation/devicetree/bindings/clock/sprd/sprd,adjustable-pll-clock.txt b/Documentation/devicetree/bindings/clock/sprd/sprd,adjustable-pll-clock.txt >> >> new file mode 100644 >> >> index 0000000..476e315 >> >> --- /dev/null >> >> +++ b/Documentation/devicetree/bindings/clock/sprd/sprd,adjustable-pll-clock.txt >> >> @@ -0,0 +1,17 @@ >> >> +Spreadtrum adjustable pll clock driver >> >> + >> >> +Required properties: >> >> + >> >> +- compatible : must be one of: >> >> + "sprd,sc9836-adjustable-pll-clock" >> >> + "sprd,sc9860-adjustable-pll-clock" >> >> + >> >> +Example: >> >> + clk_mpll0: clk@40400024 { >> >> + compatible = "sprd,sc9860-adjustable-pll-clock"; >> >> + #clock-cells = <0>; >> >> + reg = <0 0x40400024 0 0x4>, >> >> + <0 0x40400028 0 0x4>; >> >> + clocks = <&clk_mpll_gates 2>; >> >> + clock-output-names = "clk_mpll0"; >> >> + }; >> > >> > The properties listed in the example must all be either >> > defined as "required" or "optional" properties and have a >> > description. >> >> Since these common properties are documented in the common clock binding [1] >> >> [1] Documentation/devicetree/bindings/clock/clock-bindings.txt >> >> I will add explanation in this file like I do in other bindings >> introduced in this patch, and will address 'reg' which probably is not >> similar to the common usage. >> >> > >> > The reg property here is a bit odd, as it lists two consecutive >> > 4-byte areas, and both are suspiciously close to a round >> > address (0x40400000), so I would guess that they are >> > in fact part of a clock controller with several registers. >> >> They are PLL clock configuration registers. >> >> Different PLL has different configurations which listed in pll_cfg.h, >> and probably has different number of registers for storing >> configurations, and on some Spreadtrum's SoCs PLL clock configuration >> registers aren't consecutive, but all PLLs on all Spreadtrum's SoCs >> (at least so far) are using the same driver. >> >> > >> > If so, please define a node for the entire clock controller, >> > the DT description should reflect the design of the hardware >> > rather than the design of your device driver. >> >> I also realized our implementation might not be easy to be understood, >> but I haven't thought out a better solution considering the hardware >> limitation I explained above. > > This binding is going down the wrong path. Please look at how > drivers such as sunxi-ng have done the binding in comparison to > original sunxi design. We don't put this level of detail into DT, > instead we put the details into the driver code and have clock > controller nodes in DT. A quick glance shows that this binding is > making a node per-clk, which is not going to be accepted. I have had a look at sunxi-ng design, I will try to rewrite the driver included in this patch-set, also DT and bindings. Thanks for your comments, Chunyan > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html