[RFC] gpio: rcar: Request GPIO before enabling interrupt

[RFC] gpio: rcar: Request GPIO before enabling interrupt

[Fabrizio Castro]

There are cases when the bootloader configures a pin to work
as a function rather than GPIO, and other cases when the pin
is configured as a function at POR.
This commit makes sure the pin is configured as a GPIO the
moment we need it to work as an interrupt.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---

Dear All,

we have got some issues while trying to bring up the interrupt
line of the HDMI transmitter on the iwg23s, as GP2_29 is configured
as a function when the kernel starts, and therefore setting up the
interrupt from the driver won't have the desired effect.
This patch makes sure the pinctrl driver sets GP2[29] to GP-2-29
before enabling the interrupt, but it doesn't addresses the
"direction problem", as in the pin will be a GPIO after calling
gc->request, but the direction would be whatever was previously
configured. There could be the option of moving the additional
code added by this patch to the bottom of function
gpio_rcar_irq_set_type, but is that going to behave properly on
every SoC this driver is supporting?
Is configuring every gpio with direction input while probing
something that should be looked into to reduce concerns over
switching from function to GPIO?

Comments very welcome!

Thanks,
Fab

 drivers/gpio/gpio-rcar.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
index 3c82bb3..8c69431 100644
--- a/drivers/gpio/gpio-rcar.c
+++ b/drivers/gpio/gpio-rcar.c
@@ -147,6 +147,14 @@ static int gpio_rcar_irq_set_type(struct irq_data *d, unsigned int type)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct gpio_rcar_priv *p = gpiochip_get_data(gc);
 	unsigned int hwirq = irqd_to_hwirq(d);
+	int err;
+
+	err = gc->request(gc, hwirq);
+	if (err) {
+		dev_err(&p->pdev->dev, "Can't request GPIO %d from %s\n", hwirq,
+			gc->label);
+		return err;
+	}
 
 	dev_dbg(&p->pdev->dev, "sense irq = %d, type = %d\n", hwirq, type);
 
-- 
2.7.4

Re: [RFC] gpio: rcar: Request GPIO before enabling interrupt

[Linus Walleij]

On Fri, Oct 26, 2018 at 9:57 PM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:

> There are cases when the bootloader configures a pin to work
> as a function rather than GPIO, and other cases when the pin
> is configured as a function at POR.
> This commit makes sure the pin is configured as a GPIO the
> moment we need it to work as an interrupt.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
(...)
> +       err = gc->request(gc, hwirq);

This is done in some drivers when what you want is exactly
the work carried out by that callback. But can't you just call
gpio_rcar_request() directly in this case so it is clear that
the driver is meddling with the internal state of the hardware
and not really intending to loop out into the external
API or external callbacks?

You're not on one of these platforms that prefer setting up
the pin as GPIO using a pin control hog in the device tree?

Sadly there is sometimes more than one way to do things
around here :/

Geert will know what is best.

Yours,
Linus Walleij

RE: [RFC] gpio: rcar: Request GPIO before enabling interrupt

[Fabrizio Castro]

Hello Linus,

Thank you for your feedback!

I am sorry for the delay of my answer, I was hoping others
would jump in the discussion as well.

> > +       err = gc->request(gc, hwirq);
>
> This is done in some drivers when what you want is exactly
> the work carried out by that callback. But can't you just call
> gpio_rcar_request() directly in this case so it is clear that
> the driver is meddling with the internal state of the hardware
> and not really intending to loop out into the external
> API or external callbacks?

gpio_rcar_request is static unfortunately, maybe I should
export the symbol?

>
> You're not on one of these platforms that prefer setting up
> the pin as GPIO using a pin control hog in the device tree?

My personal preference would be to deal with this from
within irqchip, as when you hook up a gpio as interrupt
from the DT the kernel should do everything that's necessary
to make it happen, but that is just a personal opinion.
Anyway, I did give gpio-hog a try and it works for me.

>
> Sadly there is sometimes more than one way to do things
> around here :/

so true

>
> Geert will know what is best.

Yeah, I am really keen in hearing from him about this, in the meantime
I went through a bunch of manuals, and moving the gpio request to the
bottom of gpio_rcar_irq_set_type seems to be okay for RCar devices in
general, but Geert knows definitely better.
I'll send this other option out as a patch this time, hoping to get more
feedbacks about the topic.

Again, thank you.

Fab

>
> Yours,
> Linus Walleij



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [RFC] gpio: rcar: Request GPIO before enabling interrupt

[Geert Uytterhoeven]

Hi Fabrizio,

