Re: [PATCH v6 2/3] clk: canaan: Add clock driver for Canaan K230

[Xukai Wang <kingxukai@zohomail.com>]

On 2025/4/18 20:31, Troy Mitchell wrote:
> On Tue, Apr 15, 2025 at 10:25:12PM +0800, Xukai Wang wrote:
>> +	switch (rate_cfg->method) {
>> +	/*
>> +	 * K230_MUL: div_mask+1/div_max...
>> +	 * K230_DIV: mul_max/div_mask+1
>> +	 * K230_MUL_DIV: mul_mask/div_mask...
>> +	 */
>> +	case K230_MUL:
>> +		div = rate_cfg->rate_div_max;
>> +		mul = (readl(rate_cfg->rate_reg) >> rate_cfg->rate_div_shift)
>> +			& rate_cfg->rate_div_mask;
>> +		mul++;
>> +		break;
>> +	case K230_DIV:
>> +		mul = rate_cfg->rate_mul_max;
>> +		div = (readl(rate_cfg->rate_reg) >> rate_cfg->rate_div_shift)
>> +			& rate_cfg->rate_div_mask;
>> +		div++;
>> +		break;
>> +	case K230_MUL_DIV:
>> +		if (!rate_cfg_c) {
>> +			mul = (readl(rate_cfg->rate_reg) >> rate_cfg->rate_mul_shift)
>> +				& rate_cfg->rate_mul_mask;
>> +			div = (readl(rate_cfg->rate_reg) >> rate_cfg->rate_div_shift)
>> +				& rate_cfg->rate_div_mask;
>> +		} else {
>> +			mul = (readl(rate_cfg_c->rate_reg_c) >> rate_cfg_c->rate_mul_shift_c)
>> +				& rate_cfg_c->rate_mul_mask_c;
>> +			div = (readl(rate_cfg->rate_reg) >> rate_cfg->rate_div_shift)
>> +				& rate_cfg->rate_div_mask;
>> +		}
>> +		break;
> Should we report an error in other cases?

This is impossible. The compiler will warn if an enum case is missing.[1]

>> +	}
>> +
>> +	return mul_u64_u32_div(parent_rate, mul, div);
>> +}
>> +
>> +static int k230_clk_find_approximate(struct k230_clk *clk,
>> +				     u32 mul_min,
>> +				     u32 mul_max,
>> +				     u32 div_min,
>> +				     u32 div_max,
>> +				     enum k230_clk_div_type method,
>> +				     unsigned long rate,
>> +				     unsigned long parent_rate,
>> +				     u32 *div,
>> +				     u32 *mul)
>> +{
>> +	long abs_min;
>> +	long abs_current;
>> +	long perfect_divide;
>> +	struct k230_clk_cfg *cfg = k230_clk_cfgs[clk->id];
>> +	struct k230_clk_rate_cfg *rate_cfg = cfg->rate_cfg;
>> +
>> +	const u32 codec_clk[9] = {
>> +		2048000,
>> +		3072000,
>> +		4096000,
>> +		6144000,
>> +		8192000,
>> +		11289600,
>> +		12288000,
>> +		24576000,
>> +		49152000
>> +	};
>> +
>> +	const u32 codec_div[9][2] = {
>> +		{3125, 16},
>> +		{3125, 24},
>> +		{3125, 32},
>> +		{3125, 48},
>> +		{3125, 64},
>> +		{15625, 441},
>> +		{3125, 96},
>> +		{3125, 192},
>> +		{3125, 384}
>> +	};
>> +
>> +	const u32 pdm_clk[20] = {
>> +		128000,
>> +		192000,
>> +		256000,
>> +		384000,
>> +		512000,
>> +		768000,
>> +		1024000,
>> +		1411200,
>> +		1536000,
>> +		2048000,
>> +		2822400,
>> +		3072000,
>> +		4096000,
>> +		5644800,
>> +		6144000,
>> +		8192000,
>> +		11289600,
>> +		12288000,
>> +		24576000,
>> +		49152000
>> +	};
>> +
>> +	const u32 pdm_div[20][2] = {
>> +		{3125, 1},
>> +		{6250, 3},
>> +		{3125, 2},
>> +		{3125, 3},
>> +		{3125, 4},
>> +		{3125, 6},
>> +		{3125, 8},
>> +		{125000, 441},
>> +		{3125, 12},
>> +		{3125, 16},
>> +		{62500, 441},
>> +		{3125, 24},
>> +		{3125, 32},
>> +		{31250, 441},
>> +		{3125, 48},
>> +		{3125, 64},
>> +		{15625, 441},
>> +		{3125, 96},
>> +		{3125, 192},
>> +		{3125, 384}
>> +	};
>> +
>> +	switch (method) {
>> +	/* only mul can be changeable 1/12,2/12,3/12...*/
>> +	case K230_MUL:
>> +		perfect_divide = (long)((parent_rate * 1000) / rate);
>> +		abs_min = abs(perfect_divide -
>> +			     (long)(((long)div_max * 1000) / (long)mul_min));
>> +		*mul = mul_min;
>> +
>> +		for (u32 i = mul_min + 1; i <= mul_max; i++) {
>> +			abs_current = abs(perfect_divide -
>> +					(long)((long)((long)div_max * 1000) / (long)i));
>> +			if (abs_min > abs_current) {
>> +				abs_min = abs_current;
>> +				*mul = i;
>> +			}
>> +		}
>> +
>> +		*div = div_max;
>> +		break;
>> +	/* only div can be changeable, 1/1,1/2,1/3...*/
>> +	case K230_DIV:
>> +		perfect_divide = (long)((parent_rate * 1000) / rate);
>> +		abs_min = abs(perfect_divide -
>> +			     (long)(((long)div_min * 1000) / (long)mul_max));
>> +		*div = div_min;
>> +
>> +		for (u32 i = div_min + 1; i <= div_max; i++) {
>> +			abs_current = abs(perfect_divide -
>> +					 (long)((long)((long)i * 1000) / (long)mul_max));
>> +			if (abs_min > abs_current) {
>> +				abs_min = abs_current;
>> +				*div = i;
>> +			}
>> +		}
>> +
>> +		*mul = mul_max;
>> +		break;
>> +	/* mul and div can be changeable. */
>> +	case K230_MUL_DIV:
>> +		if (rate_cfg->rate_reg_off == K230_CLK_CODEC_ADC_MCLKDIV_OFFSET ||
>> +		    rate_cfg->rate_reg_off == K230_CLK_CODEC_DAC_MCLKDIV_OFFSET) {
>> +			for (u32 j = 0; j < 9; j++) {
>> +				if (0 == (rate - codec_clk[j])) {
>> +					*div = codec_div[j][0];
>> +					*mul = codec_div[j][1];
>> +				}
>> +			}
>> +		} else if (rate_cfg->rate_reg_off == K230_CLK_AUDIO_CLKDIV_OFFSET ||
>> +			   rate_cfg->rate_reg_off == K230_CLK_PDM_CLKDIV_OFFSET) {
>> +			for (u32 j = 0; j < 20; j++) {
>> +				if (0 == (rate - pdm_clk[j])) {
>>
>> +					*div = pdm_div[j][0];
>> +					*mul = pdm_div[j][1];
>> +				}
>> +			}
>> +		} else {
>> +			return -EINVAL;
>> +		}
>> +		break;
> Should we report an error when other case?

The default case is impossible.
The compiler will warn if an enum case is missing.
And Stephen Boyd suggested to remove the default case[1].
Link: https://lore.kernel.org/all/3fb73691f50e599c361dddaff08d3af5.sboyd@kernel.org/ [1]


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv