clk: lpc32xx: add common clock framework driver

Hello Vladimir Zapolskiy (and other clk devs as well),

The patch f7c82a60ba26: "clk: lpc32xx: add common clock framework driver" from Dec 6, 2015, leads to the following static checker warning:

	drivers/clk/nxp/clk-lpc32xx.c:1019 clk_mux_get_parent()
	warn: signedness bug returning '(-22)'

drivers/clk/nxp/clk-lpc32xx.c
1003 static u8 clk_mux_get_parent(struct clk_hw *hw)

               ^^
  1004  {
  1005          struct lpc32xx_clk_mux *mux = to_lpc32xx_mux(hw);
  1006          u32 num_parents = clk_hw_get_num_parents(hw);
  1007          u32 val;
  1008  
  1009          regmap_read(clk_regmap, mux->reg, &val);
  1010          val >>= mux->shift;
  1011          val &= mux->mask;
  1012  
  1013          if (mux->table) {
  1014                  u32 i;
  1015  
  1016                  for (i = 0; i < num_parents; i++)
  1017                          if (mux->table[i] == val)
  1018                                  return i;
  1019                  return -EINVAL;
                        ^^^^^^^^^^^^^^
  1020          }
  1021  
  1022          if (val >= num_parents)
  1023                  return -EINVAL;
                        ^^^^^^^^^^^^^^

This obviously doesn't work since this is a u8 function.

I looked at how this was called to see what we should return on error. This is called from __clk_init(). It tests for negative error codes. Ooops. Also there is no static checker warning for that but I will create one. I'm not sure what to do here. I bet the correct thing is to convert all the get_parent() function to return int. Another option might be to add documentation, but I feel like in the kernel int returns are self documenting and that's the best option.

1024
1025 return val;
1026 }

regards,
dan carpenter

Hello Dan,

oops, I blindly missed it, which static checker do you use by the way?

GCC and sparse haven't produced this error, may be I have to upgrade them...

Definitely, thank you for reporting.

"Negative" values may be returned here only if the driver has another kind of error, namely if description of some mux clock parents does not correspond to the actual hardware. It may happen due to a typo in the code or a typo in the spec for example, but if there is no first class error, then the underlined returns are dead code. To my best knowledge there is no such problem in this particular driver code.

However it is worth to mention that this piece of code is practically a regmap version of common clock framework clk-mux.c clk_mux_get_parent() function, which is commonly used and has the same problem spread over multiple drivers.

Moreover talking about CCF this kind of a problem touches other hardware querying functions:
* .is_prepared (clamped return value to bool in the framework code),
* .is_enabled (clamped return value to bool in the framework code),
* .recalc_rate (fallback to 0 seems to be a valid workaround),
* .get_parent (discussed here),
* .get_phase (this one looks okay).

I've found other potentially faulty examples from this class, e.g.

* clk_is_enabled_regmap() from qcom/clk-regmap.c -- clamped to bool,
* omap2_init_dpll_parent() from ti/clkt_dpll.c,
* mux_get_parent() from clk-qoriq.c -- fallback to index 0,
* wm831x_clkout_get_parent() from clk-wm831x.c -- fallback to index 0,
* kona_peri_clk_get_parent() from bcm/clk-kona.c -- fallback to index 0,
* etc.

Well, not exactly, but more or less. It is important to say that the function (and the second user of .get_parent() __clk_init_parent) will be significantly reworked in v4.6 (not in clk tree yet), I have started to work on the fix on basis of that future version of clk.c.

Sounds great, thank you in advance.

I agree with you, I'm implementing this fix now, when it is ready I'll share the change with the CCF maintainers and the list for review.

Hopefully Mike and Stephen agree also, let see their comments.

Thank you again for reporting.

--
With best wishes,
Vladimir