Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Wang Zhang <silver_code@hust.edu.cn>
To:     Peter Korsgaard <peter@korsgaard.com>, Andrew Lunn <andrew@lunn.ch>
Cc:     hust-os-kernel-patches@googlegroups.com,
        Wang Zhang <silver_code@hust.edu.cn>,
        linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH v3] i2c: ocores: use devm_ managed clks
Date:   Wed, 24 May 2023 23:43:18 +0800
Message-Id: <20230524154318.2259-1-silver_code@hust.edu.cn>
In-Reply-To: <5572a733.abc0.18846f13b0b.Coremail.m202171703@hust.edu.cn>
References: <5572a733.abc0.18846f13b0b.Coremail.m202171703@hust.edu.cn>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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>
---
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 | 57 +++++++++++++--------------------
 1 file changed, 22 insertions(+), 35 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 2e575856c5cd..316d72cde3b9 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -552,16 +552,14 @@ 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 (ret) {
-			dev_err(&pdev->dev,
-				"clk_prepare_enable failed: %d\n", ret);
-			return ret;
-		}
+	if (IS_ERR(i2c->clk)) {
+		dev_err(&pdev->dev,
+			"devm_clk_get_optional_enabled failed\n");
+		return PTR_ERR(i2c->clk);
+	}
+	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 +571,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 +675,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 +706,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 +724,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 +733,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 +747,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 +760,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 +768,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