[PATCH v3 0/4] ata: libahci: Allow using a regulator for each port

[PATCH v3 0/4] ata: libahci: Allow using a regulator for each port

[Gregory CLEMENT]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 2038 bytes --]

Hi,

The current implementation of the libahci allows using one PHY per
port but we still have one single regulator for the whole
controller. This series adds the support of multiple regulators.

This is the third version of the series.

I use now the same binding introduced in the first version. This one
was acked in the first place by Hans and was discussed during the
review of the second version.

To be able to use it, I made the port the platform devices when the
device tree was used. An other change was to manage the deferred
case. The regulator were now attached to a different device that the
SATA host, so the devres_release_group don't manage them, and they
have to be released in a separate call.

Thanks,

Grégory

Changelog:
 v2 -> v3:
- put back the regulator inside the sub-node ports
- made the ports platform devices when the device tree is used
- released the regulator in case of error in the probe function

 v1 -> v2:
- Kept the case when no child node are present under the ahci node
- Fix the test done under the label disable_target_pwrs
- No more use an of_ version of the regulator framework and instead
  associate each regulator of a port with an unique name.
- Added the acked-by on the clean-up patch

Gregory CLEMENT (4):
  ata: libahci: Clean-up the ahci_platform_en/disable_phys functions
  Documentation: bindings: Add the regulator property to the sub-nodes
    AHCI bindings
  ata: libahci: Allow using multiple regulators
  ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

 .../devicetree/bindings/ata/ahci-platform.txt      |   9 +-
 arch/arm/boot/dts/armada-388-gp.dts                | 126 +++++++++++
 drivers/ata/ahci.h                                 |   2 +-
 drivers/ata/ahci_imx.c                             |  14 +-
 drivers/ata/libahci_platform.c                     | 233 ++++++++++++++-------
 include/linux/ahci_platform.h                      |   2 +
 6 files changed, 302 insertions(+), 84 deletions(-)

-- 
1.9.1

[PATCH v3 1/4] ata: libahci: Clean-up the ahci_platform_en/disable_phys functions

[Gregory CLEMENT]

