[PATCH v3 00/13] gpio/omap: Some more driver cleanup and fixes

[PATCH v3 00/13] gpio/omap: Some more driver cleanup and fixes

[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.

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

Power Test: Off-mode and Retention on OMAP3430 (Suspend and Idle paths).
Functional Test: OMAP2430, OMAP3430SDP, ZOOM3, OMAP4430, OMAP4-BLAZE

v3:
- Added 4 more additional patches to the previous series
  which are all bug fixes.

v2:
- Added a new patch to update wakeup_en register in _set_gpio_wakeup()
 in addition to updating bank->context.wake_en.

- Added a new patch to remove redundant decoding of gpio offset in
 gpio_get(), _get_gpio_datain() and _get_gpio_dataout().

- Added a new patch to remove suspend/resume callbacks because the
 operations performed with the callbacks are redundant.

Tarun Kanti DebBarma (13):
  gpio/omap: remove saved_fallingdetect, saved_risingdetect fields
  gpio/omap: fix wakeup_en register update in _set_gpio_wakeup()
  gpio/omap: remove suspend_wakeup field from struct gpio_bank
  gpio/omap: remove saved_wakeup field from struct gpio_bank
  gpio/omap: get rid of retrigger variable in gpio_irq_handler
  gpio/omap: fix trigger type to unsigned
  gpio/omap: fix _set_gpio_irqenable implementation
  gpio/omap: remove redundant decoding of gpio offset
  gpio/omap: remove suspend/resume callbacks
  gpio/omap: fix missing dataout context save in _set_gpio_dataout_reg
  gpio/omap: fix dataout register overwrite in _set_gpio_dataout_*
  gpio/omap: fix incorrect context restore logic in omap_gpio_runtime_resume
  gpio/omap: fix incorrect update to context.irqenable1

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

[PATCH v3 01/13] gpio/omap: remove saved_fallingdetect, saved_risingdetect fields

[Tarun Kanti DebBarma]

Since we already have context.fallingdetect and context.risingdetect
there is no more need to have these additional fields. Also, getting
rid of extra reads associated with them.

Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Acked-by: Felipe Balbi <balbi@ti.com>
---
 drivers/gpio/gpio-omap.c |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 752ae9b..c9369d2 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -60,8 +60,6 @@ struct gpio_bank {
 	u32 enabled_non_wakeup_gpios;
 	struct gpio_regs context;
 	u32 saved_datain;
-	u32 saved_fallingdetect;
-	u32 saved_risingdetect;
 	u32 level_mask;
 	u32 toggle_mask;
 	spinlock_t lock;
@@ -1234,11 +1232,9 @@ static int omap_gpio_runtime_suspend(struct device *dev)
 
 	bank->saved_datain = __raw_readl(bank->base +
 						bank->regs->datain);
-	l1 = __raw_readl(bank->base + bank->regs->fallingdetect);
-	l2 = __raw_readl(bank->base + bank->regs->risingdetect);
+	l1 = bank->context.fallingdetect;
+	l2 = bank->context.risingdetect;
 
-	bank->saved_fallingdetect = l1;
-	bank->saved_risingdetect = l2;
 	l1 &= ~bank->enabled_non_wakeup_gpios;
 	l2 &= ~bank->enabled_non_wakeup_gpios;
 
@@ -1297,9 +1293,9 @@ static int omap_gpio_runtime_resume(struct device *dev)
 		}
 	}
 
