Re: [PATCH v1 06/19] uvcvideo: Recognize UVC 1.5 encoding units.

2013-11-10 Thread Laurent Pinchart 
Hi Pawel,

Thank you for the patch.

On Friday 30 August 2013 11:17:05 Pawel Osciak wrote:
 Add encoding unit definitions and descriptor parsing code and allow them to
 be added to chains.
 
 Signed-off-by: Pawel Osciak posc...@chromium.org
 ---
  drivers/media/usb/uvc/uvc_ctrl.c   | 37 ++---
  drivers/media/usb/uvc/uvc_driver.c | 67 ++-
  drivers/media/usb/uvc/uvcvideo.h   | 14 +++-
  include/uapi/linux/usb/video.h |  1 +
  4 files changed, 105 insertions(+), 14 deletions(-)
 
 diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
 b/drivers/media/usb/uvc/uvc_ctrl.c index ba159a4..72d6724 100644
 --- a/drivers/media/usb/uvc/uvc_ctrl.c
 +++ b/drivers/media/usb/uvc/uvc_ctrl.c
 @@ -777,6 +777,7 @@ static const __u8 uvc_processing_guid[16] =
 UVC_GUID_UVC_PROCESSING; static const __u8 uvc_camera_guid[16] =
 UVC_GUID_UVC_CAMERA;
  static const __u8 uvc_media_transport_input_guid[16] =
   UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
 +static const __u8 uvc_encoding_guid[16] = UVC_GUID_UVC_ENCODING;
 
  static int uvc_entity_match_guid(const struct uvc_entity *entity,
   const __u8 guid[16])
 @@ -795,6 +796,9 @@ static int uvc_entity_match_guid(const struct uvc_entity
 *entity, return memcmp(entity-extension.guidExtensionCode,
 guid, 16) == 0;
 
 + case UVC_VC_ENCODING_UNIT:
 + return memcmp(uvc_encoding_guid, guid, 16) == 0;
 +
   default:
   return 0;
   }
 @@ -2105,12 +2109,13 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
  {
   struct uvc_entity *entity;
   unsigned int i;
 + int num_found;
 
   /* Walk the entities list and instantiate controls */
   list_for_each_entry(entity, dev-entities, list) {
   struct uvc_control *ctrl;
 - unsigned int bControlSize = 0, ncontrols;
 - __u8 *bmControls = NULL;
 + unsigned int bControlSize = 0, ncontrols = 0;
 + __u8 *bmControls = NULL, *bmControlsRuntime = NULL;
 
   if (UVC_ENTITY_TYPE(entity) == UVC_VC_EXTENSION_UNIT) {
   bmControls = entity-extension.bmControls;
 @@ -2121,13 +2126,25 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
   } else if (UVC_ENTITY_TYPE(entity) == UVC_ITT_CAMERA) {
   bmControls = entity-camera.bmControls;
   bControlSize = entity-camera.bControlSize;
 + } else if (UVC_ENTITY_TYPE(entity) == UVC_VC_ENCODING_UNIT) {
 + bmControls = entity-encoding.bmControls;
 + bmControlsRuntime = entity-encoding.bmControlsRuntime;
 + bControlSize = entity-encoding.bControlSize;
   }
 
   /* Remove bogus/blacklisted controls */
   uvc_ctrl_prune_entity(dev, entity);
 
   /* Count supported controls and allocate the controls array */
 - ncontrols = memweight(bmControls, bControlSize);
 + for (i = 0; i  bControlSize; ++i) {
 + if (bmControlsRuntime) {
 + ncontrols += hweight8(bmControls[i]
 +   | bmControlsRuntime[i]);

Nitpicking, could you move the | to the end of the previous line to match the 
style of the rest of the code ?

 + } else {
 + ncontrols += hweight8(bmControls[i]);
 + }

The { } are not needed.

 + }
 +
   if (ncontrols == 0)
   continue;
 
 @@ -2139,8 +2156,17 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
 
   /* Initialize all supported controls */
   ctrl = entity-controls;
 - for (i = 0; i  bControlSize * 8; ++i) {
 - if (uvc_test_bit(bmControls, i) == 0)
 + for (i = 0, num_found = 0;
 + i  bControlSize * 8  num_found  ncontrols; ++i) {
 + if (uvc_test_bit(bmControls, i) == 1)
 + ctrl-on_init = 1;
 + if (bmControlsRuntime 
 + uvc_test_bit(bmControlsRuntime, i) == 1)
 + ctrl-in_runtime = 1;
 + else if (!bmControlsRuntime)
 + ctrl-in_runtime = ctrl-on_init;
 +
 + if (ctrl-on_init == 0  ctrl-in_runtime == 0)
   continue;

Wouldn't it simplify the code if you assigned bmControls to bmControlsRuntime 
when bmControlsRuntime is NULL before counting the controls above ? You could 
get rid of the if inside the count loop, and you could also simplify this 
construct.

 
   ctrl-entity = entity;
 @@ -2148,6 +2174,7 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
 
   uvc_ctrl_init_ctrl(dev, ctrl);
   ctrl++;
 + num_found++;

[PATCH v1 06/19] uvcvideo: Recognize UVC 1.5 encoding units.

2013-08-29 Thread Pawel Osciak 
Add encoding unit definitions and descriptor parsing code and allow them to
be added to chains.

Signed-off-by: Pawel Osciak posc...@chromium.org
---
 drivers/media/usb/uvc/uvc_ctrl.c   | 37 ++---
 drivers/media/usb/uvc/uvc_driver.c | 67 +-
 drivers/media/usb/uvc/uvcvideo.h   | 14 +++-
 include/uapi/linux/usb/video.h |  1 +
 4 files changed, 105 insertions(+), 14 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index ba159a4..72d6724 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -777,6 +777,7 @@ static const __u8 uvc_processing_guid[16] = 
UVC_GUID_UVC_PROCESSING;
 static const __u8 uvc_camera_guid[16] = UVC_GUID_UVC_CAMERA;
 static const __u8 uvc_media_transport_input_guid[16] =
UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
+static const __u8 uvc_encoding_guid[16] = UVC_GUID_UVC_ENCODING;
 
 static int uvc_entity_match_guid(const struct uvc_entity *entity,
const __u8 guid[16])
@@ -795,6 +796,9 @@ static int uvc_entity_match_guid(const struct uvc_entity 
*entity,
return memcmp(entity-extension.guidExtensionCode,
  guid, 16) == 0;
 
+   case UVC_VC_ENCODING_UNIT:
+   return memcmp(uvc_encoding_guid, guid, 16) == 0;
+
default:
return 0;
}
@@ -2105,12 +2109,13 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
 {
struct uvc_entity *entity;
unsigned int i;
+   int num_found;
 
/* Walk the entities list and instantiate controls */
list_for_each_entry(entity, dev-entities, list) {
struct uvc_control *ctrl;
-   unsigned int bControlSize = 0, ncontrols;
-   __u8 *bmControls = NULL;
+   unsigned int bControlSize = 0, ncontrols = 0;
+   __u8 *bmControls = NULL, *bmControlsRuntime = NULL;
 
if (UVC_ENTITY_TYPE(entity) == UVC_VC_EXTENSION_UNIT) {
bmControls = entity-extension.bmControls;
@@ -2121,13 +2126,25 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
} else if (UVC_ENTITY_TYPE(entity) == UVC_ITT_CAMERA) {
bmControls = entity-camera.bmControls;
bControlSize = entity-camera.bControlSize;
+   } else if (UVC_ENTITY_TYPE(entity) == UVC_VC_ENCODING_UNIT) {
+   bmControls = entity-encoding.bmControls;
+   bmControlsRuntime = entity-encoding.bmControlsRuntime;
+   bControlSize = entity-encoding.bControlSize;
}
 
/* Remove bogus/blacklisted controls */
uvc_ctrl_prune_entity(dev, entity);
 
/* Count supported controls and allocate the controls array */
-   ncontrols = memweight(bmControls, bControlSize);
+   for (i = 0; i  bControlSize; ++i) {
+   if (bmControlsRuntime) {
+   ncontrols += hweight8(bmControls[i]
+ | bmControlsRuntime[i]);
+   } else {
+   ncontrols += hweight8(bmControls[i]);
+   }
+   }
+
if (ncontrols == 0)
continue;
 
@@ -2139,8 +2156,17 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
 
/* Initialize all supported controls */
ctrl = entity-controls;
-   for (i = 0; i  bControlSize * 8; ++i) {
-   if (uvc_test_bit(bmControls, i) == 0)
+   for (i = 0, num_found = 0;
+   i  bControlSize * 8  num_found  ncontrols; ++i) {
+   if (uvc_test_bit(bmControls, i) == 1)
+   ctrl-on_init = 1;
+   if (bmControlsRuntime 
+   uvc_test_bit(bmControlsRuntime, i) == 1)
+   ctrl-in_runtime = 1;
+   else if (!bmControlsRuntime)
+   ctrl-in_runtime = ctrl-on_init;
+
+   if (ctrl-on_init == 0  ctrl-in_runtime == 0)
continue;
 
ctrl-entity = entity;
@@ -2148,6 +2174,7 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
 
uvc_ctrl_init_ctrl(dev, ctrl);
ctrl++;
+   num_found++;
}
}
 
diff --git a/drivers/media/usb/uvc/uvc_driver.c 
b/drivers/media/usb/uvc/uvc_driver.c
index d7ff707..d950b40 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1155,6 +1155,37 @@ static int uvc_parse_standard_control(struct uvc_device 
*dev,
list_add_tail(unit-list, dev-entities);
break;
 
+   case UVC_VC_ENCODING_UNIT:
+