Messages in this thread | | | Subject | Re: [PATCH v1 1/2] devres: Switch to use dev_err_probe() for unification | From | Philipp Stanner <> | Date | Thu, 29 Feb 2024 09:52:34 +0100 |
| |
On Tue, 2024-02-27 at 19:58 +0200, Andy Shevchenko wrote: > The devm_*() APIs are supposed to be called during the ->probe() > stage. > Many drivers (especially new ones) has switched to use
has -> have
> dev_err_probe() > for error messaging for the sake of unification. Let's do the same in > the devres APIs.
No objections on principle. Just one thing about the implementation:
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > lib/devres.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/lib/devres.c b/lib/devres.c > index fe0c63caeb68..27f280a39dca 100644 > --- a/lib/devres.c > +++ b/lib/devres.c > @@ -125,12 +125,13 @@ __devm_ioremap_resource(struct device *dev, > const struct resource *res, > resource_size_t size; > void __iomem *dest_ptr; > char *pretty_name; > + int ret; > > BUG_ON(!dev); > > if (!res || resource_type(res) != IORESOURCE_MEM) { > - dev_err(dev, "invalid resource %pR\n", res); > - return IOMEM_ERR_PTR(-EINVAL); > + ret = dev_err_probe(dev, -EINVAL, "invalid resource > %pR\n", res); > + return IOMEM_ERR_PTR(ret);
So as I see it -EINVAL is just piped through dev_err_probe() and is never changed. Don't you think it would be better to drop variable 'ret' and just do return IOMEM_ERR_PTR(-EINVAL); as before?
That way it would be obvious that the error code is never changed and it will always return -EINVAL. Otherwise you have to look up the function definition of dev_err_probe().
The same would apply below.
Regards, P.
> } > > if (type == DEVM_IOREMAP && res->flags & > IORESOURCE_MEM_NONPOSTED) > @@ -144,20 +145,20 @@ __devm_ioremap_resource(struct device *dev, > const struct resource *res, > else > pretty_name = devm_kstrdup(dev, dev_name(dev), > GFP_KERNEL); > if (!pretty_name) { > - dev_err(dev, "can't generate pretty name for resource > %pR\n", res); > - return IOMEM_ERR_PTR(-ENOMEM); > + ret = dev_err_probe(dev, -ENOMEM, "can't generate > pretty name for resource %pR\n", res); > + return IOMEM_ERR_PTR(ret); > } > > if (!devm_request_mem_region(dev, res->start, size, > pretty_name)) { > - dev_err(dev, "can't request region for resource > %pR\n", res); > - return IOMEM_ERR_PTR(-EBUSY); > + ret = dev_err_probe(dev, -EBUSY, "can't request > region for resource %pR\n", res); > + return IOMEM_ERR_PTR(ret); > } > > dest_ptr = __devm_ioremap(dev, res->start, size, type); > if (!dest_ptr) { > - dev_err(dev, "ioremap failed for resource %pR\n", > res); > devm_release_mem_region(dev, res->start, size); > - dest_ptr = IOMEM_ERR_PTR(-ENOMEM); > + ret = dev_err_probe(dev, -ENOMEM, "ioremap failed for > resource %pR\n", res); > + return IOMEM_ERR_PTR(ret); > } > > return dest_ptr;
| |