[RFC v3 0/2] Request GPIO when enabling interrupt

[RFC v3 0/2] Request GPIO when enabling interrupt

[Fabrizio Castro]

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

[RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable

[Fabrizio Castro]

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

[RFC v3 2/2] gpio: rcar: Set pin as a GPIO when configuring an interrupt

[Fabrizio Castro]

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

Re: [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable

[Linus Walleij]

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

RE: [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable

[Fabrizio Castro]

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.

Re: [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable

[Geert Uytterhoeven]

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

RE: [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable

[Fabrizio Castro]

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

Re: [RFC v3 1/2] pinctrl: core: Add pinctrl_mux_gpio_request_enable

[Linus Walleij]

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