Hi Quentin, On Tue, Jul 21, 2026 at 4:19 PM Quentin Schulz <[email protected]> wrote: > > 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 <[email protected]> > > --- > > 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.
Indeed, can do that. Will reshuffle in v2. Thanks for the idea! Best regards, Alexey
