From mboxrd@z Thu Jan 1 00:00:00 1970 From: khilman@ti.com (Kevin Hilman) Date: Tue, 13 Mar 2012 09:27:21 -0700 Subject: [PATCH v3 11/13] gpio/omap: fix dataout register overwrite in _set_gpio_dataout_* In-Reply-To: (Tarun Kanti DebBarma's message of "Tue, 13 Mar 2012 12:22:28 +0530") 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> Message-ID: <8762e8v53q.fsf@ti.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.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 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; + }