[PATCH 01/32] pwm: sun4i: convert to devm_platform_ioremap_resource

[PATCH 01/32] pwm: sun4i: convert to devm_platform_ioremap_resource

[Yangtao Li]

Use devm_platform_ioremap_resource() to simplify code.

Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
---
 drivers/pwm/pwm-sun4i.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 581d23287333..f2afd312f77c 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -344,7 +344,6 @@ MODULE_DEVICE_TABLE(of, sun4i_pwm_dt_ids);
 static int sun4i_pwm_probe(struct platform_device *pdev)
 {
 	struct sun4i_pwm_chip *pwm;
-	struct resource *res;
 	int ret;
 
 	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
@@ -355,8 +354,7 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
 	if (!pwm->data)
 		return -ENODEV;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	pwm->base = devm_ioremap_resource(&pdev->dev, res);
+	pwm->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pwm->base))
 		return PTR_ERR(pwm->base);
 
-- 
2.17.1

Re: [PATCH 01/32] pwm: sun4i: convert to devm_platform_ioremap_resource

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 889 bytes --]

Hello Thierry,

On Thu, Nov 12, 2020 at 08:06:49PM +0100, Thierry Reding wrote:
> I also think that it's overly narrow is scope, so you can't actually
> "blindly" use this helper and I've seen quite a few cases where this was
> unknowingly used for cases where it shouldn't have been used and then
> broke things (because some drivers must not do the request_mem_region()
> for example).

You have a link to such an accident?

> And then there are cases where the driver needs the
> resource for some other purpose, so you can't use the helper either, or
> at least it looses all of its advantages in those cases.

There is devm_platform_get_and_ioremap_resource() for (some of) these
cases.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 01/32] pwm: sun4i: convert to devm_platform_ioremap_resource

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3659 bytes --]

On Thu, Nov 12, 2020 at 05:13:46PM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Sun, Dec 29, 2019 at 08:05:39AM +0000, Yangtao Li wrote:
> > Use devm_platform_ioremap_resource() to simplify code.
> > 
> > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
> > ---
> >  drivers/pwm/pwm-sun4i.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > index 581d23287333..f2afd312f77c 100644
> > --- a/drivers/pwm/pwm-sun4i.c
> > +++ b/drivers/pwm/pwm-sun4i.c
> > @@ -344,7 +344,6 @@ MODULE_DEVICE_TABLE(of, sun4i_pwm_dt_ids);
> >  static int sun4i_pwm_probe(struct platform_device *pdev)
> >  {
> >  	struct sun4i_pwm_chip *pwm;
> > -	struct resource *res;
> >  	int ret;
> >  
> >  	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> > @@ -355,8 +354,7 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> >  	if (!pwm->data)
> >  		return -ENODEV;
> >  
> > -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -	pwm->base = devm_ioremap_resource(&pdev->dev, res);
> > +	pwm->base = devm_platform_ioremap_resource(pdev, 0);
> >  	if (IS_ERR(pwm->base))
> >  		return PTR_ERR(pwm->base);
> 
> Can you please comment why you don't apply this series?

I did in fact apply this yesterday, but I now see that I didn't reply to
the thread to report that.

> My point of view is:
> 
> devm_platform_ioremap_resource is the designated wrapper to replace
> platform_get_resource() and devm_ioremap_resource(). So I don't see a
> good reason to continue open coding it.
> 
> The patch series doesn't apply to 5.10-rc1 as is. (pwm-puv3 was removed
> and a simple conflict in the pwm-rockchip driver.) The overall diffstat
> (of the fixed series applied on top of 5.10-rc1) is
> 
> 	31 files changed, 32 insertions(+), 96 deletions(-)
> 
> and it converts all of drivers/pwm but a single instance of
> platform_get_resource() + devm_ioremap_resource() (for pwm-lpss where
> platform_get_resource and devm_ioremap_resource are in different
> functions (different files even)) which isn't trivial to fix.
> 
> So in my eyes applying this series is the right thing to do.

For the record, I personally think this helper is a bit over the top. I
do agree that it's nice to create helpers for common code sequences, but
this is a *lot* of churn all across the kernel to save just two lines,
which I don't think is worth it in this case. Often these helpers allow
common mistakes to be avoided while at the same time reducing lines of
code, but devm_ioremap_resource() was already created to address the
pitfalls (like returning all sorts of weird and inconsistent error
codes). So this helper doesn't actually add any value other than saving
a few lines, which I don't think justifies the churn. I would've been
sold on this if the ratio had been slightly higher, like maybe saving a
dozen or so lines, but as it is, I just don't think it's worth the churn
that it's causing.

I also think that it's overly narrow is scope, so you can't actually
"blindly" use this helper and I've seen quite a few cases where this was
unknowingly used for cases where it shouldn't have been used and then
broke things (because some drivers must not do the request_mem_region()
for example). And then there are cases where the driver needs the
resource for some other purpose, so you can't use the helper either, or
at least it looses all of its advantages in those cases.

That said, the helper is there and has been widely accepted, so my
opinion has been overruled by the majority.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 01/32] pwm: sun4i: convert to devm_platform_ioremap_resource

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2136 bytes --]

Hello Thierry,

On Sun, Dec 29, 2019 at 08:05:39AM +0000, Yangtao Li wrote:
> Use devm_platform_ioremap_resource() to simplify code.
> 
> Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
> ---
>  drivers/pwm/pwm-sun4i.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 581d23287333..f2afd312f77c 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -344,7 +344,6 @@ MODULE_DEVICE_TABLE(of, sun4i_pwm_dt_ids);
>  static int sun4i_pwm_probe(struct platform_device *pdev)
>  {
>  	struct sun4i_pwm_chip *pwm;
> -	struct resource *res;
>  	int ret;
>  
>  	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> @@ -355,8 +354,7 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
>  	if (!pwm->data)
>  		return -ENODEV;
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	pwm->base = devm_ioremap_resource(&pdev->dev, res);
> +	pwm->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(pwm->base))
>  		return PTR_ERR(pwm->base);

Can you please comment why you don't apply this series?

My point of view is:

devm_platform_ioremap_resource is the designated wrapper to replace
platform_get_resource() and devm_ioremap_resource(). So I don't see a
good reason to continue open coding it.

The patch series doesn't apply to 5.10-rc1 as is. (pwm-puv3 was removed
and a simple conflict in the pwm-rockchip driver.) The overall diffstat
(of the fixed series applied on top of 5.10-rc1) is

	31 files changed, 32 insertions(+), 96 deletions(-)

and it converts all of drivers/pwm but a single instance of
platform_get_resource() + devm_ioremap_resource() (for pwm-lpss where
platform_get_resource and devm_ioremap_resource are in different
functions (different files even)) which isn't trivial to fix.

So in my eyes applying this series is the right thing to do.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 01/32] pwm: sun4i: convert to devm_platform_ioremap_resource

[Uwe Kleine-König]

On Sun, Dec 29, 2019 at 08:05:39AM +0000, Yangtao Li wrote:
> Use devm_platform_ioremap_resource() to simplify code.
> 
> Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[PATCH 01/32] pwm: sun4i: convert to devm_platform_ioremap_resource

[Yangtao Li]

Use devm_platform_ioremap_resource() to simplify code.

Signed-off-by: Yangtao Li <tiny.windzz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/pwm/pwm-sun4i.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 581d23287333..f2afd312f77c 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -344,7 +344,6 @@ MODULE_DEVICE_TABLE(of, sun4i_pwm_dt_ids);
 static int sun4i_pwm_probe(struct platform_device *pdev)
 {
 	struct sun4i_pwm_chip *pwm;
-	struct resource *res;
 	int ret;
 
 	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
@@ -355,8 +354,7 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
 	if (!pwm->data)
 		return -ENODEV;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	pwm->base = devm_ioremap_resource(&pdev->dev, res);
+	pwm->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pwm->base))
 		return PTR_ERR(pwm->base);
 
-- 
2.17.1