Re: [PATCH 3/3] ata: libahci: Allow to use multiple regulators
From: Hans de Goede
Date: Sat Dec 27 2014 - 08:06:46 EST
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@xxxxxxxxxx>" 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@xxxxxxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/