The phy_ functions handle the NULL pointer case, so there is no need
to skip them if there is a NULL pointer. Moreover, after the error
label there is already no check on the pointer. This patch removes the
unnecessary tests and brings some consistency.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/ata/libahci_platform.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 0b03f9056692..a147aaadca85 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -54,9 +54,6 @@ static int ahci_platform_enable_phys(struct ahci_host_priv *hpriv)
 	int rc, i;
 
 	for (i = 0; i < hpriv->nports; i++) {
-		if (!hpriv->phys[i])
-			continue;
-
 		rc = phy_init(hpriv->phys[i]);
 		if (rc)
 			goto disable_phys;
@@ -89,9 +86,6 @@ static void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
 	int i;
 
 	for (i = 0; i < hpriv->nports; i++) {
-		if (!hpriv->phys[i])
-			continue;
-
 		phy_power_off(hpriv->phys[i]);
 		phy_exit(hpriv->phys[i]);
 	}
-- 
1.9.1

[PATCH v3 2/4] Documentation: bindings: Add the regulator property to the sub-nodes AHCI bindings

[Gregory CLEMENT]

It is now possible to use a regulator property for each port of the
AHCI controller.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 Documentation/devicetree/bindings/ata/ahci-platform.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index 4ab09f2202d4..c2340eeeb97f 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -37,9 +37,10 @@ Required properties when using sub-nodes:
 
 
 Sub-nodes required properties:
-- reg               : the port number
-- phys              : reference to the SATA PHY node
-
+- reg		    : the port number
+And at least one of the following properties:
+- phys		    : reference to the SATA PHY node
+- target-supply    : regulator for SATA target power
 
 Examples:
         sata@ffe08000 {
@@ -68,10 +69,12 @@ With sub-nodes:
 		sata0: sata-port@0 {
 			reg = <0>;
 			phys = <&sata_phy 0>;
+			target-supply = <&reg_sata0>;
 		};
 
 		sata1: sata-port@1 {
 			reg = <1>;
 			phys = <&sata_phy 1>;
+			target-supply = <&reg_sata1>;;
 		};
 	};
-- 
1.9.1

[PATCH v3 3/4] ata: libahci: Allow using multiple regulators

[Gregory CLEMENT]

The current implementation of the libahci allows using multiple PHYs
but not multiple regulators. This patch adds the support of multiple
regulators. Until now it was mandatory to have a PHY under a subnode,
now a port subnode can contain either a regulator or a PHY (or both).

In order to be able to asociate a port with a regulator the port are
now a platform device in the device tree case.

There was only one driver which used directly the regulator field of
the ahci_host_priv structure. To preserve the bisectability the change
in the ahci_imx driver was done in the same patch.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/ata/ahci.h             |   2 +-
 drivers/ata/ahci_imx.c         |  14 +--
 drivers/ata/libahci_platform.c | 227 +++++++++++++++++++++++++++++------------
 include/linux/ahci_platform.h  |   2 +
 4 files changed, 170 insertions(+), 75 deletions(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 40f0e34f17af..275358ae0b3f 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -333,7 +333,7 @@ struct ahci_host_priv {
 	u32			em_msg_type;	/* EM message type */
 	bool			got_runtime_pm; /* Did we do pm_runtime_get? */
 	struct clk		*clks[AHCI_MAX_CLKS]; /* Optional */
-	struct regulator	*target_pwr;	/* Optional */
+	struct regulator	**target_pwrs;	/* Optional */
 	/*
 	 * If platform uses PHYs. There is a 1:1 relation between the port number and
 	 * the PHY position in this array.
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index 35d51c59a370..41632e57d46f 100644
--- a/drivers/ata/ahci_imx.c
+++ b/drivers/ata/ahci_imx.c
@@ -221,11 +221,9 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv)
 	if (imxpriv->no_device)
 		return 0;
 
-	if (hpriv->target_pwr) {
-		ret = regulator_enable(hpriv->target_pwr);
-		if (ret)
-			return ret;
-	}
+	ret = ahci_platform_enable_regulators(hpriv);
+	if (ret)
+		return ret;
 
 	ret = clk_prepare_enable(imxpriv->sata_ref_clk);
 	if (ret < 0)
@@ -270,8 +268,7 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv)
 disable_clk:
 	clk_disable_unprepare(imxpriv->sata_ref_clk);
 disable_regulator:
-	if (hpriv->target_pwr)
-		regulator_disable(hpriv->target_pwr);
+	ahci_platform_disable_regulators(hpriv);
 
 	return ret;
 }
@@ -291,8 +288,7 @@ static void imx_sata_disable(struct ahci_host_priv *hpriv)
 
 	clk_disable_unprepare(imxpriv->sata_ref_clk);
 
-	if (hpriv->target_pwr)
-		regulator_disable(hpriv->target_pwr);
+	ahci_platform_disable_regulators(hpriv);
 }
 
 static void ahci_imx_error_handler(struct ata_port *ap)
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index a147aaadca85..1db968ef6ff8 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -24,6 +24,7 @@
 #include <linux/ahci_platform.h>
 #include <linux/phy/phy.h>
 #include <linux/pm_runtime.h>
+#include <linux/of_platform.h>
 #include "ahci.h"
 
 static void ahci_host_stop(struct ata_host *host);
@@ -138,6 +139,59 @@ void ahci_platform_disable_clks(struct ahci_host_priv *hpriv)
 EXPORT_SYMBOL_GPL(ahci_platform_disable_clks);
 
 /**
+ * ahci_platform_enable_regulators - Enable regulators
+ * @hpriv: host private area to store config values
+ *
+ * This function enables all the regulators found in
+ * hpriv->target_pwrs, if any.  If a regulator fails to be enabled, it
+ * disables all the regulators already enabled in reverse order and
+ * returns an error.
+ *
+ * RETURNS:
+ * 0 on success otherwise a negative error code
+ */
+int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv)
+{
+	int rc, i;
+
+	for (i = 0; i < hpriv->nports; i++) {
+		if (!hpriv->target_pwrs[i])
+			continue;
+
+		rc = regulator_enable(hpriv->target_pwrs[i]);
+		if (rc)
+			goto disable_target_pwrs;
+	}
+
+	return 0;
+
+disable_target_pwrs:
+	while (--i >= 0)
+		if (hpriv->target_pwrs[i])
+			regulator_disable(hpriv->target_pwrs[i]);
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(ahci_platform_enable_regulators);
+
+/**
+ * ahci_platform_disable_regulators - Disable regulators
+ * @hpriv: host private area to store config values
+ *
+ * This function disables all regulators found in hpriv->target_pwrs.
+ */
+void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv)
+{
+	int i;
+
+	for (i = 0; i < hpriv->nports; i++) {
+		if (!hpriv->target_pwrs[i])
+			continue;
+		regulator_disable(hpriv->target_pwrs[i]);
+	}
+}
+EXPORT_SYMBOL_GPL(ahci_platform_disable_regulators);
+/**
  * ahci_platform_enable_resources - Enable platform resources
  * @hpriv: host private area to store config values
  *
@@ -157,11 +211,9 @@ int ahci_platform_enable_resources(struct ahci_host_priv *hpriv)
 {
 	int rc;
 
-	if (hpriv->target_pwr) {
-		rc = regulator_enable(hpriv->target_pwr);
-		if (rc)
-			return rc;
-	}
+	rc = ahci_platform_enable_regulators(hpriv);
+	if (rc)
+		return rc;
 
 	rc = ahci_platform_enable_clks(hpriv);
 	if (rc)
@@ -177,8 +229,8 @@ disable_clks:
 	ahci_platform_disable_clks(hpriv);
 
 disable_regulator:
-	if (hpriv->target_pwr)
-		regulator_disable(hpriv->target_pwr);
+	ahci_platform_disable_regulators(hpriv);
+
 	return rc;
 }
 EXPORT_SYMBOL_GPL(ahci_platform_enable_resources);
@@ -199,8 +251,7 @@ void ahci_platform_disable_resources(struct ahci_host_priv *hpriv)
 
 	ahci_platform_disable_clks(hpriv);
 
-	if (hpriv->target_pwr)
-		regulator_disable(hpriv->target_pwr);
+	ahci_platform_disable_regulators(hpriv);
 }
 EXPORT_SYMBOL_GPL(ahci_platform_disable_resources);
 
@@ -218,6 +269,59 @@ static void ahci_platform_put_resources(struct device *dev, void *res)
 		clk_put(hpriv->clks[c]);
 }
 
+static int ahci_platform_get_phy(struct ahci_host_priv *hpriv, u32 port,
+				struct device *dev, struct device_node *node)
+{
+	int rc;
+
+	hpriv->phys[port] = devm_of_phy_get(dev, node, NULL);
+
+	if (!IS_ERR(hpriv->phys[port]))
+		return 0;
+
+	rc = PTR_ERR(hpriv->phys[port]);
+	switch (rc) {
+	case -ENOSYS:
+		/* No PHY support. Check if PHY is required. */
+		if (of_find_property(node, "phys", NULL)) {
+			dev_err(dev,
+				"couldn't get PHY in node %s: ENOSYS\n",
+				node->name);
+			break;
+		}
+	case -ENODEV:
+		/* continue normally */
+		hpriv->phys[port] = NULL;
+		rc = 0;
+		break;
+
+	default:
+		dev_err(dev,
+			"couldn't get PHY in node %s: %d\n",
+			node->name, rc);
+
+		break;
+	}
+
+	return rc;
+}
+
+static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
+				struct device *dev)
+{
+	struct regulator *target_pwr;
+	int rc = 0;
+
+	target_pwr = devm_regulator_get_optional(dev, "target");
+
+	if (!IS_ERR(target_pwr))
+		hpriv->target_pwrs[port] = target_pwr;
+	else
+		rc = PTR_ERR(target_pwr);
+
+	return rc;
+}
+
 /**
  * ahci_platform_get_resources - Get platform resources
  * @pdev: platform device to get resources for
@@ -240,7 +344,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 	struct ahci_host_priv *hpriv;
 	struct clk *clk;
 	struct device_node *child;
-	int i, enabled_ports = 0, rc = -ENOMEM;
+	int i, sz, enabled_ports = 0, rc = -ENOMEM, child_nodes;
 	u32 mask_port_map = 0;
 
 	if (!devres_open_group(dev, NULL, GFP_KERNEL))
@@ -261,14 +365,6 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 		goto err_out;
 	}
 
-	hpriv->target_pwr = devm_regulator_get_optional(dev, "target");
-	if (IS_ERR(hpriv->target_pwr)) {
-		rc = PTR_ERR(hpriv->target_pwr);
-		if (rc == -EPROBE_DEFER)
-			goto err_out;
-		hpriv->target_pwr = NULL;
-	}
-
 	for (i = 0; i < AHCI_MAX_CLKS; i++) {
 		/*
 		 * For now we must use clk_get(dev, NULL) for the first clock,
@@ -290,19 +386,33 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 		hpriv->clks[i] = clk;
 	}
 
-	hpriv->nports = of_get_child_count(dev->of_node);
+	hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
 
-	if (hpriv->nports) {
-		hpriv->phys = devm_kzalloc(dev,
-					   hpriv->nports * sizeof(*hpriv->phys),
-					   GFP_KERNEL);
-		if (!hpriv->phys) {
-			rc = -ENOMEM;
-			goto err_out;
-		}
+	/*
+	 * If no sub-node was found, we still need to set nports to
+	 * one in order to be able to use the
+	 * ahci_platform_[en|dis]able_[phys|regulators] functions.
+	 */
+	if (!child_nodes)
+		hpriv->nports = 1;
+
+	sz = hpriv->nports * sizeof(*hpriv->phys);
+	hpriv->phys = devm_kzalloc(dev, sz, GFP_KERNEL);
+	if (!hpriv->phys) {
+		rc = -ENOMEM;
+		goto err_out;
+	}
+	sz = hpriv->nports * sizeof(*hpriv->target_pwrs);
+	hpriv->target_pwrs = devm_kzalloc(dev, sz, GFP_KERNEL);
+	if (!hpriv->target_pwrs) {
+		rc = -ENOMEM;
+		goto err_out;
+	}
 
