gpio/omap: Some more driver cleanup and fixes

There are two ways through which wakeup_en register can be programmed using gpiolib APIs as shown below. It is seen that in the second case in _set_gpio_wakeup(), even though bank->suspend_wakeup is updated correctly, its value is not programmed in wakeup_en register. Fix this.

chip.irq_set_type()->gpio_irq_type()->_set_gpio_triggering()->set_gpio_trigger() chip.irq_set_wake()->gpio_wake_enable()->_set_gpio_wakeup()

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---

This local variable is just assigned zero and then OR'ed with isr. It does not appear to serve any purpose and so removing it.

Signed-off-by Tarun Kanti DebBarma <[email protected]>

Reviewed-by Santosh Shilimkar <[email protected]>

Acked-by Felipe Balbi <[email protected]>

---
?drivers/gpio/gpio-omap.c | ? ?3 ---
?1 files changed, 0 insertions(+), 3 deletions(-)

drivers/gpio/gpio-omap.c +0 -4

index 3765654..de5fe8f 100644
@@ -626,7 +626,6 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 ? ? ? ?u32 isr;
 ? ? ? ?unsigned int gpio_irq, gpio_index;
 ? ? ? ?struct gpio_bank *bank;
- ? ? ? u32 retrigger = 0;
 ? ? ? ?int unmasked = 0;
 ? ? ? ?struct irq_chip *chip = irq_desc_get_chip(desc);
 

@@ -663,8 +662,6 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 ? ? ? ? ? ? ? ? ? ? ? ?chained_irq_exit(chip, desc);
 ? ? ? ? ? ? ? ?}
 
- ? ? ? ? ? ? ? isr |= retrigger;
- ? ? ? ? ? ? ? retrigger = 0;
 ? ? ? ? ? ? ? ?if (!isr)
 ? ? ? ? ? ? ? ? ? ? ? ?break;
 
--

1.7.0.4

Quoted 3 lines · Tarun Kanti DebBarma

This local variable is just assigned zero and then OR'ed with isr. It does not appear to serve any purpose and so removing it.

Missed following comments from Kevin given in v2. Sorry about that. I have updated the change in:
git://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev
for_3.4/gpio_further_cleanup_fixes

"commit 672e302e3c (ARM: OMAP: use edge/level handlers from generic IRQ framework) removed retrigger support in favor of using generic IRQ framework. This patch cleans up some unused remnants of that removal.

This local variable is just assigned zero and then OR'ed with isr. It does not appear to serve any purpose and so removing it.

Signed-off-by Tarun Kanti DebBarma <[email protected]>

Reviewed-by Santosh Shilimkar <[email protected]>

Acked-by Felipe Balbi <[email protected]>

---
 drivers/gpio/gpio-omap.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

drivers/gpio/gpio-omap.c +0 -3

index 3765654..de5fe8f 100644
@@ -626,7 +626,6 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 	u32 isr;
 	unsigned int gpio_irq, gpio_index;
 	struct gpio_bank *bank;
-	u32 retrigger = 0;
 	int unmasked = 0;
 	struct irq_chip *chip = irq_desc_get_chip(desc);
 

@@ -663,8 +662,6 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 			chained_irq_exit(chip, desc);
 		}
 
-		isr |= retrigger;
-		retrigger = 0;
 		if (!isr)
 			break;

In gpio_get(), _get_gpio_datain() and _get_gpio_dataout() get rid of un-necessary operation to compute gpio mask. The gpio offset passed to gpio_get() is sufficient to do that.

Here is Russell's original comment:
Can someone explain to me this:
#define GPIO_INDEX(bank, gpio) (gpio % bank->width) #define GPIO_BIT(bank, gpio) (1 << GPIO_INDEX(bank, gpio))

static int _get_gpio_datain(struct gpio_bank *bank, int gpio)

{
       void __iomem *reg = bank->base + bank->regs->datain;

       return (__raw_readl(reg) & GPIO_BIT(bank, gpio)) != 0;
}

static int gpio_get(struct gpio_chip *chip, unsigned offset)

{
       struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
       void __iomem *reg = bank->base;
       int gpio = chip->base + offset;
       u32 mask = GPIO_BIT(bank, gpio);

       if (gpio_is_input(bank, mask))
               return _get_gpio_datain(bank, gpio);
       else
               return _get_gpio_dataout(bank, gpio);
}

Given that bank->width on OMAP is either 32 or 16, and GPIO numbers for any GPIO chip are always aligned to 32 or 16, why does this code bother adding the chips base gpio number and then modulo the width?

