| 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 |
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 --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,
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(-)