Everything here sits in a while (true) loop. However, this introduces a couple of layers of indentation. We can simplify the code by introducing a single goto instead of using continue/break. This will also make adding loops in the next patch easier.

Signed-off-by: Sean Anderson seanga2@gmail.com ---

drivers/clk/clk_kendryte.c | 105 ++++++++++++++++++------------------- 1 file changed, 52 insertions(+), 53 deletions(-)

diff --git a/drivers/clk/clk_kendryte.c b/drivers/clk/clk_kendryte.c index 37bd624eca..d2cee2cf97 100644 --- a/drivers/clk/clk_kendryte.c +++ b/drivers/clk/clk_kendryte.c @@ -709,6 +709,10 @@ TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in, * Whether we swapped r and od while enforcing frequency limits */ bool swapped = false; + /* + * Whether the intermediate frequencies are out-of-spec + */ + bool out_of_spec; u64 last_od = od; u64 last_r = r;

@@ -767,76 +771,71 @@ TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in, * aren't in spec, try swapping r and od. If everything is * in-spec, calculate the relative error. */ - while (true) { +again: + out_of_spec = false; + if (r > max_r) { + out_of_spec = true; + } else { /* - * Whether the intermediate frequencies are out-of-spec + * There is no way to only divide once; we need + * to examine the frequency with and without the + * effect of od. */ - bool out_of_spec = false; + u64 vco = DIV_ROUND_CLOSEST_ULL(rate_in * f, r);

- if (r > max_r) { + if (vco > 1750000000 || vco < 340000000) out_of_spec = true; - } else { - /* - * There is no way to only divide once; we need - * to examine the frequency with and without the - * effect of od. - */ - u64 vco = DIV_ROUND_CLOSEST_ULL(rate_in * f, r); + }

- if (vco > 1750000000 || vco < 340000000) - out_of_spec = true; + if (out_of_spec) { + u64 new_r, new_od; + + if (!swapped) { + u64 tmp = r; + + r = od; + od = tmp; + swapped = true; + goto again; }

- if (out_of_spec) { - if (!swapped) { - u64 tmp = r; - - r = od; - od = tmp; - swapped = true; - continue; - } else { - /* - * Try looking ahead to see if there are - * additional factors for the same - * product. - */ - if (i + 1 < ARRAY_SIZE(factors)) { - u64 new_r, new_od; - - i++; - new_r = UNPACK_R(factors[i]); - new_od = UNPACK_OD(factors[i]); - if (r * od == new_r * new_od) { - r = new_r; - od = new_od; - swapped = false; - continue; - } - i--; - } - break; + /* + * Try looking ahead to see if there are additional + * factors for the same product. + */ + if (i + 1 < ARRAY_SIZE(factors)) { + i++; + new_r = UNPACK_R(factors[i]); + new_od = UNPACK_OD(factors[i]); + if (r * od == new_r * new_od) { + r = new_r; + od = new_od; + swapped = false; + goto again; } + i--; }

- error = DIV_ROUND_CLOSEST_ULL(f * inv_ratio, r * od); - /* The lower 16 bits are spurious */ - error = abs((error - BIT(32))) >> 16; + /* We ran out of things to try */ + continue; + }

- if (error < best_error) { - best->r = r; - best->f = f; - best->od = od; - best_error = error; - } - break; + error = DIV_ROUND_CLOSEST_ULL(f * inv_ratio, r * od); + /* The lower 16 bits are spurious */ + error = abs((error - BIT(32))) >> 16; + + if (error < best_error) { + best->r = r; + best->f = f; + best->od = od; + best_error = error; } } while (f < 64 && i + 1 < ARRAY_SIZE(factors) && error != 0);

+ log_debug("best error %lld\n", best_error); if (best_error == S64_MAX) return -EINVAL;

- log_debug("best error %lld\n", best_error); return 0; }

On 2021/07/27 12:51, Sean Anderson wrote:

Some clock functions return ulong but still have "negative" errors. To deal with this, cast the relevant arguments to long.

Fixes: 609bd60b94 ("clk: k210: Rewrite to remove CCF") Reported-by: Coverity Scan scan-admin@coverity.com Signed-off-by: Sean Anderson seanga2@gmail.com

---

drivers/clk/clk_kendryte.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk_kendryte.c b/drivers/clk/clk_kendryte.c index 3148756968..37bd624eca 100644 --- a/drivers/clk/clk_kendryte.c +++ b/drivers/clk/clk_kendryte.c @@ -439,7 +439,7 @@ static const struct k210_clk_params k210_clks[] = { #ifdef CONFIG_CLK_K210_SET_RATE static int k210_pll_enable(struct k210_clk_priv *priv, int id); static int k210_pll_disable(struct k210_clk_priv *priv, int id); -static ulong k210_pll_get_rate(struct k210_clk_priv *priv, int id, ulong rate_in); +static ulong k210_pll_get_rate(struct k210_clk_priv *priv, int id, long rate_in);

/*

• The PLL included with the Kendryte K210 appears to be a True Circuits, Inc.

@@ -841,7 +841,7 @@ TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in, }

static ulong k210_pll_set_rate(struct k210_clk_priv *priv, int id, ulong rate,

Shouldn't this one return a long, in case of error ? It seems that the commit messages hints at such a change, but you are changing the argument type instead. A little confusing. What am I missing ?

• 	       ulong rate_in)
  

• 	       long rate_in)
  

{ int err; const struct k210_pll_params *pll = &k210_plls[id]; @@ -890,7 +890,7 @@ static ulong k210_pll_set_rate(struct k210_clk_priv *priv, int id, ulong rate, #endif /* CONFIG_CLK_K210_SET_RATE */

static ulong k210_pll_get_rate(struct k210_clk_priv *priv, int id,

Same here ?

• 	       ulong rate_in)
  

• 	       long rate_in)
  

I would assume that these functions are called if the rate_in argument is correct, so I do not really understand why the argument type needs to be changed...

{ u64 r, f, od; u32 reg = readl(priv->base + k210_plls[id].off);