2013/1/24 Mark Rutland [off-list ref]:

On Thu, Jan 24, 2013 at 06:38:50AM +0000, Chao Xie wrote:

quoted

All blocks are removed. Add the device tree support for ehci.

Similarly to the last two patches, could you please add a binding document?

quoted

Signed-off-by: Chao Xie <redacted>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
---
 drivers/usb/host/ehci-mv.c |  105 +++++++++++++++++++++++++++++++++----------
 1 files changed, 80 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/host/ehci-mv.c b/drivers/usb/host/ehci-mv.c
index 171e145..79eca0c 100644
--- a/drivers/usb/host/ehci-mv.c
+++ b/drivers/usb/host/ehci-mv.c

@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/clk.h>
+#include <linux/of.h>
 #include <linux/err.h>
 #include <linux/usb/otg.h>
 #include <linux/usb/mv_usb2.h>

@@ -32,11 +33,9 @@ struct ehci_hcd_mv {

      struct usb_phy *otg;

-     struct mv_usb_platform_data *pdata;
-
      /* clock source and total clock number */
      unsigned int clknum;
-     struct clk *clk[0];
+     struct clk **clk;
 };

 static void ehci_clock_enable(struct ehci_hcd_mv *ehci_mv)

@@ -138,22 +137,55 @@ static const struct hc_driver mv_ehci_hc_driver = {
      .bus_resume = ehci_bus_resume,
 };

+static int mv_ehci_parse_dt(struct platform_device *pdev,
+                             struct ehci_hcd_mv *ehci_mv)
+{
+     struct device_node *np = pdev->dev.of_node;
+     unsigned int clks_num;
+     int i, ret;
+     const char *clk_name;
+
+     if (!np)
+             return 1;

Perhaps -ENODEV?

quoted

+
+     clks_num = of_property_count_strings(np, "clocks");
+     if (clks_num < 0)
+             return clks_num;
+
+     ehci_mv->clk = devm_kzalloc(&pdev->dev,
+             sizeof(struct clk *) * clks_num, GFP_KERNEL);
+     if (ehci_mv->clk == NULL)
+             return -ENOMEM;
+
+     for (i = 0; i < clks_num; i++) {
+             ret = of_property_read_string_index(np, "clocks", i,
+                     &clk_name);
+             if (ret)
+                     return ret;
+             ehci_mv->clk[i] = clk_get(NULL, clk_name);
+             if (IS_ERR(ehci_mv->clk[i]))
+                     return PTR_ERR(ehci_mv->clk[i]);
+     }
+
+     ehci_mv->clknum = clks_num;
+
+     ret = of_property_read_u32(np, "mode", &ehci_mv->mode);
+     if (ret)
+             return ret;

Again, this is a bad idea.

quoted

+
+     return 0;
+}
+
 static int mv_ehci_probe(struct platform_device *pdev)
 {
-     struct mv_usb_platform_data *pdata = pdev->dev.platform_data;
      struct usb_hcd *hcd;
      struct ehci_hcd *ehci;
      struct ehci_hcd_mv *ehci_mv;
      struct resource *r;
-     int clk_i, retval = -ENODEV;
+     int retval = -ENODEV;
      u32 offset;
      size_t size;

-     if (!pdata) {
-             dev_err(&pdev->dev, "missing platform_data\n");
-             return -ENODEV;
-     }
-
      if (usb_disabled())
              return -ENODEV;

@@ -161,7 +193,7 @@ static int mv_ehci_probe(struct platform_device *pdev)
      if (!hcd)
              return -ENOMEM;

-     size = sizeof(*ehci_mv) + sizeof(struct clk *) * pdata->clknum;
+     size = sizeof(*ehci_mv);
      ehci_mv = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
      if (ehci_mv == NULL) {
              dev_err(&pdev->dev, "cannot allocate ehci_hcd_mv\n");

@@ -170,19 +202,36 @@ static int mv_ehci_probe(struct platform_device *pdev)
      }

      platform_set_drvdata(pdev, ehci_mv);
-     ehci_mv->pdata = pdata;
      ehci_mv->hcd = hcd;

-     ehci_mv->clknum = pdata->clknum;
-     for (clk_i = 0; clk_i < ehci_mv->clknum; clk_i++) {
-             ehci_mv->clk[clk_i] =
-                 devm_clk_get(&pdev->dev, pdata->clkname[clk_i]);
-             if (IS_ERR(ehci_mv->clk[clk_i])) {
-                     dev_err(&pdev->dev, "error get clck \"%s\"\n",
-                             pdata->clkname[clk_i]);
-                     retval = PTR_ERR(ehci_mv->clk[clk_i]);
-                     goto err_clear_drvdata;
+     retval = mv_ehci_parse_dt(pdev, ehci_mv);
+     if (retval > 0) {

Is this why you returned 1 from mv_ehci_parse_dt? So you only do this if you
don't have a dt node?

If so, why not rip the check (and positive error code) out of mv_ehci_parse_dt,
and then here:

if (pdev->dev.of_node) {
        retval = mv_ehci_parse_dt(pdev, ehci_mv);
} else
        fall back to mv_usb_platform_data ...
}

That makes it obvious that one side depends on dt and the other's a fallback,
and doesn't rely on nonstandard return code behaviour.

I will change it.

Also, why not return immediately if mv_ehci_parse_dt fails?

I do not understand. if mv_ehci_parse_dt returns negative value, the
probe will be finished immediately.

[...]

Thanks,
Mark.

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