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

On 7/22/2026 12:35 AM, Alexandre Mergnat wrote:
> 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]>
Thank you for the review and the Reviewed-by tag!

Agreed. CAPLL and DDRPLL will be split back into separate case labels so
that determine_rate() stays structurally parallel with recalc_rate(), with
the shared `req->rate = pll_freq; return 0;` tail after the switch and an
explicit `default:` label for unknown PLL IDs. I'll include this cleanup
in the next revision.

BR,
Joey