[PATCH 0/6] gpio/omap: Some more driver cleanup and fixes

[PATCH 0/6] 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 fix include correction of _set_gpio_irqenable() implementation and fix
type mismatch of gpio trigger parameter.

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

I have applied Benoit's GPIO patches in following series on top of Kevin's
before applying my changes.
gpio/omap: Cleanup and adaptation to Device Tree
git://git.kernel.org/pub/scm/linux/kernel/git/bcousson/linux-omap-dt.git for_3.4/dt_gpio

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

Tarun Kanti DebBarma (6):
  gpio/omap: remove saved_fallingdetect, saved_risingdetect fields
  gpio/omap: remove saved_wakeup field from struct gpio_bank
  gpio/omap: remove suspend_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

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

[PATCH 1/6] 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>
---
 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 f29252f..40a1fb2 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -59,8 +59,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;
@@ -1224,11 +1222,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;
 
@@ -1287,9 +1283,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);
 
@@ -1306,14 +1302,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 2/6] gpio/omap: remove saved_wakeup field from struct gpio_bank

[Tarun Kanti DebBarma]

There is no more need to have saved_wakeup. Instead we can use
context.wake_en which holds the current wakeup enable register
context. This also means that the read from wakeup enable register
is not needed.

Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
---
 drivers/gpio/gpio-omap.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 40a1fb2..64f15d5 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -54,7 +54,6 @@ struct gpio_bank {
 	u16 irq;
 	u16 virtual_irq_start;
 	u32 suspend_wakeup;
-	u32 saved_wakeup;
 	u32 non_wakeup_gpios;
 	u32 enabled_non_wakeup_gpios;
 	struct gpio_regs context;
@@ -772,7 +771,7 @@ 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);
+	bank->context.wake_en = __raw_readl(mask_reg);
 	__raw_writel(0xffff & ~bank->suspend_wakeup, mask_reg);
 	spin_unlock_irqrestore(&bank->lock, flags);
 
@@ -788,7 +787,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;
@@ -1133,7 +1132,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)
@@ -1142,10 +1140,7 @@ static int omap_gpio_suspend(struct device *dev)
 	if (!bank->regs->wkup_en || !bank->suspend_wakeup)
 		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->suspend_wakeup, 1);
 	spin_unlock_irqrestore(&bank->lock, flags);
@@ -1163,12 +1158,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 3/6] 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>
---
 drivers/gpio/gpio-omap.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 64f15d5..b62e861 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -53,7 +53,6 @@ struct gpio_bank {
 	void __iomem *base;
 	u16 irq;
 	u16 virtual_irq_start;
-	u32 suspend_wakeup;
 	u32 non_wakeup_gpios;
 	u32 enabled_non_wakeup_gpios;
 	struct gpio_regs context;
@@ -497,9 +496,9 @@ 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;
 
 	spin_unlock_irqrestore(&bank->lock, flags);
 
@@ -772,7 +771,7 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
 
 	spin_lock_irqsave(&bank->lock, flags);
 	bank->context.wake_en = __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;
@@ -1137,12 +1136,12 @@ 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;
 
 	spin_lock_irqsave(&bank->lock, flags);
 	_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 4/6] 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>
---
 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 b62e861..3dd4b3a 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -623,7 +623,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);
 
@@ -660,8 +659,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 5/6] 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>
---
 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 3dd4b3a..67535c8 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -232,7 +232,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;
@@ -314,7 +314,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 6/6] 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>
---
 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 67535c8..acc71a0 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -473,7 +473,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

Re: [PATCH 0/6] gpio/omap: Some more driver cleanup and fixes

[Shilimkar, Santosh]

