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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 8 Mar 2021 20:52:00 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Marc Zyngier <maz@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v2 5/5] gpiolib: Reuse device's fwnode to create IRQ
 domain

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@...ux.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