| Message ID | 20191004122923.22674-1-amelie.delaunay@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] pinctrl: stmfx: add irq_request/release_resources callbacks | expand |
On Fri, Oct 4, 2019 at 2:29 PM Amelie Delaunay <amelie.delaunay@st.com> wrote: > When an STMFX IO is used as interrupt through the interrupt-controller > binding, the STMFX driver should configure this IO as input. Default > value of STMFX IO direction is input, but if the IO is used as output > before the interrupt use, it will not work without these callbacks. > > Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com> OK I see what you want to achieve. > +static int stmfx_gpio_irq_request_resources(struct irq_data *data) > +{ > + struct gpio_chip *gpio_chip = irq_data_get_irq_chip_data(data); > + struct stmfx_pinctrl *pctl = gpiochip_get_data(gpio_chip); > + int ret; > + > + ret = stmfx_gpio_direction_input(&pctl->gpio_chip, data->hwirq); > + if (ret) > + return ret; > + > + ret = gpiochip_lock_as_irq(&pctl->gpio_chip, data->hwirq); > + if (ret) { > + dev_err(pctl->dev, "Unable to lock gpio %lu as IRQ: %d\n", > + data->hwirq, ret); > + return ret; > + } > + > + return 0; > +} Just call gpiochip_reqres_irq() instead of calling the lock etc explicitly. > +static void stmfx_gpio_irq_release_resources(struct irq_data *data) > +{ > + struct gpio_chip *gpio_chip = irq_data_get_irq_chip_data(data); > + struct stmfx_pinctrl *pctl = gpiochip_get_data(gpio_chip); > + > + gpiochip_unlock_as_irq(&pctl->gpio_chip, data->hwirq); > +} Just call gpiochip_relres_irq() But all this duplicated a lot of code from the core which is not so nice. > @@ -678,6 +706,8 @@ static int stmfx_pinctrl_probe(struct platform_device *pdev) > pctl->irq_chip.irq_set_type = stmfx_pinctrl_irq_set_type; > pctl->irq_chip.irq_bus_lock = stmfx_pinctrl_irq_bus_lock; > pctl->irq_chip.irq_bus_sync_unlock = stmfx_pinctrl_irq_bus_sync_unlock; > + pctl->irq_chip.irq_request_resources = stmfx_gpio_irq_request_resources; > + pctl->irq_chip.irq_release_resources = stmfx_gpio_irq_release_resources; What about just adding pctl->irq_chip.irq_enable and do stmfx_gpio_direction_input() in that callback instead? gpiolib will helpfully wrap it. Yours, Linus Walleij
Hi Linus, On 10/5/19 6:49 PM, Linus Walleij wrote: > On Fri, Oct 4, 2019 at 2:29 PM Amelie Delaunay <amelie.delaunay@st.com> > wrote: > >> When an STMFX IO is used as interrupt through the interrupt-controller >> binding, the STMFX driver should configure this IO as input. Default >> value of STMFX IO direction is input, but if the IO is used as output >> before the interrupt use, it will not work without these callbacks. >> >> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com> > > OK I see what you want to achieve. > >> +static int stmfx_gpio_irq_request_resources(struct irq_data *data) >> +{ >> + struct gpio_chip *gpio_chip = irq_data_get_irq_chip_data(data); >> + struct stmfx_pinctrl *pctl = gpiochip_get_data(gpio_chip); >> + int ret; >> + >> + ret = stmfx_gpio_direction_input(&pctl->gpio_chip, data->hwirq); >> + if (ret) >> + return ret; >> + >> + ret = gpiochip_lock_as_irq(&pctl->gpio_chip, data->hwirq); >> + if (ret) { >> + dev_err(pctl->dev, "Unable to lock gpio %lu as IRQ: %d\n", >> + data->hwirq, ret); >> + return ret; >> + } >> + >> + return 0; >> +} > > Just call gpiochip_reqres_irq() instead of calling the lock etc > explicitly. > >> +static void stmfx_gpio_irq_release_resources(struct irq_data *data) >> +{ >> + struct gpio_chip *gpio_chip = irq_data_get_irq_chip_data(data); >> + struct stmfx_pinctrl *pctl = gpiochip_get_data(gpio_chip); >> + >> + gpiochip_unlock_as_irq(&pctl->gpio_chip, data->hwirq); >> +} > > Just call gpiochip_relres_irq() > > But all this duplicated a lot of code from the core which is not so nice. > >> @@ -678,6 +706,8 @@ static int stmfx_pinctrl_probe(struct platform_device *pdev) >> pctl->irq_chip.irq_set_type = stmfx_pinctrl_irq_set_type; >> pctl->irq_chip.irq_bus_lock = stmfx_pinctrl_irq_bus_lock; >> pctl->irq_chip.irq_bus_sync_unlock = stmfx_pinctrl_irq_bus_sync_unlock; >> + pctl->irq_chip.irq_request_resources = stmfx_gpio_irq_request_resources; >> + pctl->irq_chip.irq_release_resources = stmfx_gpio_irq_release_resources; > > What about just adding > > pctl->irq_chip.irq_enable and do stmfx_gpio_direction_input() > in that callback instead? gpiolib will helpfully wrap it. Thanks for pointing that out to me. I can't use .irq_enable because of I2C transfer to set gpio direction (scheduling while atomic BUG occurs in this case). Indeed, .irq_enable is called under raw_spin_lock_irqsave in __setup_irq() while irq_request_resources is called before. I could apply gpio direction in stmfx_pinctrl_irq_bus_sync_unlock depending on pctl->irq_gpi_src[] (checking which one is set, to set input direction), but this will be applied each time a consumer requests a stmfx gpio irq. IMHO, keeping .irq_request/release_resources callbacks and using gpiochip_reqres_irq()/gpiochip_relres_irq() seems to be the best compromise. Regards, Amelie
On Mon, Oct 7, 2019 at 4:53 PM Amelie DELAUNAY <amelie.delaunay@st.com> wrote: > On 10/5/19 6:49 PM, Linus Walleij wrote: > > On Fri, Oct 4, 2019 at 2:29 PM Amelie Delaunay <amelie.delaunay@st.com> > >> + pctl->irq_chip.irq_request_resources = stmfx_gpio_irq_request_resources; > >> + pctl->irq_chip.irq_release_resources = stmfx_gpio_irq_release_resources; > > > > What about just adding > > > > pctl->irq_chip.irq_enable and do stmfx_gpio_direction_input() > > in that callback instead? gpiolib will helpfully wrap it. > > Thanks for pointing that out to me. > > I can't use .irq_enable because of I2C transfer to set gpio direction > (scheduling while atomic BUG occurs in this case). Indeed, .irq_enable > is called under raw_spin_lock_irqsave in __setup_irq() while > irq_request_resources is called before. > > I could apply gpio direction in stmfx_pinctrl_irq_bus_sync_unlock > depending on pctl->irq_gpi_src[] (checking which one is set, to set > input direction), but this will be applied each time a consumer requests > a stmfx gpio irq. Oh I get it, hm. I thought it would be covered by the sync_unlock() but I guess not then. > IMHO, keeping .irq_request/release_resources callbacks and using > gpiochip_reqres_irq()/gpiochip_relres_irq() seems to be the best compromise. OK let's go with that for now, please put in some comments as to why this gets done there so we know when reading the code. Yours, Linus Walleij
diff --git a/drivers/pinctrl/pinctrl-stmfx.c b/drivers/pinctrl/pinctrl-stmfx.c index 564660028fcc..e3a3dcc145b4 100644 --- a/drivers/pinctrl/pinctrl-stmfx.c +++ b/drivers/pinctrl/pinctrl-stmfx.c @@ -505,6 +505,34 @@ static void stmfx_pinctrl_irq_bus_sync_unlock(struct irq_data *data) mutex_unlock(&pctl->lock); } +static int stmfx_gpio_irq_request_resources(struct irq_data *data) +{ + struct gpio_chip *gpio_chip = irq_data_get_irq_chip_data(data); + struct stmfx_pinctrl *pctl = gpiochip_get_data(gpio_chip); + int ret; + + ret = stmfx_gpio_direction_input(&pctl->gpio_chip, data->hwirq); + if (ret) + return ret; + + ret = gpiochip_lock_as_irq(&pctl->gpio_chip, data->hwirq); + if (ret) { + dev_err(pctl->dev, "Unable to lock gpio %lu as IRQ: %d\n", + data->hwirq, ret); + return ret; + } + + return 0; +} + +static void stmfx_gpio_irq_release_resources(struct irq_data *data) +{ + struct gpio_chip *gpio_chip = irq_data_get_irq_chip_data(data); + struct stmfx_pinctrl *pctl = gpiochip_get_data(gpio_chip); + + gpiochip_unlock_as_irq(&pctl->gpio_chip, data->hwirq); +} + static void stmfx_pinctrl_irq_toggle_trigger(struct stmfx_pinctrl *pctl, unsigned int offset) { @@ -678,6 +706,8 @@ static int stmfx_pinctrl_probe(struct platform_device *pdev) pctl->irq_chip.irq_set_type = stmfx_pinctrl_irq_set_type; pctl->irq_chip.irq_bus_lock = stmfx_pinctrl_irq_bus_lock; pctl->irq_chip.irq_bus_sync_unlock = stmfx_pinctrl_irq_bus_sync_unlock; + pctl->irq_chip.irq_request_resources = stmfx_gpio_irq_request_resources; + pctl->irq_chip.irq_release_resources = stmfx_gpio_irq_release_resources; ret = gpiochip_irqchip_add_nested(&pctl->gpio_chip, &pctl->irq_chip, 0, handle_bad_irq, IRQ_TYPE_NONE);
When an STMFX IO is used as interrupt through the interrupt-controller binding, the STMFX driver should configure this IO as input. Default value of STMFX IO direction is input, but if the IO is used as output before the interrupt use, it will not work without these callbacks. Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com> --- drivers/pinctrl/pinctrl-stmfx.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)