| Message ID | 1542727156-31432-2-git-send-email-fabrizio.castro@bp.renesas.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Request GPIO when enabling interrupt | expand |
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
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.
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
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
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
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) { }
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(+)