[PATCH v1 1/4] gpiolib: Unify the checks on fwnode type

[PATCH v1 1/4] gpiolib: Unify the checks on fwnode type

[Andy Shevchenko]

We have (historically) different approaches how we identify the type
of a given fwnode. Let's standardize them across the library code.

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index adf55db080d8..484ac92903ab 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3678,11 +3678,12 @@ EXPORT_SYMBOL_GPL(fwnode_gpiod_get_index);
  */
 int gpiod_count(struct device *dev, const char *con_id)
 {
+	const struct fwnode_handle *fwnode = dev_fwnode(dev);
 	int count = -ENOENT;
 
-	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
+	if (is_of_node(fwnode))
 		count = of_gpio_get_count(dev, con_id);
-	else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev))
+	else if (is_acpi_node(fwnode))
 		count = acpi_gpio_count(dev, con_id);
 
 	if (count < 0)
@@ -3820,18 +3821,17 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 	int ret;
 	/* Maybe we have a device name, maybe not */
 	const char *devname = dev ? dev_name(dev) : "?";
+	const struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
 
 	dev_dbg(dev, "GPIO lookup for consumer %s\n", con_id);
 
-	if (dev) {
-		/* Using device tree? */
-		if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
-			dev_dbg(dev, "using device tree for GPIO lookup\n");
-			desc = of_find_gpio(dev, con_id, idx, &lookupflags);
-		} else if (ACPI_COMPANION(dev)) {
-			dev_dbg(dev, "using ACPI for GPIO lookup\n");
-			desc = acpi_find_gpio(dev, con_id, idx, &flags, &lookupflags);
-		}
+	/* Using device tree? */
+	if (is_of_node(fwnode)) {
+		dev_dbg(dev, "using device tree for GPIO lookup\n");
+		desc = of_find_gpio(dev, con_id, idx, &lookupflags);
+	} else if (is_acpi_node(fwnode)) {
+		dev_dbg(dev, "using ACPI for GPIO lookup\n");
+		desc = acpi_find_gpio(dev, con_id, idx, &flags, &lookupflags);
 	}
 
 	/*
@@ -3915,9 +3915,6 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 	struct gpio_desc *desc = ERR_PTR(-ENODEV);
 	int ret;
 
-	if (!fwnode)
-		return ERR_PTR(-EINVAL);
-
 	if (is_of_node(fwnode)) {
 		desc = gpiod_get_from_of_node(to_of_node(fwnode),
 					      propname, index,
@@ -3933,7 +3930,8 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 
 		acpi_gpio_update_gpiod_flags(&dflags, &info);
 		acpi_gpio_update_gpiod_lookup_flags(&lflags, &info);
-	}
+	} else
+		return ERR_PTR(-EINVAL);
 
 	/* Currently only ACPI takes this path */
 	ret = gpiod_request(desc, label);
-- 
2.30.1

[PATCH v1 2/4] gpiolib: Move of_node operations to gpiolib-of and correct fwnode use

[Andy Shevchenko]

The initial value of the OF node based on presence of parent, but
at the same time this operation somehow appeared separately from others
that handle the OF case. On the other hand there is no need to assign
dev->fwnode in the OF case if code properly retrieves fwnode, i.e.
via dev_fwnode() helper.

Amend gpiolib.c and gpiolib-of.c code in order to group OF operations.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-of.c | 6 ++++--
 drivers/gpio/gpiolib.c    | 9 ++++-----
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index baf0153b7bca..bbcc7c073f63 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -1042,11 +1042,13 @@ void of_gpiochip_remove(struct gpio_chip *chip)
 
 void of_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev)
 {
+	/* Set default OF node to parent's one if present */
+	if (gc->parent)
+		gdev->dev.of_node = gc->parent->of_node;
+
 	/* If the gpiochip has an assigned OF node this takes precedence */
 	if (gc->of_node)
 		gdev->dev.of_node = gc->of_node;
 	else
 		gc->of_node = gdev->dev.of_node;
-	if (gdev->dev.of_node)
-		gdev->dev.fwnode = of_fwnode_handle(gdev->dev.of_node);
 }
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 484ac92903ab..4af8a8c1316e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -585,12 +585,9 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	if (!gdev)
 		return -ENOMEM;
 	gdev->dev.bus = &gpio_bus_type;
