[PATCH v2 1/4] gpio: dwapb: Call acpi_gpiochip_free_interrupts() on GPIO chip de-registration

[PATCH v2 1/4] gpio: dwapb: Call acpi_gpiochip_free_interrupts() on GPIO chip de-registration

[Andy Shevchenko]

Add missed acpi_gpiochip_free_interrupts() call when unregistering ports.

While at it, drop extra check to call acpi_gpiochip_request_interrupts().
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.

Fixes: e6cb3486f5a1 ("gpio: dwapb: add gpio-signaled acpi event support")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Serge Semin <fancer.lancer@gmail.com>
---
v2: made it 1st in the series, add Fixes (Serge)
 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 8639c4a7f469..e5d844304f8d 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);
 
-	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);
+	}
 }
 
 static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
-- 
2.26.2

[PATCH v2 2/4] gpio: dwapb: avoid error message for optional IRQ

[Andy Shevchenko]

platform_get_irq() will generate an error message if the requested IRQ
is not present. Use platform_get_irq_optional() to avoid the error message
being generated.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
---
v2: added Rb tag (Serge)
 drivers/gpio/gpio-dwapb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index e5d844304f8d..944dae80d687 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -549,7 +549,7 @@ static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
 		if (np)
 			pp->irq[j] = of_irq_get(np, j);
 		else if (has_acpi_companion(dev))
-			pp->irq[j] = platform_get_irq(to_platform_device(dev), j);
+			pp->irq[j] = platform_get_irq_optional(to_platform_device(dev), j);
 
 		if (pp->irq[j] >= 0)
 			pp->has_irq = true;
-- 
2.26.2

[PATCH v2 3/4] gpio: dwapb: Don't use 0 as valid Linux interrupt number

[Andy Shevchenko]

0 is not valid IRQ number in Linux interrupt number space.
Refactor the code with this kept in mind.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Serge Semin <fancer.lancer@gmail.com>
---
v2: drop memchr_inv() use by moving it to the patch which needs it (Serge)
 drivers/gpio/gpio-dwapb.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 944dae80d687..c939afac44c9 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -417,7 +417,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 		int i;
 
 		for (i = 0; i < pp->ngpio; i++) {
-			if (pp->irq[i] >= 0)
+			if (pp->irq[i])
 				irq_set_chained_handler_and_data(pp->irq[i],
 						dwapb_irq_handler, gpio);
 		}
@@ -538,18 +538,18 @@ static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
 			  struct dwapb_port_property *pp)
 {
 	struct device_node *np = NULL;
-	int j;
+	int irq = -ENXIO, j;
 
 	if (fwnode_property_read_bool(fwnode, "interrupt-controller"))
 		np = to_of_node(fwnode);
 
 	for (j = 0; j < pp->ngpio; j++) {
-		pp->irq[j] = -ENXIO;
-
 		if (np)
-			pp->irq[j] = of_irq_get(np, j);
+			irq = of_irq_get(np, j);
 		else if (has_acpi_companion(dev))
-			pp->irq[j] = platform_get_irq_optional(to_platform_device(dev), j);
+			irq = platform_get_irq_optional(to_platform_device(dev), j);
+		if (irq > 0)
+			pp->irq[j] = irq;
 
 		if (pp->irq[j] >= 0)
 			pp->has_irq = true;
-- 
2.26.2

[PATCH v2 4/4] gpio: dwapb: Remove unneeded has_irq member in struct dwapb_port_property

[Andy Shevchenko]

has_irq member of struct dwapb_port_property is used only in one place,
so, make it local test instead and remove from the structure.
This local test is using memchr_inv() which is quite effective in comparison
to the original loop and possible little overhead can be neglected.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Serge Semin <fancer.lancer@gmail.com>
Cc: Lee Jones <lee.jones@linaro.org>
---
v2:
 - fixed compilation error (kbuild bot)
 - added Lee into Cc
 - moved memchr_inv() here (Serge)
 drivers/gpio/gpio-dwapb.c                | 14 +++++++-------
 drivers/mfd/intel_quark_i2c_gpio.c       |  1 -
 include/linux/platform_data/gpio-dwapb.h |  1 -
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index c939afac44c9..1d8d55bd63aa 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -366,6 +366,11 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	irq_hw_number_t hwirq;
 	int err, i;
 
+	if (memchr_inv(pp->irq, 0, sizeof(pp->irq)) == NULL) {
+		dev_warn(gpio->dev, "no IRQ for port%d\n", pp->idx);
+		return;
+	}
+
 	gpio->domain = irq_domain_create_linear(fwnode, ngpio,
 						 &irq_generic_chip_ops, gpio);
 	if (!gpio->domain)
@@ -501,7 +506,8 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
 	if (pp->idx == 0)
 		port->gc.set_config = dwapb_gpio_set_config;
 
-	if (pp->has_irq)
+	/* Only port A can provide interrupts in all configurations of the IP */
+	if (pp->idx == 0)
 		dwapb_configure_irqs(gpio, port, pp);
 
 	err = gpiochip_add_data(&port->gc, port);
@@ -550,13 +556,7 @@ static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
 			irq = platform_get_irq_optional(to_platform_device(dev), j);
 		if (irq > 0)
 			pp->irq[j] = irq;
-
-		if (pp->irq[j] >= 0)
-			pp->has_irq = true;
 	}
-
-	if (!pp->has_irq)
-		dev_warn(dev, "no irq for port%d\n", pp->idx);
 }
 
 static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