Surely this means if - for argument sake - you registered a GPIO chip with 8 lines followed by one with 16 lines, GPIO0..7 would be chip 0 bit 0..7, GPIO8..15 would be chip 1 bit 8..15, GPIO16..23 would be chip 1 bit 0..7.

However, if you registered a GPIO chip with 16 lines first, it would mean GPIO0..15 would be chip 0 bit 0..15, and GPIO16..31 would be chip 1 bit 0..15.

Surely this kind of behaviour is not intended?

Is there a reason why the bitmask can't just be (1 << offset) where offset is passed into these functions as GPIO number - chip->base ?

Reported-by Russell King - ARM Linux <[email protected]>

Signed-off-by Tarun Kanti DebBarma <[email protected]>

Both omap_gpio_suspend() and omap_gpio_resume() does programming of wakeup_en register.
_gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0); _gpio_rmw(base, bank->regs->wkup_en, bank->context.wake_en, 1);

This is redundant in omap_gpio_suspend() because wakeup_en register automatically gets initialized in _set_gpio_wakeup() and set_gpio_trigger() while being called either from chip.irq_set_wake() or chip.irq_set_type().

This is redundant in omap_gpio_resume() because wakeup_en

register is programmed in omap_gpio_restore_context() called which is called from runtime resume callback.

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---

There are two functions, _set_gpio_dataout_reg() and _set_gpio_dataout_mask() which writes to dataout register and the dataout context must be saved. It is missing in the first function, _set_gpio_dataout_reg(). Fix this.

Reported-by Govindraj Raja <[email protected]>

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---

Quoted 7 lines · Tarun Kanti DebBarma

There are two functions, _set_gpio_dataout_reg() and _set_gpio_dataout_mask() which writes to dataout register and the dataout context must be saved. It is missing in the first function, _set_gpio_dataout_reg(). Fix this.

Reported-by Govindraj Raja <[email protected]>

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---

Good catch. Is the suspend/resume caught this issue?

This can go as a fix as well.

Reviewed-by Santosh Shilimkar <[email protected]>

Regards
Santosh

In the existing _set_gpio_dataout_*() implementation, the dataout register is overwritten every time the function is called. This is not intended behavior because that would end up one user of a GPIO line overwriting what is written by another. Fix this so that previous value is always preserved until explicitly changed by respective user/driver of the GPIO line.

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---

In the existing _set_gpio_dataout_*() implementation, the dataout register is overwritten every time the function is called. This is not intended behavior because that would end up one user of a GPIO line overwriting what is written by another. Fix this so that previous value is always preserved until explicitly changed by respective user/driver of the GPIO line.

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---
 drivers/gpio/gpio-omap.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

drivers/gpio/gpio-omap.c +0 -0

index 04c2677..2e8e476 100644

drivers/gpio/gpio-omap.c +0 -0

 	__raw_writel(l, reg);
 	bank->context.dataout = l;
 }

Quoted 20 lines · Tarun Kanti DebBarma

In the existing _set_gpio_dataout_*() implementation, the dataout register is overwritten every time the function is called. This is not intended behavior because that would end up one user of a GPIO line overwriting what is written by another. Fix this so that previous value is always preserved until explicitly changed by respective user/driver of the GPIO line.

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---
?drivers/gpio/gpio-omap.c | ? ?3 +++
?1 files changed, 3 insertions(+), 0 deletions(-)

drivers/gpio/gpio-omap.c +0 -0

index 04c2677..2e8e476 100644
@@ -114,6 +114,7 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable)

? ? ? else
? ? ? ? ? ? ? reg += bank->regs->clr_dataout;

Quoted 1 line

+ ? ? l |= __raw_readl(bank->base + bank->regs->set_dataout);

minor: IMO, it's more reader-friendly if this looks like

? ? ? l = __raw_read(...)
? ? ? l |= GPIO_BIT(...)
? ? ? __raw_write(...)

Quoted 10 lines

? ? ? __raw_writel(l, reg);
? ? ? bank->context.dataout = l;
?}

drivers/gpio/gpio-omap.c +0 -0

@@ -130,6 +131,8 @@ static void _set_gpio_dataout_mask(struct gpio_bank *bank, int gpio, int enable)
 ? ? ? ? ? ? ? l |= gpio_bit;

? ? ? else
? ? ? ? ? ? ? l &= ~gpio_bit;
+
+ ? ? l |= __raw_readl(bank->base + bank->regs->set_dataout);

There's already a __raw_read() in this function just above.

Quoted 4 lines

