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

[Serge Semin <fancer.lancer@xxxxxxxxx>]

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>
> ---
>  drivers/gpio/gpio-dwapb.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 78662d6d73634e..2975f2d369874a 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -505,26 +505,33 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
>  		dwapb_configure_irqs(gpio, port, pp);
>  
>  	err = gpiochip_add_data(&port->gc, port);
> -	if (err)
> +	if (err) {
>  		dev_err(gpio->dev, "failed to register gpiochip for port%d\n",
>  			port->idx);
> -	else
> -		port->is_registered = true;
> +		return err;
> +	}
>  
>  	/* Add GPIO-signaled ACPI event support */

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

>  
> -	return err;
> +	port->is_registered = true;
> +
> +	return 0;
>  }
>  

>  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

>  
>  static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
> -- 
> 2.26.2
>