On Thu, Feb 23, 2012 at 5:40 PM, Tarun Kanti DebBarma
<tarun.kanti@ti.com> wrote:
> 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 fix include correction of _set_gpio_irqenable() implementation and fix
> type mismatch of gpio trigger parameter.
>
> 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
>
> I have applied Benoit's GPIO patches in following series on top of Kevin's
> before applying my changes.
> gpio/omap: Cleanup and adaptation to Device Tree
> git://git.kernel.org/pub/scm/linux/kernel/git/bcousson/linux-omap-dt.git for_3.4/dt_gpio
>
> 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
>
> Tarun Kanti DebBarma (6):
>  gpio/omap: remove saved_fallingdetect, saved_risingdetect fields
>  gpio/omap: remove saved_wakeup field from struct gpio_bank
>  gpio/omap: remove suspend_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
>
>  drivers/gpio/gpio-omap.c |   56 +++++++++++++++++++--------------------------
>  1 files changed, 24 insertions(+), 32 deletions(-)
>
Nice clean-up series. I have gone through this series one more time
Thanks for updating change-logs. I noticed you dropped the edge triggered
irq wakeup fix....I see on the list now.... Kevin has fixed that already.

Series looks good to me. You can add:
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Regards
Santosh

Re: [PATCH 1/6] gpio/omap: remove saved_fallingdetect, saved_risingdetect fields

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 408 bytes --]

On Thu, Feb 23, 2012 at 05:40:26PM +0530, Tarun Kanti DebBarma wrote:
> 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>

Looks trivial and correct:

Acked-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/6] gpio/omap: remove saved_wakeup field from struct gpio_bank

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]

On Thu, Feb 23, 2012 at 05:40:27PM +0530, Tarun Kanti DebBarma wrote:
> There is no more need to have saved_wakeup. Instead we can use
> context.wake_en which holds the current wakeup enable register
> context. This also means that the read from wakeup enable register
> is not needed.
> 
> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>

Likewise:

Acked-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/6] gpio/omap: remove suspend_wakeup field from struct gpio_bank

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 351 bytes --]

On Thu, Feb 23, 2012 at 05:40:28PM +0530, Tarun Kanti DebBarma wrote:
> 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>

Likewise:

Acked-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/6] gpio/omap: get rid of retrigger variable in gpio_irq_handler

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 360 bytes --]

On Thu, Feb 23, 2012 at 05:40:29PM +0530, Tarun Kanti DebBarma 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.
> 
> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>

hehe, makes a lot of sense :-)

Acked-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/6] gpio/omap: fix trigger type to unsigned

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1062 bytes --]

On Thu, Feb 23, 2012 at 05:40:30PM +0530, Tarun Kanti DebBarma wrote:
> 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>

Cool, I guess sparse would also have caught that error :-)

Acked-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 6/6] gpio/omap: fix _set_gpio_irqenable implementation

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

On Thu, Feb 23, 2012 at 05:40:31PM +0530, Tarun Kanti DebBarma wrote:
> 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>

This one should probably be ported to stable releases, adding
stable@vger to the cc list

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 67535c8..acc71a0 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -473,7 +473,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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/6] gpio/omap: fix trigger type to unsigned

[Shubhrajyoti]

On Thursday 23 February 2012 05:40 PM, Tarun Kanti DebBarma wrote:
> 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>
> ---
>  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 3dd4b3a..67535c8 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -232,7 +232,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;
> @@ -314,7 +314,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)
you meant unsigned int ? could it be made u32.
>  {
>  	void __iomem *reg = bank->base;
>  	void __iomem *base = bank->base;

Re: [PATCH 5/6] gpio/omap: fix trigger type to unsigned

[Russell King - ARM Linux]

On Thu, Feb 23, 2012 at 06:09:16PM +0530, Shubhrajyoti wrote:
> On Thursday 23 February 2012 05:40 PM, Tarun Kanti DebBarma wrote:
> > -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)
> you meant unsigned int ? could it be made u32.

In C, unsigned is the same as unsigned int.

Re: [PATCH 2/6] gpio/omap: remove saved_wakeup field from struct gpio_bank

[Kevin Hilman]

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

> There is no more need to have saved_wakeup. Instead we can use
> context.wake_en which holds the current wakeup enable register
> context. This also means that the read from wakeup enable register
> is not needed.
>
> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>

Looks right, but one question below...

> ---
>  drivers/gpio/gpio-omap.c |   13 ++++---------
>  1 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 40a1fb2..64f15d5 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -54,7 +54,6 @@ struct gpio_bank {
>  	u16 irq;
>  	u16 virtual_irq_start;
>  	u32 suspend_wakeup;
> -	u32 saved_wakeup;
>  	u32 non_wakeup_gpios;
>  	u32 enabled_non_wakeup_gpios;
>  	struct gpio_regs context;
> @@ -772,7 +771,7 @@ 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);
> +	bank->context.wake_en = __raw_readl(mask_reg);

