gpiolib: Make gpiochip_hierarchy_add_domain() return domain

PATCH In review Andy Shevchenko 4 participants 13 messages Jun 21, 2023 – Jul 10, 2023 GPIO

Share & tools

Andy Shevchenko

author

2023-06-21 17:50 UTC #0 lore raw

PATCH 1/5 gpiolib: Make gpiochip_hierarchy_add_domain() return domain

As a preparatory patch and for the sake of consistency, make gpiochip_hierarchy_add_domain() return IRQ domain. While at it, rename it to gpiochip_hierarchy_create_domain() to show the change.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/gpio/gpiolib.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

drivers/gpio/gpiolib.c +13 ~~-11~~

index 0d25d9d439a2..058deaa1aa36 100644
@@ -1289,12 +1289,14 @@ static void gpiochip_hierarchy_setup_domain_ops(struct irq_domain_ops *ops)
 		ops->free = irq_domain_free_irqs_common;
 }
 
-static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
+static struct irq_domain *gpiochip_hierarchy_create_domain(struct gpio_chip *gc)
 {
+	struct irq_domain *domain;
+
 	if (!gc->irq.child_to_parent_hwirq ||
 	    !gc->irq.fwnode) {
 		chip_err(gc, "missing irqdomain vital data\n");
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	if (!gc->irq.child_offset_to_irq)

@@ -1306,7 +1308,7 @@ static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
 
 	gpiochip_hierarchy_setup_domain_ops(&gc->irq.child_irq_domain_ops);
 
-	gc->irq.domain = irq_domain_create_hierarchy(
+	domain = irq_domain_create_hierarchy(
 		gc->irq.parent_domain,
 		0,
 		gc->ngpio,

@@ -1314,12 +1316,12 @@ static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
 		&gc->irq.child_irq_domain_ops,
 		gc);
 
-	if (!gc->irq.domain)
-		return -ENOMEM;
+	if (!domain)
+		return ERR_PTR(-ENOMEM);
 
 	gpiochip_set_hierarchical_irqchip(gc, gc->irq.chip);
 
-	return 0;
+	return domain;
 }
 
 static bool gpiochip_hierarchy_is_hierarchical(struct gpio_chip *gc)

@@ -1363,9 +1365,9 @@ EXPORT_SYMBOL_GPL(gpiochip_populate_parent_fwspec_fourcell);
 
 #else
 
-static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
+static struct irq_domain *gpiochip_hierarchy_create_domain(struct gpio_chip *gc)
 {
-	return -EINVAL;
+	return ERR_PTR(-EINVAL);
 }
 
 static bool gpiochip_hierarchy_is_hierarchical(struct gpio_chip *gc)

@@ -1664,9 +1666,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
 
 	/* If a parent irqdomain is provided, let's build a hierarchy */
 	if (gpiochip_hierarchy_is_hierarchical(gc)) {
-		int ret = gpiochip_hierarchy_add_domain(gc);
-		if (ret)
-			return ret;
+		gc->irq.domain = gpiochip_hierarchy_create_domain(gc);
+		if (IS_ERR(gc->irq.domain))
+			return PTR_ERR(gc->irq.domain);
 	} else {
 		gc->irq.domain = irq_domain_create_simple(fwnode,
 			gc->ngpio,

Signature · 1 line

2.40.0.1.gaa8946217a0b

Andy Shevchenko

author

2023-06-21 17:49 UTC #1 lore raw

PATCH 2/5 gpiolib: Factor out gpiochip_simple_create_domain()

As a preparatory patch and for the sake of consistency, factor out gpiochip_simple_create_domain().

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/gpio/gpiolib.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

drivers/gpio/gpiolib.c +16 -7

index 058deaa1aa36..fac1124d5016 100644
@@ -1444,6 +1444,19 @@ static const struct irq_domain_ops gpiochip_domain_ops = {
 	.xlate	= irq_domain_xlate_twocell,
 };
 
+static struct irq_domain *gpiochip_simple_create_domain(struct gpio_chip *gc)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(&gc->gpiodev->dev);
+	struct irq_domain *domain;
+
+	domain = irq_domain_create_simple(fwnode, gc->ngpio, gc->irq.first,
+					  &gpiochip_domain_ops, gc);
+	if (!domain)
+		return ERR_PTR(-EINVAL);
+
+	return domain;
+}
+
 /*
  * TODO: move these activate/deactivate in under the hierarchicial
  * irqchip implementation as static once SPMI and SSBI (all external

@@ -1670,13 +1683,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
 		if (IS_ERR(gc->irq.domain))
 			return PTR_ERR(gc->irq.domain);
 	} else {
-		gc->irq.domain = irq_domain_create_simple(fwnode,
-			gc->ngpio,
-			gc->irq.first,
-			&gpiochip_domain_ops,
-			gc);
-		if (!gc->irq.domain)
-			return -EINVAL;
+		gc->irq.domain = gpiochip_simple_create_domain(gc);
+		if (IS_ERR(gc->irq.domain))
+			return PTR_ERR(gc->irq.domain);
 	}
 
 	if (gc->irq.parent_handler) {

Signature · 1 line

2.40.0.1.gaa8946217a0b

Linus Walleij

Reviewed-by on v1 2/5

2023-06-30 11:48 UTC #2 lore raw

Re: [PATCH v1 2/5] gpiolib: Factor out gpiochip_simple_create_domain()

Quoted 4 lines · Andy Shevchenko

As a preparatory patch and for the sake of consistency, factor out gpiochip_simple_create_domain().
Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Andy Shevchenko

author

2023-06-21 17:50 UTC #3 lore raw

PATCH 3/5 gpiolib: Do not assign error pointer to the GPIO IRQ chip domain

In reply to Andy Shevchenko

Check domain for being an error pointer before assigning it to the GPIO IRQ chip domain.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/gpio/gpiolib.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/gpio/gpiolib.c +6 -6

index fac1124d5016..0e40f9a44519 100644
@@ -1647,6 +1647,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
 {
 	struct fwnode_handle *fwnode = dev_fwnode(&gc->gpiodev->dev);
 	struct irq_chip *irqchip = gc->irq.chip;
+	struct irq_domain *domain;
 	unsigned int type;
 	unsigned int i;

@@ -1679,14 +1680,13 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
 
 	/* If a parent irqdomain is provided, let's build a hierarchy */
 	if (gpiochip_hierarchy_is_hierarchical(gc)) {
-		gc->irq.domain = gpiochip_hierarchy_create_domain(gc);
-		if (IS_ERR(gc->irq.domain))
-			return PTR_ERR(gc->irq.domain);
+		domain = gpiochip_hierarchy_create_domain(gc);
 	} else {
-		gc->irq.domain = gpiochip_simple_create_domain(gc);
-		if (IS_ERR(gc->irq.domain))
-			return PTR_ERR(gc->irq.domain);
+		domain = gpiochip_simple_create_domain(gc);
 	}
+	if (IS_ERR(domain))
+		return PTR_ERR(domain);
+	gc->irq.domain = domain;
 
 	if (gc->irq.parent_handler) {
 		for (i = 0; i < gc->irq.num_parents; i++) {

Signature · 1 line

2.40.0.1.gaa8946217a0b

Linus Walleij

Reviewed-by on v1 3/5

2023-06-30 11:48 UTC #4 lore raw

Re: [PATCH v1 3/5] gpiolib: Do not assign error pointer to the GPIO IRQ chip domain

Quoted 4 lines · Andy Shevchenko

Check domain for being an error pointer before assigning it to the GPIO IRQ chip domain.
Signed-off-by Andy Shevchenko <[email protected]>

And this concludes patches 1,2,3, nice!

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Andy Shevchenko

author

2023-06-21 17:49 UTC #5 lore raw

PATCH 4/5 gpiolib: Split out gpiochip_irqchip_add_allocated_domain() helper

In reply to Andy Shevchenko

The gpiochip_irqchip_add_allocated_domain() can be used in another place in the code.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/gpio/gpiolib.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

drivers/gpio/gpiolib.c +23 ~~-16~~

index 0e40f9a44519..59d87e60b108 100644
@@ -1635,6 +1635,28 @@ static void gpiochip_set_irq_hooks(struct gpio_chip *gc)
 	}
 }
 
+static int gpiochip_irqchip_add_allocated_domain(struct gpio_chip *gc,
+						 struct irq_domain *domain,
+						 bool allocated_externally)
+{
+	if (!domain)
+		return -EINVAL;
+
+	gc->to_irq = gpiochip_to_irq;
+	gc->irq.domain = domain;
+	gc->irq.domain_is_allocated_externally = allocated_externally;
+
+	/*
+	 * Using barrier() here to prevent compiler from reordering
+	 * gc->irq.initialized before adding irqdomain.
+	 */
+	barrier();
+
+	gc->irq.initialized = true;
+
+	return 0;
+}
+
 /**
  * gpiochip_add_irqchip() - adds an IRQ chip to a GPIO chip
  * @gc: the GPIO chip to add the IRQ chip to

@@ -1788,22 +1810,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gc)
 int gpiochip_irqchip_add_domain(struct gpio_chip *gc,
 				struct irq_domain *domain)
 {
-	if (!domain)
-		return -EINVAL;
-
-	gc->to_irq = gpiochip_to_irq;
-	gc->irq.domain = domain;
-	gc->irq.domain_is_allocated_externally = true;
-
-	/*
-	 * Using barrier() here to prevent compiler from reordering
-	 * gc->irq.initialized before adding irqdomain.
-	 */
-	barrier();
-
-	gc->irq.initialized = true;
-
-	return 0;
+	return gpiochip_irqchip_add_allocated_domain(gc, domain, true);
 }
 EXPORT_SYMBOL_GPL(gpiochip_irqchip_add_domain);

Signature · 1 line

2.40.0.1.gaa8946217a0b

Linus Walleij

Reviewed-by on v1 4/5

2023-06-30 11:51 UTC #6 lore raw

Re: [PATCH v1 4/5] gpiolib: Split out gpiochip_irqchip_add_allocated_domain() helper

Quoted 4 lines · Andy Shevchenko

The gpiochip_irqchip_add_allocated_domain() can be used in another place in the code.
Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Andy Shevchenko

author

2023-06-21 17:49 UTC #7 lore raw

PATCH 5/5 gpiolib: Replace open coded gpiochip_irqchip_add_allocated_domain()

In reply to Andy Shevchenko

Replace open coded variant of gpiochip_irqchip_add_allocated_domain() in gpiochip_add_irqchip().

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/gpio/gpiolib.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

drivers/gpio/gpiolib.c +7 ~~-13~~

index 59d87e60b108..bc8b9d6afe0e 100644
@@ -1642,6 +1642,9 @@ static int gpiochip_irqchip_add_allocated_domain(struct gpio_chip *gc,
 	if (!domain)
 		return -EINVAL;
 
+	if (gc->to_irq)
+		chip_warn(gc, "to_irq is redefined in %s and you shouldn't rely on it\n", __func__);
+
 	gc->to_irq = gpiochip_to_irq;
 	gc->irq.domain = domain;
 	gc->irq.domain_is_allocated_externally = allocated_externally;

@@ -1672,6 +1675,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
 	struct irq_domain *domain;
 	unsigned int type;
 	unsigned int i;
+	int ret;
 
 	if (!irqchip)
 		return 0;

@@ -1692,10 +1696,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
 		 "%pfw: Ignoring %u default trigger\n", fwnode, type))
 		type = IRQ_TYPE_NONE;
 
-	if (gc->to_irq)
-		chip_warn(gc, "to_irq is redefined in %s and you shouldn't rely on it\n", __func__);
-
-	gc->to_irq = gpiochip_to_irq;
 	gc->irq.default_type = type;
 	gc->irq.lock_key = lock_key;
 	gc->irq.request_key = request_key;

@@ -1708,7 +1708,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
 	}
 	if (IS_ERR(domain))
 		return PTR_ERR(domain);