+	gdev->dev.parent = gc->parent;
 	gdev->chip = gc;
 	gc->gpiodev = gdev;
-	if (gc->parent) {
-		gdev->dev.parent = gc->parent;
-		gdev->dev.of_node = gc->parent->of_node;
-	}
 
 	of_gpio_dev_init(gc, gdev);
 
@@ -4212,11 +4209,13 @@ EXPORT_SYMBOL_GPL(gpiod_put_array);
 
 static int gpio_bus_match(struct device *dev, struct device_driver *drv)
 {
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+
 	/*
 	 * Only match if the fwnode doesn't already have a proper struct device
 	 * created for it.
 	 */
-	if (dev->fwnode && dev->fwnode->dev != dev)
+	if (fwnode && fwnode->dev != dev)
 		return 0;
 	return 1;
 }
-- 
2.30.1

[PATCH v1 3/4] gpiolib: Introduce acpi_gpio_dev_init() and call it from core

[Andy Shevchenko]

In the ACPI case we may use the firmware node in the similar way
as it's done for OF case. We may use that fwnode for other purposes
in the future.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 7 +++++++
 drivers/gpio/gpiolib-acpi.h | 4 ++++
 drivers/gpio/gpiolib.c      | 1 +
 3 files changed, 12 insertions(+)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 495f779b2ab9..27a4a4e0a48d 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1291,6 +1291,13 @@ void acpi_gpiochip_remove(struct gpio_chip *chip)
 	kfree(acpi_gpio);
 }
 
+void acpi_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev)
+{
+	/* Set default fwnode to parent's one if present */
+	if (gc->parent)
+		ACPI_COMPANION_SET(&gdev->dev, ACPI_COMPANION(gc->parent));
+}
+
 static int acpi_gpio_package_count(const union acpi_object *obj)
 {
 	const union acpi_object *element = obj->package.elements;
diff --git a/drivers/gpio/gpiolib-acpi.h b/drivers/gpio/gpiolib-acpi.h
index e2edb632b2cc..e476558d9471 100644
--- a/drivers/gpio/gpiolib-acpi.h
+++ b/drivers/gpio/gpiolib-acpi.h
@@ -36,6 +36,8 @@ struct acpi_gpio_info {
 void acpi_gpiochip_add(struct gpio_chip *chip);
 void acpi_gpiochip_remove(struct gpio_chip *chip);
 
+void acpi_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev);
+
 void acpi_gpiochip_request_interrupts(struct gpio_chip *chip);
 void acpi_gpiochip_free_interrupts(struct gpio_chip *chip);
 
@@ -58,6 +60,8 @@ int acpi_gpio_count(struct device *dev, const char *con_id);
 static inline void acpi_gpiochip_add(struct gpio_chip *chip) { }
 static inline void acpi_gpiochip_remove(struct gpio_chip *chip) { }
 
+static inline void acpi_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev) { }
+
 static inline void
 acpi_gpiochip_request_interrupts(struct gpio_chip *chip) { }
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4af8a8c1316e..6827736ba05c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -590,6 +590,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	gc->gpiodev = gdev;
 
 	of_gpio_dev_init(gc, gdev);
+	acpi_gpio_dev_init(gc, gdev);
 
 	gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
 	if (gdev->id < 0) {
-- 
2.30.1

[PATCH v1 4/4] gpiolib: Reuse device's fwnode to create IRQ domain

[Andy Shevchenko]

When IRQ domain is created for an ACPI case, the name of it becomes unknown-%d
since for now it utilizes of_node member only and doesn't consider fwnode case.
Convert IRQ domain creation code to utilize fwnode instead.

Before/After the change on Intel Galileo Gen 2 with two GPIO (IRQ) controllers:

  unknown-1	==>	\_SB.PCI0.GIP0.GPO
  unknown-2	==>	\_SB.NIO3

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6827736ba05c..54cfea4e4a03 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1457,9 +1457,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
 				struct lock_class_key *lock_key,
 				struct lock_class_key *request_key)
 {
+	struct fwnode_handle *fwnode = dev_fwnode(&gc->gpiodev->dev);
 	struct irq_chip *irqchip = gc->irq.chip;
-	const struct irq_domain_ops *ops = NULL;
-	struct device_node *np;
+	const struct irq_domain_ops *ops;
 	unsigned int type;
 	unsigned int i;
 
@@ -1471,7 +1471,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
 		return -EINVAL;
 	}
 
