Re: [PATCH v4 3/6] clk: introduce the common clock framework

[Turquette, Mike]

On Wed, Jan 4, 2012 at 6:11 PM, Rob Herring <robherring2@xxxxxxxxx> wrote:
> On 01/04/2012 07:01 PM, Turquette, Mike wrote:
>> On Wed, Jan 4, 2012 at 6:32 AM, Rob Herring <robherring2@xxxxxxxxx> wrote:
>>> On 01/03/2012 08:15 PM, Richard Zhao wrote:
>>>> On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
>>>>> On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>>>>>> On Tue, 13 Dec 2011, Mike Turquette wrote:
>>>
>>> snip
>>>
>>>>>>> +/**
>>>>>>> + * clk_init - initialize the data structures in a struct clk
>>>>>>> + * @dev: device initializing this clk, placeholder for now
>>>>>>> + * @clk: clk being initialized
>>>>>>> + *
>>>>>>> + * Initializes the lists in struct clk, queries the hardware for the
>>>>>>> + * parent and rate and sets them both.  Adds the clk to the sysfs tree
>>>>>>> + * topology.
>>>>>>> + *
>>>>>>> + * Caller must populate clk->name and clk->flags before calling
>>>>>>
>>>>>> I'm not too happy about this construct. That leaves struct clk and its
>>>>>> members exposed to the world instead of making it a real opaque
>>>>>> cookie. I know from my own painful experience, that this will lead to
>>>>>> random fiddling in that data structure in drivers and arch code just
>>>>>> because the core code has a shortcoming.
>>>>>>
>>>>>> Why can't we make struct clk a real cookie and confine the data
>>>>>> structure to the core code ?
>>>>>>
>>>>>> That would change the init call to something like:
>>>>>>
>>>>>> struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
>>>>>>                     struct clk *parent)
>>>>>>
>>>>>> And have:
>>>>>> struct clk_hw {
>>>>>>       struct clk_hw_ops *ops;
>>>>>>       const char        *name;
>>>>>>       unsigned long     flags;
>>>>>> };
>>>>>>
>>>>>> Implementers can do:
>>>>>> struct my_clk_hw {
>>>>>>       struct clk_hw    hw;
>>>>>>       mydata;
>>>>>> };
>>>>>>
>>>>>> And then change the clk ops callbacks to take struct clk_hw * as an
>>>>>> argument.
>>>> We have to define static clocks before we adopt DT binding.
>>>> If clk is opaque and allocate memory in clk core, it'll make hard
>>>> to define static clocks. And register/init will pass a long parameter
>>>> list.
>>>
>>> DT is not a prerequisite for having dynamically created clocks. You can
>>> make clock init dynamic without DT.
>>
>> Agreed.
>>
>>> What data goes in struct clk vs. struct clk_hw could change over time.
>>> So perhaps we can start with some data in clk_hw and plan to move it to
>>> struct clk later. Even if almost everything ends up in clk_hw initially,
>>> at least the structure is in place to have common, core-only data
>>> separate from platform data.
>>
>> What is the point of this?
>
> To have a way forward. It would be nice to have a clk infrastructure
> before I retire...

Haha, agreed.

>
>> The original clk_hw was defined simply as:
>>
>> struct clk_hw {
>>         struct clk *clk;
>> };
>>
>> It's only purpose in life was as a handle for navigation between the
>> opaque struct clk and the hardware-specific struct my_clk_hw.  struct
>> clk_hw is defined in clk.h and everyone can see it.  If we're suddenly
>> OK putting clk data in this structure then why bother with an opaque
>> struct clk at all?
>>
>>> What is the actual data you need to be static and accessible to the
>>> platform code? A ptr to parent clk is the main thing I've seen for
>>> static initialization. So make the parent ptr be struct clk_hw* and
>>> allow the platforms to access.
>>
>> To answer your question on what data we're trying to expose: platform
>> code commonly needs the parent pointer and the clk rate (and by
>> extension, the rate of the parent).  For debug/error prints it is also
>> nice to have the clk name.  Generic clk flags are also conceivably
>> something that platform code might want.
>
> I agree with the need to have the parent and flags from a static init
> perspective. There's not really a good reason the others can't be
> accessed thru accessors though.
>
>> I'd like to spin the question around: if we're OK exposing some stuff
>> (in your example above, the parent pointer), then what clk data are
>> you trying to hide?
>
> Well, everything from drivers which the current clk implementations do
> hide. Catching abuse in with drivers coming in from all different trees
> and lists will be impossible.
>
> For platform code it is more fuzzy. I don't think platform code should
> be allowed to muck with prepare/enable counts for example.

So there is a clear dichotomy: drivers shouldn't be exposed to any of
it and platform code should be exposed to some of it.

How about a drivers/clk/clk-private.h which will define struct clk and
must only be included by clk drivers in drivers/clk/*?

This establishes a bright line between those things which are allowed
to know the details of struct clk and those that are not: namely that
clk drivers in drivers/clk/ may use '#include "clk-private.h"'.
Obviously struct clk is opaque to the rest of the kernel (in the same
way it has been prior to the common clk patches) and there is no need
for struct clk_hw anymore.  Also helper functions are no longer needed
for clock driver code, which I think *is* a manageable set of code to
review.  Also clk drivers must live in drivers/clk/ for this to work
(without a big ugly path in someone's #include directive somewhere).

Thoughts?

Regards,
Mike

>
> Rob
>
>>
>> Regards,
>> Mike
>>
>>>
>>> Rob
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/