Re: [RFC/PATCH 2/2] usb:gadget: Add SuperSpeed support to the Gadget Framework

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Sun, Oct 3, 2010 at 1:32 PM, tlinder <tlinder@xxxxxxxxxxxxxx> wrote:
>
> From: Tatyana Linder <tlinder@xxxxxxxxxxxxxx>
>
> This patch adds the SuperSpeed functionality to the gadget framework.
> In order not to force all the FDs to supply SuperSpeed descriptors when
> operating in SuperSpeed mode the following approach was taken:
> If we're operating in SuperSpeed mode and the FD didn't supply SuperSpeed
> descriptors, the composite layer will automatically create SuperSpeed
> descriptors with default values.
> Support for new SuperSpeed BOS descriptor was added.
> Support for SET_FEATURE and GET_STATUS requests in SuperSpeed mode was
> added.
>
> Signed-off-by: Tatyana Linder <tlinder@xxxxxxxxxxxxxx>

Some comments on coding guidelines below.

> ---
> This patch was verified by USBCV 3.0 and 2.0.
>
>  drivers/usb/gadget/Kconfig     |   20 ++-
>  drivers/usb/gadget/composite.c |  319 +++++++++++++++++++++++++++++++++++++---
>  include/linux/usb/ch9.h        |   54 +++++++-
>  include/linux/usb/composite.h  |   24 +++
>  include/linux/usb/gadget.h     |   19 +++
>  5 files changed, 407 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index cd27f9b..9afdd08 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -520,11 +520,11 @@ config USB_GADGET_DUMMY_HCD
>          side is the master; the gadget side is the slave.  Gadget drivers
>          can be high, full, or low speed; and they have access to endpoints
>          like those from NET2280, PXA2xx, or SA1100 hardware.
> -
> +
>          This may help in some stages of creating a driver to embed in a
>          Linux device, since it lets you debug several parts of the gadget
>          driver without its hardware or drivers being involved.
> -
> +
>          Since such a gadget side driver needs to interoperate with a host
>          side Linux-USB device driver, this may help to debug both sides
>          of a USB protocol stack.
> @@ -552,6 +552,18 @@ config USB_GADGET_DUALSPEED
>          Means that gadget drivers should include extra descriptors
>          and code to handle dual-speed controllers.
>
> +config USB_GADGET_SUPERSPEED
> +       boolean "Gadget opperating in Super Speed"

spelling "operating"

> +       depends on USB_GADGET
> +       depends on USB_GADGET_DUALSPEED
> +       default n
> +       help
> +         Enabling this feature enables Super Speed support in the Gadget
> +         driver. It means that gadget drivers should include extra (SuperSpeed)
> +         descriptors.
> +         For composite devices: if SupeSpeed descriptors weren't supplied by

spelling "Superspeed"

