[PATCH 2/3] USB: add of_platform glue driver for FSL USB DR controller

[Grant Likely]

On Wed, Jul 28, 2010 at 5:58 AM, Anatolij Gustschin <agust at denx.de> wrote:
> Hi Grant,
>
> On Wed, 28 Jul 2010 02:16:08 -0600
> Grant Likely <grant.likely at secretlab.ca> wrote:
>
>> On Thu, Jul 22, 2010 at 10:25 AM, Anatolij Gustschin <agust at denx.de> wrote:
>> > The driver creates platform devices based on the information
>> > from USB nodes in the flat device tree. This is the replacement
>> > for old arch fsl_soc usb code removed by the previous patch.
>> > It uses usual of-style binding, available EHCI-HCD and UDC
>> > drivers can be bound to the created devices. The new of-style
>> > driver additionaly instantiates USB OTG platform device, as the
>> > appropriate USB OTG driver will be added soon.
>> >
>> > Signed-off-by: Anatolij Gustschin <agust at denx.de>
>> > Cc: Kumar Gala <galak at kernel.crashing.org>
>> > Cc: Grant Likely <grant.likely at secretlab.ca>
>>
>> Hi Anatolij,
>>
>> Looks pretty good, but some comments below.
>
> Thanks for review and comments! Greg already merged this series and
> therefore I'll submit an incremental cleanup patch to address
> outstanding issues. My reply is below.

Greg maintains a patchwork tree IIRC.  I believe he drops patches from
his linux-next branch if they need to be reworked.  Greg, do I have
this right?

Also, my preference would be to see some 3rd party testing before
committing to having this merged.

