| Message ID | 8762e8v53q.fsf@ti.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Mar 13, 2012 at 9:57 PM, Kevin Hilman <khilman@ti.com> wrote: > "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. Alright. -- Tarun > > 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; > + }
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; + } _______________________________________________ linux-arm-kernel mailing list