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. 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
