2020-10-22 17:26:24

by Ricardo Ribalda

Subject: [PATCH 3/6] media: uvcvideo: Add UVC_GUID_EXT_GPIO_CONTROLLER

Create a new GUID for GPIO controller entities that do not belong to the
USB video device.

This GUID is selected on an address range completely different that the
UVC standard to avoid collisions.

Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/usb/uvc/uvc_ctrl.c | 4 ++++
drivers/media/usb/uvc/uvcvideo.h | 3 +++
2 files changed, 7 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 0a8835742d49..913739915863 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -830,6 +830,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_gpio_guid[16] = UVC_GUID_EXT_GPIO_CONTROLLER;

static int uvc_entity_match_guid(const struct uvc_entity *entity,
const u8 guid[16])
@@ -848,6 +849,9 @@ static int uvc_entity_match_guid(const struct uvc_entity *entity,
return memcmp(entity->extension.guidExtensionCode,
guid, 16) == 0;

+ case UVC_GPIO_UNIT:
+ return memcmp(uvc_gpio_guid, guid, 16) == 0;
+
default:
return 0;
}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 08922d889bb6..8e5a9fc35820 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -56,6 +56,9 @@
#define UVC_GUID_UVC_SELECTOR \
{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, \
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x02}
+#define UVC_GUID_EXT_GPIO_CONTROLLER \
+ {0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xf, \
+ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x01}

#define UVC_GUID_FORMAT_MJPEG \
{ 'M', 'J', 'P', 'G', 0x00, 0x00, 0x10, 0x00, \
--
2.29.0.rc1.297.gfa9743e501-goog

2020-11-04 11:35:30

by Laurent Pinchart

Subject: Re: [PATCH 3/6] media: uvcvideo: Add UVC_GUID_EXT_GPIO_CONTROLLER

Hi Ricardo,

Thank you for the patch.

On Thu, Oct 22, 2020 at 03:37:50PM +0200, Ricardo Ribalda wrote:
> Create a new GUID for GPIO controller entities that do not belong to the
> USB video device.
>
> This GUID is selected on an address range completely different that the
> UVC standard to avoid collisions.

I'd squash this patch with 5/6.

> Signed-off-by: Ricardo Ribalda <[email protected]>
> ---
> drivers/media/usb/uvc/uvc_ctrl.c | 4 ++++
> drivers/media/usb/uvc/uvcvideo.h | 3 +++
> 2 files changed, 7 insertions(+)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 0a8835742d49..913739915863 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -830,6 +830,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_gpio_guid[16] = UVC_GUID_EXT_GPIO_CONTROLLER;
>
> static int uvc_entity_match_guid(const struct uvc_entity *entity,
> const u8 guid[16])
> @@ -848,6 +849,9 @@ static int uvc_entity_match_guid(const struct uvc_entity *entity,
> return memcmp(entity->extension.guidExtensionCode,
> guid, 16) == 0;
>
> + case UVC_GPIO_UNIT:

This won't compile, UVC_GPIO_UNIT is defined in 5/6.

> + return memcmp(uvc_gpio_guid, guid, 16) == 0;

I wonder if it would make sense to store the GUID in the uvc_entity
structure instead of adding new entries to this function.

> +
> default:
> return 0;
> }
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 08922d889bb6..8e5a9fc35820 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -56,6 +56,9 @@
> #define UVC_GUID_UVC_SELECTOR \
> {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, \
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x02}

None of the GUIDs above are defined by the UVC specification. You could
use { 0x00 * 14, 0x01, 0x03 } or { 0x00 * 14, 0x02, 0x01 } instead of
going for 0xff. Not that it matters much, it's all internal.

> +#define UVC_GUID_EXT_GPIO_CONTROLLER \
> + {0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xf, \

I assume the last value was meant to be 0xff ?

> + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x01}
>
> #define UVC_GUID_FORMAT_MJPEG \
> { 'M', 'J', 'P', 'G', 0x00, 0x00, 0x10, 0x00, \

--
Regards,

Laurent Pinchart