Re: ARM clock API to PowerPC

[Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>]

On Wed, 2009-08-12 at 23:28 +0100, Russell King wrote:

> On Thu, Aug 13, 2009 at 07:56:32AM +1000, Benjamin Herrenschmidt wrote:
> > Maybe we can make clock-names non-optional though in the DT as an
> > incentive not to use the simple 1-clock "NULL" path.
> 
> We used to pass names.  Everyone got the idea that they could ignore
> the struct device argument, and chaos ensued in drivers - people wanted
> to name each of their individual clk structures uniquely, and pass
> clock names, or even struct clk pointers into drivers via platform data.
> Some drivers conditionalized the clock name depending on the SoC they
> were built for in the driver code.

Hi Russell ! Thanks a lot for your feedback.

Ok. So I may have misunderstood what names were for. In my mind, those
were the name of the clock input on the HW device :-) Hence my
clock-map, which maps a clock input to a clock provider. IE. It was all
to be taken as a tuple (device,name) that defines a given clock input on
a given device.

> Providing the clkdev infrastructure (which I'll talk about in another
> email, probably tomorrow) and ensuring that single-clock drivers pass
> a NULL name has ensured that people back away from that broken kind
> of thinking.  It has certainly cut down on the code size and the
> complexity in drivers.

Ok, thanks, I need to read up on clkdev then, I've missed that bit.

> IIRC, there were some drivers shrunk by about 100 LOC by using the
> clk API as I originally intended it to be used - which clkdev
> facilitates.

Ok.

> > > It's not just the device tree, it's also the drivers which have to be
> > > able to cope with whatever random device tree that's thrown at them.
> > 
> > Well, the clocks are named. At some stage, the binding for a given
> > device will define what clock names it expects. I don't see that
> > differing from what the ARM folks do.
> 
> The difference is that I'm trying to avoid the "name each clock source
> and have each driver ask for the clock by name".  Such an approach
> at first seems simple and logical, but experience has shown that it
> eventually creates more problems as things progress.

Right. I didn't intend to name the clock sources. I intended to name the
clock -inputs- of a given device. IE. clk_get(dev, name) in my mind
meant "give me the clock provider that feeds my "name" input).

> Take a look at these two commits:
> 
> 39a80c7f379e1c1d3e63b204b8353b7381d0a3d5
> 4c5e1946b5f89c33e3bc8ed73fa7ba8f31e37cc5

Thanks, I will.

> to see how moving from a per-clk naming system to a dev+consumer naming
> allowed omap_wdt to be cleaned up.  (OMAP3 added more clk naming
> conditions in the driver, so had this cleanup not happened the driver
> would have more stuff in it.)
> 
> What I'm saying is that always passing a bunch of names has been well
> proven to lead people down the wrong path of matching only by names
> and then running into problems later.  We need drivers passing a NULL
> name to ensure that people get the right idea.  Comments in code/headers
> don't seem to work. ;(

Allright but passing a NULL doesn't help for drivers with multiple clock
inputs. IE. How do you want to deal with that ? Do you want to deprecate
the named API and instead provide a new clk_get_for_input(dev,
clk_input) (clk_input could be name or numerical ... tbd) ?

Or am I missing a piece of the puzzle ?

Cheers,
Ben.