On Fri, 20 May 2011 12:02:59 +0200, Sebastian Andrzej Siewior wrote:
+struct usb_udc {
+ struct usb_gadget_driver *driver;
+ struct usb_gadget *gadget;
+ struct device dev;
+ struct list_head list;
+};
* Michal Nazarewicz | 2011-05-20 20:28:59 [+0200]:
maybe it's better to just stash those fields inside usb_gadget
structure?
On Mon, 23 May 2011 16:45:51 +0200, Sebastian Andrzej Siewior wrote:
I don't think so. This could lead to the access those members directly
and bypass the udc_class for what ever reasons.
I don't see it as a problem. UDC drivers have a driver pointer
anyway, and have no need to touch UDC class device nor the list.
Moreover, if we choose to just add UDC dev and list to usb_gadget,
some of the functions in the UDC class will be much simpler. Some of
the list traversals could be easily avoided.
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.
Three more or less random UDC drivers:
http://lxr.linux.no/linux+*/drivers/usb/gadget/s3c-hsotg.c#L147
http://lxr.linux.no/linux+*/drivers/usb/gadget/at91_udc.h#L130
http://lxr.linux.no/linux+*/drivers/usb/gadget/mv_udc.h#L165
and all store pointer to struct usb_gadget_driver.
Therefore, stashing fields of the usb_udc structure inside usb_gadget
structure will save us storing pointer to usb_gadget_driver two times.
+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?
Why don't we want to do that? If something hands, user should be able to
interrupt
operation with a signal, I don't see why she shouldn't.
+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.
Coding style requires the braces be dropped though.
http://lxr.linux.no/linux+*/Documentation/CodingStyle#L166
+ 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.
I'd still go with mutex_unlock() just after the list_del(). No need
to hold mutexes that we don't need.
--
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
