[PATCH v4 00/12] gpio/omap: Some more driver cleanup and fixes

[PATCH v4 00/12] 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. Also, the suspend/resume callbacks are
removed bacause they are not needed any more.

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:
git://git.secretlab.ca/git/linux-2.6.git gpio/next
Commit: 81b279d80a63628e580c71a31d30a8c3b3047ad4

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

Power Test: 
Off-mode and Retention on OMAP3430 (Suspend and Idle paths).
Also confirmed that dataout register content preserved over
off-mode.
            
Functional Test:
OMAP2430, OMAP3430SDP, ZOOM3, OMAP4430, OMAP4-BLAZE, PANDA 

v4:
a) Implemented all comments on v3 which are mostly related to
avoiding unnecessary register read while updating the context.

b) Folded:
gpio/omap: fix dataout register overwrite in _set_gpio_dataout
into:
gpio/omap: fix missing dataout context save in _set_gpio_dataout_reg

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 (12):
  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 incorrect context restore logic in omap_gpio_runtime_*
  gpio/omap: fix incorrect update to context.irqenable1

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

[PATCH v4 01/12] 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 7cbad85..0c1c529 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -63,8 +63,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;
@@ -1244,11 +1242,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;
 
@@ -1307,9 +1303,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);
 
@@ -1326,14 +1322,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 v4 02/12] 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 0c1c529..171f951 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -509,6 +509,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 v4 03/12] 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 171f951..3a4f151 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -57,7 +57,6 @@ struct gpio_bank {
 	u16 irq;
 	int irq_base;
 	struct irq_domain *domain;
-	u32 suspend_wakeup;
 	u32 saved_wakeup;
 	u32 non_wakeup_gpios;
 	u32 enabled_non_wakeup_gpios;
@@ -505,11 +504,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;
@@ -779,7 +778,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;
@@ -1160,7 +1159,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;
@@ -1168,7 +1167,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 v4 04/12] 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 3a4f151..3b91ade 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -57,7 +57,6 @@ struct gpio_bank {
 	u16 irq;
 	int irq_base;
 	struct irq_domain *domain;
-	u32 saved_wakeup;
 	u32 non_wakeup_gpios;
 	u32 enabled_non_wakeup_gpios;
 	struct gpio_regs context;
@@ -777,7 +776,6 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
 	unsigned long		flags;
 
 	spin_lock_irqsave(&bank->lock, flags);
-	bank->saved_wakeup = __raw_readl(mask_reg);
 	__raw_writel(0xffff & ~bank->context.wake_en, mask_reg);
 	spin_unlock_irqrestore(&bank->lock, flags);
 
@@ -793,7 +791,7 @@ static int omap_mpuio_resume_noirq(struct device *dev)
 	unsigned long		flags;
 
 	spin_lock_irqsave(&bank->lock, flags);
-	__raw_writel(bank->saved_wakeup, mask_reg);
+	__raw_writel(bank->context.wake_en, mask_reg);
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	return 0;
@@ -1153,7 +1151,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)
@@ -1162,10 +1159,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);
@@ -1183,12 +1177,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 v4 04/12] gpio/omap: remove saved_wakeup field from struct gpio_bank

[Roger Quadros]

Hi,

Just bumped across this patch and have a query.

