On Wed, Jul 28, 2010 at 5:58 AM, Anatolij Gustschin <agust@xxxxxxx> wrote:
> Hi Grant,
>
> On Wed, 28 Jul 2010 02:16:08 -0600
> Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:
>
>> On Thu, Jul 22, 2010 at 10:25 AM, Anatolij Gustschin <agust@xxxxxxx> 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@xxxxxxx>
>> > Cc: Kumar Gala <galak@xxxxxxxxxxxxxxxxxxx>
>> > Cc: Grant Likely <grant.likely@xxxxxxxxxxxx>
>>
>> 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.
--
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
