Hi Laurent
On Wed, 8 Jun 2022 at 18:58, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Ricardo,
>
> On Wed, Jun 08, 2022 at 02:12:22PM +0200, Ricardo Ribalda wrote:
> > On Wed, 8 Jun 2022 at 08:50, Laurent Pinchart wrote:
> > >
> > > From: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > >
> > > 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
>
> The full descriptors make the commit messages really long, should we
> include the device descriptor only ?
Fine with me, I thought you preferred the whole lsusb -v.
>
> [snip]
>
> > > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > ---
> > > Changes since v3:
> > >
> > > - Turn the power line quirk into a control mapping overrides array
> > > ---
> > > drivers/media/usb/uvc/uvc_driver.c | 31 ++++++++++++++++++++++++++++++
> > > 1 file changed, 31 insertions(+)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index 6c86faecbea2..559d8a240f76 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -2643,6 +2643,23 @@ MODULE_PARM_DESC(timeout, "Streaming control requests timeout");
> > > * Driver initialization and cleanup
> > > */
> > >
> > > +static const struct uvc_menu_info power_line_frequency_controls_limited[] = {
> > > + { 1, "50 Hz" },
> > > + { 2, "60 Hz" },
> > > +};
> > > +
> > > +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_count = ARRAY_SIZE(power_line_frequency_controls_limited),
> > > +};
> > > +
> > > static const struct uvc_device_info uvc_quirk_probe_minmax = {
> > > .quirks = UVC_QUIRK_PROBE_MINMAX,
> > > };
> > > @@ -2673,6 +2690,20 @@ 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)&(const struct uvc_device_info){
> > > + .mappings = (const struct uvc_control_mapping *[]) {
> > > + &uvc_ctrl_power_line_mapping_limited,
> > > + NULL, /* Sentinel */
> > > + },
> > > + } },
> >
> > What about: ?
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > b/drivers/media/usb/uvc/uvc_driver.c
> > index b8df40546b29..ea8c8d572ef2 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -2660,6 +2660,13 @@ static const struct uvc_control_mapping
> > uvc_ctrl_power_line_mapping_limited = {
> > .menu_count = ARRAY_SIZE(power_line_frequency_controls_limited),
> > };
> >
> > +static const struct uvc_device_info uvc_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,
> > };
> > @@ -2698,12 +2705,7 @@ static const struct usb_device_id uvc_ids[] = {
> > .bInterfaceClass = USB_CLASS_VIDEO,
> > .bInterfaceSubClass = 1,
> > .bInterfaceProtocol = 0,
> > - .driver_info = (kernel_ulong_t)&(const struct uvc_device_info){
> > - .mappings = (const struct uvc_control_mapping *[]) {
> > - &uvc_ctrl_power_line_mapping_limited,
> > - NULL, /* Sentinel */
> > - },
> > - } },
> > + .driver_info = (kernel_ulong_t)&uvc_power_line_limited },
>
> Looks good to me. I'll do so.
Or I can do it if you are busy, just let me know what you prefer.
btw, just want to state the obvious here, but with this approach we
cannot quirk the device via modprobe.
What about a mixed approach?, where we leave the quirk, and then in
uvc_ctrl_init_device we patch the mappings? This has also the benefit
that we do not leak the mappings into the uvc_driver.c file
Let me know if you like it and then I send a new version
Regards!
>
> > I can probably test it on real hw today
>
> Thank you.
By today I mean today :P
>
> > > /* LogiLink Wireless Webcam */
> > > { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> > > | USB_DEVICE_ID_MATCH_INT_INFO,
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
