* [PATCH -next] clk: tegra: clk-dfll: indicate correct error reason in tegra_dfll_register
@ 2020-09-18 9:46 Qilong Zhang
2020-09-22 19:57 ` Stephen Boyd
0 siblings, 1 reply; 6+ messages in thread
From: Qilong Zhang @ 2020-09-18 9:46 UTC (permalink / raw)
To: mturquette, sboyd
Cc: pdeschrijver, pgaikwad, thierry.reding, jonathanh, linux-clk
From: Zhang Qilong <zhangqilong3@huawei.com>
Calling devm_ioremap means getting devices resource have been
successful. When remap operation failed, we should return '-ENOMEM'
instead of '-ENODEV' to differentiate between getting resource and
mapping memory for reminding callers. Moreover, it is not consistent
with devm_kzalloc operation.
Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
---
drivers/clk/tegra/clk-dfll.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
index cfbaa90c7adb..6637b73be9f1 100644
--- a/drivers/clk/tegra/clk-dfll.c
+++ b/drivers/clk/tegra/clk-dfll.c
@@ -1993,7 +1993,7 @@ int tegra_dfll_register(struct platform_device *pdev,
td->base = devm_ioremap(td->dev, mem->start, resource_size(mem));
if (!td->base) {
dev_err(td->dev, "couldn't ioremap DFLL control registers\n");
- return -ENODEV;
+ return -ENOMEM;
}
mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
@@ -2005,7 +2005,7 @@ int tegra_dfll_register(struct platform_device *pdev,
td->i2c_base = devm_ioremap(td->dev, mem->start, resource_size(mem));
if (!td->i2c_base) {
dev_err(td->dev, "couldn't ioremap i2c_base resource\n");
- return -ENODEV;
+ return -ENOMEM;
}
mem = platform_get_resource(pdev, IORESOURCE_MEM, 2);
@@ -2019,7 +2019,7 @@ int tegra_dfll_register(struct platform_device *pdev,
if (!td->i2c_controller_base) {
dev_err(td->dev,
"couldn't ioremap i2c_controller_base resource\n");
- return -ENODEV;
+ return -ENOMEM;
}
mem = platform_get_resource(pdev, IORESOURCE_MEM, 3);
@@ -2032,7 +2032,7 @@ int tegra_dfll_register(struct platform_device *pdev,
if (!td->lut_base) {
dev_err(td->dev,
"couldn't ioremap lut_base resource\n");
- return -ENODEV;
+ return -ENOMEM;
}
ret = dfll_init_clks(td);
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH -next] clk: tegra: clk-dfll: indicate correct error reason in tegra_dfll_register
2020-09-18 9:46 [PATCH -next] clk: tegra: clk-dfll: indicate correct error reason in tegra_dfll_register Qilong Zhang
@ 2020-09-22 19:57 ` Stephen Boyd
2020-09-23 8:16 ` Thierry Reding
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2020-09-22 19:57 UTC (permalink / raw)
To: Qilong Zhang, mturquette
Cc: pdeschrijver, pgaikwad, thierry.reding, jonathanh, linux-clk
Quoting Qilong Zhang (2020-09-18 02:46:42)
> From: Zhang Qilong <zhangqilong3@huawei.com>
>
> Calling devm_ioremap means getting devices resource have been
> successful. When remap operation failed, we should return '-ENOMEM'
> instead of '-ENODEV' to differentiate between getting resource and
> mapping memory for reminding callers. Moreover, it is not consistent
> with devm_kzalloc operation.
>
> Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
> ---
> drivers/clk/tegra/clk-dfll.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
> index cfbaa90c7adb..6637b73be9f1 100644
> --- a/drivers/clk/tegra/clk-dfll.c
> +++ b/drivers/clk/tegra/clk-dfll.c
> @@ -1993,7 +1993,7 @@ int tegra_dfll_register(struct platform_device *pdev,
> td->base = devm_ioremap(td->dev, mem->start, resource_size(mem));
> if (!td->base) {
> dev_err(td->dev, "couldn't ioremap DFLL control registers\n");
> - return -ENODEV;
> + return -ENOMEM;
Can you remove the dev_err() lines too? They're pretty much useless.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] clk: tegra: clk-dfll: indicate correct error reason in tegra_dfll_register
2020-09-22 19:57 ` Stephen Boyd
@ 2020-09-23 8:16 ` Thierry Reding
2020-09-23 23:32 ` Stephen Boyd
0 siblings, 1 reply; 6+ messages in thread
From: Thierry Reding @ 2020-09-23 8:16 UTC (permalink / raw)
To: Stephen Boyd
Cc: Qilong Zhang, mturquette, pdeschrijver, pgaikwad, jonathanh,
linux-clk
[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]
On Tue, Sep 22, 2020 at 12:57:46PM -0700, Stephen Boyd wrote:
> Quoting Qilong Zhang (2020-09-18 02:46:42)
> > From: Zhang Qilong <zhangqilong3@huawei.com>
> >
> > Calling devm_ioremap means getting devices resource have been
> > successful. When remap operation failed, we should return '-ENOMEM'
> > instead of '-ENODEV' to differentiate between getting resource and
> > mapping memory for reminding callers. Moreover, it is not consistent
> > with devm_kzalloc operation.
> >
> > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
> > ---
> > drivers/clk/tegra/clk-dfll.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
> > index cfbaa90c7adb..6637b73be9f1 100644
> > --- a/drivers/clk/tegra/clk-dfll.c
> > +++ b/drivers/clk/tegra/clk-dfll.c
> > @@ -1993,7 +1993,7 @@ int tegra_dfll_register(struct platform_device *pdev,
> > td->base = devm_ioremap(td->dev, mem->start, resource_size(mem));
> > if (!td->base) {
> > dev_err(td->dev, "couldn't ioremap DFLL control registers\n");
> > - return -ENODEV;
> > + return -ENOMEM;
>
> Can you remove the dev_err() lines too? They're pretty much useless.
I find them somewhat useful because they indicate which particular
resource wasn't properly mapped. If we get an -ENOMEM without the error
message, we'll have to go and guess which one it is.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] clk: tegra: clk-dfll: indicate correct error reason in tegra_dfll_register
2020-09-23 8:16 ` Thierry Reding
@ 2020-09-23 23:32 ` Stephen Boyd
2020-09-24 7:17 ` Thierry Reding
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2020-09-23 23:32 UTC (permalink / raw)
To: Thierry Reding
Cc: Qilong Zhang, mturquette, pdeschrijver, pgaikwad, jonathanh,
linux-clk
Quoting Thierry Reding (2020-09-23 01:16:54)
> On Tue, Sep 22, 2020 at 12:57:46PM -0700, Stephen Boyd wrote:
> > Quoting Qilong Zhang (2020-09-18 02:46:42)
> > > From: Zhang Qilong <zhangqilong3@huawei.com>
> > >
> > > Calling devm_ioremap means getting devices resource have been
> > > successful. When remap operation failed, we should return '-ENOMEM'
> > > instead of '-ENODEV' to differentiate between getting resource and
> > > mapping memory for reminding callers. Moreover, it is not consistent
> > > with devm_kzalloc operation.
> > >
> > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
> > > ---
> > > drivers/clk/tegra/clk-dfll.c | 8 ++++----
> > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
> > > index cfbaa90c7adb..6637b73be9f1 100644
> > > --- a/drivers/clk/tegra/clk-dfll.c
> > > +++ b/drivers/clk/tegra/clk-dfll.c
> > > @@ -1993,7 +1993,7 @@ int tegra_dfll_register(struct platform_device *pdev,
> > > td->base = devm_ioremap(td->dev, mem->start, resource_size(mem));
> > > if (!td->base) {
> > > dev_err(td->dev, "couldn't ioremap DFLL control registers\n");
> > > - return -ENODEV;
> > > + return -ENOMEM;
> >
> > Can you remove the dev_err() lines too? They're pretty much useless.
>
> I find them somewhat useful because they indicate which particular
> resource wasn't properly mapped. If we get an -ENOMEM without the error
> message, we'll have to go and guess which one it is.
>
Doesn't that print the stacktrace when we run out of memory?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] clk: tegra: clk-dfll: indicate correct error reason in tegra_dfll_register
2020-09-23 23:32 ` Stephen Boyd
@ 2020-09-24 7:17 ` Thierry Reding
2020-10-08 2:08 ` Stephen Boyd
0 siblings, 1 reply; 6+ messages in thread
From: Thierry Reding @ 2020-09-24 7:17 UTC (permalink / raw)
To: Stephen Boyd
Cc: Qilong Zhang, mturquette, pdeschrijver, pgaikwad, jonathanh,
linux-clk
[-- Attachment #1: Type: text/plain, Size: 2027 bytes --]
On Wed, Sep 23, 2020 at 04:32:40PM -0700, Stephen Boyd wrote:
> Quoting Thierry Reding (2020-09-23 01:16:54)
> > On Tue, Sep 22, 2020 at 12:57:46PM -0700, Stephen Boyd wrote:
> > > Quoting Qilong Zhang (2020-09-18 02:46:42)
> > > > From: Zhang Qilong <zhangqilong3@huawei.com>
> > > >
> > > > Calling devm_ioremap means getting devices resource have been
> > > > successful. When remap operation failed, we should return '-ENOMEM'
> > > > instead of '-ENODEV' to differentiate between getting resource and
> > > > mapping memory for reminding callers. Moreover, it is not consistent
> > > > with devm_kzalloc operation.
> > > >
> > > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
> > > > ---
> > > > drivers/clk/tegra/clk-dfll.c | 8 ++++----
> > > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
> > > > index cfbaa90c7adb..6637b73be9f1 100644
> > > > --- a/drivers/clk/tegra/clk-dfll.c
> > > > +++ b/drivers/clk/tegra/clk-dfll.c
> > > > @@ -1993,7 +1993,7 @@ int tegra_dfll_register(struct platform_device *pdev,
> > > > td->base = devm_ioremap(td->dev, mem->start, resource_size(mem));
> > > > if (!td->base) {
> > > > dev_err(td->dev, "couldn't ioremap DFLL control registers\n");
> > > > - return -ENODEV;
> > > > + return -ENOMEM;
> > >
> > > Can you remove the dev_err() lines too? They're pretty much useless.
> >
> > I find them somewhat useful because they indicate which particular
> > resource wasn't properly mapped. If we get an -ENOMEM without the error
> > message, we'll have to go and guess which one it is.
> >
>
> Doesn't that print the stacktrace when we run out of memory?
slab allocator functions like kmalloc() and friends do, but I'm not
aware of ioremap() doing so. Perhaps if it runs out of virtual memory
for the mapping it would, but there are other reasons why this can fail.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] clk: tegra: clk-dfll: indicate correct error reason in tegra_dfll_register
2020-09-24 7:17 ` Thierry Reding
@ 2020-10-08 2:08 ` Stephen Boyd
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Boyd @ 2020-10-08 2:08 UTC (permalink / raw)
To: Thierry Reding
Cc: Qilong Zhang, mturquette, pdeschrijver, pgaikwad, jonathanh,
linux-clk
Quoting Thierry Reding (2020-09-24 00:17:06)
> On Wed, Sep 23, 2020 at 04:32:40PM -0700, Stephen Boyd wrote:
> > Quoting Thierry Reding (2020-09-23 01:16:54)
> > > On Tue, Sep 22, 2020 at 12:57:46PM -0700, Stephen Boyd wrote:
> > > > Quoting Qilong Zhang (2020-09-18 02:46:42)
> > > > > td->base = devm_ioremap(td->dev, mem->start, resource_size(mem));
> > > > > if (!td->base) {
> > > > > dev_err(td->dev, "couldn't ioremap DFLL control registers\n");
> > > > > - return -ENODEV;
> > > > > + return -ENOMEM;
> > > >
> > > > Can you remove the dev_err() lines too? They're pretty much useless.
> > >
> > > I find them somewhat useful because they indicate which particular
> > > resource wasn't properly mapped. If we get an -ENOMEM without the error
> > > message, we'll have to go and guess which one it is.
> > >
> >
> > Doesn't that print the stacktrace when we run out of memory?
>
> slab allocator functions like kmalloc() and friends do, but I'm not
> aware of ioremap() doing so. Perhaps if it runs out of virtual memory
> for the mapping it would, but there are other reasons why this can fail.
>
Ok, but the other failure modes are going to happen outside of
developing the driver? What failure of ioremap() are you trying to
catch?
And even if knowing which resource didn't map properly, wouldn't we be
better off moving the error message closer to the actual problem in
ioremap() so it can tell us the problem that was seen? Otherwise I fear
the driver is going to tell the user that some error happened but we
won't be able to really figure out what the error is.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-08 2:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-18 9:46 [PATCH -next] clk: tegra: clk-dfll: indicate correct error reason in tegra_dfll_register Qilong Zhang
2020-09-22 19:57 ` Stephen Boyd
2020-09-23 8:16 ` Thierry Reding
2020-09-23 23:32 ` Stephen Boyd
2020-09-24 7:17 ` Thierry Reding
2020-10-08 2:08 ` Stephen Boyd
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox