Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965999Ab2B1SqD (ORCPT ); Tue, 28 Feb 2012 13:46:03 -0500 Received: from na3sys009aog118.obsmtp.com ([74.125.149.244]:39489 "EHLO na3sys009aog118.obsmtp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757131Ab2B1SqA convert rfc822-to-8bit (ORCPT ); Tue, 28 Feb 2012 13:46:00 -0500 Authentication-Results: mr.google.com; spf=pass (google.com: domain of khilman@ti.com designates 10.68.225.231 as permitted sender) smtp.mail=khilman@ti.com From: Kevin Hilman To: "DebBarma\, Tarun Kanti" Cc: linux-omap@vger.kernel.org, grant.likely@secretlab.ca, tony@atomide.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 3/6] gpio/omap: remove suspend_wakeup field from struct gpio_bank Organization: Texas Instruments, Inc. References: <1329999031-6914-1-git-send-email-tarun.kanti@ti.com> <1329999031-6914-4-git-send-email-tarun.kanti@ti.com> <87ty2bu91e.fsf@ti.com> Date: Tue, 28 Feb 2012 10:45:57 -0800 In-Reply-To: (Tarun Kanti DebBarma's message of "Tue, 28 Feb 2012 15:09:05 +0530") Message-ID: <87r4xeyexm.fsf@ti.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4894 Lines: 130 "DebBarma, Tarun Kanti" writes: > On Tue, Feb 28, 2012 at 5:24 AM, Kevin Hilman wrote: >> Tarun Kanti DebBarma 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 >> >> 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 -- 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/