[PATCHv7 00/36] ARM: OMAP: clock data conversion to DT

[Paul Walmsley]

On Mon, 14 Oct 2013, Tero Kristo wrote:

> On 10/14/2013 02:45 AM, Paul Walmsley wrote:
>
> > 8. A few random comments about the individual clock binding data formats
> > themselves, based on a quick look:
> > 
> > A. It seems pretty odd and unnecessarily obscure to do something like
> > this:
> > 
> > dpll_usb_ck: dpll_usb_ck at ... {
> >         ...
> >         reg = <0x4a008180 0x4>, <0x4a008184 0x4>, <0x4a008188 0x4>,
> > <0x4a00818c 0x4>;
> >         ...
> > };
> >
> > It's at least self-documenting to do something like this:
> > 
> > dpll_usb_ck: dpll_usb_ck at ... {
> >         ...
> >         control-reg = < ... >;
> >         idlest-reg = < ... >;
> >         .. etc. ..
> > };
> 
> Some earlier version had something along these lines, but it was turned down.
> I had also reg-names as documentation purposes along, but this was unnecessary
> and was dropped also.
> 
> > Which itself might not even be needed, depending on how the DPLL control
> > code is implemented.  For example, if the relative offsets are always the
> > same for all OMAP4-family devices, maybe there's not even a need to
> > explicitly encode that into the DT data.
> 
> If I want to get rid of these, I need to add extra compatible strings for the
> dpll types. There are several weird register offsets for omap3/am3 devices.
> omap4/5/dra7/am4 behave more sanely.
> 
> > B. Seems like you can remove the "ti," prefix on the properties, since
> > they have no pretentions at genericity: they are specific to the
> > PRCM/CM/PRM IP block data, and registered by those drivers.
> 
> Can or should? It seems existing bindings use "ti," prefix even on non-generic
> bindings, meaning if I look at any other data in DT.

My comments in #8 are just regarding minor issues that don't seem
right.  I don't have a significant objection to staying with the
existing property names here if you think that they are important.

> > C. Looks like the patches use the property "autoidle-low" to indicate that
> > the autoidle bit should be inverted.  "Low" seems like the wrong
> > expression here - it invokes the actual voltage logic level of a hardware
> > signal, and we have no idea whether the hardware signal is using a low
> > voltage or a high voltage to express this condition.  Would suggest
> > something like 'invert-autoidle-bit' instead.
> >
> > D. Regarding "ti,index-starts-at-one", it seems best to explicitly state
> > which index starts at one.  The code mentions a "mux index" so please
> > consider renaming this something like "mux-index-starts-at-one" or
> > "one-based-mux-index"
> 
> The index is explicitly defined by the clock node where this is present. If it
> is a mux-clock, then it is for mux-index. If for divider clock, it is index
> for the divider.



- Paul