-	__raw_writel(bank->saved_fallingdetect,
+	__raw_writel(bank->context.fallingdetect,
 			bank->base + bank->regs->fallingdetect);
-	__raw_writel(bank->saved_risingdetect,
+	__raw_writel(bank->context.risingdetect,
 			bank->base + bank->regs->risingdetect);
 	l = __raw_readl(bank->base + bank->regs->datain);
 
@@ -1316,14 +1312,15 @@ static int omap_gpio_runtime_resume(struct device *dev)
 	 * No need to generate IRQs for the rising edge for gpio IRQs
 	 * configured with falling edge only; and vice versa.
 	 */
-	gen0 = l & bank->saved_fallingdetect;
+	gen0 = l & bank->context.fallingdetect;
 	gen0 &= bank->saved_datain;
 
-	gen1 = l & bank->saved_risingdetect;
+	gen1 = l & bank->context.risingdetect;
 	gen1 &= ~(bank->saved_datain);
 
 	/* FIXME: Consider GPIO IRQs with level detections properly! */
-	gen = l & (~(bank->saved_fallingdetect) & ~(bank->saved_risingdetect));
+	gen = l & (~(bank->context.fallingdetect) &
+					 ~(bank->context.risingdetect));
 	/* Consider all GPIO IRQs needed to be updated */
 	gen |= gen0 | gen1;
 
-- 
1.7.0.4

[PATCH v3 02/13] gpio/omap: fix wakeup_en register update in _set_gpio_wakeup()

[Tarun Kanti DebBarma]

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 <tarun.kanti@ti.com>
---
 drivers/gpio/gpio-omap.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index c9369d2..895df7f 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -504,6 +504,7 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)
 	else
 		bank->suspend_wakeup &= ~gpio_bit;
 
+	__raw_writel(bank->suspend_wakeup, bank->base + bank->regs->wkup_en);
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	return 0;
-- 
1.7.0.4

[PATCH v3 03/13] gpio/omap: remove suspend_wakeup field from struct gpio_bank

[Tarun Kanti DebBarma]

Since we already have bank->context.wake_en to keep track
of gpios which are wakeup enabled, there is no need to have
this field any more.

Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Acked-by: Felipe Balbi <balbi@ti.com>
---
 drivers/gpio/gpio-omap.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 895df7f..14c61e2 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -54,7 +54,6 @@ struct gpio_bank {
 	void __iomem *base;
 	u16 irq;
 	u16 virtual_irq_start;
-	u32 suspend_wakeup;
 	u32 saved_wakeup;
 	u32 non_wakeup_gpios;
 	u32 enabled_non_wakeup_gpios;
@@ -500,11 +499,11 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)
 
 	spin_lock_irqsave(&bank->lock, flags);
 	if (enable)
-		bank->suspend_wakeup |= gpio_bit;
+		bank->context.wake_en |= gpio_bit;
 	else
-		bank->suspend_wakeup &= ~gpio_bit;
+		bank->context.wake_en &= ~gpio_bit;
 
-	__raw_writel(bank->suspend_wakeup, bank->base + bank->regs->wkup_en);
+	__raw_writel(bank->context.wake_en, bank->base + bank->regs->wkup_en);
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	return 0;
@@ -776,7 +775,7 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
 
 	spin_lock_irqsave(&bank->lock, flags);
 	bank->saved_wakeup = __raw_readl(mask_reg);
-	__raw_writel(0xffff & ~bank->suspend_wakeup, mask_reg);
+	__raw_writel(0xffff & ~bank->context.wake_en, mask_reg);
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	return 0;
@@ -1150,7 +1149,7 @@ static int omap_gpio_suspend(struct device *dev)
 	if (!bank->mod_usage || !bank->loses_context)
 		return 0;
 
-	if (!bank->regs->wkup_en || !bank->suspend_wakeup)
+	if (!bank->regs->wkup_en || !bank->context.wake_en)
 		return 0;
 
 	wakeup_enable = bank->base + bank->regs->wkup_en;
@@ -1158,7 +1157,7 @@ static int omap_gpio_suspend(struct device *dev)
 	spin_lock_irqsave(&bank->lock, flags);
 	bank->saved_wakeup = __raw_readl(wakeup_enable);
 	_gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0);
-	_gpio_rmw(base, bank->regs->wkup_en, bank->suspend_wakeup, 1);
+	_gpio_rmw(base, bank->regs->wkup_en, bank->context.wake_en, 1);
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	return 0;
-- 
1.7.0.4

[PATCH v3 04/13] gpio/omap: remove saved_wakeup field from struct gpio_bank

[Tarun Kanti DebBarma]

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 <tarun.kanti@ti.com>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Acked-by: Felipe Balbi <balbi@ti.com>
---
 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 14c61e2..3765654 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -54,7 +54,6 @@ struct gpio_bank {
 	void __iomem *base;
 	u16 irq;
 	u16 virtual_irq_start;
-	u32 saved_wakeup;
 	u32 non_wakeup_gpios;
 	u32 enabled_non_wakeup_gpios;
 	struct gpio_regs context;
@@ -774,7 +773,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);
 	spin_unlock_irqrestore(&bank->lock, flags);
 
@@ -790,7 +788,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);
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	return 0;
@@ -1143,7 +1141,6 @@ static int omap_gpio_suspend(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct gpio_bank *bank = platform_get_drvdata(pdev);
 	void __iomem *base = bank->base;
-	void __iomem *wakeup_enable;
 	unsigned long flags;
 
 	if (!bank->mod_usage || !bank->loses_context)
@@ -1152,10 +1149,7 @@ static int omap_gpio_suspend(struct device *dev)
 	if (!bank->regs->wkup_en || !bank->context.wake_en)
 		return 0;
 
-	wakeup_enable = bank->base + bank->regs->wkup_en;
-
 	spin_lock_irqsave(&bank->lock, flags);
-	bank->saved_wakeup = __raw_readl(wakeup_enable);
 	_gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0);
 	_gpio_rmw(base, bank->regs->wkup_en, bank->context.wake_en, 1);
 	spin_unlock_irqrestore(&bank->lock, flags);
@@ -1173,12 +1167,12 @@ static int omap_gpio_resume(struct device *dev)
 	if (!bank->mod_usage || !bank->loses_context)
 		return 0;
 
-	if (!bank->regs->wkup_en || !bank->saved_wakeup)
+	if (!bank->regs->wkup_en || !bank->context.wake_en)
 		return 0;
 
 	spin_lock_irqsave(&bank->lock, flags);
 	_gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0);
-	_gpio_rmw(base, bank->regs->wkup_en, bank->saved_wakeup, 1);
+	_gpio_rmw(base, bank->regs->wkup_en, bank->context.wake_en, 1);
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	return 0;
-- 
1.7.0.4

[PATCH v3 05/13] gpio/omap: get rid of retrigger variable in gpio_irq_handler