On 03/16/2012 04:05 PM, Tarun Kanti DebBarma wrote:
> 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 3a4f151..3b91ade 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -57,7 +57,6 @@ struct gpio_bank {
>  	u16 irq;
>  	int irq_base;
>  	struct irq_domain *domain;
> -	u32 saved_wakeup;
>  	u32 non_wakeup_gpios;
>  	u32 enabled_non_wakeup_gpios;
>  	struct gpio_regs context;
> @@ -777,7 +776,6 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
>  	unsigned long		flags;
>  
>  	spin_lock_irqsave(&bank->lock, flags);
> -	bank->saved_wakeup = __raw_readl(mask_reg);
>  	__raw_writel(0xffff & ~bank->context.wake_en, mask_reg);

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

>  	spin_unlock_irqrestore(&bank->lock, flags);
>  
> @@ -793,7 +791,7 @@ static int omap_mpuio_resume_noirq(struct device *dev)
>  	unsigned long		flags;
>  
>  	spin_lock_irqsave(&bank->lock, flags);
> -	__raw_writel(bank->saved_wakeup, mask_reg);
> +	__raw_writel(bank->context.wake_en, mask_reg);

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

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

>  	spin_unlock_irqrestore(&bank->lock, flags);
>  
>  	return 0;


cheers,
-roger

[PATCH v4 04/12] gpio/omap: remove saved_wakeup field from struct gpio_bank

[DebBarma, Tarun Kanti]

Hi Roger,

On Mon, Jul 9, 2012 at 3:13 PM, Roger Quadros <rogerq@ti.com> wrote:
> Hi,
>
> Just bumped across this patch and have a query.
>
> On 03/16/2012 04:05 PM, Tarun Kanti DebBarma wrote:
>> 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 3a4f151..3b91ade 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -57,7 +57,6 @@ struct gpio_bank {
>>       u16 irq;
>>       int irq_base;
>>       struct irq_domain *domain;
>> -     u32 saved_wakeup;
>>       u32 non_wakeup_gpios;
>>       u32 enabled_non_wakeup_gpios;
>>       struct gpio_regs context;
>> @@ -777,7 +776,6 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
>>       unsigned long           flags;
>>
>>       spin_lock_irqsave(&bank->lock, flags);
>> -     bank->saved_wakeup = __raw_readl(mask_reg);
>>       __raw_writel(0xffff & ~bank->context.wake_en, mask_reg);
>
> OK, here you are overwriting the mask_reg with the wakeup bitmask
> without saving the mask_reg's original content.
This is based upon understanding that set_gpio_trigger() is the common
function where update of wake_en register takes place. Unless, mask_reg
in this case refers to something else, effectively we would be saving the
same value to saved_wakeup what is already present in wake_en.
I will verify this specific to this function.

>
>>       spin_unlock_irqrestore(&bank->lock, flags);
>>
>> @@ -793,7 +791,7 @@ static int omap_mpuio_resume_noirq(struct device *dev)
>>       unsigned long           flags;
>>
>>       spin_lock_irqsave(&bank->lock, flags);
>> -     __raw_writel(bank->saved_wakeup, mask_reg);
>> +     __raw_writel(bank->context.wake_en, mask_reg);
>
> Now you are restoring nothing but the same content that you stored
> during suspend. This will cause the non-wakeup gpio interrupts to get
> masked between a suspend/resume. So isn't this a bug?
That's right, the same value is restored back which was last updated in
set_gpio_trigger() that got stored in wake_en register. Let me know if
I am missing your points here.

>
> Proper solution would be to save the mask_reg context into another
> register than context.wake_en during suspend.
As I said, this would make sense if mask_reg is referring to different
register than what is used in set_gpio_trigger(). I will have a look.

BTW, did you observe anything unusual during some testing?

Thanks.
--
Tarun
>
>>       spin_unlock_irqrestore(&bank->lock, flags);
>>
>>       return 0;
>
>
> cheers,
> -roger

[PATCH v4 04/12] gpio/omap: remove saved_wakeup field from struct gpio_bank

[Roger Quadros]

Tarun,

On 07/09/2012 02:16 PM, DebBarma, Tarun Kanti wrote:
> Hi Roger,
> 
> On Mon, Jul 9, 2012 at 3:13 PM, Roger Quadros <rogerq@ti.com> wrote:
>> Hi,
>>
>> Just bumped across this patch and have a query.
>>
>> On 03/16/2012 04:05 PM, Tarun Kanti DebBarma wrote:
>>> 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 3a4f151..3b91ade 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -57,7 +57,6 @@ struct gpio_bank {
>>>       u16 irq;
>>>       int irq_base;
>>>       struct irq_domain *domain;
>>> -     u32 saved_wakeup;
>>>       u32 non_wakeup_gpios;
>>>       u32 enabled_non_wakeup_gpios;
>>>       struct gpio_regs context;
>>> @@ -777,7 +776,6 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
>>>       unsigned long           flags;
>>>
>>>       spin_lock_irqsave(&bank->lock, flags);
>>> -     bank->saved_wakeup = __raw_readl(mask_reg);
>>>       __raw_writel(0xffff & ~bank->context.wake_en, mask_reg);
>>
>> OK, here you are overwriting the mask_reg with the wakeup bitmask
>> without saving the mask_reg's original content.
> This is based upon understanding that set_gpio_trigger() is the common
> function where update of wake_en register takes place. Unless, mask_reg
> in this case refers to something else, effectively we would be saving the
> same value to saved_wakeup what is already present in wake_en.
> I will verify this specific to this function.
> 
>>
>>>       spin_unlock_irqrestore(&bank->lock, flags);
>>>
>>> @@ -793,7 +791,7 @@ static int omap_mpuio_resume_noirq(struct device *dev)
>>>       unsigned long           flags;
>>>
>>>       spin_lock_irqsave(&bank->lock, flags);
>>> -     __raw_writel(bank->saved_wakeup, mask_reg);
>>> +     __raw_writel(bank->context.wake_en, mask_reg);
>>
>> Now you are restoring nothing but the same content that you stored
>> during suspend. This will cause the non-wakeup gpio interrupts to get
>> masked between a suspend/resume. So isn't this a bug?
> That's right, the same value is restored back which was last updated in
> set_gpio_trigger() that got stored in wake_en register. Let me know if
> I am missing your points here.

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

> 
>>
>> Proper solution would be to save the mask_reg context into another
>> register than context.wake_en during suspend.
> As I said, this would make sense if mask_reg is referring to different
> register than what is used in set_gpio_trigger(). I will have a look.

OK thanks.

> 
> BTW, did you observe anything unusual during some testing?

No, I haven't done any tests.

cheers,
-roger

[PATCH v4 04/12] gpio/omap: remove saved_wakeup field from struct gpio_bank

[DebBarma, Tarun Kanti]

On Mon, Jul 9, 2012 at 5:21 PM, Roger Quadros <rogerq@ti.com> wrote:
> Tarun,
>
> On 07/09/2012 02:16 PM, DebBarma, Tarun Kanti wrote:
>> Hi Roger,
>>
>> On Mon, Jul 9, 2012 at 3:13 PM, Roger Quadros <rogerq@ti.com> wrote:
>>> Hi,
>>>
>>> Just bumped across this patch and have a query.
>>>
>>> On 03/16/2012 04:05 PM, Tarun Kanti DebBarma wrote:
>>>> 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 3a4f151..3b91ade 100644
>>>> --- a/drivers/gpio/gpio-omap.c
>>>> +++ b/drivers/gpio/gpio-omap.c
>>>> @@ -57,7 +57,6 @@ struct gpio_bank {
>>>>       u16 irq;
>>>>       int irq_base;
>>>>       struct irq_domain *domain;
>>>> -     u32 saved_wakeup;
>>>>       u32 non_wakeup_gpios;
>>>>       u32 enabled_non_wakeup_gpios;
>>>>       struct gpio_regs context;
>>>> @@ -777,7 +776,6 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
>>>>       unsigned long           flags;
>>>>
>>>>       spin_lock_irqsave(&bank->lock, flags);
>>>> -     bank->saved_wakeup = __raw_readl(mask_reg);
>>>>       __raw_writel(0xffff & ~bank->context.wake_en, mask_reg);
>>>
>>> OK, here you are overwriting the mask_reg with the wakeup bitmask
>>> without saving the mask_reg's original content.
>> This is based upon understanding that set_gpio_trigger() is the common
>> function where update of wake_en register takes place. Unless, mask_reg
>> in this case refers to something else, effectively we would be saving the
>> same value to saved_wakeup what is already present in wake_en.
>> I will verify this specific to this function.
>>
>>>
>>>>       spin_unlock_irqrestore(&bank->lock, flags);
>>>>
>>>> @@ -793,7 +791,7 @@ static int omap_mpuio_resume_noirq(struct device *dev)
>>>>       unsigned long           flags;
>>>>
>>>>       spin_lock_irqsave(&bank->lock, flags);
>>>> -     __raw_writel(bank->saved_wakeup, mask_reg);
>>>> +     __raw_writel(bank->context.wake_en, mask_reg);
>>>
>>> Now you are restoring nothing but the same content that you stored
>>> during suspend. This will cause the non-wakeup gpio interrupts to get
>>> masked between a suspend/resume. So isn't this a bug?
>> That's right, the same value is restored back which was last updated in
>> set_gpio_trigger() that got stored in wake_en register. Let me know if
>> I am missing your points here.
>
> If it is writing the same thing then isn't this write redundant?
Not, really. During suspend if the register has lost the context
we need to restore the value from wake_en.
--
Tarun
>
>>
>>>
>>> Proper solution would be to save the mask_reg context into another
>>> register than context.wake_en during suspend.
>> As I said, this would make sense if mask_reg is referring to different
>> register than what is used in set_gpio_trigger(). I will have a look.
>
> OK thanks.
>
>>
>> BTW, did you observe anything unusual during some testing?
>
> No, I haven't done any tests.
>
> cheers,
> -roger

[PATCH v4 04/12] gpio/omap: remove saved_wakeup field from struct gpio_bank

[Roger Quadros]

On 07/09/2012 03:30 PM, DebBarma, Tarun Kanti wrote:
> On Mon, Jul 9, 2012 at 5:21 PM, Roger Quadros <rogerq@ti.com> wrote:
>> Tarun,
>>
>> On 07/09/2012 02:16 PM, DebBarma, Tarun Kanti wrote:
>>> Hi Roger,
>>>
>>> On Mon, Jul 9, 2012 at 3:13 PM, Roger Quadros <rogerq@ti.com> wrote:
>>>> Hi,
>>>>
>>>> Just bumped across this patch and have a query.
>>>>
>>>> On 03/16/2012 04:05 PM, Tarun Kanti DebBarma wrote:
>>>>> 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 3a4f151..3b91ade 100644
>>>>> --- a/drivers/gpio/gpio-omap.c
>>>>> +++ b/drivers/gpio/gpio-omap.c
>>>>> @@ -57,7 +57,6 @@ struct gpio_bank {
>>>>>       u16 irq;
>>>>>       int irq_base;
>>>>>       struct irq_domain *domain;
>>>>> -     u32 saved_wakeup;
>>>>>       u32 non_wakeup_gpios;
>>>>>       u32 enabled_non_wakeup_gpios;
>>>>>       struct gpio_regs context;
>>>>> @@ -777,7 +776,6 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
>>>>>       unsigned long           flags;
>>>>>
>>>>>       spin_lock_irqsave(&bank->lock, flags);
>>>>> -     bank->saved_wakeup = __raw_readl(mask_reg);
>>>>>       __raw_writel(0xffff & ~bank->context.wake_en, mask_reg);
>>>>
>>>> OK, here you are overwriting the mask_reg with the wakeup bitmask
>>>> without saving the mask_reg's original content.
>>> This is based upon understanding that set_gpio_trigger() is the common
>>> function where update of wake_en register takes place. Unless, mask_reg
>>> in this case refers to something else, effectively we would be saving the
>>> same value to saved_wakeup what is already present in wake_en.
>>> I will verify this specific to this function.
>>>
>>>>
>>>>>       spin_unlock_irqrestore(&bank->lock, flags);
>>>>>
>>>>> @@ -793,7 +791,7 @@ static int omap_mpuio_resume_noirq(struct device *dev)
>>>>>       unsigned long           flags;
>>>>>
>>>>>       spin_lock_irqsave(&bank->lock, flags);
>>>>> -     __raw_writel(bank->saved_wakeup, mask_reg);
>>>>> +     __raw_writel(bank->context.wake_en, mask_reg);
>>>>
>>>> Now you are restoring nothing but the same content that you stored
>>>> during suspend. This will cause the non-wakeup gpio interrupts to get
>>>> masked between a suspend/resume. So isn't this a bug?
>>> That's right, the same value is restored back which was last updated in
>>> set_gpio_trigger() that got stored in wake_en register. Let me know if
>>> I am missing your points here.
>>
>> If it is writing the same thing then isn't this write redundant?
> Not, really. During suspend if the register has lost the context
> we need to restore the value from wake_en.

Shouldn't that be taken care of by omap_gpio_restore_context() that too
only if the context was lost?

> --
> Tarun
>>
>>>
>>>>
>>>> Proper solution would be to save the mask_reg context into another
>>>> register than context.wake_en during suspend.
>>> As I said, this would make sense if mask_reg is referring to different
>>> register than what is used in set_gpio_trigger(). I will have a look.
>>
>> OK thanks.
>>
>>>
>>> BTW, did you observe anything unusual during some testing?
>>

Did you get a chance to see if the two registers are same or different?

regards,
-roger

[PATCH v4 04/12] gpio/omap: remove saved_wakeup field from struct gpio_bank

[DebBarma, Tarun Kanti]

Hi Roger,

On Mon, Jul 9, 2012 at 4:46 PM, DebBarma, Tarun Kanti
<tarun.kanti@ti.com> wrote:
> Hi Roger,
>
> On Mon, Jul 9, 2012 at 3:13 PM, Roger Quadros <rogerq@ti.com> wrote:
>> Hi,
>>
>> Just bumped across this patch and have a query.
>>
>> On 03/16/2012 04:05 PM, Tarun Kanti DebBarma wrote:
>>> 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 3a4f151..3b91ade 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -57,7 +57,6 @@ struct gpio_bank {
>>>       u16 irq;
>>>       int irq_base;
>>>       struct irq_domain *domain;
>>> -     u32 saved_wakeup;
>>>       u32 non_wakeup_gpios;
>>>       u32 enabled_non_wakeup_gpios;
>>>       struct gpio_regs context;
>>> @@ -777,7 +776,6 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
>>>       unsigned long           flags;
>>>
>>>       spin_lock_irqsave(&bank->lock, flags);
>>> -     bank->saved_wakeup = __raw_readl(mask_reg);
>>>       __raw_writel(0xffff & ~bank->context.wake_en, mask_reg);
>>
>> OK, here you are overwriting the mask_reg with the wakeup bitmask
>> without saving the mask_reg's original content.
> This is based upon understanding that set_gpio_trigger() is the common
> function where update of wake_en register takes place. Unless, mask_reg
> in this case refers to something else, effectively we would be saving the
> same value to saved_wakeup what is already present in wake_en.
> I will verify this specific to this function.
I had a look at this change and it appears for OMAP1 we have to explicitly
save the register here. This is because set_gpio_trigger() refers to wake_en
register for OMAP2+ platforms, whereas here on OMAP1 it is referring to
irqenable register. In other words we need to have:
bank->context.wake_en = __raw_readl(mask_reg);
--
Tarun

>
>>
>>>       spin_unlock_irqrestore(&bank->lock, flags);
>>>
>>> @@ -793,7 +791,7 @@ static int omap_mpuio_resume_noirq(struct device *dev)
>>>       unsigned long           flags;
>>>
>>>       spin_lock_irqsave(&bank->lock, flags);
>>> -     __raw_writel(bank->saved_wakeup, mask_reg);
>>> +     __raw_writel(bank->context.wake_en, mask_reg);
>>
>> Now you are restoring nothing but the same content that you stored
>> during suspend. This will cause the non-wakeup gpio interrupts to get
>> masked between a suspend/resume. So isn't this a bug?
> That's right, the same value is restored back which was last updated in
> set_gpio_trigger() that got stored in wake_en register. Let me know if
> I am missing your points here.
>
>>
>> Proper solution would be to save the mask_reg context into another
>> register than context.wake_en during suspend.
> As I said, this would make sense if mask_reg is referring to different
> register than what is used in set_gpio_trigger(). I will have a look.
>
> BTW, did you observe anything unusual during some testing?
>
> Thanks.
> --
> Tarun
>>
>>>       spin_unlock_irqrestore(&bank->lock, flags);
>>>
>>>       return 0;
>>
>>
>> cheers,
>> -roger

[PATCH v4 05/12] gpio/omap: get rid of retrigger variable in gpio_irq_handler

[Tarun Kanti DebBarma]

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.

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 3b91ade..47143b7 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -627,7 +627,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);
 
@@ -664,8 +663,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 v4 06/12] 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 47143b7..4d23da1 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -241,7 +241,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;
@@ -323,7 +323,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 v4 07/12] 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 4d23da1..8b453dd 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -480,7 +480,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 v4 08/12] 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>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@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 8b453dd..cce2c73 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -140,18 +140,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)
@@ -852,19 +852,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 v4 09/12] 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 also 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>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@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 cce2c73..dba69b8 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1142,50 +1142,6 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
 
 #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);
 