-	gc->irq.domain = domain;
 
 	if (gc->irq.parent_handler) {
 		for (i = 0; i < gc->irq.num_parents; i++) {

@@ -1732,14 +1731,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
 
 	gpiochip_set_irq_hooks(gc);
 
-	/*
-	 * Using barrier() here to prevent compiler from reordering
-	 * gc->irq.initialized before initialization of above
-	 * GPIO chip irq members.
-	 */
-	barrier();
-
-	gc->irq.initialized = true;
+	ret = gpiochip_irqchip_add_allocated_domain(gc, domain, false);
+	if (ret)
+		return ret;
 
 	acpi_gpiochip_request_interrupts(gc);

Signature · 1 line

2.40.0.1.gaa8946217a0b

Linus Walleij

Reviewed-by on v1 5/5

2023-06-30 11:52 UTC #8 lore raw

Re: [PATCH v1 5/5] gpiolib: Replace open coded gpiochip_irqchip_add_allocated_domain()

Quoted 4 lines · Andy Shevchenko

Replace open coded variant of gpiochip_irqchip_add_allocated_domain() in gpiochip_add_irqchip().
Signed-off-by Andy Shevchenko <[email protected]>

and this concludes patches 4,5 very nicely as well.

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Andy Shevchenko

2023-06-30 13:58 UTC #9 lore raw

Re: [PATCH v1 5/5] gpiolib: Replace open coded gpiochip_irqchip_add_allocated_domain()

Quoted 10 lines · Linus Walleij

Quoted 4 lines · Andy Shevchenko
Replace open coded variant of gpiochip_irqchip_add_allocated_domain() in gpiochip_add_irqchip().
Signed-off-by Andy Shevchenko <[email protected]>
and this concludes patches 4,5 very nicely as well.

Yep!

Quoted 1 line

Reviewed-by Linus Walleij <[email protected]>

Thank you for the review!

While at it, I would like to ask on ->to_irq() callback. IIUC assigning it with an IRQ chip makes a dead code in the driver. Am I correct? If not, can somebody shed some light on how the RT5677 driver, for example, works with GPIO IRQ?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Linus Walleij

2023-07-03 22:26 UTC #10 lore raw

Re: [PATCH v1 5/5] gpiolib: Replace open coded gpiochip_irqchip_add_allocated_domain()

Quoted 3 lines · Andy Shevchenko

While at it, I would like to ask on ->to_irq() callback. IIUC assigning it with an IRQ chip makes a dead code in the driver. Am I correct?

It's fine to assign it with an IRQ chip but not with GPIOLIB_IRQCHIP, i.e. gc->irq better be NULL.

Quoted 2 lines

If not, can somebody shed some light on how the RT5677 driver, for example, works with GPIO IRQ?

That is theoretically fine (I don't know this HW in particular). It looks a bit fragile...

It's just a helper to translate a GPIO line to the corresponding Linux IRQ number and when using GPIOLIB_IRQCHIP the GPIO core will do this using the irqdomain, else it is up to the driver.

Yours,
Linus Walleij

Linus Walleij

Reviewed-by on v1 1/5

2023-06-30 11:47 UTC #11 lore raw

In reply to Andy Shevchenko

Quoted 6 lines · Andy Shevchenko

As a preparatory patch and for the sake of consistency, make gpiochip_hierarchy_add_domain() return IRQ domain. While at it, rename it to gpiochip_hierarchy_create_domain() to show the change.
Signed-off-by Andy Shevchenko <[email protected]>

Looks good!

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Bartosz Golaszewski

2023-07-10 09:33 UTC #12 lore raw

In reply to Andy Shevchenko

Quoted 8 lines · Andy Shevchenko

As a preparatory patch and for the sake of consistency, make gpiochip_hierarchy_add_domain() return IRQ domain. While at it, rename it to gpiochip_hierarchy_create_domain() to show the change.
Signed-off-by Andy Shevchenko <[email protected]>
---

Series applied, thanks!

Bart