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@... {
> > ...
> > 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@... {
> > ...
> > 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
--
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
