[PATCH v2 0/5] gpiolib: switch to fwnode in the core

[PATCH v2 0/5] gpiolib: switch to fwnode in the core

[Andy Shevchenko]

GPIO library uses of_node and fwnode in the core in non-unified way.
The series cleans this up and improves IRQ domain creation for non-OF cases
where currently the names of the domain are 'unknown'.

This has been tested on Intel Galileo Gen 2.

In v2:
- added a new patch due to functionality in irq_comain_add_simple() (Linus)
- tagged patches 2-4 (Linus)
- Cc'ed to Rafael

Andy Shevchenko (5):
  irqdomain: Introduce irq_domain_create_simple() API
  gpiolib: Unify the checks on fwnode type
  gpiolib: Move of_node operations to gpiolib-of and correct fwnode use
  gpiolib: Introduce acpi_gpio_dev_init() and call it from core
  gpiolib: Reuse device's fwnode to create IRQ domain

 Documentation/core-api/irq/irq-domain.rst | 22 ++++----
 drivers/gpio/gpiolib-acpi.c               |  7 +++
 drivers/gpio/gpiolib-acpi.h               |  4 ++
 drivers/gpio/gpiolib-of.c                 |  6 ++-
 drivers/gpio/gpiolib.c                    | 66 +++++++++--------------
 include/linux/irqdomain.h                 |  5 ++
 kernel/irq/irqdomain.c                    | 16 ++++--
 7 files changed, 71 insertions(+), 55 deletions(-)

-- 
2.30.1

[PATCH v2 1/5] irqdomain: Introduce irq_domain_create_simple() API

[Andy Shevchenko]

Linus Walleij pointed out that ird_domain_add_simple() gained
additional functionality and can't be anymore replaced with
a simple conditional. In preparation to upgrade GPIO library
to use fwnode, introduce irq_domain_create_simple() API which is
functional equivalent to the existing irq_domain_add_simple(),
but takes a pointer to the struct fwnode_handle as a parameter.

While at it, amend documentation to mention irq_domain_create_*()
functions where it makes sense.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/core-api/irq/irq-domain.rst | 22 ++++++++++++----------
 include/linux/irqdomain.h                 |  5 +++++
 kernel/irq/irqdomain.c                    | 16 +++++++++++++---
 3 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/Documentation/core-api/irq/irq-domain.rst b/Documentation/core-api/irq/irq-domain.rst
index a77c24c27f7b..8214e215a8bf 100644
--- a/Documentation/core-api/irq/irq-domain.rst
+++ b/Documentation/core-api/irq/irq-domain.rst
@@ -42,10 +42,10 @@ irq_domain usage
 ================
 
 An interrupt controller driver creates and registers an irq_domain by
-calling one of the irq_domain_add_*() functions (each mapping method
-has a different allocator function, more on that later).  The function
-will return a pointer to the irq_domain on success.  The caller must
-provide the allocator function with an irq_domain_ops structure.
+calling one of the irq_domain_add_*() or irq_domain_create_*() functions
+(each mapping method has a different allocator function, more on that later).
+The function will return a pointer to the irq_domain on success. The caller
+must provide the allocator function with an irq_domain_ops structure.
 
 In most cases, the irq_domain will begin empty without any mappings
 between hwirq and IRQ numbers.  Mappings are added to the irq_domain
@@ -147,6 +147,7 @@ Legacy
 	irq_domain_add_simple()
 	irq_domain_add_legacy()
 	irq_domain_add_legacy_isa()
+	irq_domain_create_simple()
 	irq_domain_create_legacy()
 
 The Legacy mapping is a special case for drivers that already have a
@@ -169,13 +170,13 @@ supported.  For example, ISA controllers would use the legacy map for
 mapping Linux IRQs 0-15 so that existing ISA drivers get the correct IRQ
 numbers.
 
-Most users of legacy mappings should use irq_domain_add_simple() which
-will use a legacy domain only if an IRQ range is supplied by the
-system and will otherwise use a linear domain mapping. The semantics
-of this call are such that if an IRQ range is specified then
+Most users of legacy mappings should use irq_domain_add_simple() or
+irq_domain_create_simple() which will use a legacy domain only if an IRQ range
+is supplied by the system and will otherwise use a linear domain mapping.
+The semantics of this call are such that if an IRQ range is specified then
 descriptors will be allocated on-the-fly for it, and if no range is
