2013/6/21 Michael Trimarchi michael@amarulasolutions.com:
On 06/21/2013 06:40 AM, Vipin Kumar wrote:
On 6/20/2013 7:26 PM, Axel Lin wrote:
2013/6/20 Marek Vasutmarex@denx.de
Dear Axel Lin,
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 Linaxel.lin@ingics.com
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 d3c728e..8878608 100644 --- a/drivers/gpio/spear_gpio.c +++ b/drivers/gpio/spear_gpio.c @@ -52,7 +52,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)]);elsewritel(0,®s->gpiodata[DATA_REG_ADDR(gpio)]);How can this possibly work? Writing 0 to the whole bank will unset all the GPIOs, no ?
Because each GPIO is controlled by a register. And only one bit will be set when set gpio to high.
So it's safe to write 0 for clearing the bit.
Note, the gpio_get_value() implementation also assumes there is only one bit will be set. ( If this is not true, both gpio_get_value() and gpio_set_value() need fix.)
Vipin, can you review this patch and confirm this behavior?
Yes this is right. and the code is fine
The problem is not in set one bit but in reset one bit. Can you check the else path?
Hi, I'm not the best person to answer this question because I don't have the hardware and datasheet. In the case only one bit is meaningful and the reset bits are 0, it looks ok for me to write 0 for clearing the bit. ( note, each gpio pin is controlled by different register.)
This patch is acked and reviewed by Stefan Roese and Vipin Kumar. I'm wondering if this patch is acceptable? Or maybe a test-by can help to make this patch acceptable?
Regards, Axel