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

[Axel Lin]

2013/6/21 Michael Trimarchi <mich...@amarulasolutions.com>:
> On 06/21/2013 06:40 AM, Vipin Kumar wrote:
>> On 6/20/2013 7:26 PM, Axel Lin wrote:
>>> 2013/6/20 Marek Vasut<ma...@denx.de>
>>>>
>>>> Dear Axel Lin,
>>>>
>>>>> 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....@ingics.com>
>>>>> ---
>>>>>   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 d3c728e..8878608 100644
>>>>> --- a/drivers/gpio/spear_gpio.c
>>>>> +++ b/drivers/gpio/spear_gpio.c
>>>>> @@ -52,7 +52,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)]);
>>>>
>>>> How can this possibly work? Writing 0 to the whole bank will unset all the
>>>> GPIOs, no ?
>>>
>>>
>>> Because each GPIO is controlled by a register.
>>> And only one bit will be set when set gpio to high.
>>>
>>> So it's safe to write 0 for clearing the bit.
>>>
>>> Note, the gpio_get_value() implementation also assumes there is only one bit
>>> will be set. ( If this is not true, both gpio_get_value() and 
>>> gpio_set_value()
>>> need fix.)
>>>
>>> Vipin, can you review this patch and confirm this behavior?
>>>
>>
>> Yes this is right. and the code is fine
>>
>
> The problem is not in set one bit but in reset one bit. Can you check
> the else path?

Hi,
I'm not the best person to answer this question because I don't have the
hardware and datasheet.
In the case only one bit is meaningful and the reset bits are 0,
it looks ok for me to write 0 for clearing the bit.
( note, each gpio pin is controlled by different register.)

This patch is acked and reviewed by Stefan Roese and Vipin Kumar.
I'm wondering if this patch is acceptable?
Or maybe a test-by can help to make this patch acceptable?

Regards,
Axel
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot