| Message ID | 20210304150215.80652-6-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Thu, Mar 4, 2021 at 4:02 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> > --- > drivers/gpio/gpiolib.c | 28 ++++++++-------------------- > 1 file changed, 8 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 6827736ba05c..254d59b088fe 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; > - } Why is the above message not worth printing any more? If there is a good enough reason, it would be good to mention it in the changelog. > - > 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,10 @@ 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; I'm guessing that the code above is replaced in order to avoid initializing ops to NULL, but IMO that should be a separate patch or at least the extra cleanup should be mentioned in the changelog. Personally, I would do the essential change first and put all of the tangentially related cleanups into a separate follow-up patch. > - gc->irq.domain = irq_domain_add_simple(np, > - gc->ngpio, > - gc->irq.first, > - ops, gc); > + ops = gc->irq.domain_ops ?: &gpiochip_domain_ops; > + gc->irq.domain = irq_domain_create_simple(fwnode, gc->ngpio, > + gc->irq.first, > + ops, gc); > if (!gc->irq.domain) > return -EINVAL; > } > --
On Mon, Mar 08, 2021 at 07:32:47PM +0100, Rafael J. Wysocki wrote: > On Thu, Mar 4, 2021 at 4:02 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 Thanks for review! I'm wondering why you commented against v2 instead of v3... I assume it's just a typo while the comments themselves are applicable to v3. Hence my reply below. ... > > - 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; > > - } > > Why is the above message not worth printing any more? If there is a > good enough reason, it would be good to mention it in the changelog. The reason is good enough, we drop duplicated printing since we got fwnode in either case DT or ACPI. Basically this part is unification and due to nature of the whole change it can't be done separately. I will do in v4. ... > > /* Some drivers provide custom irqdomain ops */ > > - if (gc->irq.domain_ops) > > - ops = gc->irq.domain_ops; > > - > > - if (!ops) > > - ops = &gpiochip_domain_ops; > > I'm guessing that the code above is replaced in order to avoid > initializing ops to NULL, but IMO that should be a separate patch or > at least the extra cleanup should be mentioned in the changelog. > > Personally, I would do the essential change first and put all of the > tangentially related cleanups into a separate follow-up patch. Okay, I will split it to a separate clean up in v4. ... While at it, can you then provide an immutable tag / branch that Bart and I may inject into our repos?
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 6827736ba05c..254d59b088fe 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,10 @@ 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; + gc->irq.domain = irq_domain_create_simple(fwnode, gc->ngpio, + gc->irq.first, + 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 | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-)