[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.

Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Acked-by: Felipe Balbi <balbi@ti.com>
---
 drivers/gpio/gpio-omap.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 3765654..de5fe8f 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -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

[PATCH v3 06/13] gpio/omap: fix trigger type to unsigned

[Tarun Kanti DebBarma]

The GPIO trigger parameter is of type unsigned.
enum {
        IRQ_TYPE_NONE           = 0x00000000,
        IRQ_TYPE_EDGE_RISING    = 0x00000001,
        IRQ_TYPE_EDGE_FALLING   = 0x00000002,
        IRQ_TYPE_EDGE_BOTH      = (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING),
        IRQ_TYPE_LEVEL_HIGH     = 0x00000004,
        IRQ_TYPE_LEVEL_LOW      = 0x00000008,
        IRQ_TYPE_LEVEL_MASK     = (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH),
        IRQ_TYPE_SENSE_MASK     = 0x0000000f,

        IRQ_TYPE_PROBE          = 0x00000010,
...
};
Even though gpio_irq_type(struct irq_data *d, unsigned type) has the right type
of parameter, the subsequent called functions set_gpio_triggering() and
set_gpio_trigger() wrongly makes it signed integer. Fix this.

Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Acked-by: Felipe Balbi <balbi@ti.com>
---
 drivers/gpio/gpio-omap.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index de5fe8f..bb994db 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -234,7 +234,7 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
 }
 
 static inline void set_gpio_trigger(struct gpio_bank *bank, int gpio,
-						int trigger)
+						unsigned trigger)
 {
 	void __iomem *base = bank->base;
 	u32 gpio_bit = 1 << gpio;
@@ -316,7 +316,8 @@ static void _toggle_gpio_edge_triggering(struct gpio_bank *bank, int gpio)
 static void _toggle_gpio_edge_triggering(struct gpio_bank *bank, int gpio) {}
 #endif
 
-static int _set_gpio_triggering(struct gpio_bank *bank, int gpio, int trigger)
+static int _set_gpio_triggering(struct gpio_bank *bank, int gpio,
+							unsigned trigger)
 {
 	void __iomem *reg = bank->base;
 	void __iomem *base = bank->base;
-- 
1.7.0.4

[PATCH v3 07/13] gpio/omap: fix _set_gpio_irqenable implementation

[Tarun Kanti DebBarma]

This function should be capable of both enabling and disabling interrupts
based upon the *enable* parameter. Right now the function only enables
the interrupt and *enable* is not used at all. So add the interrupt
disable capability also using the parameter.

Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Acked-by: Felipe Balbi <balbi@ti.com>
---
 drivers/gpio/gpio-omap.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index bb994db..19f8f44 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -475,7 +475,10 @@ static void _disable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)
 
 static inline void _set_gpio_irqenable(struct gpio_bank *bank, int gpio, int enable)
 {
-	_enable_gpio_irqbank(bank, GPIO_BIT(bank, gpio));
+	if (enable)
+		_enable_gpio_irqbank(bank, GPIO_BIT(bank, gpio));
+	else
+		_disable_gpio_irqbank(bank, GPIO_BIT(bank, gpio));
 }
 
 /*
-- 
1.7.0.4

[PATCH v3 08/13] gpio/omap: remove redundant decoding of gpio offset

[Tarun Kanti DebBarma]

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 <linux@arm.linux.org.uk>
Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
---
 drivers/gpio/gpio-omap.c |   18 +++++++-----------
 1 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 19f8f44..186ce92 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -133,18 +133,18 @@ static void _set_gpio_dataout_mask(struct gpio_bank *bank, int gpio, int enable)
 	bank->context.dataout = l;
 }
 
-static int _get_gpio_datain(struct gpio_bank *bank, int gpio)
+static int _get_gpio_datain(struct gpio_bank *bank, int offset)
 {
 	void __iomem *reg = bank->base + bank->regs->datain;
 
-	return (__raw_readl(reg) & GPIO_BIT(bank, gpio)) != 0;
+	return (__raw_readl(reg) & (1 << offset)) != 0;
 }
 
-static int _get_gpio_dataout(struct gpio_bank *bank, int gpio)
+static int _get_gpio_dataout(struct gpio_bank *bank, int offset)
 {
 	void __iomem *reg = bank->base + bank->regs->dataout;
 
-	return (__raw_readl(reg) & GPIO_BIT(bank, gpio)) != 0;
+	return (__raw_readl(reg) & (1 << offset)) != 0;
 }
 
 static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
@@ -849,19 +849,15 @@ static int gpio_is_input(struct gpio_bank *bank, int mask)
 static int gpio_get(struct gpio_chip *chip, unsigned offset)
 {
 	struct gpio_bank *bank;
-	void __iomem *reg;
-	int gpio;
 	u32 mask;
 
-	gpio = chip->base + offset;
 	bank = container_of(chip, struct gpio_bank, chip);
-	reg = bank->base;
-	mask = GPIO_BIT(bank, gpio);
+	mask = (1 << offset);
 
 	if (gpio_is_input(bank, mask))
-		return _get_gpio_datain(bank, gpio);
+		return _get_gpio_datain(bank, offset);
 	else
-		return _get_gpio_dataout(bank, gpio);
+		return _get_gpio_dataout(bank, offset);
 }
 
 static int gpio_output(struct gpio_chip *chip, unsigned offset, int value)
-- 
1.7.0.4

[PATCH v3 09/13] gpio/omap: remove suspend/resume callbacks

[Tarun Kanti DebBarma]

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 <tarun.kanti@ti.com>
---
 drivers/gpio/gpio-omap.c |   47 ----------------------------------------------
 1 files changed, 0 insertions(+), 47 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 186ce92..8b4a7ba 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1132,50 +1132,6 @@ err_exit:
 
 #ifdef CONFIG_ARCH_OMAP2PLUS
 
-#if defined(CONFIG_PM_SLEEP)
-static int omap_gpio_suspend(struct device *dev)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-	struct gpio_bank *bank = platform_get_drvdata(pdev);
-	void __iomem *base = bank->base;
-	unsigned long flags;
-
-	if (!bank->mod_usage || !bank->loses_context)
-		return 0;
-
-	if (!bank->regs->wkup_en || !bank->context.wake_en)
-		return 0;
-
-	spin_lock_irqsave(&bank->lock, flags);
-	_gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0);
-	_gpio_rmw(base, bank->regs->wkup_en, bank->context.wake_en, 1);
-	spin_unlock_irqrestore(&bank->lock, flags);
-
-	return 0;
-}
-
-static int omap_gpio_resume(struct device *dev)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-	struct gpio_bank *bank = platform_get_drvdata(pdev);
-	void __iomem *base = bank->base;
-	unsigned long flags;
-
-	if (!bank->mod_usage || !bank->loses_context)
-		return 0;
-
-	if (!bank->regs->wkup_en || !bank->context.wake_en)
-		return 0;
-
-	spin_lock_irqsave(&bank->lock, flags);
-	_gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0);
-	_gpio_rmw(base, bank->regs->wkup_en, bank->context.wake_en, 1);
-	spin_unlock_irqrestore(&bank->lock, flags);
-
-	return 0;
-}
-#endif /* CONFIG_PM_SLEEP */
-
 #if defined(CONFIG_PM_RUNTIME)
 static void omap_gpio_restore_context(struct gpio_bank *bank);
 
@@ -1407,14 +1363,11 @@ static void omap_gpio_restore_context(struct gpio_bank *bank)
 }
 #endif /* CONFIG_PM_RUNTIME */
 #else
