On Sun, Feb 05, 2012 at 06:05:37PM +0530, Shilimkar, Santosh wrote:
> On Sun, Feb 5, 2012 at 5:05 PM, Felipe Balbi <balbi@ti.com> wrote:
> > Hi,
> >
> > On Sun, Feb 05, 2012 at 02:46:19PM +0530, Shilimkar, Santosh wrote:
> >> >> bank->mod_usage check is used to take care of doing pm_runtime_get*/put* only
> >> >> if all the GPIOs in a particular bank are enabled or disabled respectively.
> >> >
> >> > and why should you care about that ? The first get will enable the
> >> > resources you need, the second get will just increase a counter and so
> >> > on. So if you have 32 gets, you will disable the module when you have 32
> >> > puts.
> >> >
> >> Am not sure if it is clear to you that the GPIO resources like clock,
> >> debounce clk are per bank wise and not per GPIO line. So doing 32
> >
> > this is just one more reason to have usage counters.
> >
> >> get/put per bank is stupid. runtime pm is for managing pm
> >
> > what's stupid is trying to use the pm usage counters as a binary flag,
> > see below.
> >
> >> resources and if the pm resource is per bank, it has to be
> >> handled per bank.
> >
> > hehe, what are you talking about Santosh ? That's the whole point of
> > reference counting. If there are 32 users for 1 resource, you want to
> > make sure that you only free the resources (clocks, or whatever resource
> > you want) after all 32 users are done with it.
> >
> > Trying to use the pm usage counter as a binary flag, that's the stupid
> > part of the OMAP GPIO driver.
> >
> > Instead of adding checks such as:
> >
> > if (module_isnt_used())
> >        enable_clocks();
> >
> > on get and:
> >
> > if (module_isnt_needed_anymore())
> >        disable_clcoks()
> >
> > on put is the most useless piece of code on that driver. Because such
> > checks are already available on PM core through usage counters. The way
> > that part of the code works is as follow:
> >
> > get() {
> >        if (pm_counter_is_zero(dev)) {
> >                atomic_inc();
> >
> >                rpm_resume();
> >        }
> > }
> >
> > put() {
> >        atomic_dec();
> >
> >        if (pm_counter_is_zero(dev)) {
> >                rpm_suspend();
> >        }
> > }
> >
> > Do you not see that you're duplicating functionality by trying to use
> > the pm counter a binary flag ?
> >
> Ahh.. Now I see your point. I miss-understood the point first time
> and thought that we have disconnect on the pm resources from
> number of GPIO perspective.
> 
> What you are saying is to use pm runtime reference counters rather
> than creating local ones for GPIO which seems to be right thing to
> do. Sorry for the noise.

no problem.

> The agressive clock cutting was tried initially without much success
> and may be we can revisit this one more time.

I think one issue will be wrt to IRQ handling. If you want pm_runtime_*
calls to be irq_safe, they will keep the parent always on, so we need to
find a way to make that part work properly, I guess.

> As per as this series is concerned, we would like to fix
> the build error pointed by Kevin and queue it for 3.4.

sure, go ahead. No problems with that at all.

-- 
balbi