Hi Paul,
On Thu, Jun 23, 2011 at 01:08:41PM -0700, Paul Zimmerman wrote:
> This has been lightly tested using a Linux host. I was able to ssh from
> device to host and host to device, no obvious problems seen.
>
> Signed-off-by: Paul Zimmerman <paulz@xxxxxxxxxxxx>
> ---
>
> Hi Felipe,
>
> In the spirit of the other gadget patches that have appeared lately, here
> is one to add SS descriptors to the Ethernet gadget. Hope you find it
> useful.
cool, thanks... a few details below
> diff --git a/drivers/usb/gadget/f_ecm.c b/drivers/usb/gadget/f_ecm.c
> index ddedbc8..b54e2ac 100644
> --- a/drivers/usb/gadget/f_ecm.c
> +++ b/drivers/usb/gadget/f_ecm.c
> @@ -77,10 +77,12 @@ static inline struct f_ecm *func_to_ecm(struct usb_function *f)
> /* peak (theoretical) bulk transfer rate in bits-per-second */
> static inline unsigned ecm_bitrate(struct usb_gadget *g)
> {
> - if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH)
> - return 13 * 512 * 8 * 1000 * 8;
> + if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_SUPER)
> + return 13 * 1024 * 8 * 1000 * 8;
> + else if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH)
> + return 13 * 512 * 8 * 1000 * 8;
> else
> - return 19 * 64 * 1 * 1000 * 8;
> + return 19 * 64 * 1 * 1000 * 8;
you have accidentally added extra whitespaces around the '*' operators,
could you keep to only one space on each side ?
> @@ -231,6 +233,7 @@ static struct usb_endpoint_descriptor hs_ecm_notify_desc = {
> .wMaxPacketSize = cpu_to_le16(ECM_STATUS_BYTECOUNT),
> .bInterval = LOG2_STATUS_INTERVAL_MSEC + 4,
> };
> +
> static struct usb_endpoint_descriptor hs_ecm_in_desc = {
> .bLength = USB_DT_ENDPOINT_SIZE,
> .bDescriptorType = USB_DT_ENDPOINT,
> @@ -265,6 +268,71 @@ static struct usb_descriptor_header *ecm_hs_function[] = {
> NULL,
> };
>
> +/* super speed support: */
> +
> +static struct usb_endpoint_descriptor ss_ecm_notify_desc = {
> + .bLength = USB_DT_ENDPOINT_SIZE,
> + .bDescriptorType = USB_DT_ENDPOINT,
> +
> + .bEndpointAddress = USB_DIR_IN,
> + .bmAttributes = USB_ENDPOINT_XFER_INT,
> + .wMaxPacketSize = cpu_to_le16(ECM_STATUS_BYTECOUNT),
> + .bInterval = LOG2_STATUS_INTERVAL_MSEC + 4,
> +};
> +
> +static struct usb_ss_ep_comp_descriptor ss_ecm_intr_comp_desc = {
> + .bLength = 6,
> + .bDescriptorType = USB_DT_SS_ENDPOINT_COMP,
> + .bMaxBurst = 0,
> + .bmAttributes = 0,
static variable, you don't need to zero initialize. It would be nice,
though, to keep a comment saying that it could be tweaked later.
Something like:
/* .bMaxBurst = CHANGE_HERE, */
> + .wBytesPerInterval = cpu_to_le16(ECM_STATUS_BYTECOUNT),
> +};
> +
> +static struct usb_endpoint_descriptor ss_ecm_in_desc = {
> + .bLength = USB_DT_ENDPOINT_SIZE,
> + .bDescriptorType = USB_DT_ENDPOINT,
> +
> + .bEndpointAddress = USB_DIR_IN,
> + .bmAttributes = USB_ENDPOINT_XFER_BULK,
> + .wMaxPacketSize = cpu_to_le16(1024),
> +};
> +
> +static struct usb_endpoint_descriptor ss_ecm_out_desc = {
> + .bLength = USB_DT_ENDPOINT_SIZE,
> + .bDescriptorType = USB_DT_ENDPOINT,
> +
> + .bEndpointAddress = USB_DIR_OUT,
> + .bmAttributes = USB_ENDPOINT_XFER_BULK,
> + .wMaxPacketSize = cpu_to_le16(1024),
> +};
> +
> +static struct usb_ss_ep_comp_descriptor ss_ecm_bulk_comp_desc = {
> + .bLength = 6,
this should be:
.bLength = sizeof(ss_ecm_ulk_comp_desc),
> + .bDescriptorType = USB_DT_SS_ENDPOINT_COMP,
> + .bMaxBurst = 0,
> + .bmAttributes = 0,
> + .wBytesPerInterval = 0,
same comment as above.
> +};
> +
> +static struct usb_descriptor_header *ecm_ss_function[] = {
> + /* CDC ECM control descriptors */
> + (struct usb_descriptor_header *) &ecm_control_intf,
> + (struct usb_descriptor_header *) &ecm_header_desc,
> + (struct usb_descriptor_header *) &ecm_union_desc,
> + (struct usb_descriptor_header *) &ecm_desc,
for improving readability, please add a blank line here,
> + /* NOTE: status endpoint might need to be removed */
> + (struct usb_descriptor_header *) &ss_ecm_notify_desc,
> + (struct usb_descriptor_header *) &ss_ecm_intr_comp_desc,
and here.
> + /* data interface, altsettings 0 and 1 */
> + (struct usb_descriptor_header *) &ecm_data_nop_intf,
> + (struct usb_descriptor_header *) &ecm_data_intf,
> + (struct usb_descriptor_header *) &ss_ecm_in_desc,
> + (struct usb_descriptor_header *) &ss_ecm_bulk_comp_desc,
> + (struct usb_descriptor_header *) &ss_ecm_out_desc,
> + (struct usb_descriptor_header *) &ss_ecm_bulk_comp_desc,
> + NULL,
> +};
> +
> /* string descriptors: */
>
> static struct usb_string ecm_string_defs[] = {
> @@ -677,6 +745,18 @@ ecm_bind(struct usb_configuration *c, struct usb_function *f)
> f->hs_descriptors = usb_copy_descriptors(ecm_hs_function);
> if (!f->hs_descriptors)
> goto fail;
> +
> + ss_ecm_in_desc.bEndpointAddress =
> + hs_ecm_in_desc.bEndpointAddress;
> + ss_ecm_out_desc.bEndpointAddress =
> + hs_ecm_out_desc.bEndpointAddress;
> + ss_ecm_notify_desc.bEndpointAddress =
> + hs_ecm_notify_desc.bEndpointAddress;
> +
> + /* copy descriptors, and track endpoint copies */
> + f->ss_descriptors = usb_copy_descriptors(ecm_ss_function);
> + if (!f->ss_descriptors)
> + goto fail;
> }
>
> /* NOTE: all that is done without knowing or caring about
> @@ -696,6 +776,8 @@ ecm_bind(struct usb_configuration *c, struct usb_function *f)
> fail:
> if (f->descriptors)
I never understood why the if (f->descriptors), kfree(NULL) is safe
anyway. If you wish, you could send a patch, before this one, cleaning
the unnecessary check...
> usb_free_descriptors(f->descriptors);
> + if (f->hs_descriptors)
> + usb_free_descriptors(f->hs_descriptors);
then on this patch, you don't need the if (f->hs_descriptors)
same comments are valid to f_eem.c and f_subset.c ;-)
--
balbi
Attachment:
signature.asc
Description: Digital signature
