Re: [PATCH 4/6] clk: rockchip: pll: let rockchip_rk3588_pll_k_get update m directly

[Quentin Schulz via U-Boot]

Hi Alexey,

On 7/13/26 8:35 PM, Alexey Charkov wrote:
Selecting the next integer multiplier m is coupled to setting a negative
fractional coefficient k. The current code checks for negative k in two
separate places, which is error-prone.

Yeah and it's kinda ugly :)

Let rockchip_rk3588_pll_k_get update m directly, to make it the single
source of truth for the final value of the integer multiplier m, which
also reduces the number of scattered conditional branches in the code.

Signed-off-by: Alexey Charkov <alch...@flipper.net>
---
  drivers/clk/rockchip/clk_pll.c | 21 +++++++++++++--------
  1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/rockchip/clk_pll.c b/drivers/clk/rockchip/clk_pll.c
index c6fbeb71c77a..6324c11091af 100644
--- a/drivers/clk/rockchip/clk_pll.c
+++ b/drivers/clk/rockchip/clk_pll.c
@@ -168,16 +168,21 @@ rockchip_pll_clk_set_by_auto(ulong fin_hz,
  }
  
  static s16
-rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, u64 fin_hz, u64 fvco)
+rockchip_rk3588_pll_k_get(u32 *m, u32 p, u32 s, u64 fin_hz, u64 fvco)
  {
        u64 fref, ffrac;
        int k;
  
  	fref = fin_hz / p;
-       ffrac = fvco - (m * fref);
+       ffrac = fvco - (*m) * fref;
        k = ffrac * 65536 / fref;
        if (k > 32767) {
-               ffrac = ((m + 1) * fref) - fvco;
+               /*
+                * The requested rate is closer to the next integer multiplier
+                * m, so pick it and use a negative fractional coefficient k
+                */
+               *m += 1;
+               ffrac = (*m) * fref - fvco;
                /*
                 * Round up to avoid overshooting requested rate for negative k
                 */
@@ -201,7 +206,10 @@ rockchip_rk3588_pll_frac_by_auto(unsigned long fin_hz, 
unsigned long fout_hz)
                        for (m = 64; m <= 1023; m++) {
                                if ((fvco >= m * fin_hz / p) &&
                                    (fvco < (m + 1) * fin_hz / p)) {
-                                       k = rockchip_rk3588_pll_k_get(m, p, s,
+                                       u32 m_tmp = m;
+
+                                       k = rockchip_rk3588_pll_k_get(&m_tmp,
+                                                                     p, s,
                                                                      fin_hz,
                                                                      fvco);
                                        if (!k)
@@ -209,10 +217,7 @@ rockchip_rk3588_pll_frac_by_auto(unsigned long fin_hz, 
unsigned long fout_hz)
                                        rate_table->p = p;
                                        rate_table->s = s;
                                        rate_table->k = k;
-                                       if (k > 32767)
-                                               rate_table->m = m + 1;
-                                       else
-                                               rate_table->m = m;
+                                       rate_table->m = m_tmp;
                                        return rate_table;

I don't like this much more, it's a halfway-solution to me. I suggest to 
pass rate_table pointer directly, instead of only m, and update 
rate_table m, p, s and k if we can find a k that works, otherwise leave 
the pointer untouched. Change the function to return 0 on success, 1 (or 
-EINVAL) otherwise and return rate_table in the for-loop if 0 is 
returned, otherwise continue.

Cheers,
Quentin