On Tue, Mar 13, 2012 at 12:03 PM, DebBarma, Tarun Kanti
<[email protected]> wrote:
> On Tue, Mar 13, 2012 at 11:33 AM, DebBarma, Tarun Kanti
> <[email protected]> wrote:
>> On Tue, Mar 13, 2012 at 3:24 AM, Kevin Hilman <[email protected]> wrote:
>>> Tarun Kanti DebBarma <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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;
>>>> }
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