index 41326b48da55..84ca7902e1df 100644
--- a/drivers/mfd/intel_quark_i2c_gpio.c
+++ b/drivers/mfd/intel_quark_i2c_gpio.c
@@ -216,7 +216,6 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
 	pdata->properties->ngpio	= INTEL_QUARK_MFD_NGPIO;
 	pdata->properties->gpio_base	= INTEL_QUARK_MFD_GPIO_BASE;
 	pdata->properties->irq[0]	= pdev->irq;
-	pdata->properties->has_irq	= true;
 	pdata->properties->irq_shared	= true;
 
 	cell->platform_data = pdata;
diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h
index 3c606c450d05..ff1be737bad6 100644
--- a/include/linux/platform_data/gpio-dwapb.h
+++ b/include/linux/platform_data/gpio-dwapb.h
@@ -12,7 +12,6 @@ struct dwapb_port_property {
 	unsigned int	ngpio;
 	unsigned int	gpio_base;
 	int		irq[32];
-	bool		has_irq;
 	bool		irq_shared;
 };
 
-- 
2.26.2

Re: [PATCH v2 4/4] gpio: dwapb: Remove unneeded has_irq member in struct dwapb_port_property

[Andy Shevchenko]

On Mon, May 18, 2020 at 08:41:38PM +0300, Andy Shevchenko wrote:
> has_irq member of struct dwapb_port_property is used only in one place,
> so, make it local test instead and remove from the structure.
> This local test is using memchr_inv() which is quite effective in comparison
> to the original loop and possible little overhead can be neglected.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Serge Semin <fancer.lancer@gmail.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> ---
> v2:
>  - fixed compilation error (kbuild bot)

>  - added Lee into Cc

Lee, JFYI, you have not been forgotten in the v1, this is due to missed change
in the MFD driver in v1 and since kbuild bot reported it this is the first
version which includes MFD changes.

>  - moved memchr_inv() here (Serge)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] gpio: dwapb: Don't use 0 as valid Linux interrupt number

[Andy Shevchenko]

On Mon, May 18, 2020 at 08:41:37PM +0300, Andy Shevchenko wrote:
> 0 is not valid IRQ number in Linux interrupt number space.
> Refactor the code with this kept in mind.

>  		if (pp->irq[j] >= 0)

Oops, missed this one to drop >= 0.
I'll wait for your and Lee's comments / tags for the rest and then send v3.

>  			pp->has_irq = true;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/4] gpio: dwapb: Remove unneeded has_irq member in struct dwapb_port_property

[Lee Jones]

On Mon, 18 May 2020, Andy Shevchenko wrote:

> has_irq member of struct dwapb_port_property is used only in one place,
> so, make it local test instead and remove from the structure.
> This local test is using memchr_inv() which is quite effective in comparison
> to the original loop and possible little overhead can be neglected.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Serge Semin <fancer.lancer@gmail.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> ---
> v2:
>  - fixed compilation error (kbuild bot)
>  - added Lee into Cc
>  - moved memchr_inv() here (Serge)
>  drivers/gpio/gpio-dwapb.c                | 14 +++++++-------

>  drivers/mfd/intel_quark_i2c_gpio.c       |  1 -

Acked-by: Lee Jones <lee.jones@linaro.org>

>  include/linux/platform_data/gpio-dwapb.h |  1 -
>  3 files changed, 7 insertions(+), 9 deletions(-)

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 1/4] gpio: dwapb: Call acpi_gpiochip_free_interrupts() on GPIO chip de-registration

[Serge Semin]

On Mon, May 18, 2020 at 08:41:35PM +0300, Andy Shevchenko wrote:
> Add missed acpi_gpiochip_free_interrupts() call when unregistering ports.
> 
> While at it, drop extra check to call acpi_gpiochip_request_interrupts().
> 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.
> 
> Fixes: e6cb3486f5a1 ("gpio: dwapb: add gpio-signaled acpi event support")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Serge Semin <fancer.lancer@gmail.com>

Andy, thanks for the series. After fixing patch 3, supposedly you were going to
to do it like this:
-               if (irq > 0)
-                       pp->irq[j] = irq;
-
-               if (pp->irq[j] >= 0)
-                       pp->has_irq = true;
+               if (irq > 0) {
+                       pp->irq[j] = irq;
+                       pp->has_irq = true;
+               }
or similar as you prefer, feel free to add my tag to the whole series:
Acked-by: Serge Semin <fancer.lancer@gmail.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>

-Sergey

> ---
> v2: made it 1st in the series, add Fixes (Serge)
>  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 8639c4a7f469..e5d844304f8d 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);
>  
> -	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);
> +	}
>  }
>  
>  static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
> -- 
> 2.26.2
>