[PATCH] gpio: lynxpoint: Pass irqchip when adding gpiochip

[PATCH] gpio: lynxpoint: 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.

For chained irqchips this is a pretty straight-forward
conversion.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: David Cohen <david.a.cohen@linux.intel.com>
Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Andy: when you're happy with this you can either supply an
ACK and I will merge it or you can merge it into your tree
for a later pull request, just tell me what you prefer.
---
 drivers/gpio/gpio-lynxpoint.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpio-lynxpoint.c b/drivers/gpio/gpio-lynxpoint.c
index 31b4a091ab60..e8ec07910eb7 100644
--- a/drivers/gpio/gpio-lynxpoint.c
+++ b/drivers/gpio/gpio-lynxpoint.c
@@ -358,25 +358,30 @@ static int lp_gpio_probe(struct platform_device *pdev)
 	gc->can_sleep = false;
 	gc->parent = dev;
 
-	ret = devm_gpiochip_add_data(dev, gc, lg);
-	if (ret) {
-		dev_err(dev, "failed adding lp-gpio chip\n");
-		return ret;
-	}
-
 	/* set up interrupts  */
 	if (irq_rc && irq_rc->start) {
+		struct gpio_irq_chip *girq;
+
+		girq = &gc->irq;
+		girq->chip = &lp_irqchip;
+		girq->parent_handler = lp_gpio_irq_handler;
+		girq->num_parents = 1;
+		girq->parents = devm_kcalloc(&pdev->dev, 1,
+					     sizeof(*girq->parents),
+					     GFP_KERNEL);
+		if (!girq->parents)
+			return -ENOMEM;
+		girq->parents[0] = (unsigned)irq_rc->start;
+		girq->default_type = IRQ_TYPE_NONE;
+		girq->handler = handle_simple_irq;
+
 		lp_gpio_irq_init_hw(lg);
-		ret = gpiochip_irqchip_add(gc, &lp_irqchip, 0,
-					   handle_simple_irq, IRQ_TYPE_NONE);
-		if (ret) {
-			dev_err(dev, "failed to add irqchip\n");
-			return ret;
-		}
+	}
 
-		gpiochip_set_chained_irqchip(gc, &lp_irqchip,
-					     (unsigned)irq_rc->start,
-					     lp_gpio_irq_handler);
+	ret = devm_gpiochip_add_data(dev, gc, lg);
+	if (ret) {
+		dev_err(dev, "failed adding lp-gpio chip\n");
+		return ret;
 	}
 
 	pm_runtime_enable(dev);
-- 
2.21.0

Re: [PATCH] gpio: lynxpoint: Pass irqchip when adding gpiochip

[Andy Shevchenko]

On Mon, Aug 12, 2019 at 10:13:51AM +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.
> 
> For chained irqchips this is a pretty straight-forward
> conversion.
> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: David Cohen <david.a.cohen@linux.intel.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Andy: when you're happy with this you can either supply an
> ACK and I will merge it or you can merge it into your tree
> for a later pull request, just tell me what you prefer.

I'll take through my tree.
See comment below.

> +		girq->num_parents = 1;
> +		girq->parents = devm_kcalloc(&pdev->dev, 1,
> +					     sizeof(*girq->parents),
> +					     GFP_KERNEL);
> +		if (!girq->parents)
> +			return -ENOMEM;

I understand the point to use kcalloc() for one entry, though I would make
intention more explicitly, i.e. use girq->num_parents in it instead of hard
coded value.

> +		girq->parents[0] = (unsigned)irq_rc->start;
> +		girq->default_type = IRQ_TYPE_NONE;

> +		girq->handler = handle_simple_irq;

> -		ret = gpiochip_irqchip_add(gc, &lp_irqchip, 0,
> -					   handle_simple_irq, IRQ_TYPE_NONE);

Hmm... Now I'm wondering, shall we use handle_bad_irq() here?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: lynxpoint: Pass irqchip when adding gpiochip

[Linus Walleij]

On Mon, Aug 12, 2019 at 12:58 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Aug 12, 2019 at 10:13:51AM +0200, Linus Walleij wrote:

