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

[Michael Trimarchi]

Hi
Il giorno 30/giu/2013 06:18, "Axel Lin" <axel....@ingics.com> ha scritto:
>
> 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?
>

If each pin is controlled by a different register why you need to 1<<gpio
in set path?

And how it works for gpio 33?

Michael

> Regards,
> Axel

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