-#define omap_gpio_suspend NULL
-#define omap_gpio_resume NULL
 #define omap_gpio_runtime_suspend NULL
 #define omap_gpio_runtime_resume NULL
 #endif
 
 static const struct dev_pm_ops gpio_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(omap_gpio_suspend, omap_gpio_resume)
 	SET_RUNTIME_PM_OPS(omap_gpio_runtime_suspend, omap_gpio_runtime_resume,
 									NULL)
 };
-- 
1.7.0.4

[PATCH v3 10/13] gpio/omap: fix missing dataout context save in _set_gpio_dataout_reg

[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 <govindraj.raja@ti.com>
Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
---
 drivers/gpio/gpio-omap.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 8b4a7ba..04c2677 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -115,6 +115,7 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable)
 		reg += bank->regs->clr_dataout;
 
 	__raw_writel(l, reg);
+	bank->context.dataout = l;
 }
 
 /* set data out value using mask register */
-- 
1.7.0.4

[PATCH v3 11/13] gpio/omap: fix dataout register overwrite in _set_gpio_dataout_*

[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 <tarun.kanti@ti.com>
---
 drivers/gpio/gpio-omap.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 04c2677..2e8e476 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -114,6 +114,7 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable)
 	else
 		reg += bank->regs->clr_dataout;
 
+	l |= __raw_readl(bank->base + bank->regs->set_dataout);
 	__raw_writel(l, reg);
 	bank->context.dataout = l;
 }
@@ -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);
 	__raw_writel(l, reg);
 	bank->context.dataout = l;
 }
-- 
1.7.0.4

[PATCH v3 12/13] gpio/omap: fix incorrect context restore logic in omap_gpio_runtime_resume

[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 <govindraj.raja@ti.com>
Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
---
 drivers/gpio/gpio-omap.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 2e8e476..ccfbae0 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1227,7 +1227,7 @@ static int omap_gpio_runtime_resume(struct device *dev)
 	__raw_writel(bank->context.risingdetect,
 		     bank->base + bank->regs->risingdetect);
 
-	if (!bank->enabled_non_wakeup_gpios || !bank->workaround_enabled) {
+	if (!bank->workaround_enabled) {
 		spin_unlock_irqrestore(&bank->lock, flags);
 		return 0;
 	}
-- 
1.7.0.4

[PATCH v3 13/13] gpio/omap: fix incorrect update to context.irqenable1

[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. 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 <tarun.kanti@ti.com>
---
 drivers/gpio/gpio-omap.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index ccfbae0..8b0d3ab 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -453,7 +453,8 @@ static void _enable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)
 	}
 
 	__raw_writel(l, reg);
-	bank->context.irqenable1 = l;
+	bank->context.irqenable1 =
+			__raw_readl(bank->base + bank->regs->irqenable);
 }
 
 static void _disable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)
@@ -474,7 +475,8 @@ static void _disable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)
 	}
 
 	__raw_writel(l, reg);
