Re: [PATCH 2/3 v7] uvcvideo: add extensible device information
Hi Laurent, Thanks for reviewing. On Tue, 28 Nov 2017, Laurent Pinchart wrote: > Hi Guennadi, > > Thank you for the patch. > > On Wednesday, 8 November 2017 18:00:13 EET Guennadi Liakhovetski wrote: > > From: Guennadi Liakhovetski > > > > Currently the UVC driver assigns a quirk bitmask to the .driver_info > > field of struct usb_device_id. This patch instroduces a struct to store > > quirks and possibly other per-device parameters in the future. > > I'd drop the "possibly" as I'm fairly certain we will have other parameters > in > the future. Otherwise the patch would be a bit pointless :-) > > > Signed-off-by: Guennadi Liakhovetski > > --- > > > > v7: this is a new patch in the series. Needed to enable specifying > > private camera metadata formats > > > > drivers/media/usb/uvc/uvc_driver.c | 127 -- > > 1 file changed, 78 insertions(+), 49 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c > > b/drivers/media/usb/uvc/uvc_driver.c index 6d22b22..cbf79b9 100644 > > --- a/drivers/media/usb/uvc/uvc_driver.c > > +++ b/drivers/media/usb/uvc/uvc_driver.c > > @@ -2001,11 +2001,18 @@ static int uvc_register_chains(struct uvc_device > > *dev) * USB probe, disconnect, suspend and resume > > */ > > > > +struct uvc_device_info { > > + u32 quirks; > > +}; > > + > > static int uvc_probe(struct usb_interface *intf, > > const struct usb_device_id *id) > > { > > struct usb_device *udev = interface_to_usbdev(intf); > > struct uvc_device *dev; > > + const struct uvc_device_info *info = > > + (const struct uvc_device_info *)id->driver_info; > > + u32 quirks = info ? info->quirks : 0; > > int function; > > int ret; > > > > @@ -2032,7 +2039,7 @@ static int uvc_probe(struct usb_interface *intf, > > dev->intf = usb_get_intf(intf); > > dev->intfnum = intf->cur_altsetting->desc.bInterfaceNumber; > > dev->quirks = (uvc_quirks_param == -1) > > - ? id->driver_info : uvc_quirks_param; > > + ? quirks : uvc_quirks_param; > > > > if (udev->product != NULL) > > strlcpy(dev->name, udev->product, sizeof dev->name); > > @@ -2073,7 +2080,7 @@ static int uvc_probe(struct usb_interface *intf, > > le16_to_cpu(udev->descriptor.idVendor), > > le16_to_cpu(udev->descriptor.idProduct)); > > > > - if (dev->quirks != id->driver_info) { > > + if (dev->quirks != quirks) { > > uvc_printk(KERN_INFO, "Forcing device quirks to 0x%x by module " > > "parameter for testing purpose.\n", dev->quirks); > > uvc_printk(KERN_INFO, "Please report required quirks to the " > > @@ -2271,6 +2278,28 @@ static int uvc_clock_param_set(const char *val, > > struct kernel_param *kp) * Driver initialization and cleanup > > */ > > > > +static struct uvc_device_info uvc_quirk_probe_minmax = { > > + .quirks = UVC_QUIRK_PROBE_MINMAX, > > +}; > > + > > +static struct uvc_device_info uvc_quirk_fix_bandwidth = { > > + .quirks = UVC_QUIRK_FIX_BANDWIDTH, > > +}; > > + > > +static struct uvc_device_info uvc_quirk_probe_def = { > > + .quirks = UVC_QUIRK_PROBE_DEF, > > +}; > > + > > +static struct uvc_device_info uvc_quirk_stream_no_fid = { > > + .quirks = UVC_QUIRK_STREAM_NO_FID, > > +}; > > + > > +static struct uvc_device_info uvc_quirk_force_y8 = { > > + .quirks = UVC_QUIRK_FORCE_Y8, > > +}; > > You can make all of these static const. > > > +#define UVC_QUIRK_INFO(q) (kernel_ulong_t)&(struct uvc_device_info){.quirks > > = q} > > + > > /* > > * The Logitech cameras listed below have their interface class set to > > * VENDOR_SPEC because they don't announce themselves as UVC devices, even > > @@ -2285,7 +2314,7 @@ static int uvc_clock_param_set(const char *val, struct > > kernel_param *kp) .bInterfaceClass = USB_CLASS_VIDEO, > > .bInterfaceSubClass = 1, > > .bInterfaceProtocol = 0, > > - .driver_info = UVC_QUIRK_PROBE_MINMAX }, > > + .driver_info = (kernel_ulong_t)&uvc_quirk_probe_minmax }, > > /* Genius eFace 2025 */ > > { .match_flags = USB_DEVICE_ID_MATCH_DEVICE > > > > | USB_DEVICE_ID_MATCH_INT_INFO, > > > > @@ -2294,7 +2323,7 @@ static int uvc_clock_param_set(const char *val, struct > > kernel_param *kp) .bInterfaceClass = USB_CLASS_VIDEO, > > .bInterfaceSubClass = 1, > > .bInterfaceProtocol = 0, > > - .driver_info = UVC_QUIRK_PROBE_MINMAX }, > > + .driver_info = (kernel_ulong_t)&uvc_quirk_probe_minmax }, > > /* Microsoft Lifecam NX-6000 */ > > { .match_flags = USB_DEVICE_ID_MATCH_DEVICE > > > > | USB_DEVICE_ID_MATCH_INT_INFO, > > > > @@ -2303,7 +2332,7 @@ static int uvc_clock_param_set(const char *val, struct > > kernel_param *kp) .bInterfaceClass = USB_CLASS_VIDEO, > > .bInterfaceSubClass = 1, > > .bInterfaceProtocol =
Re: [PATCH 2/3 v7] uvcvideo: add extensible device information
Hi Guennadi, Thank you for the patch. On Wednesday, 8 November 2017 18:00:13 EET Guennadi Liakhovetski wrote: > From: Guennadi Liakhovetski > > Currently the UVC driver assigns a quirk bitmask to the .driver_info > field of struct usb_device_id. This patch instroduces a struct to store > quirks and possibly other per-device parameters in the future. I'd drop the "possibly" as I'm fairly certain we will have other parameters in the future. Otherwise the patch would be a bit pointless :-) > Signed-off-by: Guennadi Liakhovetski > --- > > v7: this is a new patch in the series. Needed to enable specifying > private camera metadata formats > > drivers/media/usb/uvc/uvc_driver.c | 127 -- > 1 file changed, 78 insertions(+), 49 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_driver.c > b/drivers/media/usb/uvc/uvc_driver.c index 6d22b22..cbf79b9 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -2001,11 +2001,18 @@ static int uvc_register_chains(struct uvc_device > *dev) * USB probe, disconnect, suspend and resume > */ > > +struct uvc_device_info { > + u32 quirks; > +}; > + > static int uvc_probe(struct usb_interface *intf, >const struct usb_device_id *id) > { > struct usb_device *udev = interface_to_usbdev(intf); > struct uvc_device *dev; > + const struct uvc_device_info *info = > + (const struct uvc_device_info *)id->driver_info; > + u32 quirks = info ? info->quirks : 0; > int function; > int ret; > > @@ -2032,7 +2039,7 @@ static int uvc_probe(struct usb_interface *intf, > dev->intf = usb_get_intf(intf); > dev->intfnum = intf->cur_altsetting->desc.bInterfaceNumber; > dev->quirks = (uvc_quirks_param == -1) > - ? id->driver_info : uvc_quirks_param; > + ? quirks : uvc_quirks_param; > > if (udev->product != NULL) > strlcpy(dev->name, udev->product, sizeof dev->name); > @@ -2073,7 +2080,7 @@ static int uvc_probe(struct usb_interface *intf, > le16_to_cpu(udev->descriptor.idVendor), > le16_to_cpu(udev->descriptor.idProduct)); > > - if (dev->quirks != id->driver_info) { > + if (dev->quirks != quirks) { > uvc_printk(KERN_INFO, "Forcing device quirks to 0x%x by module " > "parameter for testing purpose.\n", dev->quirks); > uvc_printk(KERN_INFO, "Please report required quirks to the " > @@ -2271,6 +2278,28 @@ static int uvc_clock_param_set(const char *val, > struct kernel_param *kp) * Driver initialization and cleanup > */ > > +static struct uvc_device_info uvc_quirk_probe_minmax = { > + .quirks = UVC_QUIRK_PROBE_MINMAX, > +}; > + > +static struct uvc_device_info uvc_quirk_fix_bandwidth = { > + .quirks = UVC_QUIRK_FIX_BANDWIDTH, > +}; > + > +static struct uvc_device_info uvc_quirk_probe_def = { > + .quirks = UVC_QUIRK_PROBE_DEF, > +}; > + > +static struct uvc_device_info uvc_quirk_stream_no_fid = { > + .quirks = UVC_QUIRK_STREAM_NO_FID, > +}; > + > +static struct uvc_device_info uvc_quirk_force_y8 = { > + .quirks = UVC_QUIRK_FORCE_Y8, > +}; You can make all of these static const. > +#define UVC_QUIRK_INFO(q) (kernel_ulong_t)&(struct uvc_device_info){.quirks > = q} > + > /* > * The Logitech cameras listed below have their interface class set to > * VENDOR_SPEC because they don't announce themselves as UVC devices, even > @@ -2285,7 +2314,7 @@ static int uvc_clock_param_set(const char *val, struct > kernel_param *kp) .bInterfaceClass= USB_CLASS_VIDEO, > .bInterfaceSubClass = 1, > .bInterfaceProtocol = 0, > - .driver_info = UVC_QUIRK_PROBE_MINMAX }, > + .driver_info = (kernel_ulong_t)&uvc_quirk_probe_minmax }, > /* Genius eFace 2025 */ > { .match_flags = USB_DEVICE_ID_MATCH_DEVICE > > | USB_DEVICE_ID_MATCH_INT_INFO, > > @@ -2294,7 +2323,7 @@ static int uvc_clock_param_set(const char *val, struct > kernel_param *kp) .bInterfaceClass= USB_CLASS_VIDEO, > .bInterfaceSubClass = 1, > .bInterfaceProtocol = 0, > - .driver_info = UVC_QUIRK_PROBE_MINMAX }, > + .driver_info = (kernel_ulong_t)&uvc_quirk_probe_minmax }, > /* Microsoft Lifecam NX-6000 */ > { .match_flags = USB_DEVICE_ID_MATCH_DEVICE > > | USB_DEVICE_ID_MATCH_INT_INFO, > > @@ -2303,7 +2332,7 @@ static int uvc_clock_param_set(const char *val, struct > kernel_param *kp) .bInterfaceClass= USB_CLASS_VIDEO, > .bInterfaceSubClass = 1, > .bInterfaceProtocol = 0, > - .driver_info = UVC_QUIRK_PROBE_MINMAX }, > + .driver_info = (kernel_ulong_t)&uvc_quirk_probe_minmax }, > /* Microsoft Lifecam NX-3000 */ > { .match_flags = USB_DEVICE_ID_MATCH_
[PATCH 2/3 v7] uvcvideo: add extensible device information
From: Guennadi Liakhovetski Currently the UVC driver assigns a quirk bitmask to the .driver_info field of struct usb_device_id. This patch instroduces a struct to store quirks and possibly other per-device parameters in the future. Signed-off-by: Guennadi Liakhovetski --- v7: this is a new patch in the series. Needed to enable specifying private camera metadata formats drivers/media/usb/uvc/uvc_driver.c | 127 +++-- 1 file changed, 78 insertions(+), 49 deletions(-) diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index 6d22b22..cbf79b9 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -2001,11 +2001,18 @@ static int uvc_register_chains(struct uvc_device *dev) * USB probe, disconnect, suspend and resume */ +struct uvc_device_info { + u32 quirks; +}; + static int uvc_probe(struct usb_interface *intf, const struct usb_device_id *id) { struct usb_device *udev = interface_to_usbdev(intf); struct uvc_device *dev; + const struct uvc_device_info *info = + (const struct uvc_device_info *)id->driver_info; + u32 quirks = info ? info->quirks : 0; int function; int ret; @@ -2032,7 +2039,7 @@ static int uvc_probe(struct usb_interface *intf, dev->intf = usb_get_intf(intf); dev->intfnum = intf->cur_altsetting->desc.bInterfaceNumber; dev->quirks = (uvc_quirks_param == -1) - ? id->driver_info : uvc_quirks_param; + ? quirks : uvc_quirks_param; if (udev->product != NULL) strlcpy(dev->name, udev->product, sizeof dev->name); @@ -2073,7 +2080,7 @@ static int uvc_probe(struct usb_interface *intf, le16_to_cpu(udev->descriptor.idVendor), le16_to_cpu(udev->descriptor.idProduct)); - if (dev->quirks != id->driver_info) { + if (dev->quirks != quirks) { uvc_printk(KERN_INFO, "Forcing device quirks to 0x%x by module " "parameter for testing purpose.\n", dev->quirks); uvc_printk(KERN_INFO, "Please report required quirks to the " @@ -2271,6 +2278,28 @@ static int uvc_clock_param_set(const char *val, struct kernel_param *kp) * Driver initialization and cleanup */ +static struct uvc_device_info uvc_quirk_probe_minmax = { + .quirks = UVC_QUIRK_PROBE_MINMAX, +}; + +static struct uvc_device_info uvc_quirk_fix_bandwidth = { + .quirks = UVC_QUIRK_FIX_BANDWIDTH, +}; + +static struct uvc_device_info uvc_quirk_probe_def = { + .quirks = UVC_QUIRK_PROBE_DEF, +}; + +static struct uvc_device_info uvc_quirk_stream_no_fid = { + .quirks = UVC_QUIRK_STREAM_NO_FID, +}; + +static struct uvc_device_info uvc_quirk_force_y8 = { + .quirks = UVC_QUIRK_FORCE_Y8, +}; + +#define UVC_QUIRK_INFO(q) (kernel_ulong_t)&(struct uvc_device_info){.quirks = q} + /* * The Logitech cameras listed below have their interface class set to * VENDOR_SPEC because they don't announce themselves as UVC devices, even @@ -2285,7 +2314,7 @@ static int uvc_clock_param_set(const char *val, struct kernel_param *kp) .bInterfaceClass = USB_CLASS_VIDEO, .bInterfaceSubClass = 1, .bInterfaceProtocol = 0, - .driver_info = UVC_QUIRK_PROBE_MINMAX }, + .driver_info = (kernel_ulong_t)&uvc_quirk_probe_minmax }, /* Genius eFace 2025 */ { .match_flags = USB_DEVICE_ID_MATCH_DEVICE | USB_DEVICE_ID_MATCH_INT_INFO, @@ -2294,7 +2323,7 @@ static int uvc_clock_param_set(const char *val, struct kernel_param *kp) .bInterfaceClass = USB_CLASS_VIDEO, .bInterfaceSubClass = 1, .bInterfaceProtocol = 0, - .driver_info = UVC_QUIRK_PROBE_MINMAX }, + .driver_info = (kernel_ulong_t)&uvc_quirk_probe_minmax }, /* Microsoft Lifecam NX-6000 */ { .match_flags = USB_DEVICE_ID_MATCH_DEVICE | USB_DEVICE_ID_MATCH_INT_INFO, @@ -2303,7 +2332,7 @@ static int uvc_clock_param_set(const char *val, struct kernel_param *kp) .bInterfaceClass = USB_CLASS_VIDEO, .bInterfaceSubClass = 1, .bInterfaceProtocol = 0, - .driver_info = UVC_QUIRK_PROBE_MINMAX }, + .driver_info = (kernel_ulong_t)&uvc_quirk_probe_minmax }, /* Microsoft Lifecam NX-3000 */ { .match_flags = USB_DEVICE_ID_MATCH_DEVICE | USB_DEVICE_ID_MATCH_INT_INFO, @@ -2312,7 +2341,7 @@ static int uvc_clock_param_set(const char *val, struct kernel_param *kp) .bInterfaceClass = USB_CLASS_VIDEO, .bInterfaceSubClass = 1, .bInterfaceProtocol = 0, - .driver_info = UVC_QUIRK_PROBE_DEF }, + .driver_info