On 04/03/16 12:24, Shawn Lin wrote:

Hi Adrian,

On 2016/3/4 17:14, Adrian Hunter wrote:

quoted

On 19/02/16 04:20, Shawn Lin wrote:

quoted

This patch adds Generic PHY access for sdhci-of-arasan. Driver

[...]

quoted

quoted

  /**
   * sdhci_arasan_suspend - Suspend method for the driver
   * @dev:    Address of the device structure
- * Returns 0 on success and error value on error
+ * Return: 0 on success and error value on error

Really should be a separate patch for the comment fixes.

yep, I won't touch it when I rebase my patch for this topic.

quoted

quoted

   *
   * Put the device in a low power state.
   */

@@ -88,6 +91,14 @@ static int sdhci_arasan_suspend(struct device *dev)
      if (ret)
          return ret;

+    if (!IS_ERR(sdhci_arasan->phy)) {
+        ret = phy_power_off(sdhci_arasan->phy);
+        if (ret) {
+            dev_err(dev, "Cannot power off phy.\n");

I would expect you ought to call sdhci_resume_host() here.

oh, got it.

quoted

quoted

+            return ret;
+        }
+    }
+
      clk_disable(pltfm_host->clk);
      clk_disable(sdhci_arasan->clk_ahb);

@@ -97,7 +108,7 @@ static int sdhci_arasan_suspend(struct device *dev)
  /**
   * sdhci_arasan_resume - Resume method for the driver
   * @dev:    Address of the device structure
- * Returns 0 on success and error value on error
+ * Return: 0 on success and error value on error
   *
   * Resume operation after suspend
   */

@@ -118,11 +129,24 @@ static int sdhci_arasan_resume(struct device *dev)
      ret = clk_enable(pltfm_host->clk);
      if (ret) {
          dev_err(dev, "Cannot enable SD clock.\n");
-        clk_disable(sdhci_arasan->clk_ahb);
-        return ret;
+        goto err_clk_en;
+    }
+
+    if (!IS_ERR(sdhci_arasan->phy)) {
+        ret = phy_power_on(sdhci_arasan->phy);
+        if (ret) {
+            dev_err(dev, "Cannot power on phy.\n");
+            goto err_phy_power;
+        }
      }

      return sdhci_resume_host(host);
+
+err_phy_power:
+    clk_disable(pltfm_host->clk);
+err_clk_en:
+    clk_disable(sdhci_arasan->clk_ahb);

I don't know that disabling clocks is necessarily the right thing to do if
the resume fails.  You might want to consider what happens if the system
tries to use the device when it is in that state.  It's been a long time
since I did any ARM work but aren't you risking bus errors.  Might just as
well not bother and save a few lines of code.

Firstly, I really just follow the orginal err handle.
It's risking bus errors if disabling ahb clk, but ahb clk is most very
likely to shared by many controllers. So regarding to the clk reference
count, the ahb_clk is not *really* disabled and just decrease the
reference count. How about keep these handles here?

I would remove the error handling on the grounds that it does not do
anything useful.

quoted

quoted

+    return ret;
  }
  #endif /* ! CONFIG_PM_SLEEP */

@@ -183,6 +207,30 @@ static int sdhci_arasan_probe(struct platform_device

*pdev)
          goto clk_disable_all;
      }

+    sdhci_arasan->phy = ERR_PTR(-ENODEV);
+    if (of_device_is_compatible(pdev->dev.of_node,
+                    "arasan,sdhci-5.1")) {
+        sdhci_arasan->phy = devm_phy_get(&pdev->dev,
+                         "phy_arasan");
+        if (IS_ERR(sdhci_arasan->phy)) {
+            ret = PTR_ERR(sdhci_arasan->phy);
+            dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n");
+            goto clk_disable_all;
+        }
+
+        ret = phy_init(sdhci_arasan->phy);
+        if (ret < 0) {
+            dev_err(&pdev->dev, "phy_init err.\n");
+            goto clk_disable_all;
+        }
+
+        ret = phy_power_on(sdhci_arasan->phy);
+        if (ret < 0) {
+            dev_err(&pdev->dev, "phy_power_on err.\n");
+            goto err_phy_power;
+        }
+    }
+
      ret = sdhci_add_host(host);
      if (ret)
          goto err_pltfm_free;

@@ -190,7 +238,12 @@ static int sdhci_arasan_probe(struct platform_device

*pdev)
      return 0;

  err_pltfm_free:
+    if (!IS_ERR(sdhci_arasan->phy))
+        phy_power_off(sdhci_arasan->phy);
      sdhci_pltfm_free(pdev);
+err_phy_power:
+    if (!IS_ERR(sdhci_arasan->phy))

Looks like you are using sdhci_arasan after it has been free'd by
sdhci_pltfm_free().

Also there seem to be cases where sdhci_arasan_probe() can exit after
sdhci_pltfm_init() without calling sdhci_pltfm_free().

Good catch, will fix it.

quoted

quoted

+        phy_exit(sdhci_arasan->phy);
  clk_disable_all:
      clk_disable_unprepare(clk_xin);
  clk_dis_ahb:

@@ -205,6 +258,11 @@ static int sdhci_arasan_remove(struct

platform_device *pdev)
      struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
      struct sdhci_arasan_data *sdhci_arasan = pltfm_host->priv;

+    if (!IS_ERR(sdhci_arasan->phy)) {
+        phy_power_off(sdhci_arasan->phy);
+        phy_exit(sdhci_arasan->phy);
+    }
+

When you re-base, take care to keep this chunk above
sdhci_pltfm_unregister().

sure.

Thanks for reviewing it.

quoted

quoted

      clk_disable_unprepare(sdhci_arasan->clk_ahb);

      return sdhci_pltfm_unregister(pdev);