-	bank->context.irqenable1 = l;
+	bank->context.irqenable1 =
+			__raw_readl(bank->base + bank->regs->irqenable);
 }
 
 static inline void _set_gpio_irqenable(struct gpio_bank *bank, int gpio, int enable)
-- 
1.7.0.4

[PATCH v3 02/13] gpio/omap: fix wakeup_en register update in _set_gpio_wakeup()

[Santosh Shilimkar]

On Wednesday 07 March 2012 12:15 PM, Tarun Kanti DebBarma wrote:
> 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 <tarun.kanti@ti.com>
> ---
Looks good.
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Regards
Santosh

[PATCH v3 08/13] gpio/omap: remove redundant decoding of gpio offset

[Santosh Shilimkar]

On Wednesday 07 March 2012 12:15 PM, Tarun Kanti DebBarma wrote:
> 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 <linux@arm.linux.org.uk>
> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
Looks good.
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Regards
Santosh

[PATCH v3 09/13] gpio/omap: remove suspend/resume callbacks

[Santosh Shilimkar]

On Wednesday 07 March 2012 12:15 PM, Tarun Kanti DebBarma wrote:
> 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
s/This is/This is also

> register is programmed in omap_gpio_restore_context() called
> which is called from runtime resume callback.
> 
> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
> ---
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Regards
Santosh

[PATCH v3 10/13] gpio/omap: fix missing dataout context save in _set_gpio_dataout_reg

[Santosh Shilimkar]

On Wednesday 07 March 2012 12:16 PM, Tarun Kanti DebBarma wrote:
> 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 <govindraj.raja@ti.com>
> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
> ---
Good catch. Is the suspend/resume caught this issue?

This can go as a fix as well.

Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Regards
Santosh

[PATCH v3 11/13] gpio/omap: fix dataout register overwrite in _set_gpio_dataout_*

[Santosh Shilimkar]

On Wednesday 07 March 2012 12:16 PM, Tarun Kanti DebBarma wrote:
> 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 <tarun.kanti@ti.com>
> ---
This can also go as fix.

Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Regards
Santosh

[PATCH v3 12/13] gpio/omap: fix incorrect context restore logic in omap_gpio_runtime_resume

[Santosh Shilimkar]

On Wednesday 07 March 2012 12:16 PM, Tarun Kanti DebBarma wrote:
> 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 <govindraj.raja@ti.com>
> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
> ---
>  drivers/gpio/gpio-omap.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 2e8e476..ccfbae0 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -1227,7 +1227,7 @@ static int omap_gpio_runtime_resume(struct device *dev)
>  	__raw_writel(bank->context.risingdetect,
>  		     bank->base + bank->regs->risingdetect);
>  
> -	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

[PATCH v3 13/13] gpio/omap: fix incorrect update to context.irqenable1

[Santosh Shilimkar]

On Wednesday 07 March 2012 12:16 PM, Tarun Kanti DebBarma wrote:
> 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 <tarun.kanti@ti.com>
> ---
OK.
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Regards
Santosh

[PATCH v3 10/13] gpio/omap: fix missing dataout context save in _set_gpio_dataout_reg

[DebBarma, Tarun Kanti]

On Wed, Mar 7, 2012 at 5:33 PM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
> On Wednesday 07 March 2012 12:16 PM, Tarun Kanti DebBarma wrote:
>> 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 <govindraj.raja@ti.com>
>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
>> ---
> Good catch. Is the suspend/resume caught this issue?
That's right.
--
Tarun
>
> This can go as a fix as well.
>
> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>
> Regards
> Santosh

[PATCH v3 12/13] gpio/omap: fix incorrect context restore logic in omap_gpio_runtime_resume

[DebBarma, Tarun Kanti]

On Wed, Mar 7, 2012 at 5:37 PM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
> On Wednesday 07 March 2012 12:16 PM, Tarun Kanti DebBarma wrote:
>> 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 <govindraj.raja@ti.com>
>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
>> ---
>> ?drivers/gpio/gpio-omap.c | ? ?2 +-
>> ?1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 2e8e476..ccfbae0 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -1227,7 +1227,7 @@ static int omap_gpio_runtime_resume(struct device *dev)
>> ? ? ? __raw_writel(bank->context.risingdetect,
>> ? ? ? ? ? ? ? ? ? ?bank->base + bank->regs->risingdetect);
>>
>> - ? ? 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;
[...]
--
Tarun

>
> Regards
> Santosh

[PATCH v3 12/13] gpio/omap: fix incorrect context restore logic in omap_gpio_runtime_resume

[Shilimkar, Santosh]

On Thu, Mar 8, 2012 at 4:58 AM, DebBarma, Tarun Kanti
<tarun.kanti@ti.com> wrote:
> On Wed, Mar 7, 2012 at 5:37 PM, Santosh Shilimkar
> <santosh.shilimkar@ti.com> wrote:
>> On Wednesday 07 March 2012 12:16 PM, Tarun Kanti DebBarma wrote:
>>> 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 <govindraj.raja@ti.com>
>>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
>>> ---
>>> ?drivers/gpio/gpio-omap.c | ? ?2 +-
>>> ?1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>> index 2e8e476..ccfbae0 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -1227,7 +1227,7 @@ static int omap_gpio_runtime_resume(struct device *dev)
>>> ? ? ? __raw_writel(bank->context.risingdetect,
>>> ? ? ? ? ? ? ? ? ? ?bank->base + bank->regs->risingdetect);
>>>
>>> - ? ? 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;
> [...]

