| Message ID | 20190102185106.56913-1-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] driver core: platform: Add an error message to platform_get_irq*() | expand |
On Wed, Jan 2, 2019 at 7:51 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. > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- > > Changes from v2: > * Don't refactor platform_get_irq(), just wrap it > > Changes from v1: > * Update error text to indicate irq index instead of IRQn, use %u > > drivers/base/platform.c | 31 +++++++++++++++++++++++-------- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 1c958eb33ef4..388461306dd4 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -79,12 +79,7 @@ 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) > { > #ifdef CONFIG_SPARC > /* sparc does not have irqs represented as IORESOURCE_IRQ resources */ > @@ -130,6 +125,22 @@ int platform_get_irq(struct platform_device *dev, unsigned int num) > return r ? r->start : -ENXIO; > #endif > } > + > +/** > + * 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) > +{ > + int ret; > + > + ret = __platform_get_irq(dev, num); > + if (ret < 0 && ret != -EPROBE_DEFER) > + dev_err(&dev->dev, "IRQ index %u not found\n", num); Why don't you log the error code too? > + > + return ret; > +} > EXPORT_SYMBOL_GPL(platform_get_irq); > > /** > @@ -142,7 +153,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)) >= 0) > nr++; > > if (ret == -EPROBE_DEFER) > @@ -195,7 +206,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); > > -- > Sent by a computer through tubes >
Quoting Rafael J. Wysocki (2019-01-03 01:40:26) > On Wed, Jan 2, 2019 at 7:51 PM Stephen Boyd <swboyd@chromium.org> wrote: > > @@ -130,6 +125,22 @@ int platform_get_irq(struct platform_device *dev, unsigned int num) > > return r ? r->start : -ENXIO; > > #endif > > } > > + > > +/** > > + * 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) > > +{ > > + int ret; > > + > > + ret = __platform_get_irq(dev, num); > > + if (ret < 0 && ret != -EPROBE_DEFER) > > + dev_err(&dev->dev, "IRQ index %u not found\n", num); > > Why don't you log the error code too? > I don't see much benefit to seeing -ENXIO or -EINVAL printed here, so I left out the error code.
On Thu, Jan 3, 2019 at 5:12 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Rafael J. Wysocki (2019-01-03 01:40:26) > > On Wed, Jan 2, 2019 at 7:51 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > @@ -130,6 +125,22 @@ int platform_get_irq(struct platform_device *dev, unsigned int num) > > > return r ? r->start : -ENXIO; > > > #endif > > > } > > > + > > > +/** > > > + * 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) > > > +{ > > > + int ret; > > > + > > > + ret = __platform_get_irq(dev, num); > > > + if (ret < 0 && ret != -EPROBE_DEFER) > > > + dev_err(&dev->dev, "IRQ index %u not found\n", num); > > > > Why don't you log the error code too? > > > > I don't see much benefit to seeing -ENXIO or -EINVAL printed here, so I > left out the error code. OK, so the value of the message is to tell the user that some driver asked for an invalid IRQ, right?
Quoting Rafael J. Wysocki (2019-01-03 09:22:56) > On Thu, Jan 3, 2019 at 5:12 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > I don't see much benefit to seeing -ENXIO or -EINVAL printed here, so I > > left out the error code. > > OK, so the value of the message is to tell the user that some driver > asked for an invalid IRQ, right? Yes.
On Thu, Jan 3, 2019 at 6:25 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Rafael J. Wysocki (2019-01-03 09:22:56) > > On Thu, Jan 3, 2019 at 5:12 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > I don't see much benefit to seeing -ENXIO or -EINVAL printed here, so I > > > left out the error code. > > > > OK, so the value of the message is to tell the user that some driver > > asked for an invalid IRQ, right? > > Yes. OK This can help to reduce code duplication a bit, so Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> for the v3.
On Wed, Jan 2, 2019 at 8:51 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. > Thanks, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- > > Changes from v2: > * Don't refactor platform_get_irq(), just wrap it > > Changes from v1: > * Update error text to indicate irq index instead of IRQn, use %u > > drivers/base/platform.c | 31 +++++++++++++++++++++++-------- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 1c958eb33ef4..388461306dd4 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -79,12 +79,7 @@ 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) > { > #ifdef CONFIG_SPARC > /* sparc does not have irqs represented as IORESOURCE_IRQ resources */ > @@ -130,6 +125,22 @@ int platform_get_irq(struct platform_device *dev, unsigned int num) > return r ? r->start : -ENXIO; > #endif > } > + > +/** > + * 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) > +{ > + int ret; > + > + ret = __platform_get_irq(dev, num); > + if (ret < 0 && ret != -EPROBE_DEFER) > + dev_err(&dev->dev, "IRQ index %u not found\n", num); > + > + return ret; > +} > EXPORT_SYMBOL_GPL(platform_get_irq); > > /** > @@ -142,7 +153,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)) >= 0) > nr++; > > if (ret == -EPROBE_DEFER) > @@ -195,7 +206,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); > > -- > Sent by a computer through tubes >
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 1c958eb33ef4..388461306dd4 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -79,12 +79,7 @@ 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) { #ifdef CONFIG_SPARC /* sparc does not have irqs represented as IORESOURCE_IRQ resources */ @@ -130,6 +125,22 @@ int platform_get_irq(struct platform_device *dev, unsigned int num) return r ? r->start : -ENXIO; #endif } + +/** + * 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) +{ + int ret; + + ret = __platform_get_irq(dev, num); + if (ret < 0 && ret != -EPROBE_DEFER) + dev_err(&dev->dev, "IRQ index %u not found\n", num); + + return ret; +} EXPORT_SYMBOL_GPL(platform_get_irq); /** @@ -142,7 +153,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)) >= 0) nr++; if (ret == -EPROBE_DEFER) @@ -195,7 +206,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 v2: * Don't refactor platform_get_irq(), just wrap it Changes from v1: * Update error text to indicate irq index instead of IRQn, use %u drivers/base/platform.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-)