[PATCH 1/1] pinctrl: stmfx: add irq_request/release_resources callbacks

[PATCH 1/1] pinctrl: stmfx: add irq_request/release_resources callbacks

[Amelie Delaunay]

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(+)

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);
-- 
2.17.1

Re: [PATCH 1/1] pinctrl: stmfx: add irq_request/release_resources callbacks

[Linus Walleij]

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

Re: [PATCH 1/1] pinctrl: stmfx: add irq_request/release_resources callbacks

[Amelie DELAUNAY]

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

Re: [PATCH 1/1] pinctrl: stmfx: add irq_request/release_resources callbacks

[Linus Walleij]

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