[PATCH] RFC: pinctrl: cherryview: Pass irqchip when adding gpiochip

[PATCH] RFC: pinctrl: cherryview: 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 driver is something of a special case, so we need to
discuss it.

It picks a number of IRQ descriptors before setting up
the gpio_irq_chip using devm_irq_alloc_descs() giving a
fixed irq base in the IRQ numberspace. It then games the
irqchip API by associating IRQs from that base and upward
with as many pins there are in the "community" which is a
set of pins. Then after each gpio_chip is registered, it
fills in the pin to IRQ map for each GPIO range inside
that "community" with irq_domain_associate_many() which
works fine since the descriptors were allocated
previously.

This is actually a hierarchical irq_chip as far as I can
tell. The problem is that very likely the Intel root IRQ
chip is not hierarchical so it does not support using the
facilities for hierarchical irqdomains.

I will soon merge the patch providing hierarchical irqchip
support in gpiolib:
https://lore.kernel.org/linux-gpio/20190808123242.5359-1-linus.walleij@linaro.org/

Will we need to bite the bullet and convert the root
irqchip for the intels to support hierarcical irqdomain?

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 37 +++++++++++++++-------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 03ec7a5d9d0b..932da7071356 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1547,6 +1547,7 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 {
 	const struct chv_gpio_pinrange *range;
 	struct gpio_chip *chip = &pctrl->chip;
+	struct gpio_irq_chip *girq;
 	bool need_valid_mask = !dmi_check_system(chv_no_valid_mask);
 	const struct chv_community *community = pctrl->community;
 	int ret, i, irq_base;
@@ -1559,12 +1560,6 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 	chip->base = -1;
 	chip->irq.need_valid_mask = need_valid_mask;
 
-	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
-	if (ret) {
-		dev_err(pctrl->dev, "Failed to register gpiochip\n");
-		return ret;
-	}
-
 	for (i = 0; i < community->ngpio_ranges; i++) {
 		range = &community->gpio_ranges[i];
 		ret = gpiochip_add_pin_range(chip, dev_name(pctrl->dev),
@@ -1610,6 +1605,11 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 	/* Clear all interrupts */
 	chv_writel(0xffff, pctrl->regs + CHV_INTSTAT);
 
+	/*
+	 * FIXME: this picks as many IRQs as there are lines in the
+	 * "community", which is then later associated per-range below
+	 * registering the gpio_chip. This is actually hierarchical IRQ.
+	 */
 	if (!need_valid_mask) {
 		irq_base = devm_irq_alloc_descs(pctrl->dev, -1, 0,
 						community->npins, NUMA_NO_NODE);
@@ -1619,13 +1619,30 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 		}
 	}
 
-	ret = gpiochip_irqchip_add(chip, &chv_gpio_irqchip, 0,
-				   handle_bad_irq, IRQ_TYPE_NONE);
+	girq = &chip->irq;
+	girq->chip = &chv_gpio_irqchip;
+	girq->parent_handler = chv_gpio_irq_handler;
+	girq->num_parents = 1;
+	girq->parents = devm_kcalloc(pctrl->dev, 1,
+				     sizeof(*girq->parents),
+				     GFP_KERNEL);
+	if (!girq->parents)
+		return -ENOMEM;
+	girq->parents[0] = irq;
+	girq->default_handler = IRQ_TYPE_NONE;
+	girq->handler = handle_bad_irq;
+
+	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
 	if (ret) {
-		dev_err(pctrl->dev, "failed to add IRQ chip\n");
+		dev_err(pctrl->dev, "Failed to register gpiochip\n");
 		return ret;
 	}
 
+	/*
+	 * FIXME: this associates a different IRQ with each discrete range
+	 * inside the community. If we use the hierarchical irq support,
+	 * the .translate() function can do this translation for each IRQ.
+	 */
 	if (!need_valid_mask) {
 		for (i = 0; i < community->ngpio_ranges; i++) {
 			range = &community->gpio_ranges[i];
@@ -1636,8 +1653,6 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 		}
 	}
 
-	gpiochip_set_chained_irqchip(chip, &chv_gpio_irqchip, irq,
-				     chv_gpio_irq_handler);
 	return 0;
 }
 
-- 
2.21.0

Re: [PATCH] RFC: pinctrl: cherryview: Pass irqchip when adding gpiochip

[Andy Shevchenko]

On Tue, Aug 13, 2019 at 09:01:23AM +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 driver is something of a special case, so we need to
> discuss it.
> 
> It picks a number of IRQ descriptors before setting up
> the gpio_irq_chip using devm_irq_alloc_descs() giving a
> fixed irq base in the IRQ numberspace. It then games the
> irqchip API by associating IRQs from that base and upward
> with as many pins there are in the "community" which is a
> set of pins. Then after each gpio_chip is registered, it
> fills in the pin to IRQ map for each GPIO range inside
> that "community" with irq_domain_associate_many() which
> works fine since the descriptors were allocated
> previously.
> 
> This is actually a hierarchical irq_chip as far as I can
> tell. The problem is that very likely the Intel root IRQ
> chip is not hierarchical so it does not support using the
> facilities for hierarchical irqdomains.
> 
> I will soon merge the patch providing hierarchical irqchip
> support in gpiolib:
> https://lore.kernel.org/linux-gpio/20190808123242.5359-1-linus.walleij@linaro.org/
> 
> Will we need to bite the bullet and convert the root
> irqchip for the intels to support hierarcical irqdomain?

We have few fixes for this driver, perhaps you can send a new version based on
them when they appear in your tree.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] RFC: pinctrl: cherryview: Pass irqchip when adding gpiochip

[Linus Walleij]

On Thu, Oct 24, 2019 at 3:38 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, Aug 13, 2019 at 09:01:23AM +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 driver is something of a special case, so we need to
> > discuss it.
> >
> > It picks a number of IRQ descriptors before setting up
> > the gpio_irq_chip using devm_irq_alloc_descs() giving a
> > fixed irq base in the IRQ numberspace. It then games the
> > irqchip API by associating IRQs from that base and upward
> > with as many pins there are in the "community" which is a
> > set of pins. Then after each gpio_chip is registered, it
> > fills in the pin to IRQ map for each GPIO range inside
> > that "community" with irq_domain_associate_many() which
> > works fine since the descriptors were allocated
> > previously.
> >
> > This is actually a hierarchical irq_chip as far as I can
> > tell. The problem is that very likely the Intel root IRQ
> > chip is not hierarchical so it does not support using the
> > facilities for hierarchical irqdomains.
> >
> > I will soon merge the patch providing hierarchical irqchip
> > support in gpiolib:
> > https://lore.kernel.org/linux-gpio/20190808123242.5359-1-linus.walleij@linaro.org/
> >
> > Will we need to bite the bullet and convert the root
> > irqchip for the intels to support hierarcical irqdomain?
>
> We have few fixes for this driver, perhaps you can send a new version based on
> them when they appear in your tree.

I'm pretty scared of this driver so I am keeping my hands off it
for the time being. Also this patch has compile errors. I try
not to change to many complicated things at one. Maybe next
kernel cycle...

Yours,
Linus Walleij