From: rob.herring@calxeda.com (Rob Herring)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup
Date: Tue, 21 Aug 2012 07:53:55 -0500 [thread overview]
Message-ID: <503384E3.4030705@calxeda.com> (raw)
In-Reply-To: <CAKGA1bnfhMC8QG9OqBbpy5iq6ZAfEgrYMg_LVd50U7yikLhDAg@mail.gmail.com>
On 08/20/2012 12:22 PM, Matt Sealey wrote:
> On Mon, Aug 20, 2012 at 10:16 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
>> On Mon, Aug 20, 2012 at 09:54:48AM -0500, Matt Sealey wrote:
>>> Yet another arbitrary array...
>>>
>>> I'm not sure why you would move the registration lookup into the DT
>>
>> Because I do not want to patch kernel every time I need a new lookup.
>> Furthermore, looking at function imx53_qsb_init in imx53-dt.c, you
>> will find that lookup for sgtl5000 is really board specific and should
>> go to device tree.
>>
>>> and use an arbitrarily ordered array again. Sure, you're looking it up
>>> entirely within the device tree here, but a better solution would be
>>> to not name clocks twice, and structure your clock definition
>>> properly.
>>>
>>> What's wrong with;
>>>
>>> ccm at 020c4000 {
>>> ...
>>> my_clock: clock at 0 {
>>> name = "my_clock_name";
>>> }
>>> }
>>>
>>> uart at nnnnnnnn {
>>> ...
>>> clocks = <&my_clock>;
>>> }
>>>
>>> ?
>>>
>> It turns a property into 185 nodes, which will just bloat the device
>> tree. This issue has been discussed for a plenty of times.
>
> It's not bloat just because it is by its very definition "a big list", is it?
>
> How do you explain duplicating the clock names in the array AND in the
> device node as NOT being bloated?
Read the clock binding doc. Names are optional and the 2 names are not
the same. One is the name of the output on the CCM and one is the name
on input to the module.
While generally optional, Shawn has chosen to require at least the
output names. In the case of defining a signal clock controller node
with lots of outputs, that is the right choice.
Rob
>
> You're going to have to define these clocks as a tree with parents and
> leaf nodes anyway in the clock subsystem. Why not define these in the
> device tree in total and reference them by handle when you build the
> entire clock tree from the ground up? Or will it just be all the
> clocks defined in Linux, but the lookups (which is what I see here)
> moved into the DT? Why not form the lookups as part of the definition
> of the clock tree?
>
WARNING: multiple messages have this Message-ID (diff)
From: Rob Herring <rob.herring@calxeda.com>
To: Matt Sealey <matt@genesi-usa.com>
Cc: Mike Turquette <mturquette@linaro.org>,
devicetree-discuss@lists.ozlabs.org,
Shawn Guo <shawn.guo@linaro.org>,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup
Date: Tue, 21 Aug 2012 07:53:55 -0500 [thread overview]
Message-ID: <503384E3.4030705@calxeda.com> (raw)
In-Reply-To: <CAKGA1bnfhMC8QG9OqBbpy5iq6ZAfEgrYMg_LVd50U7yikLhDAg@mail.gmail.com>
On 08/20/2012 12:22 PM, Matt Sealey wrote:
> On Mon, Aug 20, 2012 at 10:16 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
>> On Mon, Aug 20, 2012 at 09:54:48AM -0500, Matt Sealey wrote:
>>> Yet another arbitrary array...
>>>
>>> I'm not sure why you would move the registration lookup into the DT
>>
>> Because I do not want to patch kernel every time I need a new lookup.
>> Furthermore, looking at function imx53_qsb_init in imx53-dt.c, you
>> will find that lookup for sgtl5000 is really board specific and should
>> go to device tree.
>>
>>> and use an arbitrarily ordered array again. Sure, you're looking it up
>>> entirely within the device tree here, but a better solution would be
>>> to not name clocks twice, and structure your clock definition
>>> properly.
>>>
>>> What's wrong with;
>>>
>>> ccm@020c4000 {
>>> ...
>>> my_clock: clock@0 {
>>> name = "my_clock_name";
>>> }
>>> }
>>>
>>> uart@nnnnnnnn {
>>> ...
>>> clocks = <&my_clock>;
>>> }
>>>
>>> ?
>>>
>> It turns a property into 185 nodes, which will just bloat the device
>> tree. This issue has been discussed for a plenty of times.
>
> It's not bloat just because it is by its very definition "a big list", is it?
>
> How do you explain duplicating the clock names in the array AND in the
> device node as NOT being bloated?
Read the clock binding doc. Names are optional and the 2 names are not
the same. One is the name of the output on the CCM and one is the name
on input to the module.
While generally optional, Shawn has chosen to require at least the
output names. In the case of defining a signal clock controller node
with lots of outputs, that is the right choice.
Rob
>
> You're going to have to define these clocks as a tree with parents and
> leaf nodes anyway in the clock subsystem. Why not define these in the
> device tree in total and reference them by handle when you build the
> entire clock tree from the ground up? Or will it just be all the
> clocks defined in Linux, but the lookups (which is what I see here)
> moved into the DT? Why not form the lookups as part of the definition
> of the clock tree?
>
next prev parent reply other threads:[~2012-08-21 12:53 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-08-16 8:08 [PATCH 0/2] Move imx6q clock lookup over to device tree Shawn Guo
2012-08-16 8:08 ` Shawn Guo
2012-08-16 8:08 ` [PATCH 1/2] clk: add of_clk_src_onecell_get() support Shawn Guo
2012-08-16 8:08 ` Shawn Guo
2012-08-20 14:30 ` Shawn Guo
2012-08-20 14:30 ` Shawn Guo
2012-08-16 8:08 ` [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup Shawn Guo
2012-08-16 8:08 ` Shawn Guo
2012-08-20 12:51 ` Rob Herring
2012-08-20 12:51 ` Rob Herring
2012-08-20 14:54 ` Matt Sealey
2012-08-20 14:54 ` Matt Sealey
2012-08-20 15:16 ` Shawn Guo
2012-08-20 15:16 ` Shawn Guo
2012-08-20 17:22 ` Matt Sealey
2012-08-20 17:22 ` Matt Sealey
2012-08-21 12:27 ` Russell King - ARM Linux
2012-08-21 12:27 ` Russell King - ARM Linux
2012-08-21 13:11 ` Rob Herring
2012-08-21 13:11 ` Rob Herring
2012-08-22 8:32 ` Russell King - ARM Linux
2012-08-22 8:32 ` Russell King - ARM Linux
2012-08-22 9:27 ` Shawn Guo
2012-08-22 9:27 ` Shawn Guo
2012-08-21 12:53 ` Rob Herring [this message]
2012-08-21 12:53 ` Rob Herring
2012-08-22 22:50 ` Matt Sealey
2012-08-22 22:50 ` Matt Sealey
2012-08-23 0:08 ` Matt Sealey
2012-08-23 0:08 ` Matt Sealey
2012-08-23 1:32 ` Rob Herring
2012-08-23 1:32 ` Rob Herring
2012-08-22 2:47 ` Shawn Guo
2012-08-22 2:47 ` Shawn Guo
2012-08-19 14:05 ` [PATCH 3/4] clk: mxs: replace imx28 " Shawn Guo
2012-08-19 14:05 ` [PATCH 4/4] clk: mxs: replace imx23 " Shawn Guo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=503384E3.4030705@calxeda.com \
--to=rob.herring@calxeda.com \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.