diff mbox series

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

Message ID 20191229080610.7597-1-tiny.windzz@gmail.com (mailing list archive)
State New, archived
Headers show
Series [01/32] pwm: sun4i: convert to devm_platform_ioremap_resource | expand

Commit Message

Yangtao Li Dec. 29, 2019, 8:05 a.m. UTC
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(-)

Comments

Uwe Kleine-König May 23, 2020, 5:11 p.m. UTC | #1
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>
Uwe Kleine-König Nov. 12, 2020, 4:13 p.m. UTC | #2
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
Thierry Reding Nov. 12, 2020, 7:06 p.m. UTC | #3
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
Uwe Kleine-König Nov. 12, 2020, 9:14 p.m. UTC | #4
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
Uwe Kleine-König Nov. 13, 2020, 7:03 a.m. UTC | #5
Hello,

[Added lkml and the people involved in commit 7945f929f1a7
("drivers: provide devm_platform_ioremap_resource()") to Cc:. For the
new readers: This is about patches making use of
devm_platform_ioremap_resource() instead of open coding it. Full context
at https://lore.kernel.org/r/20201112190649.GA908613@ulmo]

On Thu, Nov 12, 2020 at 10:14:29PM +0100, Uwe Kleine-König wrote:
> 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?

I got a hint in private here: https://lore.kernel.org/r/1555670144-24220-1-git-send-email-aisheng.dong@nxp.com

devm_platform_ioremap_resource() is platform_get_resource() +
devm_ioremap_resource() and here it was used to replace
platform_get_resource() + devm_ioremap().

IMHO the unlucky thing in this situation is that devm_ioremap_resource()
and devm_ioremap() are different by more than just how they get the area
to remap. (i.e. devm_ioremap_resource() also does
devm_request_mem_region().)

So the problem is not the added wrapper, but unclear semantics in the
functions it uses. In my eyes devm_ioremap() and
devm_platform_ioremap_resource() should better be named
devm_request_ioremap() and devm_platform_request_ioremap_resource()
respectively. Is it worth to rename these for clearity?

Best regards
Uwe
Bartosz Golaszewski Nov. 13, 2020, 9:12 a.m. UTC | #6
On Fri, Nov 13, 2020 at 8:04 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> [Added lkml and the people involved in commit 7945f929f1a7
> ("drivers: provide devm_platform_ioremap_resource()") to Cc:. For the
> new readers: This is about patches making use of
> devm_platform_ioremap_resource() instead of open coding it. Full context
> at https://lore.kernel.org/r/20201112190649.GA908613@ulmo]
>
> On Thu, Nov 12, 2020 at 10:14:29PM +0100, Uwe Kleine-König wrote:
> > 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?
>
> I got a hint in private here: https://lore.kernel.org/r/1555670144-24220-1-git-send-email-aisheng.dong@nxp.com
>
> devm_platform_ioremap_resource() is platform_get_resource() +
> devm_ioremap_resource() and here it was used to replace
> platform_get_resource() + devm_ioremap().
>
> IMHO the unlucky thing in this situation is that devm_ioremap_resource()
> and devm_ioremap() are different by more than just how they get the area
> to remap. (i.e. devm_ioremap_resource() also does
> devm_request_mem_region().)
>
> So the problem is not the added wrapper, but unclear semantics in the
> functions it uses. In my eyes devm_ioremap() and
> devm_platform_ioremap_resource() should better be named
> devm_request_ioremap() and devm_platform_request_ioremap_resource()
> respectively. Is it worth to rename these for clearity?

But devm_ioremap() doesn't request the region. Did you mean
devm_ioremap_resource() should become devm_request_ioremap_resource()?

Bartosz
Uwe Kleine-König Nov. 13, 2020, 9:35 a.m. UTC | #7
On Fri, Nov 13, 2020 at 10:12:46AM +0100, Bartosz Golaszewski wrote:
> On Fri, Nov 13, 2020 at 8:04 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello,
> >
> > [Added lkml and the people involved in commit 7945f929f1a7
> > ("drivers: provide devm_platform_ioremap_resource()") to Cc:. For the
> > new readers: This is about patches making use of
> > devm_platform_ioremap_resource() instead of open coding it. Full context
> > at https://lore.kernel.org/r/20201112190649.GA908613@ulmo]
> >
> > On Thu, Nov 12, 2020 at 10:14:29PM +0100, Uwe Kleine-König wrote:
> > > 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?
> >
> > I got a hint in private here: https://lore.kernel.org/r/1555670144-24220-1-git-send-email-aisheng.dong@nxp.com
> >
> > devm_platform_ioremap_resource() is platform_get_resource() +
> > devm_ioremap_resource() and here it was used to replace
> > platform_get_resource() + devm_ioremap().
> >
> > IMHO the unlucky thing in this situation is that devm_ioremap_resource()
> > and devm_ioremap() are different by more than just how they get the area
> > to remap. (i.e. devm_ioremap_resource() also does
> > devm_request_mem_region().)
> >
> > So the problem is not the added wrapper, but unclear semantics in the
> > functions it uses. In my eyes devm_ioremap() and
> > devm_platform_ioremap_resource() should better be named
> > devm_request_ioremap() and devm_platform_request_ioremap_resource()
> > respectively. Is it worth to rename these for clearity?
> 
> But devm_ioremap() doesn't request the region. Did you mean
> devm_ioremap_resource() should become devm_request_ioremap_resource()?

Yes indeed. The last paragraph should be:

So the problem is not the added wrapper, but unclear semantics in the
functions it uses. In my eyes devm_ioremap_resource() and
devm_platform_ioremap_resource() should better be named
devm_request_ioremap_resource() and
devm_platform_request_ioremap_resource().

(Note that I created a patch series that implements this suggestion, but
you were not on Cc: as I extensively trimmed the recipents assuming most
people are not interested. See https://lore.kernel.org/r/20201113085327.125041-1-u.kleine-koenig@pengutronix.de)

Best regards
Uwe
Thierry Reding Nov. 13, 2020, 4:11 p.m. UTC | #8
On Fri, Nov 13, 2020 at 08:03:43AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> [Added lkml and the people involved in commit 7945f929f1a7
> ("drivers: provide devm_platform_ioremap_resource()") to Cc:. For the
> new readers: This is about patches making use of
> devm_platform_ioremap_resource() instead of open coding it. Full context
> at https://lore.kernel.org/r/20201112190649.GA908613@ulmo]
> 
> On Thu, Nov 12, 2020 at 10:14:29PM +0100, Uwe Kleine-König wrote:
> > 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?
> 
> I got a hint in private here: https://lore.kernel.org/r/1555670144-24220-1-git-send-email-aisheng.dong@nxp.com
> 
> devm_platform_ioremap_resource() is platform_get_resource() +
> devm_ioremap_resource() and here it was used to replace
> platform_get_resource() + devm_ioremap().
> 
> IMHO the unlucky thing in this situation is that devm_ioremap_resource()
> and devm_ioremap() are different by more than just how they get the area
> to remap. (i.e. devm_ioremap_resource() also does
> devm_request_mem_region().)
> 
> So the problem is not the added wrapper, but unclear semantics in the
> functions it uses.

The semantics aren't unclear. It's just that the symbol name doesn't
spell out every detail that the function implements, which, frankly, no
function name ever does, at least not for anything beyond simple
instructional examples. That's what we have documentation for and why
people should read the documentation before they use a function and make
(potentially wrong) assumption about what it does.

>                    In my eyes devm_ioremap() and
> devm_platform_ioremap_resource() should better be named
> devm_request_ioremap() and devm_platform_request_ioremap_resource()
> respectively. Is it worth to rename these for clearity?

I think function names are always a compromise between giving you the
gist of what the implementation does and being short enough so it
doesn't become difficult to read or use.

One of the reasons why I dislike the addition of helpers for every
common special case (like devm_platform_ioremap_resource()) is because
it doesn't (always) actually make things easier for developers and/or
maintainers. Replacing three lines of code with one is a minor
improvement, even though there may be many callsites and therefore in
the sum this being a fairly sizeable reduction. The flip side is that
now we've got an extra symbol with an unwieldy name that people need
to become familiar with, and then, like the link above shows, it doesn't
work in all cases, so you either need to fall back to the open-coded
version or you keep adding helpers until you've covered all cases. And
then we end up with a bunch of helpers that you actually have to go and
read the documentation for in order to find out which one exactly fits
your use-case.

Without the helpers it's pretty simple to write, even if a little
repetitive:

  1) get the resource you want to map
  2) request the resource
  3) map the resource

2) & 3) are very commonly done together, so it makes sense to have a
generic helper for them. If you look at the implementation, the
devm_ioremap_request() implementation does quite a bit of things in
addition to just requesting and remapping, and that's the reason why
that helper makes sense.

