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

[Albert ARIBAUD]

Hi Michael,

On Sat, 14 Sep 2013 11:38:02 +0200, Michael Trimarchi
<michael at amarulasolutions.com> wrote:

> 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

Ok-- I got the "masking address" trick this time, thanks. It is indeed
unusual, and the code is indeed correct. However, I would like the
patch to include a few lines of comment to explain how and why it
works, for the benefit of its future readers who might find it weird
too.

> Michael

Amicalement,
-- 
Albert.