| Message ID | 20181228215620.120672-1-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] driver core: platform: Add an error message to platform_get_irq*() | expand |
On Fri, Dec 28, 2018 at 11:56 PM Stephen Boyd <swboyd@chromium.org> wrote: > > A grep of the kernel shows that many drivers print an error message if > they fail to get the irq they're looking for. Furthermore, those drivers > all decide to print the device name, or not, and the irq they were > requesting, or not, etc. Let's consolidate all these error messages into > the API itself, allowing us to get rid of the error messages in each > driver. > +static int __platform_get_irq(struct platform_device *dev, unsigned int num, bool warn) > { > +error: > + if (warn) > + dev_err(&dev->dev, "IRQ index %u not found\n", num); > + > + return ret; > +} > + > +/** > + * platform_get_irq - get an IRQ for a device > + * @dev: platform device > + * @num: IRQ number index > + */ > +int platform_get_irq(struct platform_device *dev, unsigned int num) > +{ > + return __platform_get_irq(dev, num, true); Hmm... Why not just do int ret = __plaform_get_irq(); if (ret) dev_err(); return ret; instead of big refactoring of platform_get_irq()? > }
Quoting Andy Shevchenko (2018-12-30 02:42:39) > On Fri, Dec 28, 2018 at 11:56 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > A grep of the kernel shows that many drivers print an error message if > > they fail to get the irq they're looking for. Furthermore, those drivers > > all decide to print the device name, or not, and the irq they were > > requesting, or not, etc. Let's consolidate all these error messages into > > the API itself, allowing us to get rid of the error messages in each > > driver. > > > +static int __platform_get_irq(struct platform_device *dev, unsigned int num, bool warn) > > { > > > +error: > > + if (warn) > > + dev_err(&dev->dev, "IRQ index %u not found\n", num); > > + > > + return ret; > > +} > > + > > +/** > > + * platform_get_irq - get an IRQ for a device > > + * @dev: platform device > > + * @num: IRQ number index > > + */ > > +int platform_get_irq(struct platform_device *dev, unsigned int num) > > +{ > > + return __platform_get_irq(dev, num, true); > > Hmm... Why not just do > int ret = __plaform_get_irq(); > if (ret) > dev_err(); > return ret; > > instead of big refactoring of platform_get_irq()? Sure. Thanks for the suggestion. But we need to check for ret < 0 I suppose so that we don't print an error message for all the irq numbers. I'll send the updated patch as a v3.
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 1c958eb33ef4..e7af7cd63d0b 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -79,23 +79,18 @@ struct resource *platform_get_resource(struct platform_device *dev, } EXPORT_SYMBOL_GPL(platform_get_resource); -/** - * platform_get_irq - get an IRQ for a device - * @dev: platform device - * @num: IRQ number index - */ -int platform_get_irq(struct platform_device *dev, unsigned int num) +static int __platform_get_irq(struct platform_device *dev, unsigned int num, bool warn) { + int ret = -ENXIO; + #ifdef CONFIG_SPARC /* sparc does not have irqs represented as IORESOURCE_IRQ resources */ if (!dev || num >= dev->archdata.num_irqs) - return -ENXIO; + goto error; return dev->archdata.irqs[num]; #else struct resource *r; if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node) { - int ret; - ret = of_irq_get(dev->dev.of_node, num); if (ret > 0 || ret == -EPROBE_DEFER) return ret; @@ -104,11 +99,11 @@ int platform_get_irq(struct platform_device *dev, unsigned int num) r = platform_get_resource(dev, IORESOURCE_IRQ, num); if (has_acpi_companion(&dev->dev)) { if (r && r->flags & IORESOURCE_DISABLED) { - int ret; - ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r); - if (ret) + if (ret > 0 || ret == -EPROBE_DEFER) return ret; + if (ret) + goto error; } } @@ -122,13 +117,32 @@ int platform_get_irq(struct platform_device *dev, unsigned int num) struct irq_data *irqd; irqd = irq_get_irq_data(r->start); - if (!irqd) - return -ENXIO; + if (!irqd) { + ret = -ENXIO; + goto error; + } + irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS); } - return r ? r->start : -ENXIO; + if (r) + return r->start; #endif +error: + if (warn) + dev_err(&dev->dev, "IRQ index %u not found\n", num); + + return ret; +} + +/** + * platform_get_irq - get an IRQ for a device + * @dev: platform device + * @num: IRQ number index + */ +int platform_get_irq(struct platform_device *dev, unsigned int num) +{ + return __platform_get_irq(dev, num, true); } EXPORT_SYMBOL_GPL(platform_get_irq); @@ -142,7 +156,7 @@ int platform_irq_count(struct platform_device *dev) { int ret, nr = 0; - while ((ret = platform_get_irq(dev, nr)) >= 0) + while ((ret = __platform_get_irq(dev, nr, false)) >= 0) nr++; if (ret == -EPROBE_DEFER) @@ -195,7 +209,11 @@ int platform_get_irq_byname(struct platform_device *dev, const char *name) } r = platform_get_resource_byname(dev, IORESOURCE_IRQ, name); - return r ? r->start : -ENXIO; + if (r) + return r->start; + + dev_err(&dev->dev, "IRQ %s not found\n", name); + return -ENXIO; } EXPORT_SYMBOL_GPL(platform_get_irq_byname);
A grep of the kernel shows that many drivers print an error message if they fail to get the irq they're looking for. Furthermore, those drivers all decide to print the device name, or not, and the irq they were requesting, or not, etc. Let's consolidate all these error messages into the API itself, allowing us to get rid of the error messages in each driver. Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- Changes from v1: * Update error text to indicate irq index instead of IRQn, use %u drivers/base/platform.c | 52 +++++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 17 deletions(-)