Re: Re: [PATCH v3] i2c: ocores: use devm_ managed clks

[张网]

Hi Andrew,

I would like to express my sincere gratitude for taking the time and effort to review 
my submitted patch. I understand that reviewing can be a time-consuming process 
and I truly appreciate your dedication.

As we move forward, I would like to inquire about the first version[1] of the patch I submitted. 
As clk_disable_unprepare() has checks for error pointer and NULL already, I think there is no 
need to add the check. So both the first version of the patch and this one can work on this 
branch.

If there are any further changes or revisions needed, please do not hesitate to let me know. 
I am committed to learning and improving, and I welcome any feedback you may have. 
Thank you again for your support and guidance throughout this process.

Best regards,
Wang Zhang
---
[1] http://patchwork.ozlabs.org/project/linux-i2c/patch/20230416071854.58335-1-silver_code@xxxxxxxxxxx/

"Andrew Lunn" <andrew@xxxxxxx>写道：
> On Sat, Apr 22, 2023 at 08:32:53PM +0800, Wang Zhang wrote:
> > If any wrong occurs in ocores_i2c_of_probe, the i2c->clk needs to be
> > released. But the function returns directly in line 701 without freeing
> > the clock. Even though we can fix it by freeing the clock manually if
> > platform_get_irq_optional fails, it may not be following the best practice.
> > The original code for this driver contains if (IS_ERR()) checks
> > throughout, explicitly allowing the driver to continue loading even if
> > devm_clk_get() fails.
> > 
> > While it is not entirely clear why the original author implemented this
> > behavior, there may have been certain circumstances or issues that were not
> > apparent to us. It's possible that they were trying to work around a bug by
> > employing an unconventional solution.Using `devm_clk_get_enabled()` rather
> > than devm_clk_get() can automatically track the usage of clocks and free
> > them when they are no longer needed or an error occurs.
> > 
> > fixing it by changing `ocores_i2c_of_probe` to use
> > `devm_clk_get_optional_enabled()` rather than `devm_clk_get()`, changing
> > `goto err_clk' to direct return and removing `err_clk`.
> > 
> > Signed-off-by: Wang Zhang <silver_code@xxxxxxxxxxx>
> 
> Reviewed-by: Andrew Lunn <andrew@xxxxxxx>
> 
>     Andrew