> +         the FD, they will be automatically generated with default values.
> +
>  #
>  # USB Gadget Drivers
>  #
> @@ -633,7 +645,7 @@ config USB_ETH
>        help
>          This driver implements Ethernet style communication, in one of
>          several ways:
> -
> +
>           - The "Communication Device Class" (CDC) Ethernet Control Model.
>             That protocol is often avoided with pure Ethernet adapters, in
>             favor of simpler vendor-specific hardware, but is widely
> @@ -673,7 +685,7 @@ config USB_ETH_RNDIS
>           If you say "y" here, the Ethernet gadget driver will try to provide
>           a second device configuration, supporting RNDIS to talk to such
>           Microsoft USB hosts.
> -
> +
>           To make MS-Windows work with this, use Documentation/usb/linux.inf
>           as the "driver info file".  For versions of MS-Windows older than
>           XP, you'll need to download drivers from Microsoft's website; a URL
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index c619c80..a5dcfe1 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -70,6 +70,102 @@ static char *iSerialNumber;
>  module_param(iSerialNumber, charp, 0);
>  MODULE_PARM_DESC(iSerialNumber, "SerialNumber string");
>
> +/** Default endpoint companion descriptor */
> +static struct usb_ss_ep_comp_descriptor ep_comp_desc = {
> +               .bDescriptorType = USB_DT_SS_ENDPOINT_COMP,
> +               .bLength = 0x06,
> +               .bMaxBurst = 0, /*the default is we don't support bursting*/

Commenting style not proper. It should be like "/* Leave a space as shown */"

> +               .bmAttributes = 0, /*2^0 streams supported*/
> +               .wBytesPerInterval = 0,
> +};
> +
> +/**This function receives a pointer to usb_function and adds
> + * missing super speed descriptors in the ss_descriptor field
> + * according to its hs_descriptors field.
> + *
> + * In more details: this function copies f->hs_descriptors while
> + * updating the endpoint descriptor and adding endpoint
> + * companion descriptor
> + */
> +static void create_ss_descriptors(struct usb_function *f)
> +{
> +       unsigned bytes; /*number of bytes to allocate*/
> +       unsigned n_desc;    /* number of descriptors*/
> +       void *mem; /*allocated memory to copy to*/
> +       struct usb_descriptor_header **tmp;
> +       struct usb_endpoint_descriptor  *ep_desc ;
> +       struct usb_descriptor_header **src = f->hs_descriptors;
> +
> +       if (!f->hs_descriptors)
> +               return;
> +
> +       /* Count number of EPs (in order to know how many SS_EP_COMPANION
> +          descriptors to add), the total number of descriptors and the sum of
> +          each descriptor bLength field in order to know how much memory to
> +          allocate*/

Multi line comment style as per coding guidelines is as below.

/*
 * First line
 * Second line
 * Third line
 */


> +       for (bytes = 0, n_desc = 0, tmp = src; *tmp; tmp++, n_desc++) {
> +               if ((*tmp)->bDescriptorType == USB_DT_ENDPOINT) {
> +                       bytes += ep_comp_desc.bLength;
> +                       n_desc++;
> +               }
> +               bytes += (*tmp)->bLength;
> +       }
> +
> +       bytes += (n_desc + 1) * sizeof(*tmp);
> +       mem = kmalloc(bytes, GFP_KERNEL);
> +       if (!mem)
> +               return;
> +
> +       /* fill in pointers starting at "tmp",
> +        * to descriptors copied starting at "mem";
> +        * and return "ret"
> +        */

Same here.

> +       tmp = mem;
> +       f->ss_descriptors  = mem;
> +       mem += (n_desc + 1) * sizeof(*tmp);
> +       while (*src) {
> +               /*Copy the original descriptor*/
> +               memcpy(mem, *src, (*src)->bLength);
> +               switch ((*src)->bDescriptorType) {
> +               case USB_DT_ENDPOINT:
> +                       /*update ep descriptor*/
> +                       ep_desc = (struct usb_endpoint_descriptor *)mem;
> +                       switch (ep_desc->bmAttributes &
> +                               USB_ENDPOINT_XFERTYPE_MASK) {
> +                       case USB_ENDPOINT_XFER_CONTROL:
> +                               ep_desc->wMaxPacketSize = 512;
> +                               ep_desc->bInterval = 0;
> +                               break;
> +                       case USB_ENDPOINT_XFER_BULK:
> +                               ep_desc->wMaxPacketSize = 1024;
> +                               ep_desc->bInterval = 0;
> +                               break;
> +                       case USB_ENDPOINT_XFER_INT:
> +                       case USB_ENDPOINT_XFER_ISOC:
> +                               break;
> +                       }
> +                       *tmp = mem;
> +                       tmp++;
> +                       mem += (*src)->bLength;
> +                       /*add ep companion descriptor*/
> +                       memcpy(mem, &ep_comp_desc, ep_comp_desc.bLength);
> +                       *tmp = mem;
> +                       tmp++;
> +                       mem += ep_comp_desc.bLength;
> +                       break;
> +               default:
> +                       *tmp = mem;
> +                       tmp++;
> +                       mem += (*src)->bLength;
> +                       break;
> +               }
> +               src++;
> +       }
> +       *tmp = NULL; /*The last (struct usb_descriptor_header *) in the
> +                       descriptors vector is NULL*/

Fix commenting style.

> +       f->ss_desc_allocated = true;
> +}
> +
>  /*-------------------------------------------------------------------------*/
>  /**
>  * next_ep_desc - advance to the next EP descriptor
> @@ -110,6 +206,9 @@ int ep_choose(struct usb_gadget *g, struct usb_function *f, struct usb_ep *_ep)
>        struct usb_endpoint_descriptor *chosen_desc = NULL;
>        struct usb_descriptor_header **speed_desc = NULL;
>
> +       struct usb_ss_ep_comp_descriptor *comp_desc = NULL;
> +       int want_comp_desc = 0;
> +
>        struct usb_descriptor_header **d_spd; /* cursor for speed desc */
>
>        if (!g || !f || !_ep)
> @@ -117,6 +216,13 @@ int ep_choose(struct usb_gadget *g, struct usb_function *f, struct usb_ep *_ep)
>
>        /* select desired speed */
>        switch (g->speed) {
> +       case USB_SPEED_SUPER:
> +               if (gadget_is_superspeed(g)) {
> +                       speed_desc = f->ss_descriptors;
> +                       want_comp_desc = 1;
> +                       break;
> +               }
> +               /*else: Fall trough*/
>        case USB_SPEED_HIGH:
>                if (gadget_is_dualspeed(g)) {
>                        speed_desc = f->hs_descriptors;
> @@ -143,7 +249,18 @@ ep_found:
>        /* commit results */
>        _ep->maxpacket = chosen_desc->wMaxPacketSize;
>        _ep->desc = chosen_desc;
> -
> +       _ep->comp_desc = NULL;
> +       if (want_comp_desc) {
> +               /**
> +                * Companion descriptor should follow EP descriptor
> +                * USB 3.0 spec, #9.6.7
> +                */

Same here. No two **.

> +               comp_desc = (struct usb_ss_ep_comp_descriptor *)*(++d_spd);
> +               if (!comp_desc ||
> +                   (comp_desc->bDescriptorType != USB_DT_SS_ENDPOINT_COMP))
> +                       return -EIO;
> +               _ep->comp_desc = comp_desc;
> +       }
>        return 0;
>  }
>
> @@ -185,6 +302,12 @@ int usb_add_function(struct usb_configuration *config,
>                        list_del(&function->list);
>                        function->config = NULL;
>                }
> +               /*Add SS descriptors if there are any. This has to be done
> +                 after the bind since we need the hs_descriptors to be set in
> +                 usb_function and some of the FDs does it in the bind. */

And here.

Please run scripts/checkpatch.pl to fix up the coding style issues at
several places in this patch.

Regards,
Maulik
--
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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux