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

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

[Gregory CLEMENT]

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. 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).

While I was working on it I also did a small clean-up, it is the
purpose of the 1st patch.

The main patch of the series (the 3rd one) also depends of a patch of
the regulator framework that I submitted yesterday: "regulator: core:
Add the device tree version to the regulator_get family",
https://lkml.org/lkml/2014/12/26/155

I refactored the PHY support in the same time, so I would like to have
a feedback from the Berlin platform to ensure that no regressions was
introduced.

Thanks,

Grégory


Gregory CLEMENT (3):
  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 to use multiple regulators

 .../devicetree/bindings/ata/ahci-platform.txt      |   3 +-
 drivers/ata/ahci.h                                 |   2 +-
 drivers/ata/ahci_imx.c                             |  14 +-
 drivers/ata/libahci_platform.c                     | 212 ++++++++++++++-------
 include/linux/ahci_platform.h                      |   2 +
 5 files changed, 153 insertions(+), 80 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Gregory CLEMENT]

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. 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).

While I was working on it I also did a small clean-up, it is the
purpose of the 1st patch.

The main patch of the series (the 3rd one) also depends of a patch of
the regulator framework that I submitted yesterday: "regulator: core:
Add the device tree version to the regulator_get family",
https://lkml.org/lkml/2014/12/26/155

I refactored the PHY support in the same time, so I would like to have
a feedback from the Berlin platform to ensure that no regressions was
introduced.

Thanks,

Grégory


Gregory CLEMENT (3):
  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 to use multiple regulators

 .../devicetree/bindings/ata/ahci-platform.txt      |   3 +-
 drivers/ata/ahci.h                                 |   2 +-
 drivers/ata/ahci_imx.c                             |  14 +-
 drivers/ata/libahci_platform.c                     | 212 ++++++++++++++-------
 include/linux/ahci_platform.h                      |   2 +
 5 files changed, 153 insertions(+), 80 deletions(-)

-- 
1.9.1

[PATCH 1/3] 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>
---
 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 2/3] 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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index 4ab09f2202d4..6be531a328ae 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -38,8 +38,9 @@ Required properties when using sub-nodes:
 
 Sub-nodes required properties:
 - 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 {
-- 
1.9.1

[PATCH 3/3] ata: libahci: Allow to use 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).

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.

This patch also depend of a patch of the regulator framework:
"regulator: core: Add the device tree version to the regulator_get
family"

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 | 206 ++++++++++++++++++++++++++++-------------
 include/linux/ahci_platform.h  |   2 +
 4 files changed, 151 insertions(+), 73 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..f3bf4311152d 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -138,6 +138,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 +210,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 +228,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 +250,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 +268,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 device_node *node)
+{
+	struct regulator *target_pwr;
+	int rc = 0;
+
+	target_pwr = devm_of_regulator_get_optional(dev, "target", node);
+
+	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 +343,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;
 	u32 mask_port_map = 0;
 
 	if (!devres_open_group(dev, NULL, GFP_KERNEL))
@@ -261,14 +364,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,
@@ -292,15 +387,24 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 
 	hpriv->nports = 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, keep this for device tree compatibility */
+	if (!hpriv->nports)
+		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 (hpriv->nports) {
 		for_each_child_of_node(dev->of_node, child) {
 			u32 port;
 
@@ -319,14 +423,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 
 			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);
+			rc = ahci_platform_get_regulator(hpriv, port,
+							 dev, child);
+			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 +447,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, dev->of_node);
+		if (rc == -EPROBE_DEFER)
+			goto err_out;
 	}
-
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 	hpriv->got_runtime_pm = true;
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

Re: [PATCH 3/3] ata: libahci: Allow to use multiple regulators

[Hans de Goede]

Hi Gregory,

Thanks for working on this. Overall the patch-set / concept looks good,
you can add "Acked-by: Hans de Goede <hdegoede@redhat.com>" to the first
2 patches.

I've some comments on this patch, see below.

On 27-12-14 11:34, 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).
>
> 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.
>
> This patch also depend of a patch of the regulator framework:
> "regulator: core: Add the device tree version to the regulator_get
> family"
>
> 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 | 206 ++++++++++++++++++++++++++++-------------
>   include/linux/ahci_platform.h  |   2 +
>   4 files changed, 151 insertions(+), 73 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..f3bf4311152d 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -138,6 +138,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])

I'm pretty sure you want:

		if (hpriv->target_pwrs[i])

here, so without the '!' .

> +			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 +210,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 +228,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 +250,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 +268,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 device_node *node)
> +{
> +	struct regulator *target_pwr;
> +	int rc = 0;
> +
> +	target_pwr = devm_of_regulator_get_optional(dev, "target", node);
> +
> +	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 +343,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;
>   	u32 mask_port_map = 0;
>
>   	if (!devres_open_group(dev, NULL, GFP_KERNEL))
> @@ -261,14 +364,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,
> @@ -292,15 +387,24 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>
>   	hpriv->nports = 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, keep this for device tree compatibility */
> +	if (!hpriv->nports)
> +		hpriv->nports = 1;

So now nports is always >= 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 (hpriv->nports) {

And this is always true,

>   		for_each_child_of_node(dev->of_node, child) {
>   			u32 port;
>
> @@ -319,14 +423,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>
>   			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);
> +			rc = ahci_platform_get_regulator(hpriv, port,
> +							 dev, child);
> +			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 +447,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
>   		 */

And thus the code below (which is in the else) never gets executed, and you've
broken the case where there are no subnodes.

I think it is best to fix this by keeping nports == 0 when there are no subnodes
(because we really do not know nports then, so advertising 1 is sorta wrong anyways),
and use something like this to calculate the array sizes:

sz = (nports ? nports : 1) * sizeof(*hpriv->phys);
...
sz = (nports ? nports : 1) * sizeof(*hpriv->target_pwrs);

> -		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, dev->of_node);
> +		if (rc == -EPROBE_DEFER)
> +			goto err_out;
>   	}
> -
>   	pm_runtime_enable(dev);
>   	pm_runtime_get_sync(dev);
>   	hpriv->got_runtime_pm = true;
> 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 3/3] ata: libahci: Allow to use multiple regulators

[Hans de Goede]

Hi,

On 27-12-14 13:58, Hans de Goede wrote:
> Hi Gregory,
>
> Thanks for working on this. Overall the patch-set / concept looks good,
> you can add "Acked-by: Hans de Goede <hdegoede@redhat.com>" to the first
> 2 patches.
>
> I've some comments on this patch, see below.
>
> On 27-12-14 11:34, 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).
>>
>> 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.
>>
>> This patch also depend of a patch of the regulator framework:
>> "regulator: core: Add the device tree version to the regulator_get
>> family"
>>
>> 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 | 206 ++++++++++++++++++++++++++++-------------
>>   include/linux/ahci_platform.h  |   2 +
>>   4 files changed, 151 insertions(+), 73 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..f3bf4311152d 100644
>> --- a/drivers/ata/libahci_platform.c
>> +++ b/drivers/ata/libahci_platform.c
>> @@ -138,6 +138,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])
>
> I'm pretty sure you want:
>
>          if (hpriv->target_pwrs[i])
>
> here, so without the '!' .
>
>> +            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 +210,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 +228,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 +250,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 +268,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 device_node *node)
>> +{
>> +    struct regulator *target_pwr;
>> +    int rc = 0;
>> +
>> +    target_pwr = devm_of_regulator_get_optional(dev, "target", node);
>> +
>> +    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 +343,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;
>>       u32 mask_port_map = 0;
>>
>>       if (!devres_open_group(dev, NULL, GFP_KERNEL))
>> @@ -261,14 +364,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,
>> @@ -292,15 +387,24 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>>
>>       hpriv->nports = 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, keep this for device tree compatibility */
>> +    if (!hpriv->nports)
>> +        hpriv->nports = 1;
>
> So now nports is always >= 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 (hpriv->nports) {
>
> And this is always true,
>
>>           for_each_child_of_node(dev->of_node, child) {
>>               u32 port;
>>
>> @@ -319,14 +423,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>>
>>               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);
>> +            rc = ahci_platform_get_regulator(hpriv, port,
>> +                             dev, child);
>> +            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 +447,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
>>            */
>
> And thus the code below (which is in the else) never gets executed, and you've
> broken the case where there are no subnodes.
>
> I think it is best to fix this by keeping nports == 0 when there are no subnodes
> (because we really do not know nports then, so advertising 1 is sorta wrong anyways),

Erm, scrap that we were already setting nports = 1 later on in the no child nodes
case to make ahci_platform_[en|dis]able_phys do the right thing.


> and use something like this to calculate the array sizes:
>
> sz = (nports ? nports : 1) * sizeof(*hpriv->phys);
> ...
> sz = (nports ? nports : 1) * sizeof(*hpriv->target_pwrs);

Still using the above and setting nports = 1 in the "else" case is
an option, but I think that adding a local child_nodes var and doing:

hpriv->nports = child_nodes = of_get_child_count(dev->of_node);

And changing the "if (hpriv->nports)" into "if (child_nodes)",
is a better solution.

Regards,

Hans


>
>> -        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, dev->of_node);
>> +        if (rc == -EPROBE_DEFER)
>> +            goto err_out;
>>       }
>> -
>>       pm_runtime_enable(dev);
>>       pm_runtime_get_sync(dev);
>>       hpriv->got_runtime_pm = true;
>> 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ata: libahci: Allow to use multiple regulators

[Gregory CLEMENT]

