Peter, thank you for your review!
Peter Stuge schrieb:
Subject: [PATCH 3/5] usb: API change, control receive endpoint_t
Changed the usb API where the control function first parameter now is a pointer of endpoint_t instead of a pointer of usbdevice_t.
Changing the API like this is a good thing.
The old API was based on a misunderstanding of the spec on my part, and the little detail that so far the stack doesn't have drivers for devices with more than one control pipe. So yes, this is a very good thing! :-) (and necessary because OHCI does things in different ways than UHCI in that area)
@@ -109,7 +109,7 @@ set_feature (usbdev_t *dev, int endp, int feature, int rtype)
- dev->controller->control (&dev->endpoints[endp], OUT, sizeof (dr), &dr, 0, 0);
This is good. (Or is it? Is endp specified in the API to be the index, and not the endpoint number?)
There's no correlation between endpoints[x] and endpoint number x in the code. Simply because there can be two endpoints x(in) and x(out). The only one that's guaranteed is endpoints[0] which points to endpoint 0 - but that one is a special case as direction doesn't matter for it (according to the spec).
@@ -183,7 +184,7 @@ set_configuration (usbdev_t *dev)
- dev->controller->control (&dev->endpoints[0], OUT, sizeof (dr), &dr, 0, 0);
Is index 0 in endpoints guaranteed to always be the default endpoint?
drivers/usb/usb.c(set_address) initializes endpoints[0] to the default endpoint (type control, endpoint 0, proper maxPacketSize).
Given that this is the endpoint used to do basic initialization, I doubt any device vendor is stupid enough to move the default endpoint to somewhere else after that. And if there is such a device, the special device driver for that device could still change endpoints[0] easily.
Regards, Patrick