Hi again Leandro,
Leandro Dorileo wrote:
The hci_reg_* functions use the reg_base in hci_t structure to determine which port to write to/read from, this structure is used both for OHCI, UHCI and will be for EHCI, so doesn`t matter if the controller is UHCI or OHCI there will always be a reg_base, with that I thought I could move it to usb.h and reuse it.
Right, but my point is that the different HCIs may not have the same registers so just changing the names of the register acccess functions may not be very useful?
We are still assuming the first endpoint to be the control one. We may need to change the functions in usb.c with a depper adaptation to accommodate drivers for devices with more than a single control endpoint but for now endpoint[0] should work.
How is this array populated?
The array is populated based on interface_descriptor_t which reports the bNumEndpoints(number of endpoints) but before anything it does configure an endpoint[0].type = CONTROL.
Ok, but what about the endpoint address?
Is the array index guaranteed to always be the same as the endpoint address sent on the bus?
Or is the endpoint address for each endpoint stored in the struct?
Note there can be two endpoints with the same address. (IN and OUT)
My point is that the default pipe is the endpoint with address 0, so only if that is actually guaranteed by endpoint[0] then it's ok to use endpoint[0]. Otherwise I guess there should be a function to look up the default endpoint for the device.
@@ -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?)
Yes sorry, maybe wrong. It should be [0].
Hm, I don't know.
SET_FEATURE can be sent to either device (bmRequestType=0), interface or endpoint. What is endp? Is it explicitly documented to be the index of the endpoints array - or is it the endpoint address as described in the USB spec?
Also, possibly the function should accept an intf argument. The same issue with USB stack array index vs. actual number in hardware applies to interfaces. Interfaces have a number in hardware, and that's what I think applications calling libpayload should be using.
I realize this discussion is a little out of scope for your project, because it is about libpayload USB in general, but I still think it should be cleared up - I think libpayload USB will be more popular in the future thanks to added HCI support.
@@ -123,7 +123,7 @@ get_status (usbdev_t *dev, int intf, int rtype, int len, void *data)
- dev->controller->control (&dev->endpoints[intf], IN, sizeof (dr), &dr, len, data);
Here an interface number is suddenly used as index in the endpoints array. Please explain how that can be correct?
Same here.
Again, I'm not sure.
GET_STATUS can also be sent to device, interface or endpoint. The function should probably handle that somehow.
@@ -141,7 +142,7 @@ get_descriptor (usbdev_t *dev, unsigned char bmRequestType, int descType,
- if (dev->controller->control (ep, IN, sizeof (dr), &dr, 8, buf)) {
Where does this ep come from?
From the declaration you commented above. :)
Right! Thanks! ;)
@@ -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?
Yes,
Ok.
in fact it should be used in every call commented previously.
Well, see above..
@@ -201,7 +202,7 @@ clear_stall (endpoint_t *ep)
- dev->controller->control (ep, OUT, sizeof (dr), &dr, 0, 0);
Good.
Actually it isn't. CLEAR_FEATURE can be sent to device, interface or endpoint just like SET_FEATURE and GET_STATUS.
Thanks a lot for reviewing.
No problem.
Maybe the whole API should be changed to match devices with more than one control endpoint, but this is a first change.
Right. I think that's a good idea.
//Peter