| Message ID | 20200717112558.15960-1-linus.walleij@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpio: crystalcove: Use irqchip template | expand |
Hi, On 7/17/20 1:25 PM, Linus Walleij wrote: > This makes the driver use the irqchip template to assign > properties to the gpio_irq_chip instead of using the > explicit calls to gpiochip_irqchip_add_nested() and > gpiochip_set_nested_irqchip(). The irqchip is instead > added while adding the gpiochip. > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > Cc: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > Intel folks and Hans: I hope someone can test this, I'm > a bit uncertain if IRQs could fire before registering > the chip and if we need a hw_init() in this driver to cope. I've added this to my personal tree for testing. I will get back to you when I've either hit an issue, or used it for a while without issues :) Hmm, testing this might be tricky, I don't think any boards actually use any GPIOs on the PMIC (which this driver is for) as interrupts... So the best I can do is boot a machine and test there are no regressions I guess. Regards, Hans > --- > drivers/gpio/gpio-crystalcove.c | 24 +++++++++++++++--------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpio/gpio-crystalcove.c b/drivers/gpio/gpio-crystalcove.c > index 14d1f4c933b6..424a00ba1c97 100644 > --- a/drivers/gpio/gpio-crystalcove.c > +++ b/drivers/gpio/gpio-crystalcove.c > @@ -330,6 +330,7 @@ static int crystalcove_gpio_probe(struct platform_device *pdev) > int retval; > struct device *dev = pdev->dev.parent; > struct intel_soc_pmic *pmic = dev_get_drvdata(dev); > + struct gpio_irq_chip *girq; > > if (irq < 0) > return irq; > @@ -353,14 +354,15 @@ static int crystalcove_gpio_probe(struct platform_device *pdev) > cg->chip.dbg_show = crystalcove_gpio_dbg_show; > cg->regmap = pmic->regmap; > > - retval = devm_gpiochip_add_data(&pdev->dev, &cg->chip, cg); > - if (retval) { > - dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval); > - return retval; > - } > - > - gpiochip_irqchip_add_nested(&cg->chip, &crystalcove_irqchip, 0, > - handle_simple_irq, IRQ_TYPE_NONE); > + girq = &ch->chip.irq; > + girq->chip = &crystalcove_irqchip; > + /* This will let us handle the parent IRQ in the driver */ > + girq->parent_handler = NULL; > + girq->num_parents = 0; > + girq->parents = NULL; > + girq->default_type = IRQ_TYPE_NONE; > + girq->handler = handle_simple_irq; > + girq->threaded = true; > > retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler, > IRQF_ONESHOT, KBUILD_MODNAME, cg); > @@ -370,7 +372,11 @@ static int crystalcove_gpio_probe(struct platform_device *pdev) > return retval; > } > > - gpiochip_set_nested_irqchip(&cg->chip, &crystalcove_irqchip, irq); > + retval = devm_gpiochip_add_data(&pdev->dev, &cg->chip, cg); > + if (retval) { > + dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval); > + return retval; > + } > > return 0; > } >
Hi, On 7/17/20 3:59 PM, Hans de Goede wrote: > Hi, > > On 7/17/20 1:25 PM, Linus Walleij wrote: >> This makes the driver use the irqchip template to assign >> properties to the gpio_irq_chip instead of using the >> explicit calls to gpiochip_irqchip_add_nested() and >> gpiochip_set_nested_irqchip(). The irqchip is instead >> added while adding the gpiochip. >> >> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> Cc: Hans de Goede <hdegoede@redhat.com> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> >> --- >> Intel folks and Hans: I hope someone can test this, I'm >> a bit uncertain if IRQs could fire before registering >> the chip and if we need a hw_init() in this driver to cope. > > I've added this to my personal tree for testing. I will get back > to you when I've either hit an issue, or used it for a while without > issues :) > > Hmm, testing this might be tricky, I don't think any boards > actually use any GPIOs on the PMIC (which this driver is for) > as interrupts... > > So the best I can do is boot a machine and test there are no > regressions I guess. Erm, it does not even compile: drivers/gpio/gpio-crystalcove.c: In function ‘crystalcove_gpio_probe’: drivers/gpio/gpio-crystalcove.c:357:10: error: ‘ch’ undeclared (first use in this function); did you mean ‘cg’? 357 | girq = &ch->chip.irq; | ^~ | cg drivers/gpio/gpio-crystalcove.c:357:10: note: each undeclared identifier is reported only once for each function it appears in I've fixed this up locally. Regards, Hans >> + girq->default_type = IRQ_TYPE_NONE; >> + girq->handler = handle_simple_irq; >> + girq->threaded = true; >> retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler, >> IRQF_ONESHOT, KBUILD_MODNAME, cg); >> @@ -370,7 +372,11 @@ static int crystalcove_gpio_probe(struct platform_device *pdev) >> return retval; >> } >> - gpiochip_set_nested_irqchip(&cg->chip, &crystalcove_irqchip, irq); >> + retval = devm_gpiochip_add_data(&pdev->dev, &cg->chip, cg); >> + if (retval) { >> + dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval); >> + return retval; >> + } >> return 0; >> } >>
On Fri, Jul 17, 2020 at 5:00 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 7/17/20 1:25 PM, Linus Walleij wrote: ... > Hmm, testing this might be tricky, I don't think any boards > actually use any GPIOs on the PMIC (which this driver is for) > as interrupts... > > So the best I can do is boot a machine and test there are no > regressions I guess. I was about to send a PR to Linus, but I will wait for your answer. From my perspective the approach is fine. We might have needed some kind of masking, but I have no idea what those GPIOs can be used for... -- With Best Regards, Andy Shevchenko
On Fri, Jul 17, 2020 at 4:02 PM Hans de Goede <hdegoede@redhat.com> wrote: > Erm, it does not even compile: > > drivers/gpio/gpio-crystalcove.c: In function ‘crystalcove_gpio_probe’: > drivers/gpio/gpio-crystalcove.c:357:10: error: ‘ch’ undeclared (first use in this function); did you mean ‘cg’? > 357 | girq = &ch->chip.irq; > | ^~ > | cg > drivers/gpio/gpio-crystalcove.c:357:10: note: each undeclared identifier is reported only once for each function it appears in > > I've fixed this up locally. Thanks, the SOC_PMIC isn't in the Intel default build, and you know me and Intel ... Sorry for that. Yours, Linus Walleij
diff --git a/drivers/gpio/gpio-crystalcove.c b/drivers/gpio/gpio-crystalcove.c index 14d1f4c933b6..424a00ba1c97 100644 --- a/drivers/gpio/gpio-crystalcove.c +++ b/drivers/gpio/gpio-crystalcove.c @@ -330,6 +330,7 @@ static int crystalcove_gpio_probe(struct platform_device *pdev) int retval; struct device *dev = pdev->dev.parent; struct intel_soc_pmic *pmic = dev_get_drvdata(dev); + struct gpio_irq_chip *girq; if (irq < 0) return irq; @@ -353,14 +354,15 @@ static int crystalcove_gpio_probe(struct platform_device *pdev) cg->chip.dbg_show = crystalcove_gpio_dbg_show; cg->regmap = pmic->regmap; - retval = devm_gpiochip_add_data(&pdev->dev, &cg->chip, cg); - if (retval) { - dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval); - return retval; - } - - gpiochip_irqchip_add_nested(&cg->chip, &crystalcove_irqchip, 0, - handle_simple_irq, IRQ_TYPE_NONE); + girq = &ch->chip.irq; + girq->chip = &crystalcove_irqchip; + /* This will let us handle the parent IRQ in the driver */ + girq->parent_handler = NULL; + girq->num_parents = 0; + girq->parents = NULL; + girq->default_type = IRQ_TYPE_NONE; + girq->handler = handle_simple_irq; + girq->threaded = true; retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler, IRQF_ONESHOT, KBUILD_MODNAME, cg); @@ -370,7 +372,11 @@ static int crystalcove_gpio_probe(struct platform_device *pdev) return retval; } - gpiochip_set_nested_irqchip(&cg->chip, &crystalcove_irqchip, irq); + retval = devm_gpiochip_add_data(&pdev->dev, &cg->chip, cg); + if (retval) { + dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval); + return retval; + } return 0; }
This makes the driver use the irqchip template to assign properties to the gpio_irq_chip instead of using the explicit calls to gpiochip_irqchip_add_nested() and gpiochip_set_nested_irqchip(). The irqchip is instead added while adding the gpiochip. Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> Cc: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- Intel folks and Hans: I hope someone can test this, I'm a bit uncertain if IRQs could fire before registering the chip and if we need a hw_init() in this driver to cope. --- drivers/gpio/gpio-crystalcove.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) -- 2.26.2