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@xxxxxxxxxxxxxx>
> ---
> 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,
--
Regards,
Laurent Pinchart