+	if (child_nodes) {
 		for_each_child_of_node(dev->of_node, child) {
 			u32 port;
+			struct platform_device *port_dev;
 
 			if (!of_device_is_available(child))
 				continue;
@@ -316,17 +426,20 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 				dev_warn(dev, "invalid port number %d\n", port);
 				continue;
 			}
-
 			mask_port_map |= BIT(port);
 
-			hpriv->phys[port] = devm_of_phy_get(dev, child, NULL);
-			if (IS_ERR(hpriv->phys[port])) {
-				rc = PTR_ERR(hpriv->phys[port]);
-				dev_err(dev,
-					"couldn't get PHY in node %s: %d\n",
-					child->name, rc);
+			of_platform_device_create(child, NULL, NULL);
+
+			port_dev = of_find_device_by_node(child);
+
+			rc = ahci_platform_get_regulator(hpriv, port,
+							&port_dev->dev);
+			if (rc == -EPROBE_DEFER)
+				goto err_out;
+
+			rc = ahci_platform_get_phy(hpriv, port, dev, child);
+			if (rc)
 				goto err_out;
-			}
 
 			enabled_ports++;
 		}
@@ -343,38 +456,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 		 * If no sub-node was found, keep this for device tree
 		 * compatibility
 		 */
-		struct phy *phy = devm_phy_get(dev, "sata-phy");
-		if (!IS_ERR(phy)) {
-			hpriv->phys = devm_kzalloc(dev, sizeof(*hpriv->phys),
-						   GFP_KERNEL);
-			if (!hpriv->phys) {
-				rc = -ENOMEM;
-				goto err_out;
-			}
-
-			hpriv->phys[0] = phy;
-			hpriv->nports = 1;
-		} else {
-			rc = PTR_ERR(phy);
-			switch (rc) {
-				case -ENOSYS:
-					/* No PHY support. Check if PHY is required. */
-					if (of_find_property(dev->of_node, "phys", NULL)) {
-						dev_err(dev, "couldn't get sata-phy: ENOSYS\n");
-						goto err_out;
-					}
-				case -ENODEV:
-					/* continue normally */
-					hpriv->phys = NULL;
-					break;
-
-				default:
-					goto err_out;
+		rc = ahci_platform_get_phy(hpriv, 0, dev, dev->of_node);
+		if (rc)
+			goto err_out;
 
-			}
-		}
+		rc = ahci_platform_get_regulator(hpriv, 0, dev);
+		if (rc == -EPROBE_DEFER)
+			goto err_out;
 	}
-
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 	hpriv->got_runtime_pm = true;
@@ -383,6 +472,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 	return hpriv;
 
 err_out:
+	/*
+	 * In case of a deferred probe, previous regulators may have
+	 * been already get, so put them to be able to get them again
+	 * in the next probe.
+	 */
+	for (i = 0; i < hpriv->nports; i++)
+		if (hpriv->target_pwrs[i])
+			devm_regulator_put(hpriv->target_pwrs[i]);
 	devres_release_group(dev, NULL);
 	return ERR_PTR(rc);
 }
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 642d6ae4030c..f65b33809170 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -24,6 +24,8 @@ struct platform_device;
 
 int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
 void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
+int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv);
+void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv);
 int ahci_platform_enable_resources(struct ahci_host_priv *hpriv);
 void ahci_platform_disable_resources(struct ahci_host_priv *hpriv);
 struct ahci_host_priv *ahci_platform_get_resources(
-- 
1.9.1

[PATCH v3 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Gregory CLEMENT]

Add the regulators to each SATA port.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/boot/dts/armada-388-gp.dts | 126 ++++++++++++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)

diff --git a/arch/arm/boot/dts/armada-388-gp.dts b/arch/arm/boot/dts/armada-388-gp.dts
index 4df22bf91683..590b383db323 100644
--- a/arch/arm/boot/dts/armada-388-gp.dts
+++ b/arch/arm/boot/dts/armada-388-gp.dts
@@ -173,6 +173,16 @@
 				status = "okay";
 				#address-cells = <1>;
 				#size-cells = <0>;
+
+				sata0: sata-port@0 {
+					reg = <0>;
+					target-supply = <&reg_5v_sata0>;
+				};
+
+				sata1: sata-port@1 {
+					reg = <1>;
+					target-supply = <&reg_5v_sata1>;
+				};
 			};
 
 			sata@e0000 {
@@ -181,6 +191,16 @@
 				status = "okay";
 				#address-cells = <1>;
 				#size-cells = <0>;
+
+				sata2: sata-port@0 {
+					reg = <0>;
+					target-supply = <&reg_5v_sata2>;
+				};
+
+				sata3: sata-port@1 {
+					reg = <1>;
+					target-supply = <&reg_5v_sata3>;
+				};
 			};
 
 			sdhci@d8000 {
@@ -278,6 +298,112 @@
 		regulator-always-on;
 		gpio = <&expander0 4 GPIO_ACTIVE_HIGH>;
 	};
