Re: [V5 PATCH 24/26] usb: gadget: mv_udc: add device tree support

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

 



On Fri, Jan 25, 2013 at 02:13:54AM +0000, chao xie wrote:
> 2013/1/24 Mark Rutland <mark.rutland@xxxxxxx>:
> > Hello,
> >
> > On Thu, Jan 24, 2013 at 06:38:48AM +0000, Chao Xie wrote:
> >> In original driver, we have callbacks in platform data, and some
> >> dynamically allocations variables in platform data.
> >> Now, these blocks are removed, the device tree support is easier
> >> now.
> >
> > Please could you also add a binding document? See
> > Documentation/devicetree/bindings/usb for examples of existing bindings.
> >
> > Also, please Cc devicetree-discuss when introducing a new binding as you are
> > doing here.
> >
> >>
> >> Signed-off-by: Chao Xie <chao.xie@xxxxxxxxxxx>
> >> ---
> >>  drivers/usb/gadget/mv_udc.h      |    5 +-
> >>  drivers/usb/gadget/mv_udc_core.c |  106 ++++++++++++++++++++++++++++++--------
> >>  2 files changed, 86 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/usb/gadget/mv_udc.h b/drivers/usb/gadget/mv_udc.h
> >> index 50ae7c7..de84722 100644
> >> --- a/drivers/usb/gadget/mv_udc.h
> >> +++ b/drivers/usb/gadget/mv_udc.h
> >> @@ -179,6 +179,7 @@ struct mv_udc {
> >>       int                             irq;
> >>
> >>       unsigned int                    extern_attr;
> >> +     unsigned int                    mode;
> >>       struct notifier_block           notifier;
> >>
> >>       struct mv_cap_regs __iomem      *cap_regs;
> >> @@ -222,11 +223,9 @@ struct mv_udc {
> >>       struct mv_usb2_phy      *phy;
> >>       struct usb_phy          *transceiver;
> >>
> >> -     struct mv_usb_platform_data     *pdata;
> >> -
> >>       /* some SOC has mutiple clock sources for USB*/
> >>       unsigned int    clknum;
> >> -     struct clk      *clk[0];
> >> +     struct clk      **clk;
> >>  };
> >>
> >>  /* endpoint data structure */
> >> diff --git a/drivers/usb/gadget/mv_udc_core.c b/drivers/usb/gadget/mv_udc_core.c
> >> index 2e5907f..a4ee9a1 100644
> >> --- a/drivers/usb/gadget/mv_udc_core.c
> >> +++ b/drivers/usb/gadget/mv_udc_core.c
> >> @@ -34,6 +34,7 @@
> >>  #include <linux/irq.h>
> >>  #include <linux/platform_device.h>
> >>  #include <linux/clk.h>
> >> +#include <linux/of.h>
> >>  #include <linux/platform_data/mv_usb.h>
> >>  #include <linux/usb/mv_usb2.h>
> >>  #include <asm/unaligned.h>
> >> @@ -2153,21 +2154,57 @@ static int mv_udc_remove(struct platform_device *pdev)
> >>       return 0;
> >>  }
> >>
> >> +static int mv_udc_parse_dt(struct platform_device *pdev,
> >> +                             struct mv_udc *udc)
> >> +{
> >> +     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;
> >> +
> >> +     udc->clk = devm_kzalloc(&pdev->dev,
> >> +             sizeof(struct clk *) * clks_num, GFP_KERNEL);
> >> +     if (udc->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;
> >> +             udc->clk[i] = devm_clk_get(&pdev->dev, clk_name);
> >> +             if (IS_ERR(udc->clk[i]))
> >> +                     return PTR_ERR(udc->clk[i]);
> >
> > I was going to ask if you couldn't use of_clk_get, but I see you want to use
> > the devm_* functions to handle cleanup. It seems a shame there's not a standard
> > devm_of_clk_get.
> >
> It is nice to have someone to review the device tree part patches.
> In fact main target of the modification is to remove the
> pointers/callbacks in the platform_data, so
> i can easly to add device tree support.
> 
> of_clk_get is is based on CONFIG_COMMON_CLK. of_clk_get is not simple.
> The orginal way we use to
> get a clock used two arguments: dev_id and con_id. The dev_id is the
> name of the device.
> of_clk_get need clock framework changes. It means that clock driver
> need add device tree support. Now
> we are doing the clock tree support for Marvell MMP SOCes, but it will
> not be done in a short time.
> As i think, of_clk_get still have some problems. It parses the
> property's name as "clocks", but it does not support
> mutipile clocks. If the device driver original has two clocks, the old
> way can do it as
> clk_get(dev, "clock 1");
> clk_get(dev, "clock 2");
> of_clk_get can not do it. I have not asked this question in the
> mailist, and i will do it soon.

I may have misunderstood here, but as far as I can see, of_clk_get supports
multiple clocks -- it even takes an index parameter:

struct clk *of_clk_get(struct device_node *np, int index)

I can see several dts files which have a clocks property with multiple clocks.
In arch/arm/boot/dts/vexpress-v2m.dtsi there's a "arm,sp810" node with
multiple clocks, which I believe is dealt with by vexpress_clk_of_init in
drivers/clk/versatile/clk-vexpress.c (though this uses of_clk_get_by_name).

This also raises the issue that you expect the clocks property to be a list of
strings, while other bindings expect the clocks property to be list of
phandle/clock-specifier pairs.

It's worth taking a look at:
Documentation/devicetree/bindings/clock/clock-bindings.txt

Going against the common convention here is a very bad idea.

> 
> >> +     }
> >> +
> >> +     udc->clknum = clks_num;
> >> +
> >> +     ret = of_property_read_u32(np, "extern_attr", &udc->extern_attr);
> >> +     if (ret)
> >> +             return ret;
> >
> > This looks like a *very* bad idea. The udc::extern_attr field seems to be a set
> > of flags, which this could reads in directly (without any sanity checking),
> > effectively making it an undocumented ABI. This might be better as a set of
> > valueless properties.
> >
> > Will unsigned int be the same as u32 on all platforms this could be used on?
> > If not, you're passing the wrong type into of_property_read_u32.
> >
> > Additionally, in devicetree, '-' is used in preference of '_' in compatible
> > and property names.
> >
> I see. So what you mean is if the extern_attr has two flags, FLAG_A and FLAG_B,
> i need add two boolean properties Property_FLAG_A and Property_FLAG_B?

Something like that. The properties might not map directly to flags - it's
better to describe the hardware reason these flags are required rather than
what these falgs do to linux.

> 
> >> +
> >> +     ret = of_property_read_u32(np, "mode", &udc->mode);
> >> +     if (ret)
> >> +             return ret;
> >
> > If I've understood correctly, this property will either be MV_USB_MODE_OTG (0)
> > or MV_USB_MODE_OTG (1). Again, I don't think this is good to be exported as an
> > ABI, especially as nothing in the enum definition points out that this affects
> > anything outside the kernel.
> >
> > If this isn't something that wants to be changed at runtime, this should
> > probably be a string property, with "host" and "otg" as valid values. Looking
> > in Documentation/devicetree, the nvidia,tegra20-ehci binding uses similar
> > strings in its dr_mode property. There may be other (undocumented) precedents.
> >
> Thanks. I will check the examples, and change it.

:)

