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? > > Amicalement, > -- > Albert. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
