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

[DebBarma, Tarun Kanti]

On Tue, Mar 13, 2012 at 12:03 PM, DebBarma, Tarun Kanti
<tarun.kanti at ti.com> wrote:
> On Tue, Mar 13, 2012 at 11:33 AM, DebBarma, Tarun Kanti
> <tarun.kanti at ti.com> wrote:
>> On Tue, Mar 13, 2012 at 3:24 AM, Kevin Hilman <khilman at ti.com> wrote:
>>> Tarun Kanti DebBarma <tarun.kanti at 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 at 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;
--
Tarun
>>
>>>
>>>>       __raw_writel(l, reg);
>>>>       bank->context.dataout = l;
>>>>  }
>>>> @@ -130,6 +131,8 @@ static void _set_gpio_dataout_mask(struct gpio_bank *bank, int gpio, int enable)
>>>>               l |= gpio_bit;
>>>>       else
>>>>               l &= ~gpio_bit;
>>>> +
>>>> +     l |= __raw_readl(bank->base + bank->regs->set_dataout);
>>>
>>> There's already a __raw_read() in this function just above.
>> Right. Thanks.
>> --
>> Tarun
>>>
>>>>       __raw_writel(l, reg);
>>>>       bank->context.dataout = l;
>>>>  }