Re: [PATCH v2 3/6] clk: hi6220: Document devicetree bindings for hi6220 clock

[Bintian]

Hello Arnd,

On 2015/4/14 18:19, Arnd Bergmann wrote:
> On Tuesday 14 April 2015 11:35:26 Bintian wrote:
> > Hello Arnd,
> >
> > On 2015/4/13 23:32, Arnd Bergmann wrote:
> > > On Monday 13 April 2015 17:17:37 Bintian Wang wrote:
> > > > +- compatible: the compatible should be one of the following strings to
> > > > +       indicate the clock controller functionality.
> > > > +
> > > > +       - "hisilicon,aoctrl"
> > > > +       - "hisilicon,sysctrl"
> > > > +       - "hisilicon,mediactrl"
> > > > +       - "hisilicon,pmctrl"
> > > > +
> > >
> > > These ones already have bindings, you can't reuse the strings.
> > > Please work with someone in hisilicon to set up a registry of
> > > device names so you can avoid conflicts in the future.
> > All the clock registers are under above four system controllers,
> > discussed with Mark and Haojian two months ago, I think using above
> > same four binding strings is enough for clk module.
> > On second thoughts, there really some problems for future hisilicon
> > code upstream, how about change back to the first version of this
> > patch set, just like following:
> > +		sys_ctrl: sys_ctrl {
> > +			compatible = "hisilicon,sysctrl", "syscon";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			reg = <0x0 0xf7030000 0x0 0x2000>;
> > +			ranges = <0 0x0 0xf7030000 0x2000>;
> > +
> > +			clock_sys: clock1@0 {
> > +				compatible = "hisilicon,hi6220-clock-sys";
> > +				reg = <0 0x1000>;
> > +				#clock-cells = <1>;
> > +			};
> > +		};
>
> Sub-nodes are fine, but you also can't have a device node that
> is just 'compatible = "hisilicon,sysctrl", "syscon";' when
> hisilicon,sysctrl can refer to multiple mutually incompatible
> controllers.
>
> The minimum fix would be to mandate the string to be
> compatible = "hisilicon,hi6220-sysctrl", "hisilicon,sysctrl", "syscon";
> for this model, and use respective compatible strings for the other
> chips.
It's really a smart fix!

For the four system controllers, how about change the following strings:
+       - "hisilicon,aoctrl"
+       - "hisilicon,sysctrl"
+       - "hisilicon,mediactrl"
+       - "hisilicon,pmctrl"

to
+       - "hisilicon,hi6220-aoctrl"
+       - "hisilicon,hi6220-sysctrl"
+       - "hisilicon,hi6220-mediactrl"
+       - "hisilicon,hi6220-pmctrl"
?

and I also use "hisilicon,hi6220-xxxx" for hi6220 clk driver directly ?

> For the documentation, I think it would make sense to move that
> description to "hisilicon,sysctrl" and list all the variants of that
> component and the respective compatible strings there, rather than
> have one document per chip and list multiple components in it.
Sure,

Thanks,

Bintian
> 	Arnd
>
> .

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/