clk: uniphier: add core support code for UniPhier clock driver
Hello Masahiro Yamada,
The patch 734d82f4a678: "clk: uniphier: add core support code for UniPhier clock driver" from Sep 16, 2016, leads to the following static checker warning:
drivers/clk/uniphier/clk-uniphier-mux.c:56 uniphier_clk_mux_get_parent()
warn: signedness bug returning '(-22)'
drivers/clk/uniphier/clk-uniphier-mux.c
40 static u8 uniphier_clk_mux_get_parent(struct clk_hw *hw)
^^
41 {
42 struct uniphier_clk_mux *mux = to_uniphier_clk_mux(hw);
43 int num_parents = clk_hw_get_num_parents(hw);
44 int ret;
45 u32 val;
46 u8 i;
47
48 ret = regmap_read(mux->regmap, mux->reg, &val);
49 if (ret)
50 return ret;
51
52 for (i = 0; i < num_parents; i++)
53 if ((mux->masks[i] & val) == mux->vals[i])
54 return i;
55
56 return -EINVAL;
^^^^^^^We can't return -EINVAL as a u8.
57 }
There are a bunch of similar bugs:
drivers/clk/samsung/clk-s3c2410-dclk.c:72 s3c24xx_clkout_get_parent() warn: signedness bug returning '(-22)' drivers/clk/uniphier/clk-uniphier-mux.c:56 uniphier_clk_mux_get_parent() warn: signedness bug returning '(-22)' drivers/clk/microchip/clk-core.c:296 roclk_get_parent() warn: signedness bug returning '(-22)' drivers/clk/microchip/clk-core.c:837 sclk_get_parent() warn: signedness bug returning '(-22)' drivers/clk/ti/mux.c:51 ti_clk_mux_get_parent() warn: signedness bug returning '(-22)' drivers/clk/ti/mux.c:61 ti_clk_mux_get_parent() warn: signedness bug returning '(-22)' drivers/clk/nxp/clk-lpc32xx.c:1018 clk_mux_get_parent() warn: signedness bug returning '(-22)' drivers/clk/nxp/clk-lpc32xx.c:1022 clk_mux_get_parent() warn: signedness bug returning '(-22)' drivers/clk/rockchip/clk-ddr.c:91 rockchip_ddrclk_get_parent() warn: signedness bug returning '(-22)' drivers/clk/clk-mux.c:51 clk_mux_get_parent() warn: signedness bug returning '(-22)' drivers/clk/clk-mux.c:61 clk_mux_get_parent() warn: signedness bug returning '(-22)'
regards,
dan carpenter
Hi Dan, Stephen,
2016-10-12 18:34 GMT+09:00 Dan Carpenter <[email protected]>:
Quoted 30 lines
Hello Masahiro Yamada,
The patch
734d82f4a678: "clk: uniphier: add core support code for UniPhier clock driver" from Sep 16, 2016, leads to the following static checker warning:
drivers/clk/uniphier/clk-uniphier-mux.c:56uniphier_clk_mux_get_parent()
warn: signedness bug returning '(-22)'
drivers/clk/uniphier/clk-uniphier-mux.c40 static u8 uniphier_clk_mux_get_parent(struct clk_hw *hw) ^^ 41 { 42 struct uniphier_clk_mux *mux = to_uniphier_clk_mux(hw); 43 int num_parents = clk_hw_get_num_parents(hw); 44 int ret; 45 u32 val; 46 u8 i; 47 48 ret = regmap_read(mux->regmap, mux->reg, &val); 49 if (ret) 50 return ret; 51 52 for (i = 0; i < num_parents; i++) 53 if ((mux->masks[i] & val) == mux->vals[i]) 54 return i; 55 56 return -EINVAL; ^^^^^^^
Quoted 3 lines
We can't return
-EINVALas a u8.57 }
I think we should change the return type of .get_parent() callback to "int" so we can handle error code.
(perhaps, the argument of .set_parent() as well for consistency?)
[1]
I guess regmap_read() is often involved in .get_parent() and it may return a negative value on failure.
[2]
On boot-up, the register my be read as
a value that is not covered by the clk driver.
Quoted 13 lines
There are a bunch of similar bugs:
drivers/clk/samsung/clk-s3c2410-dclk.c:72s3c24xx_clkout_get_parent()warn: signedness bug returning '(-22)'drivers/clk/uniphier/clk-uniphier-mux.c:56uniphier_clk_mux_get_parent()warn: signedness bug returning '(-22)'drivers/clk/microchip/clk-core.c:296roclk_get_parent()warn: signedness bug returning '(-22)'drivers/clk/microchip/clk-core.c:837sclk_get_parent()warn: signedness bug returning '(-22)'drivers/clk/ti/mux.c:51ti_clk_mux_get_parent()warn: signedness bug returning '(-22)'drivers/clk/ti/mux.c:61ti_clk_mux_get_parent()warn: signedness bug returning '(-22)'drivers/clk/nxp/clk-lpc32xx.c:1018clk_mux_get_parent()warn: signedness bug returning '(-22)'drivers/clk/nxp/clk-lpc32xx.c:1022clk_mux_get_parent()warn: signedness bug returning '(-22)'drivers/clk/rockchip/clk-ddr.c:91rockchip_ddrclk_get_parent()warn: signedness bug returning '(-22)'drivers/clk/clk-mux.c:51clk_mux_get_parent()warn: signedness bug returning '(-22)'drivers/clk/clk-mux.c:61clk_mux_get_parent()warn: signedness bug returning '(-22)'
Yes, I followed drivers/clk/clk-mux.c as a reference implementation.
Signature · 2 lines
Best Regards Masahiro Yamada
Quoted 11 lines · Masahiro Yamada
2016-10-12 18:34 GMT+09:00 Dan Carpenter <[email protected]>:
Quoted 4 lines
We can't return
-EINVALas a u8.57 }I think we should change the return type of .get_parent() callback to "int" so we can handle error code.
(perhaps, the argument of .set_parent() as well for consistency?)
Yeah we've been thinking of changing get_parent() to return a struct clk_hw pointer (or an error pointer). That would simplify things by avoiding lots of remapping. But to do that immediately we would need to convert all of the drivers. I guess we could have a get_parent_hw() op for that and convert everyone slowly.
Should we make the set_parent op take a hw pointer too? I'm not sure about it. It would be nice to have some generic way to map the hw pointer to a hw index for a clock. Having the tables of parent pointers in the core and making the drivers know about that order and how it relates to the values they program into the hardware is awkward sometimes.
Quoted 9 lines
[1]
I guessregmap_read()is often involved in .get_parent() and it may return a negative value on failure.[2]
On boot-up, the register my be read as
a value that is not covered by the clk driver.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Hi Stephen,
2016-10-28 10:28 GMT+09:00 Stephen Boyd <[email protected]>:
Quoted 9 lines · Masahiro Yamada
Quoted 8 lines
2016-10-12 18:34 GMT+09:00 Dan Carpenter <[email protected]>:
Quoted 4 lines
We can't return
-EINVALas a u8.57 }
Quoted 7 lines
Quoted 3 lines
I think we should change the return type of .get_parent() callback to "int" so we can handle error code.
(perhaps, the argument of .set_parent() as well for consistency?)Yeah we've been thinking of changing
get_parent()to return a structclk_hwpointer (or an error pointer). That would simplify things by avoiding lots of remapping.
I am not quite sure about this.
Currently, parent's clk_hw can be retrieved in the following process:
[1] read a register and get the parent index
[2] Get core->parents[index]
[1] is a job of low-level driver and [2] is managed in the core framework.
I am not sure if I understood correctly, but your suggestion means that
[2] will be moved to lowlevel drivers as well?
If so, I felt like similar code will be duplicated and I wonder if it makes sense.
Signature · 2 lines
Best Regards Masahiro Yamada