pinctrl/at91: Fix pin_to_mask

PATCH New Alexander Stein 6 participants 10 messages Apr 11, 2014 – Apr 22, 2014

Subsystems (3)

Arm/Microchip (AT91) SoC support Pin control Pin controller - Microchip AT91

lore

Share & tools

lore

Alexander Stein

author

2014-04-11 15:35 UTC #0 lore raw

PATCH 1/1 pinctrl/at91: Fix pin_to_mask

We need first to reduce the pin number to only a GPIO bank before we can create the mask.
Otherwise only GPIO bank 0 has correct masks as the bits in the other banks are shifted out of range.

Signed-off-by Alexander Stein <[email protected]>

---
 drivers/pinctrl/pinctrl-at91.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/pinctrl/pinctrl-at91.c +1 -1

index 63176f2..6669e13 100644
@@ -316,7 +316,7 @@ static inline int pin_to_bank(unsigned pin)
 
 static unsigned pin_to_mask(unsigned int pin)
 {
-	return 1 << pin;
+	return 1 << (pin % MAX_NB_GPIO_PER_BANK);
 }
 
 static void at91_mux_disable_interrupt(void __iomem *pio, unsigned mask)

Signature · 1 line

1.9.2

Jean-Christophe PLAGNIOL-VILLARD

2014-04-11 15:42 UTC #1 lore raw

PATCH 1/1 pinctrl/at91: Fix pin_to_mask

Quoted 16 lines · Alexander Stein

We need first to reduce the pin number to only a GPIO bank before we can create the mask.
Otherwise only GPIO bank 0 has correct masks as the bits in the other banks are shifted out of range.
Signed-off-by Alexander Stein <[email protected]>
---
drivers/pinctrl/pinctrl-at91.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pinctrl/pinctrl-at91.c +0 -0
index 63176f2..6669e13 100644

drivers/pinctrl/pinctrl-at91.c static inline int pin_to_bank(unsigned pin) · line 320

Quoted 6 lines

drivers/pinctrl/pinctrl-at91.c +1 -1

@@ -316,7 +316,7 @@ static inline int pin_to_bank(unsigned pin)
 
 static unsigned pin_to_mask(unsigned int pin)
 {
-	return 1 << pin;
+	return 1 << (pin % MAX_NB_GPIO_PER_BANK);
 }

no need pin_to_mask is already called with it

Quoted 6 lines

drivers/pinctrl/pinctrl-at91.c +0 -0
 static void at91_mux_disable_interrupt(void __iomem *pio, unsigned mask)
Signature · 1 line
1.9.2

Alexander Stein

author

2014-04-11 15:45 UTC #2 lore raw

PATCH 1/1 pinctrl/at91: Fix pin_to_mask

On Friday 11 April 2014, 23:42:19 wrote Jean-Christophe PLAGNIOL-VILLARD:

Quoted 20 lines · Alexander Stein

Quoted 18 lines
We need first to reduce the pin number to only a GPIO bank before we can create the mask.
Otherwise only GPIO bank 0 has correct masks as the bits in the other banks are shifted out of range.
Signed-off-by Alexander Stein <[email protected]>
---
drivers/pinctrl/pinctrl-at91.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pinctrl/pinctrl-at91.c +0 -0
index 63176f2..6669e13 100644
@@ -316,7 +316,7 @@ static inline int pin_to_bank(unsigned pin)
 

Quoted 6 lines

drivers/pinctrl/pinctrl-at91.c static inline int pin_to_bank(unsigned pin) · line 320
Quoted 5 lines
drivers/pinctrl/pinctrl-at91.c +1 -1
 static unsigned pin_to_mask(unsigned int pin)
 {
-	return 1 << pin;
+	return 1 << (pin % MAX_NB_GPIO_PER_BANK);
 }
no need pin_to_mask is already called with it

But this only true for the call within at91_pinconf_set, but not for those in at91_gpio_dbg_show
at91_pmx_enable
at91_pmx_disable

Regards,
Alexander

Mark Roszko

2014-04-11 17:53 UTC #3 lore raw

PATCH 1/1 pinctrl/at91: Fix pin_to_mask

