Date:	Mon, 2 Aug 2010 20:25:21 +0200
From:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
To:	Michał Nazarewicz <m.nazarewicz@...sung.com>
Cc:	linux-usb@...r.kernel.org, David Brownell <david-b@...bell.net>,
	Julia Lawall <julia@...u.dk>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb gadget: don't save bind callback in struct
	usb_gadget_driver

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists