Re: [V5 PATCH 26/26] usb: ehci: ehci-mv: add device tree support
[...]
> >> @@ -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.
Yes, you're right. I got myself confused about the logic.
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [V5 PATCH 26/26] usb: ehci: ehci-mv: add device tree support
2013/1/24 Mark Rutland :
> On Thu, Jan 24, 2013 at 06:38:50AM +, 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?
>
>> Signed-off-by: Chao Xie
>> Acked-by: Alan Stern
>> ---
>> 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
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -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.
>
>> +
>> + 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: sen
Re: [V5 PATCH 26/26] usb: ehci: ehci-mv: add device tree support
On Thu, Jan 24, 2013 at 06:38:50AM +, 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?
> Signed-off-by: Chao Xie
> Acked-by: Alan Stern
> ---
> 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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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.
> +
> + 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[V5 PATCH 26/26] usb: ehci: ehci-mv: add device tree support
All blocks are removed. Add the device tree support for ehci.
Signed-off-by: Chao Xie
Acked-by: Alan Stern
---
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
#include
#include
+#include
#include
#include
#include
@@ -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;
+
+ 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;
+
+ 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) {
+ struct mv_usb_platform_data *pdata = pdev->dev.platform_data;
+ int clk_i = 0;
+
+ /* no CONFIG_OF */
+ if (pdata == NULL) {
+ dev_err(&pdev->dev, "missing platform_data\n");
+ return -ENODEV;
+ }
+ ehci_mv->mode = pdata->mode;
+ ehci_mv->clknum = pdata->clknum;
+
+ size = sizeof(struct clk *) * ehci_mv->clknum;
+ ehci_mv->clk = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
+ if (ehci_mv->clk == NULL)
+ return -ENOMEM;
+ 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])) {
+ retval = PTR_ERR(ehci_mv->clk[clk_i]);
+ return retval;
+ }
}
+ } else if (retval < 0) {
+ dev_err(&pdev->dev, "error parse dt\n");
+ return retval;
