Hi Grant,
On Tue, 28 Sep 2010 19:01:28 +0900
Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:
...
> Looks pretty good. Comments below. Main comment is that with the
> recent changes in mainline, this no longer needs to be an
> of_platform_driver. It can be a plain old platform_driver instead.
> It gets registered as a platform_driver anyway, but of_platform_driver
> is a shim that adds a bit of overhead, so it is best to avoid it.
> I'll detail the changes you need to make in my comments below.
Thanks. I'll change to platform_driver. My reply below.
...
> > +{
> > + 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;
> > +
> > + pdata = match->data;
> > + if (!pdata) {
> > + memset(&data, 0, sizeof(data));
> > + pdata = &data;
> > + }
>
> This is bad behaviour. *match->data must not be modified in probe
> because multiple instances of the device can exist. The target of
> pdata is modified later in the probe routine.
>
> However, the above 4 lines can be removed entirely since none of the
> fsl_usb2_mph_dr_of_match entries actually set the data pointer. The
> local 'data' structure can be used directly.
A match entry for mpc5121 is added by the third patch. I'll fix
this so that only local 'data' structure is modified later in the
probe routine.
Thanks,
Anatolij
--
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
