The dummy_clk_set_parent function is marked as 'static' but is
no longer referenced from the pxa25x clk driver after the last use
of the RATE_RO_OPS() macro is gone from this file, causing a
harmless build warning:
In file included from drivers/clk/pxa/clk-pxa25x.c:24:0:
drivers/clk/pxa/clk-pxa.h:146:12: error: 'dummy_clk_set_parent' defined but not used [-Werror=unused-function]
This marks the functon as 'inline', which lets the compiler simply
drop it when it gets referenced.
Fixes: 9fe694295098 ("clk: pxa: transfer CPU clock setting from pxa2xx-cpufreq")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/clk/pxa/clk-pxa.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/pxa/clk-pxa.h b/drivers/clk/pxa/clk-pxa.h
index f60a7bccae4e..58abfa816d53 100644
--- a/drivers/clk/pxa/clk-pxa.h
+++ b/drivers/clk/pxa/clk-pxa.h
@@ -143,7 +143,7 @@ struct pxa2xx_freq {
unsigned int clkcfg;
};
-static int dummy_clk_set_parent(struct clk_hw *hw, u8 index)
+static inline int dummy_clk_set_parent(struct clk_hw *hw, u8 index)
{
return 0;
}
--
2.9.0
The new pxa2xx_determine_rate() function seems lacking in a few
regards:
- For an exact match or no match at all, the rate is uninitialized
as reported by gcc -Wmaybe-unintialized:
drivers/clk/pxa/clk-pxa.c: In function 'pxa2xx_determine_rate':
drivers/clk/pxa/clk-pxa.c:243:5: error: 'rate' may be used uninitialized in this function
- If we get a non-exact match, the req->rate output is never set
to the actual rate but remains at the requested rate.
- We should not attempt to print a rate if none could be found
This rewrites the logic accordingly.
Fixes: 9fe694295098 ("clk: pxa: transfer CPU clock setting from pxa2xx-cpufreq")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/clk/pxa/clk-pxa.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/clk/pxa/clk-pxa.c b/drivers/clk/pxa/clk-pxa.c
index 50fb9d0ea58d..c423b064c753 100644
--- a/drivers/clk/pxa/clk-pxa.c
+++ b/drivers/clk/pxa/clk-pxa.c
@@ -211,7 +211,7 @@ void pxa2xx_cpll_change(struct pxa2xx_freq *freq,
int pxa2xx_determine_rate(struct clk_rate_request *req,
struct pxa2xx_freq *freqs, int nb_freqs)
{
- int i, closest_below = -1, closest_above = -1, ret = 0;
+ int i, closest_below = -1, closest_above = -1;
unsigned long rate;
for (i = 0; i < nb_freqs; i++) {
@@ -230,18 +230,19 @@ int pxa2xx_determine_rate(struct clk_rate_request *req,
req->best_parent_hw = NULL;
- if (i < nb_freqs)
- ret = 0;
- else if (closest_below >= 0)
+ if (i < nb_freqs) {
+ rate = req->rate;
+ } else if (closest_below >= 0) {
rate = freqs[closest_below].cpll;
- else if (closest_above >= 0)
+ } else if (closest_above >= 0) {
rate = freqs[closest_above].cpll;
- else
- ret = -EINVAL;
+ } else {
+ pr_debug("%s(rate=%lu) no match\n", __func__, req->rate);
+ return -EINVAL;
+ }
- pr_debug("%s(rate=%lu) rate=%lu: %d\n", __func__, req->rate, rate, ret);
- if (!rate)
- req->rate = rate;
+ pr_debug("%s(rate=%lu) rate=%lu\n", __func__, req->rate, rate);
+ req->rate = rate;
- return ret;
+ return 0;
}
--
2.9.0
Arnd Bergmann <[email protected]> writes:
> The dummy_clk_set_parent function is marked as 'static' but is
> no longer referenced from the pxa25x clk driver after the last use
> of the RATE_RO_OPS() macro is gone from this file, causing a
> harmless build warning:
>
> In file included from drivers/clk/pxa/clk-pxa25x.c:24:0:
> drivers/clk/pxa/clk-pxa.h:146:12: error: 'dummy_clk_set_parent' defined but not used [-Werror=unused-function]
>
> This marks the functon as 'inline', which lets the compiler simply
> drop it when it gets referenced.
>
> Fixes: 9fe694295098 ("clk: pxa: transfer CPU clock setting from pxa2xx-cpufreq")
> Signed-off-by: Arnd Bergmann <[email protected]>
Acked-by: Robert Jarzmik <[email protected]>
Cheers.
--
Robert
Arnd Bergmann <[email protected]> writes:
> The new pxa2xx_determine_rate() function seems lacking in a few
> regards:
>
> - For an exact match or no match at all, the rate is uninitialized
> as reported by gcc -Wmaybe-unintialized:
> drivers/clk/pxa/clk-pxa.c: In function 'pxa2xx_determine_rate':
> drivers/clk/pxa/clk-pxa.c:243:5: error: 'rate' may be used uninitialized in
> this function
Euh I don't think that is true.
For an exact match, rate is assigned the exact value in the first line after the
for(xxx).
For no match at all, there are 2 cases :
- either a closest match is found, and rate is actually assigned (see below)
- or no match is found, and it's true rate remains uninitialized, but we have
ret = -EINVAL
> - If we get a non-exact match, the req->rate output is never set
> to the actual rate but remains at the requested rate.
Euh no, that doesn't seem correct to me.
If a non-exact match is found, either by closest_below or closest_above, rate is
set (rate = freqs[closest_xxx].cpll). And a couple of lines later after the
if/else, req->rate = rate is set as well, so I don't think this part of the
commit message is accurate.
> - We should not attempt to print a rate if none could be found
True.
> This rewrites the logic accordingly.
Unless I'm wrong in the analysis above, I'd rather have just "unsigned long rate
= 0" in the variable declaration, and keep the pr_debug() even if -EINVAL is
returned (it's better for bug tracking, with a rate == 0 in this case for example).
Cheers.
--
Robert
On Tuesday, November 8, 2016 7:01:57 PM CET Robert Jarzmik wrote:
> Arnd Bergmann <[email protected]> writes:
>
> > The new pxa2xx_determine_rate() function seems lacking in a few
> > regards:
> >
> > - For an exact match or no match at all, the rate is uninitialized
> > as reported by gcc -Wmaybe-unintialized:
> > drivers/clk/pxa/clk-pxa.c: In function 'pxa2xx_determine_rate':
> > drivers/clk/pxa/clk-pxa.c:243:5: error: 'rate' may be used uninitialized in
> > this function
> Euh I don't think that is true.
>
> For an exact match, rate is assigned the exact value in the first line after the
> for(xxx).
Right, my mistake.
> For no match at all, there are 2 cases :
> - either a closest match is found, and rate is actually assigned (see below)
> - or no match is found, and it's true rate remains uninitialized, but we have
> ret = -EINVAL
Or a third case that gcc finds but that probably won't happen in practice:
- nb_freqs==0, rate is never initialized
This is what I'm addressing by returning early in the 'else' case.
> > - If we get a non-exact match, the req->rate output is never set
> > to the actual rate but remains at the requested rate.
> Euh no, that doesn't seem correct to me.
>
> If a non-exact match is found, either by closest_below or closest_above, rate is
> set (rate = freqs[closest_xxx].cpll). And a couple of lines later after the
> if/else, req->rate = rate is set as well, so I don't think this part of the
> commit message is accurate.
It is only set if rate is zero, and that normally is not the case here:
if (!rate)
req->rate = rate;
> > - We should not attempt to print a rate if none could be found
> True.
>
> > This rewrites the logic accordingly.
> Unless I'm wrong in the analysis above, I'd rather have just "unsigned long rate
> = 0" in the variable declaration, and keep the pr_debug() even if -EINVAL is
> returned (it's better for bug tracking, with a rate == 0 in this case for example).
I think it's safer not to initialize the variable, to ensure we get a
warning if the function is changed incorrectly again later.
Arnd
On 11/08, Arnd Bergmann wrote:
> The dummy_clk_set_parent function is marked as 'static' but is
> no longer referenced from the pxa25x clk driver after the last use
> of the RATE_RO_OPS() macro is gone from this file, causing a
> harmless build warning:
>
> In file included from drivers/clk/pxa/clk-pxa25x.c:24:0:
> drivers/clk/pxa/clk-pxa.h:146:12: error: 'dummy_clk_set_parent' defined but not used [-Werror=unused-function]
>
> This marks the functon as 'inline', which lets the compiler simply
> drop it when it gets referenced.
>
> Fixes: 9fe694295098 ("clk: pxa: transfer CPU clock setting from pxa2xx-cpufreq")
I hope I don't rewrite clk-next history... I need some sort of
magic git pre-commit hook that rewrites fixes tags if the hash
changes.
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
Applied to clk-next
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Tuesday, November 8, 2016 2:42:59 PM CET Stephen Boyd wrote:
> On 11/08, Arnd Bergmann wrote:
> > The dummy_clk_set_parent function is marked as 'static' but is
> > no longer referenced from the pxa25x clk driver after the last use
> > of the RATE_RO_OPS() macro is gone from this file, causing a
> > harmless build warning:
> >
> > In file included from drivers/clk/pxa/clk-pxa25x.c:24:0:
> > drivers/clk/pxa/clk-pxa.h:146:12: error: 'dummy_clk_set_parent' defined but not used [-Werror=unused-function]
> >
> > This marks the functon as 'inline', which lets the compiler simply
> > drop it when it gets referenced.
> >
> > Fixes: 9fe694295098 ("clk: pxa: transfer CPU clock setting from pxa2xx-cpufreq")
>
> I hope I don't rewrite clk-next history... I need some sort of
> magic git pre-commit hook that rewrites fixes tags if the hash
> changes.
>
I think if you end up rebasing clk-next, the correct approach
would be to fold simple bugfixes into the patches that introduce
the problems. Obviously you still need a way to find them though.
Arnd
Arnd Bergmann <[email protected]> writes:
> On Tuesday, November 8, 2016 7:01:57 PM CET Robert Jarzmik wrote:
>> Arnd Bergmann <[email protected]> writes:
>> If a non-exact match is found, either by closest_below or closest_above, rate is
>> set (rate = freqs[closest_xxx].cpll). And a couple of lines later after the
>> if/else, req->rate = rate is set as well, so I don't think this part of the
>> commit message is accurate.
>
> It is only set if rate is zero, and that normally is not the case here:
>
> if (!rate)
> req->rate = rate;
Ah ok, that's where the bug was lurking, if should have been "if (rate)".
But anyway, after comparing the end result of your code and mine, I find yours
more maintainable, especially the replacement of 'ret = 0'.
So let's proceed, thanks for finding this one out.
Acked-by: Robert Jarzmik <[email protected]>
--
Robert
On 11/08, Arnd Bergmann wrote:
> The new pxa2xx_determine_rate() function seems lacking in a few
> regards:
>
> - For an exact match or no match at all, the rate is uninitialized
> as reported by gcc -Wmaybe-unintialized:
> drivers/clk/pxa/clk-pxa.c: In function 'pxa2xx_determine_rate':
> drivers/clk/pxa/clk-pxa.c:243:5: error: 'rate' may be used uninitialized in this function
>
> - If we get a non-exact match, the req->rate output is never set
> to the actual rate but remains at the requested rate.
>
> - We should not attempt to print a rate if none could be found
>
> This rewrites the logic accordingly.
>
> Fixes: 9fe694295098 ("clk: pxa: transfer CPU clock setting from pxa2xx-cpufreq")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
Applied to clk-next
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project