For me personally, devm_platform_ioremap_resource() is just not adding
enough value to justify its existence. And then we get all these other
variants that operate on the resource name (_byname) and those which
remap write-combined (_wc). But don't we also need a _byname_wc()
variant for the combination? Where does it stop?

Thierry
Robin Murphy Nov. 13, 2020, 5:40 p.m. UTC | #9
On 2020-11-13 16:11, Thierry Reding wrote:
> On Fri, Nov 13, 2020 at 08:03:43AM +0100, Uwe Kleine-König wrote:
>> Hello,
>>
>> [Added lkml and the people involved in commit 7945f929f1a7
>> ("drivers: provide devm_platform_ioremap_resource()") to Cc:. For the
>> new readers: This is about patches making use of
>> devm_platform_ioremap_resource() instead of open coding it. Full context
>> at https://lore.kernel.org/r/20201112190649.GA908613@ulmo]
>>
>> On Thu, Nov 12, 2020 at 10:14:29PM +0100, Uwe Kleine-König wrote:
>>> 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?
>>
>> I got a hint in private here: https://lore.kernel.org/r/1555670144-24220-1-git-send-email-aisheng.dong@nxp.com
>>
>> devm_platform_ioremap_resource() is platform_get_resource() +
>> devm_ioremap_resource() and here it was used to replace
>> platform_get_resource() + devm_ioremap().
>>
>> IMHO the unlucky thing in this situation is that devm_ioremap_resource()
>> and devm_ioremap() are different by more than just how they get the area
>> to remap. (i.e. devm_ioremap_resource() also does
>> devm_request_mem_region().)
>>
>> So the problem is not the added wrapper, but unclear semantics in the
>> functions it uses.
> 
> The semantics aren't unclear. It's just that the symbol name doesn't
> spell out every detail that the function implements, which, frankly, no
> function name ever does, at least not for anything beyond simple
> instructional examples. That's what we have documentation for and why
> people should read the documentation before they use a function and make
> (potentially wrong) assumption about what it does.
> 
>>                     In my eyes devm_ioremap() and
>> devm_platform_ioremap_resource() should better be named
>> devm_request_ioremap() and devm_platform_request_ioremap_resource()
>> respectively. Is it worth to rename these for clearity?
> 
> I think function names are always a compromise between giving you the
> gist of what the implementation does and being short enough so it
> doesn't become difficult to read or use.
> 
> One of the reasons why I dislike the addition of helpers for every
> common special case (like devm_platform_ioremap_resource()) is because
> it doesn't (always) actually make things easier for developers and/or
> maintainers. Replacing three lines of code with one is a minor
> improvement, even though there may be many callsites and therefore in
> the sum this being a fairly sizeable reduction. The flip side is that
> now we've got an extra symbol with an unwieldy name that people need
> to become familiar with, and then, like the link above shows, it doesn't
> work in all cases, so you either need to fall back to the open-coded
> version or you keep adding helpers until you've covered all cases. And
> then we end up with a bunch of helpers that you actually have to go and
> read the documentation for in order to find out which one exactly fits
> your use-case.
> 
> Without the helpers it's pretty simple to write, even if a little
> repetitive:
> 
>    1) get the resource you want to map
>    2) request the resource
>    3) map the resource
> 
> 2) & 3) are very commonly done together, so it makes sense to have a
> generic helper for them. If you look at the implementation, the
> devm_ioremap_request() implementation does quite a bit of things in
> addition to just requesting and remapping, and that's the reason why
> that helper makes sense.
> 
> For me personally, devm_platform_ioremap_resource() is just not adding
> enough value to justify its existence. And then we get all these other
> variants that operate on the resource name (_byname) and those which
> remap write-combined (_wc). But don't we also need a _byname_wc()
> variant for the combination? Where does it stop?

