* [PATCH v1 0/2] devres: A couple of cleanups @ 2024-02-27 17:58 Andy Shevchenko 2024-02-27 17:58 ` [PATCH v1 1/2] devres: Switch to use dev_err_probe() for unification Andy Shevchenko 2024-02-27 17:58 ` [PATCH v1 2/2] devres: Don't use "proxy" headers Andy Shevchenko 0 siblings, 2 replies; 6+ messages in thread From: Andy Shevchenko @ 2024-02-27 17:58 UTC (permalink / raw) To: Andy Shevchenko, Philipp Stanner, linux-kernel Cc: Andrew Morton, Rasmus Villemoes A couple of ad-hoc cleanups. No functional changes intended. Andy Shevchenko (2): devres: Switch to use dev_err_probe() for unification devres: Don't use "proxy" headers lib/devres.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) -- 2.43.0.rc1.1.gbec44491f096 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v1 1/2] devres: Switch to use dev_err_probe() for unification 2024-02-27 17:58 [PATCH v1 0/2] devres: A couple of cleanups Andy Shevchenko @ 2024-02-27 17:58 ` Andy Shevchenko 2024-02-29 8:52 ` Philipp Stanner 2024-02-27 17:58 ` [PATCH v1 2/2] devres: Don't use "proxy" headers Andy Shevchenko 1 sibling, 1 reply; 6+ messages in thread From: Andy Shevchenko @ 2024-02-27 17:58 UTC (permalink / raw) To: Andy Shevchenko, Philipp Stanner, linux-kernel Cc: Andrew Morton, Rasmus Villemoes The devm_*() APIs are supposed to be called during the ->probe() stage. Many drivers (especially new ones) has switched to use dev_err_probe() for error messaging for the sake of unification. Let's do the same in the devres APIs. 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); } 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; -- 2.43.0.rc1.1.gbec44491f096 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/2] devres: Switch to use dev_err_probe() for unification 2024-02-27 17:58 ` [PATCH v1 1/2] devres: Switch to use dev_err_probe() for unification Andy Shevchenko @ 2024-02-29 8:52 ` Philipp Stanner 2024-02-29 10:39 ` Andy Shevchenko 0 siblings, 1 reply; 6+ messages in thread From: Philipp Stanner @ 2024-02-29 8:52 UTC (permalink / raw) To: Andy Shevchenko, linux-kernel; +Cc: Andrew Morton, Rasmus Villemoes, pstanner 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; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/2] devres: Switch to use dev_err_probe() for unification 2024-02-29 8:52 ` Philipp Stanner @ 2024-02-29 10:39 ` Andy Shevchenko 2024-02-29 10:41 ` Andy Shevchenko 0 siblings, 1 reply; 6+ messages in thread From: Andy Shevchenko @ 2024-02-29 10:39 UTC (permalink / raw) To: Philipp Stanner; +Cc: linux-kernel, Andrew Morton, Rasmus Villemoes On Thu, Feb 29, 2024 at 09:52:34AM +0100, Philipp Stanner wrote: > 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 Thanks, will fix. > > 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: ... > > + 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? dev_err_probe() requires error code as a parameter. Are you suggesting to have a duplication? dev_err_probe(-EINVAL); return -EINVAL; I don't think it's a good suggestion, so the answer is "No, I don't think it would be better." > 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. Same answer. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/2] devres: Switch to use dev_err_probe() for unification 2024-02-29 10:39 ` Andy Shevchenko @ 2024-02-29 10:41 ` Andy Shevchenko 0 siblings, 0 replies; 6+ messages in thread From: Andy Shevchenko @ 2024-02-29 10:41 UTC (permalink / raw) To: Philipp Stanner; +Cc: linux-kernel, Andrew Morton, Rasmus Villemoes On Thu, Feb 29, 2024 at 12:39:55PM +0200, Andy Shevchenko wrote: > On Thu, Feb 29, 2024 at 09:52:34AM +0100, Philipp Stanner wrote: > > On Tue, 2024-02-27 at 19:58 +0200, Andy Shevchenko wrote: ... > > 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(). And to this, it's a common pattern in some cases, esp. when you have a chain of the similar calls and you want to simplify the code (see CCI accessors in Video4Linux2 as an example). So, I also don't think it's something unusual. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v1 2/2] devres: Don't use "proxy" headers 2024-02-27 17:58 [PATCH v1 0/2] devres: A couple of cleanups Andy Shevchenko 2024-02-27 17:58 ` [PATCH v1 1/2] devres: Switch to use dev_err_probe() for unification Andy Shevchenko @ 2024-02-27 17:58 ` Andy Shevchenko 1 sibling, 0 replies; 6+ messages in thread From: Andy Shevchenko @ 2024-02-27 17:58 UTC (permalink / raw) To: Andy Shevchenko, Philipp Stanner, linux-kernel Cc: Andrew Morton, Rasmus Villemoes Update header inclusions to follow IWYU (Include What You Use) principle. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- lib/devres.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/devres.c b/lib/devres.c index 27f280a39dca..4fc152de6d8b 100644 --- a/lib/devres.c +++ b/lib/devres.c @@ -1,10 +1,13 @@ // SPDX-License-Identifier: GPL-2.0 +#include <linux/bug.h> #include <linux/device.h> -#include <linux/err.h> -#include <linux/io.h> -#include <linux/gfp.h> +#include <linux/errno.h> #include <linux/export.h> +#include <linux/gfp_types.h> +#include <linux/io.h> +#include <linux/ioport.h> #include <linux/of_address.h> +#include <linux/types.h> enum devm_ioremap_type { DEVM_IOREMAP = 0, -- 2.43.0.rc1.1.gbec44491f096 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-02-29 10:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-02-27 17:58 [PATCH v1 0/2] devres: A couple of cleanups Andy Shevchenko 2024-02-27 17:58 ` [PATCH v1 1/2] devres: Switch to use dev_err_probe() for unification Andy Shevchenko 2024-02-29 8:52 ` Philipp Stanner 2024-02-29 10:39 ` Andy Shevchenko 2024-02-29 10:41 ` Andy Shevchenko 2024-02-27 17:58 ` [PATCH v1 2/2] devres: Don't use "proxy" headers Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox