On Mon, Jul 9, 2012 at 5:21 PM, Roger Quadros [off-list ref] wrote:

Tarun,

On 07/09/2012 02:16 PM, DebBarma, Tarun Kanti wrote:

quoted

Hi Roger,

On Mon, Jul 9, 2012 at 3:13 PM, Roger Quadros [off-list ref] wrote:

quoted

Hi,

Just bumped across this patch and have a query.

On 03/16/2012 04:05 PM, Tarun Kanti DebBarma wrote:

quoted

There is no more need to have saved_wakeup because bank->context.wake_en
already holds that value. So getting rid of read/write operation associated
with this field.

Signed-off-by: Tarun Kanti DebBarma <redacted>
Reviewed-by: Santosh Shilimkar <redacted>
Acked-by: Felipe Balbi <redacted>
---
 drivers/gpio/gpio-omap.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 3a4f151..3b91ade 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c

@@ -57,7 +57,6 @@ struct gpio_bank {
      u16 irq;
      int irq_base;
      struct irq_domain *domain;
-     u32 saved_wakeup;
      u32 non_wakeup_gpios;
      u32 enabled_non_wakeup_gpios;
      struct gpio_regs context;

@@ -777,7 +776,6 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
      unsigned long           flags;

      spin_lock_irqsave(&bank->lock, flags);
-     bank->saved_wakeup = __raw_readl(mask_reg);
      __raw_writel(0xffff & ~bank->context.wake_en, mask_reg);

OK, here you are overwriting the mask_reg with the wakeup bitmask
without saving the mask_reg's original content.

This is based upon understanding that set_gpio_trigger() is the common
function where update of wake_en register takes place. Unless, mask_reg
in this case refers to something else, effectively we would be saving the
same value to saved_wakeup what is already present in wake_en.
I will verify this specific to this function.

quoted

quoted

      spin_unlock_irqrestore(&bank->lock, flags);

@@ -793,7 +791,7 @@ static int omap_mpuio_resume_noirq(struct device *dev)
      unsigned long           flags;

      spin_lock_irqsave(&bank->lock, flags);
-     __raw_writel(bank->saved_wakeup, mask_reg);
+     __raw_writel(bank->context.wake_en, mask_reg);

Now you are restoring nothing but the same content that you stored
during suspend. This will cause the non-wakeup gpio interrupts to get
masked between a suspend/resume. So isn't this a bug?

That's right, the same value is restored back which was last updated in
set_gpio_trigger() that got stored in wake_en register. Let me know if
I am missing your points here.

If it is writing the same thing then isn't this write redundant?

Not, really. During suspend if the register has lost the context
we need to restore the value from wake_en.
--
Tarun

quoted

quoted

Proper solution would be to save the mask_reg context into another
register than context.wake_en during suspend.

As I said, this would make sense if mask_reg is referring to different
register than what is used in set_gpio_trigger(). I will have a look.

OK thanks.

quoted

BTW, did you observe anything unusual during some testing?

No, I haven't done any tests.

cheers,
-roger