* Michal Nazarewicz | 2011-05-20 20:28:59 [+0200]:
>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?
Well, yes. It does not make much sense to have two implementations.
Besides, since every gadget implements its own usb_gadget_probe_driver()
it is kinda difficult to maintain then in parallel.
>If
>that's the case,
>maybe it's better to just stash those fields inside usb_gadget structure?
I don't think so. This could lead to the access those members directly
and bypass the udc_class for what ever reasons.
>Also, isn't it that UDC drivers store pointer to usb_gadget_driver
>internally?
Just checked and I can't see it. The ops pointer is passed/set.
>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.
Correct me if I'm wrong, but I don't see it to be the case.
>>+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.
Why do we want to abort on signal?
>
>>+ list_add_tail(&udc->list, &udc_list);
>>+ mutex_unlock(&udc_lock);
>
>What if someone calls usb_gadget_register_driver() right about now?
It should not cause any harm. However, it might make sense to delay it
after the kobject_uevent().
>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.
okay.
>
>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 don't expect the lock to be contended. It is not taken in any atomic
context and I hope it stays that way :)
>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.
Why that?
>>+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.
yes, but it looks somehow naked since we have another function where we
omit the braces.
>>+ 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().
It think, it could but then it is no longer symetric with that part
where we lock the complete function.
>>+}
>
>
>>+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().
Yup.
>
>>+ goto found;
>>+ }
>>+ }
>
>The outer { ... } are not needed.
>
>>+ pr_debug("couldn't find an available UDC\n");
>>+ return -ENODEV;
>
>Mutex remains locked.
indeed.
>>+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?
sure.
>
>>+ /*
>>+ * 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;
>>+}
>
Sebastian
--
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
