Re: [PATCH 2/2] clock: eswin: Add eic7700 clock driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 14/05/2025 02:26, dongxuyang@eswincomputing.com wrote:
>> +static int eswin_cpu_clk_init(struct platform_device *pdev)
> +{
> +	struct clk *cpu_clk;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	u32 default_freq;
> +	int ret = 0;
> +	int numa_id;
> +	char name[128] = { 0 };
> +
> +	ret = of_property_read_u32(np, "cpu-default-frequency", &default_freq);

NAK, undocumented ABI.

You already got such comments. All your patches repeat the same mistakes.

> +	if (ret) {
> +		dev_info(dev, "cpu-default-frequency not set\n");
> +		return ret;
> +	}
> +	numa_id = dev_to_node(dev->parent);
> +	if (numa_id < 0)
> +		sprintf(name, "%s", "clk_cpu_ext_src_core_clk_0");
> +	else
> +		sprintf(name, "d%d_%s", numa_id, "clk_cpu_ext_src_core_clk_0");
> +
> +	cpu_clk = __clk_lookup(name);
> +	if (!cpu_clk) {
> +		dev_err(dev, "Failed to lookup CPU clock\n");
> +		return -EINVAL;
> +	}
> +	ret = clk_set_rate(cpu_clk, default_freq);
> +	if (ret) {
> +		dev_err(dev, "Failed to set CPU frequency: %d\n", ret);
> +		return ret;
> +	}
> +	dev_info(dev, "CPU frequency set to %u Hz\n", default_freq);

Drop, this is supposed to be silent.

> +	return 0;
> +}
> +
> +static int eswin_clk_probe(struct platform_device *pdev)
> +{
> +	struct eswin_clock_data *clk_data;
> +
> +	clk_data = eswin_clk_init(pdev, EIC7700_NR_CLKS);
> +	if (!clk_data)
> +		return -EAGAIN;
> +
> +	special_div_table_init(u_3_bit_special_div_table,
> +			       ARRAY_SIZE(u_3_bit_special_div_table));
> +	special_div_table_init(u_4_bit_special_div_table,
> +			       ARRAY_SIZE(u_4_bit_special_div_table));
> +	special_div_table_init(u_6_bit_special_div_table,
> +			       ARRAY_SIZE(u_6_bit_special_div_table));
> +	special_div_table_init(u_7_bit_special_div_table,
> +			       ARRAY_SIZE(u_7_bit_special_div_table));
> +	special_div_table_init(u_8_bit_special_div_table,
> +			       ARRAY_SIZE(u_8_bit_special_div_table));
> +	special_div_table_init(u_11_bit_special_div_table,
> +			       ARRAY_SIZE(u_11_bit_special_div_table));
> +	special_div_table_init(u_16_bit_special_div_table,
> +			       ARRAY_SIZE(u_16_bit_special_div_table));
> +
> +	eswin_clk_register_fixed_rate(eic7700_fixed_rate_clks,
> +				      ARRAY_SIZE(eic7700_fixed_rate_clks),
> +				      clk_data);
> +	eswin_clk_register_pll(eic7700_pll_clks, ARRAY_SIZE(eic7700_pll_clks),
> +			       clk_data, &pdev->dev);
> +
> +	eswin_clk_register_fixed_factor(eic7700_fixed_factor_clks,
> +					ARRAY_SIZE(eic7700_fixed_factor_clks),
> +					clk_data);
> +	eswin_clk_register_mux(eic7700_mux_clks, ARRAY_SIZE(eic7700_mux_clks),
> +			       clk_data);
> +	eswin_clk_register_clk(eic7700_clks_early_0,
> +			       ARRAY_SIZE(eic7700_clks_early_0), clk_data);
> +	eswin_clk_register_divider(eic7700_div_clks,
> +				   ARRAY_SIZE(eic7700_div_clks), clk_data);
> +	eswin_clk_register_clk(eic7700_clks_early_1,
> +			       ARRAY_SIZE(eic7700_clks_early_1), clk_data);
> +	eswin_clk_register_gate(eic7700_gate_clks,
> +				ARRAY_SIZE(eic7700_gate_clks), clk_data);
> +	eswin_clk_register_clk(eic7700_clks, ARRAY_SIZE(eic7700_clks),
> +			       clk_data);
> +
> +	eswin_cpu_clk_init(pdev);
> +
> +	return 0;



...

> +int eswin_clk_register_clk(const struct eswin_clock *clks, int nums,
> +			   struct eswin_clock_data *data)
> +{
> +	struct clk *clk;
> +	int i;
> +
> +	for (i = 0; i < nums; i++) {
> +		char *name = kzalloc(strlen(clks[i].name) + 2 * sizeof(char) +
> +					     sizeof(int),
> +				     GFP_KERNEL);
> +		char *parent_name =
> +			kzalloc(strlen(clks[i].parent_name) + 2 * sizeof(char) +
> +					sizeof(int),
> +				GFP_KERNEL);
> +		if (data->numa_id < 0) {
> +			sprintf(name, "%s", clks[i].name);
> +			sprintf(parent_name, "%s", clks[i].parent_name);
> +		} else {
> +			sprintf(name, "d%d_%s", data->numa_id, clks[i].name);
> +			sprintf(parent_name, "d%d_%s", data->numa_id,
> +				clks[i].parent_name);
> +		}
> +		clk = eswin_register_clk(data, NULL, name, parent_name,
> +					 clks[i].flags, &data->lock);
> +		if (IS_ERR(clk)) {
> +			pr_err("%s: failed to register clock %s\n", __func__,

Same comments apply as for all other patches.

> +			       clks[i].name);
> +			kfree(name);
> +			kfree(parent_name);
> +			goto err;
> +		}
> +
> +		if (clks[i].alias)
> +			clk_register_clkdev(clk, clks[i].alias, NULL);
> +
> +		data->clk_data.clks[clks[i].id] = clk;
> +		kfree(name);
> +		kfree(parent_name);
> +	}
> +	return 0;
> +err:
> +	while (i--)
> +		clk_unregister_gate(data->clk_data.clks[clks[i].id]);
> +
> +	return PTR_ERR(clk);
> +}
> +EXPORT_SYMBOL_GPL(eswin_clk_register_clk);

That's not a module.

> diff --git a/drivers/clk/eswin/clk.h b/drivers/clk/eswin/clk.h
> new file mode 100644
> index 000000000000..1c0d0b771229
> --- /dev/null
> +++ b/drivers/clk/eswin/clk.h
> @@ -0,0 +1,209 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
Best regards,
Krzysztof