[V5 PATCH 26/26] usb: ehci: ehci-mv: add device tree support
From: mark.rutland@arm.com (Mark Rutland)
Date: 2013-01-24 11:16:03
On Thu, Jan 24, 2013 at 06:38:50AM +0000, Chao Xie wrote:
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 hunk ↗ jump to hunk
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?
+
+ 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 hunk ↗ jump to hunk
+ + 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.
Also, why not return immediately if mv_ehci_parse_dt fails?
[...]
Thanks,
Mark.