Unify handling of ACPI, GPIO, devictree, and platform resource
interrupts in platform_get_irq_optional(). Each of these subsystems
provide their own APIs which provide IRQ information as a struct
resource. This simplifies the logic of the function and allows callers
to get more information about the IRQ by looking at the resource flags.
For example, whether or not an IRQ is wake capable.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-irq->IRQ
-Remove cast to struct resource
-Conform to get_optional() function naming
-Revert move of irq_get_irq_data()
-Add NULL check on struct resource*
-Use fwnode to retrieve IRQ for DT/ACPI

 drivers/base/platform.c         | 74 +++++++++++++++++++++++----------
 include/linux/platform_device.h |  3 ++
 2 files changed, 54 insertions(+), 23 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 10c5779634182..3a556faddd40d 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -151,9 +151,11 @@ EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
 #endif /* CONFIG_HAS_IOMEM */
 
 /**
- * platform_get_irq_optional - get an optional IRQ for a device
+ * platform_get_irq_resource_optional - get an optional IRQ for a device and
+ *					populate the resource struct
  * @dev: platform device
  * @num: IRQ number index
+ * @r: pointer to resource to populate with IRQ information.
  *
  * Gets an IRQ for a platform device. Device drivers should check the return
  * value for errors so as to not pass a negative integer value to the
@@ -162,47 +164,42 @@ EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
  *
  * For example::
  *
- *		int irq = platform_get_irq_optional(pdev, 0);
+ *		int irq = platform_get_irq_resource_optional(pdev, 0, &res);
  *		if (irq < 0)
  *			return irq;
  *
  * Return: non-zero IRQ number on success, negative error number on failure.
  */
