| Message ID | 20190812081351.21284-1-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: lynxpoint: Pass irqchip when adding gpiochip | expand |
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?
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
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.
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
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);
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(-)