Arguably the worst thing about devm_platform_ioremap_resource() is that 
it was apparently the gateway drug to a belief that 
devm_platform_get_and_ioremap_resource() is anything other than a 
hideous way to obfuscate an assignment...

Robin.
Uwe Kleine-König Nov. 19, 2020, 5:08 p.m. UTC | #10
Hello,

On Fri, Nov 13, 2020 at 05:11:53PM +0100, Thierry Reding wrote:
> I think function names are always a compromise between giving you the
> gist of what the implementation does and being short enough so it
> doesn't become difficult to read or use.

Right. In my eyes if you have 

 - devm_platform_ioremap_resource
 - devm_platform_get_and_ioremap_resource
 - devm_ioremap_resource
 - devm_ioremap

(to list just a few) with the current semantics, the compromise is badly
shifted into the "short name" direction however; and that was the
motivation for this patch set. In my eyes it must be more obvious which
of these functions include devm_request_mem_region() and which don't.

And note, my patch series doesn't introduce new helpers, just renames
them to have a better name (and adds compat glue for the old names).

> One of the reasons why I dislike the addition of helpers for every
> common special case (like devm_platform_ioremap_resource()) is because
> it doesn't (always) actually make things easier for developers and/or
> maintainers. Replacing three lines of code with one is a minor
> improvement, even though there may be many callsites and therefore in
> the sum this being a fairly sizeable reduction. The flip side is that
> now we've got an extra symbol with an unwieldy name that people need
> to become familiar with, and then, like the link above shows, it doesn't
> work in all cases, so you either need to fall back to the open-coded
> version or you keep adding helpers until you've covered all cases. And
> then we end up with a bunch of helpers that you actually have to go and
> read the documentation for in order to find out which one exactly fits
> your use-case.

This is indeed a relevant point. An alternative is to make the helper
more flexible. This complicates the API, too, however, so this isn't
always gold, either.
 
> Without the helpers it's pretty simple to write, even if a little
> repetitive:
> 
>   1) get the resource you want to map
>   2) request the resource
>   3) map the resource
> 
> 2) & 3) are very commonly done together, so it makes sense to have a
> generic helper for them. If you look at the implementation, the
> devm_ioremap_request() implementation does quite a bit of things in
> addition to just requesting and remapping, and that's the reason why
> that helper makes sense.
> 
> For me personally, devm_platform_ioremap_resource() is just not adding
> enough value to justify its existence. And then we get all these other
> variants that operate on the resource name (_byname) and those which
> remap write-combined (_wc). But don't we also need a _byname_wc()
> variant for the combination? Where does it stop?

I'm on your side for the _wc stuff, looking at next-20201119:

 - devm_ioremap_resource_wc has a single user:
   devm_platform_ioremap_resource_wc
 - devm_platform_ioremap_resource_wc has a single user:
   drivers/misc/sram.c

Best regards
Uwe
diff mbox series

Patch

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);