-specified it will fall through to irq_domain_add_linear() which means
-*no* irq descriptors will be allocated.
+specified it will fall through to irq_domain_add_linear() or
+irq_domain_create_linear() which means *no* irq descriptors will be allocated.
 
 A typical use case for simple domains is where an irqchip provider
 is supporting both dynamic and static IRQ assignments.
@@ -186,6 +187,7 @@ that the driver using the simple domain call irq_create_mapping()
 before any irq_find_mapping() since the latter will actually work
 for the static IRQ assignment case.
 
+irq_domain_add_simple() and irq_domain_create_simple() as well as
 irq_domain_add_legacy() and irq_domain_create_legacy() are functionally
 equivalent, except for the first argument is different - the former
 accepts an Open Firmware specific 'struct device_node', while the latter
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 42d196805f58..1d4a8e7c5d5f 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -265,6 +265,11 @@ struct irq_domain *irq_domain_add_simple(struct device_node *of_node,
 					 unsigned int first_irq,
 					 const struct irq_domain_ops *ops,
 					 void *host_data);
+struct irq_domain *irq_domain_create_simple(struct fwnode_handle *fwnode,
+					    unsigned int size,
+					    unsigned int first_irq,
+					    const struct irq_domain_ops *ops,
+					    void *host_data);
 struct irq_domain *irq_domain_add_legacy(struct device_node *of_node,
 					 unsigned int size,
 					 unsigned int first_irq,
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 288151393a06..418548ea13cf 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -317,10 +317,20 @@ struct irq_domain *irq_domain_add_simple(struct device_node *of_node,
 					 unsigned int first_irq,
 					 const struct irq_domain_ops *ops,
 					 void *host_data)
+{
+	return irq_domain_create_simple(of_node_to_fwnode(of_node), size, first_irq, ops, host_data);
+}
+EXPORT_SYMBOL_GPL(irq_domain_add_simple);
+
+struct irq_domain *irq_domain_create_simple(struct fwnode_handle *fwnode,
+					    unsigned int size,
+					    unsigned int first_irq,
+					    const struct irq_domain_ops *ops,
+					    void *host_data)
 {
 	struct irq_domain *domain;
 
-	domain = __irq_domain_add(of_node_to_fwnode(of_node), size, size, 0, ops, host_data);
+	domain = __irq_domain_add(fwnode, size, size, 0, ops, host_data);
 	if (!domain)
 		return NULL;
 
@@ -328,7 +338,7 @@ struct irq_domain *irq_domain_add_simple(struct device_node *of_node,
 		if (IS_ENABLED(CONFIG_SPARSE_IRQ)) {
 			/* attempt to allocated irq_descs */
 			int rc = irq_alloc_descs(first_irq, first_irq, size,
-						 of_node_to_nid(of_node));
+						 of_node_to_nid(to_of_node(fwnode)));
 			if (rc < 0)
 				pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
 					first_irq);
@@ -338,7 +348,7 @@ struct irq_domain *irq_domain_add_simple(struct device_node *of_node,
 
 	return domain;
 }
-EXPORT_SYMBOL_GPL(irq_domain_add_simple);
+EXPORT_SYMBOL_GPL(irq_domain_create_simple);
 
 /**
  * irq_domain_add_legacy() - Allocate and register a legacy revmap irq_domain.
-- 
2.30.1

[PATCH v2 2/5] 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>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 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 v2 3/5] 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>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 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 v2 4/5] 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>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 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 1aacd2a5a1fd..21750be9c489 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 v2 5/5] 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 | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6827736ba05c..254d59b088fe 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,10 @@ 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;
+		gc->irq.domain = irq_domain_create_simple(fwnode, gc->ngpio,
+								  gc->irq.first,
+								  ops, gc);
 		if (!gc->irq.domain)
 			return -EINVAL;
 	}
-- 
2.30.1

Re: [PATCH v2 2/5] gpiolib: Unify the checks on fwnode type

[Andy Shevchenko]

On Thu, Mar 04, 2021 at 05:02:12PM +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.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  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);

Oy vey, missed to fold the fix to this line. :-(

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 v2 1/5] irqdomain: Introduce irq_domain_create_simple() API

[Marc Zyngier]

Andy,

On Thu, 04 Mar 2021 15:02:11 +0000,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> Linus Walleij pointed out that ird_domain_add_simple() gained
> additional functionality and can't be anymore replaced with
> a simple conditional. In preparation to upgrade GPIO library
> to use fwnode, introduce irq_domain_create_simple() API which is
> functional equivalent to the existing irq_domain_add_simple(),
> but takes a pointer to the struct fwnode_handle as a parameter.
> 
> While at it, amend documentation to mention irq_domain_create_*()
> functions where it makes sense.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

[...]

> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 42d196805f58..1d4a8e7c5d5f 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -265,6 +265,11 @@ struct irq_domain *irq_domain_add_simple(struct device_node *of_node,
>  					 unsigned int first_irq,
>  					 const struct irq_domain_ops *ops,
>  					 void *host_data);
> +struct irq_domain *irq_domain_create_simple(struct fwnode_handle *fwnode,
> +					    unsigned int size,
> +					    unsigned int first_irq,
> +					    const struct irq_domain_ops *ops,
> +					    void *host_data);
>  struct irq_domain *irq_domain_add_legacy(struct device_node *of_node,
>  					 unsigned int size,
>  					 unsigned int first_irq,
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 288151393a06..418548ea13cf 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -317,10 +317,20 @@ struct irq_domain *irq_domain_add_simple(struct device_node *of_node,
>  					 unsigned int first_irq,
>  					 const struct irq_domain_ops *ops,
>  					 void *host_data)
> +{
> +	return irq_domain_create_simple(of_node_to_fwnode(of_node), size, first_irq, ops, host_data);
> +}
> +EXPORT_SYMBOL_GPL(irq_domain_add_simple);

Please move this as an inline helper in linux/irqdomain.h, so that we
can drop this export altogether.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 1/5] irqdomain: Introduce irq_domain_create_simple() API

[Andy Shevchenko]

On Thu, Mar 04, 2021 at 06:29:32PM +0000, Marc Zyngier wrote:
> On Thu, 04 Mar 2021 15:02:11 +0000,
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Please move this as an inline helper in linux/irqdomain.h, so that we
> can drop this export altogether.

Done in v3, thanks!

-- 
With Best Regards,
Andy Shevchenko

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

[Rafael J. Wysocki]

On Thu, Mar 4, 2021 at 4:02 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>
> ---
>  drivers/gpio/gpiolib.c | 28 ++++++++--------------------
>  1 file changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 6827736ba05c..254d59b088fe 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;
> -       }

Why is the above message not worth printing any more?  If there is a
good enough reason, it would be good to mention it in the changelog.

> -
>         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,10 @@ 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;

I'm guessing that the code above is replaced in order to avoid
initializing ops to NULL, but IMO that should be a separate patch or
at least the extra cleanup should be mentioned in the changelog.

Personally, I would do the essential change first and put all of the
tangentially related cleanups into a separate follow-up patch.

> -               gc->irq.domain = irq_domain_add_simple(np,
> -                       gc->ngpio,
> -                       gc->irq.first,
> -                       ops, gc);
> +               ops = gc->irq.domain_ops ?: &gpiochip_domain_ops;
> +               gc->irq.domain = irq_domain_create_simple(fwnode, gc->ngpio,
> +                                                                 gc->irq.first,
> +                                                                 ops, gc);
>                 if (!gc->irq.domain)
>                         return -EINVAL;
>         }
> --

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

[Andy Shevchenko]

On Mon, Mar 08, 2021 at 07:32:47PM +0100, Rafael J. Wysocki wrote:
> On Thu, Mar 4, 2021 at 4:02 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

Thanks for review!

I'm wondering why you commented against v2 instead of v3... I assume it's just
a typo while the comments themselves are applicable to v3. Hence my reply
below.

...

> > -       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;
> > -       }
> 
> Why is the above message not worth printing any more?  If there is a
> good enough reason, it would be good to mention it in the changelog.

The reason is good enough, we drop duplicated printing since we got fwnode in
either case DT or ACPI. Basically this part is unification and due to nature of
the whole change it can't be done separately.

I will do in v4.

...

> >                 /* Some drivers provide custom irqdomain ops */
> > -               if (gc->irq.domain_ops)
> > -                       ops = gc->irq.domain_ops;
> > -
> > -               if (!ops)
> > -                       ops = &gpiochip_domain_ops;
> 
> I'm guessing that the code above is replaced in order to avoid
> initializing ops to NULL, but IMO that should be a separate patch or
> at least the extra cleanup should be mentioned in the changelog.
> 
> Personally, I would do the essential change first and put all of the
> tangentially related cleanups into a separate follow-up patch.

Okay, I will split it to a separate clean up in v4.

...

While at it, can you then provide an immutable tag / branch that Bart and I may
inject into our repos?

-- 
With Best Regards,
Andy Shevchenko