On Fri, 20 May 2011 12:02:59 +0200, Sebastian Andrzej Siewior wrote:
+/**
+ * struct usb_udc - describes one usb device controller
+ * @driver - the gadget driver pointer. For use by the class code
+ * @dev - the child device to the actual controller
+ * @gadget - the gadget. For use by the class code
+ * @list - for use by the udc class driver
+ * @name - a human friendly name representation
+ *
+ * This represents the internal data structure which is used by the
UDC-class
+ * to hold information about udc driver and gadget together.
+ */
+struct usb_udc {
+ struct usb_gadget_driver *driver;
+ struct usb_gadget *gadget;
+ struct device dev;
+ struct list_head list;
+};
So the idea is for all gadgets to use this framework, right? If that's
the case,
maybe it's better to just stash those fields inside usb_gadget structure?
Also, isn't it that UDC drivers store pointer to usb_gadget_driver
internally?
Because if so, then stashing the above fields inside the usb_gadget_driver
structure would allow UDC drivers to drop their own internal pointer to
usb_gadget_driver.
+int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget)
+{
+ struct usb_udc *udc;
+ int ret = -ENOMEM;
+
+ udc = kzalloc(sizeof(*udc), GFP_KERNEL);
+ if (!udc)
+ goto err1;
+
+ device_initialize(&udc->dev);
+ udc->dev.release = usb_udc_release;
+ udc->dev.class = udc_class;
+ udc->dev.parent = parent;
+ ret = dev_set_name(&udc->dev, "%s", kobject_name(&parent->kobj));
+ if (ret)
+ goto err2;
+
+ udc->gadget = gadget;
+
+ mutex_lock(&udc_lock);
mutex_lock_interruptible()? This applies to other places as well.
+ list_add_tail(&udc->list, &udc_list);
+ mutex_unlock(&udc_lock);
What if someone calls usb_gadget_register_driver() right about now?
I think that since you've converted udc_lock to mutex rather then spinlock,
it'd be better to extend the lock all the way to the end of the function.
On another note, it's bit of a pity that you replaced the spinlock with
mutex
but I see how that can be needed.
I remember I posted a version of this code which used a sipnlock and a fake
value of udc->driver to mark udc as being busy initialising.
+ ret = device_add(&udc->dev);
+ if (ret)
+ goto err3;
+
+ kobject_uevent(&udc->dev.kobj, KOBJ_ADD);
+ return 0;
+err3:
+ mutex_lock(&udc_lock);
+ list_del(&udc->list);
+ mutex_unlock(&udc_lock);
+
+err2:
+ put_device(&udc->dev);
+
+err1:
+ return ret;
+}
+void usb_del_gadget_udc(struct usb_gadget *gadget)
+{
+ struct usb_udc *udc = NULL;
+
+ dev_vdbg(gadget->dev.parent, "unregistering gadget\n");
+
+ mutex_lock(&udc_lock);
+ list_for_each_entry(udc, &udc_list, list) {
+ if (udc->gadget == gadget)
+ goto found;
+ }
The braces are not needed.
+ dev_err(gadget->dev.parent, "gadget not registered.\n");
+ goto out;
+found:
+
+ list_del(&udc->list);
+
+ if (udc->driver)
+ usb_gadget_remove_driver(udc);
+
+ kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
+ device_unregister(&udc->dev);
+out:
+ mutex_unlock(&udc_lock);
I think the mutex_unlock() can be put just after list_del().
+}
+int usb_gadget_register_driver(struct usb_gadget_driver *driver)
+{
+ struct usb_udc *udc = NULL;
+ int ret;
+
+ if (!driver || !driver->bind || !driver->setup)
+ return -EINVAL;
+
+ mutex_lock(&udc_lock);
+ list_for_each_entry(udc, &udc_list, list) {
+ /* For now we take the first one */
+ if (!udc->driver) {
+ pr_debug("using UDC '%s'\n", udc->gadget->name);
I don't think this pr_debug() is needed since first instruction at
"found" label is a dev_dbg().
+ goto found;
+ }
+ }
The outer { ... } are not needed.
+ pr_debug("couldn't find an available UDC\n");
+ return -ENODEV;
Mutex remains locked.
+found:
+ dev_dbg(&udc->dev, "registering UDC driver [%s]\n",
+ driver->function);
+
+ udc->driver = driver;
+ udc->dev.driver = &driver->driver;
+
+ ret = usb_gadget_start(udc->gadget, driver);
+ if (ret) {
+ dev_err(&udc->dev, "failed to start %s --> %d\n",
+ udc->driver->function, ret);
+ goto err1;
+ }
+
+#if 0
Could we just drop this from the initial patch?
+ /*
+ * in long term we try to move same/simmilar steps from every gadget's
+ * ->start() callback into here. ->start() should only change some bits
+ * in the HW if possible.
+ */
+ ret = driver->bind(udc->gadget);
+ if (ret) {
+ dev_err(&udc->dev, "bind to driver %s failed --> %d\n",
+ driver->function, ret);
+ goto err2;
+ }
+
+ ret = usb_gadget_connect(udc->gadget);
+ if (ret) {
+ dev_err(&udc->dev, "failed to start %s --> %d\n",
+ udc->gadget->name, ret);
+ goto err3;
+ }
+#endif
+ kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
+ mutex_unlock(&udc_lock);
+ return 0;
+
+#if 0
+err3:
+ driver->unbind(udc->gadget);
+
+err2:
+ usb_gadget_stop(udc->gadget);
+
+#endif
+err1:
+ udc->driver = NULL;
+ udc->dev.driver = NULL;
+ mutex_unlock(&udc_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_register_driver);
+
+int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
+{
+ struct usb_udc *udc = NULL;
+ int ret = -ENODEV;
+
+ if (!driver || !driver->unbind)
+ return -EINVAL;
+
+ mutex_lock(&udc_lock);
+ list_for_each_entry(udc, &udc_list, list) {
+ if (udc->driver == driver) {
+ usb_gadget_remove_driver(udc);
+ ret = 0;
+ break;
+ }
+ }
Outer { ... } not needed.
+ mutex_unlock(&udc_lock);
+ return ret;
+}
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@xxxxxxxxxx>-----ooO--(_)--Ooo--
--
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
