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

[Kevin Hilman]

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

> On Tue, Feb 28, 2012 at 5:24 AM, Kevin Hilman <khilman@xxxxxx> wrote:
>> Tarun Kanti DebBarma <tarun.kanti@xxxxxx> 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@xxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/