? ? ? __raw_writel(l, reg);
? ? ? bank->context.dataout = l;
?}

Quoted 22 lines · Kevin Hilman

Quoted 20 lines · Tarun Kanti DebBarma

In the existing _set_gpio_dataout_*() implementation, the dataout register is overwritten every time the function is called. This is not intended behavior because that would end up one user of a GPIO line overwriting what is written by another. Fix this so that previous value is always preserved until explicitly changed by respective user/driver of the GPIO line.

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---
?drivers/gpio/gpio-omap.c | ? ?3 +++
?1 files changed, 3 insertions(+), 0 deletions(-)

drivers/gpio/gpio-omap.c +0 -0

index 04c2677..2e8e476 100644
@@ -114,6 +114,7 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable)

? ? ? else
? ? ? ? ? ? ? reg += bank->regs->clr_dataout;

Quoted 7 lines

Quoted 1 line

+ ? ? l |= __raw_readl(bank->base + bank->regs->set_dataout);

minor: IMO, it's more reader-friendly if this looks like

? ? ? l = __raw_read(...)
? ? ? l |= GPIO_BIT(...)
? ? ? __raw_write(...)

Agreed. I will make the change.

Quoted 13 lines

Quoted 11 lines

? ? ? __raw_writel(l, reg);
? ? ? bank->context.dataout = l;
?}

drivers/gpio/gpio-omap.c +0 -0

@@ -130,6 +131,8 @@ static void _set_gpio_dataout_mask(struct gpio_bank *bank, int gpio, int enable)
 ? ? ? ? ? ? ? l |= gpio_bit;

? ? ? else
? ? ? ? ? ? ? l &= ~gpio_bit;
+
+ ? ? l |= __raw_readl(bank->base + bank->regs->set_dataout);

There's already a __raw_read() in this function just above.

Right. Thanks.
--
Tarun

Quoted 4 lines

Quoted 4 lines

? ? ? __raw_writel(l, reg);
? ? ? bank->context.dataout = l;
?}

Quoted 27 lines · Tarun Kanti

Quoted 26 lines · Kevin Hilman

Quoted 20 lines · Tarun Kanti DebBarma

In the existing _set_gpio_dataout_*() implementation, the dataout register is overwritten every time the function is called. This is not intended behavior because that would end up one user of a GPIO line overwriting what is written by another. Fix this so that previous value is always preserved until explicitly changed by respective user/driver of the GPIO line.

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---
?drivers/gpio/gpio-omap.c | ? ?3 +++
?1 files changed, 3 insertions(+), 0 deletions(-)

drivers/gpio/gpio-omap.c +0 -0

index 04c2677..2e8e476 100644
@@ -114,6 +114,7 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable)

? ? ? else
? ? ? ? ? ? ? reg += bank->regs->clr_dataout;

Quoted 1 line

+ ? ? l |= __raw_readl(bank->base + bank->regs->set_dataout);

minor: IMO, it's more reader-friendly if this looks like

Quoted 4 lines

Quoted 3 lines

? ? ? l = __raw_read(...)
? ? ? l |= GPIO_BIT(...)
? ? ? __raw_write(...)

Agreed. I will make the change.

Also, the read should be: __raw_readl(bank->base + bank->regs->dataout); instead of bank->regs->set_dataout.

Quoted 20 lines

Quoted 13 lines

Quoted 11 lines

? ? ? __raw_writel(l, reg);
? ? ? bank->context.dataout = l;
?}

drivers/gpio/gpio-omap.c +0 -0

@@ -130,6 +131,8 @@ static void _set_gpio_dataout_mask(struct gpio_bank *bank, int gpio, int enable)
 ? ? ? ? ? ? ? l |= gpio_bit;

? ? ? else
? ? ? ? ? ? ? l &= ~gpio_bit;
+
+ ? ? l |= __raw_readl(bank->base + bank->regs->set_dataout);

There's already a __raw_read() in this function just above.

Right. Thanks.
--
Tarun

Quoted 4 lines

Quoted 4 lines

? ? ? __raw_writel(l, reg);
? ? ? bank->context.dataout = l;
?}

Quoted 29 lines · Tarun Kanti

Quoted 27 lines · Tarun Kanti

Quoted 26 lines · Kevin Hilman

Quoted 20 lines · Tarun Kanti DebBarma

In the existing _set_gpio_dataout_*() implementation, the dataout register is overwritten every time the function is called. This is not intended behavior because that would end up one user of a GPIO line overwriting what is written by another. Fix this so that previous value is always preserved until explicitly changed by respective user/driver of the GPIO line.

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---
?drivers/gpio/gpio-omap.c | ? ?3 +++
?1 files changed, 3 insertions(+), 0 deletions(-)