@@ -1417,14 +1373,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 v4 10/12] 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.

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

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index dba69b8..072a841 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -116,10 +116,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;
+	}
 
 	__raw_writel(l, reg);
 }
-- 
1.7.0.4

[PATCH v4 11/12] gpio/omap: fix incorrect context restore logic in omap_gpio_runtime_*

[Tarun Kanti DebBarma]

In omap_gpio_runtime_suspend/resume() the context save/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>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 drivers/gpio/gpio-omap.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 072a841..c5aff7c 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1187,9 +1187,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;
-
 	bank->saved_datain = __raw_readl(bank->base +
 						bank->regs->datain);
 	l1 = bank->context.fallingdetect;
@@ -1236,7 +1233,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 v4 12/12] 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>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@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 c5aff7c..ae62c62 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -447,6 +447,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);
@@ -454,10 +455,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)
@@ -468,6 +469,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);
@@ -475,10 +477,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)
-- 
1.7.0.4

[PATCH v4 00/12] 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. Also, the suspend/resume callbacks are
> removed bacause they are not needed any more.

OK.

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

These fixes need to be separated out from the cleanups and prepared for
v3.4-rc.  We need these as standlone fixes (indepenent of the cleanups)
so they can be applied for 3.4-rc.  The rest of the cleanups will have
to wait until 3.5.

