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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux