| Message ID | 20241112225832.3518658-1-ragavendra.bn@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk:sophgo:clk-cv18xx-pll | expand |
Pleas improve the patch title and just write "fix..." instead of listing the file name changed. On 2024/11/13 6:58, Ragavendra wrote: > Initializing the val variable of type u32 as it was not initialized. > Please add "Fixes" tag here, see https://www.kernel.org/doc/html/latest/process/submitting-patches.html. > Signed-off-by: Ragavendra Nagraj <ragavendra.bn@gmail.com> > --- > drivers/clk/sophgo/clk-cv18xx-pll.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/sophgo/clk-cv18xx-pll.c b/drivers/clk/sophgo/clk-cv18xx-pll.c > index 29e24098bf5f..04a0419cab4e 100644 > --- a/drivers/clk/sophgo/clk-cv18xx-pll.c > +++ b/drivers/clk/sophgo/clk-cv18xx-pll.c > @@ -87,7 +87,7 @@ static int ipll_find_rate(const struct cv1800_clk_pll_limit *limit, > > static int ipll_determine_rate(struct clk_hw *hw, struct clk_rate_request *req) > { > - u32 val; > + u32 val = 0; > struct cv1800_clk_pll *pll = hw_to_cv1800_clk_pll(hw); > > return ipll_find_rate(pll->pll_limit, req->best_parent_rate, I looked at ipll_find_rate() and found that the parameters "rate"/"value" are both input and output parameters, which is really a place that needs to be treated with caution. Seems this patch change is correct, otherwise the value of "detected" in the function will be random after calling ipll_find_rate here. Actually I suggest adding some comments for ipll_find_rate() to emphasize it. Otherwise, it is easy to think that "rate" and "value" are only output parameters at first glance. But I raised a question here: In ipll_find_rate(), do we really need to make "detected" depend on the value of external input? Inochi, can you please comment on this, due to you are the author of this code. Reviewed-by: Chen Wang <unicorn_wang@outlook.com> Thanks, Chen
On Wed, Nov 13, 2024 at 08:33:54AM +0800, Chen Wang wrote: > Pleas improve the patch title and just write "fix..." instead of listing the > file name changed. > > On 2024/11/13 6:58, Ragavendra wrote: > > Initializing the val variable of type u32 as it was not initialized. > > > Please add "Fixes" tag here, see > https://www.kernel.org/doc/html/latest/process/submitting-patches.html. > > Signed-off-by: Ragavendra Nagraj <ragavendra.bn@gmail.com> > > --- > > drivers/clk/sophgo/clk-cv18xx-pll.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/clk/sophgo/clk-cv18xx-pll.c b/drivers/clk/sophgo/clk-cv18xx-pll.c > > index 29e24098bf5f..04a0419cab4e 100644 > > --- a/drivers/clk/sophgo/clk-cv18xx-pll.c > > +++ b/drivers/clk/sophgo/clk-cv18xx-pll.c > > @@ -87,7 +87,7 @@ static int ipll_find_rate(const struct cv1800_clk_pll_limit *limit, > > static int ipll_determine_rate(struct clk_hw *hw, struct clk_rate_request *req) > > { > > - u32 val; > > + u32 val = 0; > > struct cv1800_clk_pll *pll = hw_to_cv1800_clk_pll(hw); > > return ipll_find_rate(pll->pll_limit, req->best_parent_rate, > > I looked at ipll_find_rate() and found that the parameters "rate"/"value" > are both input and output parameters, which is really a place that needs to > be treated with caution. > > Seems this patch change is correct, otherwise the value of "detected" in the > function will be random after calling ipll_find_rate here. Actually I > suggest adding some comments for ipll_find_rate() to emphasize it. > Otherwise, it is easy to think that "rate" and "value" are only output > parameters at first glance. > Since ipll_determine_rate does not use "val" in later, it is OK to not init it. > But I raised a question here: In ipll_find_rate(), do we really need to make > "detected" depend on the value of external input? Inochi, can you please > comment on this, due to you are the author of this code. > As using the detected value, an extra mask is used to emit unused field. See macro PLL_COPY_REG. A more suitable fix may init the "detected" to 0 in the ipll/fpll_find_rate to make "value" is output only. Regards, Inochi
diff --git a/drivers/clk/sophgo/clk-cv18xx-pll.c b/drivers/clk/sophgo/clk-cv18xx-pll.c index 29e24098bf5f..04a0419cab4e 100644 --- a/drivers/clk/sophgo/clk-cv18xx-pll.c +++ b/drivers/clk/sophgo/clk-cv18xx-pll.c @@ -87,7 +87,7 @@ static int ipll_find_rate(const struct cv1800_clk_pll_limit *limit, static int ipll_determine_rate(struct clk_hw *hw, struct clk_rate_request *req) { - u32 val; + u32 val = 0; struct cv1800_clk_pll *pll = hw_to_cv1800_clk_pll(hw); return ipll_find_rate(pll->pll_limit, req->best_parent_rate,
Initializing the val variable of type u32 as it was not initialized. Signed-off-by: Ragavendra Nagraj <ragavendra.bn@gmail.com> --- drivers/clk/sophgo/clk-cv18xx-pll.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)