On Thu, 1 Aug 2013, Felipe Balbi wrote:
> Hi folks,
>
> as we all know naming conventions are fragile and easy to break. We've
> had weird endpoint naming conventions for far too long in the gadget
> framework.
>
> I'm trying to come up with means to get rid of that and, one of the
> ideas, was to add transfer support flags to our struct usb_ep which gets
> initialized by the UDC driver. Then ep_matches() can use those flags to
> check if it should return that endpoint or not.
The endpoint naming convention currently determines type and direction.
It works okay for simple cases but not for more complicated ones. For
example, it can't handle endpoints that support bulk or interrupt but
not isochronous. If you really want to make this general, the way to
do it is to have separate bitflags for: control, bulk-in, bulk-out, ...
For additional restrictions, I agree that a standardized system is
needed.
> The ***UNFINISHED*** patch below does just that and shows an example of
> how to initialize such flags on dwc3. Please go over it and let me know
> what you guys think.
>
> From: Felipe Balbi <balbi@xxxxxx>
>
> usb: gadget: add transfer support flags for struct usb_ep
>
> NYET-Signed-off-by: Felipe Balbi <balbi@xxxxxx>
> ---
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index f168eae..0bc3621 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1655,6 +1655,9 @@ static int dwc3_gadget_init_hw_endpoints(struct dwc3 *dwc,
> if (epnum == 0 || epnum == 1) {
> dep->endpoint.maxpacket = 512;
> dep->endpoint.maxburst = 1;
> +
> + dep->endpoint.supports_control = true;
Does DWC3 really support control transfers on two endpoints? I have
never heard of a USB device with a control endpoint other than ep0.
> diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
> index a777f7b..bf72538 100644
> --- a/drivers/usb/gadget/epautoconf.c
> +++ b/drivers/usb/gadget/epautoconf.c
> @@ -57,6 +57,27 @@ ep_matches (
> if (NULL != ep->driver_data)
> return 0;
>
> + /* first try with transfer support flags */
> + type = desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;
> + switch (type) {
> + case USB_ENDPOINT_XFER_CONTROL:
> + if (ep->supports_control)
> + return 0;
> + break;
> + case USB_ENDPOINT_XFER_BULK:
> + if (ep->supports_bulk)
> + return 0;
> + break;
> + case USB_ENDPOINT_XFER_INT:
> + if (ep->supports_interrupt)
> + return 0;
> + break;
> + case USB_ENDPOINT_XFER_ISOC:
> + if (ep->supports_isochronous)
> + return 0;
> + break;
> + }
The tests here are backwards. Returning 0 means the endpoint _doesn't_
match the requirements. So for example, the first test above should be
"if (!ep->supports_control)".
You also have to handle UDC drivers that don't define the transfer
support flags.
Alan Stern
--
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