Make sense now. Thanks for clarification Tarun.
You can add mine..
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Regards
Santosh

[PATCH v3 05/13] gpio/omap: get rid of retrigger variable in gpio_irq_handler

[DebBarma, Tarun Kanti]

On Wed, Mar 7, 2012 at 4:45 PM, Tarun Kanti DebBarma <tarun.kanti@ti.com> wrote:
> 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.
--
Tarun
>
> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Acked-by: Felipe Balbi <balbi@ti.com>
> ---
> ?drivers/gpio/gpio-omap.c | ? ?3 ---
> ?1 files changed, 0 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 3765654..de5fe8f 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -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
>

[PATCH v3 12/13] gpio/omap: fix incorrect context restore logic in omap_gpio_runtime_resume

[DebBarma, Tarun Kanti]

On Thu, Mar 8, 2012 at 12:49 PM, Shilimkar, Santosh
<santosh.shilimkar@ti.com> wrote:
> On Thu, Mar 8, 2012 at 4:58 AM, DebBarma, Tarun Kanti
> <tarun.kanti@ti.com> wrote:
>> On Wed, Mar 7, 2012 at 5:37 PM, Santosh Shilimkar
>> <santosh.shilimkar@ti.com> wrote:
>>> On Wednesday 07 March 2012 12:16 PM, Tarun Kanti DebBarma wrote:
>>>> 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 <govindraj.raja@ti.com>
>>>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
>>>> ---
>>>> ?drivers/gpio/gpio-omap.c | ? ?2 +-
>>>> ?1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>>> index 2e8e476..ccfbae0 100644
>>>> --- a/drivers/gpio/gpio-omap.c
>>>> +++ b/drivers/gpio/gpio-omap.c
>>>> @@ -1227,7 +1227,7 @@ static int omap_gpio_runtime_resume(struct device *dev)
>>>> ? ? ? __raw_writel(bank->context.risingdetect,
>>>> ? ? ? ? ? ? ? ? ? ?bank->base + bank->regs->risingdetect);
>>>>
>>>> - ? ? 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;
>> [...]
>
> Make sense now. Thanks for clarification Tarun.
> You can add mine..
> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Thanks.
I have missed removing the same change from omap_gpio_runtime_suspend().

@@ -1178,9 +1178,6 @@ static int omap_gpio_runtime_suspend(struct device *dev)
         * non-wakeup GPIOs.  Otherwise spurious IRQs will be
         * generated.  See OMAP2420 Errata item 1.101.
         */
-       if (!(bank->enabled_non_wakeup_gpios))
-               goto update_gpio_context_count;
-
FYI, I tested the change on OMAP5 code-base which did not have the above change.
Anyways, I have updated the change in:
git://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev
for_3.4/gpio_further_cleanup_fixes
--
Tarun
>
> Regards
> Santosh

[PATCH v3 05/13] gpio/omap: get rid of retrigger variable in gpio_irq_handler

[Kevin Hilman]

Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:

> 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.

Still missing the changelog changes I requested in v1.

Kevin

> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Acked-by: Felipe Balbi <balbi@ti.com>
> ---
>  drivers/gpio/gpio-omap.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 3765654..de5fe8f 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -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;

[PATCH v3 05/13] gpio/omap: get rid of retrigger variable in gpio_irq_handler

[Kevin Hilman]

"DebBarma, Tarun Kanti" <tarun.kanti@ti.com> writes:

> On Wed, Mar 7, 2012 at 4:45 PM, Tarun Kanti DebBarma <tarun.kanti@ti.com> wrote:
>> 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.

Ah, thanks.

Kevin

[PATCH v3 00/13] gpio/omap: Some more driver cleanup and fixes

[Kevin Hilman]

Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:

> 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

This branch doesn't exist.

Kevin

[PATCH v3 00/13] gpio/omap: Some more driver cleanup and fixes

[DebBarma, Tarun Kanti]

On Tue, Mar 13, 2012 at 12:24 AM, Kevin Hilman <khilman@ti.com> wrote:
> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>
>> 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
>
> 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?
--
Tarun
>
> Kevin
>

[PATCH v3 00/13] gpio/omap: Some more driver cleanup and fixes

[DebBarma, Tarun Kanti]

On Tue, Mar 13, 2012 at 1:23 AM, DebBarma, Tarun Kanti
<tarun.kanti@ti.com> wrote:
> On Tue, Mar 13, 2012 at 12:24 AM, Kevin Hilman <khilman@ti.com> wrote:
>> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>>
>>> 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
>>
>> 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?
Anyways, for the time being I am updating on top of your changes as before.
You should be able to see the branch shortly.
--
Tarun
>>
>> Kevin
>>

[PATCH v3 00/13] gpio/omap: Some more driver cleanup and fixes

[DebBarma, Tarun Kanti]

