Hi On Sun, Sep 15, 2013 at 5:20 PM, Albert ARIBAUD <albert.u.b...@aribaud.net> wrote: > 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. >
I think that patch need to have a better description Michael >> Michael > > Amicalement, > -- > Albert. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
