Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 21 Dec 2023 17:33:20 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v2 21/22] platform: Modify platform_get_irq_optional() to use resource |
| |
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
| ![\](/images/icornerr.gif) |