* [RFC v3 0/2] Request GPIO when enabling interrupt @ 2018-11-20 15:19 Fabrizio Castro 2018-11-20 15:19 ` [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable Fabrizio Castro 2018-11-20 15:19 ` [RFC v3 2/2] gpio: rcar: Set pin as a GPIO when configuring an interrupt Fabrizio Castro 0 siblings, 2 replies; 8+ messages in thread From: Fabrizio Castro @ 2018-11-20 15:19 UTC (permalink / raw) To: Linus Walleij, Geert Uytterhoeven, Bartosz Golaszewski Cc: Fabrizio Castro, linux-gpio, Simon Horman, Chris Paterson, Biju Das, linux-renesas-soc Dear All, here is a new iteration of the fix for the pinmuxing issue while requesting an interrupt. I don't like this implementation either as: * pinctrl_mux_gpio_request_enable is very similar to pinctrl_gpio_request, and the name I have picked up is not exactly brilliant... * it may cause an error message like "Pin X is busy, can't configure it as GPIO." (for cd-gpios pins for example) as it can't check the status of the pin before requesting it (can it?) * because it's discarding errors returned by pinctrl_mux_gpio_request_enable This problem needs fixing, but the solutions proposed so far don't look great, as they are not spectacularly neat. What's the best way to fix this? Ideas and comments very welcome! Thanks, Fab Fabrizio Castro (2): pinctrl: core: Add pinctrl_mux_gpio_request_enable gpio: rcar: Set pin as a GPIO when configuring an interrupt drivers/gpio/gpio-rcar.c | 3 +++ drivers/pinctrl/core.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/pinctrl/consumer.h | 6 ++++++ 3 files changed, 43 insertions(+) -- 2.7.4 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable 2018-11-20 15:19 [RFC v3 0/2] Request GPIO when enabling interrupt Fabrizio Castro @ 2018-11-20 15:19 ` Fabrizio Castro 2018-12-05 21:46 ` Linus Walleij 2018-11-20 15:19 ` [RFC v3 2/2] gpio: rcar: Set pin as a GPIO when configuring an interrupt Fabrizio Castro 1 sibling, 1 reply; 8+ messages in thread From: Fabrizio Castro @ 2018-11-20 15:19 UTC (permalink / raw) To: Linus Walleij, Geert Uytterhoeven, Bartosz Golaszewski Cc: Fabrizio Castro, linux-gpio, Simon Horman, Chris Paterson, Biju Das, linux-renesas-soc Sometimes there is the need to change the muxing of a pin to make it a GPIO without going through gpiolib. This patch adds pinctrl_mux_gpio_request_enable to deal with this new use case from code that has nothing to do with pinctrl. Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> --- drivers/pinctrl/core.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/pinctrl/consumer.h | 6 ++++++ 2 files changed, 40 insertions(+) diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index c6ff4d5..d5f4128 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -26,6 +26,7 @@ #include <linux/pinctrl/consumer.h> #include <linux/pinctrl/pinctrl.h> #include <linux/pinctrl/machine.h> +#include <linux/pinctrl/pinmux.h> #ifdef CONFIG_GPIOLIB #include <asm-generic/gpio.h> @@ -796,6 +797,39 @@ int pinctrl_gpio_request(unsigned gpio) EXPORT_SYMBOL_GPL(pinctrl_gpio_request); /** + * pinctrl_mux_gpio_request_enable() - request the pinmuxing of a single pin to + * be set as a GPIO. + * @gpio: the GPIO pin number from the GPIO subsystem number space + */ +int pinctrl_mux_gpio_request_enable(unsigned gpio) +{ + struct pinctrl_dev *pctldev; + struct pinctrl_gpio_range *range; + const struct pinmux_ops *ops; + int ret; + int pin; + + ret = pinctrl_get_device_gpio_range(gpio, &pctldev, &range); + if (ret) + return ret; + + ops = pctldev->desc->pmxops; + + mutex_lock(&pctldev->mutex); + + /* Convert to the pin controllers number space */ + pin = gpio_to_pin(range, gpio); + + if (ops->gpio_request_enable) + ret = ops->gpio_request_enable(pctldev, range, pin); + + mutex_unlock(&pctldev->mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(pinctrl_mux_gpio_request_enable); + +/** * pinctrl_gpio_free() - free control on a single pin, currently used as GPIO * @gpio: the GPIO pin number from the GPIO subsystem number space * diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h index 0412cc9..3fa32cf 100644 --- a/include/linux/pinctrl/consumer.h +++ b/include/linux/pinctrl/consumer.h @@ -26,6 +26,7 @@ struct device; /* External interface to pin control */ extern int pinctrl_gpio_request(unsigned gpio); +extern int pinctrl_mux_gpio_request_enable(unsigned gpio); extern void pinctrl_gpio_free(unsigned gpio); extern int pinctrl_gpio_direction_input(unsigned gpio); extern int pinctrl_gpio_direction_output(unsigned gpio); @@ -67,6 +68,11 @@ static inline int pinctrl_gpio_request(unsigned gpio) return 0; } +static inline int pinctrl_mux_gpio_request_enable(unsigned gpio) +{ + return 0; +} + static inline void pinctrl_gpio_free(unsigned gpio) { } -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable 2018-11-20 15:19 ` [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable Fabrizio Castro @ 2018-12-05 21:46 ` Linus Walleij 2018-12-06 9:47 ` Fabrizio Castro 0 siblings, 1 reply; 8+ messages in thread From: Linus Walleij @ 2018-12-05 21:46 UTC (permalink / raw) To: Fabrizio Castro Cc: Geert Uytterhoeven, Bartosz Golaszewski, open list:GPIO SUBSYSTEM, Simon Horman, Chris Paterson, Biju Das, Linux-Renesas On Tue, Nov 20, 2018 at 4:19 PM Fabrizio Castro <fabrizio.castro@bp.renesas.com> wrote: > Sometimes there is the need to change the muxing of a pin to make it > a GPIO without going through gpiolib. > This patch adds pinctrl_mux_gpio_request_enable to deal with this new > use case from code that has nothing to do with pinctrl. It has a lot to do with pinctrl I think, so I get confused by this commit message. > extern int pinctrl_gpio_request(unsigned gpio); > +extern int pinctrl_mux_gpio_request_enable(unsigned gpio); What's wrong with just using the existing call pinctrl_gpio_request() right above your new one? It's not like we're reference counting or something, it's just a callback. Sprinkle some comments to show what's going on. If you for some reason need a new call for this specific use case, it needs to be named after the use case, like pinctrl_gpio_request_for_irq() so it is obvious what the function is doing. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable 2018-12-05 21:46 ` Linus Walleij @ 2018-12-06 9:47 ` Fabrizio Castro 2019-03-01 13:27 ` Geert Uytterhoeven 0 siblings, 1 reply; 8+ messages in thread From: Fabrizio Castro @ 2018-12-06 9:47 UTC (permalink / raw) To: Linus Walleij Cc: Geert Uytterhoeven, Bartosz Golaszewski, open list:GPIO SUBSYSTEM, Simon Horman, Chris Paterson, Biju Das, Linux-Renesas Hello Linus, Thank you for your feedback! > From: Linus Walleij <linus.walleij@linaro.org> > Sent: 05 December 2018 21:46 > Subject: Re: [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable > > On Tue, Nov 20, 2018 at 4:19 PM Fabrizio Castro > <fabrizio.castro@bp.renesas.com> wrote: > > > Sometimes there is the need to change the muxing of a pin to make it > > a GPIO without going through gpiolib. > > This patch adds pinctrl_mux_gpio_request_enable to deal with this new > > use case from code that has nothing to do with pinctrl. > > It has a lot to do with pinctrl I think, so I get confused by this > commit message. I can improve that > > > extern int pinctrl_gpio_request(unsigned gpio); > > +extern int pinctrl_mux_gpio_request_enable(unsigned gpio); > > What's wrong with just using the existing call > pinctrl_gpio_request() right above your new one? > > It's not like we're reference counting or something, it's just > a callback. Sprinkle some comments to show what's going > on. I tried that, and it was working for me, then something changed lately in gpiolib that broke that solution, and Geert picked it up on his end. Please see this: https://patchwork.kernel.org/patch/10671325/ This patch was made to overcome the problems of the previous patch. > > If you for some reason need a new call for this specific > use case, it needs to be named after the use case, > like pinctrl_gpio_request_for_irq() > so it is obvious what the function is doing. I can do that, but I would like to hear from Geert first, no point in going around in circle if this solution is not acceptable to him. Geert, what do you think? Thanks! Fab > > Yours, > Linus Walleij [https://www2.renesas.eu/media/email/unicef.jpg] This Christmas, instead of sending out cards, Renesas Electronics Europe have decided to support Unicef with a donation. For further details click here<https://www.unicef.org/> to find out about the valuable work they do, helping children all over the world. We would like to take this opportunity to wish you a Merry Christmas and a prosperous New Year. Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable 2018-12-06 9:47 ` Fabrizio Castro @ 2019-03-01 13:27 ` Geert Uytterhoeven 2019-03-01 14:22 ` Fabrizio Castro 2019-03-08 8:24 ` Linus Walleij 0 siblings, 2 replies; 8+ messages in thread From: Geert Uytterhoeven @ 2019-03-01 13:27 UTC (permalink / raw) To: Fabrizio Castro Cc: Linus Walleij, Geert Uytterhoeven, Bartosz Golaszewski, open list:GPIO SUBSYSTEM, Simon Horman, Chris Paterson, Biju Das, Linux-Renesas Hi Fabrizio, On Thu, Dec 6, 2018 at 10:47 AM Fabrizio Castro <fabrizio.castro@bp.renesas.com> wrote: > > From: Linus Walleij <linus.walleij@linaro.org> > > Sent: 05 December 2018 21:46 > > Subject: Re: [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable > > > > On Tue, Nov 20, 2018 at 4:19 PM Fabrizio Castro > > <fabrizio.castro@bp.renesas.com> wrote: > > > > > Sometimes there is the need to change the muxing of a pin to make it > > > a GPIO without going through gpiolib. > > > This patch adds pinctrl_mux_gpio_request_enable to deal with this new > > > use case from code that has nothing to do with pinctrl. > > > > It has a lot to do with pinctrl I think, so I get confused by this > > commit message. > > I can improve that > > > > > > extern int pinctrl_gpio_request(unsigned gpio); > > > +extern int pinctrl_mux_gpio_request_enable(unsigned gpio); > > > > What's wrong with just using the existing call > > pinctrl_gpio_request() right above your new one? > > > > It's not like we're reference counting or something, it's just > > a callback. Sprinkle some comments to show what's going > > on. > > I tried that, and it was working for me, then something changed lately > in gpiolib that broke that solution, and Geert picked it up on his end. > Please see this: > https://patchwork.kernel.org/patch/10671325/ > > This patch was made to overcome the problems of the previous patch. > > > > > If you for some reason need a new call for this specific > > use case, it needs to be named after the use case, > > like pinctrl_gpio_request_for_irq() > > so it is obvious what the function is doing. > > I can do that, but I would like to hear from Geert first, no point in going > around in circle if this solution is not acceptable to him. > > Geert, what do you think? /me gives v3 a try on Koelsch, Salvator-XS, and Ebisu-4D: BUG: sleeping function called from invalid context for mmc, adv7511, gpio-keys, and Ethernet PHY. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable 2019-03-01 13:27 ` Geert Uytterhoeven @ 2019-03-01 14:22 ` Fabrizio Castro 2019-03-08 8:24 ` Linus Walleij 1 sibling, 0 replies; 8+ messages in thread From: Fabrizio Castro @ 2019-03-01 14:22 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Linus Walleij, Geert Uytterhoeven, Bartosz Golaszewski, open list:GPIO SUBSYSTEM, Simon Horman, Chris Paterson, Biju Das, Linux-Renesas Hello Geert, Thank you for your feedback! > From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Geert Uytterhoeven > Sent: 01 March 2019 13:28 > Subject: Re: [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable > > Hi Fabrizio, > > On Thu, Dec 6, 2018 at 10:47 AM Fabrizio Castro > <fabrizio.castro@bp.renesas.com> wrote: > > > From: Linus Walleij <linus.walleij@linaro.org> > > > Sent: 05 December 2018 21:46 > > > Subject: Re: [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable > > > > > > On Tue, Nov 20, 2018 at 4:19 PM Fabrizio Castro > > > <fabrizio.castro@bp.renesas.com> wrote: > > > > > > > Sometimes there is the need to change the muxing of a pin to make it > > > > a GPIO without going through gpiolib. > > > > This patch adds pinctrl_mux_gpio_request_enable to deal with this new > > > > use case from code that has nothing to do with pinctrl. > > > > > > It has a lot to do with pinctrl I think, so I get confused by this > > > commit message. > > > > I can improve that > > > > > > > > > extern int pinctrl_gpio_request(unsigned gpio); > > > > +extern int pinctrl_mux_gpio_request_enable(unsigned gpio); > > > > > > What's wrong with just using the existing call > > > pinctrl_gpio_request() right above your new one? > > > > > > It's not like we're reference counting or something, it's just > > > a callback. Sprinkle some comments to show what's going > > > on. > > > > I tried that, and it was working for me, then something changed lately > > in gpiolib that broke that solution, and Geert picked it up on his end. > > Please see this: > > https://patchwork.kernel.org/patch/10671325/ > > > > This patch was made to overcome the problems of the previous patch. > > > > > > > > If you for some reason need a new call for this specific > > > use case, it needs to be named after the use case, > > > like pinctrl_gpio_request_for_irq() > > > so it is obvious what the function is doing. > > > > I can do that, but I would like to hear from Geert first, no point in going > > around in circle if this solution is not acceptable to him. > > > > Geert, what do you think? > > /me gives v3 a try on Koelsch, Salvator-XS, and Ebisu-4D: > > BUG: sleeping function called from invalid context > > for mmc, adv7511, gpio-keys, and Ethernet PHY. Doh! It was running smoothly on my iwg23s when I tested it, I am going to have another look at this at a later stage. Thank you for looking into this! Cheers, Fab > > Gr{oetje,eeting}s, > > Geert > > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable 2019-03-01 13:27 ` Geert Uytterhoeven 2019-03-01 14:22 ` Fabrizio Castro @ 2019-03-08 8:24 ` Linus Walleij 1 sibling, 0 replies; 8+ messages in thread From: Linus Walleij @ 2019-03-08 8:24 UTC (permalink / raw) To: Geert Uytterhoeven, Marc Zyngier, Thomas Gleixner Cc: Fabrizio Castro, Geert Uytterhoeven, Bartosz Golaszewski, open list:GPIO SUBSYSTEM, Simon Horman, Chris Paterson, Biju Das, Linux-Renesas On Fri, Mar 1, 2019 at 2:27 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > /me gives v3 a try on Koelsch, Salvator-XS, and Ebisu-4D: > > BUG: sleeping function called from invalid context > > for mmc, adv7511, gpio-keys, and Ethernet PHY. This is the usual problem when you call back from any of the irqchip callbacks: almost all of them except request/release resources are called under a spinlock. The problem is creeping up in a lot of places, and I can't really solve that from the GPIO side. See for example this regression that I have no idea what to do with: https://marc.info/?l=linux-kernel&m=154349829407463&w=2 Yours, Linus Walleij ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC v3 2/2] gpio: rcar: Set pin as a GPIO when configuring an interrupt 2018-11-20 15:19 [RFC v3 0/2] Request GPIO when enabling interrupt Fabrizio Castro 2018-11-20 15:19 ` [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable Fabrizio Castro @ 2018-11-20 15:19 ` Fabrizio Castro 1 sibling, 0 replies; 8+ messages in thread From: Fabrizio Castro @ 2018-11-20 15:19 UTC (permalink / raw) To: Linus Walleij, Geert Uytterhoeven, Bartosz Golaszewski Cc: Fabrizio Castro, linux-gpio, Simon Horman, Chris Paterson, Biju Das, linux-renesas-soc As it turns out, the bootloader or a POR may get in the way of the current implementation of gpio_rcar_irq_set_type, as the pinmuxing may not be GPIO. This patch makes sure the pin is configured as a GPIO when requesting it an interrupt, as that's necessary for the interrupt to work properly. Failing to do so may damage the board as this can cause shorts. Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> --- drivers/gpio/gpio-rcar.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c index 1322f7e..615404c 100644 --- a/drivers/gpio/gpio-rcar.c +++ b/drivers/gpio/gpio-rcar.c @@ -168,6 +168,9 @@ static int gpio_rcar_irq_set_type(struct irq_data *d, unsigned int type) default: return -EINVAL; } + + pinctrl_mux_gpio_request_enable(gc->base + hwirq); + return 0; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-03-08 8:24 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-20 15:19 [RFC v3 0/2] Request GPIO when enabling interrupt Fabrizio Castro 2018-11-20 15:19 ` [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable Fabrizio Castro 2018-12-05 21:46 ` Linus Walleij 2018-12-06 9:47 ` Fabrizio Castro 2019-03-01 13:27 ` Geert Uytterhoeven 2019-03-01 14:22 ` Fabrizio Castro 2019-03-08 8:24 ` Linus Walleij 2018-11-20 15:19 ` [RFC v3 2/2] gpio: rcar: Set pin as a GPIO when configuring an interrupt Fabrizio Castro
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.