2013/6/30 Michael Trimarchi michael@amarulasolutions.com:
Hi Il giorno 30/giu/2013 06:18, "Axel Lin" axel.lin@ingics.com ha scritto:
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)]); > + else > + writel(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?
If each pin is controlled by a different register why you need to 1<<gpio in set path?
Because the meaningful bit for different register is different.
And how it works for gpio 33?
SPEAR_GPIO_COUNT is 8, so this driver only allows setting gpio0 ~ gpio7.
Vipin, any chance to double check the datasheet and confirm if this patch is ok?
Regards, Axel