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