> It is baselined on top of:
> git://git.secretlab.ca/git/linux-2.6.git gpio/next
> Commit: 81b279d80a63628e580c71a31d30a8c3b3047ad4
>
> Series is available here for reference:
> git://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev for_3.4/gpio_more_cleanup_fixes
>
> Power Test: 
> Off-mode and Retention on OMAP3430 (Suspend and Idle paths).
> Also confirmed that dataout register content preserved over
> off-mode.

I've confirmed these fixes also correct off-mode GPIO ouptut glitches
that I was seeing on n900.

Kevin

[PATCH v4 00/12] gpio/omap: Some more driver cleanup and fixes

[DebBarma, Tarun Kanti]

On Tue, Mar 20, 2012 at 6:07 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. Also, the suspend/resume callbacks are
>> removed bacause they are not needed any more.
>
> OK.
>
>> 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.
>
> These fixes need to be separated out from the cleanups and prepared for
> v3.4-rc. ?We need these as standlone fixes (indepenent of the cleanups)
> so they can be applied for 3.4-rc. ?The rest of the cleanups will have
> to wait until 3.5.
Sure, I will separate out the fixes and prepare new series.

>
>> It is baselined on top of:
>> git://git.secretlab.ca/git/linux-2.6.git gpio/next
>> Commit: 81b279d80a63628e580c71a31d30a8c3b3047ad4
>>
>> Series is available here for reference:
>> git://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev for_3.4/gpio_more_cleanup_fixes
>>
>> Power Test:
>> Off-mode and Retention on OMAP3430 (Suspend and Idle paths).
>> Also confirmed that dataout register content preserved over
>> off-mode.
>
> I've confirmed these fixes also correct off-mode GPIO ouptut glitches
> that I was seeing on n900.
OK, thanks.
--
Tarun
>
> Kevin