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

[董绪洋 <dongxuyang@eswincomputing.com>]

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

When higher cpu frequency is applied, the higher voltage must be
configured accordingly. So, from my perspective, it's better to
implement the clk, regulator and cpu frequency separately.
clk.c and clk-eic7700.c are responsible for setting clk only.
regulator-eic7700.c is for voltage configuration.
cpufreq-eic7700.c is for cpu frequency configuration, and it will call
the APIs of clk and regulator.
Is this the right approach?

The clk driver too big is because clk trees are defined in the .c file.

We appreciate your review of our proposed changes. Please let us know
if these modifications meet the project's standards. Should any
adjustments be needed, we'd welcome your specific feedback to help
us improve the implementation.

Best regards,
Xuyang Dong