[PATCH] pinctrl: intel: Pass irqchip when adding gpiochip

[PATCH] pinctrl: intel: Pass irqchip when adding gpiochip

[Linus Walleij]

We need to convert all old gpio irqchips to pass the irqchip
setup along when adding the gpio_chip. For more info see
drivers/gpio/TODO.

This is an unchained irqchip so we use the method from
drivers/gpio/gpio-mt7621.c that also requests its interrupt
instead if chaining the interrupt handler.

Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
This is based on Andy's patch to dynamically allocate the
irqchio. Mika maybe you want to queue it with the rest of
the stuff so you can sync fixes and new development?
---
 drivers/pinctrl/intel/pinctrl-intel.c | 52 ++++++++++++++++-----------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index bc013599a9a3..d63566e57f4c 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1206,6 +1206,37 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
 	pctrl->irqchip.irq_set_wake = intel_gpio_irq_wake;
 	pctrl->irqchip.flags = IRQCHIP_MASK_ON_SUSPEND;
 
+	/*
+	 * We need to request the interrupt here (instead of using a
+	 * chained handler) because on some platforms several GPIO
+	 * controllers share the same interrupt line.
+	 */
+	ret = devm_request_irq(pctrl->dev, irq, intel_gpio_irq,
+			       IRQF_SHARED | IRQF_NO_THREAD,
+			       dev_name(pctrl->dev), pctrl);
+	if (!ret) {
+		struct gpio_irq_chip *girq;
+
+		girq = &pctrl->chip.irq;
+		girq->chip = &pctrl->irqchip;
+		/*
+		 * This is an unchained interrupt. Compare to
+		 * drivers/gpio/gpio-mt7621.c that also does this:
+		 * assign no parents.
+		 *
+		 * FIXME: make the gpiolib flag this and handle unchained
+		 * GPIO interrupts better if need be.
+		 */
+		girq->parent_handler = NULL;
+		girq->num_parents = 0;
+		girq->parents = NULL;
+		girq->default_type = IRQ_TYPE_NONE;
+		girq->handler = handle_bad_irq;
+	} else {
+		/* Skip irqchip, register gpiochip anyway */
+		dev_err(pctrl->dev, "failed to request interrupt\n");
+	}
+
 	ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
 	if (ret) {
 		dev_err(pctrl->dev, "failed to register gpiochip\n");
@@ -1222,27 +1253,6 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
 		}
 	}
 
-	/*
-	 * We need to request the interrupt here (instead of providing chip
-	 * to the irq directly) because on some platforms several GPIO
-	 * controllers share the same interrupt line.
-	 */
-	ret = devm_request_irq(pctrl->dev, irq, intel_gpio_irq,
-			       IRQF_SHARED | IRQF_NO_THREAD,
-			       dev_name(pctrl->dev), pctrl);
-	if (ret) {
-		dev_err(pctrl->dev, "failed to request interrupt\n");
-		return ret;
-	}
-
-	ret = gpiochip_irqchip_add(&pctrl->chip, &pctrl->irqchip, 0,
-				   handle_bad_irq, IRQ_TYPE_NONE);
-	if (ret) {
-		dev_err(pctrl->dev, "failed to add irqchip\n");
-		return ret;
-	}
-
-	gpiochip_set_chained_irqchip(&pctrl->chip, &pctrl->irqchip, irq, NULL);
 	return 0;
 }
 
-- 
2.21.0

Re: [PATCH] pinctrl: intel: Pass irqchip when adding gpiochip

[Andy Shevchenko]

On Sun, Oct 06, 2019 at 02:59:49AM +0200, Linus Walleij wrote:
> We need to convert all old gpio irqchips to pass the irqchip
> setup along when adding the gpio_chip. For more info see
> drivers/gpio/TODO.
> 
> This is an unchained irqchip so we use the method from
> drivers/gpio/gpio-mt7621.c that also requests its interrupt
> instead if chaining the interrupt handler.

One comment below, after addressing,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> This is based on Andy's patch to dynamically allocate the
> irqchio. Mika maybe you want to queue it with the rest of
> the stuff so you can sync fixes and new development?

My Rb tag Mika may use for fixes stuff in this cycle.

> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 52 ++++++++++++++++-----------
>  1 file changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index bc013599a9a3..d63566e57f4c 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -1206,6 +1206,37 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
>  	pctrl->irqchip.irq_set_wake = intel_gpio_irq_wake;
>  	pctrl->irqchip.flags = IRQCHIP_MASK_ON_SUSPEND;
>  
> +	/*
> +	 * We need to request the interrupt here (instead of using a
> +	 * chained handler) because on some platforms several GPIO
> +	 * controllers share the same interrupt line.
> +	 */
> +	ret = devm_request_irq(pctrl->dev, irq, intel_gpio_irq,
> +			       IRQF_SHARED | IRQF_NO_THREAD,
> +			       dev_name(pctrl->dev), pctrl);

> +	if (!ret) {

Why not positive condition?

> +		struct gpio_irq_chip *girq;
> +
> +		girq = &pctrl->chip.irq;
> +		girq->chip = &pctrl->irqchip;
> +		/*
> +		 * This is an unchained interrupt. Compare to
> +		 * drivers/gpio/gpio-mt7621.c that also does this:
> +		 * assign no parents.
> +		 *
> +		 * FIXME: make the gpiolib flag this and handle unchained
> +		 * GPIO interrupts better if need be.
> +		 */
> +		girq->parent_handler = NULL;
> +		girq->num_parents = 0;
> +		girq->parents = NULL;
> +		girq->default_type = IRQ_TYPE_NONE;
> +		girq->handler = handle_bad_irq;
> +	} else {
> +		/* Skip irqchip, register gpiochip anyway */
> +		dev_err(pctrl->dev, "failed to request interrupt\n");
> +	}
> +
>  	ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
>  	if (ret) {
>  		dev_err(pctrl->dev, "failed to register gpiochip\n");
> @@ -1222,27 +1253,6 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
>  		}
>  	}
>  
> -	/*
> -	 * We need to request the interrupt here (instead of providing chip
> -	 * to the irq directly) because on some platforms several GPIO
> -	 * controllers share the same interrupt line.
> -	 */
> -	ret = devm_request_irq(pctrl->dev, irq, intel_gpio_irq,
> -			       IRQF_SHARED | IRQF_NO_THREAD,
> -			       dev_name(pctrl->dev), pctrl);
> -	if (ret) {
> -		dev_err(pctrl->dev, "failed to request interrupt\n");
> -		return ret;
> -	}
> -
> -	ret = gpiochip_irqchip_add(&pctrl->chip, &pctrl->irqchip, 0,
> -				   handle_bad_irq, IRQ_TYPE_NONE);
> -	if (ret) {
> -		dev_err(pctrl->dev, "failed to add irqchip\n");
> -		return ret;
> -	}
> -
> -	gpiochip_set_chained_irqchip(&pctrl->chip, &pctrl->irqchip, irq, NULL);
>  	return 0;
>  }
>  
> -- 
> 2.21.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: intel: Pass irqchip when adding gpiochip

[Mika Westerberg]

On Sun, Oct 06, 2019 at 02:59:49AM +0200, Linus Walleij wrote:
> We need to convert all old gpio irqchips to pass the irqchip
> setup along when adding the gpio_chip. For more info see
> drivers/gpio/TODO.
> 
> This is an unchained irqchip so we use the method from
> drivers/gpio/gpio-mt7621.c that also requests its interrupt
> instead if chaining the interrupt handler.
> 
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> This is based on Andy's patch to dynamically allocate the
> irqchio. Mika maybe you want to queue it with the rest of
> the stuff so you can sync fixes and new development?

Sure, I can take it. I suppose there will be v2 coming so waiting for
that :)

Re: [PATCH] pinctrl: intel: Pass irqchip when adding gpiochip

[Andy Shevchenko]

On Tue, Jan 07, 2020 at 03:52:16PM +0200, Mika Westerberg wrote:
> On Tue, Jan 07, 2020 at 11:32:54AM +0100, Linus Walleij wrote:
> > On Mon, Dec 30, 2019 at 11:20 AM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > > On Sun, Dec 29, 2019 at 02:30:59AM +0100, Linus Walleij wrote:
> > > > We need to convert all old gpio irqchips to pass the irqchip
> > > > setup along when adding the gpio_chip. For more info see
> > > > drivers/gpio/TODO.
> > > >
> > > > Set up the pin ranges using the new callback.
> > >
> > > Maybe have this one split as a separate patch? Same what we do for
> > > Baytrail and Cherryview.
> > 
> > I'm afraid to do that since splitting the semantic ordering was
> > something that broke a lot of times already, I was under the
> > impression that doing the two things (moving to the callback
> > and adding along with the gpio_chip) at the same time was
> > the only way to preserve the semantic ordering.
> 
> Well at least we do the same for others (add the callback in another
> patch and then pass irqchip in another) but no strong feelings. I'm fine
> with this one as well :)
> 
> > But more than anything I want someone to test it ...
> 
> I quickly tested this on Whiskey Lake and SD card detection interrupt
> still works fine after this patch.

