* [PATCH] pinctrl/at91: Fix pin_to_mask @ 2014-04-11 15:35 Alexander Stein 2014-04-11 15:42 ` Jean-Christophe PLAGNIOL-VILLARD 0 siblings, 1 reply; 10+ messages in thread From: Alexander Stein @ 2014-04-11 15:35 UTC (permalink / raw) To: linux-arm-kernel 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 <alexanders83@web.de> --- drivers/pinctrl/pinctrl-at91.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c index 63176f2..6669e13 100644 --- a/drivers/pinctrl/pinctrl-at91.c +++ b/drivers/pinctrl/pinctrl-at91.c @@ -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) -- 1.9.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] pinctrl/at91: Fix pin_to_mask 2014-04-11 15:35 [PATCH] pinctrl/at91: Fix pin_to_mask Alexander Stein @ 2014-04-11 15:42 ` Jean-Christophe PLAGNIOL-VILLARD 2014-04-11 15:45 ` Alexander Stein 0 siblings, 1 reply; 10+ messages in thread From: Jean-Christophe PLAGNIOL-VILLARD @ 2014-04-11 15:42 UTC (permalink / raw) To: linux-arm-kernel On Apr 11, 2014, at 11:35 PM, Alexander Stein <alexanders83@web.de> wrote: > > 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 <alexanders83@web.de> > --- > drivers/pinctrl/pinctrl-at91.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c > index 63176f2..6669e13 100644 > --- a/drivers/pinctrl/pinctrl-at91.c > +++ b/drivers/pinctrl/pinctrl-at91.c > @@ -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 > > > static void at91_mux_disable_interrupt(void __iomem *pio, unsigned mask) > -- > 1.9.2 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] pinctrl/at91: Fix pin_to_mask 2014-04-11 15:42 ` Jean-Christophe PLAGNIOL-VILLARD @ 2014-04-11 15:45 ` Alexander Stein 2014-04-11 17:53 ` Mark Roszko 0 siblings, 1 reply; 10+ messages in thread From: Alexander Stein @ 2014-04-11 15:45 UTC (permalink / raw) To: linux-arm-kernel On Friday 11 April 2014, 23:42:19 wrote Jean-Christophe PLAGNIOL-VILLARD: > > On Apr 11, 2014, at 11:35 PM, Alexander Stein <alexanders83@web.de> wrote: > > > > > 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 <alexanders83@web.de> > > --- > > drivers/pinctrl/pinctrl-at91.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c > > index 63176f2..6669e13 100644 > > --- a/drivers/pinctrl/pinctrl-at91.c > > +++ b/drivers/pinctrl/pinctrl-at91.c > > @@ -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 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] pinctrl/at91: Fix pin_to_mask 2014-04-11 15:45 ` Alexander Stein @ 2014-04-11 17:53 ` Mark Roszko 2014-04-11 18:33 ` [PATCH] pinctrl/at91: Fix mask creation in at91_gpio_dbg_show Alexander Stein 0 siblings, 1 reply; 10+ messages in thread From: Mark Roszko @ 2014-04-11 17:53 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] pinctrl/at91: Fix mask creation in at91_gpio_dbg_show 2014-04-11 17:53 ` Mark Roszko @ 2014-04-11 18:33 ` Alexander Stein 2014-04-14 8:14 ` Alexandre Belloni 2014-04-14 8:19 ` [PATCH] " Nicolas Ferre 0 siblings, 2 replies; 10+ messages in thread From: Alexander Stein @ 2014-04-11 18:33 UTC (permalink / raw) To: linux-arm-kernel pin_to_mask expects a bank pin number. So do not add the chip base. Signed-off-by: Alexander Stein <alexanders83@web.de> --- 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(-) diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c index 63176f2..2da0e6e 100644 --- a/drivers/pinctrl/pinctrl-at91.c +++ b/drivers/pinctrl/pinctrl-at91.c @@ -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; -- 1.9.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] pinctrl/at91: Fix mask creation in at91_gpio_dbg_show 2014-04-11 18:33 ` [PATCH] pinctrl/at91: Fix mask creation in at91_gpio_dbg_show Alexander Stein @ 2014-04-14 8:14 ` Alexandre Belloni 2014-04-14 18:53 ` [PATCH v2] " Alexander Stein 2014-04-14 8:19 ` [PATCH] " Nicolas Ferre 1 sibling, 1 reply; 10+ messages in thread From: Alexandre Belloni @ 2014-04-14 8:14 UTC (permalink / raw) To: linux-arm-kernel On 11/04/2014 at 20:33:13 +0200, Alexander Stein wrote : > 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. > Signed-off-by: Alexander Stein <alexanders83@web.de> else Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > --- > 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(-) > > diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c > index 63176f2..2da0e6e 100644 > --- a/drivers/pinctrl/pinctrl-at91.c > +++ b/drivers/pinctrl/pinctrl-at91.c > @@ -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; > > -- > 1.9.2 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] pinctrl/at91: Fix mask creation in at91_gpio_dbg_show 2014-04-14 8:14 ` Alexandre Belloni @ 2014-04-14 18:53 ` Alexander Stein 2014-04-16 7:55 ` Nicolas Ferre 2014-04-22 21:46 ` Linus Walleij 0 siblings, 2 replies; 10+ messages in thread From: Alexander Stein @ 2014-04-14 18:53 UTC (permalink / raw) To: linux-arm-kernel 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 <alexanders83@web.de> Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> --- 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(-) diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c index 63176f2..2da0e6e 100644 --- a/drivers/pinctrl/pinctrl-at91.c +++ b/drivers/pinctrl/pinctrl-at91.c @@ -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; -- 1.9.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2] pinctrl/at91: Fix mask creation in at91_gpio_dbg_show 2014-04-14 18:53 ` [PATCH v2] " Alexander Stein @ 2014-04-16 7:55 ` Nicolas Ferre 2014-04-22 21:46 ` Linus Walleij 1 sibling, 0 replies; 10+ messages in thread From: Nicolas Ferre @ 2014-04-16 7:55 UTC (permalink / raw) To: linux-arm-kernel On 14/04/2014 20:53, 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] Maybe a little bit less context to the commit message: Linus might remove some lines to it... > Note that every bank despite bank 0 only shows "periph A" which are > obviously used as GPIOs. > > Signed-off-by: Alexander Stein <alexanders83@web.de> > Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> Okay, actually this patch have to be routed to Linus Walleij, I add him in the destination field. Thanks, > --- > 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(-) > > diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c > index 63176f2..2da0e6e 100644 > --- a/drivers/pinctrl/pinctrl-at91.c > +++ b/drivers/pinctrl/pinctrl-at91.c > @@ -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; > > -- Nicolas Ferre ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] pinctrl/at91: Fix mask creation in at91_gpio_dbg_show 2014-04-14 18:53 ` [PATCH v2] " Alexander Stein 2014-04-16 7:55 ` Nicolas Ferre @ 2014-04-22 21:46 ` Linus Walleij 1 sibling, 0 replies; 10+ messages in thread From: Linus Walleij @ 2014-04-22 21:46 UTC (permalink / raw) To: linux-arm-kernel On Mon, Apr 14, 2014 at 8:53 PM, Alexander Stein <alexanders83@web.de> wrote: > 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 <alexanders83@web.de> > Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> > --- > Changes in v2: > * Added 2 Acked-by's > * Add an example of wrong output Patch applied. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] pinctrl/at91: Fix mask creation in at91_gpio_dbg_show 2014-04-11 18:33 ` [PATCH] pinctrl/at91: Fix mask creation in at91_gpio_dbg_show Alexander Stein 2014-04-14 8:14 ` Alexandre Belloni @ 2014-04-14 8:19 ` Nicolas Ferre 1 sibling, 0 replies; 10+ messages in thread From: Nicolas Ferre @ 2014-04-14 8:19 UTC (permalink / raw) To: linux-arm-kernel On 11/04/2014 20:33, Alexander Stein : > pin_to_mask expects a bank pin number. So do not add the chip base. > > Signed-off-by: Alexander Stein <alexanders83@web.de> Indeed, seems to be a bug in this function: Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> Thanks a lot. Bye, > --- > 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(-) > > diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c > index 63176f2..2da0e6e 100644 > --- a/drivers/pinctrl/pinctrl-at91.c > +++ b/drivers/pinctrl/pinctrl-at91.c > @@ -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; > > -- Nicolas Ferre ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-04-22 21:46 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-04-11 15:35 [PATCH] pinctrl/at91: Fix pin_to_mask Alexander Stein 2014-04-11 15:42 ` Jean-Christophe PLAGNIOL-VILLARD 2014-04-11 15:45 ` Alexander Stein 2014-04-11 17:53 ` Mark Roszko 2014-04-11 18:33 ` [PATCH] pinctrl/at91: Fix mask creation in at91_gpio_dbg_show Alexander Stein 2014-04-14 8:14 ` Alexandre Belloni 2014-04-14 18:53 ` [PATCH v2] " Alexander Stein 2014-04-16 7:55 ` Nicolas Ferre 2014-04-22 21:46 ` Linus Walleij 2014-04-14 8:19 ` [PATCH] " Nicolas Ferre
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).