From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 59B30C4332F for ; Wed, 4 Jan 2023 12:18:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234103AbjADMSJ (ORCPT ); Wed, 4 Jan 2023 07:18:09 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45132 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233067AbjADMSH (ORCPT ); Wed, 4 Jan 2023 07:18:07 -0500 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D4C1D11836 for ; Wed, 4 Jan 2023 04:18:06 -0800 (PST) Received: from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi [213.243.189.158]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id D00ED6C7; Wed, 4 Jan 2023 13:18:03 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1672834684; bh=r93CJllEYEDP8Xm7IUpafT7qhEYdZoawcEO3sTTRF3o=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gAgsXFMqW9RveK+Bu8cHCXQZQtljd+1YBM23XePYMXnEKxXjQBh5fHaxmbGYJIS2X TmgXeX5V6zwoTF4GvxMnLRfzc5YdG8Vv4utv0lSZhTta8yBKXHOsgsN5JrsrkVu8eO XjMjdbY+475/iAx7ttIWH8+n90lMYG8Vb9zjBuDs= Date: Wed, 4 Jan 2023 14:18:00 +0200 From: Laurent Pinchart To: Ricardo Ribalda Cc: linux-media@vger.kernel.org, Kieran Bingham Subject: Re: [PATCH v2 2/2] media: uvcvideo: Drop custom format names for DV formats Message-ID: References: <20230104111944.962-1-laurent.pinchart@ideasonboard.com> <20230104111944.962-3-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org 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 > > With or without my nits > > Reviewed-by: Ricardo Ribalda > > > --- > > 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