* [PATCH] gpio: crystalcove: Use irqchip template
@ 2020-07-17 11:25 Linus Walleij
2020-07-17 13:59 ` Hans de Goede
0 siblings, 1 reply; 5+ messages in thread
From: Linus Walleij @ 2020-07-17 11:25 UTC (permalink / raw)
To: linux-gpio
Cc: Bartosz Golaszewski, Linus Walleij, Andy Shevchenko,
Kuppuswamy Sathyanarayanan, Hans de Goede
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(-)
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;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] gpio: crystalcove: Use irqchip template
2020-07-17 11:25 [PATCH] gpio: crystalcove: Use irqchip template Linus Walleij
@ 2020-07-17 13:59 ` Hans de Goede
2020-07-17 14:02 ` Hans de Goede
2020-07-17 14:08 ` Andy Shevchenko
0 siblings, 2 replies; 5+ messages in thread
From: Hans de Goede @ 2020-07-17 13:59 UTC (permalink / raw)
To: Linus Walleij, linux-gpio
Cc: Bartosz Golaszewski, Andy Shevchenko, Kuppuswamy Sathyanarayanan
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;
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gpio: crystalcove: Use irqchip template
2020-07-17 13:59 ` Hans de Goede
@ 2020-07-17 14:02 ` Hans de Goede
2020-07-17 14:14 ` Linus Walleij
2020-07-17 14:08 ` Andy Shevchenko
1 sibling, 1 reply; 5+ messages in thread
From: Hans de Goede @ 2020-07-17 14:02 UTC (permalink / raw)
To: Linus Walleij, linux-gpio
Cc: Bartosz Golaszewski, Andy Shevchenko, Kuppuswamy Sathyanarayanan
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;
>> }
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gpio: crystalcove: Use irqchip template
2020-07-17 13:59 ` Hans de Goede
2020-07-17 14:02 ` Hans de Goede
@ 2020-07-17 14:08 ` Andy Shevchenko
1 sibling, 0 replies; 5+ messages in thread
From: Andy Shevchenko @ 2020-07-17 14:08 UTC (permalink / raw)
To: Hans de Goede
Cc: Linus Walleij, open list:GPIO SUBSYSTEM, Bartosz Golaszewski,
Andy Shevchenko, Kuppuswamy Sathyanarayanan
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gpio: crystalcove: Use irqchip template
2020-07-17 14:02 ` Hans de Goede
@ 2020-07-17 14:14 ` Linus Walleij
0 siblings, 0 replies; 5+ messages in thread
From: Linus Walleij @ 2020-07-17 14:14 UTC (permalink / raw)
To: Hans de Goede
Cc: open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Andy Shevchenko,
Kuppuswamy Sathyanarayanan
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-17 14:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-17 11:25 [PATCH] gpio: crystalcove: Use irqchip template Linus Walleij
2020-07-17 13:59 ` Hans de Goede
2020-07-17 14:02 ` Hans de Goede
2020-07-17 14:14 ` Linus Walleij
2020-07-17 14:08 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).