drivers/gpio/gpio-omap.c +0 -0

index 04c2677..2e8e476 100644
@@ -114,6 +114,7 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable)

? ? ? else
? ? ? ? ? ? ? reg += bank->regs->clr_dataout;

Quoted 1 line

+ ? ? l |= __raw_readl(bank->base + bank->regs->set_dataout);

minor: IMO, it's more reader-friendly if this looks like

Quoted 6 lines

Quoted 4 lines

Quoted 3 lines

? ? ? l = __raw_read(...)
? ? ? l |= GPIO_BIT(...)
? ? ? __raw_write(...)

Agreed. I will make the change.

Also, the read should be: __raw_readl(bank->base + bank->regs->dataout); instead of bank->regs->set_dataout.

I see a problem with this implementation. It is not correct to write l |= GPIO_BIT(...).
For example if we write to clr_dataout register we would end up clearing bits which
we are not supposed to. We should just be operating on current GPIO_BIT(...). The l |= GPIO_BIT(...) is needed just to make sure that we have the right context
stored. So the overall sequence should be something like this:

        void __iomem *reg = bank->base;
        u32 l = GPIO_BIT(bank, gpio);

        if (enable)
                reg += bank->regs->set_dataout;
        else
                reg += bank->regs->clr_dataout;

        __raw_writel(l, reg);
        l |= __raw_readl(bank->base + bank->regs->dataout);
        bank->context.dataout = l;

Quoted 54 lines · Tarun Kanti

Quoted 29 lines · Tarun Kanti

Quoted 27 lines · Tarun Kanti

Quoted 26 lines · Kevin Hilman

Quoted 20 lines · Tarun Kanti DebBarma

In the existing _set_gpio_dataout_*() implementation, the dataout register is overwritten every time the function is called. This is not intended behavior because that would end up one user of a GPIO line overwriting what is written by another. Fix this so that previous value is always preserved until explicitly changed by respective user/driver of the GPIO line.

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---
?drivers/gpio/gpio-omap.c | ? ?3 +++
?1 files changed, 3 insertions(+), 0 deletions(-)

drivers/gpio/gpio-omap.c +0 -0

index 04c2677..2e8e476 100644
@@ -114,6 +114,7 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable)

? ? ? else
? ? ? ? ? ? ? reg += bank->regs->clr_dataout;

Quoted 1 line

+ ? ? l |= __raw_readl(bank->base + bank->regs->set_dataout);

minor: IMO, it's more reader-friendly if this looks like

Quoted 6 lines

Quoted 4 lines

Quoted 3 lines

? ? ? l = __raw_read(...)
? ? ? l |= GPIO_BIT(...)
? ? ? __raw_write(...)

Agreed. I will make the change.

Also, the read should be: __raw_readl(bank->base + bank->regs->dataout); instead of bank->regs->set_dataout.

I see a problem with this implementation. It is not correct to write l |= GPIO_BIT(...).
For example if we write to clr_dataout register we would end up clearing bits which
we are not supposed to. We should just be operating on current GPIO_BIT(...). The l |= GPIO_BIT(...) is needed just to make sure that we have the right context
stored. So the overall sequence should be something like this:

? ? ? ? void __iomem *reg = bank->base;
? ? ? ? u32 l = GPIO_BIT(bank, gpio);

? ? ? ? if (enable)
? ? ? ? ? ? ? ? reg += bank->regs->set_dataout;
? ? ? ? else
? ? ? ? ? ? ? ? reg += bank->regs->clr_dataout;

Quoted 3 lines

? ? ? ? __raw_writel(l, reg);
? ? ? ? l |= __raw_readl(bank->base + bank->regs->dataout); ? ? ? ? bank->context.dataout = l;

Again, you don't need the extra read-back here. ?Just set/clear the bit in the context. ?Untested patch below.

Kevin

drivers/gpio/gpio-omap.c +5 -2

index 0b05629..db905c0 100644
@@ -111,10 +111,13 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable)
 ? ? ? ?void __iomem *reg = bank->base;
 ? ? ? ?u32 l = GPIO_BIT(bank, gpio);
 
- ? ? ? if (enable)
+ ? ? ? if (enable) {
 ? ? ? ? ? ? ? ?reg += bank->regs->set_dataout;
- ? ? ? else
+ ? ? ? ? ? ? ? bank->context.dataout |= l;
+ ? ? ? } else {
 ? ? ? ? ? ? ? ?reg += bank->regs->clr_dataout;
+ ? ? ? ? ? ? ? bank->context.dataout &= ~l;
+ ? ? ? }

