| Message ID | 20200721140153.369171-1-linus.walleij@linaro.org |
|---|---|
| State | Accepted |
| Commit | 945e72db36bd12767601b332b2aa50c888537afa |
| Headers | show |
| Series | [v2] gpio: crystalcove: Use irqchip template | expand |
On Tue, Jul 21, 2020 at 04:01:53PM +0200, 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. Took this version instead. I dunno if Hans can come with some comments / testing results, I would like to send a PR today or tomorrow. -- With Best Regards, Andy Shevchenko
On Tue, Jul 21, 2020 at 06:39:36PM +0300, Andy Shevchenko wrote: > On Tue, Jul 21, 2020 at 04:01:53PM +0200, 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. > > Took this version instead. > > I dunno if Hans can come with some comments / testing results, I would like to > send a PR today or tomorrow. Same for wcove patch. -- With Best Regards, Andy Shevchenko
Hi, On 7/21/20 7:01 AM, 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. Looks good to me. Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > 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> > --- > ChangeLog v1->V2: > - Fixed a variable name ch->cg > --- > 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..39349b0e6923 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 = &cg->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; > } > -- Sathyanarayanan Kuppuswamy Linux Kernel Developer
On Tue, Jul 21, 2020 at 10:08:57AM -0700, Kuppuswamy, Sathyanarayanan wrote: > Hi, > > On 7/21/20 7:01 AM, 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. > Looks good to me. Thanks! > Reviewed-by: Kuppuswamy Sathyanarayanan > <sathyanarayanan.kuppuswamy@linux.intel.com> It's not first time your tag goes like this. Please, fix your tools to be it like Reviewed-by: Name <address@com> (no leading spaces, no split -- one line) > > 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> > > --- > > ChangeLog v1->V2: > > - Fixed a variable name ch->cg > > --- > > 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..39349b0e6923 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 = &cg->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; > > } > > > > -- > Sathyanarayanan Kuppuswamy > Linux Kernel Developer -- With Best Regards, Andy Shevchenko
On 7/21/20 2:54 PM, Andy Shevchenko wrote: > On Tue, Jul 21, 2020 at 10:08:57AM -0700, Kuppuswamy, Sathyanarayanan wrote: >> Hi, >> >> On 7/21/20 7:01 AM, 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. >> Looks good to me. > > Thanks! > >> Reviewed-by: Kuppuswamy Sathyanarayanan >> <sathyanarayanan.kuppuswamy@linux.intel.com> > > It's not first time your tag goes like this. Please, fix your tools to be it like > Reviewed-by: Name <address@com> > (no leading spaces, no split -- one line) Ok. I will fix it in future emails. > >>> 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> >>> --- >>> ChangeLog v1->V2: >>> - Fixed a variable name ch->cg >>> --- >>> 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..39349b0e6923 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 = &cg->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; >>> } >>> >> >> -- >> Sathyanarayanan Kuppuswamy >> Linux Kernel Developer > -- Sathyanarayanan Kuppuswamy Linux Kernel Developer
Hi, On 7/21/20 5:39 PM, Andy Shevchenko wrote: > On Tue, Jul 21, 2020 at 04:01:53PM +0200, 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. > > Took this version instead. > > I dunno if Hans can come with some comments / testing results, I would like to > send a PR today or tomorrow. Sorry for being a bit slow with testing this. I've given this patch a test-run on a machine with the PMIC the driver is for and with the caveat that I've not actually tested the GPIO IRQ functionality, since that does not seem to be used on any machines, I see no adverse side effects from this patch, so it is: Tested-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans
On Wed, Jul 22, 2020 at 11:56 AM Hans de Goede <hdegoede@redhat.com> wrote: > On 7/21/20 5:39 PM, Andy Shevchenko wrote: > > On Tue, Jul 21, 2020 at 04:01:53PM +0200, 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. > > > > Took this version instead. > > > > I dunno if Hans can come with some comments / testing results, I would like to > > send a PR today or tomorrow. > > Sorry for being a bit slow with testing this. > > I've given this patch a test-run on a machine with the > PMIC the driver is for and with the caveat that I've not > actually tested the GPIO IRQ functionality, since that > does not seem to be used on any machines, I see no adverse > side effects from this patch, so it is: > > Tested-by: Hans de Goede <hdegoede@redhat.com> Thank you, Hans! It seems we also don't have such a reference design which uses PMIC GPIOs for IRQ. P.S. Does wcove case similar? -- With Best Regards, Andy Shevchenko
On Wed, Jul 22, 2020 at 11:59 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Jul 22, 2020 at 11:56 AM Hans de Goede <hdegoede@redhat.com> wrote: > P.S. Does wcove case similar? Never mind, I found the answer :-) -- With Best Regards, Andy Shevchenko
diff --git a/drivers/gpio/gpio-crystalcove.c b/drivers/gpio/gpio-crystalcove.c index 14d1f4c933b6..39349b0e6923 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 = &cg->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> --- ChangeLog v1->V2: - Fixed a variable name ch->cg --- drivers/gpio/gpio-crystalcove.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) -- 2.26.2