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

[Serge Semin <fancer.lancer@xxxxxxxxx>]

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:
> > There is no need to have an additional check to call
> > acpi_gpiochip_request_interrupts(). Even without any interrupts available
> > the registered ACPI Event handlers can be useful for debugging purposes.
> > 
> > While at it, add missed acpi_gpiochip_free_interrupts() call when
> > unregistering ports.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Cc: Serge Semin <fancer.lancer@xxxxxxxxx>
> > ---

[nip]

> > -	if (pp->has_irq)
> > -		acpi_gpiochip_request_interrupts(&port->gc);
> > +	acpi_gpiochip_request_interrupts(&port->gc);
> 
> Hm, perhaps replacing it with:
> +	if (pp->idx == 0)
> +		acpi_gpiochip_request_interrupts(&port->gc);
> could be more appropriate seeing Port A only supports IRQs, which we'd point
> out by the (idx == 0) conditional statement. So we don't have to call
> the method at most four times for each available port. Though judging by the
> acpi_gpiochip_request_interrupts() function internals it will just ignore
> GPIO chips with no IRQ support. Andy, It's up to you to decide. I'm not against
> the change the way it is, but if you agree that signifying the IRQs affiliation
> would be better, then please fill free to add the conditional statement I
> suggested.
> 

Please see my comment below, before you decide what to do with this part of the
patch.

[nip]

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

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)
gpio: dwapb: Remove redundant has_irq flag support

-Sergey