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; -- 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html