Re: [PATCH v3 11/13] gpio/omap: fix dataout register overwrite in _set_gpio_dataout_*

[Kevin Hilman <khilman@ti.com>]

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

> On Tue, Mar 13, 2012 at 12:03 PM, DebBarma, Tarun Kanti
> <tarun.kanti@ti.com> wrote:
>> On Tue, Mar 13, 2012 at 11:33 AM, DebBarma, Tarun Kanti
>> <tarun.kanti@ti.com> wrote:
>>> On Tue, Mar 13, 2012 at 3:24 AM, Kevin Hilman <khilman@ti.com> wrote:
>>>> Tarun Kanti DebBarma <tarun.kanti@ti.com> 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 <tarun.kanti@ti.com>
>>>>> ---
>>>>>  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;
+	}
--
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