Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220617103645.71560-1-ribalda@chromium.org> <20220617103645.71560-5-ribalda@chromium.org>
 <YqyJUikNtzvgujob@pendragon.ideasonboard.com>
In-Reply-To: <YqyJUikNtzvgujob@pendragon.ideasonboard.com>
From:   Ricardo Ribalda <ribalda@chromium.org>
Date:   Fri, 17 Jun 2022 16:18:57 +0200
Message-ID: <CANiDSCt4mHio7yzgo8afYipB82=DV9FG-rmJ_OnV1jk9QwQB7Q@mail.gmail.com>
Subject: Re: [PATCH v7 4/8] media: uvcvideo: Limit power line control for
 Quanta UVC Webcam
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        tfiga@chromium.org, senozhatsky@chromium.org, yunkec@google.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Laurent

On Fri, 17 Jun 2022 at 16:02, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.

Thank you :)

>
> On Fri, Jun 17, 2022 at 12:36:41PM +0200, Ricardo Ribalda wrote:
> > The device does not implement the power line control correctly. Add a
> > corresponding control mapping override.
> >
> > Bus 001 Device 003: ID 0408:3090 Quanta Computer, Inc. USB2.0 HD UVC WebCam
> > Device Descriptor:
> >   bLength                18
> >   bDescriptorType         1
> >   bcdUSB               2.00
> >   bDeviceClass          239 Miscellaneous Device
> >   bDeviceSubClass         2
> >   bDeviceProtocol         1 Interface Association
> >   bMaxPacketSize0        64
> >   idVendor           0x0408 Quanta Computer, Inc.
> >   idProduct          0x3090
> >   bcdDevice            0.04
> >   iManufacturer           3 Quanta
> >   iProduct                1 USB2.0 HD UVC WebCam
> >   iSerial                 2 0x0001
> >   bNumConfigurations      1
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 35 ++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 6c86faecbea2..4fb07084f1c0 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -2643,6 +2643,32 @@ MODULE_PARM_DESC(timeout, "Streaming control requests timeout");
> >   * Driver initialization and cleanup
> >   */
> >
> > +static const struct uvc_menu_info power_line_frequency_controls_limited[] = {
> > +     { 0, "Invalid" },
> > +     { 1, "50 Hz" },
> > +     { 2, "60 Hz" },
> > +};
>
> It's not nice to have to include the first item in the array, but we
> can't fix that without modifying uvc_menu_info, which we can't do as
> it's part of the UAPI. Let's keep it as-is, but I would then expose the
> uvc_menu_info array from uvc_ctrl.c instead of duplicating it here.

Instead of that, what about using the values from v4l2_ctrl_get_menu() ?

We will still provide a fallback in case the control is not standard.

Let me implement that in the next version and see what you think about it.

Regards!

>
> > +
> > +static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = {
> > +     .id             = V4L2_CID_POWER_LINE_FREQUENCY,
> > +     .entity         = UVC_GUID_UVC_PROCESSING,
> > +     .selector       = UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
> > +     .size           = 2,
> > +     .offset         = 0,
> > +     .v4l2_type      = V4L2_CTRL_TYPE_MENU,
> > +     .data_type      = UVC_CTRL_DATA_TYPE_ENUM,
> > +     .menu_info      = power_line_frequency_controls_limited,
> > +     .menu_min       = 1,
> > +     .menu_count     = ARRAY_SIZE(power_line_frequency_controls_limited),
> > +};
> > +
> > +static const struct uvc_device_info uvc_ctrl_power_line_limited = {
> > +     .mappings = (const struct uvc_control_mapping *[]) {
> > +             &uvc_ctrl_power_line_mapping_limited,
> > +             NULL, /* Sentinel */
> > +     },
> > +};
> > +
> >  static const struct uvc_device_info uvc_quirk_probe_minmax = {
> >       .quirks = UVC_QUIRK_PROBE_MINMAX,
> >  };
> > @@ -2673,6 +2699,15 @@ static const struct uvc_device_info uvc_quirk_force_y8 = {
> >   * though they are compliant.
> >   */
> >  static const struct usb_device_id uvc_ids[] = {
> > +     /* Quanta USB2.0 HD UVC Webcam */
> > +     { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > +                             | USB_DEVICE_ID_MATCH_INT_INFO,
> > +       .idVendor             = 0x0408,
> > +       .idProduct            = 0x3090,
> > +       .bInterfaceClass      = USB_CLASS_VIDEO,
> > +       .bInterfaceSubClass   = 1,
> > +       .bInterfaceProtocol   = 0,
> > +       .driver_info          = (kernel_ulong_t)&uvc_ctrl_power_line_limited },
> >       /* LogiLink Wireless Webcam */
> >       { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> >                               | USB_DEVICE_ID_MATCH_INT_INFO,
>
> --
> Regards,
>
> Laurent Pinchart


-- 
Ricardo Ribalda