In omap_gpio_runtime_resume() the context restore should be independent of bank->enabled_non_wakeup_gpios. This was preventing context restore of GPIO lines which are not wakeup enabled.

Reported-by Govindraj Raja <[email protected]>

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---
 drivers/gpio/gpio-omap.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/gpio/gpio-omap.c +0 -0

index 2e8e476..ccfbae0 100644

Quoted 18 lines · Tarun Kanti DebBarma

In omap_gpio_runtime_resume() the context restore should be independent of bank->enabled_non_wakeup_gpios. This was preventing context restore of GPIO lines which are not wakeup enabled.

Reported-by Govindraj Raja <[email protected]>

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---
?drivers/gpio/gpio-omap.c | ? ?2 +-
?1 files changed, 1 insertions(+), 1 deletions(-)

drivers/gpio/gpio-omap.c +0 -0

index 2e8e476..ccfbae0 100644
@@ -1227,7 +1227,7 @@ static int omap_gpio_runtime_resume(struct device *dev)
 ? ? ? __raw_writel(bank->context.risingdetect,
 ? ? ? ? ? ? ? ? ? ?bank->base + bank->regs->risingdetect);
 

drivers/gpio/gpio-omap.c static int omap_gpio_runtime_resume(struct device *dev) · line 1230

Quoted 2 lines

drivers/gpio/gpio-omap.c +1 -1

