Hi Pawel,

Thank you so much for the patch (set) ! I'll probably need a couple of days to 
review it all, let's start.

On Friday 30 August 2013 11:17:00 Pawel Osciak wrote:
> Add a new trace argument enabling UVC query details and contents logging.
> 
> Signed-off-by: Pawel Osciak <posc...@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++--------------
>  drivers/media/usb/uvc/uvcvideo.h  |  9 ++++++++
>  2 files changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index 3394c34..695f6d9 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -29,22 +29,6 @@
>  /* ------------------------------------------------------------------------
> * UVC Controls
>   */
> -
> -static int __uvc_query_ctrl(struct uvc_device *dev, __u8 query, __u8 unit,
> -                     __u8 intfnum, __u8 cs, void *data, __u16 size,
> -                     int timeout)
> -{
> -     __u8 type = USB_TYPE_CLASS | USB_RECIP_INTERFACE;
> -     unsigned int pipe;
> -
> -     pipe = (query & 0x80) ? usb_rcvctrlpipe(dev->udev, 0)
> -                           : usb_sndctrlpipe(dev->udev, 0);
> -     type |= (query & 0x80) ? USB_DIR_IN : USB_DIR_OUT;
> -
> -     return usb_control_msg(dev->udev, pipe, query, type, cs << 8,
> -                     unit << 8 | intfnum, data, size, timeout);
> -}
> -
>  static const char *uvc_query_name(__u8 query)
>  {
>       switch (query) {
> @@ -69,6 +53,35 @@ static const char *uvc_query_name(__u8 query)
>       }
>  }
> 
> +static int __uvc_query_ctrl(struct uvc_device *dev, __u8 query, __u8 unit,
> +                     __u8 intfnum, __u8 cs, void *data, __u16 size,
> +                     int timeout)
> +{
> +     __u8 type = USB_TYPE_CLASS | USB_RECIP_INTERFACE;
> +     unsigned int pipe;
> +     int ret;
> +
> +     pipe = (query & 0x80) ? usb_rcvctrlpipe(dev->udev, 0)
> +                           : usb_sndctrlpipe(dev->udev, 0);
> +     type |= (query & 0x80) ? USB_DIR_IN : USB_DIR_OUT;
> +
> +     uvc_trace(UVC_TRACE_QUERY,
> +                     "%s (%d): size=%d, unit=%d, cs=%d, intf=%d\n",

All the fields are unsigned, shouldn't you use %u instead of %d ?

> +                     uvc_query_name(query), query, size, unit, cs, intfnum);

That's a pretty strange indentation :-)

> +     uvc_trace(UVC_TRACE_QUERY, "Sent:\n");
> +     uvc_print_hex_dump(UVC_TRACE_QUERY, data, size);
> +
> +     ret = usb_control_msg(dev->udev, pipe, query, type, cs << 8,
> +                     unit << 8 | intfnum, data, size, timeout);
> +     if (ret == -EPIPE)
> +             uvc_trace(UVC_TRACE_QUERY, "Got device STALL on query!\n");
> +
> +     uvc_trace(UVC_TRACE_QUERY, "Received:\n");
> +     uvc_print_hex_dump(UVC_TRACE_QUERY, data, size);
> +
> +     return ret;
> +}
> +
>  int uvc_query_ctrl(struct uvc_device *dev, __u8 query, __u8 unit,
>                       __u8 intfnum, __u8 cs, void *data, __u16 size)
>  {
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index 9e35982..75e0153 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -574,6 +574,7 @@ struct uvc_driver {
>  #define UVC_TRACE_VIDEO              (1 << 10)
>  #define UVC_TRACE_STATS              (1 << 11)
>  #define UVC_TRACE_CLOCK              (1 << 12)
> +#define UVC_TRACE_QUERY              (1 << 13)
> 
>  #define UVC_WARN_MINMAX              0
>  #define UVC_WARN_PROBE_DEF   1
> @@ -599,6 +600,14 @@ extern unsigned int uvc_timeout_param;
>  #define uvc_printk(level, msg...) \
>       printk(level "uvcvideo: " msg)
> 
> +#define uvc_print_hex_dump(flag, buf, len) \
> +     do { \
> +             if (uvc_trace_param & flag) { \
> +                     print_hex_dump(KERN_DEBUG, "uvcvideo: ", \
> +                             DUMP_PREFIX_NONE, 16, 1, buf, len, false); \
> +             } \

No need for braces around the print_hex_dump() call.

> +     } while (0)
> +
>  /*
> --------------------------------------------------------------------------
> * Internal functions.
>   */

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to