"DebBarma, Tarun Kanti" <tarun.kanti@xxxxxx> writes:
> On Tue, Mar 13, 2012 at 12:03 PM, DebBarma, Tarun Kanti
> <tarun.kanti@xxxxxx> wrote:
>> On Tue, Mar 13, 2012 at 11:33 AM, DebBarma, Tarun Kanti
>> <tarun.kanti@xxxxxx> wrote:
>>> On Tue, Mar 13, 2012 at 3:24 AM, Kevin Hilman <khilman@xxxxxx> wrote:
>>>> Tarun Kanti DebBarma <tarun.kanti@xxxxxx> 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@xxxxxx>
>>>>> ---
>>>>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
