| Message ID | 20210902063702.32066-2-caihuoqing@baidu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drivers: Add the helper function devm_platform_get_and_ioremap_resource_byname() | expand |
On Thu, Sep 02, 2021 at 02:37:00PM +0800, Cai Huoqing wrote: > Since provide the helper function devm_platform_ioremap_resource_byname() > which is wrap platform_get_resource_byname() and devm_ioremap_resource(). > But sometimes, many drivers still need to use the resource variables > obtained by platform_get_resource(). In these cases, provide this helper > function devm_platform_get_and_ioremap_resource_byname(). > > Signed-off-by: Cai Huoqing <caihuoqing@baidu.com> > --- > v1->v2: Resend this patch as part of a patch series that uses > the new function. > > drivers/base/platform.c | 30 ++++++++++++++++++++++++++---- > include/linux/platform_device.h | 3 +++ > 2 files changed, 29 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 652531f67135..34bb581338d9 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -124,6 +124,31 @@ void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev, > } > EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource); > > +/** > + * devm_platform_get_and_ioremap_resource_byname - call devm_ioremap_resource() for a > + * platform device and get resource > + * > + * @pdev: platform device to use both for memory resource lookup as well as > + * resource management > + * @name: name of the resource > + * @res: optional output parameter to store a pointer to the obtained resource. > + * > + * Return: a pointer to the remapped memory or an ERR_PTR() encoded error code > + * on failure. > + */ > +void __iomem * > +devm_platform_get_and_ioremap_resource_byname(struct platform_device *pdev, > + const char *name, struct resource **res) > +{ > + struct resource *r; > + > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, name); > + if (res) > + *res = r; You forgot to check the return value of this call :( Which means you did not test this? Why not? But step back, _WHY_ is this needed at all? How deep are we going to get with the "devm_platform_get_and_do_this_and_that_and_that" type functions here? You show 2 users of this call, and they save what, 1-2 lines of code here? What is the real need for this? thanks, greg k-h
On 02 Sep 21 08:52:45, Greg KH wrote: > On Thu, Sep 02, 2021 at 02:37:00PM +0800, Cai Huoqing wrote: > > Since provide the helper function devm_platform_ioremap_resource_byname() > > which is wrap platform_get_resource_byname() and devm_ioremap_resource(). > > But sometimes, many drivers still need to use the resource variables > > obtained by platform_get_resource(). In these cases, provide this helper > > function devm_platform_get_and_ioremap_resource_byname(). > > > > Signed-off-by: Cai Huoqing <caihuoqing@baidu.com> > > --- > > v1->v2: Resend this patch as part of a patch series that uses > > the new function. > > > > drivers/base/platform.c | 30 ++++++++++++++++++++++++++---- > > include/linux/platform_device.h | 3 +++ > > 2 files changed, 29 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > > index 652531f67135..34bb581338d9 100644 > > --- a/drivers/base/platform.c > > +++ b/drivers/base/platform.c > > @@ -124,6 +124,31 @@ void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev, > > } > > EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource); > > > > +/** > > + * devm_platform_get_and_ioremap_resource_byname - call devm_ioremap_resource() for a > > + * platform device and get resource > > + * > > + * @pdev: platform device to use both for memory resource lookup as well as > > + * resource management > > + * @name: name of the resource > > + * @res: optional output parameter to store a pointer to the obtained resource. > > + * > > + * Return: a pointer to the remapped memory or an ERR_PTR() encoded error code > > + * on failure. > > + */ > > +void __iomem * > > +devm_platform_get_and_ioremap_resource_byname(struct platform_device *pdev, > > + const char *name, struct resource **res) > > +{ > > + struct resource *r; > > + > > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, name); > > + if (res) > > + *res = r; > > You forgot to check the return value of this call :( devm_ioremap_resource wiil check it and print error message, here: ./lib/devres.c:136:__devm_ioremap_resource( if (!res || resource_type(res) != IORESOURCE_MEM) { dev_err(dev, "invalid resource\n"); return IOMEM_ERR_PTR(-EINVAL); > > Which means you did not test this? Why not? > > But step back, _WHY_ is this needed at all? How deep are we going to > get with the "devm_platform_get_and_do_this_and_that_and_that" type > functions here? the function name seems too long, how can I rename it:) > > You show 2 users of this call, and they save what, 1-2 lines of code > here? > > What is the real need for this? > > thanks, > > greg k-h many thanks for your feedback. Cai
On Thu, Sep 02, 2021 at 04:05:39PM +0800, Cai Huoqing wrote: > On 02 Sep 21 08:52:45, Greg KH wrote: > > On Thu, Sep 02, 2021 at 02:37:00PM +0800, Cai Huoqing wrote: > > > Since provide the helper function devm_platform_ioremap_resource_byname() > > > which is wrap platform_get_resource_byname() and devm_ioremap_resource(). > > > But sometimes, many drivers still need to use the resource variables > > > obtained by platform_get_resource(). In these cases, provide this helper > > > function devm_platform_get_and_ioremap_resource_byname(). > > > > > > Signed-off-by: Cai Huoqing <caihuoqing@baidu.com> > > > --- > > > v1->v2: Resend this patch as part of a patch series that uses > > > the new function. > > > > > > drivers/base/platform.c | 30 ++++++++++++++++++++++++++---- > > > include/linux/platform_device.h | 3 +++ > > > 2 files changed, 29 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > > > index 652531f67135..34bb581338d9 100644 > > > --- a/drivers/base/platform.c > > > +++ b/drivers/base/platform.c > > > @@ -124,6 +124,31 @@ void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev, > > > } > > > EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource); > > > > > > +/** > > > + * devm_platform_get_and_ioremap_resource_byname - call devm_ioremap_resource() for a > > > + * platform device and get resource > > > + * > > > + * @pdev: platform device to use both for memory resource lookup as well as > > > + * resource management > > > + * @name: name of the resource > > > + * @res: optional output parameter to store a pointer to the obtained resource. > > > + * > > > + * Return: a pointer to the remapped memory or an ERR_PTR() encoded error code > > > + * on failure. > > > + */ > > > +void __iomem * > > > +devm_platform_get_and_ioremap_resource_byname(struct platform_device *pdev, > > > + const char *name, struct resource **res) > > > +{ > > > + struct resource *r; > > > + > > > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, name); > > > + if (res) > > > + *res = r; > > > > You forgot to check the return value of this call :( > devm_ioremap_resource wiil check it and print error message, here: > ./lib/devres.c:136:__devm_ioremap_resource( > > if (!res || resource_type(res) != IORESOURCE_MEM) { > dev_err(dev, "invalid resource\n"); > return IOMEM_ERR_PTR(-EINVAL); And then you move on and use the resource :( Please properly test your code. > > Which means you did not test this? Why not? > > > > But step back, _WHY_ is this needed at all? How deep are we going to > > get with the "devm_platform_get_and_do_this_and_that_and_that" type > > functions here? > the function name seems too long, how can I rename it:) You have not shown a requirement that this new function is needed at all. Why are you making this change? Why do you want to do this? What is it helping out with? thanks, greg k-h
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 652531f67135..34bb581338d9 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -124,6 +124,31 @@ void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev, } EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource); +/** + * devm_platform_get_and_ioremap_resource_byname - call devm_ioremap_resource() for a + * platform device and get resource + * + * @pdev: platform device to use both for memory resource lookup as well as + * resource management + * @name: name of the resource + * @res: optional output parameter to store a pointer to the obtained resource. + * + * Return: a pointer to the remapped memory or an ERR_PTR() encoded error code + * on failure. + */ +void __iomem * +devm_platform_get_and_ioremap_resource_byname(struct platform_device *pdev, + const char *name, struct resource **res) +{ + struct resource *r; + + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, name); + if (res) + *res = r; + return devm_ioremap_resource(&pdev->dev, r); +} +EXPORT_SYMBOL_GPL(devm_platform_get_and_ioremap_resource_byname); + /** * devm_platform_ioremap_resource_byname - call devm_ioremap_resource for * a platform device, retrieve the @@ -140,10 +165,7 @@ void __iomem * devm_platform_ioremap_resource_byname(struct platform_device *pdev, const char *name) { - struct resource *res; - - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name); - return devm_ioremap_resource(&pdev->dev, res); + return devm_platform_get_and_ioremap_resource_byname(pdev, name, NULL); } EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname); #endif /* CONFIG_HAS_IOMEM */ diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h index 7c96f169d274..50eb1a5b503a 100644 --- a/include/linux/platform_device.h +++ b/include/linux/platform_device.h @@ -66,6 +66,9 @@ extern void __iomem * devm_platform_ioremap_resource(struct platform_device *pdev, unsigned int index); extern void __iomem * +devm_platform_get_and_ioremap_resource_byname(struct platform_device *pdev, + const char *name, struct resource **res); +extern void __iomem * devm_platform_ioremap_resource_byname(struct platform_device *pdev, const char *name); extern int platform_get_irq(struct platform_device *, unsigned int);
Since provide the helper function devm_platform_ioremap_resource_byname() which is wrap platform_get_resource_byname() and devm_ioremap_resource(). But sometimes, many drivers still need to use the resource variables obtained by platform_get_resource(). In these cases, provide this helper function devm_platform_get_and_ioremap_resource_byname(). Signed-off-by: Cai Huoqing <caihuoqing@baidu.com> --- v1->v2: Resend this patch as part of a patch series that uses the new function. drivers/base/platform.c | 30 ++++++++++++++++++++++++++---- include/linux/platform_device.h | 3 +++ 2 files changed, 29 insertions(+), 4 deletions(-)