Prev: Removing dead code
Next: iBFT features for v2.6.36
From: Uwe Kleine-König on 2 Aug 2010 14:30
Hi Michał,

On Mon, Aug 02, 2010 at 04:25:34PM +0200, Michał Nazarewicz wrote:
> Some random thoughts, one bug and mostly just minor comments:
>
>> @@ -1954,13 +1954,14 @@ static int setup_ep0(struct udc *dev)
>> }
>> /* Called by gadget driver to register itself */
>> -int usb_gadget_register_driver(struct usb_gadget_driver *driver)
>> +int usb_gadget_probe_driver(struct usb_gadget_driver *driver,
>> + int (*bind)(struct usb_gadget *))
>> {
>> struct udc *dev = udc;
>> int retval;
>> u32 tmp;
>> - if (!driver || !driver->bind || !driver->setup
>> + if (!driver || bind || !driver->setup
>
>
> ** BUG: Should read "!bind".
Yeah, Julia already pointed that out.

>> diff --git a/drivers/usb/gadget/at91_udc.c b/drivers/usb/gadget/at91_udc.c
>> @@ -1612,7 +1613,7 @@ int usb_gadget_register_driver (struct usb_gadget_driver *driver)
>> DBG("bound to %s\n", driver->driver.name);
>> return 0;
>> }
>> -EXPORT_SYMBOL (usb_gadget_register_driver);
>> +EXPORT_SYMBOL (usb_gadget_probe_driver);
>
> How about also correcting space before "("?
Fixed.

>> diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
>> @@ -1789,7 +1789,8 @@ out:
>> return IRQ_HANDLED;
>> }
>> -int usb_gadget_register_driver(struct usb_gadget_driver *driver)
>> +int usb_gadget_probe_driver(struct usb_gadget_driver *driver,
>> + int (*bind)(struct usb_gadget *))
>> {
>> struct usba_udc *udc = &the_udc;
>> unsigned long flags;
>
> There was no checking here? How about adding?
Hmmm, I think this should be addressed in a different patch.

>> diff --git a/drivers/usb/gadget/ci13xxx_udc.c b/drivers/usb/gadget/ci13xxx_udc.c
>> @@ -2340,12 +2340,13 @@ static const struct usb_ep_ops usb_ep_ops = {
>> static const struct usb_gadget_ops usb_gadget_ops;
>> /**
>> - * usb_gadget_register_driver: register a gadget driver
>> + * usb_gadget_probe_driver: register a gadget driver
>> *
>> - * Check usb_gadget_register_driver() at "usb_gadget.h" for details
>> - * Interrupts are enabled here
>> + * Check usb_gadget_probe_driver() at "usb_gadget.h" for details.
>> + * Interrupts are enabled here.
>> */
>
> usb_gadget.h is the old name. How about correcting it as well?
done.

>
>> diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
>> @@ -3583,7 +3582,7 @@ static int __init fsg_init(void)
>> if ((rc = fsg_alloc()) != 0)
>> return rc;
>> fsg = the_fsg;
>> - if ((rc = usb_gadget_register_driver(&fsg_driver)) != 0)
>> + if ((rc = usb_gadget_probe_driver(&fsg_driver, fsg_bind)) != 0)
>
> I'm tempted to propose:
>
> + rc = usb_gadget_probe_driver(&fsg_driver, fsg_bind);
> + if (rc != 0)
>
> which is more compatible with coding style but it probably would be inconsistent
> with the rest of the code.
>
>> diff --git a/drivers/usb/gadget/langwell_udc.c b/drivers/usb/gadget/langwell_udc.c
>> @@ -1807,7 +1807,8 @@ static DEVICE_ATTR(langwell_udc, S_IRUGO, show_langwell_udc, NULL);
>> * the driver might get unbound.
>> */
>> -int usb_gadget_register_driver(struct usb_gadget_driver *driver)
>> +int usb_gadget_probe_driver(struct usb_gadget_driver *driver,
>> + int (*bind)(struct usb_gadget *))
>> {
>> struct langwell_udc *dev = the_controller;
>> unsigned long flags;
>
> Again, function has no checking, how about adding?
as above, IMHO this should be a seperate patch.

>> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
>> @@ -1698,7 +1698,8 @@ void musb_gadget_cleanup(struct musb *musb)
>> * @param driver the gadget driver
>> * @return <0 if error, 0 if everything is fine
>> */
>
> I've just noticed that it misses @param bind in the comment. Would be great to update it
> and all other occurrences.
done.

>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> @@ -798,17 +797,18 @@ struct usb_gadget_driver {
>> */
>> /**
>> - * usb_gadget_register_driver - register a gadget driver
>> - * @driver:the driver being registered
>> + * usb_gadget_probe_driver - probe a gadget driver
>> + * @driver: the driver being registered
>
> + * @bind: the driver's bind callback.
>
>> * Context: can sleep
>> *
>> * Call this in your gadget driver's module initialization function,
>> * to tell the underlying usb controller driver about your driver.
>> - * The driver's bind() function will be called to bind it to a
>> - * gadget before this registration call returns. It's expected that
>> - * the bind() functions will be in init sections.
>> + * The bind() function will be called to bind it to a gadget before this
>> + * registration call returns. It's expected that the bind() function will
>
> Maybe "the @bind function" in those two places?
>
> So for what it's worth, I haven't noticed any other obvious problems.
>
> I think it still does not fix all the section mismatch warnings -- would have to look
> closer at composite gadgets -- so I think still parts of my patch is legit.
I prepared a patch that removes bind from usb_configuration, too. Find
it in reply to this mail.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Uwe Kleine-König on 3 Aug 2010 03:50
[stripped down Cc: list to annoy fewer people]

On Mon, Aug 02, 2010 at 03:54:40PM -0700, Greg KH wrote:
> On Mon, Aug 02, 2010 at 03:12:48PM +0200, Uwe Kleine-K�nig wrote:
> > On Mon, Aug 02, 2010 at 02:51:36PM +0200, Julia Lawall wrote:
> > > > diff --git a/drivers/usb/gadget/amd5536udc.c b/drivers/usb/gadget/amd5536udc.c
> > > > index 731150d..c266c1e 100644
> > > > --- a/drivers/usb/gadget/amd5536udc.c
> > > > +++ b/drivers/usb/gadget/amd5536udc.c
> > > > @@ -1954,13 +1954,14 @@ static int setup_ep0(struct udc *dev)
> > > > }
> > > >
> > > > /* Called by gadget driver to register itself */
> > > > -int usb_gadget_register_driver(struct usb_gadget_driver *driver)
> > > > +int usb_gadget_probe_driver(struct usb_gadget_driver *driver,
> > > > + int (*bind)(struct usb_gadget *))
> > > > {
> > > > struct udc *dev = udc;
> > > > int retval;
> > > > u32 tmp;
> > > >
> > > > - if (!driver || !driver->bind || !driver->setup
> > > > + if (!driver || bind || !driver->setup
> > >
> > > I have the impression that this should be !bind rather than bind.
> > ah, now I see what you meant. Obviously you're right. I fixed it up
> > locally here for now. Greg, just tell me when/if you need a fixed
> > patch.
>
> Didn't you already post a "fixed" patch?
Yes, I did:

http://article.gmane.org/gmane.linux.ports.arm.kernel/86640
http://article.gmane.org/gmane.linux.kernel/1017575

If you append '/raw' to the URL they should be applicable by
git/quilt/whatever you use.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K�nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Uwe Kleine-König on 5 Aug 2010 05:50
On Thu, Aug 05, 2010 at 10:58:52AM +0200, Michał Nazarewicz wrote:
> On Mon, 02 Aug 2010 20:27:45 +0200, Uwe Kleine-König <u.kleine-koenig(a)pengutronix.de> wrote:
>> diff --git a/drivers/usb/gadget/s3c2410_udc.c b/drivers/usb/gadget/s3c2410_udc.c
>> @@ -2045,7 +2045,6 @@ static void __exit udc_exit(void)
>> }
>> EXPORT_SYMBOL(usb_gadget_unregister_driver);
>> -EXPORT_SYMBOL(usb_gadget_register_driver);
>
> *IF* you are going to send an updated patch, how about removing
> empty line above this EXPORT_SYMBOL().
I kept that, because the '}' doesn't belong to
usb_gadget_unregister_driver.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: Removing dead code
Next: iBFT features for v2.6.36