Re: [PATCH v2 1/1] clk: nuvoton: ma35d1-divider: use direct formula instead of divider table
From: Joey Lu
Date: Mon Oct 12 2026 - 05:32:12 EST
On 10/12/2026 4:16 PM, Jerome Brunet wrote:
On 2026-10-12 13:36 +0800, Joey Lu wrote:I tested v8 on an MA35D1 board
On 10/10/2026 12:35 AM, Jerome Brunet wrote:Have you looked at this series ?
Thanks for the review.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 perRate propagation support is lost with this.
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@xxxxxxxxx>
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);
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.
https://lore.kernel.org/all/20261004-perso-ma35d1-upstream-clk-v8-0-e1a106a128c0@xxxxxxxxxxx/
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.
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!