- ? ? if (!bank->enabled_non_wakeup_gpios || !bank->workaround_enabled) {
+ ? ? if (!bank->workaround_enabled) {

This doesn't seem to be right.
Don't you want to avoid GPIO restore for banks which are in always on domain. Infact the purpose of "enabled_non_wakeup_gpios" is exactly that ? Isn't it.

What am I missing ?

Regards
Santosh

Quoted 19 lines · Santosh Shilimkar

Quoted 18 lines · Tarun Kanti DebBarma

In omap_gpio_runtime_resume() the context restore should be independent of bank->enabled_non_wakeup_gpios. This was preventing context restore of GPIO lines which are not wakeup enabled.

Reported-by Govindraj Raja <[email protected]>

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---
?drivers/gpio/gpio-omap.c | ? ?2 +-
?1 files changed, 1 insertions(+), 1 deletions(-)

drivers/gpio/gpio-omap.c +0 -0

index 2e8e476..ccfbae0 100644
@@ -1227,7 +1227,7 @@ static int omap_gpio_runtime_resume(struct device *dev)
 ? ? ? __raw_writel(bank->context.risingdetect,
 ? ? ? ? ? ? ? ? ? ?bank->base + bank->regs->risingdetect);
 

Quoted 8 lines

drivers/gpio/gpio-omap.c static int omap_gpio_runtime_resume(struct device *dev) · line 1230

Quoted 2 lines

drivers/gpio/gpio-omap.c +1 -1

- ? ? if (!bank->enabled_non_wakeup_gpios || !bank->workaround_enabled) {
+ ? ? if (!bank->workaround_enabled) {

This doesn't seem to be right.
Don't you want to avoid GPIO restore for banks which are in always on domain. Infact the purpose of "enabled_non_wakeup_gpios" is exactly that ? Isn't it.

What am I missing ?

The bank->enabled_non_wakeup_gpios is set whenever a gpio line is programmed as edge trigger as shown below.
(This is not meant to distinguish between gpios in WKUP domain vs those in PER domain).
The context restore should happen irrespective of whether the trigger type is edge or level.
In fact context restore was not happening for a gpio line because of this condition while
testing suspend/resume.

[...]
? ? ? ? ? ? ? ?if (trigger & IRQ_TYPE_EDGE_BOTH)
? ? ? ? ? ? ? ? ? ? ? ?bank->enabled_non_wakeup_gpios |= gpio_bit; ? ? ? ? ? ? ? ?else
? ? ? ? ? ? ? ? ? ? ? ?bank->enabled_non_wakeup_gpios &= ~gpio_bit; [...]

Quoted 39 lines · Tarun Kanti

Quoted 19 lines · Santosh Shilimkar

Quoted 18 lines · Tarun Kanti DebBarma

In omap_gpio_runtime_resume() the context restore should be independent of bank->enabled_non_wakeup_gpios. This was preventing context restore of GPIO lines which are not wakeup enabled.

Reported-by Govindraj Raja <[email protected]>

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---
?drivers/gpio/gpio-omap.c | ? ?2 +-
?1 files changed, 1 insertions(+), 1 deletions(-)

drivers/gpio/gpio-omap.c +0 -0

index 2e8e476..ccfbae0 100644
@@ -1227,7 +1227,7 @@ static int omap_gpio_runtime_resume(struct device *dev)
 ? ? ? __raw_writel(bank->context.risingdetect,
 ? ? ? ? ? ? ? ? ? ?bank->base + bank->regs->risingdetect);
 

Quoted 8 lines

drivers/gpio/gpio-omap.c static int omap_gpio_runtime_resume(struct device *dev) · line 1230

Quoted 2 lines

drivers/gpio/gpio-omap.c +1 -1

- ? ? if (!bank->enabled_non_wakeup_gpios || !bank->workaround_enabled) {
+ ? ? if (!bank->workaround_enabled) {

This doesn't seem to be right.
Don't you want to avoid GPIO restore for banks which are in always on domain. Infact the purpose of "enabled_non_wakeup_gpios" is exactly that ? Isn't it.

What am I missing ?

The bank->enabled_non_wakeup_gpios is set whenever a gpio line is programmed as edge trigger as shown below.
(This is not meant to distinguish between gpios in WKUP domain vs those in PER domain).
The context restore should happen irrespective of whether the trigger type is edge or level.
In fact context restore was not happening for a gpio line because of this condition while
testing suspend/resume.

Quoted 6 lines

[...]
? ? ? ? ? ? ? ?if (trigger & IRQ_TYPE_EDGE_BOTH)
? ? ? ? ? ? ? ? ? ? ? ?bank->enabled_non_wakeup_gpios |= gpio_bit; ? ? ? ? ? ? ? ?else
? ? ? ? ? ? ? ? ? ? ? ?bank->enabled_non_wakeup_gpios &= ~gpio_bit; [...]

Make sense now. Thanks for clarification Tarun.
You can add mine..

Reviewed-by Santosh Shilimkar <[email protected]>

Regards
Santosh

In _enable_gpio_irqbank() when bank->regs->set_irqenable is valid, gpio_mask can be directly set by writing to set_irqenable register without overwriting current value. In order to ensure the same is stored in context.irqenable1, we must read from regs->irqenable instead of overwriting it with gpio_mask.
The overwriting makes sense only in the second case where irqenable is explicitly read and updated with new gpio_mask before writing it back. However, for consistency reading regs->irqenable into the bank->context.irqenable1 takes care of both the scenarios.

        if (bank->regs->set_irqenable) {
                reg += bank->regs->set_irqenable;
                l = gpio_mask;
        } else {
                reg += bank->regs->irqenable;
                l = __raw_readl(reg);
                if (bank->regs->irqenable_inv)
                        l &= ~gpio_mask;
                else
                        l |= gpio_mask;
        }

Make the same change for _disable_gpio_irqbank().

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---

In _enable_gpio_irqbank() when bank->regs->set_irqenable is valid, gpio_mask can be directly set by writing to set_irqenable register without overwriting current value.

In order to ensure the same is
stored in context.irqenable1, we must read from regs->irqenable instead of overwriting it with gpio_mask.
The overwriting makes sense only in the second case where irqenable is explicitly read and updated with new gpio_mask before writing it back. However, for consistency reading regs->irqenable into the bank->context.irqenable1 takes care of both the scenarios.

Quoted 3 lines · Tarun Kanti DebBarma

In _enable_gpio_irqbank() when bank->regs->set_irqenable is valid, gpio_mask can be directly set by writing to set_irqenable register without overwriting current value.

Ouch. ?Nice catch.

Quoted 7 lines

In order to ensure the same is
stored in context.irqenable1, we must read from regs->irqenable instead of overwriting it with gpio_mask.
The overwriting makes sense only in the second case where irqenable is explicitly read and updated with new gpio_mask before writing it back. However, for consistency reading regs->irqenable into the bank->context.irqenable1 takes care of both the scenarios.

...takes care of both scenarios, but adds and extra duplicate read for the second.

Instead, how about just move the context update into each side of the if/else? ?untested patch below to show what I mean.

Kevin

drivers/gpio/gpio-omap.c +1 -0

index 752ae9b..f8b7099 100644
@@ -442,6 +442,7 @@ static void _enable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)
 ? ? ? ?if (bank->regs->set_irqenable) {
 ? ? ? ? ? ? ? ?reg += bank->regs->set_irqenable;
 ? ? ? ? ? ? ? ?l = gpio_mask;
+ ? ? ? ? ? ? ? bank->context.irqenable1 |= gpio_mask;
 ? ? ? ?} else {
 ? ? ? ? ? ? ? ?reg += bank->regs->irqenable;
 ? ? ? ? ? ? ? ?l = __raw_readl(reg);

@@ -449,10 +450,10 @@ static void _enable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)
 ? ? ? ? ? ? ? ? ? ? ? ?l &= ~gpio_mask;

? ? ? ? ? ? ? ?else
? ? ? ? ? ? ? ? ? ? ? ?l |= gpio_mask;
+ ? ? ? ? ? ? ? bank->context.irqenable1 = l;
? ? ? ?}

? ? ? ?__raw_writel(l, reg);
- ? ? ? bank->context.irqenable1 = l;
?}

?static void _disable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)

drivers/gpio/gpio-omap.c +1 -0

@@ -463,6 +464,7 @@ static void _disable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)
 ? ? ? ?if (bank->regs->clr_irqenable) {
 ? ? ? ? ? ? ? ?reg += bank->regs->clr_irqenable;
 ? ? ? ? ? ? ? ?l = gpio_mask;
+ ? ? ? ? ? ? ? bank->context.irqenable1 &= ~gpio_mask;
 ? ? ? ?} else {
 ? ? ? ? ? ? ? ?reg += bank->regs->irqenable;
 ? ? ? ? ? ? ? ?l = __raw_readl(reg);

@@ -470,10 +472,10 @@ static void _disable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)
 ? ? ? ? ? ? ? ? ? ? ? ?l |= gpio_mask;

? ? ? ? ? ? ? ?else
? ? ? ? ? ? ? ? ? ? ? ?l &= ~gpio_mask;
+ ? ? ? ? ? ? ? bank->context.irqenable1 = l;
? ? ? ?}

