[PATCH] mach-ixp4xx: made gpio functions atomic

[PATCH] mach-ixp4xx: made gpio functions atomic

[Frans Meulenbroeks]

gpio_line_set and gpio_line config on ixp4xx were not atomic
This patch fixes this by surrounding them with local_irq_save and
local_irq_restore calls, similar to the way Lennert Buytenhek
implemented this for iop. (see arch/arm/plat-iop/gpio.c)

Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks@gmail.com>
---

Note: an alternative solution would be to use the set_bit and 
clear_bit atomic functions, but I felt it better to do things
the same way as Lennert did. Also it could be that this is a few
bytes smaller. Didn't check that.

--- git/arch/arm/mach-ixp4xx/include/mach/platform.h.orig	2011-08-09 19:16:52.645482269 +0200
+++ git/arch/arm/mach-ixp4xx/include/mach/platform.h	2011-08-09 19:17:39.915482235 +0200
@@ -14,6 +14,8 @@
 #ifndef __ASSEMBLY__
 
 #include <asm/types.h>
+#include <asm/system.h>
+#include <linux/kernel.h>
 
 #ifndef	__ARMEB__
 #define	REG_OFFSET	0
@@ -150,10 +152,14 @@
 
 static inline void gpio_line_config(u8 line, u32 direction)
 {
+	unsigned long flags;
+
+	local_irq_save(flags);
 	if (direction == IXP4XX_GPIO_IN)
 		*IXP4XX_GPIO_GPOER |= (1 << line);
 	else
 		*IXP4XX_GPIO_GPOER &= ~(1 << line);
+	local_irq_restore(flags);
 }
 
 static inline void gpio_line_get(u8 line, int *value)
@@ -163,10 +169,14 @@
 
 static inline void gpio_line_set(u8 line, int value)
 {
+	unsigned long flags;
+
+	local_irq_save(flags);
 	if (value == IXP4XX_GPIO_HIGH)
 	    *IXP4XX_GPIO_GPOUTR |= (1 << line);
 	else if (value == IXP4XX_GPIO_LOW)
 	    *IXP4XX_GPIO_GPOUTR &= ~(1 << line);
+	local_irq_restore(flags);
 }
 
 #endif // __ASSEMBLY__

[PATCH] mach-ixp4xx: made gpio functions atomic

[Russell King - ARM Linux]

On Sun, Oct 30, 2011 at 12:21:33PM +0100, Frans Meulenbroeks wrote:
> gpio_line_set and gpio_line config on ixp4xx were not atomic
> This patch fixes this by surrounding them with local_irq_save and
> local_irq_restore calls, similar to the way Lennert Buytenhek
> implemented this for iop. (see arch/arm/plat-iop/gpio.c)
> 
> Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks@gmail.com>
> ---
> 
> Note: an alternative solution would be to use the set_bit and 
> clear_bit atomic functions, but I felt it better to do things
> the same way as Lennert did. Also it could be that this is a few
> bytes smaller. Didn't check that.

set_bit and clear_bit should not be used on IO memory - on ARMv6 and
later these use the load exclusive/store exclusive operations which
are only valid on 'normal memory' mappings and not 'device' mappings.

Plus of course, IO memory is supposed to be __iomem and accessed using
the proper accessors, and using set_bit and clear_bit on such pointers
will issue sparse warnings.

[PATCH] mach-ixp4xx: made gpio functions atomic

[Russell King - ARM Linux]

On Mon, Oct 31, 2011 at 09:32:55AM +0100, Frans Meulenbroeks wrote:
> May I consider the patch accepted or is some rework needed?

Neither, I was just commenting on it.  MAINTAINERS says:

ARM/INTEL IXP4XX ARM ARCHITECTURE
M:      Imre Kaloz <kaloz@openwrt.org>
M:      Krzysztof Halasa <khc@pm.waw.pl>
L:      linux-arm-kernel at lists.infradead.org (moderated for non-subscribers)
S:      Maintained
F:      arch/arm/mach-ixp4xx/

It would be a good idea to have acks from Imre and/or Krzysztof, even
better if they can apply it themselves.

[PATCH] mach-ixp4xx: made gpio functions atomic

[Russell King - ARM Linux]

On Wed, Dec 28, 2011 at 04:45:21PM +0100, Frans Meulenbroeks wrote:
> Ping.
> 
> Didn't hear anything concerning this patch I submitted 2 months ago.
> Seems a very trivial patch to me. Would it be possible to apply this (or
> provide feedback why it is not ok).

This should really be using the gpiolib stuff rather than persisting with
its own private gpio API invention (and fixing problems in it which
gpiolib already solves.)

Any chance someone could look into doing that conversion?

[PATCH] mach-ixp4xx: made gpio functions atomic

[Frans Meulenbroeks]

gpio_line_set and gpio_line config on ixp4xx were not atomic
This patch fixes this by surrounding them with local_irq_save and
local_irq_restore calls, similar to the way Lennert Buytenhek
implemented this for iop. (see arch/arm/plat-iop/gpio.c)

Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks@gmail.com>
---

Note: an alternative solution would be to use the set_bit and 
clear_bit atomic functions, but I felt it better to do things
the same way as Lennert did. Also it could be that this is a few
bytes smaller. Didn't check that.

PS: please Cc me as I am not on the list

--- git/arch/arm/mach-ixp4xx/include/mach/platform.h.orig	2011-08-09 19:16:52.645482269 +0200
+++ git/arch/arm/mach-ixp4xx/include/mach/platform.h	2011-08-09 19:17:39.915482235 +0200
@@ -14,6 +14,8 @@
 #ifndef __ASSEMBLY__
 
 #include <asm/types.h>
+#include <asm/system.h>
+#include <linux/kernel.h>
 
 #ifndef	__ARMEB__
 #define	REG_OFFSET	0
@@ -150,10 +152,14 @@
 
 static inline void gpio_line_config(u8 line, u32 direction)
 {
+	unsigned long flags;
+
+	local_irq_save(flags);
 	if (direction == IXP4XX_GPIO_IN)
 		*IXP4XX_GPIO_GPOER |= (1 << line);
 	else
 		*IXP4XX_GPIO_GPOER &= ~(1 << line);
+	local_irq_restore(flags);
 }
 
 static inline void gpio_line_get(u8 line, int *value)
@@ -163,10 +169,14 @@
 
 static inline void gpio_line_set(u8 line, int value)
 {
+	unsigned long flags;
+
+	local_irq_save(flags);
 	if (value == IXP4XX_GPIO_HIGH)
 	    *IXP4XX_GPIO_GPOUTR |= (1 << line);
 	else if (value == IXP4XX_GPIO_LOW)
 	    *IXP4XX_GPIO_GPOUTR &= ~(1 << line);
+	local_irq_restore(flags);
 }
 
 #endif // __ASSEMBLY__