Why is this read needed?

Kevin

>  	__raw_writel(0xffff & ~bank->suspend_wakeup, mask_reg);
>  	spin_unlock_irqrestore(&bank->lock, flags);
>  

Re: [PATCH 3/6] gpio/omap: remove suspend_wakeup field from struct gpio_bank

[Kevin Hilman]

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

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

I'm not crazy about this change...

> ---
>  drivers/gpio/gpio-omap.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 64f15d5..b62e861 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -53,7 +53,6 @@ struct gpio_bank {
>  	void __iomem *base;
>  	u16 irq;
>  	u16 virtual_irq_start;
> -	u32 suspend_wakeup;
>  	u32 non_wakeup_gpios;
>  	u32 enabled_non_wakeup_gpios;
>  	struct gpio_regs context;
> @@ -497,9 +496,9 @@ 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;

The bank->context values are expected to be copies of the actual
register contents, and here that is clearly not the case.

With this change, you're using the context register to track changes
that you *might* eventually write to the register.

IMO, this is more confusing than having a separate field to track this.

Kevin

>  	spin_unlock_irqrestore(&bank->lock, flags);
>  
> @@ -772,7 +771,7 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
>  
>  	spin_lock_irqsave(&bank->lock, flags);
>  	bank->context.wake_en = __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;
> @@ -1137,12 +1136,12 @@ 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;
>  
>  	spin_lock_irqsave(&bank->lock, flags);
>  	_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;

Re: [PATCH 4/6] 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.
>
> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>

Looks like the use of this was removed when I moved things over to using
the generic IRQ framework, but I didn't fully clean up.

Can you update the changelog to something along the lines of:

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

Thanks,

Kevin

> ---
>  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 b62e861..3dd4b3a 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -623,7 +623,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);
>  
> @@ -660,8 +659,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;

Re: [PATCH 6/6] gpio/omap: fix _set_gpio_irqenable implementation

[Kevin Hilman]

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

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

Hmm, interesting.

This means that the IRQ mask/unmask stuff is not actually doing anything
since it's always leaving the IRQ enabled.  Curious that we haven't seen
side effects of that.  Maybe since the trigger type is none, the
interrupts won't fire.

In any case, this is a good fix.

Kevin


