From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754086AbeEWFmt (ORCPT ); Wed, 23 May 2018 01:42:49 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:49658 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754026AbeEWFmq (ORCPT ); Wed, 23 May 2018 01:42:46 -0400 X-Google-Smtp-Source: AB8JxZrjFf1ybNHtVX5Br/RcWFtYHOmCaQnNHT3z6eFhy9Yl1QDac900rt+urHreIKJNDy6L0fbmsw== Content-Type: text/plain; charset=us-ascii; delsp=yes; format=flowed Mime-Version: 1.0 (Mac OS X Mail 11.4 \(3445.8.2\)) Subject: Re: [PATCH] media: uvcvideo: Support realtek's UVC 1.5 device From: Kai Heng Feng In-Reply-To: <2510852.fx2XduE8hM@avalon> Date: Wed, 23 May 2018 13:42:38 +0800 Cc: ming_qian@realsil.com.cn, Mauro Carvalho Chehab , linux-media@vger.kernel.org, Linux Kernel Mailing List , Hans de Goede , Ana Guerrero Lopez Content-Transfer-Encoding: 7bit Message-Id: References: <1525831988-32017-1-git-send-email-ming_qian@realsil.com.cn> <2510852.fx2XduE8hM@avalon> To: Laurent Pinchart X-Mailer: Apple Mail (2.3445.8.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > On May 23, 2018, at 4:32 AM, Laurent Pinchart > wrote: > > Hello, > > Thank you for the patch. > > On Wednesday, 9 May 2018 05:13:08 EEST ming_qian@realsil.com.cn wrote: >> From: ming_qian >> >> The length of UVC 1.5 video control is 48, and it id 34 for UVC 1.1. >> Change it to 48 for UVC 1.5 device, >> and the UVC 1.5 device can be recognized. >> >> More changes to the driver are needed for full UVC 1.5 compatibility. >> However, at least the UVC 1.5 Realtek RTS5847/RTS5852 cameras have >> been reported to work well. > > This patch is however not specific to Realtek devices, so I think we should > make the subject line more generic. It's fine mentioning in the commit > message > itself that the Realtek RTS5847/RTS5852 cameras have been successfully > tested. > >> Signed-off-by: ming_qian >> --- >> drivers/media/usb/uvc/uvc_video.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/media/usb/uvc/uvc_video.c >> b/drivers/media/usb/uvc/uvc_video.c index aa0082f..32dfb32 100644 >> --- a/drivers/media/usb/uvc/uvc_video.c >> +++ b/drivers/media/usb/uvc/uvc_video.c >> @@ -171,6 +171,8 @@ static int uvc_get_video_ctrl(struct uvc_streaming >> *stream, int ret; >> >> size = stream->dev->uvc_version >= 0x0110 ? 34 : 26; >> + if (stream->dev->uvc_version >= 0x0150) >> + size = 48; >> if ((stream->dev->quirks & UVC_QUIRK_PROBE_DEF) && >> query == UVC_GET_DEF) >> return -EIO; >> @@ -259,6 +261,8 @@ static int uvc_set_video_ctrl(struct uvc_streaming >> *stream, int ret; >> >> size = stream->dev->uvc_version >= 0x0110 ? 34 : 26; >> + if (stream->dev->uvc_version >= 0x0150) >> + size = 48; >> data = kzalloc(size, GFP_KERNEL); >> if (data == NULL) >> return -ENOMEM; > > Instead of duplicating the computation in both functions, I think we should > move the code to a helper function. > > Furthermore there are equality checks further down both functions that > compare > the size to 34, they should be updated to also support UVC 1.5. > > I propose the following updated patch. If you're fine with it there's no > need > to resubmit, I'll queue it for v4.19. > > I have dropped the Reviewed-by and Tested-by tags as the patch has changed. > > commit a9c002732695eab2096580a0d1a1687bc2f95928 > Author: ming_qian > Date: Wed May 9 10:13:08 2018 +0800 > > media: uvcvideo: Support UVC 1.5 video probe & commit controls > > The length of UVC 1.5 video control is 48, and it is 34 for UVC 1.1. > Change it to 48 for UVC 1.5 device, and the UVC 1.5 device can be > recognized. > > More changes to the driver are needed for full UVC 1.5 compatibility. > However, at least the UVC 1.5 Realtek RTS5847/RTS5852 cameras have been > reported to work well. > > Cc: stable@vger.kernel.org > Signed-off-by: ming_qian > [Factor out code to helper function, update size checks] > Signed-off-by: Laurent Pinchart I tested this new patch and it works well. Tested-by: Kai-Heng Feng > > diff --git a/drivers/media/usb/uvc/uvc_video.c > b/drivers/media/usb/uvc/uvc_video.c > index eb9e04a59427..285b0e813b9d 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -207,14 +207,27 @@ static void uvc_fixup_video_ctrl(struct > uvc_streaming *stream, > } > } > > +static size_t uvc_video_ctrl_size(struct uvc_streaming *stream) > +{ > + /* > + * Return the size of the video probe and commit controls, which depends > + * on the protocol version. > + */ > + if (stream->dev->uvc_version < 0x0110) > + return 26; > + else if (stream->dev->uvc_version < 0x0150) > + return 34; > + else > + return 48; > +} > + > static int uvc_get_video_ctrl(struct uvc_streaming *stream, > struct uvc_streaming_control *ctrl, int probe, u8 query) > { > + u16 size = uvc_video_ctrl_size(stream); > u8 *data; > - u16 size; > int ret; > > - size = stream->dev->uvc_version >= 0x0110 ? 34 : 26; > if ((stream->dev->quirks & UVC_QUIRK_PROBE_DEF) && > query == UVC_GET_DEF) > return -EIO; > @@ -271,7 +284,7 @@ static int uvc_get_video_ctrl(struct uvc_streaming > *stream, > ctrl->dwMaxVideoFrameSize = get_unaligned_le32(&data[18]); > ctrl->dwMaxPayloadTransferSize = get_unaligned_le32(&data[22]); > > - if (size == 34) { > + if (size >= 34) { > ctrl->dwClockFrequency = get_unaligned_le32(&data[26]); > ctrl->bmFramingInfo = data[30]; > ctrl->bPreferedVersion = data[31]; > @@ -300,11 +313,10 @@ static int uvc_get_video_ctrl(struct uvc_streaming > *stream, > static int uvc_set_video_ctrl(struct uvc_streaming *stream, > struct uvc_streaming_control *ctrl, int probe) > { > + u16 size = uvc_video_ctrl_size(stream); > u8 *data; > - u16 size; > int ret; > > - size = stream->dev->uvc_version >= 0x0110 ? 34 : 26; > data = kzalloc(size, GFP_KERNEL); > if (data == NULL) > return -ENOMEM; > @@ -321,7 +333,7 @@ static int uvc_set_video_ctrl(struct uvc_streaming > *stream, > put_unaligned_le32(ctrl->dwMaxVideoFrameSize, &data[18]); > put_unaligned_le32(ctrl->dwMaxPayloadTransferSize, &data[22]); > > - if (size == 34) { > + if (size >= 34) { > put_unaligned_le32(ctrl->dwClockFrequency, &data[26]); > data[30] = ctrl->bmFramingInfo; > data[31] = ctrl->bPreferedVersion; > > -- > Regards, > > Laurent Pinchart