linux-kernel - Re: [PATCH v1 1/4] gpio: dwapb: Unify ACPI enumeration checks in get_irq() and configure

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <YQg7hNKlUlkz/fkv@smile.fi.intel.com>
Date:   Mon, 2 Aug 2021 21:37:56 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Serge Semin <fancer.lancer@...il.com>
Cc:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Lee Jones <lee.jones@...aro.org>, linux-kernel@...r.kernel.org,
        linux-gpio@...r.kernel.org,
        Hoan Tran <hoan@...amperecomputing.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>
Subject: Re: [PATCH v1 1/4] gpio: dwapb: Unify ACPI enumeration checks in
 get_irq() and configure_irqs()

On Mon, Aug 02, 2021 at 04:40:21PM +0300, Serge Semin wrote:
> Hello Andy
> Thanks for the cleanup series. A tiny note is below.

Thanks for review!

> On Mon, Jul 26, 2021 at 03:54:33PM +0300, Andy Shevchenko wrote:
> > Shared IRQ is only enabled for ACPI enumeration, there is no need
> > to have a special flag for that, since we simple can test if device
> > has been enumerated by ACPI. This unifies the checks in dwapb_get_irq()
> > and dwapb_configure_irqs().
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > ---
> >  drivers/gpio/gpio-dwapb.c                | 13 ++++++-------
> >  drivers/mfd/intel_quark_i2c_gpio.c       |  1 -
> >  include/linux/platform_data/gpio-dwapb.h |  1 -
> >  3 files changed, 6 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> > index 3eb13d6d31ef..f6ae69d5d644 100644
> > --- a/drivers/gpio/gpio-dwapb.c
> > +++ b/drivers/gpio/gpio-dwapb.c
> > @@ -436,12 +436,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
> >  	pirq->irqchip.irq_set_wake = dwapb_irq_set_wake;
> >  #endif
> >  
> 
> > -	if (!pp->irq_shared) {
> > -		girq->num_parents = pirq->nr_irqs;
> > -		girq->parents = pirq->irq;
> > -		girq->parent_handler_data = gpio;
> > -		girq->parent_handler = dwapb_irq_handler;
> > -	} else {
> > +	if (has_acpi_companion(gpio->dev)) {
> 
> Before this patch the platform flag irq_shared has been as kind of a
> hint regarding the shared IRQ case being covered here. But now it
> doesn't seem obvious why we've got the ACPI and ACPI-less cases
> differently handled. What about adding a small comment about that?
> E.g. like this: "Intel ACPI-based platforms mostly have the DW APB
> GPIO IRQ lane shared between several devices. In that case the
> parental IRQ has to be handled in the shared way so to be properly
> delivered to all the connected devices." or something more detailed
> for your preference. After that the rest of the comments in the
> if-clause could be discarded.

Sure!


-- 
With Best Regards,
Andy Shevchenko