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

[Tero Kristo <t-kristo@ti.com>]

On 10/14/2013 09:27 PM, Paul Walmsley wrote:
> 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.

Ok thanks, I'll be reworking most of the other items and will hopefully 
have something ready soonish. Taking care of the register mappings is 
rather nasty thing to do.

-Tero

>>> 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
>