Re: [PATCH v5 2/4] clk: rockchip: Add internal support for 24-bit fractional dividers

[Heiko Stuebner <heiko@sntech.de>]

Hi,


sorry it took me a bit to get to clock driver patches :-)

Am Freitag, 30. Oktober 2026, 10:43:58 Mitteleuropäische Normalzeit schrieb Cunhao Lu via B4 Relay:
> From: Elaine Zhang <zhangqing@rock-chips.com>
> 
> Some Rockchip clocks split each 24-bit numerator and denominator between
> a low register containing 16 bits of each value and a high register
> containing the remaining 8 bits.
> 
> Add an internal Rockchip fractional-divider helper and clock branch type
> for this register layout. This provides the support needed by the RK3572
> audio fractional clocks.

Please add a paragraph about the "v2" naming.
I assume the v2 is a Rockchip naming, because v1 was the other one that
could still fit the regular franctional divider.



> +static inline u32 clk_fd_v2_readl(struct clk_fractional_divider_v2 *fd,
> +				  void __iomem *reg)
> +{
> +	return readl(reg);
> +}
> +
> +static inline void clk_fd_v2_writel(struct clk_fractional_divider_v2 *fd,
> +				    void __iomem *reg, u32 val)
> +{
> +	writel(val, reg);
> +}


why do we have those? They do not serve any purpose I can see.
So please just use readl and writel directly at the appropriate places.


> +static int clk_fd_v2_get_divider(struct clk_hw *hw, unsigned long rate,
> +				 unsigned long parent_rate,
> +				 unsigned long *m, unsigned long *n)
> +{
> +	struct clk_fractional_divider_v2 *fd = to_clk_fd_v2(hw);
> +	u32 factor;
> +
> +	if (!parent_rate)
> +		return -EINVAL;
> +	if (!rate || rate > parent_rate)
> +		rate = parent_rate;
> +
> +	rational_best_approximation(rate, parent_rate,
> +				    GENMASK(fd->mwidth - 1, 0),
> +				    GENMASK(fd->nwidth - 1, 0),
> +				    m, n);
> +
> +	if (fd->flags & CLK_FRAC_DIVIDER_ZERO_BASED) {
> +		(*m)--;
> +		(*n)--;
> +	}

... blank line please ...

> +	if (!*m && !(fd->flags & CLK_FRAC_DIVIDER_ZERO_BASED))
> +		return clk_fd_v2_get_low_rate_divider(fd, rate, parent_rate, m, n);

... blank line please ...

> +	/*
> +	 * When compensation the fractional divider,
> +	 * the [1:0] bits of the numerator register are omitted,
> +	 * which will lead to a large deviation in the result.
> +	 * Therefore, it is required that the numerator must
> +	 * be at least 4.
> +	 *
> +	 * Note that there are some exceptions here:
> +	 * If there is an even frac div, we need to keep the original
> +	 * numerator(<4) and denominator. Otherwise, it may cause the
> +	 * issue that the duty ratio is not 50%.
> +	 */
> +	if (*m < 4 && *m != 0) {
> +		if (*n % 2 == 0)
> +			factor = 1;
> +		else
> +			factor = DIV_ROUND_UP(4, *m);
> +
> +		if (*m > GENMASK(fd->mwidth - 1, 0) / factor ||
> +		    *n > GENMASK(fd->nwidth - 1, 0) / factor) {
> +			if (fd->flags & CLK_FRAC_DIVIDER_ZERO_BASED)
> +				return -EINVAL;
> +			return clk_fd_v2_get_low_rate_divider(fd, rate, parent_rate, m, n);
> +		}
> +
> +		*n *= factor;
> +		*m *= factor;
> +	}
> +
> +	return 0;
> +}
> +

[...]

> +
> +static const struct clk_ops clk_fractional_divider_ops_v2 = {
> +	.recalc_rate = clk_fd_v2_recalc_rate,
> +	.determine_rate = clk_fd_v2_determine_rate,
> +	.set_rate = clk_fd_v2_set_rate,
> +};
> +
> +static struct clk_hw *clk_hw_register_fractional_divider_v2(struct device *dev,

maybe adapt the function name.
Function above abbreviated things to

clk_fd_v2_something, so naming this clk_hw_register_fd_v2()
might make that line less unwieldy?



Thanks
Heiko