Hi Ricardo,
On Wed, Jan 04, 2023 at 12:51:11PM +0100, Ricardo Ribalda wrote:
> Hi Laurent
>
> For what it's worth, I am pro squash :)
Works for me. I'll give a few more days for people to comment and then
I'll send a squashed v3.
> On Wed, 4 Jan 2023 at 12:19, Laurent Pinchart wrote:
> >
> > Unlike V4L2, UVC makes a distinction between the SD-DV, SDL-DV and HD-DV
> > formats. It also indicates whether the DV format uses 50Hz or 60Hz. This
> > information is parsed by the driver to construct a format name string
> > that is printed in a debug message, but serves no other purpose as V4L2
> > has a single V4L2_PIX_FMT_DV pixel format that covers all those cases.
> >
> > As the information is available in the UVC descriptors, and thus
> > accessible to users with lsusb if they really care, don't log it in a
> > debug message and drop the format name string to simplify the code.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> With or without my nits
>
> Reviewed-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
>
> > ---
> > drivers/media/usb/uvc/uvc_driver.c | 18 +++---------------
> > 1 file changed, 3 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 9852d6f63ae8..ba41f13a2491 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -228,7 +228,6 @@ static int uvc_parse_format(struct uvc_device *dev,
> > struct uvc_format_desc *fmtdesc;
> > struct uvc_frame *frame;
> > const unsigned char *start = buffer;
> > - char fmtname[12] = { 0, };
> > unsigned int width_multiplier = 1;
> > unsigned int interval;
> > unsigned int i, n;
> > @@ -325,14 +324,10 @@ static int uvc_parse_format(struct uvc_device *dev,
> >
> > switch (buffer[8] & 0x7f) {
> > case 0:
> > - strscpy(fmtname, "SD-DV", sizeof(fmtname));
> > - break;
> > case 1:
> > - strscpy(fmtname, "SDL-DV", sizeof(fmtname));
> > - break;
> > case 2:
> > - strscpy(fmtname, "HD-DV", sizeof(fmtname));
> > break;
> > +
> > default:
> > uvc_dbg(dev, DESCR,
> > "device %d videostreaming interface %d: unknown DV format %u\n",
> > @@ -341,9 +336,6 @@ static int uvc_parse_format(struct uvc_device *dev,
> > return -EINVAL;
> > }
> >
> > - strlcat(fmtname, buffer[8] & (1 << 7) ? " 60Hz" : " 50Hz",
> > - sizeof(fmtname));
> > -
> > format->fcc = V4L2_PIX_FMT_DV;
> > format->flags = UVC_FMT_FLAG_COMPRESSED | UVC_FMT_FLAG_STREAM;
> > format->bpp = 0;
> > @@ -370,12 +362,8 @@ static int uvc_parse_format(struct uvc_device *dev,
> > return -EINVAL;
> > }
> >
> > - if (format->fcc) {
> > - if (fmtname[0])
> > - uvc_dbg(dev, DESCR, "Found format %s\n", fmtname);
> > - else
> > - uvc_dbg(dev, DESCR, "Found format %p4cc", &format->fcc);
> > - }
>
> Maybe it is the way that I debug the issues. I run an OK execution
> with a FAIl one and then I compare results. I tend to prefer that the
> extra lines are errors and there is no missing lines.... but I if your
> prefer it this way, I am ok with it ;)
Given that formats with a 0 fourcc are useless from a userspace point of
view, I think I'd rather drop them actually. That's a candidate for a
further patch.
> > + if (format->fcc)
> > + uvc_dbg(dev, DESCR, "Found format %p4cc", &format->fcc);
> >
> > buflen -= buffer[0];
> > buffer += buffer[0];
--
Regards,
Laurent Pinchart
