Hi Axel,
On Fri, 06 Sep 2013 14:22:40 +0800, Axel Lin axel.lin@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@ingics.com Acked-by: Stefan Roese sr@denx.de Reviewed-by: Vipin Kumar vipin.kumar@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,