| Message ID | 20230611225702.891856-5-andi.shyti@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: busses: Use devm_clk_get_enabled() | expand |
> - clk_unprepare(i2c->clk); > - clk_unprepare(i2c->pclk); Are you sure we can use devm_clk_get_enabled() here which calls clk_disable_unprepare() on remove and not clk_unprepare()?
On Fri, Jun 23, 2023 at 12:03:56PM +0200, Wolfram Sang wrote: > > > - clk_unprepare(i2c->clk); > > - clk_unprepare(i2c->pclk); > > Are you sure we can use devm_clk_get_enabled() here which calls > clk_disable_unprepare() on remove and not clk_unprepare()? Unless I am missing something, I think so. This is what the driver does, gets the clock and enables it. I don't know why there is just unprepare() and not disable_unprepare() in this function. I think the patch is correct, maybe Krzysztof and Alim might spot something that I have missed. Thanks, Andi
On 23/06/2023 13:48, Andi Shyti wrote: > On Fri, Jun 23, 2023 at 12:03:56PM +0200, Wolfram Sang wrote: >> >>> - clk_unprepare(i2c->clk); >>> - clk_unprepare(i2c->pclk); >> >> Are you sure we can use devm_clk_get_enabled() here which calls >> clk_disable_unprepare() on remove and not clk_unprepare()? > > Unless I am missing something, I think so. This is what the > driver does, gets the clock and enables it. > > I don't know why there is just unprepare() and not > disable_unprepare() in this function. Your code is not equivalent and does not explain why. Pure devm_clk_get_enabled() conversion should be equivalent. If original code was correct, your patch will cause double clk disable. If original code was not correct, your patch silently fixes it without explaining that there was a bug. Did you test the patch, including the unbind path? Best regards, Krzysztof
> I don't know why there is just unprepare() and not > disable_unprepare() in this function. To save power, some drivers disable the clock by default (after some init in probe) and only enable it when a transaction is on-going. If they do everything right(tm), they may only need to unprepare the clock in remove. I don't know the details about the exynos driver, though.
Hi Krzysztof, On Fri, Jun 23, 2023 at 02:18:32PM +0200, Krzysztof Kozlowski wrote: > On 23/06/2023 13:48, Andi Shyti wrote: > > On Fri, Jun 23, 2023 at 12:03:56PM +0200, Wolfram Sang wrote: > >> > >>> - clk_unprepare(i2c->clk); > >>> - clk_unprepare(i2c->pclk); > >> > >> Are you sure we can use devm_clk_get_enabled() here which calls > >> clk_disable_unprepare() on remove and not clk_unprepare()? > > > > Unless I am missing something, I think so. This is what the > > driver does, gets the clock and enables it. > > > > I don't know why there is just unprepare() and not > > disable_unprepare() in this function. > > Your code is not equivalent and does not explain why. Pure > devm_clk_get_enabled() conversion should be equivalent. > > If original code was correct, your patch will cause double clk disable. > If original code was not correct, your patch silently fixes it without > explaining that there was a bug. > > Did you test the patch, including the unbind path? you are right! My code, indeed, doesn't do a 1to1 conversion. Will prepare it again, maybe in two patches, one for the conversion and one for clearing the removal path. Thanks for dropping in, Andi
diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c index f378cd479e558..6f76d0027aeae 100644 --- a/drivers/i2c/busses/i2c-exynos5.c +++ b/drivers/i2c/busses/i2c-exynos5.c @@ -808,30 +808,20 @@ static int exynos5_i2c_probe(struct platform_device *pdev) i2c->adap.retries = 3; i2c->dev = &pdev->dev; - i2c->clk = devm_clk_get(&pdev->dev, "hsi2c"); - if (IS_ERR(i2c->clk)) { - dev_err(&pdev->dev, "cannot get clock\n"); - return -ENOENT; - } + i2c->clk = devm_clk_get_enabled(&pdev->dev, "hsi2c"); + if (IS_ERR(i2c->clk)) + return dev_err_probe(&pdev->dev, PTR_ERR(i2c->clk), + "cannot enable clock\n"); - i2c->pclk = devm_clk_get_optional(&pdev->dev, "hsi2c_pclk"); - if (IS_ERR(i2c->pclk)) { + i2c->pclk = devm_clk_get_optional_enabled(&pdev->dev, "hsi2c_pclk"); + if (IS_ERR(i2c->pclk)) return dev_err_probe(&pdev->dev, PTR_ERR(i2c->pclk), - "cannot get pclk"); - } - - ret = clk_prepare_enable(i2c->pclk); - if (ret) - return ret; - - ret = clk_prepare_enable(i2c->clk); - if (ret) - goto err_pclk; + "cannot enable pclk"); i2c->regs = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(i2c->regs)) { ret = PTR_ERR(i2c->regs); - goto err_clk; + return ret; } i2c->adap.dev.of_node = np; @@ -846,26 +836,26 @@ static int exynos5_i2c_probe(struct platform_device *pdev) i2c->irq = ret = platform_get_irq(pdev, 0); if (ret < 0) - goto err_clk; + return ret; ret = devm_request_irq(&pdev->dev, i2c->irq, exynos5_i2c_irq, IRQF_NO_SUSPEND, dev_name(&pdev->dev), i2c); if (ret != 0) { dev_err(&pdev->dev, "cannot request HS-I2C IRQ %d\n", i2c->irq); - goto err_clk; + return ret; } i2c->variant = of_device_get_match_data(&pdev->dev); ret = exynos5_hsi2c_clock_setup(i2c); if (ret) - goto err_clk; + return ret; exynos5_i2c_reset(i2c); ret = i2c_add_adapter(&i2c->adap); if (ret < 0) - goto err_clk; + return ret; platform_set_drvdata(pdev, i2c); @@ -873,13 +863,6 @@ static int exynos5_i2c_probe(struct platform_device *pdev) clk_disable(i2c->pclk); return 0; - - err_clk: - clk_disable_unprepare(i2c->clk); - - err_pclk: - clk_disable_unprepare(i2c->pclk); - return ret; } static void exynos5_i2c_remove(struct platform_device *pdev) @@ -887,9 +870,6 @@ static void exynos5_i2c_remove(struct platform_device *pdev) struct exynos5_i2c *i2c = platform_get_drvdata(pdev); i2c_del_adapter(&i2c->adap); - - clk_unprepare(i2c->clk); - clk_unprepare(i2c->pclk); } #ifdef CONFIG_PM_SLEEP
Replace the pair of functions, devm_clk_get() and clk_prepare_enable(), with a single function devm_clk_get_enabled(). Signed-off-by: Andi Shyti <andi.shyti@kernel.org> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Cc: Alim Akhtar <alim.akhtar@samsung.com> --- drivers/i2c/busses/i2c-exynos5.c | 44 +++++++++----------------------- 1 file changed, 12 insertions(+), 32 deletions(-)