[RFC,v3,1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable
diff mbox series

Message ID 1542727156-31432-2-git-send-email-fabrizio.castro@bp.renesas.com
State Under Review
Delegated to: Geert Uytterhoeven
Headers show
Series
  • Request GPIO when enabling interrupt
Related show

Commit Message

Fabrizio Castro Nov. 20, 2018, 3:19 p.m. UTC
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(+)

Comments

Linus Walleij Dec. 5, 2018, 9:46 p.m. UTC | #1
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
Fabrizio Castro Dec. 6, 2018, 9:47 a.m. UTC | #2
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.
Geert Uytterhoeven March 1, 2019, 1:27 p.m. UTC | #3
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
Fabrizio Castro March 1, 2019, 2:22 p.m. UTC | #4
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
Linus Walleij March 8, 2019, 8:24 a.m. UTC | #5
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

Patch
diff mbox series

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)
 {
 }