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

[Joey Lu <a0987203069@gmail.com>]

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@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.

Best regards,
Joey