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

login
register
mail settings
Submitter Kevin Hilman
Date March 13, 2012, 4:27 p.m.
Message ID <8762e8v53q.fsf@ti.com>
Download mbox | patch
Permalink /patch/146444/
State New
Headers show

Comments

Kevin Hilman - March 13, 2012, 4:27 p.m.
"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.

Kevin


linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
DebBarma, Tarun Kanti - March 14, 2012, 1:53 a.m.
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;
> +       }

Patch

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