> ---
>  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 67535c8..acc71a0 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -473,7 +473,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));
>  }
>  
>  /*

Re: [PATCH 2/6] gpio/omap: remove saved_wakeup field from struct gpio_bank

[DebBarma, Tarun Kanti]

On Tue, Feb 28, 2012 at 5:20 AM, Kevin Hilman <khilman@ti.com> wrote:
> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>
>> There is no more need to have saved_wakeup. Instead we can use
>> context.wake_en which holds the current wakeup enable register
>> context. This also means that the read from wakeup enable register
>> is not needed.
>>
>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
>
> Looks right, but one question below...
>
>> ---
>>  drivers/gpio/gpio-omap.c |   13 ++++---------
>>  1 files changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 40a1fb2..64f15d5 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -54,7 +54,6 @@ struct gpio_bank {
>>       u16 irq;
>>       u16 virtual_irq_start;
>>       u32 suspend_wakeup;
>> -     u32 saved_wakeup;
>>       u32 non_wakeup_gpios;
>>       u32 enabled_non_wakeup_gpios;
>>       struct gpio_regs context;
>> @@ -772,7 +771,7 @@ 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);
>> +     bank->context.wake_en = __raw_readl(mask_reg);
>
> Why is this read needed?
Well, we don't really need as we already have context.wake_en updated elsewhere.
I will update this. Thanks.
--
Tarun
>
> Kevin
>
>>       __raw_writel(0xffff & ~bank->suspend_wakeup, mask_reg);
>>       spin_unlock_irqrestore(&bank->lock, flags);
>>

Re: [PATCH 4/6] gpio/omap: get rid of retrigger variable in gpio_irq_handler

[DebBarma, Tarun Kanti]

On Tue, Feb 28, 2012 at 5:32 AM, Kevin Hilman <khilman@ti.com> wrote:
> 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.
>>
>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
>
> Looks like the use of this was removed when I moved things over to using
> the generic IRQ framework, but I didn't fully clean up.
>
> Can you update the changelog to something along the lines of:
>
> "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.
Yes, sure.
--
Tarun
>
> Thanks,
>
> Kevin
>
>> ---
>>  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 b62e861..3dd4b3a 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -623,7 +623,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);
>>
>> @@ -660,8 +659,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;

Re: [PATCH 3/6] gpio/omap: remove suspend_wakeup field from struct gpio_bank

[DebBarma, Tarun Kanti]

On Tue, Feb 28, 2012 at 5:24 AM, Kevin Hilman <khilman@ti.com> wrote:
> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>
>> 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>
>
> I'm not crazy about this change...
>
>> ---
>>  drivers/gpio/gpio-omap.c |   11 +++++------
>>  1 files changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 64f15d5..b62e861 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -53,7 +53,6 @@ struct gpio_bank {
>>       void __iomem *base;
>>       u16 irq;
>>       u16 virtual_irq_start;
>> -     u32 suspend_wakeup;
>>       u32 non_wakeup_gpios;
>>       u32 enabled_non_wakeup_gpios;
>>       struct gpio_regs context;
>> @@ -497,9 +496,9 @@ 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;
>
> The bank->context values are expected to be copies of the actual
> register contents, and here that is clearly not the case.
Right, it should have been this:

        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->context.wake_en, bank->base + bank->regs->wkup_en);

>
> With this change, you're using the context register to track changes
> that you *might* eventually write to the register.
The above change ensures that bank->context.wake_en reflects the
latest register value.
There are two distinct paths through which bank->context.wake_en is
updated now, viz:
Path1:-
chip.irq_set_type() --> gpio_irq_type() --> _set_gpio_triggering() -->
set_gpio_trigger()

Path2:-
chip.irq_set_wake() --> gpio_wake_enable() --> irq_set_wake()

>
> IMO, this is more confusing than having a separate field to track this.
So, there is no need have a separate field to keep track of this.
I hope my understanding is right.
--
Tarun

>
> Kevin
>
>>       spin_unlock_irqrestore(&bank->lock, flags);
>>
>> @@ -772,7 +771,7 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
>>
>>       spin_lock_irqsave(&bank->lock, flags);
>>       bank->context.wake_en = __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;
>> @@ -1137,12 +1136,12 @@ 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;
>>
>>       spin_lock_irqsave(&bank->lock, flags);
>>       _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;

Re: [PATCH 3/6] gpio/omap: remove suspend_wakeup field from struct gpio_bank

[DebBarma, Tarun Kanti]

On Tue, Feb 28, 2012 at 3:09 PM, DebBarma, Tarun Kanti
<tarun.kanti@ti.com> wrote:
> On Tue, Feb 28, 2012 at 5:24 AM, Kevin Hilman <khilman@ti.com> wrote:
>> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>>
>>> 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>
>>
>> I'm not crazy about this change...
>>
>>> ---
>>>  drivers/gpio/gpio-omap.c |   11 +++++------
>>>  1 files changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>> index 64f15d5..b62e861 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -53,7 +53,6 @@ struct gpio_bank {
>>>       void __iomem *base;
>>>       u16 irq;
>>>       u16 virtual_irq_start;
>>> -     u32 suspend_wakeup;
>>>       u32 non_wakeup_gpios;
>>>       u32 enabled_non_wakeup_gpios;
>>>       struct gpio_regs context;
>>> @@ -497,9 +496,9 @@ 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;
>>
>> The bank->context values are expected to be copies of the actual
>> register contents, and here that is clearly not the case.
> Right, it should have been this:
>
>        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->context.wake_en, bank->base + bank->regs->wkup_en);
>
>>
>> With this change, you're using the context register to track changes
>> that you *might* eventually write to the register.
> The above change ensures that bank->context.wake_en reflects the
> latest register value.
> There are two distinct paths through which bank->context.wake_en is
> updated now, viz:
> Path1:-
> chip.irq_set_type() --> gpio_irq_type() --> _set_gpio_triggering() -->
> set_gpio_trigger()
>
> Path2:-
> chip.irq_set_wake() --> gpio_wake_enable() --> irq_set_wake()
Sorry, it should have been:
chip.irq_set_wake() --> gpio_wake_enable() --> _set_gpio_wakeup()

>
>>
>> IMO, this is more confusing than having a separate field to track this.
> So, there is no need have a separate field to keep track of this.
> I hope my understanding is right.
> --
> Tarun
>
>>
>> Kevin
>>
>>>       spin_unlock_irqrestore(&bank->lock, flags);
>>>
>>> @@ -772,7 +771,7 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
>>>
>>>       spin_lock_irqsave(&bank->lock, flags);
>>>       bank->context.wake_en = __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;
>>> @@ -1137,12 +1136,12 @@ 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;
>>>
>>>       spin_lock_irqsave(&bank->lock, flags);
>>>       _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;

Re: [PATCH 3/6] gpio/omap: remove suspend_wakeup field from struct gpio_bank

[Kevin Hilman]

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

> On Tue, Feb 28, 2012 at 5:24 AM, Kevin Hilman <khilman@ti.com> wrote:
>> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>>
>>> 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>
>>
>> I'm not crazy about this change...
>>
>>> ---
>>>  drivers/gpio/gpio-omap.c |   11 +++++------
>>>  1 files changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>> index 64f15d5..b62e861 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -53,7 +53,6 @@ struct gpio_bank {
>>>       void __iomem *base;
>>>       u16 irq;
>>>       u16 virtual_irq_start;
>>> -     u32 suspend_wakeup;
>>>       u32 non_wakeup_gpios;
>>>       u32 enabled_non_wakeup_gpios;
>>>       struct gpio_regs context;
>>> @@ -497,9 +496,9 @@ 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;
>>
>> The bank->context values are expected to be copies of the actual
>> register contents, and here that is clearly not the case.
> Right, it should have been this:
>
>         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->context.wake_en, bank->base + bank->regs->wkup_en);
>
>>
>> With this change, you're using the context register to track changes
>> that you *might* eventually write to the register.
> The above change ensures that bank->context.wake_en reflects the
> latest register value.

OK, but that changes the behavior of the current code.

The current code *only* writes this register in suspend and resume.
_set_gpio_wakeup() just records the value that is going to be written in
suspend.

Now, I'm not saying we shouldn't make the changes you propose above.  We
probably should be updating the wake-enable register whenever
_set_gpio_wakeup() is run so that GPIO wakeups work across runtime
suspend/resume as well.

However, you should probably make that functional change a separate
patch *before* you do $SUBJECT patch which just changes the variable
used to cache the register contents.

Kevin


> There are two distinct paths through which bank->context.wake_en is
> updated now, viz:
> Path1:-
> chip.irq_set_type() --> gpio_irq_type() --> _set_gpio_triggering() -->
> set_gpio_trigger()
>
> Path2:-
> chip.irq_set_wake() --> gpio_wake_enable() --> irq_set_wake()
>
>>
>> IMO, this is more confusing than having a separate field to track this.
> So, there is no need have a separate field to keep track of this.
> I hope my understanding is right.
> --
> Tarun
>
>>
>> Kevin
>>
>>>       spin_unlock_irqrestore(&bank->lock, flags);
>>>
>>> @@ -772,7 +771,7 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
>>>
>>>       spin_lock_irqsave(&bank->lock, flags);
>>>       bank->context.wake_en = __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;
>>> @@ -1137,12 +1136,12 @@ 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;
>>>
>>>       spin_lock_irqsave(&bank->lock, flags);
>>>       _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;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] gpio/omap: remove suspend_wakeup field from struct gpio_bank

[DebBarma, Tarun Kanti]

On Wed, Feb 29, 2012 at 12:15 AM, Kevin Hilman <khilman@ti.com> wrote:
> "DebBarma, Tarun Kanti" <tarun.kanti@ti.com> writes:
>
>> On Tue, Feb 28, 2012 at 5:24 AM, Kevin Hilman <khilman@ti.com> wrote:
>>> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>>>
>>>> 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>
>>>
>>> I'm not crazy about this change...
>>>
>>>> ---
>>>>  drivers/gpio/gpio-omap.c |   11 +++++------
>>>>  1 files changed, 5 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>>> index 64f15d5..b62e861 100644
>>>> --- a/drivers/gpio/gpio-omap.c
>>>> +++ b/drivers/gpio/gpio-omap.c
>>>> @@ -53,7 +53,6 @@ struct gpio_bank {
>>>>       void __iomem *base;
>>>>       u16 irq;
>>>>       u16 virtual_irq_start;
>>>> -     u32 suspend_wakeup;
>>>>       u32 non_wakeup_gpios;
>>>>       u32 enabled_non_wakeup_gpios;
>>>>       struct gpio_regs context;
>>>> @@ -497,9 +496,9 @@ 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;
>>>
>>> The bank->context values are expected to be copies of the actual
>>> register contents, and here that is clearly not the case.
>> Right, it should have been this:
>>
>>         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->context.wake_en, bank->base + bank->regs->wkup_en);
>>
>>>
>>> With this change, you're using the context register to track changes
>>> that you *might* eventually write to the register.
>> The above change ensures that bank->context.wake_en reflects the
>> latest register value.
>
> OK, but that changes the behavior of the current code.
Agreed.

>
> The current code *only* writes this register in suspend and resume.
> _set_gpio_wakeup() just records the value that is going to be written in
> suspend.
Yes.
>
> Now, I'm not saying we shouldn't make the changes you propose above.  We
> probably should be updating the wake-enable register whenever
> _set_gpio_wakeup() is run so that GPIO wakeups work across runtime
> suspend/resume as well.
>
> However, you should probably make that functional change a separate
> patch *before* you do $SUBJECT patch which just changes the variable
> used to cache the register contents.
Sure, I will make this change.
--
Tarun
>
> Kevin
>
>
>> There are two distinct paths through which bank->context.wake_en is
>> updated now, viz:
>> Path1:-
>> chip.irq_set_type() --> gpio_irq_type() --> _set_gpio_triggering() -->
>> set_gpio_trigger()
>>
>> Path2:-
>> chip.irq_set_wake() --> gpio_wake_enable() --> irq_set_wake()
>>
>>>
>>> IMO, this is more confusing than having a separate field to track this.
>> So, there is no need have a separate field to keep track of this.
>> I hope my understanding is right.
>> --
>> Tarun
>>
>>>
>>> Kevin
>>>
>>>>       spin_unlock_irqrestore(&bank->lock, flags);
>>>>
>>>> @@ -772,7 +771,7 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
>>>>
>>>>       spin_lock_irqsave(&bank->lock, flags);
>>>>       bank->context.wake_en = __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;
>>>> @@ -1137,12 +1136,12 @@ 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;
>>>>
>>>>       spin_lock_irqsave(&bank->lock, flags);
>>>>       _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;
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] gpio/omap: Some more driver cleanup and fixes

[Grant Likely]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2340 bytes --]

On Thu, 23 Feb 2012 17:56:33 +0530, "Shilimkar, Santosh" <santosh.shilimkar@ti.com> wrote:
> On Thu, Feb 23, 2012 at 5:40 PM, Tarun Kanti DebBarma
> <tarun.kanti@ti.com> wrote:
> > 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 fix include correction of _set_gpio_irqenable() implementation and fix
> > type mismatch of gpio trigger parameter.
> >
> > 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
> >
> > I have applied Benoit's GPIO patches in following series on top of Kevin's
> > before applying my changes.
> > gpio/omap: Cleanup and adaptation to Device Tree
> > git://git.kernel.org/pub/scm/linux/kernel/git/bcousson/linux-omap-dt.git for_3.4/dt_gpio
> >
> > 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
> >
> > Tarun Kanti DebBarma (6):
> >  gpio/omap: remove saved_fallingdetect, saved_risingdetect fields
> >  gpio/omap: remove saved_wakeup field from struct gpio_bank
> >  gpio/omap: remove suspend_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
> >
> >  drivers/gpio/gpio-omap.c |   56 +++++++++++++++++++--------------------------
> >  1 files changed, 24 insertions(+), 32 deletions(-)
> >
> Nice clean-up series. I have gone through this series one more time
> Thanks for updating change-logs. I noticed you dropped the edge triggered
> irq wakeup fix....I see on the list now.... Kevin has fixed that already.
> 
> Series looks good to me. You can add:
> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

What's the status of this series?  Should I be expecting a v2?  Or am I supposed
to pick up this one? (a pull req would make things easier for me)

g.

Re: [PATCH 0/6] gpio/omap: Some more driver cleanup and fixes

[Kevin Hilman]

Grant Likely <grant.likely@secretlab.ca> writes:

> On Thu, 23 Feb 2012 17:56:33 +0530, "Shilimkar, Santosh" <santosh.shilimkar@ti.com> wrote:
>> On Thu, Feb 23, 2012 at 5:40 PM, Tarun Kanti DebBarma
>> <tarun.kanti@ti.com> wrote:
>> > 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 fix include correction of _set_gpio_irqenable() implementation and fix
>> > type mismatch of gpio trigger parameter.
>> >
>> > 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
>> >
>> > I have applied Benoit's GPIO patches in following series on top of Kevin's
>> > before applying my changes.
>> > gpio/omap: Cleanup and adaptation to Device Tree
>> > git://git.kernel.org/pub/scm/linux/kernel/git/bcousson/linux-omap-dt.git for_3.4/dt_gpio
>> >
>> > 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
>> >
>> > Tarun Kanti DebBarma (6):
>> >  gpio/omap: remove saved_fallingdetect, saved_risingdetect fields
>> >  gpio/omap: remove saved_wakeup field from struct gpio_bank
>> >  gpio/omap: remove suspend_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
>> >
>> >  drivers/gpio/gpio-omap.c |   56 +++++++++++++++++++--------------------------
>> >  1 files changed, 24 insertions(+), 32 deletions(-)
>> >
>> Nice clean-up series. I have gone through this series one more time
>> Thanks for updating change-logs. I noticed you dropped the edge triggered
>> irq wakeup fix....I see on the list now.... Kevin has fixed that already.
>> 
>> Series looks good to me. You can add:
>> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>
> What's the status of this series?  Should I be expecting a v2?  Or am I supposed
> to pick up this one? (a pull req would make things easier for me)

I haven't been through this one yet.

You can expect a pull req from me when ready.

Kevin

Re: [PATCH 0/6] gpio/omap: Some more driver cleanup and fixes

[Grant Likely]

On Mon, Mar 12, 2012 at 12:42 PM, Kevin Hilman <khilman@ti.com> wrote:
> Grant Likely <grant.likely@secretlab.ca> writes:
>
>> On Thu, 23 Feb 2012 17:56:33 +0530, "Shilimkar, Santosh" <santosh.shilimkar@ti.com> wrote:
>>> On Thu, Feb 23, 2012 at 5:40 PM, Tarun Kanti DebBarma
>>> <tarun.kanti@ti.com> wrote:
>>> > 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 fix include correction of _set_gpio_irqenable() implementation and fix
>>> > type mismatch of gpio trigger parameter.
>>> >
>>> > 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
>>> >
>>> > I have applied Benoit's GPIO patches in following series on top of Kevin's
>>> > before applying my changes.
>>> > gpio/omap: Cleanup and adaptation to Device Tree
>>> > git://git.kernel.org/pub/scm/linux/kernel/git/bcousson/linux-omap-dt.git for_3.4/dt_gpio
>>> >
>>> > 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
>>> >
>>> > Tarun Kanti DebBarma (6):
>>> >  gpio/omap: remove saved_fallingdetect, saved_risingdetect fields
>>> >  gpio/omap: remove saved_wakeup field from struct gpio_bank
>>> >  gpio/omap: remove suspend_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
>>> >
>>> >  drivers/gpio/gpio-omap.c |   56 +++++++++++++++++++--------------------------
>>> >  1 files changed, 24 insertions(+), 32 deletions(-)
>>> >
>>> Nice clean-up series. I have gone through this series one more time
>>> Thanks for updating change-logs. I noticed you dropped the edge triggered
>>> irq wakeup fix....I see on the list now.... Kevin has fixed that already.
>>>
>>> Series looks good to me. You can add:
>>> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>
>> What's the status of this series?  Should I be expecting a v2?  Or am I supposed
>> to pick up this one? (a pull req would make things easier for me)
>
> I haven't been through this one yet.
>
> You can expect a pull req from me when ready.

Perfect, thx.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.