[PATCH v4 RESEND 6/7] gpio/omap: fix incorrect update to context.irqenable1

[PATCH v4 RESEND 6/7] gpio/omap: fix incorrect update to context.irqenable1

[Tarun Kanti DebBarma]

In _enable_gpio_irqbank() when bank->regs->set_irqenable is TRUE,
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 avoid overwriting it with
gpio_mask at the end of the function. Instead, update irqenable1
appropriately by OR'ing with gpio_mask.
For the case where bank->regs->set_irqenable is FALSE, irqenable1
can be directly overwritten with 'l' which holds correct computed
value.

        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 similar change for _disable_gpio_irqbank().

Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
Updated change log as per Kevin's suggestion.

 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 bcb1061..6c17e58 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -451,6 +451,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);
@@ -458,10 +459,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)
@@ -472,6 +473,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);
@@ -479,10 +481,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

Re: [PATCH v4 RESEND 6/7] gpio/omap: fix incorrect update to context.irqenable1

[Kevin Hilman]

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

> In _enable_gpio_irqbank() when bank->regs->set_irqenable is TRUE,
> 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 avoid overwriting it with
> gpio_mask at the end of the function. Instead, update irqenable1
> appropriately by OR'ing with gpio_mask.
> For the case where bank->regs->set_irqenable is FALSE, irqenable1
> can be directly overwritten with 'l' which holds correct computed
> value.
>
>         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 similar change for _disable_gpio_irqbank().
>
> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
> Updated change log as per Kevin's suggestion.


Thanks for the update.  I've queued the updated version of this series,
and sent pull request to Grant.

Thanks Tarun,

Kevin

[PATCH v4 RESEND 0/7] gpio/omap: Some more driver fixes

[Tarun Kanti DebBarma]

This series excludes the cleanup patches as suggested by Kevin from
the previously posted series.

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

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 (7):
  gpio/omap: fix wakeup_en register update in _set_gpio_wakeup()
  gpio/omap: fix trigger type to unsigned
  gpio/omap: fix _set_gpio_irqenable implementation
  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
  gpio/omap: fix redundant decoding of gpio offset

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

[PATCH v4 RESEND 6/7] 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 bcb1061..6c17e58 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -451,6 +451,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);
@@ -458,10 +459,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)
@@ -472,6 +473,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);
@@ -479,10 +481,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

Re: [PATCH v4 RESEND 6/7] gpio/omap: fix incorrect update to context.irqenable1

[Kevin Hilman]

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

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

...except that the code doesn't do this anymore.

I like the newer version (I hope so, since I suggested it :), but please
update the changlog to describe what the code is actually doing.

Thanks,

Kevin

Re: [PATCH v4 RESEND 6/7] gpio/omap: fix incorrect update to context.irqenable1

[DebBarma, Tarun Kanti]

On Tue, Mar 20, 2012 at 11:31 PM, Kevin Hilman <khilman@ti.com> wrote:
> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>
>> In _enable_gpio_irqbank() when bank->regs->set_irqenable is valid,
>> gpio_mask can be directly set by writing to set_irqenable register
>> without overwriting current value. 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.
>
> ...except that the code doesn't do this anymore.
Yes.
>
> I like the newer version (I hope so, since I suggested it :), but please
> update the changlog to describe what the code is actually doing.
Sure.
--
Tarun
>
> Thanks,
>
> Kevin

Re: [PATCH v4 RESEND 6/7] gpio/omap: fix incorrect update to context.irqenable1

[DebBarma, Tarun Kanti]

On Wed, Mar 21, 2012 at 8:10 AM, DebBarma, Tarun Kanti
<tarun.kanti@ti.com> wrote:
> On Tue, Mar 20, 2012 at 11:31 PM, Kevin Hilman <khilman@ti.com> wrote:
>> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>>
>>> In _enable_gpio_irqbank() when bank->regs->set_irqenable is valid,
>>> gpio_mask can be directly set by writing to set_irqenable register
>>> without overwriting current value. 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.
>>
>> ...except that the code doesn't do this anymore.
> Yes.
>>
>> I like the newer version (I hope so, since I suggested it :), but please
>> update the changlog to describe what the code is actually doing.
> Sure.
I have updated the change log here:
git://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev
for_3.4/gpio_more_fixes
--
Tarun
>>
>> Thanks,
>>
>> Kevin

Re: [PATCH v4 RESEND 6/7] gpio/omap: fix incorrect update to context.irqenable1

[Kevin Hilman]

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

> On Wed, Mar 21, 2012 at 8:10 AM, DebBarma, Tarun Kanti
> <tarun.kanti@ti.com> wrote:
>> On Tue, Mar 20, 2012 at 11:31 PM, Kevin Hilman <khilman@ti.com> wrote:
>>> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>>>
>>>> In _enable_gpio_irqbank() when bank->regs->set_irqenable is valid,
>>>> gpio_mask can be directly set by writing to set_irqenable register
>>>> without overwriting current value. 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.
>>>
>>> ...except that the code doesn't do this anymore.
>> Yes.
>>>
>>> I like the newer version (I hope so, since I suggested it :), but please
>>> update the changlog to describe what the code is actually doing.
>> Sure.
> I have updated the change log here:
> git://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev
> for_3.4/gpio_more_fixes

Please also post and updated version of the patch for review.

Kevin

Re: [PATCH v4 RESEND 6/7] gpio/omap: fix incorrect update to context.irqenable1

[DebBarma, Tarun Kanti]

On Wed, Mar 21, 2012 at 7:36 PM, Kevin Hilman <khilman@ti.com> wrote:
> "DebBarma, Tarun Kanti" <tarun.kanti@ti.com> writes:
>
>> On Wed, Mar 21, 2012 at 8:10 AM, DebBarma, Tarun Kanti
>> <tarun.kanti@ti.com> wrote:
>>> On Tue, Mar 20, 2012 at 11:31 PM, Kevin Hilman <khilman@ti.com> wrote:
>>>> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>>>>
>>>>> In _enable_gpio_irqbank() when bank->regs->set_irqenable is valid,
>>>>> gpio_mask can be directly set by writing to set_irqenable register
>>>>> without overwriting current value. 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.
>>>>
>>>> ...except that the code doesn't do this anymore.
>>> Yes.
>>>>
>>>> I like the newer version (I hope so, since I suggested it :), but please
>>>> update the changlog to describe what the code is actually doing.
>>> Sure.
>> I have updated the change log here:
>> git://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev
>> for_3.4/gpio_more_fixes
>
> Please also post and updated version of the patch for review.
Sure.
--
Tarun
>
> Kevin