The member struct pin inside the at91_pmx_pin structs is already the bank pin number, it was documented at the top of the driver.

/**
* struct at91_pmx_pin - describes an At91 pin mux
* @bank: the bank of the pin
* @pin: the pin number in the @bank
* @mux: the mux mode : gpio or periph_x of the pin i.e. alternate function.
* @conf: the configuration of the pin: PULL_UP, MULTIDRIVE etc... */

if the pmx_pin->pin member wasn't already a bank number, then other logic would break such as if (pin->pin >= MAX_NB_GPIO_PER_BANK) in pin_check_config. In fact pmx_enable calls pin_check_config and should catch an error.

Alexander Stein

author

2014-04-11 18:33 UTC #4 lore raw

PATCH 1/1 pinctrl/at91: Fix mask creation in at91_gpio_dbg_show

pin_to_mask expects a bank pin number. So do not add the chip base.

Signed-off-by Alexander Stein <[email protected]>

---
Mark, thanks for the clarification that pin_to_mask only expects tha bank pin number. This wasn't obvious to me.

 drivers/pinctrl/pinctrl-at91.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/pinctrl/pinctrl-at91.c +1 -2

index 63176f2..2da0e6e 100644
@@ -1205,8 +1205,7 @@ static void at91_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	void __iomem *pio = at91_gpio->regbase;
 
 	for (i = 0; i < chip->ngpio; i++) {
-		unsigned pin = chip->base + i;
-		unsigned mask = pin_to_mask(pin);
+		unsigned mask = pin_to_mask(i);
 		const char *gpio_label;
 		u32 pdsr;

Signature · 1 line

1.9.2

Alexandre Belloni

2014-04-14 08:14 UTC #5 lore raw

PATCH 1/1 pinctrl/at91: Fix mask creation in at91_gpio_dbg_show

Quoted 2 lines · Alexander Stein

pin_to_mask expects a bank pin number. So do not add the chip base.

I would prefer that you add a description of the effects of the bug as seen by the user.

Quoted 1 line

Signed-off-by Alexander Stein <[email protected]>

else

Acked-by Alexandre Belloni <[email protected]>

Quoted 28 lines

---
Mark, thanks for the clarification that pin_to_mask only expects tha bank pin number. This wasn't obvious to me.

 drivers/pinctrl/pinctrl-at91.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/pinctrl/pinctrl-at91.c +1 -2

index 63176f2..2da0e6e 100644
@@ -1205,8 +1205,7 @@ static void at91_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	void __iomem *pio = at91_gpio->regbase;
 
 	for (i = 0; i < chip->ngpio; i++) {
-		unsigned pin = chip->base + i;
-		unsigned mask = pin_to_mask(pin);
+		unsigned mask = pin_to_mask(i);
 		const char *gpio_label;
 		u32 pdsr;

Signature · 7 lines

1.9.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Signature · 3 lines

Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Alexander Stein

author

2014-04-14 18:53 UTC #6 lore raw

PATCH v2 1/1 pinctrl/at91: Fix mask creation in at91_gpio_dbg_show

pin_to_mask expects a bank pin number. So do not add the chip base.

Without that patch cat /sys/kernel/debug/gpio looks like that:
GPIOs 0-31, platform/fffff200.gpio, fffff200.gpio:
[spi32766.0] GPIOfffff200.gpio5: [gpio] set [ads7846_pendown] GPIOfffff200.gpio15: [gpio] set [ohci_vbus] GPIOfffff200.gpio21: [gpio] set [ohci_vbus] GPIOfffff200.gpio24: [gpio] set [button1] GPIOfffff200.gpio28: [gpio] clear [button2] GPIOfffff200.gpio29: [gpio] clear

GPIOs 32-63, platform/fffff400.gpio, fffff400.gpio:
[sda] GPIOfffff400.gpio4: [periph A]
[scl] GPIOfffff400.gpio5: [periph A] [spi32766.3] GPIOfffff400.gpio11: [periph A] [error] GPIOfffff400.gpio22: [periph A] [run] GPIOfffff400.gpio23: [periph A]

GPIOs 64-95, platform/fffff600.gpio, fffff600.gpio:
[reset_pin] GPIOfffff600.gpio29: [periph A]

GPIOs 96-127, platform/fffff800.gpio, fffff800.gpio:
[led1] GPIOfffff800.gpio5: [periph A]
[led2] GPIOfffff800.gpio6: [periph A]
[led3] GPIOfffff800.gpio7: [periph A]
[led4] GPIOfffff800.gpio8: [periph A]

GPIOs 128-159, platform/fffffa00.gpio, fffffa00.gpio:
[button3] GPIOfffffa00.gpio10: [periph A] [button4] GPIOfffffa00.gpio12: [periph A]

Note that every bank despite bank 0 only shows "periph A" which are obviously used as GPIOs.

Signed-off-by Alexander Stein <[email protected]>

Acked-by Alexandre Belloni <[email protected]>

Acked-by Nicolas Ferre <[email protected]>

---
Changes in v2:
* Added 2 Acked-by's
* Add an example of wrong output

 drivers/pinctrl/pinctrl-at91.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/pinctrl/pinctrl-at91.c +1 -2

index 63176f2..2da0e6e 100644
@@ -1205,8 +1205,7 @@ static void at91_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	void __iomem *pio = at91_gpio->regbase;
 
 	for (i = 0; i < chip->ngpio; i++) {
-		unsigned pin = chip->base + i;
-		unsigned mask = pin_to_mask(pin);
+		unsigned mask = pin_to_mask(i);
 		const char *gpio_label;
 		u32 pdsr;

Signature · 1 line

1.9.2

Nicolas Ferre

2014-04-16 07:55 UTC #7 lore raw

PATCH v2 1/1 pinctrl/at91: Fix mask creation in at91_gpio_dbg_show

On 14/04/2014 20:53, Alexander Stein :

Quoted 27 lines

pin_to_mask expects a bank pin number. So do not add the chip base.
Without that patch cat /sys/kernel/debug/gpio looks like that:
GPIOs 0-31, platform/fffff200.gpio, fffff200.gpio:
[spi32766.0] GPIOfffff200.gpio5: [gpio] set [ads7846_pendown] GPIOfffff200.gpio15: [gpio] set [ohci_vbus] GPIOfffff200.gpio21: [gpio] set [ohci_vbus] GPIOfffff200.gpio24: [gpio] set [button1] GPIOfffff200.gpio28: [gpio] clear [button2] GPIOfffff200.gpio29: [gpio] clear
GPIOs 32-63, platform/fffff400.gpio, fffff400.gpio:
[sda] GPIOfffff400.gpio4: [periph A]
[scl] GPIOfffff400.gpio5: [periph A] [spi32766.3] GPIOfffff400.gpio11: [periph A] [error] GPIOfffff400.gpio22: [periph A] [run] GPIOfffff400.gpio23: [periph A]
GPIOs 64-95, platform/fffff600.gpio, fffff600.gpio:
[reset_pin] GPIOfffff600.gpio29: [periph A]
GPIOs 96-127, platform/fffff800.gpio, fffff800.gpio:
[led1] GPIOfffff800.gpio5: [periph A]
[led2] GPIOfffff800.gpio6: [periph A]
[led3] GPIOfffff800.gpio7: [periph A]
[led4] GPIOfffff800.gpio8: [periph A]

Quoted 3 lines

GPIOs 128-159, platform/fffffa00.gpio, fffffa00.gpio:
[button3] GPIOfffffa00.gpio10: [periph A] [button4] GPIOfffffa00.gpio12: [periph A]

Maybe a little bit less context to the commit message: Linus might remove some lines to it...

Quoted 6 lines

Note that every bank despite bank 0 only shows "periph A" which are obviously used as GPIOs.
Signed-off-by Alexander Stein <[email protected]>
Acked-by Alexandre Belloni <[email protected]>
Acked-by Nicolas Ferre <[email protected]>

Okay, actually this patch have to be routed to Linus Walleij, I add him in the destination field.

Thanks,

Quoted 23 lines

---
Changes in v2:
* Added 2 Acked-by's
* Add an example of wrong output

 drivers/pinctrl/pinctrl-at91.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/pinctrl/pinctrl-at91.c +1 -2

index 63176f2..2da0e6e 100644
@@ -1205,8 +1205,7 @@ static void at91_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	void __iomem *pio = at91_gpio->regbase;
 
 	for (i = 0; i < chip->ngpio; i++) {
-		unsigned pin = chip->base + i;
-		unsigned mask = pin_to_mask(pin);
+		unsigned mask = pin_to_mask(i);
 		const char *gpio_label;
 		u32 pdsr;

Signature · 1 line

Nicolas Ferre

Linus Walleij

2014-04-22 21:46 UTC #8 lore raw

PATCH v2 1/1 pinctrl/at91: Fix mask creation in at91_gpio_dbg_show

In reply to Alexander Stein

Quoted 34 lines · Alexander Stein

pin_to_mask expects a bank pin number. So do not add the chip base.
Without that patch cat /sys/kernel/debug/gpio looks like that:
GPIOs 0-31, platform/fffff200.gpio, fffff200.gpio:
[spi32766.0] GPIOfffff200.gpio5: [gpio] set [ads7846_pendown] GPIOfffff200.gpio15: [gpio] set [ohci_vbus] GPIOfffff200.gpio21: [gpio] set [ohci_vbus] GPIOfffff200.gpio24: [gpio] set [button1] GPIOfffff200.gpio28: [gpio] clear [button2] GPIOfffff200.gpio29: [gpio] clear
GPIOs 32-63, platform/fffff400.gpio, fffff400.gpio:
[sda] GPIOfffff400.gpio4: [periph A]
[scl] GPIOfffff400.gpio5: [periph A] [spi32766.3] GPIOfffff400.gpio11: [periph A] [error] GPIOfffff400.gpio22: [periph A] [run] GPIOfffff400.gpio23: [periph A]
GPIOs 64-95, platform/fffff600.gpio, fffff600.gpio:
[reset_pin] GPIOfffff600.gpio29: [periph A]
GPIOs 96-127, platform/fffff800.gpio, fffff800.gpio:
[led1] GPIOfffff800.gpio5: [periph A]
[led2] GPIOfffff800.gpio6: [periph A]
[led3] GPIOfffff800.gpio7: [periph A]
[led4] GPIOfffff800.gpio8: [periph A]
GPIOs 128-159, platform/fffffa00.gpio, fffffa00.gpio:
[button3] GPIOfffffa00.gpio10: [periph A] [button4] GPIOfffffa00.gpio12: [periph A]
Note that every bank despite bank 0 only shows "periph A" which are obviously used as GPIOs.

Quoted 7 lines

Signed-off-by Alexander Stein <[email protected]>
Acked-by Alexandre Belloni <[email protected]>
Acked-by Nicolas Ferre <[email protected]>
---
Changes in v2:
* Added 2 Acked-by's
* Add an example of wrong output

Patch applied.

Yours,
Linus Walleij

Nicolas Ferre

2014-04-14 08:19 UTC #9 lore raw

PATCH 1/1 pinctrl/at91: Fix mask creation in at91_gpio_dbg_show

In reply to Alexander Stein

On 11/04/2014 20:33, Alexander Stein :

Quoted 3 lines

pin_to_mask expects a bank pin number. So do not add the chip base.
Signed-off-by Alexander Stein <[email protected]>

Indeed, seems to be a bug in this function:

Acked-by Nicolas Ferre <[email protected]>

Thanks a lot. Bye,

Quoted 21 lines

---
Mark, thanks for the clarification that pin_to_mask only expects tha bank pin number. This wasn't obvious to me.

 drivers/pinctrl/pinctrl-at91.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/pinctrl/pinctrl-at91.c +1 -2

index 63176f2..2da0e6e 100644
@@ -1205,8 +1205,7 @@ static void at91_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	void __iomem *pio = at91_gpio->regbase;
 
 	for (i = 0; i < chip->ngpio; i++) {
-		unsigned pin = chip->base + i;
-		unsigned mask = pin_to_mask(pin);
+		unsigned mask = pin_to_mask(i);
 		const char *gpio_label;
 		u32 pdsr;

Signature · 1 line

Nicolas Ferre