gpio: dwapb: Call acpi_gpiochip_free_interrupts() on GPIO chip de-registration

PATCH Superseded Andy Shevchenko 3 participants 8 messages May 18, 2020 – May 19, 2020

Subsystems (3)

GPIO Multifunction devices (MFD) Synopsys DesignWare apb GPIO driver

lore

Share & tools

lore

Andy Shevchenko

author

2020-05-18 18:29 UTC #0 lore raw

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

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 <[email protected]>

Cc Serge Semin <[email protected]>

---
v2: made it 1st in the series, add Fixes (Serge)

 drivers/gpio/gpio-dwapb.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

drivers/gpio/gpio-dwapb.c +16 -9

index 8639c4a7f469..e5d844304f8d 100644
@@ -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,

Signature · 1 line

2.26.2

Andy Shevchenko

author

2020-05-18 17:41 UTC #1 lore raw

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

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 <[email protected]>

Reviewed-by Serge Semin <[email protected]>

---
v2: added Rb tag (Serge)

 drivers/gpio/gpio-dwapb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpio/gpio-dwapb.c +1 -1

index e5d844304f8d..944dae80d687 100644
@@ -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;

Signature · 1 line

2.26.2

Andy Shevchenko

author

2020-05-18 18:29 UTC #2 lore raw

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

In reply to 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 <[email protected]>

Cc Serge Semin <[email protected]>

---
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(-)

drivers/gpio/gpio-dwapb.c +6 -6

index 944dae80d687..c939afac44c9 100644
@@ -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;

Signature · 1 line

2.26.2

Andy Shevchenko

author

2020-05-18 18:10 UTC #3 lore raw

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

Quoted 4 lines · Andy Shevchenko

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.

Quoted 1 line

pp->has_irq = true;

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

author

2020-05-18 17:41 UTC #4 lore raw

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

In reply to 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 <[email protected]>

Cc Serge Semin <[email protected]>

Cc Lee Jones <[email protected]>

---
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(-)

drivers/gpio/gpio-dwapb.c +7 -7

index c939afac44c9..1d8d55bd63aa 100644
@@ -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)

drivers/mfd/intel_quark_i2c_gpio.c +0 -1

index 41326b48da55..84ca7902e1df 100644
@@ -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;

include/linux/platform_data/gpio-dwapb.h +0 -1

index 3c606c450d05..ff1be737bad6 100644
@@ -12,7 +12,6 @@ struct dwapb_port_property {
 	unsigned int	ngpio;
 	unsigned int	gpio_base;
 	int		irq[32];
-	bool		has_irq;
 	bool		irq_shared;
 };

Signature · 1 line

2.26.2

Andy Shevchenko

author

2020-05-18 18:08 UTC #5 lore raw

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

Quoted 5 lines · 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.

Quoted 8 lines

Signed-off-by Andy Shevchenko <[email protected]>
Cc Serge Semin <[email protected]>
Cc Lee Jones <[email protected]>
---
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.

Quoted 1 line

- moved memchr_inv() here (Serge)

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Lee Jones

Acked-by on v2 4/4

2020-05-19 06:33 UTC #6 lore raw

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

In reply to Andy Shevchenko

Quoted 8 lines · 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 <[email protected]>
Cc Serge Semin <[email protected]>
Cc Lee Jones <[email protected]>

Quoted 8 lines

---
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 <[email protected]>

Quoted 2 lines

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

Signature · 4 lines

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

Serge Semin

Acked-by Tested-by on v2 1/4

2020-05-19 11:49 UTC #7 lore raw

In reply to Andy Shevchenko

Quoted 10 lines · 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 <[email protected]>
Cc Serge Semin <[email protected]>

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 <[email protected]>

Tested-by Serge Semin <[email protected]>

-Sergey

Quoted 55 lines

---
v2: made it 1st in the series, add Fixes (Serge)

 drivers/gpio/gpio-dwapb.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

drivers/gpio/gpio-dwapb.c +16 -9

index 8639c4a7f469..e5d844304f8d 100644
@@ -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,

Signature · 1 line

2.26.2