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

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

[Joey Lu]

The MA35D1 ADC clock divider register implements a simple closed-form
relation, rate = parent_rate / (2 * (N + 1)), but the driver models it
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.
Replace the table with direct arithmetic in recalc_rate()/determine_rate()/
set_rate(), and drop the unused mask_bit mechanism, whose only call site
passed a bitmask instead of a bit index, causing an out-of-range BIT()
shift.

Changes in v2 (addressing the Sashiko AI review of v1):
- Fix a possible division by zero: 2 * rate in determine_rate() and
  set_rate() could overflow for very large requested rates, making the
  divisor zero. Rates above parent_rate / 2 now short-circuit to a
  divider of 1 before the multiplication.
- Fix the mismatch between determine_rate() and set_rate(): both used
  a ceiling to pick the divider, yet determine_rate() reported a
  closest-rounded rate, so set_rate() could program a different divider
  than the one promised. Both now use closest rounding, as the original
  CLK_DIVIDER_ROUND_CLOSEST code did, via a single shared helper
  (ma35d1_clkdiv_get_val()) that also clamps the divider to its valid
  range.
- Not changed: min_rate/max_rate are not checked in the driver, since
  the clk core already applies them and the previous
  divider_determine_rate() based code did not either.

Joey Lu (1):
  clk: nuvoton: ma35d1-divider: use direct formula instead of divider
    table

 drivers/clk/nuvoton/clk-ma35d1-divider.c | 79 ++++++++++++------------
 drivers/clk/nuvoton/clk-ma35d1.c         |  2 +-
 drivers/clk/nuvoton/clk-ma35d1.h         |  2 +-
 3 files changed, 42 insertions(+), 41 deletions(-)

-- 
2.43.0

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

[Joey Lu]

The ADC clock divider register implements rate = parent_rate / (2 * (N + 1)),
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>
---
 drivers/clk/nuvoton/clk-ma35d1-divider.c | 79 ++++++++++++------------
 drivers/clk/nuvoton/clk-ma35d1.c         |  2 +-
 drivers/clk/nuvoton/clk-ma35d1.h         |  2 +-
 3 files changed, 42 insertions(+), 41 deletions(-)

diff --git a/drivers/clk/nuvoton/clk-ma35d1-divider.c b/drivers/clk/nuvoton/clk-ma35d1-divider.c
index e992e7c303419..12a946c989e97 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);
+
+	return 0;
 }
 
 static int ma35d1_clkdiv_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate)
 {
-	int value;
-	unsigned long flags = 0;
-	u32 data;
 	struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw);
+	unsigned long flags;
+	unsigned int val;
+	u32 data;
 
-	value = divider_get_val(rate, parent_rate, dclk->table,
-				dclk->width, CLK_DIVIDER_ROUND_CLOSEST);
+	if (!rate)
+		return -EINVAL;
+
+	val = ma35d1_clkdiv_get_val(dclk, parent_rate, rate);
 
 	spin_lock_irqsave(dclk->lock, flags);
 
 	data = readl_relaxed(dclk->reg);
 	data &= ~(clk_div_mask(dclk->width) << dclk->shift);
-	data |= (value - 1) << dclk->shift;
-	data |= dclk->mask;
+	data |= (val - 1) << dclk->shift;
 	writel_relaxed(data, dclk->reg);
 
 	spin_unlock_irqrestore(dclk->lock, flags);
+
 	return 0;
 }
 
