Date:	Sun, 5 Feb 2012 18:05:37 +0530
From:	"Shilimkar, Santosh" <santosh.shilimkar@...com>
To:	balbi@...com
Cc:	"Varadarajan, Charulatha" <charu@...com>,
	Kevin Hilman <khilman@...com>,
	"Cousson, Benoit" <b-cousson@...com>,
	Grant Likely <grant.likely@...retlab.ca>,
	Tarun Kanti DebBarma <tarun.kanti@...com>,
	linux-omap@...r.kernel.org, tony@...mide.com,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v9 01/25] gpio/omap: remove dependency on gpio_bank_count

On Sun, Feb 5, 2012 at 5:05 PM, Felipe Balbi <balbi@...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.

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

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

There are few more fixes getting cooked up for GPIO and
as part of that series we will have a look at your suggestion.

Thanks for review.

Regards
santosh




Regards
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists