Hi Alan,
Alan Stern a écrit :
> On Tue, 15 Mar 2011, Matthieu CASTET wrote:
>
>> Even if the usb gadget framework is limited to work with one driver, it
>> could be useful to have a kernel build with more than driver.
>> This allow to make generic kernel that work with different udc controller.
>>
>> The only blocker to do that is usb_gadget_register_driver
>> and usb_gadget_unregister_driver function are declared in each driver.
>>
>> For avoiding that a redirection is done for these functions :
>> At probe time the driver register them (usb_gadget_register and
>> usb_gadget_unregister), and the generic usb_gadget_register_driver and
>> usb_gadget_unregister_driver call these callback.
>> We pass struct *usb_gadget in usb_gadget_register and usb_gadget_unregister
>> for flexibility (we can latter do a more complex dispatcher).
>>
>> Signed-off-by: Matthieu CASTET <matthieu.castet@xxxxxxxxxx>
>> Ack-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
>
> Actually, there are a few things in this patch which should be
> improved. See below...
>
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -68,7 +68,7 @@ obj-$(CONFIG_USB_OTG_UTILS) += usb/otg/
>> obj-$(CONFIG_USB) += usb/
>> obj-$(CONFIG_USB_MUSB_HDRC) += usb/musb/
>> obj-$(CONFIG_PCI) += usb/
>> -obj-$(CONFIG_USB_GADGET) += usb/gadget/
>> +obj-y += usb/gadget/
>
> Why was this changed? And why doesn't the spacing match the lines
> above?
That was to make core_udc always build in kernel (not a module).
But it should be possible to make it a module.
>
>> +{
>> + if (!gadget->udc ||
>> + !gadget->udc->probe_driver || !gadget->udc->unregister_driver)
>> + return -EINVAL;
>> +
>> + if (usb_gadget_udc)
>> + return -EBUSY;
>> +
>> + usb_gadget_udc = gadget;
>
> In theory this should be protected by a mutex. But it probably doesn't
> matter.
Are there any arch where this is not atomic ?
I could use a local_irq_save/local_irq_restore if needed.
A mutex seems overkill.
>
>> +
>> + return device_register(&gadget->dev);
>
> You can't do this. If device_register() fails, you have to clean up by
> calling put_device() -- but the caller has no way of knowing what to
> do. Therefore you have to check the return code from device_register()
> and clean up here before returning.
Ok.
>> +{
>> + if (!usb_gadget_udc || usb_gadget_udc != gadget)
>> + return;
>> +
>> + usb_gadget_udc = NULL;
>> +
>> + device_unregister(&gadget->dev);
>
> What happens if a gadget driver is currently registered for this
> gadget? It will have no way to unregister, because
> usb_gadget_unregister_driver() will return -ENODEV without doing
> anything.
That an interesting problem.
Before symbol deps on usb_gadget_probe_driver make sure udc driver wasn't
unloaded before gadget driver.
We can use try_module_get in usb_gadget_probe_driver and module_put in
usb_gadget_unregister_driver to make sure the udc driver don't call
usb_gadget_unregister before usb_gadget_unregister_driver.
The struct usb_gadget_udc will look like :
struct usb_gadget_udc {
struct module *owner;
int (*probe_driver) (struct usb_gadget_driver *driver, int (*bind)(struct usb_gadget *), struct usb_gadget *gadget);
int (*unregister_driver) (struct usb_gadget_driver *driver, struct usb_gadget *gadget);
};
Matthieu
--
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
