Hi,
> > 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?
>
Sorry. Ignore the noise :)
I had some local modifications to the code.
I will test again only with patches in Laurent's branch.
Sorry again,
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
