gpio: spear_gpio: Fix gpio_set_value() implementation
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
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.
--- drivers/gpio/spear_gpio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/gpio/spear_gpio.c +4 -1
@@ -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)]);return 0 ; }
Signature · 1 line
1.8.1.2
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
Quoted 5 lines · 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 Lin <[email protected]>
Thanks,
Stefan
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
In reply to Axel LinHi
Il giorno 20/giu/2013 09:14, "Axel Lin" <[email protected]> ha scritto:
Quoted 2 lines
In current
gpio_set_value()implementation, it always sets the gpio
control bit
Quoted 14 lines
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 <[email protected]>--- drivers/gpio/spear_gpio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpio/spear_gpio.c +0
-0@@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value) {
drivers/gpio/spear_gpio.c int gpio_set_value(unsigned gpio, int value) · line 61
Quoted 10 lines
drivers/gpio/spear_gpio.c +4
-1struct 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 ; }
Does it work the clear? Seems that it sets to 0 all the bank. I'm using the mobile
Michael
Quoted 9 lines
--
1.8.1.2_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
Dear Michael Trimarchi,
Quoted 32 lines
Hi
Il giorno 20/giu/2013 09:14, "Axel Lin" <[email protected]> ha scritto:
Quoted 1 line
In current
gpio_set_value()implementation, it always sets the gpiocontrol bit
Quoted 25 lines
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 <[email protected]>---
drivers/gpio/spear_gpio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpio/spear_gpio.c +4
-1@@ -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)]);
Quoted 6 lines
Quoted 3 lines
return 0;}
Does it work the clear? Seems that it sets to 0 all the bank. I'm using the mobile
I don't think I speak language of your tribe (lol) ;-)
What's "the mobile" please ?
Best regards,
Marek Vasut
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
Hi
Il giorno 20/giu/2013 15:57, "Marek Vasut" <[email protected]> ha scritto:
Quoted 6 lines
Dear Michael Trimarchi,
Quoted 3 lines
Hi
Il giorno 20/giu/2013 09:14, "Axel Lin" <[email protected]> ha
scritto:
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
In current
gpio_set_value()implementation, it always sets the gpiocontrol bit
Quoted 1 line
no matter the value argument is 0 or 1. Thus the GPIOs never set to
low.
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
This patch fixes this bug.
Signed-off-by Axel Lin <[email protected]>---
drivers/gpio/spear_gpio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpio/spear_gpio.c +0
-0@@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value)
Quoted 7 lines
Quoted 7 lines
drivers/gpio/spear_gpio.c int gpio_set_value(unsigned gpio, int value)
Quoted 7 lines
drivers/gpio/spear_gpio.c +2
-1{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)]);
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
drivers/gpio/spear_gpio.c +2
-0+else +writel (0 , ®s->gpiodata[DATA_REG_ADDR (gpio)]);return 0;}
Does it work the clear? Seems that it sets to 0 all the bank. I'm using
the
Quoted 8 lines
Quoted 1 line
mobile
I don't think I speak language of your tribe (lol) ;-)
What's "the mobile" please ?
Best regards,
Marek Vasut
Android mobile, italian dictinary and same comment of you
:D
Michael
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
Dear Michael Trimarchi,
Quoted 56 lines
Hi
Il giorno 20/giu/2013 15:57, "Marek Vasut" <[email protected]> ha scritto:
Quoted 5 lines
Dear Michael Trimarchi,
Quoted 3 lines
Hi
Il giorno 20/giu/2013 09:14, "Axel Lin" <[email protected]> ha
scritto:
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
In current
gpio_set_value()implementation, it always sets the gpiocontrol bit
Quoted 1 line
no matter the value argument is 0 or 1. Thus the GPIOs never set to
low.
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
This patch fixes this bug.
Signed-off-by Axel Lin <[email protected]>---
drivers/gpio/spear_gpio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpio/spear_gpio.c +0
-0@@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value)Quoted 8 lines
Quoted 8 lines
drivers/gpio/spear_gpio.c int gpio_set_value(unsigned gpio, int value)
Quoted 8 lines
drivers/gpio/spear_gpio.c +2
-1{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)]);
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
drivers/gpio/spear_gpio.c +2
-0+else +writel (0 , ®s->gpiodata[DATA_REG_ADDR (gpio)]);return 0;}
Does it work the clear? Seems that it sets to 0 all the bank. I'm using
the
Quoted 2 lines
Quoted 1 line
mobile
Quoted 8 lines
Quoted 6 lines
I don't think I speak language of your tribe (lol) ;-)
What's "the mobile" please ?
Best regards,
Marek VasutAndroid mobile, italian dictinary and same comment of you
Poor you, the android {keyboard,mailer,} is such a crap ;-(
Best regards,
Marek Vasut
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
In reply to Axel LinDear Axel Lin,
Quoted 14 lines
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 <[email protected]>--- drivers/gpio/spear_gpio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpio/spear_gpio.c +0
-0
drivers/gpio/spear_gpio.c int gpio_set_value(unsigned gpio, int value) · line 58
Quoted 8 lines
drivers/gpio/spear_gpio.c +4
-1@@ -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 ?
Best regards,
Marek Vasut
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
2013/6/20 Marek Vasut <[email protected]>
Quoted 20 lines
Dear Axel Lin,
Quoted 17 lines
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 <[email protected]>--- drivers/gpio/spear_gpio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpio/spear_gpio.c +0
-0@@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value) {struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE ;
Quoted 8 lines
drivers/gpio/spear_gpio.c int gpio_set_value(unsigned gpio, int value) · line 58
Quoted 5 lines
drivers/gpio/spear_gpio.c +4
-1-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?
Thanks,
Axel
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
Hi
Quoted 31 lines · Axel Lin
2013/6/20 Marek Vasut <[email protected]>
Quoted 20 lines
Dear Axel Lin,
Quoted 17 lines
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 <[email protected]>--- drivers/gpio/spear_gpio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpio/spear_gpio.c +0
-0@@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value) {struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE ;Quoted 8 lines
drivers/gpio/spear_gpio.c int gpio_set_value(unsigned gpio, int value) · line 58
Quoted 5 lines
drivers/gpio/spear_gpio.c +4
-1-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 ?
Quoted 3 lines
Because each GPIO is controlled by a register. And only one bit will be set when set gpio to high.
Yes, but how to reset just one bit if you use the same register offset?
I don't know this core but I know two possibilities:
1) one set register and one clear register
if (enable) writel(1 << gpio, REGSET_BANK(gpio)); else writel(1 << gpio, REGCLEAR_BANK(gpio)); 2) or set operation reg = readl(REG(gpio); if (enable) reg |= 1 << gpio; else reg &= ~(1 << gpio); writel(reg, REG(GPIO));
Any other way?
Michael
Quoted 15 lines
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, bothgpio_get_value()andgpio_set_value()need fix.)Vipin, can you review this patch and confirm this behavior?
Thanks,
Axel
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Signature · 3 lines
| Michael Nazzareno Trimarchi Amarula Solutions BV | | COO - Founder Cruquiuskade 47 | | +31(0)851119172 Amsterdam 1018 AM NL |
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
Dear Michael Trimarchi,
Quoted 57 lines
Hi
Quoted 33 lines · Axel Lin
2013/6/20 Marek Vasut <[email protected]>
Quoted 22 lines
Dear Axel Lin,
Quoted 20 lines
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 <[email protected]>---
drivers/gpio/spear_gpio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpio/spear_gpio.c +0
-0@@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value) {struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE ;Quoted 8 lines
drivers/gpio/spear_gpio.c int gpio_set_value(unsigned gpio, int value) · line 58
Quoted 5 lines
drivers/gpio/spear_gpio.c +4
-1-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 ?
Quoted 2 lines
Because each GPIO is controlled by a register. And only one bit will be set when set gpio to high.
Yes, but how to reset just one bit if you use the same register offset?
I don't know this core but I know two possibilities:
1) one set register and one clear register
if (enable) writel(1 << gpio, REGSET_BANK(gpio)); else writel(1 << gpio, REGCLEAR_BANK(gpio)); 2) or set operation reg = readl(REG(gpio); if (enable) reg |= 1 << gpio; else reg &= ~(1 << gpio); writel(reg, REG(GPIO));
Quoted 1 line
Any other way?
I think it's about time to read the datasheet :b
Best regards,
Marek Vasut
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
In reply to Axel LinQuoted 34 lines · Axel Lin
2013/6/20 Marek Vasut<[email protected]>
Quoted 20 lines
Dear Axel Lin,
Quoted 17 lines
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<[email protected]>--- drivers/gpio/spear_gpio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpio/spear_gpio.c +0
-0@@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value) {struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE ;Quoted 8 lines
drivers/gpio/spear_gpio.c int gpio_set_value(unsigned gpio, int value) · line 58
Quoted 5 lines
drivers/gpio/spear_gpio.c +4
-1-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.
Quoted 8 lines
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, bothgpio_get_value()andgpio_set_value()need fix.)Vipin, can you review this patch and confirm this behavior?
Yes this is right. and the code is fine
Regards
Vipin
Quoted 4 lines
Thanks,
Axel
.
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
Quoted 37 lines · Vipin Kumar
Quoted 36 lines · Axel Lin
2013/6/20 Marek Vasut<[email protected]>
Quoted 20 lines
Dear Axel Lin,
Quoted 17 lines
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<[email protected]>--- drivers/gpio/spear_gpio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpio/spear_gpio.c +0
-0@@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value) {struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE ;Quoted 8 lines
drivers/gpio/spear_gpio.c int gpio_set_value(unsigned gpio, int value) · line 58
Quoted 5 lines
drivers/gpio/spear_gpio.c +4
-1-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.
Quoted 9 lines
Quoted 6 lines
Note, the
gpio_get_value()implementation also assumes there is only one bit will be set. ( If this is not true, bothgpio_get_value()andgpio_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?
Michael
Quoted 12 lines
Regards
VipinQuoted 4 lines
Thanks,
Axel
._______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
2013/6/21 Michael Trimarchi <[email protected]>:
Quoted 42 lines · Vipin Kumar
Quoted 41 lines · Axel Lin
Quoted 40 lines
2013/6/20 Marek Vasut<[email protected]>
Quoted 20 lines
Dear Axel Lin,
Quoted 17 lines
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<[email protected]>--- drivers/gpio/spear_gpio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpio/spear_gpio.c +0
-0@@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value) {struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE ;Quoted 8 lines
drivers/gpio/spear_gpio.c int gpio_set_value(unsigned gpio, int value) · line 58
Quoted 5 lines
drivers/gpio/spear_gpio.c +4
-1-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, bothgpio_get_value()andgpio_set_value()need fix.)
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
Hi
Il giorno 30/giu/2013 06:18, "Axel Lin" <[email protected]> ha scritto:
Quoted 9 lines
2013/6/21 Michael Trimarchi <[email protected]>:
Quoted 7 lines · Vipin Kumar
Quoted 6 lines · Axel Lin
Quoted 5 lines
2013/6/20 Marek Vasut<[email protected]>
Quoted 4 lines
Dear Axel Lin,
Quoted 1 line
In current
gpio_set_value()implementation, it always sets the gpio
control
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
bit no matter the value argument is 0 or 1. Thus the GPIOs never
set to
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
low. This patch fixes this bug.
Signed-off-by Axel Lin<[email protected]>--- drivers/gpio/spear_gpio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/gpio/spear_gpio.c int gpio_set_value(unsigned gpio, int value) · line 53
Quoted 7 lines
drivers/gpio/spear_gpio.c +0
-0@@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value) { struct gpio_regs *regs = (struct gpio_regs
*)CONFIG_GPIO_BASE;
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/gpio/spear_gpio.c int gpio_set_value(unsigned gpio, int value) · line 55
Quoted 4 lines
drivers/gpio/spear_gpio.c +2
-1-writel (1 << gpio,®s->gpiodata[DATA_REG_ADDR (gpio)]); +if (value) + writel(1<<
gpio,®s->gpiodata[DATA_REG_ADDR(gpio)]);
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/gpio/spear_gpio.c int gpio_set_value(unsigned gpio, int value) · line 57
Quoted 2 lines
drivers/gpio/spear_gpio.c +2
-0+else +writel (0 ,®s->gpiodata[DATA_REG_ADDR (gpio)]);How can this possibly work? Writing 0 to the whole bank will unset
all the
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 1 line
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
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
will be set. ( If this is not true, both
gpio_get_value()and
gpio_set_value()
Quoted 18 lines
Quoted 10 lines
Quoted 7 lines
Quoted 4 lines
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.)
Quoted 4 lines
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?
And how it works for gpio 33?
Michael
Quoted 2 lines
Regards,
Axel
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
2013/6/30 Michael Trimarchi <[email protected]>:
Quoted 71 lines
Hi
Il giorno 30/giu/2013 06:18, "Axel Lin" <[email protected]> ha scritto:Quoted 67 lines
2013/6/21 Michael Trimarchi <[email protected]>:
Quoted 49 lines · Vipin Kumar
Quoted 48 lines · Axel Lin
Quoted 47 lines
2013/6/20 Marek Vasut<[email protected]>
Quoted 23 lines
Dear Axel Lin,
Quoted 20 lines
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<[email protected]>--- drivers/gpio/spear_gpio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpio/spear_gpio.c +0
-0@@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value) {struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE ;Quoted 10 lines
drivers/gpio/spear_gpio.c int gpio_set_value(unsigned gpio, int value) · line 60
Quoted 6 lines
drivers/gpio/spear_gpio.c +4
-1-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, bothgpio_get_value()andgpio_set_value()
need fix.)Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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.)
Quoted 7 lines
Quoted 4 lines
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.
Quoted 2 lines
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
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
Quoted 80 lines · Axel Lin
2013/6/30 Michael Trimarchi<[email protected]>:
Quoted 71 lines
Hi
Il giorno 30/giu/2013 06:18, "Axel Lin"<[email protected]> ha scritto:Quoted 67 lines
2013/6/21 Michael Trimarchi<[email protected]>:
Quoted 49 lines · Vipin Kumar
Quoted 48 lines · Axel Lin
Quoted 47 lines
2013/6/20 Marek Vasut<[email protected]>
Quoted 23 lines
Dear Axel Lin,
Quoted 20 lines
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<[email protected]>--- drivers/gpio/spear_gpio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpio/spear_gpio.c +0
-0@@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value) {struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE ;Quoted 10 lines
drivers/gpio/spear_gpio.c int gpio_set_value(unsigned gpio, int value) · line 60
Quoted 6 lines
drivers/gpio/spear_gpio.c +4
-1-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, bothgpio_get_value()andgpio_set_value()
need fix.)Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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.)Quoted 7 lines
Quoted 4 lines
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?
Quoted 9 lines
Because the meaningful bit for different register is different.
Quoted 2 lines
And how it works for gpio 33?
SPEAR_GPIO_COUNTis 8, so this driver only allows setting gpio0 ~ gpio7.Vipin, any chance to double check the datasheet and confirm if this patch is ok?
The questions raised here are valid and it forced me to re-read the datasheet. For your convenience, I must tell you that the device is actually pl061 from ARM, so the driver can also be named so.
The datasheet is here
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0190b/I1002697.html
Quoting from the datasheet
"The GPIODATA register is the data register. In software control mode, values written in the GPIODATA register are transferred onto the GPOUT pins if the respective pins have been configured as outputs through the GPIODIR register.
In order to write to GPIODATA, the corresponding bits in the mask, resulting from the address bus, PADDR[9:2], must be HIGH. Otherwise the bit values remain unchanged by the write.
Similarly, the values read from this register are determined for each bit, by the mask bit derived from the address used to access the data register, PADDR[9:2]. Bits that are 1 in the address mask cause the corresponding bits in GPIODATA to be read, and bits that are 0 in the address mask cause the corresponding bits in GPIODATA to be read as 0, regardless of their value.
A read from GPIODATA returns the last bit value written if the respective pins are configured as output, or it returns the value on the corresponding input GPIN bit when these are configured as inputs. All bits are cleared by a reset."
After reading all this I am confused about numbering of the gpio's. I think the numbering should be from 1 to 8 for a device. And this would mean that we should write to *®s->datareg[1 << (gpio - 1)]* instead of the present code which is _®s->datareg[1 << (gpio + 2)]_
Moreover, One GPIO device can control only 8 pins, so there is no question of having GPIO33. In an SoC design, GPIO33 may actually map to GPIO1 of device 4. I hope I am clear on this
Regards
Vipin
Quoted 4 lines
Regards,
Axel
.
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
Quoted 29 lines
The questions raised here are valid and it forced me to re-read the datasheet. For your convenience, I must tell you that the device is actually pl061 from ARM, so the driver can also be named so.
The datasheet is here
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0190b/I1002697.htmlQuoting from the datasheet
"The GPIODATA register is the data register. In software control mode, values written in the GPIODATA register are transferred onto the GPOUT pins if the respective pins have been configured as outputs through the GPIODIR register.In order to write to GPIODATA, the corresponding bits in the mask, resulting from the address bus, PADDR[9:2], must be HIGH. Otherwise the bit values remain unchanged by the write.
Similarly, the values read from this register are determined for each bit, by the mask bit derived from the address used to access the data register, PADDR[9:2]. Bits that are 1 in the address mask cause the corresponding bits in GPIODATA to be read, and bits that are 0 in the address mask cause the corresponding bits in GPIODATA to be read as 0, regardless of their value.
A read from GPIODATA returns the last bit value written if the respective pins are configured as output, or it returns the value on the corresponding input GPIN bit when these are configured as inputs. All bits are cleared by a reset."
Quoted 4 lines
After reading all this I am confused about numbering of the gpio's. I think the numbering should be from 1 to 8 for a device. And this would mean that we should write to *®s->datareg[1 << (gpio - 1)]* instead of the present code which is _®s->datareg[1 << (gpio + 2)]_
Hi Vipin,
Thanks for the review and providing the datasheet information. You mentioned that this is PL061.
So... I just checked the gpio-pl061 driver in linux kernel. It's writing to _®s->datareg[1 << (gpio + 2)]. and seems no bug report for this.
And the gpio_get/set implementation in linux kernel has the same behavior as this patch does:
( below is from linux/drivers/gpio/gpio-pl061.c )
static int pl061_get_value(struct gpio_chip *gc, unsigned offset)
{
struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
return !!readb(chip->base + (1 << (offset + 2)));
}static void pl061_set_value(struct gpio_chip *gc, unsigned offset, int value)
{
struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
writeb(!!value << offset, chip->base + (1 << (offset + 2)));
}BTW, it would be great if you have the hardware to test.
Regards,
Axel
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
Quoted 29 lines · Axel Lin
Quoted 29 lines
The questions raised here are valid and it forced me to re-read the datasheet. For your convenience, I must tell you that the device is actually pl061 from ARM, so the driver can also be named so.
The datasheet is here
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0190b/I1002697.htmlQuoting from the datasheet
"The GPIODATA register is the data register. In software control mode, values written in the GPIODATA register are transferred onto the GPOUT pins if the respective pins have been configured as outputs through the GPIODIR register.In order to write to GPIODATA, the corresponding bits in the mask, resulting from the address bus, PADDR[9:2], must be HIGH. Otherwise the bit values remain unchanged by the write.
Similarly, the values read from this register are determined for each bit, by the mask bit derived from the address used to access the data register, PADDR[9:2]. Bits that are 1 in the address mask cause the corresponding bits in GPIODATA to be read, and bits that are 0 in the address mask cause the corresponding bits in GPIODATA to be read as 0, regardless of their value.
A read from GPIODATA returns the last bit value written if the respective pins are configured as output, or it returns the value on the corresponding input GPIN bit when these are configured as inputs. All bits are cleared by a reset."
Quoted 6 lines
Quoted 4 lines
After reading all this I am confused about numbering of the gpio's. I think the numbering should be from 1 to 8 for a device. And this would mean that we should write to *®s->datareg[1<< (gpio - 1)]* instead of the present code which is _®s->datareg[1<< (gpio + 2)]_
Hi Vipin,
Hello Alex,
Quoted 6 lines
Thanks for the review and providing the datasheet information. You mentioned that this is PL061.
So... I just checked the gpio-pl061 driver in linux kernel. It's writing to _®s->datareg[1<< (gpio + 2)]. and seems no bug report for this.
Yes, I see it now. The difference is that we are using a writel and the datareg is a u32 array.
Quoted 12 lines
And the gpio_get/set implementation in linux kernel has the same behavior as this patch does:
( below is from
linux/drivers/gpio/gpio-pl061.c)static int
pl061_get_value(struct gpio_chip *gc, unsigned offset){ struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc); return !!readb(chip->base + (1<< (offset + 2))); }
Quoted 9 lines
static void
pl061_set_value(struct gpio_chip *gc, unsigned offset, int value){ struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc); writeb(!!value<< offset, chip->base + (1<< (offset + 2))); }BTW, it would be great if you have the hardware to test.
I am sorry about this. I have now moved to a different group and I have no access to the hardware
Regards
Vipin
Quoted 4 lines
Regards,
Axel
.
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
2013/7/1 Vipin Kumar <[email protected]>:
Quoted 64 lines · Axel Lin
Quoted 50 lines
Quoted 41 lines
The questions raised here are valid and it forced me to re-read the datasheet. For your convenience, I must tell you that the device is actually
pl061 from ARM, so the driver can also be named so.The datasheet is here
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0190b/I1002697.html
Quoting from the datasheet
"The GPIODATA register is the data register. In software control mode, values written in the GPIODATA register are transferred onto the GPOUT pins
if the respective pins have been configured as outputs through the GPIODIR
register.In order to write to GPIODATA, the corresponding bits in the mask, resulting
from the address bus, PADDR[9:2], must be HIGH. Otherwise the bit values remain unchanged by the write.Similarly, the values read from this register are determined for each bit,
by the mask bit derived from the address used to access the data register,
PADDR[9:2]. Bits that are 1 in the address mask cause the corresponding bits
in GPIODATA to be read, and bits that are 0 in the address mask cause the corresponding bits in GPIODATA to be read as 0, regardless of their value.A read from GPIODATA returns the last bit value written if the respective pins are configured as output, or it returns the value on the corresponding
input GPIN bit when these are configured as inputs. All bits are cleared by
a reset."Quoted 7 lines
After reading all this I am confused about numbering of the gpio's. I think
the numbering should be from 1 to 8 for a device. And this would mean that
we should write to *®s->datareg[1<< (gpio - 1)]* instead of the present
code which is _®s->datareg[1<< (gpio + 2)]_Quoted 1 line
Hi Vipin,
Hello Alex,
Quoted 6 lines
Thanks for the review and providing the datasheet information. You mentioned that this is PL061.
So... I just checked the gpio-pl061 driver in linux kernel. It's writing to _®s->datareg[1<< (gpio + 2)]. and seems no bug report for this.
Quoted 2 lines
Yes, I see it now. The difference is that we are using a writel and the datareg is a u32 array.
Hi,
The merge window is going to close.
I'm wondering if this patch is ok or not (I think it's a bug fix). I think the only issue is nobody has this hardware to test. [Sorry to back to this topic so late... just too busy recently.]
Regards,
Axel
PATCH 1/1 gpio: spear_gpio: Fix gpio_set_value() implementation
Hi
Quoted 65 lines · Axel Lin
2013/7/1 Vipin Kumar <[email protected]>:
Quoted 64 lines · Axel Lin
Quoted 50 lines
Quoted 41 lines
The questions raised here are valid and it forced me to re-read the datasheet. For your convenience, I must tell you that the device is actually
pl061 from ARM, so the driver can also be named so.The datasheet is here
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0190b/I1002697.html
Quoting from the datasheet
"The GPIODATA register is the data register. In software control mode, values written in the GPIODATA register are transferred onto the GPOUT pins
if the respective pins have been configured as outputs through the GPIODIR
register.In order to write to GPIODATA, the corresponding bits in the mask, resulting
from the address bus, PADDR[9:2], must be HIGH. Otherwise the bit values remain unchanged by the write.Similarly, the values read from this register are determined for each bit,
by the mask bit derived from the address used to access the data register,
PADDR[9:2]. Bits that are 1 in the address mask cause the corresponding bits
in GPIODATA to be read, and bits that are 0 in the address mask cause the corresponding bits in GPIODATA to be read as 0, regardless of their value.A read from GPIODATA returns the last bit value written if the respective pins are configured as output, or it returns the value on the corresponding
input GPIN bit when these are configured as inputs. All bits are cleared by
a reset."Quoted 7 lines
After reading all this I am confused about numbering of the gpio's. I think
the numbering should be from 1 to 8 for a device. And this would mean that
we should write to *®s->datareg[1<< (gpio - 1)]* instead of the present
code which is _®s->datareg[1<< (gpio + 2)]_Quoted 1 line
Hi Vipin,
Hello Alex,
Quoted 6 lines
Thanks for the review and providing the datasheet information. You mentioned that this is PL061.
So... I just checked the gpio-pl061 driver in linux kernel. It's writing to _®s->datareg[1<< (gpio + 2)]. and seems no bug report for this.
Quoted 9 lines
Quoted 2 lines
Yes, I see it now. The difference is that we are using a writel and the datareg is a u32 array.
Hi,
The merge window is going to close.
I'm wondering if this patch is ok or not (I think it's a bug fix). I think the only issue is nobody has this hardware to test. [Sorry to back to this topic so late... just too busy recently.]
You are right. I read the documentation and it works
Michael
Quoted 2 lines
Regards,
Axel