[U-Boot] [PATCH RESEND] gpio: spear_gpio: Fix gpio_set_value() implementation

[Michael Trimarchi]

Hi Axel

On Sat, Sep 14, 2013 at 11:34 AM, Axel Lin <axel.lin at ingics.com> wrote:
> 2013/9/14 Albert ARIBAUD <albert.u.boot at aribaud.net>:
>> Hi Axel,
>>
>> On Fri, 06 Sep 2013 14:22:40 +0800, Axel Lin <axel.lin at ingics.com>
>> wrote:
>>
>>> In current gpio_set_value() implementation, it always sets the gpio control bit
>>> no matter the value argument is 0 or 1. Thus the GPIOs never set to low.
>>> This patch fixes this bug.
>>>
>>> Signed-off-by: Axel Lin <axel.lin at ingics.com>
>>> Acked-by: Stefan Roese <sr at denx.de>
>>> Reviewed-by: Vipin Kumar <vipin.kumar at st.com>
>>> ---
>>> This patch was sent on http://lists.denx.de/pipermail/u-boot/2013-June/156861.html
>>>
>>> Has Stefan's Ack:
>>> http://lists.denx.de/pipermail/u-boot/2013-June/156864.html
>>>
>>> Vipin says the code is fine, so I add Vipin's review-by.
>>> http://lists.denx.de/pipermail/u-boot/2013-June/156966.html
>>>
>>> Michael confirms it works:
>>> http://lists.denx.de/pipermail/u-boot/2013-August/160652.html
>>>
>>> No body picks up this patch, so here is a resend.
>>> Although I think this is a bug fix, but I'll let maintainers to determinate
>>> if this is the material for v2013.10.
>>> Anyway, can someone at least let me know if this patch is ok for apply at some
>>> point? I have no idea who is maintaining this file.
>>>
>>> Regards,
>>> Axel
>>>
>>>  drivers/gpio/spear_gpio.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpio/spear_gpio.c b/drivers/gpio/spear_gpio.c
>>> index 367b670..6fb4117 100644
>>> --- a/drivers/gpio/spear_gpio.c
>>> +++ b/drivers/gpio/spear_gpio.c
>>> @@ -36,7 +36,10 @@ int gpio_set_value(unsigned gpio, int value)
>>>  {
>>>       struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE;
>>>
>>> -     writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>>> +     if (value)
>>> +             writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>>> +     else
>>> +             writel(0, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>>>
>>>       return 0;
>>>  }
>>
>> Despite discussions in the previous thread and the confirmations that
>> this code is functionally equivalent to the Linux code, I still believe
>> this code is incorrect for both writing and reading.
>>
>> From the doc, writing to GPIODATA will obviously copy each of bits 7
>> to 0 of the written value into the actuals GPIO mapped to bits 7 to
>> 0 of this register (assuming they are configured as outputs, of course).
>> Based on this, the code above:
>>
>> - when setting a single GPIO, sets *but clears up to seven other GPIOs*;
>> - when clearing a single GPIO, clears it *and up to seven other GPIOs*.
>>
>> This code may have been tested only for a single active GPIO at a time,
>> for which this code would behave correctly; but as soon as two GPIOs
>> from the same bank must be set at the same time, it fails.
>>
>> Please fix this code so that setting or clearing a GPIO does not set or
>> clear any other GPIO, and perform an actual test to confirm this works
>> before submitting V2.
>
> No.
> Some people (Marek, and *Michael*) asked this question in original
> discussion thread.
> The datasheet says each GPIO is controlled by *different* register.
> (Well, it's unusual.)
> And that is why we don't need a read-write-update operation.
> Simply write 0 to the register does work. ( *Michael* replied it works )
>
>>
>> BTW: if (as the previous thread seemed to imply) no one around has the
>> hardware to test this change, then why exactly is it needed?
>>

Yes it's a strange implementation but looking at the documentation seems correct

Michael

>> Amicalement,
>> --
>> Albert.