Re: [PATCH] usb: gadget: USB3 support to the legacy printer driver

[Felipe Balbi]

Hi,

On Mon, Nov 17, 2014 at 06:19:54PM -0500, Jorge Ramirez-Ortiz wrote:
> Hi,
> 
> This patch adds USB3 support to the legacy gadget printer driver.
> Applies cleanly on fc14f9c Linux 3.18-rc5.
> 
> Please could it be considered for inclusion?

sure, if you send it properly (see Documentation/SubmittingPatches),
provide logs of your tests with a recent kernel (v3.18-rc5 would be just
awesome) and Cc myself on your resubmission.

> diff --git a/drivers/usb/gadget/legacy/printer.c b/drivers/usb/gadget/legacy/printer.c
> index 6474081..625d905 100644
> --- a/drivers/usb/gadget/legacy/printer.c
> +++ b/drivers/usb/gadget/legacy/printer.c
> @@ -3,6 +3,7 @@
>   *
>   * Copyright (C) 2003-2005 David Brownell
>   * Copyright (C) 2006 Craig W. Nadler
> + * Copyright (C) 2014 Linaro.org

I don't think the minimal change below constitutes enough to merit the
copyright. If your lawyers tell you otherwise, let me know.

Greg, what's Linux Foundation's lawyers take on this ?

> @@ -208,6 +209,43 @@ static struct usb_descriptor_header *hs_printer_function[] = {
>         NULL
>  };
>  
> +/*
> + * Added endpoint descriptors for 3.0 devices
> + */
> +
> +static struct usb_endpoint_descriptor ss_ep_in_desc = {
> +       .bLength =              USB_DT_ENDPOINT_SIZE,
> +       .bDescriptorType =      USB_DT_ENDPOINT,
> +       .bmAttributes =         USB_ENDPOINT_XFER_BULK,
> +       .wMaxPacketSize =       cpu_to_le16(1024),
> +};
> +
> +struct usb_ss_ep_comp_descriptor ss_ep_in_comp_desc = {
> +       .bLength =              sizeof(ss_ep_in_comp_desc),
> +       .bDescriptorType =      USB_DT_SS_ENDPOINT_COMP,
> +};
> +
> +static struct usb_endpoint_descriptor ss_ep_out_desc = {
> +       .bLength =              USB_DT_ENDPOINT_SIZE,
> +       .bDescriptorType =      USB_DT_ENDPOINT,
> +       .bmAttributes =         USB_ENDPOINT_XFER_BULK,
> +       .wMaxPacketSize =       cpu_to_le16(1024),
> +};
> +
> +struct usb_ss_ep_comp_descriptor ss_ep_out_comp_desc = {
> +       .bLength =              sizeof(ss_ep_out_comp_desc),
> +       .bDescriptorType =      USB_DT_SS_ENDPOINT_COMP,
> +};
> +
> +static struct usb_descriptor_header *ss_printer_function[] = {
> +       (struct usb_descriptor_header *) &intf_desc,
> +       (struct usb_descriptor_header *) &ss_ep_in_desc,
> +       (struct usb_descriptor_header *) &ss_ep_in_comp_desc,
> +       (struct usb_descriptor_header *) &ss_ep_out_desc,
> +       (struct usb_descriptor_header *) &ss_ep_out_comp_desc,
> +       NULL
> +};
> +
>  static struct usb_otg_descriptor otg_descriptor = {
>         .bLength =              sizeof otg_descriptor,
>         .bDescriptorType =      USB_DT_OTG,
> @@ -220,7 +258,20 @@ static const struct usb_descriptor_header *otg_desc[] = {
>  };
>  
>  /* maxpacket and other transfer characteristics vary by speed. */
> -#define ep_desc(g, hs, fs) (((g)->speed == USB_SPEED_HIGH)?(hs):(fs))
> +static inline struct usb_endpoint_descriptor *ep_desc(struct usb_gadget *gadget,
> +                                       struct usb_endpoint_descriptor *fs,
> +                                       struct usb_endpoint_descriptor *hs,
> +                                       struct usb_endpoint_descriptor *ss)
> +{
> +       struct usb_endpoint_descriptor *d = fs;
> +
> +       if (gadget->speed == USB_SPEED_SUPER)
> +               d = ss;
> +       else if (gadget->speed == USB_SPEED_HIGH)
> +               d = hs;

what happened to good old switch ?

> +       return d;
> +}
>  
>  /*-------------------------------------------------------------------------*/
>  
> @@ -793,11 +844,12 @@ set_printer_interface(struct printer_dev *dev)
>  {
>         int                     result = 0;
>  
> -       dev->in_ep->desc = ep_desc(dev->gadget, &hs_ep_in_desc, &fs_ep_in_desc);
> +       dev->in_ep->desc = ep_desc(dev->gadget, &fs_ep_in_desc, &hs_ep_in_desc,
> +                                                               &ss_ep_in_desc);

Fix your indentation.

>         dev->in_ep->driver_data = dev;
>  
> -       dev->out_ep->desc = ep_desc(dev->gadget, &hs_ep_out_desc,
> -                                   &fs_ep_out_desc);
> +       dev->out_ep->desc = ep_desc(dev->gadget, &fs_ep_out_desc,
> +                                       &hs_ep_out_desc, &ss_ep_out_desc);
>         dev->out_ep->driver_data = dev;
>  
>         result = usb_ep_enable(dev->in_ep);
> @@ -1016,9 +1068,11 @@ autoconf_fail:
>         /* assumes that all endpoints are dual-speed */
>         hs_ep_in_desc.bEndpointAddress = fs_ep_in_desc.bEndpointAddress;
>         hs_ep_out_desc.bEndpointAddress = fs_ep_out_desc.bEndpointAddress;
> +       ss_ep_in_desc.bEndpointAddress = fs_ep_in_desc.bEndpointAddress;
> +       ss_ep_out_desc.bEndpointAddress = fs_ep_out_desc.bEndpointAddress;
>  
>         ret = usb_assign_descriptors(f, fs_printer_function,
> -                       hs_printer_function, NULL);
> +                                    hs_printer_function, ss_printer_function);

why do you change indentation when adding just one extra argument ?

-- 
balbi

Attachment: signature.asc
Description: Digital signature