Re: [PATCH v1 3/4] gpio: dwapb: Drop extra check to call acpi_gpiochip_request_interrupts()

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Sun, May 17, 2020 at 05:47:37PM +0300, Serge Semin wrote:
> On Sun, May 17, 2020 at 04:55:24PM +0300, Serge Semin wrote:
> > On Tue, May 12, 2020 at 09:45:12PM +0300, Andy Shevchenko wrote:

...

> > >  static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
> > >  {
> > >  	unsigned int m;
> > >  
> > > -	for (m = 0; m < gpio->nr_ports; ++m)
> > > -		if (gpio->ports[m].is_registered)
> > > -			gpiochip_remove(&gpio->ports[m].gc);
> > > +	for (m = 0; m < gpio->nr_ports; ++m) {
> > > +		struct dwapb_gpio_port *port = &gpio->ports[m];
> > > +
> > > +		if (!port->is_registered)
> > > +			continue;
> > > +
> > > +		acpi_gpiochip_free_interrupts(&port->gc);
> > > +		gpiochip_remove(&port->gc);
> > > +	}
> > >  }
> > 
> > Could you please move this change to a dedicated patch? It seems to me this
> > alteration might be appropriate to be ported to the stable kernels seeing it
> > fixes e6cb3486f5a1 ("gpio: dwapb: add gpio-signaled acpi event support").
> > Linus, what do you think?
> > 
> > -Sergey
> > 
> 
> BTW after moving the change with acpi_gpiochip_free_interrupts() into a
> dedicated patch, you can freely merge the rest of this patch into the
> last one of this series. So the has_irq flag cleanup would be performed in a
> single commit. Especially if you implement the comment I provided above regarding
> conditional (idx == 0) calling of the acpi_gpiochip_request_interrupts() method.

I was thinking about this split and came to the conclusion that it will be a
bit awkward to introduce additional check in the ->dwapb_gpio_unregister() for
freeing ACPI Event handling which will be removed by one of following patch
(even taking into consideration backporting).

What I propose here is an alternative, i.e. I make this patch as first in the
series and will focus / dedicate it as a fix rather than clean up.

> So your series will look like this:
> gpio: dwapb: avoid error message for optional IRQ
> gpio: dwapb: Don't use 0 as valid Linux interrupt number

> gpio: dwapb: Call acpi_gpiochip_free_interrupts() on GPIO chip de-registration (<= This commit can be moved to the head of the series as being marked by the
> Fixes tag)

Yes, something like this, but keeping this form of the patch.

Thanks for the suggestion!

> gpio: dwapb: Remove redundant has_irq flag support

-- 
With Best Regards,
Andy Shevchenko