Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757089Ab2B1LPY (ORCPT ); Tue, 28 Feb 2012 06:15:24 -0500 Received: from na3sys009aog101.obsmtp.com ([74.125.149.67]:37769 "EHLO na3sys009aog101.obsmtp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752415Ab2B1LPX convert rfc822-to-8bit (ORCPT ); Tue, 28 Feb 2012 06:15:23 -0500 Authentication-Results: mr.google.com; spf=pass (google.com: domain of tarun.kanti@ti.com designates 10.229.136.65 as permitted sender) smtp.mail=tarun.kanti@ti.com MIME-Version: 1.0 In-Reply-To: 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 16:45:21 +0530 Message-ID: Subject: Re: [PATCH 3/6] gpio/omap: remove suspend_wakeup field from struct gpio_bank From: "DebBarma, Tarun Kanti" To: Kevin Hilman Cc: linux-omap@vger.kernel.org, grant.likely@secretlab.ca, tony@atomide.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4084 Lines: 110 On Tue, Feb 28, 2012 at 3:09 PM, DebBarma, Tarun Kanti wrote: > 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. > 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() Sorry, it should have been: chip.irq_set_wake() --> gpio_wake_enable() --> _set_gpio_wakeup() > >> >> 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-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/