Re: [PATCH v4 3/3] clk: nuvoton: ma35d1: fix ma35d1_clk_pll_determine_rate logic

On Tue, 21 Jul 2026 10:13:56 +0800, Joey Lu <[email protected]> wrote:
> diff --git a/drivers/clk/nuvoton/clk-ma35d1-pll.c b/drivers/clk/nuvoton/clk-ma35d1-pll.c
> index eb9d69d2077b..c7c0dc91a012 100644
> --- a/drivers/clk/nuvoton/clk-ma35d1-pll.c
> +++ b/drivers/clk/nuvoton/clk-ma35d1-pll.c
> @@ -255,32 +255,32 @@ static int ma35d1_clk_pll_determine_rate(struct clk_hw *hw,
> [ ... skip 14 lines ... ]
> +		if (pll->id == CAPLL) {
> +			pll_freq = ma35d1_calc_smic_pll_freq(reg_ctl[0], req->best_parent_rate);
> +		} else {
> +			reg_ctl[1] = readl_relaxed(pll->ctl1_base);
> +			pll_freq = ma35d1_calc_pll_freq(pll->mode, reg_ctl, req->best_parent_rate);
> +		}

Small, non-blocking readability suggestion: since we just switched on
pll->id, re-checking `if (pll->id == CAPLL)` inside the merged case reads a
little redundant. Would it be cleaner to keep CAPLL and DDRPLL as separate
case labels, mirroring ma35d1_clk_pll_recalc_rate() just above, where CAPLL
is the SMIC-design special case and DDRPLL uses the standard calc, and share
a single `req->rate = pll_freq; return 0;` tail? Roughly:

    case CAPLL:
            reg_ctl[0] = readl_relaxed(pll->ctl0_base);
            pll_freq = ma35d1_calc_smic_pll_freq(reg_ctl[0], req->best_parent_rate);
            break;
    case DDRPLL:
            reg_ctl[0] = readl_relaxed(pll->ctl0_base);
            reg_ctl[1] = readl_relaxed(pll->ctl1_base);
            pll_freq = ma35d1_calc_pll_freq(pll->mode, reg_ctl, req->best_parent_rate);
            break;
    case APLL:
    case EPLL:
    case VPLL:
            ret = ma35d1_pll_find_closest(...);
            if (ret < 0)
                    return ret;
            break;
    default:
            req->rate = 0;
            return 0;
    }
    req->rate = pll_freq;
    return 0;

That keeps determine_rate() and recalc_rate() structurally parallel. The
logic as written looks correct either way, so please treat this purely as a
readability suggestion.

Otherwise,

Reviewed-by: Alexandre Mergnat <[email protected]>

-- 
Alexandre Mergnat <[email protected]>