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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]