Re: [PATCH 2/3 v7] uvcvideo: add extensible device information

[Guennadi Liakhovetski]

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)_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)_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,
> >   

Re: [PATCH 2/3 v7] uvcvideo: add extensible device information

[Laurent Pinchart]

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)_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)_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)_quirk_probe_minmax },
>   /* Microsoft Lifecam NX-3000 */

[PATCH 2/3 v7] uvcvideo: add extensible device information

[Guennadi Liakhovetski]

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)_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)_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)_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