diff mbox series

[4/8] usb: gadget: uvc: move video format initialization to uvc_v4l2

Message ID 20230323-uvc-gadget-cleanup-v1-4-e41f0c5d9d8e@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series usb: gadget: uvc: fix errors reported by v4l2-compliance | expand

Commit Message

Michael Tretter March 23, 2023, 11:41 a.m. UTC
Move the setup of the initial video format to uvc_v4l2.c that handles
all the format negotiation. This keeps all format setup and
configuration code in uvc_v4l2.c and avoids scattering the format setup
across multiple files.

Furthermore, it allows to setup the default format using the format
configured in the configfs.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/usb/gadget/function/f_uvc.c     |  2 ++
 drivers/usb/gadget/function/uvc_v4l2.c  | 11 +++++++++++
 drivers/usb/gadget/function/uvc_v4l2.h  |  3 +++
 drivers/usb/gadget/function/uvc_video.c |  5 -----
 4 files changed, 16 insertions(+), 5 deletions(-)

Comments

Laurent Pinchart March 24, 2023, 9:31 a.m. UTC | #1
Hi Michael,

Thank you for the patch.

On Thu, Mar 23, 2023 at 12:41:12PM +0100, Michael Tretter wrote:
> Move the setup of the initial video format to uvc_v4l2.c that handles
> all the format negotiation. This keeps all format setup and
> configuration code in uvc_v4l2.c and avoids scattering the format setup
> across multiple files.
> 
> Furthermore, it allows to setup the default format using the format
> configured in the configfs.

I'm afraid I don't see how that last sentence matches the patch.

> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/usb/gadget/function/f_uvc.c     |  2 ++
>  drivers/usb/gadget/function/uvc_v4l2.c  | 11 +++++++++++
>  drivers/usb/gadget/function/uvc_v4l2.h  |  3 +++
>  drivers/usb/gadget/function/uvc_video.c |  5 -----
>  4 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 5e919fb65833..a16c8f80a50a 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -434,6 +434,8 @@ uvc_register_video(struct uvc_device *uvc)
>  	struct usb_composite_dev *cdev = uvc->func.config->cdev;
>  	int ret;
>  
> +	uvc_init_default_format(uvc);
> +
>  	/* TODO reference counting. */
>  	memset(&uvc->vdev, 0, sizeof(uvc->vdev));
>  	uvc->vdev.v4l2_dev = &uvc->v4l2_dev;
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index 4b8bf94e06fc..5620546eb43b 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -130,6 +130,17 @@ static struct uvcg_format *find_format_by_pix(struct uvc_device *uvc,
>  	return uformat;
>  }
>  
> +void uvc_init_default_format(struct uvc_device *uvc)
> +{
> +	struct uvc_video *video = &uvc->video;
> +
> +	video->fcc = V4L2_PIX_FMT_YUYV;
> +	video->bpp = 16;
> +	video->width = 320;
> +	video->height = 240;
> +	video->imagesize = 320 * 240 * 2;
> +}
> +

Please place the function in a better location, not in the middle of
related helpers.

>  static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc,
>  					   struct uvcg_format *uformat,
>  					   u16 rw, u16 rh)
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.h b/drivers/usb/gadget/function/uvc_v4l2.h
> index 1576005b61fd..5c3a97de0776 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.h
> +++ b/drivers/usb/gadget/function/uvc_v4l2.h
> @@ -16,4 +16,7 @@
>  extern const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops;
>  extern const struct v4l2_file_operations uvc_v4l2_fops;
>  
> +struct uvc_device;
> +void uvc_init_default_format(struct uvc_device *uvc);
> +
>  #endif /* __UVC_V4L2_H__ */
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index dd1c6b2ca7c6..27ff9ef49e16 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -516,11 +516,6 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
>  		return -EINVAL;
>  
>  	video->uvc = uvc;
> -	video->fcc = V4L2_PIX_FMT_YUYV;
> -	video->bpp = 16;
> -	video->width = 320;
> -	video->height = 240;
> -	video->imagesize = 320 * 240 * 2;

Honestly I'm not sure I see any improvement with this change. The active
format stored in the uvc_video structure is initialized in the function
that initializes the uvc_video structure, and you're moving it to an
unrelated location. I don't like this.

>  
>  	/* Initialize the video buffers queue. */
>  	uvcg_queue_init(&video->queue, uvc->v4l2_dev.dev->parent,
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 5e919fb65833..a16c8f80a50a 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -434,6 +434,8 @@  uvc_register_video(struct uvc_device *uvc)
 	struct usb_composite_dev *cdev = uvc->func.config->cdev;
 	int ret;
 
+	uvc_init_default_format(uvc);
+
 	/* TODO reference counting. */
 	memset(&uvc->vdev, 0, sizeof(uvc->vdev));
 	uvc->vdev.v4l2_dev = &uvc->v4l2_dev;
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 4b8bf94e06fc..5620546eb43b 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -130,6 +130,17 @@  static struct uvcg_format *find_format_by_pix(struct uvc_device *uvc,
 	return uformat;
 }
 
+void uvc_init_default_format(struct uvc_device *uvc)
+{
+	struct uvc_video *video = &uvc->video;
+
+	video->fcc = V4L2_PIX_FMT_YUYV;
+	video->bpp = 16;
+	video->width = 320;
+	video->height = 240;
+	video->imagesize = 320 * 240 * 2;
+}
+
 static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc,
 					   struct uvcg_format *uformat,
 					   u16 rw, u16 rh)
diff --git a/drivers/usb/gadget/function/uvc_v4l2.h b/drivers/usb/gadget/function/uvc_v4l2.h
index 1576005b61fd..5c3a97de0776 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.h
+++ b/drivers/usb/gadget/function/uvc_v4l2.h
@@ -16,4 +16,7 @@ 
 extern const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops;
 extern const struct v4l2_file_operations uvc_v4l2_fops;
 
+struct uvc_device;
+void uvc_init_default_format(struct uvc_device *uvc);
+
 #endif /* __UVC_V4L2_H__ */
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index dd1c6b2ca7c6..27ff9ef49e16 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -516,11 +516,6 @@  int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
 		return -EINVAL;
 
 	video->uvc = uvc;
-	video->fcc = V4L2_PIX_FMT_YUYV;
-	video->bpp = 16;
-	video->width = 320;
-	video->height = 240;
-	video->imagesize = 320 * 240 * 2;
 
 	/* Initialize the video buffers queue. */
 	uvcg_queue_init(&video->queue, uvc->v4l2_dev.dev->parent,