>
> ...
>> > --- a/drivers/usb/host/Kconfig
>> > +++ b/drivers/usb/host/Kconfig
>> > @@ -112,10 +112,15 @@ config XPS_USB_HCD_XILINX
>> >                support both high speed and full speed devices, or high speed
>> >                devices only.
>> >
>> > +config USB_FSL_MPH_DR_OF
>> > +       bool
>> > +       depends on PPC_OF
>>
>> Drop the depends.  This is selected by USB_EHCI_FSL and
>> USB_GADGET_FSL_USB2, which are already PPC only.
>
> Okay, will remove it.
>
> ...
>> > +struct fsl_usb2_dev_data dr_mode_data[] __devinitdata = {
>> > +       {
>> > +               "host",
>> > +               { "fsl-ehci", NULL, NULL, },
>> > +               FSL_USB2_DR_HOST,
>> > +       },
>> > +       {
>> > +               "otg",
>> > +               { "fsl-ehci", "fsl-usb2-udc", "fsl-usb2-otg", },
>> > +               FSL_USB2_DR_OTG,
>> > +       },
>> > +       {
>> > +               "periferal",
>>
>> spelling.  "peripheral"
>
> Right, thanks for catching! Will fix it.
>
>>
>> > +               { "fsl-usb2-udc", NULL, NULL, },
>> > +               FSL_USB2_DR_DEVICE,
>> > +       },
>> > +};
>>
>> Program defensively.  Use the following form:
>> +       {
>> +               .dr_mode = "host",
>> +               .drivers = { "fsl-ehci", NULL, NULL, },
>> +               .op_mode = FSL_USB2_DR_HOST,
>> +       },
>
> I'll change it too as suggested.
>
> ...
>> > +struct fsl_usb2_dev_data * __devinit get_dr_mode_data(struct device_node *np)
>> > +{
>> > +       const unsigned char *prop;
>> > +       int i;
>> > +
>> > +       prop = of_get_property(np, "dr_mode", NULL);
>> > +       if (prop) {
>> > +               for (i = 0; i < ARRAY_SIZE(dr_mode_data); i++) {
>> > +                       if (!strcmp(prop, dr_mode_data[i].dr_mode))
>> > +                               return &dr_mode_data[i];
>> > +               }
>>
>> Print a warning if you get through the loop, but don't match anything.
>>  That means that dr_mode has a bad value.
>
> I'll add a warning here.
>
> ...
>> > +struct platform_device * __devinit
>> > +fsl_usb2_device_register(struct of_device *ofdev,
>> > +                        struct fsl_usb2_platform_data *pdata,
>> > +                        const char *name, int id)
>>
>> In general, it is better to have the function name on the same line as
>> the return arguements so that grep shows the relevant info.  Move the
>> arguments to subsequent lines.
>
> It will be fixed in a clean-up patch.
>
> ...
>> > +static int __devinit fsl_usb2_mph_dr_of_probe(struct of_device *ofdev,
>> > +                                          const struct of_device_id *match)
>> > +{
>> > +       struct device_node *np = ofdev->dev.of_node;
>> > +       struct platform_device *usb_dev;
>> > +       struct fsl_usb2_platform_data data, *pdata;
>> > +       struct fsl_usb2_dev_data *dev_data;
>> > +       const unsigned char *prop;
>> > +       static unsigned int idx;
>> > +       int i;
>> > +
>> > +       if (!of_device_is_available(np))
>> > +               return -ENODEV;
>>
>> What is this for?
>
> Anton already answered this question better than I could do.
>
>> > +
>> > +       pdata = match->data;
>> > +       if (!pdata) {
>>
>> The match table doesn't have any data, so this is a no-op.
>
> The next patch [PATCH 3/3] of the series adds the data to the
> match table.
>
> ...
>> > +       if (of_device_is_compatible(np, "fsl-usb2-mph")) {
>> > +               prop = of_get_property(np, "port0", NULL);
>> > +               if (prop)
>>
>> if (of_get_property())
>>
>> > +                       pdata->port_enables |= FSL_USB2_PORT0_ENABLED;
>> > +
>> > +               prop = of_get_property(np, "port1", NULL);
>> > +               if (prop)
>>
>> Ditto
>
> Okay, I'll clean this up.
>
> ...
>> > +static struct of_platform_driver fsl_usb2_mph_dr_driver = {
>> > +       .driver = {
>> > +               .name = "fsl-usb2-mph-dr",
>> > +               .owner = THIS_MODULE,
>> > +               .of_match_table = fsl_usb2_mph_dr_of_match,
>> > +       },
>> > +       .probe  = fsl_usb2_mph_dr_of_probe,
>> > +};
>>
>> No remove hook?
>
> Since the purpose of the driver is only creation of platform devices
> according to the selected dual role controller mode described in
> the device tree, I do not see much sense of making the driver a module
> and provide a remove hook for destruction of created devices.

It is still appropriate to have a remove function so the driver can be
unbound.  The remove function should unregister the platform devices
it created in .probe().

>
> ...
>> > +static int __init fsl_usb2_mph_dr_init(void)
>> > +{
>> > +       return of_register_platform_driver(&fsl_usb2_mph_dr_driver);
>> > +}
>> > +fs_initcall(fsl_usb2_mph_dr_init);
>>
>> Why fs_initcall?  Is module_init() not sufficient?
>
> No. Using module_init() here results in initializing the driver after
> loading FLS USB OTG and EHCI-FSL drivers and reverted probing: probe()
> in EHCI-FSL driver is called before probe() in OTG driver and doesn't
> find OTG transceiver resource which is set and exported by probe() in
> OTG driver. This breaks both USB host and device controller support
> if dual role controller is configured to operate in OTG mode.

This is still broken then, and it is just luck that it is even working
now.  The three sub-devices are registered in sequence which should
handle your ordering issues if you put the OTG driver at the start of
the list; but even that is rather hacky.  You need to make the
EHCI-FSL drivers defer initialization if it has to wait for the OTG
driver.  One option might be to use a bus notifier for this.

I won't nack this patch because of this problem because the problem
already exists in the current code, but it really does need to be
fixed.

g.