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

[Joey Lu <a0987203069@gmail.com>]

On 10/12/2026 4:16 PM, Jerome Brunet wrote:
> 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@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.
I tested v8 on an MA35D1 board
This fixes the same problem I found, so I am dropping my own patch. Once 
this series lands, I will send a follow-up that switches the ADC divider 
to the generic divider.
Thanks!
>