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