[...]

quoted

quoted

Looking at omap_gpio_mod_init() it seems like much of its processing
could probably be done once at probe time (or at pm_runtime_get_sync
time) as well, namely setting the IRQ enable masks.

This must be called at the beginning of bank access to get reset state.
Otherwise, it would contain settings related to previous bank access.

What state may have changed between the last time a pin was released
from the bank and the next time a pin is requested in the bank?  Prior
to this patch, omap_gpio_mod_init() is called once at probe time, and
initializing irqenable masks etc. isn't reset at the beginning of bank
access, if I understand correctly.

Call to *_gpio_mod_init() is overhead if a client driver request/release
same pin(s) and no other pins in the same bank are used by others.
This is just a special case. We normally expect multiple multiple clients
Access pins within the bank. Here *_mod_init() would be called just once.
If a bank was used temporarily just during boot it is fair to reset it
Using *_mod_init() by new client driver.

If it's not actually necessary to do this on each first request to the
bank then it's not a large overhead or anything, but want to make sure
the PM operations being performed are correct.  This patch is
basically to runtime PM enable the GPIO bank when a pin is requested,
and disable when no pin is requested.  If other actions beyond the
runtime PM enable are needed, now that a runtime PM disable is added,
then it calls into question whether the enable method is missing
something.  More troubling are the PM actions performed in addition to
pm_runtime_get_sync...

Your concern is valid.
But overhead is not likely from *_gpio_request/free() for reason explained.
We have inherent overhead in runtime callbacks. But as I said, we can avoid
Many of the operations when bank is accessed the first time.

quoted

quoted

Ungating the interface clock, enabling wakeup, setting smart idle for
the module, and enabling the (old-style) system clock seem like either
one-time actions at probe, or a part of the pm_runtime_get_sync
callback.

Yes... sysconfig related settings are done by hwmod framework.
Debounce clocks are enabled in callback.

quoted

Or is there some other reason these power management actions should be
taken each time a GPIO is requested in the block (when none were
previously requested), after the runtime PM get_sync callback, but
not as part of the get_sync callback?  If so, what caused
the smart idle setting to be lost, or the interface clock gated, if
not the pm_runtime_put_sync?

I am not sure which smart idle setting you are referring to.

That's part of what the magic constant in this statement is doing:

         __raw_writew(0x0014, bank->base
                                 + OMAP1610_GPIO_SYSCONFIG);

Ok, now this is moved to init and is called only once.

quoted

Most stuffs done in *_get_sync() callback can skip first time.
Omap_gpio_mod_init() does resetting irqenable settings to reset value.
This should not be part of callback.

To be clear, it seems like resetting irqenable settings should be a
one-time action at probe time. The power management actions such as
enabling debounce clocks, setting module PRCM idle protocol behavior,
and ungating interface clocks seem like they should either be one-time
probe actions, or a part of the runtime PM callbacks, or balanced with
corresponding actions when the last pin in the bank is released.
Else what caused the interface clock to gate, or the slave idle
control to change, or the debounce clock to be cut?  The only thing
added here that would do that is the pm_runtime_put_sync.  If it can
cause those actions then do other calls to pm_runtime_get_sync need to
fix these up?

Debounce clk enable/disable has to be associated with *_get/put_sync().
Idle mode setting and interface clock gating is a one time operation.
This is done during init. We can however look at optimization of the
Callbacks.

Anyhow, mainly wanted to point that out as a possible optimization.
It's pretty unlikely there's an actual problem here.

Ok, thanks.
--
Tarun

Todd