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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 24/06/2025 12:33, dongxuyang@eswincomputing.com wrote:
> From: Xuyang Dong <dongxuyang@eswincomputing.com>
> 
> This driver depends on the CCF framework implementation.
>   Based on this driver, other modules in the SoC can use the APIs
>   provided by CCF to perform clock-related operations.
>   The driver supports eic7700 series chips.
> 
> Signed-off-by: Yifeng Huang <huangyifeng@eswincomputing.com>
> Signed-off-by: Xuyang Dong <dongxuyang@eswincomputing.com>
> ---
>  drivers/clk/Kconfig             |    1 +
>  drivers/clk/Makefile            |    1 +
>  drivers/clk/eswin/Kconfig       |   10 +
>  drivers/clk/eswin/Makefile      |    8 +
>  drivers/clk/eswin/clk-eic7700.c | 3809 +++++++++++++++++++++++++++++++
>  drivers/clk/eswin/clk-eic7700.h |  194 ++



...


> +void eswin_clk_register_pll(struct eswin_pll_clock *clks, int nums,
> +			    struct eswin_clock_data *data, struct device *dev)
> +{
> +	void __iomem *base = data->base;
> +	struct eswin_clk_pll *p_clk = NULL;
> +	struct clk *clk = NULL;
> +	struct clk_init_data init;
> +	int i;
> +	static struct gpio_desc *cpu_voltage_gpio;
> +
> +	p_clk = devm_kzalloc(dev, sizeof(*p_clk) * nums, GFP_KERNEL);
> +
> +	if (!p_clk)
> +		return;
> +	/*
> +	 *In the D2D system, the boost operation is performed using the GPIO on Die0.

What is the Linux coding style of comment?

> +	 *However, the same GPIO pin cannot be acquired twice, so special handling is implemented:
> +	 *Once the GPIO is acquired,the other driver simply uses it directly
> +	 */
> +	cpu_voltage_gpio =
> +		IS_ERR_OR_NULL(cpu_voltage_gpio) ?
> +			devm_gpiod_get(dev, "cpu-voltage", GPIOD_OUT_HIGH) :
> +			cpu_voltage_gpio;
> +	if (IS_ERR_OR_NULL(cpu_voltage_gpio)) {
> +		dev_warn(dev, "failed to get cpu volatge gpio\n");
> +		cpu_voltage_gpio = NULL;
> +	} else {
> +		/*cpu default freq is 1400M, the volatge should be VOLTAGE_0_8V*/
> +		eswin_clk_set_cpu_volatge(cpu_voltage_gpio, VOLTAGE_0_8V);

Amount of typos and unreadable stuff like missing spaces in this driver
is just discouraging and making review unnecessary difficult. Fix the
typos, fix the style. Driver is also way too big for simple clock driver
and I am surprised to see so many redundancies.

Anyway, your binding said it is not 1400M but something else so this is
a mess.


Best regards,
Krzysztof