? ? ? ?__raw_writel(l, reg);
- ? ? ? bank->context.irqenable1 = l;
?}

?static inline void _set_gpio_irqenable(struct gpio_bank *bank, int gpio, int enable)

The cleanup is mostly getting rid of redundant fields in struct gpio_bank{} as we already have them as part of bank->context now. Also, remove un-used variable from gpio_irq_handler.

The fixes include correction of _set_gpio_irqenable() implementation, missing wakeup_en register update in set_gpio_wakeup(), type mismatch of gpio trigger parameter in set_gpio_trigger(), incorrect dataout register update in set_gpio_dataout_() and few corrections in context save logic.

It is baselined on top of Kevin's following series:
gpio/omap: cleanup and runtime PM conversion for v3.4
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.4/gpio/runtime-pm-cleanup

Series is available here for reference:
git://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev for_3.4/gpio_further_cleanup_fixes

Quoted 10 lines · Tarun Kanti DebBarma

The cleanup is mostly getting rid of redundant fields in struct gpio_bank{} as we already have them as part of bank->context now. Also, remove un-used variable from gpio_irq_handler.

The fixes include correction of _set_gpio_irqenable() implementation, missing wakeup_en register update in set_gpio_wakeup(), type mismatch of gpio trigger parameter in set_gpio_trigger(), incorrect dataout register update in set_gpio_dataout_() and few corrections in context save logic.

Quoted 6 lines

It is baselined on top of Kevin's following series:
gpio/omap: cleanup and runtime PM conversion for v3.4
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.4/gpio/runtime-pm-cleanup

Series is available here for reference:
git://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev for_3.4/gpio_further_cleanup_fixes

This branch doesn't exist.

Kevin

Quoted 12 lines · Kevin Hilman

Quoted 10 lines · Tarun Kanti DebBarma

The cleanup is mostly getting rid of redundant fields in struct gpio_bank{} as we already have them as part of bank->context now. Also, remove un-used variable from gpio_irq_handler.

The fixes include correction of _set_gpio_irqenable() implementation, missing wakeup_en register update in set_gpio_wakeup(), type mismatch of gpio trigger parameter in set_gpio_trigger(), incorrect dataout register update in set_gpio_dataout_() and few corrections in context save logic.

Quoted 8 lines

Quoted 6 lines

It is baselined on top of Kevin's following series:
gpio/omap: cleanup and runtime PM conversion for v3.4
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.4/gpio/runtime-pm-cleanup

Series is available here for reference:
git://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev for_3.4/gpio_further_cleanup_fixes

This branch doesn't exist.

Oops... I am sorry.
I was in the process of updating the branch with latest comments after base-lining on Grant's:
git://git.secretlab.ca/git/linux-2.6.git
Branch: gpio/next
Because this has all the gpio changes including Benoit's latest gpio devicetree changes.
Should I go ahead?

Quoted 3 lines

Kevin

Quoted 21 lines · Tarun Kanti

Quoted 12 lines · Kevin Hilman

Quoted 10 lines · Tarun Kanti DebBarma

The cleanup is mostly getting rid of redundant fields in struct gpio_bank{} as we already have them as part of bank->context now. Also, remove un-used variable from gpio_irq_handler.

The fixes include correction of _set_gpio_irqenable() implementation, missing wakeup_en register update in set_gpio_wakeup(), type mismatch of gpio trigger parameter in set_gpio_trigger(), incorrect dataout register update in set_gpio_dataout_() and few corrections in context save logic.

Quoted 8 lines

Quoted 6 lines

It is baselined on top of Kevin's following series:
gpio/omap: cleanup and runtime PM conversion for v3.4
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.4/gpio/runtime-pm-cleanup

Series is available here for reference:
git://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev for_3.4/gpio_further_cleanup_fixes

This branch doesn't exist.

Quoted 8 lines

Oops... I am sorry.
I was in the process of updating the branch with latest comments after base-lining on Grant's:
git://git.secretlab.ca/git/linux-2.6.git
Branch: gpio/next
Because this has all the gpio changes including Benoit's latest gpio devicetree changes.
Should I go ahead?

Anyways, for the time being I am updating on top of your changes as before. You should be able to see the branch shortly.

Quoted 2 lines

Quoted 2 lines

Kevin

Quoted 8 lines

Quoted 8 lines

Oops... I am sorry.
I was in the process of updating the branch with latest comments after base-lining on Grant's:
git://git.secretlab.ca/git/linux-2.6.git
Branch: gpio/next
Because this has all the gpio changes including Benoit's latest gpio devicetree changes.
Should I go ahead?

Quoted 2 lines

Anyways, for the time being I am updating on top of your changes as before. You should be able to see the branch shortly.

Please note that the branch has your following patch:
gpio/omap: fix wakeups on level-triggered GPIOs

Quoted 8 lines

Quoted 8 lines

Oops... I am sorry.
I was in the process of updating the branch with latest comments after base-lining on Grant's:
git://git.secretlab.ca/git/linux-2.6.git
Branch: gpio/next
Because this has all the gpio changes including Benoit's latest gpio devicetree changes.
Should I go ahead?

Quoted 2 lines

Anyways, for the time being I am updating on top of your changes as before. You should be able to see the branch shortly.

Please note that the branch has your following patch:
gpio/omap: fix wakeups on level-triggered GPIOs

"DebBarma, Tarun Kanti" <[email protected]> writes:

[...]

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Oops... I am sorry.
I was in the process of updating the branch with latest comments after base-lining on Grant's:
git://git.secretlab.ca/git/linux-2.6.git
Branch: gpio/next
Because this has all the gpio changes including Benoit's latest gpio devicetree changes.
Should I go ahead?

Quoted 4 lines

Quoted 2 lines

Anyways, for the time being I am updating on top of your changes as before. You should be able to see the branch shortly.

Please note that the branch has your following patch:
gpio/omap: fix wakeups on level-triggered GPIOs

Please be sure to Cc the GPIO maintainer (added now.)

Grant can we consider your gpio/next a stable baseline for further GPIO fixes?

If so, Tarun, please base your branch there. Otherwise, continue to use my for_3.4/gpio/runtime-pm-cleanup, but please test it by merging with the gpio/next branch.

Quoted 12 lines · Kevin Hilman

"DebBarma, Tarun Kanti" <[email protected]> writes:

[...]

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Oops... I am sorry.
I was in the process of updating the branch with latest comments after base-lining on Grant's:
git://git.secretlab.ca/git/linux-2.6.git
Branch: gpio/next
Because this has all the gpio changes including Benoit's latest gpio devicetree changes.
Should I go ahead?

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Anyways, for the time being I am updating on top of your changes as before. You should be able to see the branch shortly.

Please note that the branch has your following patch:
gpio/omap: fix wakeups on level-triggered GPIOs

Please be sure to Cc the GPIO maintainer (added now.)

Quoted 7 lines

Grant can we consider your gpio/next a stable baseline for further GPIO fixes?

If so, Tarun, please base your branch there. ?Otherwise, continue to use my for_3.4/gpio/runtime-pm-cleanup, but please test it by merging with the gpio/next branch.

I'm not going to rebase gpio/next before the merge window; you can base on it with confidence.

g.