On Tue, Mar 13, 2012 at 1:38 AM, DebBarma, Tarun Kanti
<tarun.kanti@ti.com> wrote:
> On Tue, Mar 13, 2012 at 1:23 AM, DebBarma, Tarun Kanti
> <tarun.kanti@ti.com> wrote:
>> On Tue, Mar 13, 2012 at 12:24 AM, Kevin Hilman <khilman@ti.com> wrote:
>>> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>>>
>>>> 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
>>>
>>> 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?
> 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
--
Tarun

>>> Kevin
>>>

[PATCH v3 11/13] gpio/omap: fix dataout register overwrite in _set_gpio_dataout_*

[Kevin Hilman]

Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:

> 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 <tarun.kanti@ti.com>
> ---
>  drivers/gpio/gpio-omap.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 04c2677..2e8e476 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -114,6 +114,7 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable)
>  	else
>  		reg += bank->regs->clr_dataout;
>  
> +	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(...)

>  	__raw_writel(l, reg);
>  	bank->context.dataout = l;
>  }
> @@ -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.

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

[PATCH v3 13/13] gpio/omap: fix incorrect update to context.irqenable1

[Kevin Hilman]

Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:

> 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.

> 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


diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 752ae9b..f8b7099 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -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)
@@ -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)

[PATCH v3 00/13] gpio/omap: Some more driver cleanup and fixes

[Kevin Hilman]

"DebBarma, Tarun Kanti" <tarun.kanti@ti.com> writes:

[...]

>>> 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.
> Please note that the branch has your following patch:
> gpio/omap: fix wakeups on level-triggered GPIOs

After addressing my comments on the series, please go ahead and rebase
on Grant's gpio/next branch.

Thanks,

Kevin

[PATCH v3 00/13] gpio/omap: Some more driver cleanup and fixes

[Kevin Hilman]

"DebBarma, Tarun Kanti" <tarun.kanti@ti.com> writes:

[...]

>>> 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.
> 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.

Thanks,

Kevin

[PATCH v3 00/13] gpio/omap: Some more driver cleanup and fixes

[Grant Likely]

On Mon, 12 Mar 2012 15:28:42 -0700, Kevin Hilman <khilman@ti.com> wrote:
> "DebBarma, Tarun Kanti" <tarun.kanti@ti.com> writes:
> 
> [...]
> 
> >>> 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.
> > 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.

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

g.

[PATCH v3 00/13] gpio/omap: Some more driver cleanup and fixes

[DebBarma, Tarun Kanti]

On Tue, Mar 13, 2012 at 9:27 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, 12 Mar 2012 15:28:42 -0700, Kevin Hilman <khilman@ti.com> wrote:
>> "DebBarma, Tarun Kanti" <tarun.kanti@ti.com> writes:
>>
>> [...]
>>
>> >>> 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.
>> > 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.)
Sure.

>>
>> 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.
I will base and start doing the tests.
In the mean time I will wait for Kevin's comments on the series.
Thanks.
--
Tarun

>
> g.
>

[PATCH v3 13/13] gpio/omap: fix incorrect update to context.irqenable1

[DebBarma, Tarun Kanti]

On Tue, Mar 13, 2012 at 3:39 AM, Kevin Hilman <khilman@ti.com> wrote:
> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>
>> 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.
>
>> 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.
Right. I wanted to keep code change minimum.
>
> Instead, how about just move the context update into each side of the
> if/else? ?untested patch below to show what I mean.
Yes, initially I thought of doing in this way as well. I will make the change.
--
Tarun
>
> Kevin
>
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 752ae9b..f8b7099 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -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)
> @@ -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)
>

[PATCH v3 11/13] gpio/omap: fix dataout register overwrite in _set_gpio_dataout_*

[DebBarma, Tarun Kanti]

On Tue, Mar 13, 2012 at 3:24 AM, Kevin Hilman <khilman@ti.com> wrote:
> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>
>> 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 <tarun.kanti@ti.com>
>> ---
>> ?drivers/gpio/gpio-omap.c | ? ?3 +++
>> ?1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 04c2677..2e8e476 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -114,6 +114,7 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable)
>> ? ? ? else
>> ? ? ? ? ? ? ? reg += bank->regs->clr_dataout;
>>
>> + ? ? 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.

>
>> ? ? ? __raw_writel(l, reg);
>> ? ? ? bank->context.dataout = l;
>> ?}
>> @@ -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
>
>> ? ? ? __raw_writel(l, reg);
>> ? ? ? bank->context.dataout = l;
>> ?}

[PATCH v3 11/13] gpio/omap: fix dataout register overwrite in _set_gpio_dataout_*

[DebBarma, Tarun Kanti]

On Tue, Mar 13, 2012 at 11:33 AM, DebBarma, Tarun Kanti
<tarun.kanti@ti.com> wrote:
> On Tue, Mar 13, 2012 at 3:24 AM, Kevin Hilman <khilman@ti.com> wrote:
>> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>>
>>> 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 <tarun.kanti@ti.com>
>>> ---
>>> ?drivers/gpio/gpio-omap.c | ? ?3 +++
>>> ?1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>> index 04c2677..2e8e476 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -114,6 +114,7 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable)
>>> ? ? ? else
>>> ? ? ? ? ? ? ? reg += bank->regs->clr_dataout;
>>>
>>> + ? ? 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.
Also, the read should be: __raw_readl(bank->base + bank->regs->dataout);
instead of bank->regs->set_dataout.
--
Tarun
>
>>
>>> ? ? ? __raw_writel(l, reg);
>>> ? ? ? bank->context.dataout = l;
>>> ?}
>>> @@ -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
>>
>>> ? ? ? __raw_writel(l, reg);
>>> ? ? ? bank->context.dataout = l;
>>> ?}

