diff mbox series

[v2,2/7] media: uvcvideo: Move guid to entity

Message ID 20201104180734.286789-3-ribalda@chromium.org
State Superseded
Headers show
Series [v2,1/7] media: uvcvideo: Use pr_cont() macro | expand

Commit Message

Ricardo Ribalda Nov. 4, 2020, 6:07 p.m. UTC 
Instead of having multiple copies of the entity guid on the code, move
it to the entity structure.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c   | 30 ++++--------------------------
 drivers/media/usb/uvc/uvc_driver.c | 21 +++++++++++++++++++--
 drivers/media/usb/uvc/uvcvideo.h   |  2 +-
 3 files changed, 24 insertions(+), 29 deletions(-)

Comments

Laurent Pinchart Nov. 6, 2020, 6:06 a.m. UTC | #1 
Hi Ricardo,

Thank you for the patch.

On Wed, Nov 04, 2020 at 07:07:29PM +0100, Ricardo Ribalda wrote:
> Instead of having multiple copies of the entity guid on the code, move

> it to the entity structure.

> 

> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

> ---

>  drivers/media/usb/uvc/uvc_ctrl.c   | 30 ++++--------------------------

>  drivers/media/usb/uvc/uvc_driver.c | 21 +++++++++++++++++++--

>  drivers/media/usb/uvc/uvcvideo.h   |  2 +-

>  3 files changed, 24 insertions(+), 29 deletions(-)

> 

> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c

> index f479d8971dfb..0e480b75e724 100644

> --- a/drivers/media/usb/uvc/uvc_ctrl.c

> +++ b/drivers/media/usb/uvc/uvc_ctrl.c

> @@ -826,31 +826,10 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping,

>   * Terminal and unit management

>   */

>  

> -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 int uvc_entity_match_guid(const struct uvc_entity *entity,

> -	const u8 guid[16])

> +				 const u8 guid[16])

>  {

> -	switch (UVC_ENTITY_TYPE(entity)) {

> -	case UVC_ITT_CAMERA:

> -		return memcmp(uvc_camera_guid, guid, 16) == 0;

> -

> -	case UVC_ITT_MEDIA_TRANSPORT_INPUT:

> -		return memcmp(uvc_media_transport_input_guid, guid, 16) == 0;

> -

> -	case UVC_VC_PROCESSING_UNIT:

> -		return memcmp(uvc_processing_guid, guid, 16) == 0;

> -

> -	case UVC_VC_EXTENSION_UNIT:

> -		return memcmp(entity->extension.guidExtensionCode,

> -			      guid, 16) == 0;

> -

> -	default:

> -		return 0;

> -	}

> +	return memcmp(entity->guid, guid, sizeof(entity->guid)) == 0;

>  }

>  

>  /* ------------------------------------------------------------------------

> @@ -1776,8 +1755,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,

>  	if (data == NULL)

>  		return -ENOMEM;

>  

> -	memcpy(info->entity, ctrl->entity->extension.guidExtensionCode,

> -	       sizeof(info->entity));

> +	memcpy(info->entity, ctrl->entity->guid, sizeof(info->entity));

>  	info->index = ctrl->index;

>  	info->selector = ctrl->index + 1;

>  

> @@ -1883,7 +1861,7 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain,

>  

>  	if (!found) {

>  		uvc_trace(UVC_TRACE_CONTROL, "Control %pUl/%u not found.\n",

> -			entity->extension.guidExtensionCode, xqry->selector);

> +			entity->guid, xqry->selector);

>  		return -ENOENT;

>  	}

>  

> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c

> index 9fc0b600eab1..77fea26faa9a 100644

> --- a/drivers/media/usb/uvc/uvc_driver.c

> +++ b/drivers/media/usb/uvc/uvc_driver.c

> @@ -1019,6 +1019,11 @@ static int uvc_parse_streaming(struct uvc_device *dev,

>  	return ret;

>  }

>  

> +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_processing_guid[16] = UVC_GUID_UVC_PROCESSING;

> +

>  static struct uvc_entity *uvc_alloc_entity(u16 type, u8 id,

>  		unsigned int num_pads, unsigned int extra_size)

>  {

> @@ -1038,6 +1043,18 @@ static struct uvc_entity *uvc_alloc_entity(u16 type, u8 id,

>  	entity->id = id;

>  	entity->type = type;

>  

> +	switch (type) {

> +	case UVC_ITT_CAMERA:

> +		memcpy(entity->guid, uvc_camera_guid, 16);

> +		break;

> +	case UVC_ITT_MEDIA_TRANSPORT_INPUT:

> +		memcpy(entity->guid, uvc_media_transport_input_guid, 16);

> +		break;

> +	case UVC_VC_PROCESSING_UNIT:

> +		memcpy(entity->guid, uvc_processing_guid, 16);

> +		break;

> +	}


Given that the GUID is set in uvc_parse_vendor_control() and
uvc_parse_standard_control() for extension units, I'm wondering if it
would make sense to move it there for the other entity types too. Up to
you. Otherwise, I'd add the following comment above the switch:

	/*
	 * Set the GUID for standard entity types. For extension units, the GUID
	 * is initialized by the caller.
	 */

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>


