[PATCH v2 1/1] clk: nuvoton: ma35d1-divider: use direct formula instead of divider table

[Jerome Brunet]

On 2026-10-12 13:36 +0800, Joey Lu wrote:
> 
> On 10/10/2026 12:35 AM, Jerome Brunet wrote:
> >> The ADC clock divider register implements rate = parent_rate / (2 * (N + 1)),
> > Are you open-coding CLK_DIVIDER_EVEN_INTEGERS ?
> >
> >   * CLK_DIVIDER_EVEN_INTEGERS - clock divisor is 2, 4, 6, 8, 10, etc.
> >   *	Formula is 2 * (value read from hardware + 1)
> >
> > Am wondering if this driver is necessary at all ? Can you just use the
> > regular divider here ?
> >
> >> which was previously modeled by building a clk_div_table with one entry per
> >> possible divider value (up to 2^width entries) and feeding it through the
> >> generic divider_recalc_rate()/divider_determine_rate()/divider_get_val()
> >> helpers. That is unnecessary overhead (a large table allocation and a
> >> linear scan on every rate lookup) for a relationship with a simple
> >> closed-form expression, so compute the rate directly instead.
> >>
> >> Drop the mask_bit/mask mechanism as well. The previous code unconditionally
> >> OR'd an arbitrary bit into the divider register on every set_rate() call,
> >> and the one existing caller passed mask_bit as a bitmask value (0x1ffff)
> >> rather than a bit index, so BIT() shifted by an out-of-range amount, which
> >> is undefined behavior. The divider field write already clears and rewrites
> >> only its own bits, so no extra forced bit is needed.
> >>
> >> Select the divider with a single helper shared by determine_rate() and
> >> set_rate(), using closest rounding as before, so the rate promised by
> >> determine_rate() is the rate that set_rate() programs. The helper clamps
> >> the divider to the valid range and avoids overflowing 2 * rate for very
> >> large requested rates, which would otherwise lead to a division by zero.
> >> Reject a rate of 0 with -EINVAL.
> >>
> >> Fixes: 691521a367cf ("clk: nuvoton: Add clock driver for ma35d1 clock controller")
> >> Signed-off-by: Joey Lu <a0987203069 at gmail.com>
> >>
> >> diff --git a/drivers/clk/nuvoton/clk-ma35d1-divider.c b/drivers/clk/nuvoton/clk-ma35d1-divider.c
> >> index e992e7c30341..12a946c989e9 100644
> >> --- a/drivers/clk/nuvoton/clk-ma35d1-divider.c
> >> +++ b/drivers/clk/nuvoton/clk-ma35d1-divider.c
> >> @@ -16,8 +16,6 @@ struct ma35d1_adc_clk_div {
> >>   	void __iomem *reg;
> >>   	u8 shift;
> >>   	u8 width;
> >> -	u32 mask;
> >> -	const struct clk_div_table *table;
> >>   	/* protects concurrent access to clock divider registers */
> >>   	spinlock_t *lock;
> >>   };
> >> @@ -29,44 +27,66 @@ static inline struct ma35d1_adc_clk_div *to_ma35d1_adc_clk_div(struct clk_hw *_h
> >>   
> >>   static unsigned long ma35d1_clkdiv_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
> >>   {
> >> -	unsigned int val;
> >>   	struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw);
> >> +	unsigned int val;
> >>   
> >>   	val = readl_relaxed(dclk->reg) >> dclk->shift;
> >>   	val &= clk_div_mask(dclk->width);
> >> -	val += 1;
> >> -	return divider_recalc_rate(hw, parent_rate, val, dclk->table,
> >> -				   CLK_DIVIDER_ROUND_CLOSEST, dclk->width);
> >> +
> >> +	return DIV_ROUND_CLOSEST_ULL((u64)parent_rate, 2 * (val + 1));
> >> +}
> >> +
> >> +static unsigned int ma35d1_clkdiv_get_val(struct ma35d1_adc_clk_div *dclk,
> >> +					  unsigned long parent_rate,
> >> +					  unsigned long rate)
> >> +{
> >> +	unsigned long max = clk_div_mask(dclk->width) + 1;
> >> +	unsigned long val;
> >> +
> >> +	if (rate > parent_rate / 2)
> >> +		return 1;
> >> +
> >> +	val = DIV_ROUND_CLOSEST_ULL((u64)parent_rate, 2 * rate);
> >> +
> >> +	return clamp(val, 1UL, max);
> >>   }
> >>   
> >>   static int ma35d1_clkdiv_determine_rate(struct clk_hw *hw,
> >>   					struct clk_rate_request *req)
> >>   {
> >>   	struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw);
> >> +	unsigned int val;
> >>   
> >> -	return divider_determine_rate(hw, req, dclk->table, dclk->width,
> >> -				      CLK_DIVIDER_ROUND_CLOSEST);
> >> +	if (!req->rate)
> >> +		return -EINVAL;
> >> +
> >> +	val = ma35d1_clkdiv_get_val(dclk, req->best_parent_rate, req->rate);
> >> +	req->rate = DIV_ROUND_CLOSEST_ULL((u64)req->best_parent_rate, 2 * val);
> > Rate propagation support is lost with this.
> Thanks for the review.
> On open-coding CLK_DIVIDER_EVEN_INTEGERS: you're right. The ADC divider 
> is exactly 2 * (val + 1), so the regular divider covers it. The 
> dedicated driver is not needed. You're also right that my 
> determine_rate() lost rate propagation, because it never forwarded the 
> request to the parent the way divider_determine_rate() does.
> I tested the regular divider on hardware. That showed a separate problem 
> in the existing clock tree. The mux parents are declared with .fw_name 
> only, but the controller node has no clock-names, so the lookup fails. 
> sysclk1_mux and everything below it (pclk0, adc_div, and so on) end up 
> as orphans at 0 Hz. As a result clk_set_rate() on the ADC clock returns 
> early without writing the divider register. That hid the real behaviour 
> of the divider.
> So I will drop this series and send a new series that:
> 1. fixes the parent lookup in the clock tree, and
> 2. switches the ADC divider to the regular divider and removes the 
> dedicated driver together with the unused mask_bit parameter.

Have you looked at this series ?

https://lore.kernel.org/all/20261004-perso-ma35d1-upstream-clk-v8-0-e1a106a128c0@bootlin.com/

It does address several problems the existing driver has when it comes to
parents and incorrect usage of .fw.name

Please sync with Miquel on this.

-- 
Jerome