diff mbox series

[1/3] usb: gadget: uvc: Rename uvc_control_ep

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

Commit Message

Dan Scally Dec. 5, 2022, 2:37 p.m. UTC 
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(-)

Comments

Greg Kroah-Hartman Dec. 8, 2022, 3:33 p.m. UTC | #1 
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
Dan Scally Dec. 8, 2022, 3:39 p.m. UTC | #2 
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
Laurent Pinchart Dec. 28, 2022, 1:59 a.m. UTC | #3 
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;
>
Dan Scally Jan. 1, 2023, 8:25 p.m. UTC | #4 
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 mbox series

Patch

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;