| Message ID | 1540583850-30825-1-git-send-email-fabrizio.castro@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [RFC] gpio: rcar: Request GPIO before enabling interrupt | expand |
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
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.
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
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
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.
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);
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(+)