> > +             girq->num_parents = 1;
> > +             girq->parents = devm_kcalloc(&pdev->dev, 1,
> > +                                          sizeof(*girq->parents),
> > +                                          GFP_KERNEL);
> > +             if (!girq->parents)
> > +                     return -ENOMEM;
>
> I understand the point to use kcalloc() for one entry, though I would make
> intention more explicitly, i.e. use girq->num_parents in it instead of hard
> coded value.

That is better, but I have a loose plan to get rid of this
and just set parents to a fixed width because all the allocation
is annoying.

> > +             girq->parents[0] = (unsigned)irq_rc->start;
> > +             girq->default_type = IRQ_TYPE_NONE;
>
> > +             girq->handler = handle_simple_irq;
>
> > -             ret = gpiochip_irqchip_add(gc, &lp_irqchip, 0,
> > -                                        handle_simple_irq, IRQ_TYPE_NONE);
>
> Hmm... Now I'm wondering, shall we use handle_bad_irq() here?

If you are sure that every consumer will call .set_type() you can
use handle_bad_irq, and that is preferred.

Yours,
Linus Walleij

Re: [PATCH] gpio: lynxpoint: Pass irqchip when adding gpiochip

[Andy Shevchenko]

On Wed, Aug 14, 2019 at 10:46:49AM +0200, Linus Walleij wrote:
> On Mon, Aug 12, 2019 at 12:58 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Aug 12, 2019 at 10:13:51AM +0200, Linus Walleij wrote:
> 
> > > +             girq->num_parents = 1;
> > > +             girq->parents = devm_kcalloc(&pdev->dev, 1,
> > > +                                          sizeof(*girq->parents),
> > > +                                          GFP_KERNEL);
> > > +             if (!girq->parents)
> > > +                     return -ENOMEM;
> >
> > I understand the point to use kcalloc() for one entry, though I would make
> > intention more explicitly, i.e. use girq->num_parents in it instead of hard
> > coded value.
> 
> That is better, but I have a loose plan to get rid of this
> and just set parents to a fixed width because all the allocation
> is annoying.

I see your intentions, though for current state I think the less hard coded
constants the better. In any case I pushed updated versions to my trees.

> > > +             girq->parents[0] = (unsigned)irq_rc->start;
> > > +             girq->default_type = IRQ_TYPE_NONE;
> >
> > > +             girq->handler = handle_simple_irq;
> >
> > > -             ret = gpiochip_irqchip_add(gc, &lp_irqchip, 0,
> > > -                                        handle_simple_irq, IRQ_TYPE_NONE);
> >
> > Hmm... Now I'm wondering, shall we use handle_bad_irq() here?
> 
> If you are sure that every consumer will call .set_type() you can
> use handle_bad_irq, and that is preferred.

They should do this. Let me prepare the patch for next cycle (v5.5) and I put
it to my tree after merge window. If we see any complains from linux-next
testers, we will act accordingly.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: lynxpoint: Pass irqchip when adding gpiochip

[Linus Walleij]

On Wed, Aug 14, 2019 at 11:40 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, Aug 14, 2019 at 10:46:49AM +0200, Linus Walleij wrote:
> > On Mon, Aug 12, 2019 at 12:58 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Aug 12, 2019 at 10:13:51AM +0200, Linus Walleij wrote:
> >
> > > > +             girq->num_parents = 1;
> > > > +             girq->parents = devm_kcalloc(&pdev->dev, 1,
> > > > +                                          sizeof(*girq->parents),
> > > > +                                          GFP_KERNEL);
> > > > +             if (!girq->parents)
> > > > +                     return -ENOMEM;
> > >
> > > I understand the point to use kcalloc() for one entry, though I would make
> > > intention more explicitly, i.e. use girq->num_parents in it instead of hard
> > > coded value.
> >
> > That is better, but I have a loose plan to get rid of this
> > and just set parents to a fixed width because all the allocation
> > is annoying.
>
> I see your intentions, though for current state I think the less hard coded
> constants the better. In any case I pushed updated versions to my trees.

Thanks a lot. Yeah we live with the API we have and work from there.

> > If you are sure that every consumer will call .set_type() you can
> > use handle_bad_irq, and that is preferred.
>
> They should do this. Let me prepare the patch for next cycle (v5.5) and I put
> it to my tree after merge window. If we see any complains from linux-next
> testers, we will act accordingly.

Sounds like a plan!

Thanks,
Linus Walleij