@@ -79,39 +99,21 @@ static const struct clk_ops ma35d1_adc_clkdiv_ops = {
 struct clk_hw *ma35d1_reg_adc_clkdiv(struct device *dev, const char *name,
 				     struct clk_hw *parent_hw, spinlock_t *lock,
 				     unsigned long flags, void __iomem *reg,
-				     u8 shift, u8 width, u32 mask_bit)
+				     u8 shift, u8 width)
 {
-	struct ma35d1_adc_clk_div *div;
-	struct clk_init_data init;
-	struct clk_div_table *table;
 	struct clk_parent_data pdata = { .index = 0 };
-	u32 max_div, min_div;
+	struct ma35d1_adc_clk_div *div;
+	struct clk_init_data init = {};
 	struct clk_hw *hw;
 	int ret;
-	int i;
 
 	div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
 	if (!div)
 		return ERR_PTR(-ENOMEM);
 
-	max_div = clk_div_mask(width) + 1;
-	min_div = 1;
-
-	table = devm_kcalloc(dev, max_div + 1, sizeof(*table), GFP_KERNEL);
-	if (!table)
-		return ERR_PTR(-ENOMEM);
-
-	for (i = 0; i < max_div; i++) {
-		table[i].val = min_div + i;
-		table[i].div = 2 * table[i].val;
-	}
-	table[max_div].val = 0;
-	table[max_div].div = 0;
-
-	memset(&init, 0, sizeof(init));
 	init.name = name;
 	init.ops = &ma35d1_adc_clkdiv_ops;
-	init.flags |= flags;
+	init.flags = flags;
 	pdata.hw = parent_hw;
 	init.parent_data = &pdata;
 	init.num_parents = 1;
@@ -119,15 +121,14 @@ struct clk_hw *ma35d1_reg_adc_clkdiv(struct device *dev, const char *name,
 	div->reg = reg;
 	div->shift = shift;
 	div->width = width;
-	div->mask = mask_bit ? BIT(mask_bit) : 0;
 	div->lock = lock;
 	div->hw.init = &init;
-	div->table = table;
 
 	hw = &div->hw;
 	ret = devm_clk_hw_register(dev, hw);
 	if (ret)
 		return ERR_PTR(ret);
+
 	return hw;
 }
 EXPORT_SYMBOL_GPL(ma35d1_reg_adc_clkdiv);
diff --git a/drivers/clk/nuvoton/clk-ma35d1.c b/drivers/clk/nuvoton/clk-ma35d1.c
index 9f65d0623517a..f91a8369a2bc7 100644
--- a/drivers/clk/nuvoton/clk-ma35d1.c
+++ b/drivers/clk/nuvoton/clk-ma35d1.c
@@ -1017,7 +1017,7 @@ static int ma35d1_clocks_probe(struct platform_device *pdev)
 	hws[ADC_DIV] = ma35d1_reg_adc_clkdiv(dev, "adc_div", hws[PCLK0],
 					     &ma35d1_lock, 0,
 					     clk_base + REG_CLK_CLKDIV4,
-					     4, 17, 0x1ffff);
+					     4, 17);
 	hws[ADC_GATE] = ma35d1_clk_gate(dev, "adc_gate", "adc_div",
 					clk_base + REG_CLK_APBCLK2, 24);
 
diff --git a/drivers/clk/nuvoton/clk-ma35d1.h b/drivers/clk/nuvoton/clk-ma35d1.h
index 3adee440f60a3..1fdcd4658b5ff 100644
--- a/drivers/clk/nuvoton/clk-ma35d1.h
+++ b/drivers/clk/nuvoton/clk-ma35d1.h
@@ -13,6 +13,6 @@ struct clk_hw *ma35d1_reg_clk_pll(struct device *dev, u32 id, u8 u8mode, const c
 struct clk_hw *ma35d1_reg_adc_clkdiv(struct device *dev, const char *name,
 				     struct clk_hw *parent_hw, spinlock_t *lock,
 				     unsigned long flags, void __iomem *reg,
-				     u8 shift, u8 width, u32 mask_bit);
+				     u8 shift, u8 width);
 
 #endif /* __DRV_CLK_NUVOTON_MA35D1_H */
-- 
2.43.0

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

[Jerome Brunet]

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

-- 
Jerome

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

[Joey Lu]

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

Re: [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@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

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

[Joey Lu]

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!
>

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

[Jerome Brunet]

> The MA35D1 ADC clock divider register implements a simple closed-form
> relation, rate = parent_rate / (2 * (N + 1)), but the driver models it
> 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.
> Replace the table with direct arithmetic in recalc_rate()/determine_rate()/
> set_rate(), and drop the unused mask_bit mechanism, whose only call site
> passed a bitmask instead of a bit index, causing an out-of-range BIT()
> shift.
> 
> Changes in v2 (addressing the Sashiko AI review of v1):
> - Fix a possible division by zero: 2 * rate in determine_rate() and
>   set_rate() could overflow for very large requested rates, making the
>   divisor zero. Rates above parent_rate / 2 now short-circuit to a
>   divider of 1 before the multiplication.
> - Fix the mismatch between determine_rate() and set_rate(): both used
>   a ceiling to pick the divider, yet determine_rate() reported a
>   closest-rounded rate, so set_rate() could program a different divider
>   than the one promised. Both now use closest rounding, as the original
>   CLK_DIVIDER_ROUND_CLOSEST code did, via a single shared helper
>   (ma35d1_clkdiv_get_val()) that also clamps the divider to its valid
>   range.
> - Not changed: min_rate/max_rate are not checked in the driver, since
>   the clk core already applies them and the previous
>   divider_determine_rate() based code did not either.
>

A cover letter is not strictly necessary for a single patch,
if you have text or changelog, you can add it after the commit description,
after ---

-- 
Jerome