gpio: zevio: Get correct gpio output value
PATCH 1/1 gpio: zevio: Get correct gpio output value
Read gpio output value from ZEVIO_GPIO_OUTPUT.
The spin_lock is required to ensure the direction is not changed before reading input/output value.
---
Hi Fabian,
The document only mentions:
+10 (R/W): Direction (set bit to 0 for output, 1 for input) +14 (R/W): GPIO output bit
+18 (R): Reads GPIO input bit.
So I'm not sure if reading ZEVIO_GPIO_INPUT returns correct status when the direction is output. So I think this needs test.
Regards,
Axel
drivers/gpio/gpio-zevio.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/gpio/gpio-zevio.c +8 -2
@@ -81,9 +81,15 @@ static inline void zevio_gpio_port_set(struct zevio_gpio *c, unsigned pin,static int zevio_gpio_get (struct gpio_chip *chip,unsigned pin) {struct zevio_gpio *controller =to_zevio_gpio (chip); +u32 val, dir; -/* Only reading allowed, so no spinlock needed */ -u32 val =zevio_gpio_port_get (controller, pin,ZEVIO_GPIO_INPUT ); +spin_lock (&controller->lock); + dir =zevio_gpio_port_get (controller, pin,ZEVIO_GPIO_DIRECTION ); +if (dir |BIT (ZEVIO_GPIO_BIT (pin))) + val =zevio_gpio_port_get (controller, pin,ZEVIO_GPIO_INPUT ); +else + val =zevio_gpio_port_get (controller, pin,ZEVIO_GPIO_OUTPUT ); +spin_unlock (&controller->lock);return (val >>ZEVIO_GPIO_BIT (pin)) &0x1 ; }
Signature · 1 line
1.8.3.2
Hi,
IIRC I tested that and input and output are independant of each other.
If you set a pin as output and write a 1 into the output register, but you connect the pin to ground physically output is "1" and input "0". Which behaviour is the correct one for gpio_get?
Bye,
Fabian
Am Montag, 7. April 2014, 14:34:52 schrieb Axel Lin:
Quoted 42 lines
Read gpio output value from
ZEVIO_GPIO_OUTPUT.
Thespin_lockis required to ensure the direction is not changed before reading input/output value.Signed-off-by Axel Lin <[email protected]>---
Hi Fabian,
The document only mentions:
+10 (R/W): Direction (set bit to 0 for output, 1 for input) +14 (R/W): GPIO output bit
+18 (R): Reads GPIO input bit.
So I'm not sure if readingZEVIO_GPIO_INPUTreturns correct status when the direction is output. So I think this needs test.Regards,
Axeldrivers/gpio/gpio-zevio.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)drivers/gpio/gpio-zevio.c +8
-2@@ -81,9 +81,15 @@ static inline void zevio_gpio_port_set(struct zevio_gpio *c, unsigned pin,static int zevio_gpio_get (struct gpio_chip *chip,unsigned pin) {struct zevio_gpio *controller =to_zevio_gpio (chip); +u32 val, dir; -/* Only reading allowed, so no spinlock needed */ -u32 val =zevio_gpio_port_get (controller, pin,ZEVIO_GPIO_INPUT ); +spin_lock (&controller->lock); + dir =zevio_gpio_port_get (controller, pin,ZEVIO_GPIO_DIRECTION ); +if (dir |BIT (ZEVIO_GPIO_BIT (pin))) + val =zevio_gpio_port_get (controller, pin,ZEVIO_GPIO_INPUT ); +else + val =zevio_gpio_port_get (controller, pin,ZEVIO_GPIO_OUTPUT ); +spin_unlock (&controller->lock);return (val >>ZEVIO_GPIO_BIT (pin)) &0x1 ; }
Quoted 6 lines · Fabian Vogt
Hi,
IIRC I tested that and input and output are independant of each other.
If you set a pin as output and write a 1 into the output register, but you connect the pin to ground physically output is "1" and input "0". Which behaviour is the correct one forgpio_get?
Please don't top-post, and wrap overly long lines! Thanks.
I'd assume that gpio_get() should return the information about the pin level ("is"), regardless of internal "want" state. But that's a guess (gut feeling), can't tell whether there is a specified semantics, or whether this is supposed to be "implementation defined". Different hardware may have different capabilites (like lack of support to read back what's on the line, thus having to rely on internal registers which may differ from the pin level).
Quoted 12 lines
Am Montag, 7. April 2014, 14:34:52 schrieb Axel Lin:
Quoted 11 lines
Read gpio output value from
ZEVIO_GPIO_OUTPUT.
Thespin_lockis required to ensure the direction is not changed before reading input/output value.[ ... ]
drivers/gpio/gpio-zevio.c +1
-0@@ -81,9 +81,15 @@ static inline void zevio_gpio_port_set(struct zevio_gpio *c, unsigned pin,static int zevio_gpio_get (struct gpio_chip *chip,unsigned pin) {struct zevio_gpio *controller =to_zevio_gpio (chip); +u32 val, dir;
Quoted 9 lines
drivers/gpio/gpio-zevio.c static inline void zevio_gpio_port_set(struct zevio_gpio *c, unsigned pin, · line 92
Quoted 9 lines
drivers/gpio/gpio-zevio.c +7
-2-/* Only reading allowed, so no spinlock needed */ -u32 val =zevio_gpio_port_get (controller, pin,ZEVIO_GPIO_INPUT ); +spin_lock (&controller->lock); + dir =zevio_gpio_port_get (controller, pin,ZEVIO_GPIO_DIRECTION ); +if (dir |BIT (ZEVIO_GPIO_BIT (pin))) + val =zevio_gpio_port_get (controller, pin,ZEVIO_GPIO_INPUT ); +else + val =zevio_gpio_port_get (controller, pin,ZEVIO_GPIO_OUTPUT ); +spin_unlock (&controller->lock);
As a side effect of your making me look at the code:
Is the if() not an "always true" condition? Should this be s/|/\&/ instead?
virtually yours
Gerhard Sittig
Signature · 3 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: [email protected]
Quoted 18 lines
Quoted 6 lines
Quoted 6 lines
drivers/gpio/gpio-zevio.c +1
-0@@ -81,9 +81,15 @@ static inline void zevio_gpio_port_set(struct zevio_gpio *c, unsigned pin,static int zevio_gpio_get (struct gpio_chip *chip,unsigned pin) {struct zevio_gpio *controller =to_zevio_gpio (chip); +u32 val, dir;Quoted 9 lines
drivers/gpio/gpio-zevio.c static inline void zevio_gpio_port_set(struct zevio_gpio *c, unsigned pin, · line 92
Quoted 9 lines
drivers/gpio/gpio-zevio.c +7
-2-/* Only reading allowed, so no spinlock needed */ -u32 val =zevio_gpio_port_get (controller, pin,ZEVIO_GPIO_INPUT ); +spin_lock (&controller->lock); + dir =zevio_gpio_port_get (controller, pin,ZEVIO_GPIO_DIRECTION ); +if (dir |BIT (ZEVIO_GPIO_BIT (pin))) + val =zevio_gpio_port_get (controller, pin,ZEVIO_GPIO_INPUT ); +else + val =zevio_gpio_port_get (controller, pin,ZEVIO_GPIO_OUTPUT ); +spin_unlock (&controller->lock);As a side effect of your making me look at the code:
Quoted 2 lines
Is the
if()not an "always true" condition? Should this be s/|/\&/ instead?
You are right, I'm sending a v2 now.
Thanks for the review,
Axel