[PATCHv2] ata: sata_mv: use devm clock helpers

[PATCHv2] ata: sata_mv: use devm clock helpers

[Rosen Penev]

Replace clk_get/clk_prepare_enable/clk_put with
devm_clk_get_optional_enabled for both the main clock and
per-port clocks. This eliminates the need for manual clock
cleanup in probe error and remove paths.

The err label is retained for phy_power_off cleanup, since
devm_phy_optional_get does not manage phy power state.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: sashiko: use goto instead of return.
 drivers/ata/sata_mv.c | 46 ++++++++++++-------------------------------
 1 file changed, 13 insertions(+), 33 deletions(-)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 145ba6880c82..d260f04e6384 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -4088,21 +4088,19 @@ static int mv_platform_probe(struct platform_device *pdev)
 
 	hpriv->base -= SATAHC0_REG_BASE;
 
-	hpriv->clk = clk_get(&pdev->dev, NULL);
-	if (IS_ERR(hpriv->clk)) {
-		dev_notice(&pdev->dev, "cannot get optional clkdev\n");
-	} else {
-		rc = clk_prepare_enable(hpriv->clk);
-		if (rc)
-			goto err;
-	}
+	hpriv->clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
+	if (IS_ERR(hpriv->clk))
+		return PTR_ERR(hpriv->clk);
 
 	for (port = 0; port < n_ports; port++) {
 		char port_number[16];
 		sprintf(port_number, "%d", port);
-		hpriv->port_clks[port] = clk_get(&pdev->dev, port_number);
-		if (!IS_ERR(hpriv->port_clks[port]))
-			clk_prepare_enable(hpriv->port_clks[port]);
+		hpriv->port_clks[port] = devm_clk_get_optional_enabled(&pdev->dev, port_number);
+		if (IS_ERR(hpriv->port_clks[port])) {
+			rc = PTR_ERR(hpriv->port_clks[port]);
+			hpriv->n_ports = port;
+			goto err;
+		}
 
 		sprintf(port_number, "port%d", port);
 		hpriv->port_phys[port] = devm_phy_optional_get(&pdev->dev,
@@ -4116,8 +4114,8 @@ static int mv_platform_probe(struct platform_device *pdev)
 			/* Cleanup only the initialized ports */
 			hpriv->n_ports = port;
 			goto err;
-		} else
-			phy_power_on(hpriv->port_phys[port]);
+		}
+		phy_power_on(hpriv->port_phys[port]);
 	}
 
 	/* All the ports have been initialized */
@@ -4156,17 +4154,8 @@ static int mv_platform_probe(struct platform_device *pdev)
 		return 0;
 
 err:
-	if (!IS_ERR(hpriv->clk)) {
-		clk_disable_unprepare(hpriv->clk);
-		clk_put(hpriv->clk);
-	}
-	for (port = 0; port < hpriv->n_ports; port++) {
-		if (!IS_ERR(hpriv->port_clks[port])) {
-			clk_disable_unprepare(hpriv->port_clks[port]);
-			clk_put(hpriv->port_clks[port]);
-		}
+	for (port = 0; port < hpriv->n_ports; port++)
 		phy_power_off(hpriv->port_phys[port]);
-	}
 
 	return rc;
 }
@@ -4186,17 +4175,8 @@ static void mv_platform_remove(struct platform_device *pdev)
 	int port;
 	ata_host_detach(host);
 
-	if (!IS_ERR(hpriv->clk)) {
-		clk_disable_unprepare(hpriv->clk);
-		clk_put(hpriv->clk);
-	}
-	for (port = 0; port < host->n_ports; port++) {
-		if (!IS_ERR(hpriv->port_clks[port])) {
-			clk_disable_unprepare(hpriv->port_clks[port]);
-			clk_put(hpriv->port_clks[port]);
-		}
+	for (port = 0; port < host->n_ports; port++)
 		phy_power_off(hpriv->port_phys[port]);
-	}
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.55.0

Re: [PATCHv2] ata: sata_mv: use devm clock helpers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The return value of `phy_power_on()` is ignored, which can lead to hardware faults and unbalanced `phy_power_off()` execution during teardown.
--

--- Patch [1]: [PATCHv2] ata: sata_mv: use devm clock helpers ---
commit 4da9c2641dc6134cc0fc1f75c23bcc95ae5882fd
Author: Rosen Penev <rosenp@gmail.com>

ata: sata_mv: use devm clock helpers

This commit replaces manual clock acquisition and enablement with
devm_clk_get_optional_enabled() for the main and per-port clocks. It
simplifies the error handling and remove paths by removing the manual clock
cleanup.

> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 41647a56a9f4e..eafd1e6b827dd 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c

[ ... ]

> @@ -4120,8 +4118,8 @@ static int mv_platform_probe(struct platform_device *pdev)
>  			/* Cleanup only the initialized ports */
>  			hpriv->n_ports = port;
>  			goto err;
> -		} else
> -			phy_power_on(hpriv->port_phys[port]);
> +		}
> +		phy_power_on(hpriv->port_phys[port]);

[Severity: High]
This is a pre-existing issue, but is it safe to ignore the return value of
phy_power_on() here?

If powering on the PHY fails, the SATA controller might still attempt to
initialize and use the unpowered port. Could this result in hardware
lockups or register access faults?

[ ... ]

> @@ -4190,17 +4179,8 @@ static void mv_platform_remove(struct platform_device *pdev)
>  	int port;
>  	ata_host_detach(host);
>  
> -	if (!IS_ERR(hpriv->clk)) {
> -		clk_disable_unprepare(hpriv->clk);
> -		clk_put(hpriv->clk);
> -	}
> -	for (port = 0; port < host->n_ports; port++) {
> -		if (!IS_ERR(hpriv->port_clks[port])) {
> -			clk_disable_unprepare(hpriv->port_clks[port]);
> -			clk_put(hpriv->port_clks[port]);
> -		}
> +	for (port = 0; port < host->n_ports; port++)
>  		phy_power_off(hpriv->port_phys[port]);

[Severity: High]
This is a pre-existing issue, but does this code unconditionally power off
ports even if phy_power_on() failed during mv_platform_probe()?

If the earlier power on operation failed and was ignored, calling
phy_power_off() here could decrement the PHY's power count and runtime PM
usage count below zero, potentially causing unbalanced lifecycle operations
in the PM framework.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712223733.1084077-1-rosenp@gmail.com?part=1