> +

>  	entity->num_links = 0;

>  	entity->num_pads = num_pads;

>  	entity->pads = ((void *)(entity + 1)) + extra_size;

> @@ -1109,7 +1126,7 @@ static int uvc_parse_vendor_control(struct uvc_device *dev,

>  		if (unit == NULL)

>  			return -ENOMEM;

>  

> -		memcpy(unit->extension.guidExtensionCode, &buffer[4], 16);

> +		memcpy(unit->guid, &buffer[4], 16);

>  		unit->extension.bNumControls = buffer[20];

>  		memcpy(unit->baSourceID, &buffer[22], p);

>  		unit->extension.bControlSize = buffer[22+p];

> @@ -1368,7 +1385,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev,

>  		if (unit == NULL)

>  			return -ENOMEM;

>  

> -		memcpy(unit->extension.guidExtensionCode, &buffer[4], 16);

> +		memcpy(unit->guid, &buffer[4], 16);

>  		unit->extension.bNumControls = buffer[20];

>  		memcpy(unit->baSourceID, &buffer[22], p);

>  		unit->extension.bControlSize = buffer[22+p];

> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h

> index a3dfacf069c4..df7bf2d104a3 100644

> --- a/drivers/media/usb/uvc/uvcvideo.h

> +++ b/drivers/media/usb/uvc/uvcvideo.h

> @@ -304,6 +304,7 @@ struct uvc_entity {

>  	u8 id;

>  	u16 type;

>  	char name[64];

> +	u8 guid[16];

>  

>  	/* Media controller-related fields. */

>  	struct video_device *vdev;

> @@ -342,7 +343,6 @@ struct uvc_entity {

>  		} selector;

>  

>  		struct {

> -			u8  guidExtensionCode[16];

>  			u8  bNumControls;

>  			u8  bControlSize;

>  			u8  *bmControls;


-- 
Regards,

Laurent Pinchart
Ricardo Ribalda Nov. 6, 2020, 8:45 a.m. UTC | #2 
Hi Laurent

Thanks for the review

On Fri, Nov 6, 2020 at 7:06 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>

> Hi Ricardo,

>

> Thank you for the patch.

>

> On Wed, Nov 04, 2020 at 07:07:29PM +0100, Ricardo Ribalda wrote:

> > Instead of having multiple copies of the entity guid on the code, move

> > it to the entity structure.

> >

> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

> > ---

> >  drivers/media/usb/uvc/uvc_ctrl.c   | 30 ++++--------------------------

> >  drivers/media/usb/uvc/uvc_driver.c | 21 +++++++++++++++++++--

> >  drivers/media/usb/uvc/uvcvideo.h   |  2 +-

> >  3 files changed, 24 insertions(+), 29 deletions(-)

> >

> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c

> > index f479d8971dfb..0e480b75e724 100644

> > --- a/drivers/media/usb/uvc/uvc_ctrl.c

> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c

> > @@ -826,31 +826,10 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping,

> >   * Terminal and unit management

> >   */

> >

> > -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 int uvc_entity_match_guid(const struct uvc_entity *entity,

> > -     const u8 guid[16])

> > +                              const u8 guid[16])

> >  {

> > -     switch (UVC_ENTITY_TYPE(entity)) {

> > -     case UVC_ITT_CAMERA:

> > -             return memcmp(uvc_camera_guid, guid, 16) == 0;

> > -

> > -     case UVC_ITT_MEDIA_TRANSPORT_INPUT:

> > -             return memcmp(uvc_media_transport_input_guid, guid, 16) == 0;

> > -

> > -     case UVC_VC_PROCESSING_UNIT:

> > -             return memcmp(uvc_processing_guid, guid, 16) == 0;

> > -

> > -     case UVC_VC_EXTENSION_UNIT:

> > -             return memcmp(entity->extension.guidExtensionCode,

> > -                           guid, 16) == 0;

> > -

> > -     default:

> > -             return 0;

> > -     }

> > +     return memcmp(entity->guid, guid, sizeof(entity->guid)) == 0;

> >  }

> >

> >  /* ------------------------------------------------------------------------

> > @@ -1776,8 +1755,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,

> >       if (data == NULL)

> >               return -ENOMEM;

> >

> > -     memcpy(info->entity, ctrl->entity->extension.guidExtensionCode,

> > -            sizeof(info->entity));

> > +     memcpy(info->entity, ctrl->entity->guid, sizeof(info->entity));

> >       info->index = ctrl->index;

> >       info->selector = ctrl->index + 1;

> >

> > @@ -1883,7 +1861,7 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain,

> >

> >       if (!found) {

> >               uvc_trace(UVC_TRACE_CONTROL, "Control %pUl/%u not found.\n",

> > -                     entity->extension.guidExtensionCode, xqry->selector);

> > +                     entity->guid, xqry->selector);

> >               return -ENOENT;

> >       }

> >

> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c

> > index 9fc0b600eab1..77fea26faa9a 100644

> > --- a/drivers/media/usb/uvc/uvc_driver.c

> > +++ b/drivers/media/usb/uvc/uvc_driver.c

> > @@ -1019,6 +1019,11 @@ static int uvc_parse_streaming(struct uvc_device *dev,

> >       return ret;

> >  }

> >

> > +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_processing_guid[16] = UVC_GUID_UVC_PROCESSING;

> > +

> >  static struct uvc_entity *uvc_alloc_entity(u16 type, u8 id,

> >               unsigned int num_pads, unsigned int extra_size)

> >  {

> > @@ -1038,6 +1043,18 @@ static struct uvc_entity *uvc_alloc_entity(u16 type, u8 id,

> >       entity->id = id;

> >       entity->type = type;

> >

> > +     switch (type) {

> > +     case UVC_ITT_CAMERA:

> > +             memcpy(entity->guid, uvc_camera_guid, 16);

> > +             break;

> > +     case UVC_ITT_MEDIA_TRANSPORT_INPUT:

> > +             memcpy(entity->guid, uvc_media_transport_input_guid, 16);

> > +             break;

> > +     case UVC_VC_PROCESSING_UNIT:

> > +             memcpy(entity->guid, uvc_processing_guid, 16);

> > +             break;

> > +     }

>

> Given that the GUID is set in uvc_parse_vendor_control() and

> uvc_parse_standard_control() for extension units, I'm wondering if it

> would make sense to move it there for the other entity types too. Up to

> you. Otherwise, I'd add the following comment above the switch:

>

>         /*

>          * Set the GUID for standard entity types. For extension units, the GUID

>          * is initialized by the caller.

>          */


I added the comment. So far I am working on

https://github.com/ribalda/linux/tree/uvctest-v3

Please let me know when you are ready with v2, to send v3 to the mailing list.

Thanks!

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>

> > +

> >       entity->num_links = 0;

> >       entity->num_pads = num_pads;

> >       entity->pads = ((void *)(entity + 1)) + extra_size;

> > @@ -1109,7 +1126,7 @@ static int uvc_parse_vendor_control(struct uvc_device *dev,

> >               if (unit == NULL)

> >                       return -ENOMEM;

> >

> > -             memcpy(unit->extension.guidExtensionCode, &buffer[4], 16);

> > +             memcpy(unit->guid, &buffer[4], 16);

> >               unit->extension.bNumControls = buffer[20];

> >               memcpy(unit->baSourceID, &buffer[22], p);

> >               unit->extension.bControlSize = buffer[22+p];

> > @@ -1368,7 +1385,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev,

> >               if (unit == NULL)

> >                       return -ENOMEM;

> >

> > -             memcpy(unit->extension.guidExtensionCode, &buffer[4], 16);

> > +             memcpy(unit->guid, &buffer[4], 16);

> >               unit->extension.bNumControls = buffer[20];

> >               memcpy(unit->baSourceID, &buffer[22], p);

> >               unit->extension.bControlSize = buffer[22+p];

> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h

> > index a3dfacf069c4..df7bf2d104a3 100644

> > --- a/drivers/media/usb/uvc/uvcvideo.h

> > +++ b/drivers/media/usb/uvc/uvcvideo.h

> > @@ -304,6 +304,7 @@ struct uvc_entity {

> >       u8 id;

> >       u16 type;

> >       char name[64];

> > +     u8 guid[16];

> >

> >       /* Media controller-related fields. */

> >       struct video_device *vdev;

> > @@ -342,7 +343,6 @@ struct uvc_entity {

> >               } selector;

> >

> >               struct {

> > -                     u8  guidExtensionCode[16];

> >                       u8  bNumControls;

> >                       u8  bControlSize;

> >                       u8  *bmControls;

>

> --

> Regards,

>

> Laurent Pinchart




-- 
Ricardo Ribalda
diff mbox series

Patch

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index f479d8971dfb..0e480b75e724 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -826,31 +826,10 @@  static void uvc_set_le_value(struct uvc_control_mapping *mapping,
  * Terminal and unit management
  */
 
-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 int uvc_entity_match_guid(const struct uvc_entity *entity,
-	const u8 guid[16])
+				 const u8 guid[16])
 {
-	switch (UVC_ENTITY_TYPE(entity)) {
-	case UVC_ITT_CAMERA:
-		return memcmp(uvc_camera_guid, guid, 16) == 0;
-
-	case UVC_ITT_MEDIA_TRANSPORT_INPUT:
-		return memcmp(uvc_media_transport_input_guid, guid, 16) == 0;
-
-	case UVC_VC_PROCESSING_UNIT:
-		return memcmp(uvc_processing_guid, guid, 16) == 0;
-
-	case UVC_VC_EXTENSION_UNIT:
-		return memcmp(entity->extension.guidExtensionCode,
-			      guid, 16) == 0;
-
-	default:
-		return 0;
-	}
+	return memcmp(entity->guid, guid, sizeof(entity->guid)) == 0;
 }
 
 /* ------------------------------------------------------------------------
@@ -1776,8 +1755,7 @@  static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
 	if (data == NULL)
 		return -ENOMEM;
 
-	memcpy(info->entity, ctrl->entity->extension.guidExtensionCode,
-	       sizeof(info->entity));
+	memcpy(info->entity, ctrl->entity->guid, sizeof(info->entity));
 	info->index = ctrl->index;
 	info->selector = ctrl->index + 1;
 
@@ -1883,7 +1861,7 @@  int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
 
 	if (!found) {
 		uvc_trace(UVC_TRACE_CONTROL, "Control %pUl/%u not found.\n",
-			entity->extension.guidExtensionCode, xqry->selector);
+			entity->guid, xqry->selector);
 		return -ENOENT;
 	}
 
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 9fc0b600eab1..77fea26faa9a 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1019,6 +1019,11 @@  static int uvc_parse_streaming(struct uvc_device *dev,
 	return ret;
 }
 
+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_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
+
 static struct uvc_entity *uvc_alloc_entity(u16 type, u8 id,
 		unsigned int num_pads, unsigned int extra_size)
 {
@@ -1038,6 +1043,18 @@  static struct uvc_entity *uvc_alloc_entity(u16 type, u8 id,
 	entity->id = id;
 	entity->type = type;
 
+	switch (type) {
+	case UVC_ITT_CAMERA:
+		memcpy(entity->guid, uvc_camera_guid, 16);
+		break;
+	case UVC_ITT_MEDIA_TRANSPORT_INPUT:
+		memcpy(entity->guid, uvc_media_transport_input_guid, 16);
+		break;
+	case UVC_VC_PROCESSING_UNIT:
+		memcpy(entity->guid, uvc_processing_guid, 16);
+		break;
+	}
+
 	entity->num_links = 0;
 	entity->num_pads = num_pads;
 	entity->pads = ((void *)(entity + 1)) + extra_size;
@@ -1109,7 +1126,7 @@  static int uvc_parse_vendor_control(struct uvc_device *dev,
 		if (unit == NULL)
 			return -ENOMEM;
 
-		memcpy(unit->extension.guidExtensionCode, &buffer[4], 16);
+		memcpy(unit->guid, &buffer[4], 16);
 		unit->extension.bNumControls = buffer[20];
 		memcpy(unit->baSourceID, &buffer[22], p);
 		unit->extension.bControlSize = buffer[22+p];
@@ -1368,7 +1385,7 @@  static int uvc_parse_standard_control(struct uvc_device *dev,
 		if (unit == NULL)
 			return -ENOMEM;
 
-		memcpy(unit->extension.guidExtensionCode, &buffer[4], 16);
+		memcpy(unit->guid, &buffer[4], 16);
 		unit->extension.bNumControls = buffer[20];
 		memcpy(unit->baSourceID, &buffer[22], p);
 		unit->extension.bControlSize = buffer[22+p];
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index a3dfacf069c4..df7bf2d104a3 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -304,6 +304,7 @@  struct uvc_entity {
 	u8 id;
 	u16 type;
 	char name[64];
+	u8 guid[16];
 
 	/* Media controller-related fields. */
 	struct video_device *vdev;
@@ -342,7 +343,6 @@  struct uvc_entity {
 		} selector;
 
 		struct {
-			u8  guidExtensionCode[16];
 			u8  bNumControls;
 			u8  bControlSize;
 			u8  *bmControls;