On Tue, Mar 13, 2012 at 9:57 PM, Kevin Hilman <khilman@xxxxxx> wrote:
> "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.
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;
> + }
--
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