On Fri, Oct 26, 2018 at 9:57 PM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
> There are cases when the bootloader configures a pin to work
> as a function rather than GPIO, and other cases when the pin
> is configured as a function at POR.
> This commit makes sure the pin is configured as a GPIO the
> moment we need it to work as an interrupt.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> ---
>
> Dear All,
>
> we have got some issues while trying to bring up the interrupt
> line of the HDMI transmitter on the iwg23s, as GP2_29 is configured
> as a function when the kernel starts, and therefore setting up the
> interrupt from the driver won't have the desired effect.
> This patch makes sure the pinctrl driver sets GP2[29] to GP-2-29
> before enabling the interrupt, but it doesn't addresses the
> "direction problem", as in the pin will be a GPIO after calling
> gc->request, but the direction would be whatever was previously
> configured. There could be the option of moving the additional
> code added by this patch to the bottom of function
> gpio_rcar_irq_set_type, but is that going to behave properly on
> every SoC this driver is supporting?
> Is configuring every gpio with direction input while probing
> something that should be looked into to reduce concerns over
> switching from function to GPIO?

Configuring everything to input may cause issues where a GPIO is connected
to the reset line of an external device, and where the bootloader configured the
line to deassert the reset.

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] gpio: rcar: Request GPIO before enabling interrupt

[Geert Uytterhoeven]

Hi Fabrizio,

On Fri, Nov 2, 2018 at 8:00 PM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
> I am sorry for the delay of my answer, I was hoping others
> would jump in the discussion as well.
>
> > > +       err = gc->request(gc, hwirq);
> >
> > This is done in some drivers when what you want is exactly
> > the work carried out by that callback. But can't you just call
> > gpio_rcar_request() directly in this case so it is clear that
> > the driver is meddling with the internal state of the hardware
> > and not really intending to loop out into the external
> > API or external callbacks?
>
> gpio_rcar_request is static unfortunately, maybe I should
> export the symbol?

I think Linus meant calling gpio_rcar_request() instead of gc->request()
from gpio_rcar_irq_set_type(), i.e. from the GPIO driver, not from the
HDMI driver. So static is fine.

> > You're not on one of these platforms that prefer setting up
> > the pin as GPIO using a pin control hog in the device tree?
>
> My personal preference would be to deal with this from
> within irqchip, as when you hook up a gpio as interrupt
> from the DT the kernel should do everything that's necessary
> to make it happen, but that is just a personal opinion.
> Anyway, I did give gpio-hog a try and it works for me.

Isn't the purpose of an input GPIO hog to configure the GPIO to not
drive the line, in the absence of any other driver with a need to read
the line value?

In this case there is another driver (via the interrupt subsystem), so using
an input GPIO doesn't look like the real solution to me.

> > Geert will know what is best.

People put way too much faith in me ;-)

> Yeah, I am really keen in hearing from him about this, in the meantime
> I went through a bunch of manuals, and moving the gpio request to the
> bottom of gpio_rcar_irq_set_type seems to be okay for RCar devices in
> general, but Geert knows definitely better.

Moving it down, after all other checks, is indeed better.

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] gpio: rcar: Request GPIO before enabling interrupt

[Fabrizio Castro]

Hello Geert,

Thank you for your feedback!

> Subject: Re: [RFC] gpio: rcar: Request GPIO before enabling interrupt
>
> Hi Fabrizio,
>
> On Fri, Oct 26, 2018 at 9:57 PM Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:
> > There are cases when the bootloader configures a pin to work
> > as a function rather than GPIO, and other cases when the pin
> > is configured as a function at POR.
> > This commit makes sure the pin is configured as a GPIO the
> > moment we need it to work as an interrupt.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > ---
> >
> > Dear All,
> >
> > we have got some issues while trying to bring up the interrupt
> > line of the HDMI transmitter on the iwg23s, as GP2_29 is configured
> > as a function when the kernel starts, and therefore setting up the
> > interrupt from the driver won't have the desired effect.
> > This patch makes sure the pinctrl driver sets GP2[29] to GP-2-29
> > before enabling the interrupt, but it doesn't addresses the
> > "direction problem", as in the pin will be a GPIO after calling
> > gc->request, but the direction would be whatever was previously
> > configured. There could be the option of moving the additional
> > code added by this patch to the bottom of function
> > gpio_rcar_irq_set_type, but is that going to behave properly on
> > every SoC this driver is supporting?
> > Is configuring every gpio with direction input while probing
> > something that should be looked into to reduce concerns over
> > switching from function to GPIO?
>
> Configuring everything to input may cause issues where a GPIO is connected
> to the reset line of an external device, and where the bootloader configured the
> line to deassert the reset.

I am going to stay away from this then.

Thanks,
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



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.