From patchwork Tue Mar 13 16:27:21 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Kevin Hilman X-Patchwork-Id: 146444 Return-Path: X-Original-To: incoming-imx@patchwork.ozlabs.org Delivered-To: patchwork-incoming-imx@bilbo.ozlabs.org Received: from merlin.infradead.org (merlin.infradead.org [IPv6:2001:4978:20e::2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 860A7B6EEA for ; Wed, 14 Mar 2012 03:29:51 +1100 (EST) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1S7UZF-0006Td-Up; Tue, 13 Mar 2012 16:27:21 +0000 Received: from na3sys009aog131.obsmtp.com ([74.125.149.247] helo=psmtp.com) by merlin.infradead.org with smtps (Exim 4.76 #1 (Red Hat Linux)) id 1S7UZD-0006TD-H7 for linux-arm-kernel@lists.infradead.org; Tue, 13 Mar 2012 16:27:20 +0000 Received: from mail-iy0-f177.google.com ([209.85.210.177]) (using TLSv1) by na3sys009aob131.postini.com ([74.125.148.12]) with SMTP ID DSNKT191YBrsfVl6ba8Un4ybw9x7UxGWmZve@postini.com; Tue, 13 Mar 2012 09:27:19 PDT Received: by mail-iy0-f177.google.com with SMTP id y10so1258110iak.8 for ; Tue, 13 Mar 2012 09:27:12 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=from:to:cc:subject:organization:references:date:in-reply-to :message-id:user-agent:mime-version:content-type :content-transfer-encoding:x-gm-message-state; bh=wjKF0oZAbV+KlB4Sbi54YxH2CGrUAQ7Km9S6wV/71AI=; b=Z5GgEa5oBEJV+B2EX1yH3Wx8VsxOW8aLOz73bDvuGPE8b0Oy47xg/NPl4J2UCKmTJ7 EfvaRLE1zoMN99LgLO6ebm8RagM69fPlG0N7q+DPlvGjmdY2eX5VZm+pLU7Wk0fcMxvf dLLH95dPubiSA85frKuzoQKXVhUPlgfGe9/LylqzGqPrD2pyTG4qxufRa8m2iWdjjvI8 oRtUv8Oq/0HlXgRPvN8winZxLYR5vI+PB9EoX4lBbdW7Ag1oLFAlw1AGei76TQu+1zZN OFe+UH2wo9vGYDE3MeElAnri+L6FDzh5CAS3Eu07UVv0f9WmUiVz+uiG4f4QR0rnsKtK pmww== Received: by 10.50.11.227 with SMTP id t3mr5715593igb.45.1331656032333; Tue, 13 Mar 2012 09:27:12 -0700 (PDT) Received: from localhost (c-24-19-7-36.hsd1.wa.comcast.net. [24.19.7.36]) by mx.google.com with ESMTPS id vr4sm4224567igb.1.2012.03.13.09.27.11 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 13 Mar 2012 09:27:11 -0700 (PDT) From: Kevin Hilman To: "DebBarma\, Tarun Kanti" Subject: Re: [PATCH v3 11/13] gpio/omap: fix dataout register overwrite in _set_gpio_dataout_* Organization: Texas Instruments, Inc. References: <1331118963-26364-1-git-send-email-tarun.kanti@ti.com> <1331118963-26364-12-git-send-email-tarun.kanti@ti.com> <87y5r5wkmu.fsf@ti.com> Date: Tue, 13 Mar 2012 09:27:21 -0700 In-Reply-To: (Tarun Kanti DebBarma's message of "Tue, 13 Mar 2012 12:22:28 +0530") Message-ID: <8762e8v53q.fsf@ti.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 X-Gm-Message-State: ALoCoQkdK3tIo6yY2sCeq2zUlJLzFxKkbjRj4mJNG+nexx8einAOKa2/T8ECKGCjs9gAuLE/GPrk X-Spam-Note: CRM114 invocation failed X-Spam-Score: -1.9 (-) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-1.9 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: tony@atomide.com, linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org List-Id: linux-imx-kernel.lists.patchwork.ozlabs.org "DebBarma, Tarun Kanti" writes: > On Tue, Mar 13, 2012 at 12:03 PM, DebBarma, Tarun Kanti > wrote: >> On Tue, Mar 13, 2012 at 11:33 AM, DebBarma, Tarun Kanti >> wrote: >>> On Tue, Mar 13, 2012 at 3:24 AM, Kevin Hilman wrote: >>>> Tarun Kanti DebBarma writes: >>>> >>>>> In the existing _set_gpio_dataout_*() implementation, the dataout >>>>> register is overwritten every time the function is called. This is >>>>> not intended behavior because that would end up one user of a GPIO >>>>> line overwriting what is written by another. Fix this so that previous >>>>> value is always preserved until explicitly changed by respective >>>>> user/driver of the GPIO line. >>>>> >>>>> Signed-off-by: Tarun Kanti DebBarma >>>>> --- >>>>>  drivers/gpio/gpio-omap.c |    3 +++ >>>>>  1 files changed, 3 insertions(+), 0 deletions(-) >>>>> >>>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c >>>>> index 04c2677..2e8e476 100644 >>>>> --- a/drivers/gpio/gpio-omap.c >>>>> +++ b/drivers/gpio/gpio-omap.c >>>>> @@ -114,6 +114,7 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable) >>>>>       else >>>>>               reg += bank->regs->clr_dataout; >>>>> >>>>> +     l |= __raw_readl(bank->base + bank->regs->set_dataout); >>>> >>>> minor: IMO, it's more reader-friendly if this looks like >>>> >>>>       l = __raw_read(...) >>>>       l |= GPIO_BIT(...) >>>>       __raw_write(...) >>> Agreed. I will make the change. >> Also, the read should be: __raw_readl(bank->base + bank->regs->dataout); >> instead of bank->regs->set_dataout. > I see a problem with this implementation. It is not correct to write l > |= GPIO_BIT(...). > For example if we write to clr_dataout register we would end up > clearing bits which > we are not supposed to. We should just be operating on current GPIO_BIT(...). > The l |= GPIO_BIT(...) is needed just to make sure that we have the > right context > stored. So the overall sequence should be something like this: > > void __iomem *reg = bank->base; > u32 l = GPIO_BIT(bank, gpio); > > if (enable) > reg += bank->regs->set_dataout; > else > reg += bank->regs->clr_dataout; > > __raw_writel(l, reg); > l |= __raw_readl(bank->base + bank->regs->dataout); > bank->context.dataout = l; Again, you don't need the extra read-back here. Just set/clear the bit in the context. Untested patch below. Kevin linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index 0b05629..db905c0 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -111,10 +111,13 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable) void __iomem *reg = bank->base; u32 l = GPIO_BIT(bank, gpio); - if (enable) + if (enable) { reg += bank->regs->set_dataout; - else + bank->context.dataout |= l; + } else { reg += bank->regs->clr_dataout; + bank->context.dataout &= ~l; + } _______________________________________________ linux-arm-kernel mailing list