[PATCHv5 01/31] CLK: clkdev: add support for looking up clocks from DT

[Tomasz Figa]

On Saturday 03 of August 2013 19:48:05 Russell King - ARM Linux wrote:
> On Sat, Aug 03, 2013 at 08:39:34PM +0200, Tomasz Figa wrote:
> > Hi Russell,
> > 
> > On Saturday 03 of August 2013 19:35:43 Russell King - ARM Linux wrote:
> > > On Sat, Aug 03, 2013 at 04:02:36PM +0200, Tomasz Figa wrote:
> > > > > +	if (cl)
> > > > > +		return cl;
> > > > > +
> > > > > +	/* If clock was not found, attempt to look-up from DT */
> > > > > +	node = of_find_node_by_name(NULL, con_id);
> > > > 
> > > > Why are we introducing the "lookup by name" brokenness to the yet
> > > > (mostly) sane DT world?
> > > > 
> > > > We already have a good way of binding things together in DT, which
> > > > is
> > > > using phandles.
> > > > 
> > > > Not even saying that this (or something this patch relies on)
> > > > breaks
> > > > the ePAPR recommendation about node naming, which states that node
> > > > names should not be used to convey platform-specific data, but
> > > > instead should be as generic as possible to show what kind of
> > > > hardware is represented by the node.
> > > 
> > > Tell me this: many devices name their clock inputs.  You can find
> > > these
> > > input names in data sheets and the like.  These are the names
> > > defined
> > > by the hardware.  What is wrong about using those names in DT?
> > 
> > Yes, clock inputs. But this is about lookup by clock name, not clock
> > input name, as far as I can tell from the patch, based on looking for
> > a node with given name over the whole DT.
> 
> "con_id" is supposed to be the clock input name when the clock API is
> used correctly.

Right. Still, the code is looking for a node with the same name as the 
supposed input name, which seems strange to me, because clock input names 
are supposed to be defined in consumer's node, inside clock-names 
property.

> > > Remember that the second argument to clk_get() is the _clock_
> > > _input_
> > > _name_ on the _device_ passed in the first argument.  It is not
> > > *ever*
> > > some random name of the clock producer unless someone's fscked up
> > > their
> > > use of this API (in which case they're the ones with the problem.)
> > 
> > This is perfectly fine and this is how the generic common clock
> > framework DT bindings work - clock-names property is a list of clock
> > input names and clocks property contain specifiers of respective
> > clocks.
> 
> There seems to be a some pressure to do clk_get()->of_clk_get()
> conversions, and also to find ways to lookup clocks.

I don't really see any need for this kind of conversion. clk_get() already 
handles lookup using DT correctly and from drivers perspective nothing 
changes.

> It seems to me
> that no one understands how DT handles clocks.  Maybe that's where the
> problem is - lack of documentation about how DT clocks are handled.

Hmm, there is pretty much of description and examples in

Documentation/devicetree/bindings/clock/clock-bindings.txt

For me it was enough to learn how DT based clock lookup works, but I'm 
quite used to DT, so I'm not a good test sample.

Best regards,
Tomasz