Hi Laurent,
> Hi Bhupesh,
>
> Thanks for the patch.
>
> On Thursday 17 January 2013 16:23:49 Bhupesh Sharma wrote:
> > This patch corrects the VS_INPUT_HEADER.bEndpointAddress and Video
> > Streaming.bEndpointAddress values which were incorrectly set to 0 in
> > UVC function driver.
> >
> > As 'usb_ep_autoconfig' function returns an un-claimed usb_ep, and
> > modifies the endpoint descriptor's bEndpointAddress, so there is no
> > need to again set the bEndpointAddress for Video Streaming and
> > VS_INPUT_HEADER decriptors (for all speeds: SS/HS/FS) to the
> > bEndpointAddress obtained for Full Speed case from 'usb_ep_autoconfig'
> function call.
> >
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@xxxxxx>
> > ---
> > drivers/usb/gadget/f_uvc.c | 30 ++++++++++++++++++++----------
> > 1 files changed, 20 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/f_uvc.c b/drivers/usb/gadget/f_uvc.c
> > index b34349f..1769f3e 100644
> > --- a/drivers/usb/gadget/f_uvc.c
> > +++ b/drivers/usb/gadget/f_uvc.c
> > @@ -550,8 +550,24 @@ uvc_copy_descriptors(struct uvc_device *uvc,
> enum
> > usb_device_speed speed) UVC_COPY_DESCRIPTORS(mem, dst,
> > (const struct usb_descriptor_header**)uvc_streaming_cls);
> > uvc_streaming_header->wTotalLength =
> cpu_to_le16(streaming_size);
> > - uvc_streaming_header->bEndpointAddress =
> > - uvc_fs_streaming_ep.bEndpointAddress;
> > +
> > + switch (speed) {
> > + case USB_SPEED_SUPER:
> > + uvc_streaming_header->bEndpointAddress =
> > + uvc_ss_streaming_ep.bEndpointAddress;
> > + break;
> > +
> > + case USB_SPEED_HIGH:
> > + uvc_streaming_header->bEndpointAddress =
> > + uvc_hs_streaming_ep.bEndpointAddress;
> > + break;
> > +
> > + case USB_SPEED_FULL:
> > + default:
> > + uvc_streaming_header->bEndpointAddress =
> > + uvc_fs_streaming_ep.bEndpointAddress;
> > + break;
> > + }
>
> I don't think this will work. A superspeed device will see uvc_copy_descriptor
> called 3 times, once for each speed. As only
> uvc_ss_streaming_ep.bEndpointAddress is set in that case, high-speed and
> full-speed descriptors will have a zero endpoint value.
Not exactly. The uvc_copy_descriptor is called only once for a superspeed device (at-least on my DWC3 UDC controller)
depending on the highest speed exported by the UDC controller driver.
I have tested this patch with DWC3 controller driver and it works fine and I can confirm that my original patch works fine.
Maybe someone else can test the same with his UDC controller driver.
@Michael: Have you tested my original patch with your UDC controller and does it work fine at your end?
Regards,
Bhupesh
> I propose the following patch:
>
> diff --git a/drivers/usb/gadget/f_uvc.c b/drivers/usb/gadget/f_uvc.c index
> 02266c5..c8abe79 100644
> --- a/drivers/usb/gadget/f_uvc.c
> +++ b/drivers/usb/gadget/f_uvc.c
> @@ -548,8 +548,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum
> usb_device_speed speed)
> UVC_COPY_DESCRIPTORS(mem, dst,
> (const struct usb_descriptor_header**)uvc_streaming_cls);
> uvc_streaming_header->wTotalLength =
> cpu_to_le16(streaming_size);
> - uvc_streaming_header->bEndpointAddress =
> - uvc_fs_streaming_ep.bEndpointAddress;
> + uvc_streaming_header->bEndpointAddress = uvc->video.ep-
> >address;
>
> UVC_COPY_DESCRIPTORS(mem, dst, uvc_streaming_std);
>
> @@ -636,7 +635,14 @@ uvc_function_bind(struct usb_configuration *c,
> struct usb_function *f)
> uvc->control_ep = ep;
> ep->driver_data = uvc;
>
> - ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_streaming_ep);
> + if (gadget_is_superspeed(c->cdev->gadget))
> + ep = usb_ep_autoconfig_ss(cdev->gadget,
> &uvc_ss_streaming_ep,
> + &uvc_ss_streaming_comp);
> + else if (gadget_is_dualspeed(cdev->gadget))
> + ep = usb_ep_autoconfig(cdev->gadget,
> &uvc_hs_streaming_ep);
> + else
> + ep = usb_ep_autoconfig(cdev->gadget,
> &uvc_fs_streaming_ep);
> +
> if (!ep) {
> INFO(cdev, "Unable to allocate streaming EP\n");
> goto error;
> @@ -644,10 +650,9 @@ uvc_function_bind(struct usb_configuration *c,
> struct usb_function *f)
> uvc->video.ep = ep;
> ep->driver_data = uvc;
>
> - uvc_hs_streaming_ep.bEndpointAddress =
> - uvc_fs_streaming_ep.bEndpointAddress;
> - uvc_ss_streaming_ep.bEndpointAddress =
> - uvc_fs_streaming_ep.bEndpointAddress;
> + uvc_fs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
> + uvc_hs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
> + uvc_ss_streaming_ep.bEndpointAddress = uvc->video.ep->address;
>
> /* Allocate interface IDs. */
> if ((ret = usb_interface_id(c, f)) < 0)
>
> I will squash it into my "usb: gadget/uvc: Configure the streaming endpoint
> based on the speed" to avoid bisection issues.
>
> > UVC_COPY_DESCRIPTORS(mem, dst, uvc_streaming_std);
> >
> > @@ -681,17 +697,11 @@ uvc_function_bind(struct usb_configuration *c,
> > struct usb_function *f) /* Copy descriptors for FS. */
> > f->descriptors = uvc_copy_descriptors(uvc, USB_SPEED_FULL);
> >
> > - if (gadget_is_dualspeed(cdev->gadget)) {
> > - uvc_hs_streaming_ep.bEndpointAddress =
> > - uvc_fs_streaming_ep.bEndpointAddress;
> > + if (gadget_is_dualspeed(cdev->gadget))
> > f->hs_descriptors = uvc_copy_descriptors(uvc,
> USB_SPEED_HIGH);
> > - }
> >
> > - if (gadget_is_superspeed(c->cdev->gadget)) {
> > - uvc_ss_streaming_ep.bEndpointAddress =
> > - uvc_fs_streaming_ep.bEndpointAddress;
> > + if (gadget_is_superspeed(c->cdev->gadget))
> > f->ss_descriptors = uvc_copy_descriptors(uvc,
> USB_SPEED_SUPER);
> > - }
> >
> > /* Preallocate control endpoint request. */
> > uvc->control_req = usb_ep_alloc_request(cdev->gadget->ep0,
> > GFP_KERNEL);
>
> --
> Regards,
>
> Laurent Pinchart
--
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
