Hi Michael, On Sat, 14 Sep 2013 11:38:02 +0200, Michael Trimarchi <mich...@amarulasolutions.com> wrote:
> Hi Axel > > On Sat, Sep 14, 2013 at 11:34 AM, Axel Lin <axel....@ingics.com> wrote: > > 2013/9/14 Albert ARIBAUD <albert.u.b...@aribaud.net>: > >> Hi Axel, > >> > >> On Fri, 06 Sep 2013 14:22:40 +0800, Axel Lin <axel....@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....@ingics.com> > >>> Acked-by: Stefan Roese <s...@denx.de> > >>> Reviewed-by: Vipin Kumar <vipin.ku...@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, ®s->gpiodata[DATA_REG_ADDR(gpio)]); > >>> + if (value) > >>> + writel(1 << gpio, ®s->gpiodata[DATA_REG_ADDR(gpio)]); > >>> + else > >>> + writel(0, ®s->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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
