| Message ID | 20230422123253.137368-1-silver_code@hust.edu.cn |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] i2c: ocores: use devm_ managed clks | expand |
ping? "Wang Zhang" <silver_code@hust.edu.cn>写道： > 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@hust.edu.cn> > --- > v2->v3: use `devm_clk_get_optional_enabled()` to manage clks > v1->v2: change `ocores_i2c_of_probe` to use `devm_clk_get_enabled()` > --- > drivers/i2c/busses/i2c-ocores.c | 56 +++++++++++++-------------------- > 1 file changed, 22 insertions(+), 34 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c > index 2e575856c5cd..0b225177fdd1 100644 > --- a/drivers/i2c/busses/i2c-ocores.c > +++ b/drivers/i2c/busses/i2c-ocores.c > @@ -552,16 +552,15 @@ static int ocores_i2c_of_probe(struct platform_device *pdev, > &clock_frequency); > i2c->bus_clock_khz = 100; > > - i2c->clk = devm_clk_get(&pdev->dev, NULL); > + i2c->clk = devm_clk_get_optional_enabled(&pdev->dev, NULL); > > - if (!IS_ERR(i2c->clk)) { > - int ret = clk_prepare_enable(i2c->clk); > + if (IS_ERR(i2c->clk)) { > + dev_err(&pdev->dev, > + "devm_clk_get_optional_enabled failed\n"); > + return PTR_ERR(i2c->clk); > + } > > - if (ret) { > - dev_err(&pdev->dev, > - "clk_prepare_enable failed: %d\n", ret); > - return ret; > - } > + if (i2c->clk) { > i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000; > if (clock_frequency_present) > i2c->bus_clock_khz = clock_frequency / 1000; > @@ -573,7 +572,6 @@ static int ocores_i2c_of_probe(struct platform_device *pdev, > if (!clock_frequency_present) { > dev_err(&pdev->dev, > "Missing required parameter 'opencores,ip-clock-frequency'\n"); > - clk_disable_unprepare(i2c->clk); > return -ENODEV; > } > i2c->ip_clock_khz = clock_frequency / 1000; > @@ -678,8 +676,7 @@ static int ocores_i2c_probe(struct platform_device *pdev) > default: > dev_err(&pdev->dev, "Unsupported I/O width (%d)\n", > i2c->reg_io_width); > - ret = -EINVAL; > - goto err_clk; > + return -EINVAL; > } > } > > @@ -710,13 +707,13 @@ static int ocores_i2c_probe(struct platform_device *pdev) > pdev->name, i2c); > if (ret) { > dev_err(&pdev->dev, "Cannot claim IRQ\n"); > - goto err_clk; > + return ret; > } > } > > ret = ocores_init(&pdev->dev, i2c); > if (ret) > - goto err_clk; > + return ret; > > /* hook up driver to tree */ > platform_set_drvdata(pdev, i2c); > @@ -728,7 +725,7 @@ static int ocores_i2c_probe(struct platform_device *pdev) > /* add i2c adapter to i2c tree */ > ret = i2c_add_adapter(&i2c->adap); > if (ret) > - goto err_clk; > + return ret; > > /* add in known devices to the bus */ > if (pdata) { > @@ -737,10 +734,6 @@ static int ocores_i2c_probe(struct platform_device *pdev) > } > > return 0; > - > -err_clk: > - clk_disable_unprepare(i2c->clk); > - return ret; > } > > static int ocores_i2c_remove(struct platform_device *pdev) > @@ -755,9 +748,6 @@ static int ocores_i2c_remove(struct platform_device *pdev) > /* remove adapter & data */ > i2c_del_adapter(&i2c->adap); > > - if (!IS_ERR(i2c->clk)) > - clk_disable_unprepare(i2c->clk); > - > return 0; > } > > @@ -771,8 +761,7 @@ static int ocores_i2c_suspend(struct device *dev) > ctrl &= ~(OCI2C_CTRL_EN | OCI2C_CTRL_IEN); > oc_setreg(i2c, OCI2C_CONTROL, ctrl); > > - if (!IS_ERR(i2c->clk)) > - clk_disable_unprepare(i2c->clk); > + clk_disable_unprepare(i2c->clk); > return 0; > } > > @@ -780,19 +769,18 @@ static int ocores_i2c_resume(struct device *dev) > { > struct ocores_i2c *i2c = dev_get_drvdata(dev); > > - if (!IS_ERR(i2c->clk)) { > - unsigned long rate; > - int ret = clk_prepare_enable(i2c->clk); > + unsigned long rate; > + int ret = clk_prepare_enable(i2c->clk); > > - if (ret) { > - dev_err(dev, > - "clk_prepare_enable failed: %d\n", ret); > - return ret; > - } > - rate = clk_get_rate(i2c->clk) / 1000; > - if (rate) > - i2c->ip_clock_khz = rate; > + if (ret) { > + dev_err(dev, > + "clk_prepare_enable failed: %d\n", ret); > + return ret; > } > + rate = clk_get_rate(i2c->clk) / 1000; > + if (rate) > + i2c->ip_clock_khz = rate; > + > return ocores_init(dev, i2c); > } > > -- > 2.34.1
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@hust.edu.cn> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
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@hust.edu.cn/ "Andrew Lunn" <andrew@lunn.ch>写道： > 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@hust.edu.cn> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > Andrew
On Tue, Apr 25, 2023 at 10:47:29PM +0800, 张网 wrote: > 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@hust.edu.cn/ So this patch is about the IRQ being an error code, and doing the correct cleanup. With the change to devm_clk_get_ there is no need to disabled the clock, it will be done automatically. This means there is no cleanup the driver needs to do itself. So the patch is no longer needed. Andrew
> -----原始邮件----- > 发件人: "Andrew Lunn" <andrew@lunn.ch> > 发送时间: 2023-04-25 19:57:26 (星期二) > 收件人: "Wang Zhang" <silver_code@hust.edu.cn> > 抄送: "Peter Korsgaard" <peter@korsgaard.com>, hust-os-kernel-patches@googlegroups.com, linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org > 主题: Re: [PATCH v3] i2c: ocores: use devm_ managed clks > > 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@hust.edu.cn> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > Andrew Hi Andrew, I'm checking in about my patch submission for i2c ocores that was "review'ed" on 4/25, but its status has not been updated yet. I would greatly appreciate it if you could provide me with an update on the status of my submission. Is there any additional information or documentation that I can provide to help expedite the process? Thank you very much for your time and attention. I look forward to hearing from you soon. Regards, Wang Zhang
> Hi Andrew, > I'm checking in about my patch submission for i2c ocores that was > "review'ed" on 4/25, but its status has not been updated yet. > I would greatly appreciate it if you could provide me with an > update on the status of my submission. Is there any additional > information or documentation that I can provide to help expedite > the process? I think your patch was submitted during the merge window. This is the point in time when subsystems give Linus patches for the next release. During those two weeks new patches are not accepted. https://www.kernel.org/doc/html/latest/process/2.Process.html Now that the merge window is closed, please rebase your patch on git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next and resubmit. Include my Reviewed-by tag. Andrew
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c index 2e575856c5cd..0b225177fdd1 100644 --- a/drivers/i2c/busses/i2c-ocores.c +++ b/drivers/i2c/busses/i2c-ocores.c @@ -552,16 +552,15 @@ static int ocores_i2c_of_probe(struct platform_device *pdev, &clock_frequency); i2c->bus_clock_khz = 100; - i2c->clk = devm_clk_get(&pdev->dev, NULL); + i2c->clk = devm_clk_get_optional_enabled(&pdev->dev, NULL); - if (!IS_ERR(i2c->clk)) { - int ret = clk_prepare_enable(i2c->clk); + if (IS_ERR(i2c->clk)) { + dev_err(&pdev->dev, + "devm_clk_get_optional_enabled failed\n"); + return PTR_ERR(i2c->clk); + } - if (ret) { - dev_err(&pdev->dev, - "clk_prepare_enable failed: %d\n", ret); - return ret; - } + if (i2c->clk) { i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000; if (clock_frequency_present) i2c->bus_clock_khz = clock_frequency / 1000; @@ -573,7 +572,6 @@ static int ocores_i2c_of_probe(struct platform_device *pdev, if (!clock_frequency_present) { dev_err(&pdev->dev, "Missing required parameter 'opencores,ip-clock-frequency'\n"); - clk_disable_unprepare(i2c->clk); return -ENODEV; } i2c->ip_clock_khz = clock_frequency / 1000; @@ -678,8 +676,7 @@ static int ocores_i2c_probe(struct platform_device *pdev) default: dev_err(&pdev->dev, "Unsupported I/O width (%d)\n", i2c->reg_io_width); - ret = -EINVAL; - goto err_clk; + return -EINVAL; } } @@ -710,13 +707,13 @@ static int ocores_i2c_probe(struct platform_device *pdev) pdev->name, i2c); if (ret) { dev_err(&pdev->dev, "Cannot claim IRQ\n"); - goto err_clk; + return ret; } } ret = ocores_init(&pdev->dev, i2c); if (ret) - goto err_clk; + return ret; /* hook up driver to tree */ platform_set_drvdata(pdev, i2c); @@ -728,7 +725,7 @@ static int ocores_i2c_probe(struct platform_device *pdev) /* add i2c adapter to i2c tree */ ret = i2c_add_adapter(&i2c->adap); if (ret) - goto err_clk; + return ret; /* add in known devices to the bus */ if (pdata) { @@ -737,10 +734,6 @@ static int ocores_i2c_probe(struct platform_device *pdev) } return 0; - -err_clk: - clk_disable_unprepare(i2c->clk); - return ret; } static int ocores_i2c_remove(struct platform_device *pdev) @@ -755,9 +748,6 @@ static int ocores_i2c_remove(struct platform_device *pdev) /* remove adapter & data */ i2c_del_adapter(&i2c->adap); - if (!IS_ERR(i2c->clk)) - clk_disable_unprepare(i2c->clk); - return 0; } @@ -771,8 +761,7 @@ static int ocores_i2c_suspend(struct device *dev) ctrl &= ~(OCI2C_CTRL_EN | OCI2C_CTRL_IEN); oc_setreg(i2c, OCI2C_CONTROL, ctrl); - if (!IS_ERR(i2c->clk)) - clk_disable_unprepare(i2c->clk); + clk_disable_unprepare(i2c->clk); return 0; } @@ -780,19 +769,18 @@ static int ocores_i2c_resume(struct device *dev) { struct ocores_i2c *i2c = dev_get_drvdata(dev); - if (!IS_ERR(i2c->clk)) { - unsigned long rate; - int ret = clk_prepare_enable(i2c->clk); + unsigned long rate; + int ret = clk_prepare_enable(i2c->clk); - if (ret) { - dev_err(dev, - "clk_prepare_enable failed: %d\n", ret); - return ret; - } - rate = clk_get_rate(i2c->clk) / 1000; - if (rate) - i2c->ip_clock_khz = rate; + if (ret) { + dev_err(dev, + "clk_prepare_enable failed: %d\n", ret); + return ret; } + rate = clk_get_rate(i2c->clk) / 1000; + if (rate) + i2c->ip_clock_khz = rate; + return ocores_init(dev, i2c); }
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@hust.edu.cn> --- v2->v3: use `devm_clk_get_optional_enabled()` to manage clks v1->v2: change `ocores_i2c_of_probe` to use `devm_clk_get_enabled()` --- drivers/i2c/busses/i2c-ocores.c | 56 +++++++++++++-------------------- 1 file changed, 22 insertions(+), 34 deletions(-)