[PATCH v3 11/13] gpio/omap: fix dataout register overwrite in _set_gpio_dataout_*

[DebBarma, Tarun Kanti]

On Tue, Mar 13, 2012 at 12:03 PM, DebBarma, Tarun Kanti
<tarun.kanti@ti.com> wrote:
> On Tue, Mar 13, 2012 at 11:33 AM, DebBarma, Tarun Kanti
> <tarun.kanti@ti.com> wrote:
>> On Tue, Mar 13, 2012 at 3:24 AM, Kevin Hilman <khilman@ti.com> wrote:
>>> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>>>
>>>> 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 <tarun.kanti@ti.com>
>>>> ---
>>>> ?drivers/gpio/gpio-omap.c | ? ?3 +++
>>>> ?1 files changed, 3 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>>> index 04c2677..2e8e476 100644
>>>> --- a/drivers/gpio/gpio-omap.c
>>>> +++ b/drivers/gpio/gpio-omap.c
>>>> @@ -114,6 +114,7 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable)
>>>> ? ? ? else
>>>> ? ? ? ? ? ? ? reg += bank->regs->clr_dataout;
>>>>
>>>> + ? ? 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.
> 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;
--
Tarun
>>
>>>
>>>> ? ? ? __raw_writel(l, reg);
>>>> ? ? ? bank->context.dataout = l;
>>>> ?}
>>>> @@ -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
>>>
>>>> ? ? ? __raw_writel(l, reg);
>>>> ? ? ? bank->context.dataout = l;
>>>> ?}

[PATCH v3 11/13] gpio/omap: fix dataout register overwrite in _set_gpio_dataout_*

[Kevin Hilman]

"DebBarma, Tarun Kanti" <tarun.kanti@ti.com> writes:

> On Tue, Mar 13, 2012 at 12:03 PM, DebBarma, Tarun Kanti
> <tarun.kanti@ti.com> wrote:
>> On Tue, Mar 13, 2012 at 11:33 AM, DebBarma, Tarun Kanti
>> <tarun.kanti@ti.com> wrote:
>>> On Tue, Mar 13, 2012 at 3:24 AM, Kevin Hilman <khilman@ti.com> wrote:
>>>> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>>>>
>>>>> 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 <tarun.kanti@ti.com>
>>>>> ---
>>>>> ?drivers/gpio/gpio-omap.c | ? ?3 +++
>>>>> ?1 files changed, 3 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>>>> index 04c2677..2e8e476 100644
>>>>> --- a/drivers/gpio/gpio-omap.c
>>>>> +++ b/drivers/gpio/gpio-omap.c
>>>>> @@ -114,6 +114,7 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable)
>>>>> ? ? ? else
>>>>> ? ? ? ? ? ? ? reg += bank->regs->clr_dataout;
>>>>>
>>>>> + ? ? 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.
>> 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;

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

Kevin


diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 0b05629..db905c0 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -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;
+	}

[PATCH v3 11/13] gpio/omap: fix dataout register overwrite in _set_gpio_dataout_*

[DebBarma, Tarun Kanti]

On Tue, Mar 13, 2012 at 9:57 PM, Kevin Hilman <khilman@ti.com> wrote:
> "DebBarma, Tarun Kanti" <tarun.kanti@ti.com> writes:
>
>> On Tue, Mar 13, 2012 at 12:03 PM, DebBarma, Tarun Kanti
>> <tarun.kanti@ti.com> wrote:
>>> On Tue, Mar 13, 2012 at 11:33 AM, DebBarma, Tarun Kanti
>>> <tarun.kanti@ti.com> wrote:
>>>> On Tue, Mar 13, 2012 at 3:24 AM, Kevin Hilman <khilman@ti.com> wrote:
>>>>> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>>>>>
>>>>>> 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 <tarun.kanti@ti.com>
>>>>>> ---
>>>>>> ?drivers/gpio/gpio-omap.c | ? ?3 +++
>>>>>> ?1 files changed, 3 insertions(+), 0 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>>>>> index 04c2677..2e8e476 100644
>>>>>> --- a/drivers/gpio/gpio-omap.c
>>>>>> +++ b/drivers/gpio/gpio-omap.c
>>>>>> @@ -114,6 +114,7 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable)
>>>>>> ? ? ? else
>>>>>> ? ? ? ? ? ? ? reg += bank->regs->clr_dataout;
>>>>>>
>>>>>> + ? ? 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.
>>> 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;
>
> Again, you don't need the extra read-back here. ?Just set/clear the bit
> in the context. ?Untested patch below.
Alright.
--
Tarun
>
> Kevin
>
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 0b05629..db905c0 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -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;
> + ? ? ? }