> 
> >> +
> >> +     return 0;
> >> +}
> >> +
> >>  static int mv_udc_probe(struct platform_device *pdev)
> >>  {
> >> -     struct mv_usb_platform_data *pdata = pdev->dev.platform_data;
> >>       struct mv_udc *udc;
> >>       int retval = 0;
> >> -     int clk_i = 0;
> >>       struct resource *r;
> >>       size_t size;
> >>
> >> -     if (pdata == NULL) {
> >> -             dev_err(&pdev->dev, "missing platform_data\n");
> >> -             return -ENODEV;
> >> -     }
> >> -
> >> -     size = sizeof(*udc) + sizeof(struct clk *) * pdata->clknum;
> >> +     size = sizeof(*udc);
> >>       udc = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
> >>       if (udc == NULL) {
> >>               dev_err(&pdev->dev, "failed to allocate memory for udc\n");
> >> @@ -2175,14 +2212,43 @@ static int mv_udc_probe(struct platform_device *pdev)
> >>       }
> >>
> >>       udc->done = &release_done;
> >> -     udc->extern_attr = pdata->extern_attr;
> >> -     udc->pdata = pdev->dev.platform_data;
> >>       spin_lock_init(&udc->lock);
> >>
> >>       udc->dev = pdev;
> >>
> >> +     retval = mv_udc_parse_dt(pdev, udc);
> >> +     if (retval > 0) {
> >> +             /* no CONFIG_OF */
> >> +             struct mv_usb_platform_data *pdata = pdev->dev.platform_data;
> >> +             int clk_i = 0;
> >> +
> >> +             if (pdata == NULL) {
> >> +                     dev_err(&pdev->dev, "missing platform_data\n");
> >> +                     return -ENODEV;
> >> +             }
> >> +             udc->extern_attr = pdata->extern_attr;
> >> +             udc->mode = pdata->mode;
> >> +             udc->clknum = pdata->clknum;
> >> +
> >> +             size = sizeof(struct clk *) * udc->clknum;
> >> +             udc->clk = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
> >> +             if (udc->clk == NULL)
> >> +                     return -ENOMEM;
> >> +             for (clk_i = 0; clk_i < udc->clknum; clk_i++) {
> >> +                     udc->clk[clk_i] = devm_clk_get(&pdev->dev,
> >> +                                             pdata->clkname[clk_i]);
> >> +                     if (IS_ERR(udc->clk[clk_i])) {
> >> +                             retval = PTR_ERR(udc->clk[clk_i]);
> >> +                             return retval;
> >
> > Why not just return PTR_ERR(udc->clk[clk_i]); ?
> >
> sure. i will change it.
> 
> >> +                     }
> >> +             }
> >> +     } else if (retval < 0) {
> >> +             dev_err(&pdev->dev, "error parse dt\n");
> >> +             return retval;
> >> +     }
> >> +
> >>  #ifdef CONFIG_USB_OTG_UTILS
> >> -     if (pdata->mode == MV_USB_MODE_OTG) {
> >> +     if (udc->mode == MV_USB_MODE_OTG) {
> >>               udc->transceiver = devm_usb_get_phy(&pdev->dev,
> >>                                       USB_PHY_TYPE_USB2);
> >>               if (IS_ERR_OR_NULL(udc->transceiver)) {
> >> @@ -2191,17 +2257,6 @@ static int mv_udc_probe(struct platform_device *pdev)
> >>               }
> >>       }
> >>  #endif
> >> -
> >> -     udc->clknum = pdata->clknum;
> >> -     for (clk_i = 0; clk_i < udc->clknum; clk_i++) {
> >> -             udc->clk[clk_i] = devm_clk_get(&pdev->dev,
> >> -                                     pdata->clkname[clk_i]);
> >> -             if (IS_ERR(udc->clk[clk_i])) {
> >> -                     retval = PTR_ERR(udc->clk[clk_i]);
> >> -                     return retval;
> >> -             }
> >> -     }
> >> -
> >>       r = platform_get_resource(udc->dev, IORESOURCE_MEM, 0);
> >>       if (r == NULL) {
> >>               dev_err(&pdev->dev, "no I/O memory resource defined\n");
> >> @@ -2466,6 +2521,12 @@ static void mv_udc_shutdown(struct platform_device *pdev)
> >>       mv_udc_disable(udc);
> >>  }
> >>
> >> +static struct of_device_id mv_udc_dt_ids[] = {
> >> +     { .compatible = "mrvl,mv-udc" },
> >
> > This compatible string should be listed in the binding document you generate to
> > accompany this.
> >
> I see. i will add the documents at the device tree directory.
> I will seperate the device tree part patches from the other patches. Thanks.

Great!

Thanks,
Mark.

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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux