| Message ID | 20221205143758.1096914-2-dan.scally@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add ability to disable UVC Gadget's interrupt endpoint | expand |
On Mon, Dec 05, 2022 at 02:37:56PM +0000, Daniel Scally wrote: > The f_uvc code defines an endpoint named "uvc_control_ep" but it > is configured with a non-zero endpoint address and has its > bmAttributes flagged as USB_ENDPOINT_XFER_INT - this cannot be the > VideoControl interface's control endpoint, as the default endpoint > 0 is used for that purpose. This is instead the optional interrupt > endpoint that can be contained by a VideoControl interface. > > Rename the variables to make that clear. Does userspace documentation also need to be updated to reflect this change? thanks, greg k-h
Hi Greg On 08/12/2022 15:33, Greg KH wrote: > On Mon, Dec 05, 2022 at 02:37:56PM +0000, Daniel Scally wrote: >> The f_uvc code defines an endpoint named "uvc_control_ep" but it >> is configured with a non-zero endpoint address and has its >> bmAttributes flagged as USB_ENDPOINT_XFER_INT - this cannot be the >> VideoControl interface's control endpoint, as the default endpoint >> 0 is used for that purpose. This is instead the optional interrupt >> endpoint that can be contained by a VideoControl interface. >> >> Rename the variables to make that clear. > Does userspace documentation also need to be updated to reflect this > change? Not for this one I don't think; the data sent to userspace was already correct and isn't changing, it's just the internal variable name that is...but for #2/3 absolutely; I'll update the documentation in a v2. > thanks, > > greg k-h
Hi Dan, Thank you for the patch. On Mon, Dec 05, 2022 at 02:37:56PM +0000, Daniel Scally wrote: > The f_uvc code defines an endpoint named "uvc_control_ep" but it > is configured with a non-zero endpoint address and has its > bmAttributes flagged as USB_ENDPOINT_XFER_INT - this cannot be the > VideoControl interface's control endpoint, as the default endpoint > 0 is used for that purpose. This is instead the optional interrupt > endpoint that can be contained by a VideoControl interface. > > Rename the variables to make that clear. > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > drivers/usb/gadget/function/f_uvc.c | 24 ++++++++++++------------ > drivers/usb/gadget/function/uvc.h | 2 +- > 2 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c > index 6e196e06181e..49b7231742d6 100644 > --- a/drivers/usb/gadget/function/f_uvc.c > +++ b/drivers/usb/gadget/function/f_uvc.c > @@ -86,7 +86,7 @@ static struct usb_interface_descriptor uvc_control_intf = { > .iInterface = 0, > }; > > -static struct usb_endpoint_descriptor uvc_control_ep = { > +static struct usb_endpoint_descriptor uvc_interrupt_ep = { > .bLength = USB_DT_ENDPOINT_SIZE, > .bDescriptorType = USB_DT_ENDPOINT, > .bEndpointAddress = USB_DIR_IN, > @@ -290,14 +290,14 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt) > if (alt) > return -EINVAL; > > - uvcg_info(f, "reset UVC Control\n"); > - usb_ep_disable(uvc->control_ep); > + uvcg_info(f, "reset UVC interrupt endpoint\n"); > + usb_ep_disable(uvc->interrupt_ep); Technically it's called the "VideoControl Interrupt Endpoint", but that would be quite long, and given that UVC has a single interrupt endpoint in all interfaces, I think the name is fine. > > - if (!uvc->control_ep->desc) > - if (config_ep_by_speed(cdev->gadget, f, uvc->control_ep)) > + if (!uvc->interrupt_ep->desc) > + if (config_ep_by_speed(cdev->gadget, f, uvc->interrupt_ep)) > return -EINVAL; > > - usb_ep_enable(uvc->control_ep); > + usb_ep_enable(uvc->interrupt_ep); > > if (uvc->state == UVC_STATE_DISCONNECTED) { > memset(&v4l2_event, 0, sizeof(v4l2_event)); > @@ -375,7 +375,7 @@ uvc_function_disable(struct usb_function *f) > uvc->state = UVC_STATE_DISCONNECTED; > > usb_ep_disable(uvc->video.ep); > - usb_ep_disable(uvc->control_ep); > + usb_ep_disable(uvc->interrupt_ep); > } > > /* -------------------------------------------------------------------------- > @@ -511,7 +511,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) > * uvc_iad > * uvc_control_intf > * Class-specific UVC control descriptors > - * uvc_control_ep > + * uvc_interrupt_ep > * uvc_control_cs_ep > * uvc_ss_control_comp (for SS only) Shouldn't you also rename those two variables ? Ideally I would also rename struct uvc_control_endpoint_descriptor and UVC_DT_CONTROL_ENDPOINT_SIZE, but those are exposed to userspace :-( > * uvc_streaming_intf_alt0 > @@ -523,7 +523,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) > control_size = 0; > streaming_size = 0; > bytes = uvc_iad.bLength + uvc_control_intf.bLength > - + uvc_control_ep.bLength + uvc_control_cs_ep.bLength > + + uvc_interrupt_ep.bLength + uvc_control_cs_ep.bLength > + uvc_streaming_intf_alt0.bLength; > > if (speed == USB_SPEED_SUPER) { > @@ -569,7 +569,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) > uvc_control_header->bInCollection = 1; > uvc_control_header->baInterfaceNr[0] = uvc->streaming_intf; > > - UVC_COPY_DESCRIPTOR(mem, dst, &uvc_control_ep); > + UVC_COPY_DESCRIPTOR(mem, dst, &uvc_interrupt_ep); > if (speed == USB_SPEED_SUPER) > UVC_COPY_DESCRIPTOR(mem, dst, &uvc_ss_control_comp); > > @@ -656,12 +656,12 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f) > (opts->streaming_maxburst + 1)); > > /* Allocate endpoints. */ > - ep = usb_ep_autoconfig(cdev->gadget, &uvc_control_ep); > + ep = usb_ep_autoconfig(cdev->gadget, &uvc_interrupt_ep); > if (!ep) { > uvcg_info(f, "Unable to allocate control EP\n"); > goto error; > } > - uvc->control_ep = ep; > + uvc->interrupt_ep = ep; > > if (gadget_is_superspeed(c->cdev->gadget)) > ep = usb_ep_autoconfig_ss(cdev->gadget, &uvc_ss_streaming_ep, > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h > index 40226b1f7e14..48b71e04c2b1 100644 > --- a/drivers/usb/gadget/function/uvc.h > +++ b/drivers/usb/gadget/function/uvc.h > @@ -146,7 +146,7 @@ struct uvc_device { > } desc; > > unsigned int control_intf; > - struct usb_ep *control_ep; > + struct usb_ep *interrupt_ep; > struct usb_request *control_req; > void *control_buf; >
Hi Laurent - thank you for the review! On 28/12/2022 01:59, Laurent Pinchart wrote: > Hi Dan, > > Thank you for the patch. > > On Mon, Dec 05, 2022 at 02:37:56PM +0000, Daniel Scally wrote: >> The f_uvc code defines an endpoint named "uvc_control_ep" but it >> is configured with a non-zero endpoint address and has its >> bmAttributes flagged as USB_ENDPOINT_XFER_INT - this cannot be the >> VideoControl interface's control endpoint, as the default endpoint >> 0 is used for that purpose. This is instead the optional interrupt >> endpoint that can be contained by a VideoControl interface. >> >> Rename the variables to make that clear. >> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> drivers/usb/gadget/function/f_uvc.c | 24 ++++++++++++------------ >> drivers/usb/gadget/function/uvc.h | 2 +- >> 2 files changed, 13 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c >> index 6e196e06181e..49b7231742d6 100644 >> --- a/drivers/usb/gadget/function/f_uvc.c >> +++ b/drivers/usb/gadget/function/f_uvc.c >> @@ -86,7 +86,7 @@ static struct usb_interface_descriptor uvc_control_intf = { >> .iInterface = 0, >> }; >> >> -static struct usb_endpoint_descriptor uvc_control_ep = { >> +static struct usb_endpoint_descriptor uvc_interrupt_ep = { >> .bLength = USB_DT_ENDPOINT_SIZE, >> .bDescriptorType = USB_DT_ENDPOINT, >> .bEndpointAddress = USB_DIR_IN, >> @@ -290,14 +290,14 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt) >> if (alt) >> return -EINVAL; >> >> - uvcg_info(f, "reset UVC Control\n"); >> - usb_ep_disable(uvc->control_ep); >> + uvcg_info(f, "reset UVC interrupt endpoint\n"); >> + usb_ep_disable(uvc->interrupt_ep); > Technically it's called the "VideoControl Interrupt Endpoint", but that > would be quite long, and given that UVC has a single interrupt endpoint > in all interfaces, I think the name is fine. > >> >> - if (!uvc->control_ep->desc) >> - if (config_ep_by_speed(cdev->gadget, f, uvc->control_ep)) >> + if (!uvc->interrupt_ep->desc) >> + if (config_ep_by_speed(cdev->gadget, f, uvc->interrupt_ep)) >> return -EINVAL; >> >> - usb_ep_enable(uvc->control_ep); >> + usb_ep_enable(uvc->interrupt_ep); >> >> if (uvc->state == UVC_STATE_DISCONNECTED) { >> memset(&v4l2_event, 0, sizeof(v4l2_event)); >> @@ -375,7 +375,7 @@ uvc_function_disable(struct usb_function *f) >> uvc->state = UVC_STATE_DISCONNECTED; >> >> usb_ep_disable(uvc->video.ep); >> - usb_ep_disable(uvc->control_ep); >> + usb_ep_disable(uvc->interrupt_ep); >> } >> >> /* -------------------------------------------------------------------------- >> @@ -511,7 +511,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) >> * uvc_iad >> * uvc_control_intf >> * Class-specific UVC control descriptors >> - * uvc_control_ep >> + * uvc_interrupt_ep >> * uvc_control_cs_ep >> * uvc_ss_control_comp (for SS only) > Shouldn't you also rename those two variables ? Yes, I hadn't realised that they were related. That's fixed in the v2. > > Ideally I would also rename struct uvc_control_endpoint_descriptor and > UVC_DT_CONTROL_ENDPOINT_SIZE, but those are exposed to userspace :-( > >> * uvc_streaming_intf_alt0 >> @@ -523,7 +523,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) >> control_size = 0; >> streaming_size = 0; >> bytes = uvc_iad.bLength + uvc_control_intf.bLength >> - + uvc_control_ep.bLength + uvc_control_cs_ep.bLength >> + + uvc_interrupt_ep.bLength + uvc_control_cs_ep.bLength >> + uvc_streaming_intf_alt0.bLength; >> >> if (speed == USB_SPEED_SUPER) { >> @@ -569,7 +569,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) >> uvc_control_header->bInCollection = 1; >> uvc_control_header->baInterfaceNr[0] = uvc->streaming_intf; >> >> - UVC_COPY_DESCRIPTOR(mem, dst, &uvc_control_ep); >> + UVC_COPY_DESCRIPTOR(mem, dst, &uvc_interrupt_ep); >> if (speed == USB_SPEED_SUPER) >> UVC_COPY_DESCRIPTOR(mem, dst, &uvc_ss_control_comp); >> >> @@ -656,12 +656,12 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f) >> (opts->streaming_maxburst + 1)); >> >> /* Allocate endpoints. */ >> - ep = usb_ep_autoconfig(cdev->gadget, &uvc_control_ep); >> + ep = usb_ep_autoconfig(cdev->gadget, &uvc_interrupt_ep); >> if (!ep) { >> uvcg_info(f, "Unable to allocate control EP\n"); >> goto error; >> } >> - uvc->control_ep = ep; >> + uvc->interrupt_ep = ep; >> >> if (gadget_is_superspeed(c->cdev->gadget)) >> ep = usb_ep_autoconfig_ss(cdev->gadget, &uvc_ss_streaming_ep, >> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h >> index 40226b1f7e14..48b71e04c2b1 100644 >> --- a/drivers/usb/gadget/function/uvc.h >> +++ b/drivers/usb/gadget/function/uvc.h >> @@ -146,7 +146,7 @@ struct uvc_device { >> } desc; >> >> unsigned int control_intf; >> - struct usb_ep *control_ep; >> + struct usb_ep *interrupt_ep; >> struct usb_request *control_req; >> void *control_buf; >>
diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c index 6e196e06181e..49b7231742d6 100644 --- a/drivers/usb/gadget/function/f_uvc.c +++ b/drivers/usb/gadget/function/f_uvc.c @@ -86,7 +86,7 @@ static struct usb_interface_descriptor uvc_control_intf = { .iInterface = 0, }; -static struct usb_endpoint_descriptor uvc_control_ep = { +static struct usb_endpoint_descriptor uvc_interrupt_ep = { .bLength = USB_DT_ENDPOINT_SIZE, .bDescriptorType = USB_DT_ENDPOINT, .bEndpointAddress = USB_DIR_IN, @@ -290,14 +290,14 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt) if (alt) return -EINVAL; - uvcg_info(f, "reset UVC Control\n"); - usb_ep_disable(uvc->control_ep); + uvcg_info(f, "reset UVC interrupt endpoint\n"); + usb_ep_disable(uvc->interrupt_ep); - if (!uvc->control_ep->desc) - if (config_ep_by_speed(cdev->gadget, f, uvc->control_ep)) + if (!uvc->interrupt_ep->desc) + if (config_ep_by_speed(cdev->gadget, f, uvc->interrupt_ep)) return -EINVAL; - usb_ep_enable(uvc->control_ep); + usb_ep_enable(uvc->interrupt_ep); if (uvc->state == UVC_STATE_DISCONNECTED) { memset(&v4l2_event, 0, sizeof(v4l2_event)); @@ -375,7 +375,7 @@ uvc_function_disable(struct usb_function *f) uvc->state = UVC_STATE_DISCONNECTED; usb_ep_disable(uvc->video.ep); - usb_ep_disable(uvc->control_ep); + usb_ep_disable(uvc->interrupt_ep); } /* -------------------------------------------------------------------------- @@ -511,7 +511,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) * uvc_iad * uvc_control_intf * Class-specific UVC control descriptors - * uvc_control_ep + * uvc_interrupt_ep * uvc_control_cs_ep * uvc_ss_control_comp (for SS only) * uvc_streaming_intf_alt0 @@ -523,7 +523,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) control_size = 0; streaming_size = 0; bytes = uvc_iad.bLength + uvc_control_intf.bLength - + uvc_control_ep.bLength + uvc_control_cs_ep.bLength + + uvc_interrupt_ep.bLength + uvc_control_cs_ep.bLength + uvc_streaming_intf_alt0.bLength; if (speed == USB_SPEED_SUPER) { @@ -569,7 +569,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) uvc_control_header->bInCollection = 1; uvc_control_header->baInterfaceNr[0] = uvc->streaming_intf; - UVC_COPY_DESCRIPTOR(mem, dst, &uvc_control_ep); + UVC_COPY_DESCRIPTOR(mem, dst, &uvc_interrupt_ep); if (speed == USB_SPEED_SUPER) UVC_COPY_DESCRIPTOR(mem, dst, &uvc_ss_control_comp); @@ -656,12 +656,12 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f) (opts->streaming_maxburst + 1)); /* Allocate endpoints. */ - ep = usb_ep_autoconfig(cdev->gadget, &uvc_control_ep); + ep = usb_ep_autoconfig(cdev->gadget, &uvc_interrupt_ep); if (!ep) { uvcg_info(f, "Unable to allocate control EP\n"); goto error; } - uvc->control_ep = ep; + uvc->interrupt_ep = ep; if (gadget_is_superspeed(c->cdev->gadget)) ep = usb_ep_autoconfig_ss(cdev->gadget, &uvc_ss_streaming_ep, diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h index 40226b1f7e14..48b71e04c2b1 100644 --- a/drivers/usb/gadget/function/uvc.h +++ b/drivers/usb/gadget/function/uvc.h @@ -146,7 +146,7 @@ struct uvc_device { } desc; unsigned int control_intf; - struct usb_ep *control_ep; + struct usb_ep *interrupt_ep; struct usb_request *control_req; void *control_buf;
The f_uvc code defines an endpoint named "uvc_control_ep" but it is configured with a non-zero endpoint address and has its bmAttributes flagged as USB_ENDPOINT_XFER_INT - this cannot be the VideoControl interface's control endpoint, as the default endpoint 0 is used for that purpose. This is instead the optional interrupt endpoint that can be contained by a VideoControl interface. Rename the variables to make that clear. Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- drivers/usb/gadget/function/f_uvc.c | 24 ++++++++++++------------ drivers/usb/gadget/function/uvc.h | 2 +- 2 files changed, 13 insertions(+), 13 deletions(-)