-int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+int platform_get_irq_resource_optional(struct platform_device *dev,
+				       unsigned int num, struct resource *r)
 {
 	int ret;
+	if (IS_ERR_OR_NULL(r))
+		return -EINVAL;
 #ifdef CONFIG_SPARC
 	/* sparc does not have irqs represented as IORESOURCE_IRQ resources */
 	if (!dev || num >= dev->archdata.num_irqs)
 		goto out_not_found;
 	ret = dev->archdata.irqs[num];
+	if (ret > 0)
+		*r = DEFINE_RES_IRQ(ret);
 	goto out;
 #else
 	struct fwnode_handle *fwnode = dev_fwnode(&dev->dev);
-	struct resource *r;
+	struct resource *platform_res;
 
-	if (is_of_node(fwnode)) {
-		ret = of_irq_get(to_of_node(fwnode), num);
-		if (ret > 0 || ret == -EPROBE_DEFER)
-			goto out;
-	}
-
-	r = platform_get_resource(dev, IORESOURCE_IRQ, num);
-	if (is_acpi_device_node(fwnode)) {
-		if (r && r->flags & IORESOURCE_DISABLED) {
-			ret = acpi_irq_get(ACPI_HANDLE_FWNODE(fwnode), num, r);
-			if (ret)
-				goto out;
-		}
-	}
+	ret = fwnode_irq_get_resource(fwnode, num, r);
+	if (ret > 0 || ret == -EPROBE_DEFER)
+		goto out;
 
+	platform_res = platform_get_resource(dev, IORESOURCE_IRQ, num);
 	/*
 	 * The resources may pass trigger flags to the irqs that need
 	 * to be set up. It so happens that the trigger flags for
 	 * IORESOURCE_BITS correspond 1-to-1 to the IRQF_TRIGGER*
 	 * settings.
 	 */
-	if (r && r->flags & IORESOURCE_BITS) {
+	if (platform_res && platform_res->flags & IORESOURCE_BITS) {
 		struct irq_data *irqd;
 
 		irqd = irq_get_irq_data(r->start);
@@ -211,7 +208,8 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
 		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
 	}
 
-	if (r) {
+	if (platform_res) {
+		*r = *platform_res;
 		ret = r->start;
 		goto out;
 	}
@@ -224,10 +222,12 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
 	 * allows a common code path across either kind of resource.
 	 */
 	if (num == 0 && is_acpi_device_node(fwnode)) {
-		ret = acpi_dev_gpio_irq_get(to_acpi_device_node(fwnode), num);
+		ret = acpi_dev_get_gpio_irq_resource(to_acpi_device_node(fwnode), NULL, num, r);
 		/* Our callers expect -ENXIO for missing IRQs. */
-		if (ret >= 0 || ret == -EPROBE_DEFER)
+		if (!ret || ret == -EPROBE_DEFER) {
+			ret = ret ?: r->start;
 			goto out;
+		}
 	}
 
 #endif
@@ -238,7 +238,8 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
 		return -EINVAL;
 	return ret;
 }
-EXPORT_SYMBOL_GPL(platform_get_irq_optional);
+EXPORT_SYMBOL_GPL(platform_get_irq_resource_optional);
+
 
 /**
  * platform_get_irq - get an IRQ for a device
@@ -270,6 +271,33 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
 }
 EXPORT_SYMBOL_GPL(platform_get_irq);
 
+
+/**
+ * platform_get_irq_optional - get an optional IRQ for a device
+ * @dev: platform device
+ * @num: IRQ number index
+ *
+ * Gets an IRQ for a platform device. Device drivers should check the return
+ * value for errors so as to not pass a negative integer value to the
+ * request_irq() APIs. This is the same as platform_get_irq(), except that it
+ * does not print an error message if an IRQ can not be obtained.
+ *
+ * For example::
+ *
+ *		int irq = platform_get_irq_optional(pdev, 0);
+ *		if (irq < 0)
+ *			return irq;
+ *
+ * Return: non-zero IRQ number on success, negative error number on failure.
+ */
+int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+{
+	struct resource r;
+
+	return platform_get_irq_resource_optional(dev, num, &r);
+}
+EXPORT_SYMBOL_GPL(platform_get_irq_optional);
+
 /**
  * platform_irq_count - Count the number of IRQs a platform device uses
  * @dev: platform device
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 7a41c72c19591..2117f817d9c9c 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -102,6 +102,9 @@ devm_platform_ioremap_resource_byname(struct platform_device *pdev,
 
 extern int platform_get_irq(struct platform_device *, unsigned int);
 extern int platform_get_irq_optional(struct platform_device *, unsigned int);
+extern int platform_get_irq_resource_optional(struct platform_device *dev,
+					      unsigned int num,
+					      struct resource *r);
 extern int platform_irq_count(struct platform_device *);
 extern int devm_platform_get_irqs_affinity(struct platform_device *dev,
 					   struct irq_affinity *affd,
-- 
2.43.0.472.g3155946c3a-goog

On Wed, Dec 20, 2023 at 04:54:35PM -0700, Mark Hasemeyer wrote:
> Unify handling of ACPI, GPIO, devictree, and platform resource
> interrupts in platform_get_irq_optional(). Each of these subsystems
> provide their own APIs which provide IRQ information as a struct
> resource. This simplifies the logic of the function and allows callers
> to get more information about the IRQ by looking at the resource flags.
> For example, whether or not an IRQ is wake capable.

...

>   * For example::
>   *
> - *		int irq = platform_get_irq_optional(pdev, 0);
> + *		int irq = platform_get_irq_resource_optional(pdev, 0, &res);
>   *		if (irq < 0)
>   *			return irq;
>   *
>   * Return: non-zero IRQ number on success, negative error number on failure.

Why do we need the irq to be returned via error code?

...

>  	int ret;

Missing blank line, have you run checkpatch.pl?

> +	if (IS_ERR_OR_NULL(r))
> +		return -EINVAL;

If we ever have an error pointer in r, I prefer to see

	if (!r)
		return -EINVAL;
	if (IS_ERR(r))
		return PTR_ERR(r);

But Q is the same as earlier: when would we have the error pointer in @r?

...

> +	platform_res = platform_get_resource(dev, IORESOURCE_IRQ, num);

I would move this closer to the condition...

>  	/*
>  	 * The resources may pass trigger flags to the irqs that need
>  	 * to be set up. It so happens that the trigger flags for
>  	 * IORESOURCE_BITS correspond 1-to-1 to the IRQF_TRIGGER*
>  	 * settings.
>  	 */

...i.e. here.

> -	if (r && r->flags & IORESOURCE_BITS) {
> +	if (platform_res && platform_res->flags & IORESOURCE_BITS) {

>  	}

...

>  	if (num == 0 && is_acpi_device_node(fwnode)) {
> -		ret = acpi_dev_gpio_irq_get(to_acpi_device_node(fwnode), num);
> +		ret = acpi_dev_get_gpio_irq_resource(to_acpi_device_node(fwnode), NULL, num, r);
>  		/* Our callers expect -ENXIO for missing IRQs. */

> -		if (ret >= 0 || ret == -EPROBE_DEFER)
> +		if (!ret || ret == -EPROBE_DEFER) {

Can we save this and be consistent with above fwnode API return code check?

> +			ret = ret ?: r->start;
>  			goto out;
> +		}
>  	}

...

> -EXPORT_SYMBOL_GPL(platform_get_irq_optional);
> +EXPORT_SYMBOL_GPL(platform_get_irq_resource_optional);
> +

Stray blank line change.

...

>  EXPORT_SYMBOL_GPL(platform_get_irq);
>  
> +

Ditto.

...

> +int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
> +{
> +	struct resource r;

	struct resource r = {};

?

> +	return platform_get_irq_resource_optional(dev, num, &r);
> +}

-- 
With Best Regards,
Andy Shevchenko

> >   * For example::
> >   *
> > - *           int irq = platform_get_irq_optional(pdev, 0);
> > + *           int irq = platform_get_irq_resource_optional(pdev, 0, &res);
> >   *           if (irq < 0)
> >   *                   return irq;
> >   *
> >   * Return: non-zero IRQ number on success, negative error number on failure.
>
> Why do we need the irq to be returned via error code?

We don't really. It just matches the convention of
'platform_get_irq()' and 'of_irq_to_resource()'.

> >       int ret;
>
> Missing blank line, have you run checkpatch.pl?

Yes, I normally run checkpatch.pl. I may have missed the warning or it
didn't catch it. I'll add it.

>
> > +     if (IS_ERR_OR_NULL(r))
> > +             return -EINVAL;
>
> If we ever have an error pointer in r, I prefer to see
>
>         if (!r)
>                 return -EINVAL;
>         if (IS_ERR(r))
>                 return PTR_ERR(r);
>
> But Q is the same as earlier: when would we have the error pointer in @r?

I don't see when we would. I'll drop it.

> Can we save this and be consistent with above fwnode API return code check?
>
> > +                     ret = ret ?: r->start;
> >                       goto out;
> > +             }
> >       }

Yep!