Hi Hans,

On 27/12/2014 13:58, Hans de Goede wrote:
> Hi Gregory,
> 
> Thanks for working on this. Overall the patch-set / concept looks good,
> you can add "Acked-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>" to the first
> 2 patches.

Thanks for your review. About the second patch as the change in the regulator
framework didn't have been accepted, I had to change the binding. I will send
a the new binding in the next version.

> 
> I've some comments on this patch, see below.
> 
[...]

>> +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])
> 
> I'm pretty sure you want:
> 
> 		if (hpriv->target_pwrs[i])
> 
> here, so without the '!' .
> 
>> +			regulator_disable(hpriv->target_pwrs[i]);

yes you're right.

>> +
>> +	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 +210,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 +228,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 +250,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 +268,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 device_node *node)
>> +{
>> +	struct regulator *target_pwr;
>> +	int rc = 0;
>> +
>> +	target_pwr = devm_of_regulator_get_optional(dev, "target", node);
>> +
>> +	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 +343,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;
>>   	u32 mask_port_map = 0;
>>
>>   	if (!devres_open_group(dev, NULL, GFP_KERNEL))
>> @@ -261,14 +364,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,
>> @@ -292,15 +387,24 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>>
>>   	hpriv->nports = 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, keep this for device tree compatibility */
>> +	if (!hpriv->nports)
>> +		hpriv->nports = 1;
> 
> So now nports is always >= 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 (hpriv->nports) {
> 
> And this is always true,
> 
>>   		for_each_child_of_node(dev->of_node, child) {
>>   			u32 port;
>>
>> @@ -319,14 +423,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>>
>>   			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);
>> +			rc = ahci_platform_get_regulator(hpriv, port,
>> +							 dev, child);
>> +			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 +447,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
>>   		 */
> 
> And thus the code below (which is in the else) never gets executed, and you've
> broken the case where there are no subnodes.
> 
> I think it is best to fix this by keeping nports == 0 when there are no subnodes
> (because we really do not know nports then, so advertising 1 is sorta wrong anyways),

You are definitely right

> and use something like this to calculate the array sizes:
> 
> sz = (nports ? nports : 1) * sizeof(*hpriv->phys);
> ...
> sz = (nports ? nports : 1) * sizeof(*hpriv->target_pwrs);

I will do it.


Thanks again for your review,

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ata: libahci: Allow to use multiple regulators

[Gregory CLEMENT]

Hi Hans,

On 27/12/2014 13:58, Hans de Goede wrote:
> Hi Gregory,
> 
> Thanks for working on this. Overall the patch-set / concept looks good,
> you can add "Acked-by: Hans de Goede <hdegoede@redhat.com>" to the first
> 2 patches.

Thanks for your review. About the second patch as the change in the regulator
framework didn't have been accepted, I had to change the binding. I will send
a the new binding in the next version.

> 
> I've some comments on this patch, see below.
> 
[...]

>> +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])
> 
> I'm pretty sure you want:
> 
> 		if (hpriv->target_pwrs[i])
> 
> here, so without the '!' .
> 
>> +			regulator_disable(hpriv->target_pwrs[i]);

yes you're right.

>> +
>> +	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 +210,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 +228,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 +250,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 +268,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 device_node *node)
>> +{
>> +	struct regulator *target_pwr;
>> +	int rc = 0;
>> +
>> +	target_pwr = devm_of_regulator_get_optional(dev, "target", node);
>> +
>> +	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 +343,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;
>>   	u32 mask_port_map = 0;
>>
>>   	if (!devres_open_group(dev, NULL, GFP_KERNEL))
>> @@ -261,14 +364,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,
>> @@ -292,15 +387,24 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>>
>>   	hpriv->nports = 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, keep this for device tree compatibility */
>> +	if (!hpriv->nports)
>> +		hpriv->nports = 1;
> 
> So now nports is always >= 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 (hpriv->nports) {
> 
> And this is always true,
> 
>>   		for_each_child_of_node(dev->of_node, child) {
>>   			u32 port;
>>
>> @@ -319,14 +423,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>>
>>   			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);
>> +			rc = ahci_platform_get_regulator(hpriv, port,
>> +							 dev, child);
>> +			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 +447,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
>>   		 */
> 
> And thus the code below (which is in the else) never gets executed, and you've
> broken the case where there are no subnodes.
> 
> I think it is best to fix this by keeping nports == 0 when there are no subnodes
> (because we really do not know nports then, so advertising 1 is sorta wrong anyways),

You are definitely right

> and use something like this to calculate the array sizes:
> 
> sz = (nports ? nports : 1) * sizeof(*hpriv->phys);
> ...
> sz = (nports ? nports : 1) * sizeof(*hpriv->target_pwrs);

I will do it.


Thanks again for your review,

Gregory


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