| Message ID | 20210302153451.50593-4-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,1/4] gpiolib: Unify the checks on fwnode type | expand |
On Tue, Mar 2, 2021 at 4:35 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > When IRQ domain is created for an ACPI case, the name of it becomes unknown-%d > since for now it utilizes of_node member only and doesn't consider fwnode case. > Convert IRQ domain creation code to utilize fwnode instead. > > Before/After the change on Intel Galileo Gen 2 with two GPIO (IRQ) controllers: > > unknown-1 ==> \_SB.PCI0.GIP0.GPO > unknown-2 ==> \_SB.NIO3 > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> This first part seems to do what you want, > @@ -1457,9 +1457,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, > struct lock_class_key *lock_key, > struct lock_class_key *request_key) > { > + struct fwnode_handle *fwnode = dev_fwnode(&gc->gpiodev->dev); (...) But this: > @@ -1504,15 +1497,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, > return ret; > } else { > /* Some drivers provide custom irqdomain ops */ > - if (gc->irq.domain_ops) > - ops = gc->irq.domain_ops; > - > - if (!ops) > - ops = &gpiochip_domain_ops; > - gc->irq.domain = irq_domain_add_simple(np, > - gc->ngpio, > - gc->irq.first, > - ops, gc); > + ops = gc->irq.domain_ops ?: &gpiochip_domain_ops; > + if (gc->irq.first) > + gc->irq.domain = irq_domain_create_legacy(fwnode, gc->ngpio, > + gc->irq.first, 0, > + ops, gc); > + else > + gc->irq.domain = irq_domain_create_linear(fwnode, gc->ngpio, > + ops, gc); This looks like a refactoring and reimplementation of irq_domain_add_simple()? Why, and should it rather be a separate patch? Yours, Linus Walleij
On Wed, Mar 03, 2021 at 10:22:02AM +0100, Linus Walleij wrote: > On Tue, Mar 2, 2021 at 4:35 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > When IRQ domain is created for an ACPI case, the name of it becomes unknown-%d > > since for now it utilizes of_node member only and doesn't consider fwnode case. > > Convert IRQ domain creation code to utilize fwnode instead. > > > > Before/After the change on Intel Galileo Gen 2 with two GPIO (IRQ) controllers: > > > > unknown-1 ==> \_SB.PCI0.GIP0.GPO > > unknown-2 ==> \_SB.NIO3 > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > This first part seems to do what you want, ... > But this: > > > @@ -1504,15 +1497,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, > > return ret; > > } else { > > /* Some drivers provide custom irqdomain ops */ > > - if (gc->irq.domain_ops) > > - ops = gc->irq.domain_ops; > > - > > - if (!ops) > > - ops = &gpiochip_domain_ops; > > - gc->irq.domain = irq_domain_add_simple(np, > > - gc->ngpio, > > - gc->irq.first, > > - ops, gc); > > + ops = gc->irq.domain_ops ?: &gpiochip_domain_ops; > > + if (gc->irq.first) > > + gc->irq.domain = irq_domain_create_legacy(fwnode, gc->ngpio, > > + gc->irq.first, 0, > > + ops, gc); > > + else > > + gc->irq.domain = irq_domain_create_linear(fwnode, gc->ngpio, > > + ops, gc); > > This looks like a refactoring and reimplementation of irq_domain_add_simple()? If you named it as irq_domain_create_simple(), then yes, but the problem is that we don't have irq_domain_create_simple() API right now. > Why, and should it rather be a separate patch? Nope.
On Wed, Mar 3, 2021 at 10:35 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, Mar 03, 2021 at 10:22:02AM +0100, Linus Walleij wrote: > > But this: > > > > > @@ -1504,15 +1497,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, > > > return ret; > > > } else { > > > /* Some drivers provide custom irqdomain ops */ > > > - if (gc->irq.domain_ops) > > > - ops = gc->irq.domain_ops; > > > - > > > - if (!ops) > > > - ops = &gpiochip_domain_ops; > > > - gc->irq.domain = irq_domain_add_simple(np, > > > - gc->ngpio, > > > - gc->irq.first, > > > - ops, gc); > > > + ops = gc->irq.domain_ops ?: &gpiochip_domain_ops; > > > + if (gc->irq.first) > > > + gc->irq.domain = irq_domain_create_legacy(fwnode, gc->ngpio, > > > + gc->irq.first, 0, > > > + ops, gc); > > > + else > > > + gc->irq.domain = irq_domain_create_linear(fwnode, gc->ngpio, > > > + ops, gc); > > > > This looks like a refactoring and reimplementation of irq_domain_add_simple()? > > If you named it as irq_domain_create_simple(), then yes, but the problem is > that we don't have irq_domain_create_simple() API right now. > > > Why, and should it rather be a separate patch? > > Nope. OK I looked closer at irq_domain_add_simple(), and what it does different is to call irq_alloc_descs() for all lines if using sparse IRQs and then associate them. irq_domain_create_linear|legacy() does not allocate IRQ descriptors because it assumes something like DT or ACPI will do that on-demand when drivers request IRQs. This may be dangerous because some old platforms do not resolve IRQs at runtime and you will get NULL pointer exceptions. We then need to make sure all callers do what is done in e.g. drivers/gpio/gpio-omap.c in the #ifdef CONFIG_ARCH_OMAP1 clause: they need to be augmented to call irq_alloc_descs() explicitly, and I don't think all of them do it as nicely for us as OMAP1. I might be overly cautious though, however that is why this code uses irq_domain_add_simple(), came in commit commit 2854d167cc545d0642277bf8b77f972a91146fc6 Yours, Linus Walleij
On Thu, Mar 04, 2021 at 09:06:08AM +0100, Linus Walleij wrote: > On Wed, Mar 3, 2021 at 10:35 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Mar 03, 2021 at 10:22:02AM +0100, Linus Walleij wrote: > > > > But this: > > > > > > > @@ -1504,15 +1497,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, > > > > return ret; > > > > } else { > > > > /* Some drivers provide custom irqdomain ops */ > > > > - if (gc->irq.domain_ops) > > > > - ops = gc->irq.domain_ops; > > > > - > > > > - if (!ops) > > > > - ops = &gpiochip_domain_ops; > > > > - gc->irq.domain = irq_domain_add_simple(np, > > > > - gc->ngpio, > > > > - gc->irq.first, > > > > - ops, gc); > > > > + ops = gc->irq.domain_ops ?: &gpiochip_domain_ops; > > > > + if (gc->irq.first) > > > > + gc->irq.domain = irq_domain_create_legacy(fwnode, gc->ngpio, > > > > + gc->irq.first, 0, > > > > + ops, gc); > > > > + else > > > > + gc->irq.domain = irq_domain_create_linear(fwnode, gc->ngpio, > > > > + ops, gc); > > > > > > This looks like a refactoring and reimplementation of irq_domain_add_simple()? > > > > If you named it as irq_domain_create_simple(), then yes, but the problem is > > that we don't have irq_domain_create_simple() API right now. > > > > > Why, and should it rather be a separate patch? > > > > Nope. > > OK I looked closer at irq_domain_add_simple(), and what it does different > is to call irq_alloc_descs() for all lines if using sparse IRQs and then > associate them. irq_domain_create_linear|legacy() does not allocate IRQ > descriptors because it assumes something like DT or ACPI will do that > on-demand when drivers request IRQs. > > This may be dangerous because some old platforms do not resolve IRQs > at runtime and you will get NULL pointer exceptions. > > We then need to make sure all callers do what is done in e.g. > drivers/gpio/gpio-omap.c in the #ifdef CONFIG_ARCH_OMAP1 clause: > they need to be augmented to call irq_alloc_descs() explicitly, > and I don't think all of them do it as nicely for us as OMAP1. > > I might be overly cautious though, however that is why this code > uses irq_domain_add_simple(), came in commit > commit 2854d167cc545d0642277bf8b77f972a91146fc6 Ah, thanks! I was puzzled how and why the approach above had been extended like now. This explains it. Okay, I will introduce irq_domain_create_simple().
On Thu, Mar 4, 2021 at 1:25 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Mar 04, 2021 at 09:06:08AM +0100, Linus Walleij wrote: > > On Wed, Mar 3, 2021 at 10:35 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Wed, Mar 03, 2021 at 10:22:02AM +0100, Linus Walleij wrote: > > > > > > But this: > > > > > > > > > @@ -1504,15 +1497,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, > > > > > return ret; > > > > > } else { > > > > > /* Some drivers provide custom irqdomain ops */ > > > > > - if (gc->irq.domain_ops) > > > > > - ops = gc->irq.domain_ops; > > > > > - > > > > > - if (!ops) > > > > > - ops = &gpiochip_domain_ops; > > > > > - gc->irq.domain = irq_domain_add_simple(np, > > > > > - gc->ngpio, > > > > > - gc->irq.first, > > > > > - ops, gc); > > > > > + ops = gc->irq.domain_ops ?: &gpiochip_domain_ops; > > > > > + if (gc->irq.first) > > > > > + gc->irq.domain = irq_domain_create_legacy(fwnode, gc->ngpio, > > > > > + gc->irq.first, 0, > > > > > + ops, gc); > > > > > + else > > > > > + gc->irq.domain = irq_domain_create_linear(fwnode, gc->ngpio, > > > > > + ops, gc); > > > > > > > > This looks like a refactoring and reimplementation of irq_domain_add_simple()? > > > > > > If you named it as irq_domain_create_simple(), then yes, but the problem is > > > that we don't have irq_domain_create_simple() API right now. > > > > > > > Why, and should it rather be a separate patch? > > > > > > Nope. > > > > OK I looked closer at irq_domain_add_simple(), and what it does different > > is to call irq_alloc_descs() for all lines if using sparse IRQs and then > > associate them. irq_domain_create_linear|legacy() does not allocate IRQ > > descriptors because it assumes something like DT or ACPI will do that > > on-demand when drivers request IRQs. > > > > This may be dangerous because some old platforms do not resolve IRQs > > at runtime and you will get NULL pointer exceptions. > > > > We then need to make sure all callers do what is done in e.g. > > drivers/gpio/gpio-omap.c in the #ifdef CONFIG_ARCH_OMAP1 clause: > > they need to be augmented to call irq_alloc_descs() explicitly, > > and I don't think all of them do it as nicely for us as OMAP1. > > > > I might be overly cautious though, however that is why this code > > uses irq_domain_add_simple(), came in commit > > commit 2854d167cc545d0642277bf8b77f972a91146fc6 > > Ah, thanks! I was puzzled how and why the approach above had been extended like > now. This explains it. Okay, I will introduce irq_domain_create_simple(). OK So please resend the series with that done and with the R-bys from Linus added. I'll apply it from Patchwork. Thanks!
On Thu, Mar 04, 2021 at 02:41:24PM +0100, Rafael J. Wysocki wrote: > On Thu, Mar 4, 2021 at 1:25 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Thu, Mar 04, 2021 at 09:06:08AM +0100, Linus Walleij wrote: > > > On Wed, Mar 3, 2021 at 10:35 AM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Wed, Mar 03, 2021 at 10:22:02AM +0100, Linus Walleij wrote: > > > > > > > > But this: > > > > > > > > > > > @@ -1504,15 +1497,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, > > > > > > return ret; > > > > > > } else { > > > > > > /* Some drivers provide custom irqdomain ops */ > > > > > > - if (gc->irq.domain_ops) > > > > > > - ops = gc->irq.domain_ops; > > > > > > - > > > > > > - if (!ops) > > > > > > - ops = &gpiochip_domain_ops; > > > > > > - gc->irq.domain = irq_domain_add_simple(np, > > > > > > - gc->ngpio, > > > > > > - gc->irq.first, > > > > > > - ops, gc); > > > > > > + ops = gc->irq.domain_ops ?: &gpiochip_domain_ops; > > > > > > + if (gc->irq.first) > > > > > > + gc->irq.domain = irq_domain_create_legacy(fwnode, gc->ngpio, > > > > > > + gc->irq.first, 0, > > > > > > + ops, gc); > > > > > > + else > > > > > > + gc->irq.domain = irq_domain_create_linear(fwnode, gc->ngpio, > > > > > > + ops, gc); > > > > > > > > > > This looks like a refactoring and reimplementation of irq_domain_add_simple()? > > > > > > > > If you named it as irq_domain_create_simple(), then yes, but the problem is > > > > that we don't have irq_domain_create_simple() API right now. > > > > > > > > > Why, and should it rather be a separate patch? > > > > > > > > Nope. > > > > > > OK I looked closer at irq_domain_add_simple(), and what it does different > > > is to call irq_alloc_descs() for all lines if using sparse IRQs and then > > > associate them. irq_domain_create_linear|legacy() does not allocate IRQ > > > descriptors because it assumes something like DT or ACPI will do that > > > on-demand when drivers request IRQs. > > > > > > This may be dangerous because some old platforms do not resolve IRQs > > > at runtime and you will get NULL pointer exceptions. > > > > > > We then need to make sure all callers do what is done in e.g. > > > drivers/gpio/gpio-omap.c in the #ifdef CONFIG_ARCH_OMAP1 clause: > > > they need to be augmented to call irq_alloc_descs() explicitly, > > > and I don't think all of them do it as nicely for us as OMAP1. > > > > > > I might be overly cautious though, however that is why this code > > > uses irq_domain_add_simple(), came in commit > > > commit 2854d167cc545d0642277bf8b77f972a91146fc6 > > > > Ah, thanks! I was puzzled how and why the approach above had been extended like > > now. This explains it. Okay, I will introduce irq_domain_create_simple(). > > OK > > So please resend the series with that done and with the R-bys from > Linus added. I'll apply it from Patchwork. Done! https://lore.kernel.org/linux-gpio/20210304150215.80652-1-andriy.shevchenko@linux.intel.com/T/#u P.S. you seems haven't switched yet to b4 :-)
On Thu, Mar 4, 2021 at 4:40 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Mar 04, 2021 at 02:41:24PM +0100, Rafael J. Wysocki wrote: > > On Thu, Mar 4, 2021 at 1:25 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Thu, Mar 04, 2021 at 09:06:08AM +0100, Linus Walleij wrote: > > > > On Wed, Mar 3, 2021 at 10:35 AM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > On Wed, Mar 03, 2021 at 10:22:02AM +0100, Linus Walleij wrote: > > > > > > > > > > But this: > > > > > > > > > > > > > @@ -1504,15 +1497,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, > > > > > > > return ret; > > > > > > > } else { > > > > > > > /* Some drivers provide custom irqdomain ops */ > > > > > > > - if (gc->irq.domain_ops) > > > > > > > - ops = gc->irq.domain_ops; > > > > > > > - > > > > > > > - if (!ops) > > > > > > > - ops = &gpiochip_domain_ops; > > > > > > > - gc->irq.domain = irq_domain_add_simple(np, > > > > > > > - gc->ngpio, > > > > > > > - gc->irq.first, > > > > > > > - ops, gc); > > > > > > > + ops = gc->irq.domain_ops ?: &gpiochip_domain_ops; > > > > > > > + if (gc->irq.first) > > > > > > > + gc->irq.domain = irq_domain_create_legacy(fwnode, gc->ngpio, > > > > > > > + gc->irq.first, 0, > > > > > > > + ops, gc); > > > > > > > + else > > > > > > > + gc->irq.domain = irq_domain_create_linear(fwnode, gc->ngpio, > > > > > > > + ops, gc); > > > > > > > > > > > > This looks like a refactoring and reimplementation of irq_domain_add_simple()? > > > > > > > > > > If you named it as irq_domain_create_simple(), then yes, but the problem is > > > > > that we don't have irq_domain_create_simple() API right now. > > > > > > > > > > > Why, and should it rather be a separate patch? > > > > > > > > > > Nope. > > > > > > > > OK I looked closer at irq_domain_add_simple(), and what it does different > > > > is to call irq_alloc_descs() for all lines if using sparse IRQs and then > > > > associate them. irq_domain_create_linear|legacy() does not allocate IRQ > > > > descriptors because it assumes something like DT or ACPI will do that > > > > on-demand when drivers request IRQs. > > > > > > > > This may be dangerous because some old platforms do not resolve IRQs > > > > at runtime and you will get NULL pointer exceptions. > > > > > > > > We then need to make sure all callers do what is done in e.g. > > > > drivers/gpio/gpio-omap.c in the #ifdef CONFIG_ARCH_OMAP1 clause: > > > > they need to be augmented to call irq_alloc_descs() explicitly, > > > > and I don't think all of them do it as nicely for us as OMAP1. > > > > > > > > I might be overly cautious though, however that is why this code > > > > uses irq_domain_add_simple(), came in commit > > > > commit 2854d167cc545d0642277bf8b77f972a91146fc6 > > > > > > Ah, thanks! I was puzzled how and why the approach above had been extended like > > > now. This explains it. Okay, I will introduce irq_domain_create_simple(). > > > > OK > > > > So please resend the series with that done and with the R-bys from > > Linus added. I'll apply it from Patchwork. > > Done! Thanks. > https://lore.kernel.org/linux-gpio/20210304150215.80652-1-andriy.shevchenko@linux.intel.com/T/#u > > P.S. you seems haven't switched yet to b4 :-) Nope.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 6827736ba05c..54cfea4e4a03 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1457,9 +1457,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, struct lock_class_key *lock_key, struct lock_class_key *request_key) { + struct fwnode_handle *fwnode = dev_fwnode(&gc->gpiodev->dev); struct irq_chip *irqchip = gc->irq.chip; - const struct irq_domain_ops *ops = NULL; - struct device_node *np; + const struct irq_domain_ops *ops; unsigned int type; unsigned int i; @@ -1471,7 +1471,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, return -EINVAL; } - np = gc->gpiodev->dev.of_node; type = gc->irq.default_type; /* @@ -1479,16 +1478,10 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, * used to configure the interrupts, as you may end up with * conflicting triggers. Tell the user, and reset to NONE. */ - if (WARN(np && type != IRQ_TYPE_NONE, - "%s: Ignoring %u default trigger\n", np->full_name, type)) + if (WARN(fwnode && type != IRQ_TYPE_NONE, + "%pfw: Ignoring %u default trigger\n", fwnode, type)) type = IRQ_TYPE_NONE; - if (has_acpi_companion(gc->parent) && type != IRQ_TYPE_NONE) { - acpi_handle_warn(ACPI_HANDLE(gc->parent), - "Ignoring %u default trigger\n", type); - type = IRQ_TYPE_NONE; - } - if (gc->to_irq) chip_warn(gc, "to_irq is redefined in %s and you shouldn't rely on it\n", __func__); @@ -1504,15 +1497,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, return ret; } else { /* Some drivers provide custom irqdomain ops */ - if (gc->irq.domain_ops) - ops = gc->irq.domain_ops; - - if (!ops) - ops = &gpiochip_domain_ops; - gc->irq.domain = irq_domain_add_simple(np, - gc->ngpio, - gc->irq.first, - ops, gc); + ops = gc->irq.domain_ops ?: &gpiochip_domain_ops; + if (gc->irq.first) + gc->irq.domain = irq_domain_create_legacy(fwnode, gc->ngpio, + gc->irq.first, 0, + ops, gc); + else + gc->irq.domain = irq_domain_create_linear(fwnode, gc->ngpio, + ops, gc); if (!gc->irq.domain) return -EINVAL; }
When IRQ domain is created for an ACPI case, the name of it becomes unknown-%d since for now it utilizes of_node member only and doesn't consider fwnode case. Convert IRQ domain creation code to utilize fwnode instead. Before/After the change on Intel Galileo Gen 2 with two GPIO (IRQ) controllers: unknown-1 ==> \_SB.PCI0.GIP0.GPO unknown-2 ==> \_SB.NIO3 Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpiolib.c | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-)