diff mbox series

[v4,23/24] platform: Modify platform_get_irq_optional() to use resource

Message ID 20240102140734.v4.23.Ife9ebad2bbfbab3a05e90040f344d750aa0aac7e@changeid
State New
Headers
Series Improve IRQ wake capability reporting and update the cros_ec driver to use it |

Commit Message

Mark Hasemeyer Jan. 2, 2024, 9:07 p.m. UTC 
  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 v4:
-platform_get_irq_optional() returns 0 on success

Changes in v3:
-Remove PTR_ERR check
-Move platform_res assignment
-Check for irq == 0 to trigger WARN msg
-Refactor error handling of acpi_dev_get_gpio_irq_resource() to be
consistent with fwnode_irq_get_resource()
-Remove extra blank lines
-Initialize struct resource on stack

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         | 90 +++++++++++++++++++++------------
 include/linux/platform_device.h |  3 ++
 2 files changed, 61 insertions(+), 32 deletions(-)

Comments

Andy Shevchenko Jan. 6, 2024, 2:55 p.m. UTC | #1 
On Tue, Jan 02, 2024 at 02:07:47PM -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.

...

> +	ret = fwnode_irq_get_resource(fwnode, num, r);

I still prefer this not to return positive value. Since you _require_ @r to be
not NULL, i.e. valid, the returning positive value makes no sense.

> +	ret = ret < 0 ? ret : 0;
> +	if (!ret || ret == -EPROBE_DEFER)
> +		goto out;

...

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

Same comment, please split definition and assignment.
Mark Hasemeyer Jan. 8, 2024, 7:09 p.m. UTC | #2 
On Sat, Jan 6, 2024 at 7:56 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Tue, Jan 02, 2024 at 02:07:47PM -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.
>
> ...
>
> > +     ret = fwnode_irq_get_resource(fwnode, num, r);
>
> I still prefer this not to return positive value. Since you _require_ @r to be
> not NULL, i.e. valid, the returning positive value makes no sense.
>
> > +     ret = ret < 0 ? ret : 0;
> > +     if (!ret || ret == -EPROBE_DEFER)
> > +             goto out;

I agree. But echoing my response from v3 patch 24:
- The fwnode patch is already reviewed and approved.
- The fwnode patch uses of_irq_to_resource() which already existed and
returns the irq number on success. The error handling translation will
just get pushed to the fwnode subsystem unless of_irq_to_resource() is
also modified which means updating 8 or so drivers that reference it.

I can either:
-Leave it
-Modify the fwnode subsystem to perform the error translation of
of_irq_to_resource()
-Modify the fwnode and OF subsystems and update all driver references

The fwnode and OF patches are already reviewed. I imagine coding
changes would imply dropping any Reviewed-by tags and requesting
another review?
I'd really prefer to not blow up the patch series anymore, but if you
feel strongly, we can come up with a solution.

>
> > +     struct resource *platform_res = platform_get_resource(dev, IORESOURCE_IRQ, num);
>
> Same comment, please split definition and assignment.

Will do.
Andy Shevchenko Jan. 14, 2024, 2:04 p.m. UTC | #3 
On Mon, Jan 08, 2024 at 12:09:10PM -0700, Mark Hasemeyer wrote:
> On Sat, Jan 6, 2024 at 7:56 AM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Tue, Jan 02, 2024 at 02:07:47PM -0700, Mark Hasemeyer wrote:

..

> > > +     ret = fwnode_irq_get_resource(fwnode, num, r);
> >
> > I still prefer this not to return positive value. Since you _require_ @r to be
> > not NULL, i.e. valid, the returning positive value makes no sense.
> >
> > > +     ret = ret < 0 ? ret : 0;
> > > +     if (!ret || ret == -EPROBE_DEFER)
> > > +             goto out;
> 
> I agree. But echoing my response from v3 patch 24:
> - The fwnode patch is already reviewed and approved.
> - The fwnode patch uses of_irq_to_resource() which already existed and
> returns the irq number on success. The error handling translation will
> just get pushed to the fwnode subsystem unless of_irq_to_resource() is
> also modified which means updating 8 or so drivers that reference it.
> 
> I can either:
> -Leave it
> -Modify the fwnode subsystem to perform the error translation of
> of_irq_to_resource()
> -Modify the fwnode and OF subsystems and update all driver references
> 
> The fwnode and OF patches are already reviewed. I imagine coding
> changes would imply dropping any Reviewed-by tags and requesting
> another review?
> I'd really prefer to not blow up the patch series anymore, but if you
> feel strongly, we can come up with a solution.

fwnode is quite generic API and I won't fail it from day 1.
Yet we have already some deviations in fwnode/device vs. OF/ACPI cases
(first comes to mind is device_for_each_child_node() which assumes
 "availability").

So, I would prefer fwnode API to not inherit issues/features of OF specific
code. Maybe this can be considered as "yes, please update it".
diff mbox series

Patch

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 10c5779634182..8a42b48922e68 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -151,50 +151,45 @@  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
- * 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.
+ * value for errors. If no error is returned, the IRQ can be found in r->start.
  *
  * For example::
  *
- *		int irq = platform_get_irq_optional(pdev, 0);
- *		if (irq < 0)
- *			return irq;
+ *		int res = platform_get_irq_resource_optional(pdev, 0, &res);
+ *		if (!res)
+ *			return res.start;
  *
- * Return: non-zero IRQ number on success, negative error number on failure.
+ * Return: 0 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 (!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];
+	*r = DEFINE_RES_IRQ(dev->archdata.irqs[num]);
+	ret = 0;
 	goto out;
 #else
 	struct fwnode_handle *fwnode = dev_fwnode(&dev->dev);
-	struct resource *r;
 
-	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);
+	ret = ret < 0 ? ret : 0;
+	if (!ret || ret == -EPROBE_DEFER)
+		goto out;
 
 	/*
 	 * The resources may pass trigger flags to the irqs that need
@@ -202,7 +197,9 @@  int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
 	 * IORESOURCE_BITS correspond 1-to-1 to the IRQF_TRIGGER*
 	 * settings.
 	 */
-	if (r && r->flags & IORESOURCE_BITS) {
+	struct resource *platform_res = platform_get_resource(dev, IORESOURCE_IRQ, num);
+
+	if (platform_res && platform_res->flags & IORESOURCE_BITS) {
 		struct irq_data *irqd;
 
 		irqd = irq_get_irq_data(r->start);
@@ -211,8 +208,9 @@  int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
 		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
 	}
 
-	if (r) {
-		ret = r->start;
+	if (platform_res) {
+		*r = *platform_res;
+		ret = 0;
 		goto out;
 	}
 
@@ -224,9 +222,9 @@  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)
 			goto out;
 	}
 
@@ -234,11 +232,11 @@  int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
 out_not_found:
 	ret = -ENXIO;
 out:
-	if (WARN(!ret, "0 is an invalid IRQ number\n"))
+	if (WARN(!ret && !r->start, "0 is an invalid IRQ number\n"))
 		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 +268,34 @@  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)
+{
+	int ret;
+	struct resource r = {};
+
+	ret = platform_get_irq_resource_optional(dev, num, &r);
+	return ret ?: r.start;
+}
+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,