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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
