On 07/01/2015 09:26 PM, Daniel Kurtz wrote: > On Thu, Jul 2, 2015 at 10:52 AM, James Liao <jamesjj.liao@xxxxxxxxxxxx> wrote: >> Hi Daniel, >> >>>> +Required Properties: >>>> + >>>> +- compatible: Should be: >>>> + - "mediatek,mt8173-imgsys", "syscon" >>>> +- #clock-cells: Must be 1 >>>> + >>>> +The imgsys controller uses the common clk binding from >>>> +Documentation/devicetree/bindings/clock/clock-bindings.txt >>>> +The available clocks are defined in dt-bindings/clock/mt*-clk.h. >>>> + >>>> +Example: >>>> + >>>> +imgsys: imgsys@15000000 { >>> Since these nodes will be supplying clocks to the rest of the system, >>> I think the "name" part of each of these should all be >>> "clock-controller", like topckgen and apmixedsys: >>> >>> imgsys: clock-controller@15000000 { >> These subsystems (and topckgen also) also contains other functions such >> as reset controller, which may be implemented in clk/mediatek/ in the >> future. It is suitable to use "clock-controller" as their name? > Hmm, > > I don't know the "right way" to do this either. > Pardon me if you've already had these discussions. > I only recently started looking at these clock nodes in detail :-). > > I think what we really have in register space is a "syscon", as > described in [0]: > [0] Documentation/devicetree/bindings/mfd/syscon.txt > > So, we can define this block of registers as a syscon: > > mmsys_syscon: syscon@14000000 { > compatible = "mediatek,mt8173-mmsys", "syscon"; > reg = <0 0x14000000 0 0x1000>; > }; > > > Then for the clock controller functionality, we create a node with a > "clock-controller" name and a "-clock" compatible, like this: > > mmsys_clock: clock-controller { > compatible = "mediatek,mt8173-mmsys-clock"; > #clock-cells = <1>; > mediatek,syscon = <&mmsys_syscon>; > }; > > You could then do: > CLK_OF_DECLARE(mtk_mmsys, "mediatek,mt8173-mmsys-clock", mtk_mmsys_init); > > > If you want to reuse the same register range for some other > functionality, we could then use a different node, with a different > compatible: > > mmsys: reset-controller { > compatible = "mediatek,mt8173-mmsys-reset"; > mediatek,syscon = <&mmsys_syscon>; > }; > > What do you think of this approach? DT nodes typically have a reg property. Not having a reg property is a good indicator of a problem with the binding. A syscon is used when you have a DT node with a reg property and some driver attached to it, but you need to poke some bits in another register region that isn't part of the reg property. Instead of having multiple nodes with two reg properties where the second one is the same, we use a phandle and a syscon. If clock-controller isn't acceptable maybe clock-reset-contoller would work? Or "power-controller"? We certainly shouldn't be making up multiple nodes for one hardware block. Of course, the subject of the patch is "bindings for clock controllers", so it may be that the registers are predominantly clock related and so the name is appropriate already. -- 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