arm/orion: use set_bit and clear_bit in gpio implementation

set_bit and clear_bit are already atomic operations. So use this functions to set or unset gpio configurations. This makes the additional spinlock unneeded.

set_bit and clear_bit are already atomic operations. So use this functions to set or unset gpio configurations. This makes the additional spinlock unneeded.

Signed-off-by Stefan Bigler <[email protected]>

Signed-off-by Holger Brunck <[email protected]>

cc Lennert Buytenhek <[email protected]>

cc Nicolas Pitre <[email protected]>

cc Russell King <[email protected]>

cc Thomas Gleixner <[email protected]>

---
 arch/arm/plat-orion/gpio.c |   38 ++++++--------------------------------
 1 files changed, 6 insertions(+), 32 deletions(-)

arch/arm/plat-orion/gpio.c +6 -32

index 41ab97e..d9d5577 100644
@@ -31,7 +31,6 @@
 
 struct orion_gpio_chip {
 	struct gpio_chip	chip;
-	spinlock_t		lock;
 	void __iomem		*base;
 	unsigned long		valid_input;
 	unsigned long		valid_output;

@@ -86,39 +85,27 @@ static int orion_gpio_chip_count;
 static inline void
 __set_direction(struct orion_gpio_chip *ochip, unsigned pin, int input)
 {
-	u32 u;
-
-	u = readl(GPIO_IO_CONF(ochip));
 	if (input)
-		u |= 1 << pin;
+		set_bit(pin, (void*)GPIO_IO_CONF(ochip));
 	else
-		u &= ~(1 << pin);
-	writel(u, GPIO_IO_CONF(ochip));
+		clear_bit(pin, (void*)GPIO_IO_CONF(ochip));
 }
 
 static void __set_level(struct orion_gpio_chip *ochip, unsigned pin, int high)
 {
-	u32 u;
-
-	u = readl(GPIO_OUT(ochip));
 	if (high)
-		u |= 1 << pin;
+		set_bit(pin, (void*)GPIO_OUT(ochip));
 	else
-		u &= ~(1 << pin);
-	writel(u, GPIO_OUT(ochip));
+		clear_bit(pin, (void*)GPIO_OUT(ochip));
 }
 
 static inline void
 __set_blinking(struct orion_gpio_chip *ochip, unsigned pin, int blink)
 {
-	u32 u;
-
-	u = readl(GPIO_BLINK_EN(ochip));
 	if (blink)
-		u |= 1 << pin;
+		set_bit(pin, (void*)GPIO_BLINK_EN(ochip));
 	else
-		u &= ~(1 << pin);
-	writel(u, GPIO_BLINK_EN(ochip));
+		clear_bit(pin, (void*)GPIO_BLINK_EN(ochip));
 }
 
 static inline int

@@ -159,14 +146,11 @@ static int orion_gpio_direction_input(struct gpio_chip *chip, unsigned pin)
 {
 	struct orion_gpio_chip *ochip =
 		container_of(chip, struct orion_gpio_chip, chip);
-	unsigned long flags;
 
 	if (!orion_gpio_is_valid(ochip, pin, GPIO_INPUT_OK))
 		return -EINVAL;
 
-	spin_lock_irqsave(&ochip->lock, flags);
 	__set_direction(ochip, pin, 1);
-	spin_unlock_irqrestore(&ochip->lock, flags);
 
 	return 0;
 }

@@ -191,16 +175,13 @@ orion_gpio_direction_output(struct gpio_chip *chip, unsigned pin, int value)
 {
 	struct orion_gpio_chip *ochip =
 		container_of(chip, struct orion_gpio_chip, chip);
-	unsigned long flags;
 
 	if (!orion_gpio_is_valid(ochip, pin, GPIO_OUTPUT_OK))
 		return -EINVAL;
 
-	spin_lock_irqsave(&ochip->lock, flags);
 	__set_blinking(ochip, pin, 0);
 	__set_level(ochip, pin, value);
 	__set_direction(ochip, pin, 0);
-	spin_unlock_irqrestore(&ochip->lock, flags);
 
 	return 0;
 }

@@ -209,11 +190,8 @@ static void orion_gpio_set(struct gpio_chip *chip, unsigned pin, int value)
 {
 	struct orion_gpio_chip *ochip =
 		container_of(chip, struct orion_gpio_chip, chip);
-	unsigned long flags;
 
-	spin_lock_irqsave(&ochip->lock, flags);
 	__set_level(ochip, pin, value);
-	spin_unlock_irqrestore(&ochip->lock, flags);
 }
 
 static int orion_gpio_to_irq(struct gpio_chip *chip, unsigned pin)

@@ -283,15 +261,12 @@ void __init orion_gpio_set_valid(unsigned pin, int mode)
 void orion_gpio_set_blink(unsigned pin, int blink)
 {
 	struct orion_gpio_chip *ochip = orion_gpio_chip_find(pin);
-	unsigned long flags;
 
 	if (ochip == NULL)
 		return;
 
-	spin_lock_irqsave(&ochip->lock, flags);
 	__set_level(ochip, pin, 0);
 	__set_blinking(ochip, pin, blink);
-	spin_unlock_irqrestore(&ochip->lock, flags);
 }
 EXPORT_SYMBOL(orion_gpio_set_blink);
 

@@ -399,7 +374,6 @@ void __init orion_gpio_init(int gpio_base, int ngpio,
 	ochip->chip.base = gpio_base;
 	ochip->chip.ngpio = ngpio;
 	ochip->chip.can_sleep = 0;
-	spin_lock_init(&ochip->lock);
 	ochip->base = (void __iomem *)base;
 	ochip->valid_input = 0;
 	ochip->valid_output = 0;

Signature · 1 line

1.7.1

Quoted 10 lines · Holger Brunck

set_bit and clear_bit are already atomic operations. So use this functions to set or unset gpio configurations. This makes the additional spinlock unneeded.

Signed-off-by Stefan Bigler <[email protected]>

Signed-off-by Holger Brunck <[email protected]>

cc Lennert Buytenhek <[email protected]>

cc Nicolas Pitre <[email protected]>

cc Russell King <[email protected]>

cc Thomas Gleixner <[email protected]>

NAK.

Although this ends up equivalent in practice at the moment, the IO space accessed through readl() and writel() is conceptually a different thing than normal memory where the atomic set/clear bit functions are used. There is no guarantee that the value passed to readl()/writel() will always match the virtual address where the access is performed.

Furthermore, there is no guarantee made by the atomic bit manipulation routines about the actual access to memory. At one point those used to be implemented with byte accesses not word accesses, and there is nothing preventing the implementation from changing in similar ways in the future that might have unwanted effects with IO memory.

And if using an ARMv6+ core as found in the Marvell Dove or MV78xx0 SOCs which this patch also affects then the atomic bitops are implemented in terms of the load/store exclusive instructions which are architecturally not defined when applied to IO space.