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

-- 
Jerome