+
+	reg_sata0: pwr-sata0 {
+		compatible = "regulator-fixed";
+		regulator-name = "pwr_en_sata0";
+		enable-active-high;
+		regulator-always-on;
+
+	};
+
+	reg_5v_sata0: v5-sata0 {
+		compatible = "regulator-fixed";
+		regulator-name = "v5.0-sata0";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata0>;
+	};
+
+	reg_12v_sata0: v12-sata0 {
+		compatible = "regulator-fixed";
+		regulator-name = "v12.0-sata0";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata0>;
+	};
+
+	reg_sata1: pwr-sata1 {
+		regulator-name = "pwr_en_sata1";
+		compatible = "regulator-fixed";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		enable-active-high;
+		regulator-always-on;
+		gpio = <&expander0 3 GPIO_ACTIVE_HIGH>;
+	};
+
+	reg_5v_sata1: v5-sata1 {
+		compatible = "regulator-fixed";
+		regulator-name = "v5.0-sata1";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata1>;
+	};
+
+	reg_12v_sata1: v12-sata1 {
+		compatible = "regulator-fixed";
+		regulator-name = "v12.0-sata1";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata1>;
+	};
+
+	reg_sata2: pwr-sata2 {
+		compatible = "regulator-fixed";
+		regulator-name = "pwr_en_sata2";
+		enable-active-high;
+		regulator-always-on;
+		gpio = <&expander0 11 GPIO_ACTIVE_HIGH>;
+	};
+
+	reg_5v_sata2: v5-sata2 {
+		compatible = "regulator-fixed";
+		regulator-name = "v5.0-sata2";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata2>;
+	};
+
+	reg_12v_sata2: v12-sata2 {
+		compatible = "regulator-fixed";
+		regulator-name = "v12.0-sata2";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata2>;
+	};
+
+	reg_sata3: pwr-sata3 {
+		compatible = "regulator-fixed";
+		regulator-name = "pwr_en_sata3";
+		enable-active-high;
+		regulator-always-on;
+		gpio = <&expander0 12 GPIO_ACTIVE_HIGH>;
+	};
+
+	reg_5v_sata3: v5-sata3 {
+		compatible = "regulator-fixed";
+		regulator-name = "v5.0-sata3";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata3>;
+	};
+
+	reg_12v_sata3: v12-sata3 {
+		compatible = "regulator-fixed";
+		regulator-name = "v12.0-sata3";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata3>;
+	};
 };
 
 &pinctrl {
-- 
1.9.1

Re: [PATCH v3 3/4] ata: libahci: Allow using multiple regulators

[Hans de Goede]

Hi,

On 13-01-15 15:22, Gregory CLEMENT wrote:
> The current implementation of the libahci allows using multiple PHYs
> but not multiple regulators. This patch adds the support of multiple
> regulators. Until now it was mandatory to have a PHY under a subnode,
> now a port subnode can contain either a regulator or a PHY (or both).
>
> In order to be able to asociate a port with a regulator the port are
> now a platform device in the device tree case.
>
> There was only one driver which used directly the regulator field of
> the ahci_host_priv structure. To preserve the bisectability the change
> in the ahci_imx driver was done in the same patch.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Looks good, still needs one more (small) revision though, see comments
inline.

> ---
>   drivers/ata/ahci.h             |   2 +-
>   drivers/ata/ahci_imx.c         |  14 +--
>   drivers/ata/libahci_platform.c | 227 +++++++++++++++++++++++++++++------------
>   include/linux/ahci_platform.h  |   2 +
>   4 files changed, 170 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index 40f0e34f17af..275358ae0b3f 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -333,7 +333,7 @@ struct ahci_host_priv {
>   	u32			em_msg_type;	/* EM message type */
>   	bool			got_runtime_pm; /* Did we do pm_runtime_get? */
>   	struct clk		*clks[AHCI_MAX_CLKS]; /* Optional */
> -	struct regulator	*target_pwr;	/* Optional */
> +	struct regulator	**target_pwrs;	/* Optional */
>   	/*
>   	 * If platform uses PHYs. There is a 1:1 relation between the port number and
>   	 * the PHY position in this array.
> diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
> index 35d51c59a370..41632e57d46f 100644
> --- a/drivers/ata/ahci_imx.c
> +++ b/drivers/ata/ahci_imx.c
> @@ -221,11 +221,9 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv)
>   	if (imxpriv->no_device)
>   		return 0;
>
> -	if (hpriv->target_pwr) {
> -		ret = regulator_enable(hpriv->target_pwr);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = ahci_platform_enable_regulators(hpriv);
> +	if (ret)
> +		return ret;
>
>   	ret = clk_prepare_enable(imxpriv->sata_ref_clk);
>   	if (ret < 0)
> @@ -270,8 +268,7 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv)
>   disable_clk:
>   	clk_disable_unprepare(imxpriv->sata_ref_clk);
>   disable_regulator:
> -	if (hpriv->target_pwr)
> -		regulator_disable(hpriv->target_pwr);
> +	ahci_platform_disable_regulators(hpriv);
>
>   	return ret;
>   }
> @@ -291,8 +288,7 @@ static void imx_sata_disable(struct ahci_host_priv *hpriv)
>
>   	clk_disable_unprepare(imxpriv->sata_ref_clk);
>
> -	if (hpriv->target_pwr)
> -		regulator_disable(hpriv->target_pwr);
> +	ahci_platform_disable_regulators(hpriv);
>   }
>
>   static void ahci_imx_error_handler(struct ata_port *ap)
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index a147aaadca85..1db968ef6ff8 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -24,6 +24,7 @@
>   #include <linux/ahci_platform.h>
>   #include <linux/phy/phy.h>
>   #include <linux/pm_runtime.h>
> +#include <linux/of_platform.h>
>   #include "ahci.h"
>
>   static void ahci_host_stop(struct ata_host *host);
> @@ -138,6 +139,59 @@ void ahci_platform_disable_clks(struct ahci_host_priv *hpriv)
>   EXPORT_SYMBOL_GPL(ahci_platform_disable_clks);
>
>   /**
> + * ahci_platform_enable_regulators - Enable regulators
> + * @hpriv: host private area to store config values
> + *
> + * This function enables all the regulators found in
> + * hpriv->target_pwrs, if any.  If a regulator fails to be enabled, it
> + * disables all the regulators already enabled in reverse order and
> + * returns an error.
> + *
> + * RETURNS:
> + * 0 on success otherwise a negative error code
> + */
> +int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv)
> +{
> +	int rc, i;
> +
> +	for (i = 0; i < hpriv->nports; i++) {
> +		if (!hpriv->target_pwrs[i])
> +			continue;
> +
> +		rc = regulator_enable(hpriv->target_pwrs[i]);
> +		if (rc)
> +			goto disable_target_pwrs;
> +	}
> +
> +	return 0;
> +
> +disable_target_pwrs:
> +	while (--i >= 0)
> +		if (hpriv->target_pwrs[i])
> +			regulator_disable(hpriv->target_pwrs[i]);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(ahci_platform_enable_regulators);
> +
> +/**
> + * ahci_platform_disable_regulators - Disable regulators
> + * @hpriv: host private area to store config values
> + *
> + * This function disables all regulators found in hpriv->target_pwrs.
> + */
> +void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv)
> +{
> +	int i;
> +
> +	for (i = 0; i < hpriv->nports; i++) {
> +		if (!hpriv->target_pwrs[i])
> +			continue;
> +		regulator_disable(hpriv->target_pwrs[i]);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(ahci_platform_disable_regulators);
> +/**
>    * ahci_platform_enable_resources - Enable platform resources
>    * @hpriv: host private area to store config values
>    *
> @@ -157,11 +211,9 @@ int ahci_platform_enable_resources(struct ahci_host_priv *hpriv)
>   {
>   	int rc;
>
> -	if (hpriv->target_pwr) {
> -		rc = regulator_enable(hpriv->target_pwr);
> -		if (rc)
> -			return rc;
> -	}
> +	rc = ahci_platform_enable_regulators(hpriv);
> +	if (rc)
> +		return rc;
>
>   	rc = ahci_platform_enable_clks(hpriv);
>   	if (rc)
> @@ -177,8 +229,8 @@ disable_clks:
>   	ahci_platform_disable_clks(hpriv);
>
>   disable_regulator:
> -	if (hpriv->target_pwr)
> -		regulator_disable(hpriv->target_pwr);
> +	ahci_platform_disable_regulators(hpriv);
> +
>   	return rc;
>   }
>   EXPORT_SYMBOL_GPL(ahci_platform_enable_resources);
> @@ -199,8 +251,7 @@ void ahci_platform_disable_resources(struct ahci_host_priv *hpriv)
>
>   	ahci_platform_disable_clks(hpriv);
>
> -	if (hpriv->target_pwr)
> -		regulator_disable(hpriv->target_pwr);
> +	ahci_platform_disable_regulators(hpriv);
>   }
>   EXPORT_SYMBOL_GPL(ahci_platform_disable_resources);
>
> @@ -218,6 +269,59 @@ static void ahci_platform_put_resources(struct device *dev, void *res)
>   		clk_put(hpriv->clks[c]);
>   }
>
> +static int ahci_platform_get_phy(struct ahci_host_priv *hpriv, u32 port,
> +				struct device *dev, struct device_node *node)
> +{
> +	int rc;
> +
> +	hpriv->phys[port] = devm_of_phy_get(dev, node, NULL);
> +
> +	if (!IS_ERR(hpriv->phys[port]))
> +		return 0;
> +
> +	rc = PTR_ERR(hpriv->phys[port]);
> +	switch (rc) {
> +	case -ENOSYS:
> +		/* No PHY support. Check if PHY is required. */
> +		if (of_find_property(node, "phys", NULL)) {
> +			dev_err(dev,
> +				"couldn't get PHY in node %s: ENOSYS\n",
> +				node->name);
> +			break;
> +		}
> +	case -ENODEV:
> +		/* continue normally */
> +		hpriv->phys[port] = NULL;
> +		rc = 0;
> +		break;
> +
> +	default:
> +		dev_err(dev,
> +			"couldn't get PHY in node %s: %d\n",
> +			node->name, rc);
> +
> +		break;
> +	}
> +
> +	return rc;
> +}
> +
> +static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
> +				struct device *dev)
> +{
> +	struct regulator *target_pwr;
> +	int rc = 0;
> +
> +	target_pwr = devm_regulator_get_optional(dev, "target");

As you've already found out the devm framework does not work for freeing
things which are tied to the child node devices, so it is better to
use the non devm function here, the devm variant just adds overhead
(extra malloc under the hood), without buying us anything here.

> +
> +	if (!IS_ERR(target_pwr))
> +		hpriv->target_pwrs[port] = target_pwr;
> +	else
> +		rc = PTR_ERR(target_pwr);
> +
> +	return rc;
> +}
> +
>   /**
>    * ahci_platform_get_resources - Get platform resources
>    * @pdev: platform device to get resources for


You're not modifying ahci_platform_put_resources to put the regulators here,
since the regulators may now be bound to the platfrom-devs for the child nodes,
they will not get cleaned up by the devm framework on driver unbind, as the
driver is only bound to the main device and thus the devm framework only
cleans up resources attached to the main device.

So you need to modify ahci_platform_put_resources to explicitly put the
reference to the regulators.

Note since ahci_platform_put_resources gets called during probe errors too,
you need to check hpriv->target_pwrs before you deref it as it may be NULL!

Please also add a comment to ahci_platform_put_resources why the regulators
cannot be devm managed.

> @@ -240,7 +344,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>   	struct ahci_host_priv *hpriv;
>   	struct clk *clk;
>   	struct device_node *child;
> -	int i, enabled_ports = 0, rc = -ENOMEM;
> +	int i, sz, enabled_ports = 0, rc = -ENOMEM, child_nodes;
>   	u32 mask_port_map = 0;
>
>   	if (!devres_open_group(dev, NULL, GFP_KERNEL))
> @@ -261,14 +365,6 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>   		goto err_out;
>   	}
>
> -	hpriv->target_pwr = devm_regulator_get_optional(dev, "target");
> -	if (IS_ERR(hpriv->target_pwr)) {
> -		rc = PTR_ERR(hpriv->target_pwr);
> -		if (rc == -EPROBE_DEFER)
> -			goto err_out;
> -		hpriv->target_pwr = NULL;
> -	}
> -
>   	for (i = 0; i < AHCI_MAX_CLKS; i++) {
>   		/*
>   		 * For now we must use clk_get(dev, NULL) for the first clock,
> @@ -290,19 +386,33 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>   		hpriv->clks[i] = clk;
>   	}
>
> -	hpriv->nports = of_get_child_count(dev->of_node);
> +	hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
>
> -	if (hpriv->nports) {
> -		hpriv->phys = devm_kzalloc(dev,
> -					   hpriv->nports * sizeof(*hpriv->phys),
> -					   GFP_KERNEL);
> -		if (!hpriv->phys) {
> -			rc = -ENOMEM;
> -			goto err_out;
> -		}
> +	/*
> +	 * If no sub-node was found, we still need to set nports to
> +	 * one in order to be able to use the
> +	 * ahci_platform_[en|dis]able_[phys|regulators] functions.
> +	 */
> +	if (!child_nodes)
> +		hpriv->nports = 1;
> +
> +	sz = hpriv->nports * sizeof(*hpriv->phys);
> +	hpriv->phys = devm_kzalloc(dev, sz, GFP_KERNEL);
> +	if (!hpriv->phys) {
> +		rc = -ENOMEM;
> +		goto err_out;
> +	}
> +	sz = hpriv->nports * sizeof(*hpriv->target_pwrs);
> +	hpriv->target_pwrs = devm_kzalloc(dev, sz, GFP_KERNEL);
> +	if (!hpriv->target_pwrs) {
> +		rc = -ENOMEM;
> +		goto err_out;
> +	}
>
> +	if (child_nodes) {
>   		for_each_child_of_node(dev->of_node, child) {
>   			u32 port;
> +			struct platform_device *port_dev;
>
>   			if (!of_device_is_available(child))
>   				continue;
> @@ -316,17 +426,20 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>   				dev_warn(dev, "invalid port number %d\n", port);
>   				continue;
>   			}
> -
>   			mask_port_map |= BIT(port);
>
> -			hpriv->phys[port] = devm_of_phy_get(dev, child, NULL);
> -			if (IS_ERR(hpriv->phys[port])) {
> -				rc = PTR_ERR(hpriv->phys[port]);
> -				dev_err(dev,
> -					"couldn't get PHY in node %s: %d\n",
> -					child->name, rc);
> +			of_platform_device_create(child, NULL, NULL);
> +
> +			port_dev = of_find_device_by_node(child);

This call can fail, you need to error check it before dereferencing the port_dev
pointer below.

> +
> +			rc = ahci_platform_get_regulator(hpriv, port,
> +							&port_dev->dev);
> +			if (rc == -EPROBE_DEFER)
> +				goto err_out;
> +
> +			rc = ahci_platform_get_phy(hpriv, port, dev, child);
> +			if (rc)
>   				goto err_out;
> -			}
>
>   			enabled_ports++;
>   		}
> @@ -343,38 +456,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>   		 * If no sub-node was found, keep this for device tree
>   		 * compatibility
>   		 */
> -		struct phy *phy = devm_phy_get(dev, "sata-phy");
> -		if (!IS_ERR(phy)) {
> -			hpriv->phys = devm_kzalloc(dev, sizeof(*hpriv->phys),
> -						   GFP_KERNEL);
> -			if (!hpriv->phys) {
> -				rc = -ENOMEM;
> -				goto err_out;
> -			}
> -
> -			hpriv->phys[0] = phy;
> -			hpriv->nports = 1;
> -		} else {
> -			rc = PTR_ERR(phy);
> -			switch (rc) {
> -				case -ENOSYS:
> -					/* No PHY support. Check if PHY is required. */
> -					if (of_find_property(dev->of_node, "phys", NULL)) {
> -						dev_err(dev, "couldn't get sata-phy: ENOSYS\n");
> -						goto err_out;
> -					}
> -				case -ENODEV:
> -					/* continue normally */
> -					hpriv->phys = NULL;
> -					break;
> -
> -				default:
> -					goto err_out;
> +		rc = ahci_platform_get_phy(hpriv, 0, dev, dev->of_node);
> +		if (rc)
> +			goto err_out;
>
> -			}
> -		}
> +		rc = ahci_platform_get_regulator(hpriv, 0, dev);
> +		if (rc == -EPROBE_DEFER)
> +			goto err_out;
>   	}
> -
>   	pm_runtime_enable(dev);
>   	pm_runtime_get_sync(dev);
>   	hpriv->got_runtime_pm = true;
> @@ -383,6 +472,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>   	return hpriv;
>
>   err_out:
> +	/*
> +	 * In case of a deferred probe, previous regulators may have
> +	 * been already get, so put them to be able to get them again
> +	 * in the next probe.
> +	 */
> +	for (i = 0; i < hpriv->nports; i++)
> +		if (hpriv->target_pwrs[i])
> +			devm_regulator_put(hpriv->target_pwrs[i]);

