[v1,5/6] platform: Modify platform_get_irq_optional() to use resource
Commit Message
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.
Rename the function to platform_get_irq_resource() to better describe
the function's new behavior.
Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---
drivers/base/platform.c | 78 ++++++++++++++++++---------------
include/linux/platform_device.h | 9 +++-
2 files changed, 50 insertions(+), 37 deletions(-)
Comments
On Wed, Dec 13, 2023 at 11:00:23AM -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.
IRQ
> For example, whether or not an irq is wake capable.
IRQ
> Rename the function to platform_get_irq_resource() to better describe
> the function's new behavior.
...
> - * platform_get_irq_optional - get an optional IRQ for a device
> + * platform_get_irq_resource - get an IRQ for a device and populate resource struct
> * @dev: platform device
> * @num: IRQ number index
> + * @r: pointer to resource to populate with irq information. It is not modified on failure.
IRQ
It's inconsistent with just a few lines above!
Also same comment about second remark, no need to have it. It's implied.
...
> + *r = (struct resource)DEFINE_RES_IRQ(ret);
Why is the annotation needed?
...
> - struct resource *r;
> + struct resource *platform_res;
>
> if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node) {
> - ret = of_irq_get(dev->dev.of_node, num);
> + ret = of_irq_to_resource(dev->dev.of_node, num, r);
> if (ret > 0 || ret == -EPROBE_DEFER)
> goto out;
> }
> + if (has_acpi_companion(&dev->dev)) {
> + ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> + if (!ret || ret == -EPROBE_DEFER) {
> + ret = ret ?: r->start;
> + goto out;
> + }
> + }
Consider combine the above to use fwnode_irq_get() in the separate prerequisite
change.
...
> + /*
> + * 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 (ret > 0 && r->flags & IORESOURCE_BITS) {
> + struct irq_data *irqd;
> + irqd = irq_get_irq_data(r->start);
> + if (!irqd)
> + ret = -ENXIO;
> + else
Redundant.
Just return directly from the above.
> + irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
NIH resource_type().
> + }
> return ret;
...
What you need to add to all the functions is either
- check that resource pointer is not NULL, and/or
- documentation that explain this requirement or mark it optional
(but second seems nonsense)
On Wed, Dec 13, 2023 at 11:00:23AM -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.
>
> Rename the function to platform_get_irq_resource() to better describe
> the function's new behavior.
This is misleading as the original function is still there.
The get_optional() functions are designed to not print an error message
where as the non-optional variant will. You've broken that pattern here
in that there is no platform_get_irq_resource_optional() (at least named
that because your implementation is that since there is no error
message).
What about versions equivalent to platform_get_irq_byname()
and platform_get_irq_byname_optional(), though I guess we need users
first.
>
> Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
> ---
>
> drivers/base/platform.c | 78 ++++++++++++++++++---------------
> include/linux/platform_device.h | 9 +++-
> 2 files changed, 50 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 76bfcba250039..6b58bde776d4f 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -151,9 +151,10 @@ 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 - get an IRQ for a device and populate resource struct
> * @dev: platform device
> * @num: IRQ number index
> + * @r: pointer to resource to populate with irq information. It is not modified on failure.
> *
> * 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,59 +163,47 @@ EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
> *
> * For example::
> *
> - * int irq = platform_get_irq_optional(pdev, 0);
> + * int irq = platform_get_irq_resource(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(struct platform_device *dev, unsigned int num, struct resource *r)
> {
> int ret;
> #ifdef CONFIG_SPARC
> /* sparc does not have irqs represented as IORESOURCE_IRQ resources */
> if (!dev || num >= dev->archdata.num_irqs)
> - goto out_not_found;
> + return -ENXIO;
> ret = dev->archdata.irqs[num];
> + if (ret >= 0)
> + *r = (struct resource)DEFINE_RES_IRQ(ret);
> goto out;
> #else
> - struct resource *r;
> + struct resource *platform_res;
>
> if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node) {
> - ret = of_irq_get(dev->dev.of_node, num);
> + ret = of_irq_to_resource(dev->dev.of_node, num, r);
> if (ret > 0 || ret == -EPROBE_DEFER)
> goto out;
> }
>
> - r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> - if (has_acpi_companion(&dev->dev)) {
> - if (r && r->flags & IORESOURCE_DISABLED) {
> - ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> - if (ret)
> - goto out;
> - }
> - }
> -
> - /*
> - * 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) {
> - struct irq_data *irqd;
> -
> - irqd = irq_get_irq_data(r->start);
> - if (!irqd)
> - goto out_not_found;
> - irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
> - }
> -
> - if (r) {
> + platform_res = platform_get_resource(dev, IORESOURCE_IRQ, num);
> + if (platform_res && !(platform_res->flags & IORESOURCE_DISABLED)) {
> + *r = *platform_res;
> ret = r->start;
> goto out;
> }
>
> + if (has_acpi_companion(&dev->dev)) {
> + ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> + if (!ret || ret == -EPROBE_DEFER) {
> + ret = ret ?: r->start;
> + goto out;
> + }
> + }
> +
> /*
> * For the index 0 interrupt, allow falling back to GpioInt
> * resources. While a device could have both Interrupt and GpioInt
> @@ -223,21 +212,38 @@ 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 && has_acpi_companion(&dev->dev)) {
> - ret = acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
> + ret = acpi_dev_get_gpio_irq_resource(ACPI_COMPANION(&dev->dev), 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
> -out_not_found:
> ret = -ENXIO;
> out:
> if (WARN(!ret, "0 is an invalid IRQ number\n"))
> return -EINVAL;
> +
> + /*
> + * 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 (ret > 0 && r->flags & IORESOURCE_BITS) {
> + struct irq_data *irqd;
> +
> + irqd = irq_get_irq_data(r->start);
> + if (!irqd)
> + ret = -ENXIO;
> + else
> + irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
We were not doing any of this in the DT or Sparc cases before. It's
probably just redundant for DT. It might break Sparc.
Rob
> > + *r = (struct resource)DEFINE_RES_IRQ(ret);
>
> Why is the annotation needed?
It's not needed anymore. I'll remove it.
> > - struct resource *r;
> > + struct resource *platform_res;
> >
> > if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node) {
> > - ret = of_irq_get(dev->dev.of_node, num);
> > + ret = of_irq_to_resource(dev->dev.of_node, num, r);
> > if (ret > 0 || ret == -EPROBE_DEFER)
> > goto out;
> > }
>
>
> > + if (has_acpi_companion(&dev->dev)) {
> > + ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> > + if (!ret || ret == -EPROBE_DEFER) {
> > + ret = ret ?: r->start;
> > + goto out;
> > + }
> > + }
>
> Consider combine the above to use fwnode_irq_get() in the separate prerequisite
> change.
I like the idea. It doesn't look like 'struct fwnode_operations'
provides a way to retrieve information in a 'struct resource' format.
Perhaps this could be followed up on in a separate patch train?
>
> > + irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
>
> NIH resource_type().
>
> > + }
> > return ret;
Does NIH mean "not invented here"? resource_type() masks
IORESOURCE_TYPE_BITS, not IORESOURCE_BITS. I'm not quite sure what you
mean here.
On Mon, Dec 18, 2023 at 01:23:35PM -0700, Mark Hasemeyer wrote:
...
> > Consider combine the above to use fwnode_irq_get() in the separate prerequisite
> > change.
>
> I like the idea. It doesn't look like 'struct fwnode_operations'
> provides a way to retrieve information in a 'struct resource' format.
It can be added, but it's orthogonal to this series. What I suggest is
to unify them here followed by your patch in this series. I.o.w. add
one more patch as a prerequisite.
> Perhaps this could be followed up on in a separate patch train?
Definitely, but again, not (directly) related to this series.
...
> > > + irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
> >
> > NIH resource_type().
> >
> > > + }
> > > return ret;
>
> Does NIH mean "not invented here"?
Yes.
> resource_type() masks
> IORESOURCE_TYPE_BITS, not IORESOURCE_BITS. I'm not quite sure what you
> mean here.
Ah, my bad, indeed, you are right.
@@ -151,9 +151,10 @@ 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 - get an IRQ for a device and populate resource struct
* @dev: platform device
* @num: IRQ number index
+ * @r: pointer to resource to populate with irq information. It is not modified on failure.
*
* 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,59 +163,47 @@ EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
*
* For example::
*
- * int irq = platform_get_irq_optional(pdev, 0);
+ * int irq = platform_get_irq_resource(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(struct platform_device *dev, unsigned int num, struct resource *r)
{
int ret;
#ifdef CONFIG_SPARC
/* sparc does not have irqs represented as IORESOURCE_IRQ resources */
if (!dev || num >= dev->archdata.num_irqs)
- goto out_not_found;
+ return -ENXIO;
ret = dev->archdata.irqs[num];
+ if (ret >= 0)
+ *r = (struct resource)DEFINE_RES_IRQ(ret);
goto out;
#else
- struct resource *r;
+ struct resource *platform_res;
if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node) {
- ret = of_irq_get(dev->dev.of_node, num);
+ ret = of_irq_to_resource(dev->dev.of_node, num, r);
if (ret > 0 || ret == -EPROBE_DEFER)
goto out;
}
- r = platform_get_resource(dev, IORESOURCE_IRQ, num);
- if (has_acpi_companion(&dev->dev)) {
- if (r && r->flags & IORESOURCE_DISABLED) {
- ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
- if (ret)
- goto out;
- }
- }
-
- /*
- * 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) {
- struct irq_data *irqd;
-
- irqd = irq_get_irq_data(r->start);
- if (!irqd)
- goto out_not_found;
- irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
- }
-
- if (r) {
+ platform_res = platform_get_resource(dev, IORESOURCE_IRQ, num);
+ if (platform_res && !(platform_res->flags & IORESOURCE_DISABLED)) {
+ *r = *platform_res;
ret = r->start;
goto out;
}
+ if (has_acpi_companion(&dev->dev)) {
+ ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
+ if (!ret || ret == -EPROBE_DEFER) {
+ ret = ret ?: r->start;
+ goto out;
+ }
+ }
+
/*
* For the index 0 interrupt, allow falling back to GpioInt
* resources. While a device could have both Interrupt and GpioInt
@@ -223,21 +212,38 @@ 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 && has_acpi_companion(&dev->dev)) {
- ret = acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
+ ret = acpi_dev_get_gpio_irq_resource(ACPI_COMPANION(&dev->dev), 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
-out_not_found:
ret = -ENXIO;
out:
if (WARN(!ret, "0 is an invalid IRQ number\n"))
return -EINVAL;
+
+ /*
+ * 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 (ret > 0 && r->flags & IORESOURCE_BITS) {
+ struct irq_data *irqd;
+
+ irqd = irq_get_irq_data(r->start);
+ if (!irqd)
+ ret = -ENXIO;
+ else
+ irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
+ }
return ret;
}
-EXPORT_SYMBOL_GPL(platform_get_irq_optional);
+EXPORT_SYMBOL_GPL(platform_get_irq_resource);
/**
* platform_get_irq - get an IRQ for a device
@@ -101,7 +101,14 @@ devm_platform_ioremap_resource_byname(struct platform_device *pdev,
#endif
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(struct platform_device *dev, unsigned int num,
+ struct resource *r);
+static inline int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+{
+ struct resource r;
+
+ return platform_get_irq_resource(dev, num, &r);
+}
extern int platform_irq_count(struct platform_device *);
extern int devm_platform_get_irqs_affinity(struct platform_device *dev,
struct irq_affinity *affd,