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

Message ID 20210304150215.80652-6-andriy.shevchenko@linux.intel.com
State New
Headers show
Series
  • Untitled series #107405
Related show

Commit Message

Andy Shevchenko March 4, 2021, 3:02 p.m.
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(-)

Comments

Rafael J. Wysocki March 8, 2021, 6:32 p.m. | #1
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;
>         }
> --
Andy Shevchenko March 8, 2021, 6:52 p.m. | #2
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?

Patch

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;
 	}