Once you've added the regulator_put calls to ahci_platform_put_resources
you can drop this as the devres_release_group() call will call
ahci_platform_put_resources().

>   	devres_release_group(dev, NULL);
>   	return ERR_PTR(rc);
>   }
> diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
> index 642d6ae4030c..f65b33809170 100644
> --- a/include/linux/ahci_platform.h
> +++ b/include/linux/ahci_platform.h
> @@ -24,6 +24,8 @@ struct platform_device;
>
>   int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
>   void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
> +int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv);
> +void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv);
>   int ahci_platform_enable_resources(struct ahci_host_priv *hpriv);
>   void ahci_platform_disable_resources(struct ahci_host_priv *hpriv);
>   struct ahci_host_priv *ahci_platform_get_resources(
>

Regards,

Hans

Re: [PATCH v3 3/4] ata: libahci: Allow using multiple regulators

[Gregory CLEMENT]

Hi Hans,

On 15/01/2015 09:46, Hans de Goede wrote:
> Hi,
> 
> On 13-01-15 15:22, Gregory CLEMENT wrote:
>> The current implementation of the libahci allows using multiple PHYs
>> but not multiple regulators. This patch adds the support of multiple
>> regulators. Until now it was mandatory to have a PHY under a subnode,
>> now a port subnode can contain either a regulator or a PHY (or both).
>>
>> In order to be able to asociate a port with a regulator the port are
>> now a platform device in the device tree case.
>>
>> There was only one driver which used directly the regulator field of
>> the ahci_host_priv structure. To preserve the bisectability the change
>> in the ahci_imx driver was done in the same patch.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> 
> Looks good, still needs one more (small) revision though, see comments
> inline.

OK I will take care of it

Thanks,

Gregory

> 
>> ---
>>   drivers/ata/ahci.h             |   2 +-
>>   drivers/ata/ahci_imx.c         |  14 +--
>>   drivers/ata/libahci_platform.c | 227 +++++++++++++++++++++++++++++------------
>>   include/linux/ahci_platform.h  |   2 +
>>   4 files changed, 170 insertions(+), 75 deletions(-)
>>
>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
>> index 40f0e34f17af..275358ae0b3f 100644
>> --- a/drivers/ata/ahci.h
>> +++ b/drivers/ata/ahci.h
>> @@ -333,7 +333,7 @@ struct ahci_host_priv {
>>   	u32			em_msg_type;	/* EM message type */
>>   	bool			got_runtime_pm; /* Did we do pm_runtime_get? */
>>   	struct clk		*clks[AHCI_MAX_CLKS]; /* Optional */
>> -	struct regulator	*target_pwr;	/* Optional */
>> +	struct regulator	**target_pwrs;	/* Optional */
>>   	/*
>>   	 * If platform uses PHYs. There is a 1:1 relation between the port number and
>>   	 * the PHY position in this array.
>> diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
>> index 35d51c59a370..41632e57d46f 100644
>> --- a/drivers/ata/ahci_imx.c
>> +++ b/drivers/ata/ahci_imx.c
>> @@ -221,11 +221,9 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv)
>>   	if (imxpriv->no_device)
>>   		return 0;
>>
>> -	if (hpriv->target_pwr) {
>> -		ret = regulator_enable(hpriv->target_pwr);
>> -		if (ret)
>> -			return ret;
>> -	}
>> +	ret = ahci_platform_enable_regulators(hpriv);
>> +	if (ret)
>> +		return ret;
>>
>>   	ret = clk_prepare_enable(imxpriv->sata_ref_clk);
>>   	if (ret < 0)
>> @@ -270,8 +268,7 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv)
>>   disable_clk:
>>   	clk_disable_unprepare(imxpriv->sata_ref_clk);
>>   disable_regulator:
>> -	if (hpriv->target_pwr)
>> -		regulator_disable(hpriv->target_pwr);
>> +	ahci_platform_disable_regulators(hpriv);
>>
>>   	return ret;
>>   }
>> @@ -291,8 +288,7 @@ static void imx_sata_disable(struct ahci_host_priv *hpriv)
>>
>>   	clk_disable_unprepare(imxpriv->sata_ref_clk);
>>
>> -	if (hpriv->target_pwr)
>> -		regulator_disable(hpriv->target_pwr);
>> +	ahci_platform_disable_regulators(hpriv);
>>   }
>>
>>   static void ahci_imx_error_handler(struct ata_port *ap)
>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
>> index a147aaadca85..1db968ef6ff8 100644
>> --- a/drivers/ata/libahci_platform.c
>> +++ b/drivers/ata/libahci_platform.c
>> @@ -24,6 +24,7 @@
>>   #include <linux/ahci_platform.h>
>>   #include <linux/phy/phy.h>
>>   #include <linux/pm_runtime.h>
>> +#include <linux/of_platform.h>
>>   #include "ahci.h"
>>
>>   static void ahci_host_stop(struct ata_host *host);
>> @@ -138,6 +139,59 @@ void ahci_platform_disable_clks(struct ahci_host_priv *hpriv)
>>   EXPORT_SYMBOL_GPL(ahci_platform_disable_clks);
>>
>>   /**
>> + * ahci_platform_enable_regulators - Enable regulators
>> + * @hpriv: host private area to store config values
>> + *
>> + * This function enables all the regulators found in
>> + * hpriv->target_pwrs, if any.  If a regulator fails to be enabled, it
>> + * disables all the regulators already enabled in reverse order and
>> + * returns an error.
>> + *
>> + * RETURNS:
>> + * 0 on success otherwise a negative error code
>> + */
>> +int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv)
>> +{
>> +	int rc, i;
>> +
>> +	for (i = 0; i < hpriv->nports; i++) {
>> +		if (!hpriv->target_pwrs[i])
>> +			continue;
>> +
>> +		rc = regulator_enable(hpriv->target_pwrs[i]);
>> +		if (rc)
>> +			goto disable_target_pwrs;
>> +	}
>> +
>> +	return 0;
>> +
>> +disable_target_pwrs:
>> +	while (--i >= 0)
>> +		if (hpriv->target_pwrs[i])
>> +			regulator_disable(hpriv->target_pwrs[i]);
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_GPL(ahci_platform_enable_regulators);
>> +
>> +/**
>> + * ahci_platform_disable_regulators - Disable regulators
>> + * @hpriv: host private area to store config values
>> + *
>> + * This function disables all regulators found in hpriv->target_pwrs.
>> + */
>> +void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < hpriv->nports; i++) {
>> +		if (!hpriv->target_pwrs[i])
>> +			continue;
>> +		regulator_disable(hpriv->target_pwrs[i]);
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(ahci_platform_disable_regulators);
>> +/**
>>    * ahci_platform_enable_resources - Enable platform resources
>>    * @hpriv: host private area to store config values
>>    *
>> @@ -157,11 +211,9 @@ int ahci_platform_enable_resources(struct ahci_host_priv *hpriv)
>>   {
>>   	int rc;
>>
>> -	if (hpriv->target_pwr) {
>> -		rc = regulator_enable(hpriv->target_pwr);
>> -		if (rc)
>> -			return rc;
>> -	}
>> +	rc = ahci_platform_enable_regulators(hpriv);
>> +	if (rc)
>> +		return rc;
>>
>>   	rc = ahci_platform_enable_clks(hpriv);
>>   	if (rc)
>> @@ -177,8 +229,8 @@ disable_clks:
>>   	ahci_platform_disable_clks(hpriv);
>>
>>   disable_regulator:
>> -	if (hpriv->target_pwr)
>> -		regulator_disable(hpriv->target_pwr);
>> +	ahci_platform_disable_regulators(hpriv);
>> +
>>   	return rc;
>>   }
>>   EXPORT_SYMBOL_GPL(ahci_platform_enable_resources);
>> @@ -199,8 +251,7 @@ void ahci_platform_disable_resources(struct ahci_host_priv *hpriv)
>>
>>   	ahci_platform_disable_clks(hpriv);
>>
>> -	if (hpriv->target_pwr)
>> -		regulator_disable(hpriv->target_pwr);
>> +	ahci_platform_disable_regulators(hpriv);
>>   }
>>   EXPORT_SYMBOL_GPL(ahci_platform_disable_resources);
>>
>> @@ -218,6 +269,59 @@ static void ahci_platform_put_resources(struct device *dev, void *res)
>>   		clk_put(hpriv->clks[c]);
>>   }
>>
>> +static int ahci_platform_get_phy(struct ahci_host_priv *hpriv, u32 port,
>> +				struct device *dev, struct device_node *node)
>> +{
>> +	int rc;
>> +
>> +	hpriv->phys[port] = devm_of_phy_get(dev, node, NULL);
>> +
>> +	if (!IS_ERR(hpriv->phys[port]))
>> +		return 0;
>> +
>> +	rc = PTR_ERR(hpriv->phys[port]);
>> +	switch (rc) {
>> +	case -ENOSYS:
>> +		/* No PHY support. Check if PHY is required. */
>> +		if (of_find_property(node, "phys", NULL)) {
>> +			dev_err(dev,
>> +				"couldn't get PHY in node %s: ENOSYS\n",
>> +				node->name);
>> +			break;
>> +		}
>> +	case -ENODEV:
>> +		/* continue normally */
>> +		hpriv->phys[port] = NULL;
>> +		rc = 0;
>> +		break;
>> +
>> +	default:
>> +		dev_err(dev,
>> +			"couldn't get PHY in node %s: %d\n",
>> +			node->name, rc);
>> +
>> +		break;
>> +	}
>> +
>> +	return rc;
>> +}
>> +
>> +static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
>> +				struct device *dev)
>> +{
>> +	struct regulator *target_pwr;
>> +	int rc = 0;
>> +
>> +	target_pwr = devm_regulator_get_optional(dev, "target");
> 
> As you've already found out the devm framework does not work for freeing
> things which are tied to the child node devices, so it is better to
> use the non devm function here, the devm variant just adds overhead
> (extra malloc under the hood), without buying us anything here.
> 
>> +
>> +	if (!IS_ERR(target_pwr))
>> +		hpriv->target_pwrs[port] = target_pwr;
>> +	else
>> +		rc = PTR_ERR(target_pwr);
>> +
>> +	return rc;
>> +}
>> +
>>   /**
>>    * ahci_platform_get_resources - Get platform resources
>>    * @pdev: platform device to get resources for
> 
> 
> You're not modifying ahci_platform_put_resources to put the regulators here,
> since the regulators may now be bound to the platfrom-devs for the child nodes,
> they will not get cleaned up by the devm framework on driver unbind, as the
> driver is only bound to the main device and thus the devm framework only
> cleans up resources attached to the main device.
> 
> So you need to modify ahci_platform_put_resources to explicitly put the
> reference to the regulators.
> 
> Note since ahci_platform_put_resources gets called during probe errors too,
> you need to check hpriv->target_pwrs before you deref it as it may be NULL!
> 
> Please also add a comment to ahci_platform_put_resources why the regulators
> cannot be devm managed.
> 
>> @@ -240,7 +344,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>>   	struct ahci_host_priv *hpriv;
>>   	struct clk *clk;
>>   	struct device_node *child;
>> -	int i, enabled_ports = 0, rc = -ENOMEM;
>> +	int i, sz, enabled_ports = 0, rc = -ENOMEM, child_nodes;
>>   	u32 mask_port_map = 0;
>>
>>   	if (!devres_open_group(dev, NULL, GFP_KERNEL))
>> @@ -261,14 +365,6 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>>   		goto err_out;
>>   	}
>>
>> -	hpriv->target_pwr = devm_regulator_get_optional(dev, "target");
>> -	if (IS_ERR(hpriv->target_pwr)) {
>> -		rc = PTR_ERR(hpriv->target_pwr);
>> -		if (rc == -EPROBE_DEFER)
>> -			goto err_out;
>> -		hpriv->target_pwr = NULL;
>> -	}
>> -
>>   	for (i = 0; i < AHCI_MAX_CLKS; i++) {
>>   		/*
>>   		 * For now we must use clk_get(dev, NULL) for the first clock,
>> @@ -290,19 +386,33 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>>   		hpriv->clks[i] = clk;
>>   	}
>>
>> -	hpriv->nports = of_get_child_count(dev->of_node);
>> +	hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
>>
>> -	if (hpriv->nports) {
>> -		hpriv->phys = devm_kzalloc(dev,
>> -					   hpriv->nports * sizeof(*hpriv->phys),
>> -					   GFP_KERNEL);
>> -		if (!hpriv->phys) {
>> -			rc = -ENOMEM;
>> -			goto err_out;
>> -		}
>> +	/*
>> +	 * If no sub-node was found, we still need to set nports to
>> +	 * one in order to be able to use the
>> +	 * ahci_platform_[en|dis]able_[phys|regulators] functions.
>> +	 */
>> +	if (!child_nodes)
>> +		hpriv->nports = 1;
>> +
>> +	sz = hpriv->nports * sizeof(*hpriv->phys);
>> +	hpriv->phys = devm_kzalloc(dev, sz, GFP_KERNEL);
>> +	if (!hpriv->phys) {
>> +		rc = -ENOMEM;
>> +		goto err_out;
>> +	}
>> +	sz = hpriv->nports * sizeof(*hpriv->target_pwrs);
>> +	hpriv->target_pwrs = devm_kzalloc(dev, sz, GFP_KERNEL);
>> +	if (!hpriv->target_pwrs) {
>> +		rc = -ENOMEM;
>> +		goto err_out;
>> +	}
>>
>> +	if (child_nodes) {
>>   		for_each_child_of_node(dev->of_node, child) {
>>   			u32 port;
>> +			struct platform_device *port_dev;
>>
>>   			if (!of_device_is_available(child))
>>   				continue;
>> @@ -316,17 +426,20 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>>   				dev_warn(dev, "invalid port number %d\n", port);
>>   				continue;
>>   			}
>> -
>>   			mask_port_map |= BIT(port);
>>
>> -			hpriv->phys[port] = devm_of_phy_get(dev, child, NULL);
>> -			if (IS_ERR(hpriv->phys[port])) {
>> -				rc = PTR_ERR(hpriv->phys[port]);
>> -				dev_err(dev,
>> -					"couldn't get PHY in node %s: %d\n",
>> -					child->name, rc);
>> +			of_platform_device_create(child, NULL, NULL);
>> +
>> +			port_dev = of_find_device_by_node(child);
> 
> This call can fail, you need to error check it before dereferencing the port_dev
> pointer below.
> 
>> +
>> +			rc = ahci_platform_get_regulator(hpriv, port,
>> +							&port_dev->dev);
>> +			if (rc == -EPROBE_DEFER)
>> +				goto err_out;
>> +
>> +			rc = ahci_platform_get_phy(hpriv, port, dev, child);
>> +			if (rc)
>>   				goto err_out;
>> -			}
>>
>>   			enabled_ports++;
>>   		}
>> @@ -343,38 +456,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>>   		 * If no sub-node was found, keep this for device tree
>>   		 * compatibility
>>   		 */
>> -		struct phy *phy = devm_phy_get(dev, "sata-phy");
>> -		if (!IS_ERR(phy)) {
>> -			hpriv->phys = devm_kzalloc(dev, sizeof(*hpriv->phys),
>> -						   GFP_KERNEL);
>> -			if (!hpriv->phys) {
>> -				rc = -ENOMEM;
>> -				goto err_out;
>> -			}
>> -
>> -			hpriv->phys[0] = phy;
>> -			hpriv->nports = 1;
>> -		} else {
>> -			rc = PTR_ERR(phy);
>> -			switch (rc) {
>> -				case -ENOSYS:
>> -					/* No PHY support. Check if PHY is required. */
>> -					if (of_find_property(dev->of_node, "phys", NULL)) {
>> -						dev_err(dev, "couldn't get sata-phy: ENOSYS\n");
>> -						goto err_out;
>> -					}
>> -				case -ENODEV:
>> -					/* continue normally */
>> -					hpriv->phys = NULL;
>> -					break;
>> -
>> -				default:
>> -					goto err_out;
>> +		rc = ahci_platform_get_phy(hpriv, 0, dev, dev->of_node);
>> +		if (rc)
>> +			goto err_out;
>>
>> -			}
>> -		}
>> +		rc = ahci_platform_get_regulator(hpriv, 0, dev);
>> +		if (rc == -EPROBE_DEFER)
>> +			goto err_out;
>>   	}
>> -
>>   	pm_runtime_enable(dev);
>>   	pm_runtime_get_sync(dev);
>>   	hpriv->got_runtime_pm = true;
>> @@ -383,6 +472,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>>   	return hpriv;
>>
>>   err_out:
>> +	/*
>> +	 * In case of a deferred probe, previous regulators may have
>> +	 * been already get, so put them to be able to get them again
>> +	 * in the next probe.
>> +	 */
>> +	for (i = 0; i < hpriv->nports; i++)
>> +		if (hpriv->target_pwrs[i])
>> +			devm_regulator_put(hpriv->target_pwrs[i]);
> 
> Once you've added the regulator_put calls to ahci_platform_put_resources
> you can drop this as the devres_release_group() call will call
> ahci_platform_put_resources().
> 
>>   	devres_release_group(dev, NULL);
>>   	return ERR_PTR(rc);
>>   }
>> diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
>> index 642d6ae4030c..f65b33809170 100644
>> --- a/include/linux/ahci_platform.h
>> +++ b/include/linux/ahci_platform.h
>> @@ -24,6 +24,8 @@ struct platform_device;
>>
>>   int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
>>   void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
>> +int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv);
>> +void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv);
>>   int ahci_platform_enable_resources(struct ahci_host_priv *hpriv);
>>   void ahci_platform_disable_resources(struct ahci_host_priv *hpriv);
>>   struct ahci_host_priv *ahci_platform_get_resources(
>>
> 
> Regards,
> 
> Hans
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com