* re: clk: iproc: add initial common clock support
@ 2015-06-26 10:20 Dan Carpenter
2015-06-26 15:31 ` Ray Jui
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2015-06-26 10:20 UTC (permalink / raw)
To: rjui; +Cc: linux-clk
Hello Ray Jui,
The patch 5fe225c105fd: "clk: iproc: add initial common clock
support" from May 5, 2015, leads to the following static checker
warning:
drivers/clk/bcm/clk-iproc-asiu.c:229 iproc_asiu_setup()
warn: did you mean to pass the address of 'clk_name'
drivers/clk/bcm/clk-iproc-asiu.c
218 for (i = 0; i < num_clks; i++) {
219 struct clk_init_data init;
220 struct clk *clk;
221 const char *parent_name;
222 struct iproc_asiu_clk *asiu_clk;
223 const char *clk_name;
224
225 clk_name = kzalloc(IPROC_CLK_NAME_LEN, GFP_KERNEL);
We shouldn't do this allocation. of_property_read_string_index() will
just re-assign "clk_name" so the memory is leaked.
226 if (WARN_ON(!clk_name))
227 goto err_clk_register;
228
229 ret = of_property_read_string_index(node, "clock-output-names",
230 i, &clk_name);
231 if (WARN_ON(ret))
232 goto err_clk_register;
233
234 asiu_clk = &asiu->clks[i];
235 asiu_clk->name = clk_name;
236 asiu_clk->asiu = asiu;
237 asiu_clk->div = div[i];
238 asiu_clk->gate = gate[i];
239 init.name = clk_name;
240 init.ops = &iproc_asiu_ops;
241 init.flags = 0;
242 parent_name = of_clk_get_parent_name(node, 0);
243 init.parent_names = (parent_name ? &parent_name : NULL);
244 init.num_parents = (parent_name ? 1 : 0);
245 asiu_clk->hw.init = &init;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: clk: iproc: add initial common clock support
2015-06-26 10:20 clk: iproc: add initial common clock support Dan Carpenter
@ 2015-06-26 15:31 ` Ray Jui
0 siblings, 0 replies; 4+ messages in thread
From: Ray Jui @ 2015-06-26 15:31 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-clk
Hi Dan,
On 6/26/2015 3:20 AM, Dan Carpenter wrote:
> Hello Ray Jui,
>
> The patch 5fe225c105fd: "clk: iproc: add initial common clock
> support" from May 5, 2015, leads to the following static checker
> warning:
>
> drivers/clk/bcm/clk-iproc-asiu.c:229 iproc_asiu_setup()
> warn: did you mean to pass the address of 'clk_name'
>
> drivers/clk/bcm/clk-iproc-asiu.c
> 218 for (i = 0; i < num_clks; i++) {
> 219 struct clk_init_data init;
> 220 struct clk *clk;
> 221 const char *parent_name;
> 222 struct iproc_asiu_clk *asiu_clk;
> 223 const char *clk_name;
> 224
> 225 clk_name = kzalloc(IPROC_CLK_NAME_LEN, GFP_KERNEL);
>
> We shouldn't do this allocation. of_property_read_string_index() will
> just re-assign "clk_name" so the memory is leaked.
>
I'll submit a patch to fix this. Thanks.
> 226 if (WARN_ON(!clk_name))
> 227 goto err_clk_register;
> 228
> 229 ret = of_property_read_string_index(node, "clock-output-names",
> 230 i, &clk_name);
> 231 if (WARN_ON(ret))
> 232 goto err_clk_register;
> 233
> 234 asiu_clk = &asiu->clks[i];
> 235 asiu_clk->name = clk_name;
> 236 asiu_clk->asiu = asiu;
> 237 asiu_clk->div = div[i];
> 238 asiu_clk->gate = gate[i];
> 239 init.name = clk_name;
> 240 init.ops = &iproc_asiu_ops;
> 241 init.flags = 0;
> 242 parent_name = of_clk_get_parent_name(node, 0);
> 243 init.parent_names = (parent_name ? &parent_name : NULL);
> 244 init.num_parents = (parent_name ? 1 : 0);
> 245 asiu_clk->hw.init = &init;
>
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* re: clk: iproc: add initial common clock support
@ 2015-06-26 10:23 Dan Carpenter
2015-06-26 15:34 ` Ray Jui
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2015-06-26 10:23 UTC (permalink / raw)
To: rjui; +Cc: linux-clk
Hello Ray Jui,
The patch 5fe225c105fd: "clk: iproc: add initial common clock
support" from May 5, 2015, leads to the following static checker
warning:
drivers/clk/bcm/clk-iproc-pll.c:369 iproc_pll_recalc_rate()
warn: should 'ndiv_int << ctrl->ndiv_int.shift' be a 64 bit type?
drivers/clk/bcm/clk-iproc-pll.c
341 static unsigned long iproc_pll_recalc_rate(struct clk_hw *hw,
342 unsigned long parent_rate)
343 {
344 struct iproc_clk *clk = to_iproc_clk(hw);
345 struct iproc_pll *pll = clk->pll;
346 const struct iproc_pll_ctrl *ctrl = pll->ctrl;
347 u32 val;
348 u64 ndiv;
349 unsigned int ndiv_int, ndiv_frac, pdiv;
350
351 if (parent_rate == 0)
352 return 0;
353
354 /* PLL needs to be locked */
355 val = readl(pll->pll_base + ctrl->status.offset);
356 if ((val & (1 << ctrl->status.shift)) == 0) {
357 clk->rate = 0;
358 return 0;
359 }
360
361 /*
362 * PLL output frequency =
363 *
364 * ((ndiv_int + ndiv_frac / 2^20) * (parent clock rate / pdiv)
365 */
366 val = readl(pll->pll_base + ctrl->ndiv_int.offset);
367 ndiv_int = (val >> ctrl->ndiv_int.shift) &
368 bit_mask(ctrl->ndiv_int.width);
369 ndiv = ndiv_int << ctrl->ndiv_int.shift;
ndiv is declared u64 but only the lower 32 bits are used because of
shift wrapping.
370
371 if (ctrl->flags & IPROC_CLK_PLL_HAS_NDIV_FRAC) {
372 val = readl(pll->pll_base + ctrl->ndiv_frac.offset);
373 ndiv_frac = (val >> ctrl->ndiv_frac.shift) &
374 bit_mask(ctrl->ndiv_frac.width);
375
376 if (ndiv_frac != 0)
377 ndiv = (ndiv_int << ctrl->ndiv_int.shift) | ndiv_frac;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Here as well.
378 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: clk: iproc: add initial common clock support
2015-06-26 10:23 Dan Carpenter
@ 2015-06-26 15:34 ` Ray Jui
0 siblings, 0 replies; 4+ messages in thread
From: Ray Jui @ 2015-06-26 15:34 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-clk
Hi Dan,
On 6/26/2015 3:23 AM, Dan Carpenter wrote:
> Hello Ray Jui,
>
> The patch 5fe225c105fd: "clk: iproc: add initial common clock
> support" from May 5, 2015, leads to the following static checker
> warning:
>
> drivers/clk/bcm/clk-iproc-pll.c:369 iproc_pll_recalc_rate()
> warn: should 'ndiv_int << ctrl->ndiv_int.shift' be a 64 bit type?
>
> drivers/clk/bcm/clk-iproc-pll.c
> 341 static unsigned long iproc_pll_recalc_rate(struct clk_hw *hw,
> 342 unsigned long parent_rate)
> 343 {
> 344 struct iproc_clk *clk = to_iproc_clk(hw);
> 345 struct iproc_pll *pll = clk->pll;
> 346 const struct iproc_pll_ctrl *ctrl = pll->ctrl;
> 347 u32 val;
> 348 u64 ndiv;
> 349 unsigned int ndiv_int, ndiv_frac, pdiv;
> 350
> 351 if (parent_rate == 0)
> 352 return 0;
> 353
> 354 /* PLL needs to be locked */
> 355 val = readl(pll->pll_base + ctrl->status.offset);
> 356 if ((val & (1 << ctrl->status.shift)) == 0) {
> 357 clk->rate = 0;
> 358 return 0;
> 359 }
> 360
> 361 /*
> 362 * PLL output frequency =
> 363 *
> 364 * ((ndiv_int + ndiv_frac / 2^20) * (parent clock rate / pdiv)
> 365 */
> 366 val = readl(pll->pll_base + ctrl->ndiv_int.offset);
> 367 ndiv_int = (val >> ctrl->ndiv_int.shift) &
> 368 bit_mask(ctrl->ndiv_int.width);
> 369 ndiv = ndiv_int << ctrl->ndiv_int.shift;
>
> ndiv is declared u64 but only the lower 32 bits are used because of
> shift wrapping.
>
> 370
> 371 if (ctrl->flags & IPROC_CLK_PLL_HAS_NDIV_FRAC) {
> 372 val = readl(pll->pll_base + ctrl->ndiv_frac.offset);
> 373 ndiv_frac = (val >> ctrl->ndiv_frac.shift) &
> 374 bit_mask(ctrl->ndiv_frac.width);
> 375
> 376 if (ndiv_frac != 0)
> 377 ndiv = (ndiv_int << ctrl->ndiv_int.shift) | ndiv_frac;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Here as well.
>
> 378 }
>
> regards,
> dan carpenter
>
Got it. I'll fix both.
Thanks,
Ray
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-06-26 15:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-26 10:20 clk: iproc: add initial common clock support Dan Carpenter
2015-06-26 15:31 ` Ray Jui
-- strict thread matches above, loose matches on Subject: below --
2015-06-26 10:23 Dan Carpenter
2015-06-26 15:34 ` Ray Jui
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).