[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.

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

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!

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

[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