My understanding that Linus asked to test a proposed (two patch) series...

Linus, can you send v2 as series of two patches as Mika suggested?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: intel: Pass irqchip when adding gpiochip

[Mika Westerberg]

On Tue, Jan 07, 2020 at 11:32:54AM +0100, Linus Walleij wrote:
> On Mon, Dec 30, 2019 at 11:20 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Sun, Dec 29, 2019 at 02:30:59AM +0100, Linus Walleij wrote:
> > > We need to convert all old gpio irqchips to pass the irqchip
> > > setup along when adding the gpio_chip. For more info see
> > > drivers/gpio/TODO.
> > >
> > > Set up the pin ranges using the new callback.
> >
> > Maybe have this one split as a separate patch? Same what we do for
> > Baytrail and Cherryview.
> 
> I'm afraid to do that since splitting the semantic ordering was
> something that broke a lot of times already, I was under the
> impression that doing the two things (moving to the callback
> and adding along with the gpio_chip) at the same time was
> the only way to preserve the semantic ordering.

Well at least we do the same for others (add the callback in another
patch and then pass irqchip in another) but no strong feelings. I'm fine
with this one as well :)

> But more than anything I want someone to test it ...

I quickly tested this on Whiskey Lake and SD card detection interrupt
still works fine after this patch.

Re: [PATCH] pinctrl: intel: Pass irqchip when adding gpiochip

[Linus Walleij]

On Mon, Dec 30, 2019 at 11:20 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Sun, Dec 29, 2019 at 02:30:59AM +0100, Linus Walleij wrote:
> > We need to convert all old gpio irqchips to pass the irqchip
> > setup along when adding the gpio_chip. For more info see
> > drivers/gpio/TODO.
> >
> > Set up the pin ranges using the new callback.
>
> Maybe have this one split as a separate patch? Same what we do for
> Baytrail and Cherryview.

I'm afraid to do that since splitting the semantic ordering was
something that broke a lot of times already, I was under the
impression that doing the two things (moving to the callback
and adding along with the gpio_chip) at the same time was
the only way to preserve the semantic ordering.

But more than anything I want someone to test it ...

Yours,
Linus Walleij

Re: [PATCH] pinctrl: intel: Pass irqchip when adding gpiochip

[Mika Westerberg]

On Sun, Dec 29, 2019 at 02:30:59AM +0100, Linus Walleij wrote:
> We need to convert all old gpio irqchips to pass the irqchip
> setup along when adding the gpio_chip. For more info see
> drivers/gpio/TODO.
> 
> Set up the pin ranges using the new callback.

