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

[Axel Lin]

2013/6/30 Michael Trimarchi <michael at amarulasolutions.com>:
> Hi
> Il giorno 30/giu/2013 06:18, "Axel Lin" <axel.lin at ingics.com> ha scritto:
>
>
>>
>> 2013/6/21 Michael Trimarchi <michael at 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<marex at 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.lin at 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?
>>
>
> If each pin is controlled by a different register why you need to 1<<gpio in
> set path?

Because the meaningful bit for different register is different.

>
> And how it works for gpio 33?

SPEAR_GPIO_COUNT is 8, so this driver only allows setting gpio0 ~ gpio7.

Vipin, any chance to double check the datasheet and confirm if this patch is ok?

Regards,
Axel