-	np = gc->gpiodev->dev.of_node;
 	type = gc->irq.default_type;
 
 	/*
@@ -1479,16 +1478,10 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
 	 * used to configure the interrupts, as you may end up with
 	 * conflicting triggers. Tell the user, and reset to NONE.
 	 */
-	if (WARN(np && type != IRQ_TYPE_NONE,
-		 "%s: Ignoring %u default trigger\n", np->full_name, type))
+	if (WARN(fwnode && type != IRQ_TYPE_NONE,
+		 "%pfw: Ignoring %u default trigger\n", fwnode, type))
 		type = IRQ_TYPE_NONE;
 
-	if (has_acpi_companion(gc->parent) && type != IRQ_TYPE_NONE) {
-		acpi_handle_warn(ACPI_HANDLE(gc->parent),
-				 "Ignoring %u default trigger\n", 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__);
 
@@ -1504,15 +1497,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
 			return ret;
 	} else {
 		/* Some drivers provide custom irqdomain ops */
-		if (gc->irq.domain_ops)
-			ops = gc->irq.domain_ops;
-
-		if (!ops)
-			ops = &gpiochip_domain_ops;
-		gc->irq.domain = irq_domain_add_simple(np,
-			gc->ngpio,
-			gc->irq.first,
-			ops, gc);
+		ops = gc->irq.domain_ops ?: &gpiochip_domain_ops;
+		if (gc->irq.first)
+			gc->irq.domain = irq_domain_create_legacy(fwnode, gc->ngpio,
+								  gc->irq.first, 0,
+								  ops, gc);
+		else
+			gc->irq.domain = irq_domain_create_linear(fwnode, gc->ngpio,
+								  ops, gc);
 		if (!gc->irq.domain)
 			return -EINVAL;
 	}
-- 
2.30.1

Re: [PATCH v1 1/4] gpiolib: Unify the checks on fwnode type

[Andy Shevchenko]

On Tue, Mar 02, 2021 at 05:34:48PM +0200, Andy Shevchenko wrote:
> We have (historically) different approaches how we identify the type
> of a given fwnode. Let's standardize them across the library code.

This patch has one functional mistake (see below), otherwise I will anyway wait
for people to comment on the series.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpiolib.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index adf55db080d8..484ac92903ab 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -3678,11 +3678,12 @@ EXPORT_SYMBOL_GPL(fwnode_gpiod_get_index);
>   */
>  int gpiod_count(struct device *dev, const char *con_id)
>  {
> +	const struct fwnode_handle *fwnode = dev_fwnode(dev);

It should be

	const struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;

>  	int count = -ENOENT;
>  
> -	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
> +	if (is_of_node(fwnode))
>  		count = of_gpio_get_count(dev, con_id);
> -	else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev))
> +	else if (is_acpi_node(fwnode))
>  		count = acpi_gpio_count(dev, con_id);
>  
>  	if (count < 0)
> @@ -3820,18 +3821,17 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
>  	int ret;
>  	/* Maybe we have a device name, maybe not */
>  	const char *devname = dev ? dev_name(dev) : "?";
> +	const struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
>  
>  	dev_dbg(dev, "GPIO lookup for consumer %s\n", con_id);
>  
> -	if (dev) {
> -		/* Using device tree? */
> -		if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
> -			dev_dbg(dev, "using device tree for GPIO lookup\n");
> -			desc = of_find_gpio(dev, con_id, idx, &lookupflags);
> -		} else if (ACPI_COMPANION(dev)) {
> -			dev_dbg(dev, "using ACPI for GPIO lookup\n");
> -			desc = acpi_find_gpio(dev, con_id, idx, &flags, &lookupflags);
> -		}
> +	/* Using device tree? */
> +	if (is_of_node(fwnode)) {
> +		dev_dbg(dev, "using device tree for GPIO lookup\n");
> +		desc = of_find_gpio(dev, con_id, idx, &lookupflags);
> +	} else if (is_acpi_node(fwnode)) {
> +		dev_dbg(dev, "using ACPI for GPIO lookup\n");
> +		desc = acpi_find_gpio(dev, con_id, idx, &flags, &lookupflags);
>  	}
>  
>  	/*
> @@ -3915,9 +3915,6 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
>  	struct gpio_desc *desc = ERR_PTR(-ENODEV);
>  	int ret;
>  
> -	if (!fwnode)
> -		return ERR_PTR(-EINVAL);
> -
>  	if (is_of_node(fwnode)) {
>  		desc = gpiod_get_from_of_node(to_of_node(fwnode),
>  					      propname, index,
> @@ -3933,7 +3930,8 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
>  
>  		acpi_gpio_update_gpiod_flags(&dflags, &info);
>  		acpi_gpio_update_gpiod_lookup_flags(&lflags, &info);
> -	}
> +	} else
> +		return ERR_PTR(-EINVAL);
>  
>  	/* Currently only ACPI takes this path */
>  	ret = gpiod_request(desc, label);
> -- 
> 2.30.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] gpiolib: Unify the checks on fwnode type

[Linus Walleij]

On Tue, Mar 2, 2021 at 4:35 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> We have (historically) different approaches how we identify the type
> of a given fwnode. Let's standardize them across the library code.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Looks way better like this, with your follow-up fix folded in:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v1 2/4] gpiolib: Move of_node operations to gpiolib-of and correct fwnode use

[Linus Walleij]

On Tue, Mar 2, 2021 at 4:35 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> The initial value of the OF node based on presence of parent, but
> at the same time this operation somehow appeared separately from others
> that handle the OF case. On the other hand there is no need to assign
> dev->fwnode in the OF case if code properly retrieves fwnode, i.e.
> via dev_fwnode() helper.
>
> Amend gpiolib.c and gpiolib-of.c code in order to group OF operations.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I parsed it in my head a few times and it looks like it will
end up working the same, so:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v1 3/4] gpiolib: Introduce acpi_gpio_dev_init() and call it from core

[Linus Walleij]

On Tue, Mar 2, 2021 at 4:36 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> In the ACPI case we may use the firmware node in the similar way
> as it's done for OF case. We may use that fwnode for other purposes
> in the future.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

Yours,
Linus Walleij

Re: [PATCH v1 4/4] gpiolib: Reuse device's fwnode to create IRQ domain

[Linus Walleij]

On Tue, Mar 2, 2021 at 4:35 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> When IRQ domain is created for an ACPI case, the name of it becomes unknown-%d
> since for now it utilizes of_node member only and doesn't consider fwnode case.
> Convert IRQ domain creation code to utilize fwnode instead.
>
> Before/After the change on Intel Galileo Gen 2 with two GPIO (IRQ) controllers:
>
>   unknown-1     ==>     \_SB.PCI0.GIP0.GPO
>   unknown-2     ==>     \_SB.NIO3
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

This first part seems to do what you want,

> @@ -1457,9 +1457,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
>                                 struct lock_class_key *lock_key,
>                                 struct lock_class_key *request_key)
>  {
> +       struct fwnode_handle *fwnode = dev_fwnode(&gc->gpiodev->dev);

(...)

But this:

> @@ -1504,15 +1497,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
>                         return ret;
>         } else {
>                 /* Some drivers provide custom irqdomain ops */
> -               if (gc->irq.domain_ops)
> -                       ops = gc->irq.domain_ops;
> -
> -               if (!ops)
> -                       ops = &gpiochip_domain_ops;
> -               gc->irq.domain = irq_domain_add_simple(np,
> -                       gc->ngpio,
> -                       gc->irq.first,
> -                       ops, gc);
> +               ops = gc->irq.domain_ops ?: &gpiochip_domain_ops;
> +               if (gc->irq.first)
> +                       gc->irq.domain = irq_domain_create_legacy(fwnode, gc->ngpio,
> +                                                                 gc->irq.first, 0,
> +                                                                 ops, gc);
> +               else
> +                       gc->irq.domain = irq_domain_create_linear(fwnode, gc->ngpio,
> +                                                                 ops, gc);

This looks like a refactoring and reimplementation of irq_domain_add_simple()?

Why, and should it rather be a separate patch?

Yours,
Linus Walleij

Re: [PATCH v1 4/4] gpiolib: Reuse device's fwnode to create IRQ domain

[Andy Shevchenko]

On Wed, Mar 03, 2021 at 10:22:02AM +0100, Linus Walleij wrote:
> On Tue, Mar 2, 2021 at 4:35 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > When IRQ domain is created for an ACPI case, the name of it becomes unknown-%d
> > since for now it utilizes of_node member only and doesn't consider fwnode case.
> > Convert IRQ domain creation code to utilize fwnode instead.
> >
> > Before/After the change on Intel Galileo Gen 2 with two GPIO (IRQ) controllers:
> >
> >   unknown-1     ==>     \_SB.PCI0.GIP0.GPO
> >   unknown-2     ==>     \_SB.NIO3
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> This first part seems to do what you want,

...

> But this:
> 
> > @@ -1504,15 +1497,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
> >                         return ret;
> >         } else {
> >                 /* Some drivers provide custom irqdomain ops */
> > -               if (gc->irq.domain_ops)
> > -                       ops = gc->irq.domain_ops;
> > -
> > -               if (!ops)
> > -                       ops = &gpiochip_domain_ops;
> > -               gc->irq.domain = irq_domain_add_simple(np,
> > -                       gc->ngpio,
> > -                       gc->irq.first,
> > -                       ops, gc);
> > +               ops = gc->irq.domain_ops ?: &gpiochip_domain_ops;
> > +               if (gc->irq.first)
> > +                       gc->irq.domain = irq_domain_create_legacy(fwnode, gc->ngpio,
> > +                                                                 gc->irq.first, 0,
> > +                                                                 ops, gc);
> > +               else
> > +                       gc->irq.domain = irq_domain_create_linear(fwnode, gc->ngpio,
> > +                                                                 ops, gc);
> 
> This looks like a refactoring and reimplementation of irq_domain_add_simple()?

If you named it as irq_domain_create_simple(), then yes, but the problem is
that we don't have irq_domain_create_simple() API right now.

> Why, and should it rather be a separate patch?

Nope.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/4] gpiolib: Reuse device's fwnode to create IRQ domain

[Linus Walleij]

On Wed, Mar 3, 2021 at 10:35 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, Mar 03, 2021 at 10:22:02AM +0100, Linus Walleij wrote:

> > But this:
> >
> > > @@ -1504,15 +1497,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
> > >                         return ret;
> > >         } else {
> > >                 /* Some drivers provide custom irqdomain ops */
> > > -               if (gc->irq.domain_ops)
> > > -                       ops = gc->irq.domain_ops;
> > > -
> > > -               if (!ops)
> > > -                       ops = &gpiochip_domain_ops;
> > > -               gc->irq.domain = irq_domain_add_simple(np,
> > > -                       gc->ngpio,
> > > -                       gc->irq.first,
> > > -                       ops, gc);
> > > +               ops = gc->irq.domain_ops ?: &gpiochip_domain_ops;
> > > +               if (gc->irq.first)
> > > +                       gc->irq.domain = irq_domain_create_legacy(fwnode, gc->ngpio,
> > > +                                                                 gc->irq.first, 0,
> > > +                                                                 ops, gc);
> > > +               else
> > > +                       gc->irq.domain = irq_domain_create_linear(fwnode, gc->ngpio,
> > > +                                                                 ops, gc);
> >
> > This looks like a refactoring and reimplementation of irq_domain_add_simple()?
>
> If you named it as irq_domain_create_simple(), then yes, but the problem is
> that we don't have irq_domain_create_simple() API right now.
>
> > Why, and should it rather be a separate patch?
>
> Nope.

OK I looked closer at irq_domain_add_simple(), and what it does different
is to call irq_alloc_descs() for all lines if using sparse IRQs and then
associate them. irq_domain_create_linear|legacy() does not allocate IRQ
descriptors because it assumes something like DT or ACPI will do that
on-demand when drivers request IRQs.

This may be dangerous because some old platforms do not resolve IRQs
at runtime and you will get NULL pointer exceptions.

We then need to make sure all callers do what is done in e.g.
drivers/gpio/gpio-omap.c in the #ifdef CONFIG_ARCH_OMAP1 clause:
they need to be augmented to call irq_alloc_descs() explicitly,
and I don't think all of them do it as nicely for us as OMAP1.

I might be overly cautious though, however that is why this code
uses irq_domain_add_simple(), came in commit
commit 2854d167cc545d0642277bf8b77f972a91146fc6

Yours,
Linus Walleij

Re: [PATCH v1 4/4] gpiolib: Reuse device's fwnode to create IRQ domain

[Andy Shevchenko]

On Thu, Mar 04, 2021 at 09:06:08AM +0100, Linus Walleij wrote:
> On Wed, Mar 3, 2021 at 10:35 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Mar 03, 2021 at 10:22:02AM +0100, Linus Walleij wrote:
> 
> > > But this:
> > >
> > > > @@ -1504,15 +1497,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
> > > >                         return ret;
> > > >         } else {
> > > >                 /* Some drivers provide custom irqdomain ops */
> > > > -               if (gc->irq.domain_ops)
> > > > -                       ops = gc->irq.domain_ops;
> > > > -
> > > > -               if (!ops)
> > > > -                       ops = &gpiochip_domain_ops;
> > > > -               gc->irq.domain = irq_domain_add_simple(np,
> > > > -                       gc->ngpio,
> > > > -                       gc->irq.first,
> > > > -                       ops, gc);
> > > > +               ops = gc->irq.domain_ops ?: &gpiochip_domain_ops;
> > > > +               if (gc->irq.first)
> > > > +                       gc->irq.domain = irq_domain_create_legacy(fwnode, gc->ngpio,
> > > > +                                                                 gc->irq.first, 0,
> > > > +                                                                 ops, gc);
> > > > +               else
> > > > +                       gc->irq.domain = irq_domain_create_linear(fwnode, gc->ngpio,
> > > > +                                                                 ops, gc);
> > >
> > > This looks like a refactoring and reimplementation of irq_domain_add_simple()?
> >
> > If you named it as irq_domain_create_simple(), then yes, but the problem is
> > that we don't have irq_domain_create_simple() API right now.
> >
> > > Why, and should it rather be a separate patch?
> >
> > Nope.
> 
> OK I looked closer at irq_domain_add_simple(), and what it does different
> is to call irq_alloc_descs() for all lines if using sparse IRQs and then
> associate them. irq_domain_create_linear|legacy() does not allocate IRQ
> descriptors because it assumes something like DT or ACPI will do that
> on-demand when drivers request IRQs.
> 
> This may be dangerous because some old platforms do not resolve IRQs
> at runtime and you will get NULL pointer exceptions.
> 
> We then need to make sure all callers do what is done in e.g.
> drivers/gpio/gpio-omap.c in the #ifdef CONFIG_ARCH_OMAP1 clause:
> they need to be augmented to call irq_alloc_descs() explicitly,
> and I don't think all of them do it as nicely for us as OMAP1.
> 
> I might be overly cautious though, however that is why this code
> uses irq_domain_add_simple(), came in commit
> commit 2854d167cc545d0642277bf8b77f972a91146fc6

Ah, thanks! I was puzzled how and why the approach above had been extended like
now. This explains it. Okay, I will introduce irq_domain_create_simple().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/4] gpiolib: Reuse device's fwnode to create IRQ domain

[Rafael J. Wysocki]

On Thu, Mar 4, 2021 at 1:25 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Mar 04, 2021 at 09:06:08AM +0100, Linus Walleij wrote:
> > On Wed, Mar 3, 2021 at 10:35 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Wed, Mar 03, 2021 at 10:22:02AM +0100, Linus Walleij wrote:
> >
> > > > But this:
> > > >
> > > > > @@ -1504,15 +1497,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
> > > > >                         return ret;
> > > > >         } else {
> > > > >                 /* Some drivers provide custom irqdomain ops */
> > > > > -               if (gc->irq.domain_ops)
> > > > > -                       ops = gc->irq.domain_ops;
> > > > > -
> > > > > -               if (!ops)
> > > > > -                       ops = &gpiochip_domain_ops;
> > > > > -               gc->irq.domain = irq_domain_add_simple(np,
> > > > > -                       gc->ngpio,
> > > > > -                       gc->irq.first,
> > > > > -                       ops, gc);
> > > > > +               ops = gc->irq.domain_ops ?: &gpiochip_domain_ops;
> > > > > +               if (gc->irq.first)
> > > > > +                       gc->irq.domain = irq_domain_create_legacy(fwnode, gc->ngpio,
> > > > > +                                                                 gc->irq.first, 0,
> > > > > +                                                                 ops, gc);
> > > > > +               else
> > > > > +                       gc->irq.domain = irq_domain_create_linear(fwnode, gc->ngpio,
> > > > > +                                                                 ops, gc);
> > > >
> > > > This looks like a refactoring and reimplementation of irq_domain_add_simple()?
> > >
> > > If you named it as irq_domain_create_simple(), then yes, but the problem is
> > > that we don't have irq_domain_create_simple() API right now.
> > >
> > > > Why, and should it rather be a separate patch?
> > >
> > > Nope.
> >
> > OK I looked closer at irq_domain_add_simple(), and what it does different
> > is to call irq_alloc_descs() for all lines if using sparse IRQs and then
> > associate them. irq_domain_create_linear|legacy() does not allocate IRQ
> > descriptors because it assumes something like DT or ACPI will do that
> > on-demand when drivers request IRQs.
> >
> > This may be dangerous because some old platforms do not resolve IRQs
> > at runtime and you will get NULL pointer exceptions.
> >
> > We then need to make sure all callers do what is done in e.g.
> > drivers/gpio/gpio-omap.c in the #ifdef CONFIG_ARCH_OMAP1 clause:
> > they need to be augmented to call irq_alloc_descs() explicitly,
> > and I don't think all of them do it as nicely for us as OMAP1.
> >
> > I might be overly cautious though, however that is why this code
> > uses irq_domain_add_simple(), came in commit
> > commit 2854d167cc545d0642277bf8b77f972a91146fc6
>
> Ah, thanks! I was puzzled how and why the approach above had been extended like
> now. This explains it. Okay, I will introduce irq_domain_create_simple().

OK

So please resend the series with that done and with the R-bys from
Linus added.  I'll apply it from Patchwork.

Thanks!

Re: [PATCH v1 4/4] gpiolib: Reuse device's fwnode to create IRQ domain

[Andy Shevchenko]

On Thu, Mar 04, 2021 at 02:41:24PM +0100, Rafael J. Wysocki wrote:
> On Thu, Mar 4, 2021 at 1:25 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Thu, Mar 04, 2021 at 09:06:08AM +0100, Linus Walleij wrote:
> > > On Wed, Mar 3, 2021 at 10:35 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Wed, Mar 03, 2021 at 10:22:02AM +0100, Linus Walleij wrote:
> > >
> > > > > But this:
> > > > >
> > > > > > @@ -1504,15 +1497,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
> > > > > >                         return ret;
> > > > > >         } else {
> > > > > >                 /* Some drivers provide custom irqdomain ops */
> > > > > > -               if (gc->irq.domain_ops)
> > > > > > -                       ops = gc->irq.domain_ops;
> > > > > > -
> > > > > > -               if (!ops)
> > > > > > -                       ops = &gpiochip_domain_ops;
> > > > > > -               gc->irq.domain = irq_domain_add_simple(np,
> > > > > > -                       gc->ngpio,
> > > > > > -                       gc->irq.first,
> > > > > > -                       ops, gc);
> > > > > > +               ops = gc->irq.domain_ops ?: &gpiochip_domain_ops;
> > > > > > +               if (gc->irq.first)
> > > > > > +                       gc->irq.domain = irq_domain_create_legacy(fwnode, gc->ngpio,
> > > > > > +                                                                 gc->irq.first, 0,
> > > > > > +                                                                 ops, gc);
> > > > > > +               else
> > > > > > +                       gc->irq.domain = irq_domain_create_linear(fwnode, gc->ngpio,
> > > > > > +                                                                 ops, gc);
> > > > >
> > > > > This looks like a refactoring and reimplementation of irq_domain_add_simple()?
> > > >
> > > > If you named it as irq_domain_create_simple(), then yes, but the problem is
> > > > that we don't have irq_domain_create_simple() API right now.
> > > >
> > > > > Why, and should it rather be a separate patch?
> > > >
> > > > Nope.
> > >
> > > OK I looked closer at irq_domain_add_simple(), and what it does different
> > > is to call irq_alloc_descs() for all lines if using sparse IRQs and then
> > > associate them. irq_domain_create_linear|legacy() does not allocate IRQ
> > > descriptors because it assumes something like DT or ACPI will do that
> > > on-demand when drivers request IRQs.
> > >
> > > This may be dangerous because some old platforms do not resolve IRQs
> > > at runtime and you will get NULL pointer exceptions.
> > >
> > > We then need to make sure all callers do what is done in e.g.
> > > drivers/gpio/gpio-omap.c in the #ifdef CONFIG_ARCH_OMAP1 clause:
> > > they need to be augmented to call irq_alloc_descs() explicitly,
> > > and I don't think all of them do it as nicely for us as OMAP1.
> > >
> > > I might be overly cautious though, however that is why this code
> > > uses irq_domain_add_simple(), came in commit
> > > commit 2854d167cc545d0642277bf8b77f972a91146fc6
> >
> > Ah, thanks! I was puzzled how and why the approach above had been extended like
> > now. This explains it. Okay, I will introduce irq_domain_create_simple().
> 
> OK
> 
> So please resend the series with that done and with the R-bys from
> Linus added.  I'll apply it from Patchwork.

Done!

https://lore.kernel.org/linux-gpio/20210304150215.80652-1-andriy.shevchenko@linux.intel.com/T/#u

P.S. you seems haven't switched yet to b4 :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/4] gpiolib: Reuse device's fwnode to create IRQ domain

[Rafael J. Wysocki]

On Thu, Mar 4, 2021 at 4:40 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Mar 04, 2021 at 02:41:24PM +0100, Rafael J. Wysocki wrote:
> > On Thu, Mar 4, 2021 at 1:25 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Thu, Mar 04, 2021 at 09:06:08AM +0100, Linus Walleij wrote:
> > > > On Wed, Mar 3, 2021 at 10:35 AM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Wed, Mar 03, 2021 at 10:22:02AM +0100, Linus Walleij wrote:
> > > >
> > > > > > But this:
> > > > > >
> > > > > > > @@ -1504,15 +1497,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
> > > > > > >                         return ret;
> > > > > > >         } else {
> > > > > > >                 /* Some drivers provide custom irqdomain ops */
> > > > > > > -               if (gc->irq.domain_ops)
> > > > > > > -                       ops = gc->irq.domain_ops;
> > > > > > > -
> > > > > > > -               if (!ops)
> > > > > > > -                       ops = &gpiochip_domain_ops;
> > > > > > > -               gc->irq.domain = irq_domain_add_simple(np,
> > > > > > > -                       gc->ngpio,
> > > > > > > -                       gc->irq.first,
> > > > > > > -                       ops, gc);
> > > > > > > +               ops = gc->irq.domain_ops ?: &gpiochip_domain_ops;
> > > > > > > +               if (gc->irq.first)
> > > > > > > +                       gc->irq.domain = irq_domain_create_legacy(fwnode, gc->ngpio,
> > > > > > > +                                                                 gc->irq.first, 0,
> > > > > > > +                                                                 ops, gc);
> > > > > > > +               else
> > > > > > > +                       gc->irq.domain = irq_domain_create_linear(fwnode, gc->ngpio,
> > > > > > > +                                                                 ops, gc);
> > > > > >
> > > > > > This looks like a refactoring and reimplementation of irq_domain_add_simple()?
> > > > >
> > > > > If you named it as irq_domain_create_simple(), then yes, but the problem is
> > > > > that we don't have irq_domain_create_simple() API right now.
> > > > >
> > > > > > Why, and should it rather be a separate patch?
> > > > >
> > > > > Nope.
> > > >
> > > > OK I looked closer at irq_domain_add_simple(), and what it does different
> > > > is to call irq_alloc_descs() for all lines if using sparse IRQs and then
> > > > associate them. irq_domain_create_linear|legacy() does not allocate IRQ
> > > > descriptors because it assumes something like DT or ACPI will do that
> > > > on-demand when drivers request IRQs.
> > > >
> > > > This may be dangerous because some old platforms do not resolve IRQs
> > > > at runtime and you will get NULL pointer exceptions.
> > > >
> > > > We then need to make sure all callers do what is done in e.g.
> > > > drivers/gpio/gpio-omap.c in the #ifdef CONFIG_ARCH_OMAP1 clause:
> > > > they need to be augmented to call irq_alloc_descs() explicitly,
> > > > and I don't think all of them do it as nicely for us as OMAP1.
> > > >
> > > > I might be overly cautious though, however that is why this code
> > > > uses irq_domain_add_simple(), came in commit
> > > > commit 2854d167cc545d0642277bf8b77f972a91146fc6
> > >
> > > Ah, thanks! I was puzzled how and why the approach above had been extended like
> > > now. This explains it. Okay, I will introduce irq_domain_create_simple().
> >
> > OK
> >
> > So please resend the series with that done and with the R-bys from
> > Linus added.  I'll apply it from Patchwork.
>
> Done!

Thanks.

> https://lore.kernel.org/linux-gpio/20210304150215.80652-1-andriy.shevchenko@linux.intel.com/T/#u
>
> P.S. you seems haven't switched yet to b4 :-)

Nope.