Re: [PATCH v2 3/3] dt-bindings: clock: Modify Actions Soc clock bindings

[Andreas FÃrber]

Hi,

Am 01.07.2018 um 19:37 schrieb Manivannan Sadhasivam:
> On Sun, Jul 01, 2018 at 07:26:20PM +0200, Saravanan Sekar wrote:
>> Hi Mani
>>
>>
>> On 06/30/18 11:32, Manivannan Sadhasivam wrote:
>>> Hi Saravanan,
>>>
>>> I agree with modifying the existing binding to accomodate other
>>> SoC's of the same family. But the binding should be
>>> "actions,owl-cmu.txt" since it reflects the family name.
>>
>> Agree, will modify the name
>>
>>> Andreas, what do you think?

I concur that sx00 is insufficient. Older models were called ATMxxxx.
Unfortunately with owl- it then no longer matches the compatible, but I
wouldn't mind.

>>> On Thu, Jun 28, 2018 at 09:18:05PM +0200, Saravanan Sekar wrote:
>>>> Modify clock bindings common Actions Semi Soc family S700/S900.
>>>>
>>>> Signed-off-by: Parthiban Nallathambi <pn@xxxxxxx>
>>>> Signed-off-by: Saravanan Sekar <sravanhome@xxxxxxxxx>
>>>> ---
>>>>   ...tions,s900-cmu.txt => actions,sx00-cmu.txt} | 18 ++++++++++--------
>>>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>>>   rename Documentation/devicetree/bindings/clock/{actions,s900-cmu.txt => actions,sx00-cmu.txt} (71%)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/clock/actions,s900-cmu.txt b/Documentation/devicetree/bindings/clock/actions,sx00-cmu.txt
>>>> similarity index 71%
>>>> rename from Documentation/devicetree/bindings/clock/actions,s900-cmu.txt
>>>> rename to Documentation/devicetree/bindings/clock/actions,sx00-cmu.txt
>>>> index 93e4fb827cd6..8dc7edb4d198 100644
>>>> --- a/Documentation/devicetree/bindings/clock/actions,s900-cmu.txt
>>>> +++ b/Documentation/devicetree/bindings/clock/actions,sx00-cmu.txt
>>>> @@ -1,12 +1,14 @@
>>>> -* Actions S900 Clock Management Unit (CMU)
>>>> +* Actions S900/S700 Clock Management Unit (CMU)
>>> Same as above. Should be Actions OWL SoC's Clock Management Unit (CMU).
>>
>> sure
>>
> 
> During the review of I2C controller driver, Andreas pointed out that
> we should use Owl instead of OWL in all places and also Actions should
> be replaced by Actions Semiconductor.

Actually I was just asking to include the company name and not just say
Owl. Whether it's Actions, Actions Semi or Actions Semiconductor is not
that important to me - that would be for the Actions Semi colleagues to
comment - which I see are not in CC... Please fix that! You don't need
mp-cs I think, but the others please.

> So, please change it in relevant
> places. For this binding, title should be:
> 
> Actions Semiconductor Owl SoC's Clock Management Unit (CMU).

"SoC's" looks weird there, do we have such precedence to add it?
"Actions Owl Clock Management Unit (CMU)" might do?

Saravanan, please compare other patch titles and adapt your subjects
accordingly: "modify bindings" is not very meaningful, since a patch
always modifies something and the prefix already indicates dt-bindings
as target. "Soc" is misspelled. The time-saving information to put there
would be addition of S700.

>>>> -The Actions S900 clock management unit generates and supplies clock to various
>>>> -controllers within the SoC. The clock binding described here is applicable to
>>>> -S900 SoC.
>>>> +The Actions S900/S700 clock management unit generates and supplies clock to
>>>> +various controllers within the SoC. The clock binding described here is
>>>> +applicable to S900/S700 SoC.

"S900 and S700 SoCs"? (keep the slash above)

>>>>   Required Properties:
>>>> -- compatible: should be "actions,s900-cmu"
>>>> +- compatible: should be one of this
>>> Change to: compatible: should be one of the following:
>>
>> sure
>>
>>> Thanks,
>>> Mani
>>>
>>>> +	"actions,s900-cmu"
>>>> +	"actions,s700-cmu"

Mani, should we order alphabetically? I.e., will S500 go before S900, or
will people just always append at the end now? Not saying we have to,
but keeping it consistent across Actions bindings would be desirable.

>>>>   - reg: physical base address of the controller and length of memory mapped
>>>>     region.
>>>>   - clocks: Reference to the parent clocks ("hosc", "losc")
>>>> @@ -15,9 +17,9 @@ Required Properties:
>>>>   Each clock is assigned an identifier, and client nodes can use this identifier
>>>>   to specify the clock which they consume.
>>>> -All available clocks are defined as preprocessor macros in
>>>> -dt-bindings/clock/actions,s900-cmu.h header and can be used in device
>>>> -tree sources.
>>>> +All available clocks are defined as preprocessor macros in corresponding
>>>> +dt-bindings/clock/actions,s900-cmu.h or s700-cmu.h header and can be used in

actions,s700-cmu.h for humans please or at least ...,s700-cmu.h.

>>>> +device tree sources.
>>>>   External clocks:

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 NÃrnberg, Germany
GF: Felix ImendÃrffer, Jane Smithard, Graham Norton
HRB 21284 (AG NÃrnberg)