qe_lib: Set gpio data before changing the direction to output
PATCH 1/1 qe_lib: Set gpio data before changing the direction to output
This avoids having a short glitch if the desired initial value is not the same as what was previously in the data register.
---
I can't think of a reason not to do this. The data register has no effect except when the pin is configured as an output, right?
Please enlighten me if this is not correct. The behaviour I see gels with my thinking, but there may be a case I haven't thought of.
arch/powerpc/sysdev/qe_lib/gpio.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
arch/powerpc/sysdev/qe_lib/gpio.c +1 -2
@@ -107,12 +107,11 @@ static int qe_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)spin_lock_irqsave (&qe_gc->lock, flags); +qe_gpio_set (gc, gpio, val);__par_io_config_pin (mm_gc->regs, gpio,QE_PIO_DIR_OUT ,0 ,0 ,0 );spin_unlock_irqrestore (&qe_gc->lock, flags); -qe_gpio_set (gc, gpio, val); -return 0 ; }
Signature · 1 line
1.6.3.3
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v2 1/1 qe_lib: Set gpio data before changing the direction to output
This avoids having a short glitch if the desired initial value is not the same as what was previously in the data register.
---
Quoted 5 lines · Anton Vorontsov
There is a recursive locking bug:
_set()takes the same spinlock. So you'd better move this call two lines upper. Otherwise the patch looks OK.Thanks!
Thanks - here is v2.
arch/powerpc/sysdev/qe_lib/gpio.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
arch/powerpc/sysdev/qe_lib/gpio.c +2 -2
@@ -105,14 +105,14 @@ static int qe_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)struct qe_gpio_chip *qe_gc =to_qe_gpio_chip (mm_gc);unsigned long flags; +qe_gpio_set (gc, gpio, val); +spin_lock_irqsave (&qe_gc->lock, flags);__par_io_config_pin (mm_gc->regs, gpio,QE_PIO_DIR_OUT ,0 ,0 ,0 );spin_unlock_irqrestore (&qe_gc->lock, flags); -qe_gpio_set (gc, gpio, val); -return 0 ; }
Signature · 1 line
1.6.3.3
Quoted 4 lines · Michael Barkowski
This avoids having a short glitch if the desired initial value is not the same as what was previously in the data register.
Signed-off-by Michael Barkowski <[email protected]>
Thanks!
Quoted 37 lines
---
Quoted 5 lines · Anton Vorontsov
There is a recursive locking bug:
_set()takes the same spinlock. So you'd better move this call two lines upper. Otherwise the patch looks OK.Thanks!
Thanks - here is v2.
arch/powerpc/sysdev/qe_lib/gpio.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)arch/powerpc/sysdev/qe_lib/gpio.c +2
-2@@ -105,14 +105,14 @@ static int qe_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)struct qe_gpio_chip *qe_gc =to_qe_gpio_chip (mm_gc);unsigned long flags; +qe_gpio_set (gc, gpio, val); +spin_lock_irqsave (&qe_gc->lock, flags);__par_io_config_pin (mm_gc->regs, gpio,QE_PIO_DIR_OUT ,0 ,0 ,0 );spin_unlock_irqrestore (&qe_gc->lock, flags); -qe_gpio_set (gc, gpio, val); -return 0 ; }Signature · 1 line
1.6.3.3
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2
Quoted 7 lines · Anton Vorontsov
Quoted 4 lines · Michael Barkowski
This avoids having a short glitch if the desired initial value is not the same as what was previously in the data register.
Signed-off-by Michael Barkowski <[email protected]>Acked-by Anton Vorontsov <[email protected]>
I don't have the time to test this patch, so I abstain from acking. :-) If Anton likes it, that's good enough for me.
Signature · 2 lines
Timur Tabi Linux kernel developer at Freescale
Quoted 11 lines · Timur Tabi
Quoted 7 lines · Anton Vorontsov
Quoted 4 lines · Michael Barkowski
This avoids having a short glitch if the desired initial value is not the same as what was previously in the data register.
Signed-off-by Michael Barkowski <[email protected]>Acked-by Anton Vorontsov <[email protected]>I don't have the time to test this patch, so I abstain from acking. :-) If Anton likes it, that's good enough for me.
You made me doubt for a moment. :-) Thanks for the suspiciousness.
What happens if a pin was previously configured as input? Does our write to the data register survive? For MPC8xxx GPIO controllers it does. And randomly taken QE spec says:
A write to CPDAT is latched, and if the corresponding CPDIR bits have configured the port pin as an output, the latched value is driven onto the respective pin. However, if the corresponding CPDIR bits have configured the port pin as an input, the latched value is prevented from reaching the pin.
I guess we're safe, but Michael, could you actually test it (if not already)?
Thanks!
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2
Quoted 17 lines · Anton Vorontsov
Quoted 9 lines · Timur Tabi
Quoted 6 lines · Anton Vorontsov
Quoted 4 lines · Michael Barkowski
This avoids having a short glitch if the desired initial value is not the same as what was previously in the data register.
Signed-off-by Michael Barkowski <[email protected]>Acked-by Anton Vorontsov <[email protected]>I don't have the time to test this patch, so I abstain from acking. :-) If Anton likes it, that's good enough for me.
You made me doubt for a moment. :-) Thanks for the suspiciousness.
What happens if a pin was previously configured as input? Does our write to the data register survive? For MPC8xxx GPIO controllers it does. And randomly taken QE spec says:
Quoted 9 lines
A write to CPDAT is latched, and if the corresponding CPDIR bits have configured the port pin as an output, the latched value is driven onto the respective pin. However, if the corresponding CPDIR bits have configured the port pin as an input, the latched value is prevented from reaching the pin.
I guess we're safe, but Michael, could you actually test it (if not already)?
I had tested it before with the pin initially configured as "disabled".
I have now also tested it with the pin initially configured as "input".
The value written to CPDAT seems to survive and is driven onto the pin once CPDIR is changed to 1, just as noted in the spec.
Tested on 8360, by probing with a logic analyzer.
There are lots of users of this code. I understand if you'd like it to stay open for testing by others.
Signature · 1 line
Michael Barkowski
Quoted 28 lines · Michael Barkowski
Quoted 17 lines · Anton Vorontsov
Quoted 9 lines · Timur Tabi
Quoted 6 lines · Anton Vorontsov
Quoted 4 lines · Michael Barkowski
This avoids having a short glitch if the desired initial value is not the same as what was previously in the data register.
Signed-off-by Michael Barkowski <[email protected]>Acked-by Anton Vorontsov <[email protected]>I don't have the time to test this patch, so I abstain from acking. :-) If Anton likes it, that's good enough for me.
You made me doubt for a moment. :-) Thanks for the suspiciousness.
What happens if a pin was previously configured as input? Does our write to the data register survive? For MPC8xxx GPIO controllers it does. And randomly taken QE spec says:
Quoted 9 lines
A write to CPDAT is latched, and if the corresponding CPDIR bits have configured the port pin as an output, the latched value is driven onto the respective pin. However, if the corresponding CPDIR bits have configured the port pin as an input, the latched value is prevented from reaching the pin.
I guess we're safe, but Michael, could you actually test it (if not already)?
Quoted 8 lines
I had tested it before with the pin initially configured as "disabled".
I have now also tested it with the pin initially configured as "input".
The value written to CPDAT seems to survive and is driven onto the pin once CPDIR is changed to 1, just as noted in the spec.
Tested on 8360, by probing with a logic analyzer.
Great, thanks a lot! I think the patch is perfect.
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2
Kumar Gala
AppliedQuoted 10 lines · Michael Barkowski
This avoids having a short glitch if the desired initial value is not the same as what was previously in the data register.
Signed-off-by Michael Barkowski <[email protected]>---
Quoted 4 lines · Anton Vorontsov
There is a recursive locking bug:
_set()takes the same spinlock. So you'd better move this call two lines upper. Otherwise the patch looks OK.
Quoted 6 lines
Quoted 1 line
Thanks!
Thanks - here is v2.
arch/powerpc/sysdev/qe_lib/gpio.c| 4 ++--1 files changed, 2 insertions(+), 2 deletions(-)
applied to next.
- k
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 8 lines · Timur Tabi
Quoted 6 lines · Michael Barkowski
arch/powerpc/sysdev/qe_lib/gpio.c +0
-0@@ -107,12 +107,11 @@ static int qe_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
Quoted 7 lines
arch/powerpc/sysdev/qe_lib/gpio.c static int qe_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) · line 110
Quoted 3 lines
arch/powerpc/sysdev/qe_lib/gpio.c +1
-0spin_lock_irqsave (&qe_gc->lock, flags); +qe_gpio_set (gc, gpio, val);
qe_gpio_setalready callsspin_lock_irqsave(), so you'll have nested spinlocks, which will lock up on SMP.Let me guess, you didn't test this on a dual-core system?
That is correct. See v2 and please test, YMMV, etc
Signature · 1 line
Michael Barkowski