Maybe have this one split as a separate patch? Same what we do for
Baytrail and Cherryview.

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 61 +++++++++++++++------------
>  1 file changed, 35 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 4860bc9a4e48..ffacd77861f7 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -1160,8 +1160,8 @@ static irqreturn_t intel_gpio_irq(int irq, void *data)
>  	return ret;
>  }
>  
> -static int intel_gpio_add_pin_ranges(struct intel_pinctrl *pctrl,
> -				     const struct intel_community *community)
> +static int intel_gpio_add_community_ranges(struct intel_pinctrl *pctrl,
> +				const struct intel_community *community)
>  {
>  	int ret = 0, i;
>  
> @@ -1181,6 +1181,24 @@ static int intel_gpio_add_pin_ranges(struct intel_pinctrl *pctrl,
>  	return ret;
>  }
>  
> +static int intel_gpio_add_pin_ranges(struct gpio_chip *gc)
> +{
> +	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
> +	int ret, i;
> +
> +	for (i = 0; i < pctrl->ncommunities; i++) {
> +		struct intel_community *community = &pctrl->communities[i];
> +
> +		ret = intel_gpio_add_community_ranges(pctrl, community);
> +		if (ret) {
> +			dev_err(pctrl->dev, "failed to add GPIO pin range\n");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static unsigned int intel_gpio_ngpio(const struct intel_pinctrl *pctrl)
>  {
>  	const struct intel_community *community;
> @@ -1205,7 +1223,8 @@ static unsigned int intel_gpio_ngpio(const struct intel_pinctrl *pctrl)
>  
>  static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
>  {
> -	int ret, i;
> +	int ret;
> +	struct gpio_irq_chip *girq;

Nit:

Can you order these in "reverse christmas tree" like,

	struct gpio_irq_chip *girq;
	int ret;

Otherwise looks good to me, thanks!

[PATCH] pinctrl: intel: Pass irqchip when adding gpiochip

[Linus Walleij]

We need to convert all old gpio irqchips to pass the irqchip
setup along when adding the gpio_chip. For more info see
drivers/gpio/TODO.

Set up the pin ranges using the new callback.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 61 +++++++++++++++------------
 1 file changed, 35 insertions(+), 26 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 4860bc9a4e48..ffacd77861f7 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1160,8 +1160,8 @@ static irqreturn_t intel_gpio_irq(int irq, void *data)
 	return ret;
 }
 
-static int intel_gpio_add_pin_ranges(struct intel_pinctrl *pctrl,
-				     const struct intel_community *community)
+static int intel_gpio_add_community_ranges(struct intel_pinctrl *pctrl,
+				const struct intel_community *community)
 {
 	int ret = 0, i;
 
@@ -1181,6 +1181,24 @@ static int intel_gpio_add_pin_ranges(struct intel_pinctrl *pctrl,
 	return ret;
 }
 
+static int intel_gpio_add_pin_ranges(struct gpio_chip *gc)
+{
+	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
+	int ret, i;
+
+	for (i = 0; i < pctrl->ncommunities; i++) {
+		struct intel_community *community = &pctrl->communities[i];
+
+		ret = intel_gpio_add_community_ranges(pctrl, community);
+		if (ret) {
+			dev_err(pctrl->dev, "failed to add GPIO pin range\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static unsigned int intel_gpio_ngpio(const struct intel_pinctrl *pctrl)
 {
 	const struct intel_community *community;
@@ -1205,7 +1223,8 @@ static unsigned int intel_gpio_ngpio(const struct intel_pinctrl *pctrl)
 
 static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
 {
-	int ret, i;
+	int ret;
+	struct gpio_irq_chip *girq;
 
 	pctrl->chip = intel_gpio_chip;
 
@@ -1214,6 +1233,7 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
 	pctrl->chip.label = dev_name(pctrl->dev);
 	pctrl->chip.parent = pctrl->dev;
 	pctrl->chip.base = -1;
+	pctrl->chip.add_pin_ranges = intel_gpio_add_pin_ranges;
 	pctrl->irq = irq;
 
 	/* Setup IRQ chip */
@@ -1225,26 +1245,9 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
 	pctrl->irqchip.irq_set_wake = intel_gpio_irq_wake;
 	pctrl->irqchip.flags = IRQCHIP_MASK_ON_SUSPEND;
 
-	ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
-	if (ret) {
-		dev_err(pctrl->dev, "failed to register gpiochip\n");
-		return ret;
-	}
-
-	for (i = 0; i < pctrl->ncommunities; i++) {
-		struct intel_community *community = &pctrl->communities[i];
-
-		ret = intel_gpio_add_pin_ranges(pctrl, community);
-		if (ret) {
-			dev_err(pctrl->dev, "failed to add GPIO pin range\n");
-			return ret;
-		}
-	}
-
 	/*
-	 * We need to request the interrupt here (instead of providing chip
-	 * to the irq directly) because on some platforms several GPIO
-	 * controllers share the same interrupt line.
+	 * On some platforms several GPIO controllers share the same interrupt
+	 * line.
 	 */
 	ret = devm_request_irq(pctrl->dev, irq, intel_gpio_irq,
 			       IRQF_SHARED | IRQF_NO_THREAD,
@@ -1254,14 +1257,20 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
 		return ret;
 	}
 
-	ret = gpiochip_irqchip_add(&pctrl->chip, &pctrl->irqchip, 0,
-				   handle_bad_irq, IRQ_TYPE_NONE);
+	girq = &pctrl->chip.irq;
+	girq->chip = &pctrl->irqchip;
+	/* This will let us handle the IRQ in the driver */
+	girq->parent_handler = NULL;
+	girq->num_parents = 0;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_bad_irq;
+
+	ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
 	if (ret) {
-		dev_err(pctrl->dev, "failed to add irqchip\n");
+		dev_err(pctrl->dev, "failed to register gpiochip\n");
 		return ret;
 	}
 
-	gpiochip_set_chained_irqchip(&pctrl->chip, &pctrl->irqchip, irq, NULL);
 	return 0;
 }
 
-- 
2.23.0