Hi Dan,
Thank you for the patch.
On Mon, Dec 05, 2022 at 02:37:58PM +0000, Daniel Scally wrote:
> The f_uvc code includes an interrupt endpoint against the VideoControl
> interface. According to section 2.4.2 of the UVC specification however
> this endpoint is optional in at least some cases:
>
> "This endpoint is optional, but may be mandatory under certain
> conditions"
>
> The conditions enumerated are whether...
>
> 1. The device supports hardware triggers
> 2. The device implements any AutoUpdate controls
> 3. The device implements any Asynchronous controls
>
> As all of those things are implementation dependent, this endpoint
> might be unnecessary for some users. Check whether the user has
> requested it be disable via configfs and don't proceed with its
s/disable/disabled/
> instantiation if so.
>
> Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
> ---
> drivers/usb/gadget/function/f_uvc.c | 42 ++++++++++++++++++-----------
> drivers/usb/gadget/function/uvc.h | 1 +
> 2 files changed, 27 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 49b7231742d6..76ec84d3a5fd 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -79,7 +79,7 @@ static struct usb_interface_descriptor uvc_control_intf = {
> .bDescriptorType = USB_DT_INTERFACE,
> .bInterfaceNumber = UVC_INTF_VIDEO_CONTROL,
> .bAlternateSetting = 0,
> - .bNumEndpoints = 1,
> + .bNumEndpoints = 0,
> .bInterfaceClass = USB_CLASS_VIDEO,
> .bInterfaceSubClass = UVC_SC_VIDEOCONTROL,
> .bInterfaceProtocol = 0x00,
> @@ -290,14 +290,16 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
> if (alt)
> return -EINVAL;
>
> - uvcg_info(f, "reset UVC interrupt endpoint\n");
> - usb_ep_disable(uvc->interrupt_ep);
> + if (!uvc->disable_interrupt_ep) {
> + uvcg_info(f, "reset UVC interrupt endpoint\n");
> + usb_ep_disable(uvc->interrupt_ep);
>
> - if (!uvc->interrupt_ep->desc)
> - if (config_ep_by_speed(cdev->gadget, f, uvc->interrupt_ep))
> - return -EINVAL;
> + if (!uvc->interrupt_ep->desc)
> + if (config_ep_by_speed(cdev->gadget, f, uvc->interrupt_ep))
Line wrap would be nice.
if (config_ep_by_speed(cdev->gadget, f,
uvc->interrupt_ep))
> + return -EINVAL;
>
> - usb_ep_enable(uvc->interrupt_ep);
> + usb_ep_enable(uvc->interrupt_ep);
> + }
>
> if (uvc->state == UVC_STATE_DISCONNECTED) {
> memset(&v4l2_event, 0, sizeof(v4l2_event));
> @@ -375,7 +377,8 @@ uvc_function_disable(struct usb_function *f)
> uvc->state = UVC_STATE_DISCONNECTED;
>
> usb_ep_disable(uvc->video.ep);
> - usb_ep_disable(uvc->interrupt_ep);
> + if (!uvc->disable_interrupt_ep)
> + usb_ep_disable(uvc->interrupt_ep);
> }
>
> /* --------------------------------------------------------------------------
> @@ -523,8 +526,10 @@ 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_interrupt_ep.bLength + uvc_control_cs_ep.bLength
> - + uvc_streaming_intf_alt0.bLength;
> + + uvc_control_cs_ep.bLength + uvc_streaming_intf_alt0.bLength;
Wrong indentation.
> +
> + if (!uvc->disable_interrupt_ep)
> + bytes += uvc_interrupt_ep.bLength;
>
> if (speed == USB_SPEED_SUPER) {
> bytes += uvc_ss_control_comp.bLength;
> @@ -569,7 +574,8 @@ 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_interrupt_ep);
> + if (!uvc->disable_interrupt_ep)
> + UVC_COPY_DESCRIPTOR(mem, dst, &uvc_interrupt_ep);
> if (speed == USB_SPEED_SUPER)
> UVC_COPY_DESCRIPTOR(mem, dst, &uvc_ss_control_comp);
This is the companion descriptor for the interrupt endpoint, it should
only be included if the interrupt endpoint is enabled. Same for
uvc_control_cs_ep.
>
> @@ -656,12 +662,16 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
> (opts->streaming_maxburst + 1));
>
> /* Allocate endpoints. */
> - ep = usb_ep_autoconfig(cdev->gadget, &uvc_interrupt_ep);
> - if (!ep) {
> - uvcg_info(f, "Unable to allocate control EP\n");
> - goto error;
> + if (!opts->disable_interrupt_ep) {
> + ep = usb_ep_autoconfig(cdev->gadget, &uvc_interrupt_ep);
> + if (!ep) {
> + uvcg_info(f, "Unable to allocate interrupt EP\n");
> + goto error;
> + }
> + uvc->interrupt_ep = ep;
> + uvc_control_intf.bNumEndpoints = 1;
> }
> - uvc->interrupt_ep = ep;
> + uvc->disable_interrupt_ep = opts->disable_interrupt_ep;
Given that you always test for !uvc->disable_interrupt_ep, how about
naming this enable_interrupt_ep and switching the tests ? I think
positive logic is more readable that double negation. I would then move
this line up, in order to test uvc->enable_interrupt_ep instead of
!opts->disable_interrupt_ep above.
>
> 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 48b71e04c2b1..0d0ef9b90b1a 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -149,6 +149,7 @@ struct uvc_device {
> struct usb_ep *interrupt_ep;
> struct usb_request *control_req;
> void *control_buf;
> + bool disable_interrupt_ep;
>
> unsigned int streaming_intf;
>
--
Regards,
Laurent Pinchart
