[bug report] clk: uniphier: add core support code for UniPhier clock driver

[bug report] clk: uniphier: add core support code for UniPhier clock driver

[Dan Carpenter]

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

Re: [bug report] clk: uniphier: add core support code for UniPhier clock driver

[Masahiro Yamada]

Hi Dan, Stephen,


2016-10-12 18:34 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> 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  }



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.






> 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)'


Yes, I followed drivers/clk/clk-mux.c as a reference implementation.





-- 
Best Regards
Masahiro Yamada

Re: [bug report] clk: uniphier: add core support code for UniPhier clock driver

[Stephen Boyd]

On 10/13, Masahiro Yamada wrote:
> 2016-10-12 18:34 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> >
> > We can't return -EINVAL as 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.

> 
> [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.
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [bug report] clk: uniphier: add core support code for UniPhier clock driver

[Masahiro Yamada]

Hi Stephen,



2016-10-28 10:28 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> On 10/13, Masahiro Yamada wrote:
>> 2016-10-12 18:34 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
>> >
>> > We can't return -EINVAL as 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.


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.



-- 
Best Regards
Masahiro Yamada