[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 at 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 at 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 at ti.com>
>>>> Reviewed-by: Santosh Shilimkar <santosh.shilimkar at ti.com>
>>>> Acked-by: Felipe Balbi <balbi at 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