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

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

[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 <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0d25d9d439a2..058deaa1aa36 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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,
-- 
2.40.0.1.gaa8946217a0b

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

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 058deaa1aa36..fac1124d5016 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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) {
-- 
2.40.0.1.gaa8946217a0b

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

[Linus Walleij]

On Wed, Jun 21, 2023 at 7:49 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> As a preparatory patch and for the sake of consistency,
> factor out gpiochip_simple_create_domain().
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index fac1124d5016..0e40f9a44519 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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++) {
-- 
2.40.0.1.gaa8946217a0b

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

[Linus Walleij]

On Wed, Jun 21, 2023 at 7:49 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Check domain for being an error pointer before assigning it to
> the GPIO IRQ chip domain.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

And this concludes patches 1,2,3, nice!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0e40f9a44519..59d87e60b108 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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);
 
-- 
2.40.0.1.gaa8946217a0b

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

[Linus Walleij]

On Wed, Jun 21, 2023 at 7:49 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> The gpiochip_irqchip_add_allocated_domain() can be used
> in another place in the code.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 59d87e60b108..bc8b9d6afe0e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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);
 
-- 
2.40.0.1.gaa8946217a0b

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

[Linus Walleij]

On Wed, Jun 21, 2023 at 7:49 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Replace open coded variant of gpiochip_irqchip_add_allocated_domain()
> in gpiochip_add_irqchip().
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

and this concludes patches 4,5 very nicely as well.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Andy Shevchenko]

On Fri, Jun 30, 2023 at 2:52 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Jun 21, 2023 at 7:49 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>
> > Replace open coded variant of gpiochip_irqchip_add_allocated_domain()
> > in gpiochip_add_irqchip().
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> and this concludes patches 4,5 very nicely as well.

Yep!

> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

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?


-- 
With Best Regards,
Andy Shevchenko

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

[Linus Walleij]

On Fri, Jun 30, 2023 at 3:58 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

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

> 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

Re: [PATCH v1 1/5] gpiolib: Make gpiochip_hierarchy_add_domain() return domain

[Linus Walleij]

On Wed, Jun 21, 2023 at 7:49 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> 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 <andriy.shevchenko@linux.intel.com>

Looks good!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v1 1/5] gpiolib: Make gpiochip_hierarchy_add_domain() return domain

[Bartosz Golaszewski]

On Wed, Jun 21, 2023 at 7:49 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> 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 <andriy.shevchenko@linux.intel.com>
> ---

Series applied, thanks!

Bart