Re: [PATCH v2 5/5] gpiolib: Reuse device's fwnode to create IRQ domain

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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?

-- 
With Best Regards,
Andy Shevchenko