Re: DT binding review for Armada display subsystem

[Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Hi,

On Sunday 14 of July 2013 00:09:55 Russell King - ARM Linux wrote:
> On Sun, Jul 14, 2013 at 12:16:58AM +0200, Sylwester Nawrocki wrote:
> > On 07/13/2013 11:02 PM, Russell King - ARM Linux wrote:
> >> On Sat, Jul 13, 2013 at 10:43:29PM +0200, Sylwester Nawrocki wrote:
> >>> I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it
> >>> seems they're working on v4 with clock object reference counting.
> >>> Presumably we need both clk_get() to be taking reference on the
> >>> module and reference counted clk free, e.g. in cases where clock
> >>> provider is a hot-pluggable device. It might be too paranoid
> >>> though, I haven't seen hardware configurations where a clock source
> >>> could be unplugged safely when whole system is running.
> >> 
> >> I'm not going to accept refcounting being thrown into clk_get().  The
> >> clkdev API already has refcounting, as much as it needs to.  It just
> >> needs people to use the hooks that I provided back in 2008 when I
> >> created the clkdev API for doing _precisely_ this job.
> >> 
> >> Have a read through these commits, which backup my statement above:
> >> 
> >> 0318e693d3a56836632bf1a2cfdafb7f34bcc703 - initial commit of the
> >> clkdev API d72fbdf01fc77628c0b837d0dd2fd564fa26ede6 - converting
> >> Integrator to clkdev API
> >> 
> >> and it will show you how to do refcounting.  The common clk API just
> >> needs to stop defining __clk_get() and __clk_put() to be an empty
> >> function and implement them appropriately for it's clk
> >> implementation,
> >> like they were always meant to be.
> > 
> > Sure, I've already seen the above commits. This is how I understood it
> > as well - __clk_get(), __clk_put() need to be defined by the common
> > clk
> > API, since it is going to replace all the arch specific
> > implementations.> 
> >> __clk_get() and __clk_put() are the clk-implementation specific parts
> >> of the clkdev API, because the clkdev API is utterly divorsed from
> >> the
> >> internals of what a 'struct clk' actually is.  clkdev just treats a
> >> 'struct clk' as a completely opaque type and never bothers poking
> >> about inside it.
> > 
> > OK, but at the clock's implementation side we may need, e.g. to use
> > kref to keep track of the clock's state, and free it only when it is
> > unprepared, all its children are unregistered, etc. ? Is it not what
> > you stated in your comment to patch [1] ?
> 
> If you want to do refcounting on a clock (which you run into problems
> as I highlighted earlier in this thread) then yes, you need to use a
> kref, and take a reference in __clk_get() and drop it in __clk_put()
> to make things safe.
> 
> Whether you also take a reference on the module supplying the clock or
> not depends whether you need to keep the code around to manipulate that
> clock while there are users of it.
> 
> As I've already said - consider the possibilities with this scenario.
> Here's one:
> 
>   A clock consumer is using a clock, but the clock supplier has been
>   removed.  The clock consumer wants to change the state of the clock
>   in some way - eg, system is suspending.  clk_disable() doesn't fail,
>   but on resume, clk_enable() does... (and how many people assume that
>   clk_enable() never fails?)  And who knows what rate the clock is now
>   producing or whether it's even producing anything...
> 
> I'm not saying don't do the refcounting thing I mentioned back in June.
> I'm merely pointing out the issues that there are.  There isn't a one
> right answer here because each has their own advantages and
> disadvantages (and problems.)

What about having Mike on CC for such clock-related discussion?

Best regards,
Tomasz