[PATCH 0/6] Show privacy_gpio as a v4l2_ctrl

[PATCH 0/6] Show privacy_gpio as a v4l2_ctrl

[Ricardo Ribalda]

Some devices can implement a physical switch to disable the input of the
camera on demand. Think of it like an elegant privacy sticker.
    
The system can read the status of the privacy switch via a GPIO.

The ACPI table maps this GPIO to the USB device via _CRS and _DSD
descriptors, so the kernel can find it.

The userspace applications need to know if the privacy pin is enabled
or not. 

The obvious way to show it to userspace is via the V4L2_CID_PRIVACY control.

This patchset implement this functionality.

Ricardo Ribalda (6):
  media: uvcvideo: Add UVC_CTRL_FLAG_ENTITY_GET_INFO
  media: uvcvideo: Add UVC_CTRL_FLAG_ENTITY_GET_CUR
  media: uvcvideo: Add UVC_GUID_EXT_GPIO_CONTROLLER
  media: uvcvideo: Add Privacy control based on EXT_GPIO
  media: uvcvideo: Implement UVC_GPIO_UNIT
  media: uvcvideo: Handle IRQs from the privacy_pin

 drivers/media/usb/uvc/uvc_ctrl.c   |  56 ++++++++++++++--
 drivers/media/usb/uvc/uvc_driver.c | 102 +++++++++++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  14 ++++
 include/uapi/linux/uvcvideo.h      |   3 +
 4 files changed, 169 insertions(+), 6 deletions(-)

-- 
2.29.0.rc1.297.gfa9743e501-goog

[PATCH 1/6] media: uvcvideo: Add UVC_CTRL_FLAG_ENTITY_GET_INFO

[Ricardo Ribalda]

This flag allows controls to get their properties from an entity defined
function instead of via a query to the USB device.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 9 +++++++--
 drivers/media/usb/uvc/uvcvideo.h | 3 +++
 include/uapi/linux/uvcvideo.h    | 2 ++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index f479d8971dfb..7acdc055613b 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1708,8 +1708,13 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
 	if (data == NULL)
 		return -ENOMEM;
 
-	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
-			     info->selector, data, 1);
+	if (ctrl->info.flags & UVC_CTRL_FLAG_ENTITY_GET_INFO)
+		ret = ctrl->entity->get_info ?
+			ctrl->entity->get_info(ctrl->entity, ctrl->info.selector, data) :
+			-EINVAL;
+	else
+		ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
+				     info->selector, data, 1);
 	if (!ret)
 		info->flags |= (data[0] & UVC_CONTROL_CAP_GET ?
 				UVC_CTRL_FLAG_GET_CUR : 0)
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index a3dfacf069c4..08922d889bb6 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -353,6 +353,9 @@ struct uvc_entity {
 	u8 bNrInPins;
 	u8 *baSourceID;
 
+	int (*get_info)(struct uvc_entity *entity, u8 cs, u8 *caps);
+	int (*get_cur)(struct uvc_entity *entity, u8 cs, void *data, u16 size);
+
 	unsigned int ncontrols;
 	struct uvc_control *controls;
 };
diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
index f80f05b3c423..69b636290c31 100644
--- a/include/uapi/linux/uvcvideo.h
+++ b/include/uapi/linux/uvcvideo.h
@@ -30,6 +30,8 @@
 #define UVC_CTRL_FLAG_AUTO_UPDATE	(1 << 7)
 /* Control supports asynchronous reporting */
 #define UVC_CTRL_FLAG_ASYNCHRONOUS	(1 << 8)
+/* Entity queries */
+#define UVC_CTRL_FLAG_ENTITY_GET_INFO	(1 << 9)
 
 #define UVC_CTRL_FLAG_GET_RANGE \
 	(UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_GET_MIN | \
-- 
2.29.0.rc1.297.gfa9743e501-goog

Re: [PATCH 1/6] media: uvcvideo: Add UVC_CTRL_FLAG_ENTITY_GET_INFO

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Thu, Oct 22, 2020 at 03:37:48PM +0200, Ricardo Ribalda wrote:
> This flag allows controls to get their properties from an entity defined

s/entity defined/entity-defined/

> function instead of via a query to the USB device.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 9 +++++++--
>  drivers/media/usb/uvc/uvcvideo.h | 3 +++
>  include/uapi/linux/uvcvideo.h    | 2 ++
>  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index f479d8971dfb..7acdc055613b 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1708,8 +1708,13 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
>  	if (data == NULL)
>  		return -ENOMEM;
>  
> -	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> -			     info->selector, data, 1);
> +	if (ctrl->info.flags & UVC_CTRL_FLAG_ENTITY_GET_INFO)

Do we need the flag, couldn't we use entity->get_info if it is non-null,
and call uvc_query_ctrl() otherwise ?

> +		ret = ctrl->entity->get_info ?
> +			ctrl->entity->get_info(ctrl->entity, ctrl->info.selector, data) :
> +			-EINVAL;
> +	else
> +		ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> +				     info->selector, data, 1);
>  	if (!ret)
>  		info->flags |= (data[0] & UVC_CONTROL_CAP_GET ?
>  				UVC_CTRL_FLAG_GET_CUR : 0)
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index a3dfacf069c4..08922d889bb6 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -353,6 +353,9 @@ struct uvc_entity {
>  	u8 bNrInPins;
>  	u8 *baSourceID;
>  
> +	int (*get_info)(struct uvc_entity *entity, u8 cs, u8 *caps);
> +	int (*get_cur)(struct uvc_entity *entity, u8 cs, void *data, u16 size);

Looks like the second function should be part of patch 2/6 instead. I
would however squash 1/6 and 2/6.

> +
>  	unsigned int ncontrols;
>  	struct uvc_control *controls;
>  };
> diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
> index f80f05b3c423..69b636290c31 100644
> --- a/include/uapi/linux/uvcvideo.h
> +++ b/include/uapi/linux/uvcvideo.h
> @@ -30,6 +30,8 @@
>  #define UVC_CTRL_FLAG_AUTO_UPDATE	(1 << 7)
>  /* Control supports asynchronous reporting */
>  #define UVC_CTRL_FLAG_ASYNCHRONOUS	(1 << 8)
> +/* Entity queries */
> +#define UVC_CTRL_FLAG_ENTITY_GET_INFO	(1 << 9)
>  
>  #define UVC_CTRL_FLAG_GET_RANGE \
>  	(UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_GET_MIN | \

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] media: uvcvideo: Add UVC_CTRL_FLAG_ENTITY_GET_INFO

[Ricardo Ribalda]

On Wed, Nov 4, 2020 at 12:10 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Thu, Oct 22, 2020 at 03:37:48PM +0200, Ricardo Ribalda wrote:
> > This flag allows controls to get their properties from an entity defined
>
> s/entity defined/entity-defined/
>
> > function instead of via a query to the USB device.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c | 9 +++++++--
> >  drivers/media/usb/uvc/uvcvideo.h | 3 +++
> >  include/uapi/linux/uvcvideo.h    | 2 ++
> >  3 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index f479d8971dfb..7acdc055613b 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1708,8 +1708,13 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
> >       if (data == NULL)
> >               return -ENOMEM;
> >
> > -     ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> > -                          info->selector, data, 1);
> > +     if (ctrl->info.flags & UVC_CTRL_FLAG_ENTITY_GET_INFO)
>
> Do we need the flag, couldn't we use entity->get_info if it is non-null,
> and call uvc_query_ctrl() otherwise ?

The idea behind the flag is to support in the same entity controls
that are uvc_query_ctrl() based
and "entity private functions".

As this moment, there is only the " GPIO entity"  that has has private
functions, and does not require it.

So I can simply remove the flag and add it later (if needed).

Thanks

>
> > +             ret = ctrl->entity->get_info ?
> > +                     ctrl->entity->get_info(ctrl->entity, ctrl->info.selector, data) :
> > +                     -EINVAL;
> > +     else
> > +             ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> > +                                  info->selector, data, 1);
> >       if (!ret)
> >               info->flags |= (data[0] & UVC_CONTROL_CAP_GET ?
> >                               UVC_CTRL_FLAG_GET_CUR : 0)
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index a3dfacf069c4..08922d889bb6 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -353,6 +353,9 @@ struct uvc_entity {
> >       u8 bNrInPins;
> >       u8 *baSourceID;
> >
> > +     int (*get_info)(struct uvc_entity *entity, u8 cs, u8 *caps);
> > +     int (*get_cur)(struct uvc_entity *entity, u8 cs, void *data, u16 size);
>
> Looks like the second function should be part of patch 2/6 instead. I
> would however squash 1/6 and 2/6.
>
> > +
> >       unsigned int ncontrols;
> >       struct uvc_control *controls;
> >  };
> > diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
> > index f80f05b3c423..69b636290c31 100644
> > --- a/include/uapi/linux/uvcvideo.h
> > +++ b/include/uapi/linux/uvcvideo.h
> > @@ -30,6 +30,8 @@
> >  #define UVC_CTRL_FLAG_AUTO_UPDATE    (1 << 7)
> >  /* Control supports asynchronous reporting */
> >  #define UVC_CTRL_FLAG_ASYNCHRONOUS   (1 << 8)
> > +/* Entity queries */
> > +#define UVC_CTRL_FLAG_ENTITY_GET_INFO        (1 << 9)
> >
> >  #define UVC_CTRL_FLAG_GET_RANGE \
> >       (UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_GET_MIN | \
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH 1/6] media: uvcvideo: Add UVC_CTRL_FLAG_ENTITY_GET_INFO

[Laurent Pinchart]

Hi Ricardo,

On Wed, Nov 04, 2020 at 12:48:25PM +0100, Ricardo Ribalda wrote:
> On Wed, Nov 4, 2020 at 12:10 PM Laurent Pinchart wrote:
> > On Thu, Oct 22, 2020 at 03:37:48PM +0200, Ricardo Ribalda wrote:
> > > This flag allows controls to get their properties from an entity defined
> >
> > s/entity defined/entity-defined/
> >
> > > function instead of via a query to the USB device.
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_ctrl.c | 9 +++++++--
> > >  drivers/media/usb/uvc/uvcvideo.h | 3 +++
> > >  include/uapi/linux/uvcvideo.h    | 2 ++
> > >  3 files changed, 12 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index f479d8971dfb..7acdc055613b 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -1708,8 +1708,13 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
> > >       if (data == NULL)
> > >               return -ENOMEM;
> > >
> > > -     ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> > > -                          info->selector, data, 1);
> > > +     if (ctrl->info.flags & UVC_CTRL_FLAG_ENTITY_GET_INFO)
> >
> > Do we need the flag, couldn't we use entity->get_info if it is non-null,
> > and call uvc_query_ctrl() otherwise ?
> 
> The idea behind the flag is to support in the same entity controls
> that are uvc_query_ctrl() based
> and "entity private functions".
> 
> As this moment, there is only the " GPIO entity"  that has has private
> functions, and does not require it.
> 
> So I can simply remove the flag and add it later (if needed).

I hope we won't need more hacks of this kind ;-)

(It's a pretty clever hack I have to say)

> > > +             ret = ctrl->entity->get_info ?
> > > +                     ctrl->entity->get_info(ctrl->entity, ctrl->info.selector, data) :
> > > +                     -EINVAL;
> > > +     else
> > > +             ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> > > +                                  info->selector, data, 1);
> > >       if (!ret)
> > >               info->flags |= (data[0] & UVC_CONTROL_CAP_GET ?
> > >                               UVC_CTRL_FLAG_GET_CUR : 0)
> > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > index a3dfacf069c4..08922d889bb6 100644
> > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -353,6 +353,9 @@ struct uvc_entity {
> > >       u8 bNrInPins;
> > >       u8 *baSourceID;
> > >
> > > +     int (*get_info)(struct uvc_entity *entity, u8 cs, u8 *caps);
> > > +     int (*get_cur)(struct uvc_entity *entity, u8 cs, void *data, u16 size);
> >
> > Looks like the second function should be part of patch 2/6 instead. I
> > would however squash 1/6 and 2/6.
> >
> > > +
> > >       unsigned int ncontrols;
> > >       struct uvc_control *controls;
> > >  };
> > > diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
> > > index f80f05b3c423..69b636290c31 100644
> > > --- a/include/uapi/linux/uvcvideo.h
> > > +++ b/include/uapi/linux/uvcvideo.h
> > > @@ -30,6 +30,8 @@
> > >  #define UVC_CTRL_FLAG_AUTO_UPDATE    (1 << 7)
> > >  /* Control supports asynchronous reporting */
> > >  #define UVC_CTRL_FLAG_ASYNCHRONOUS   (1 << 8)
> > > +/* Entity queries */
> > > +#define UVC_CTRL_FLAG_ENTITY_GET_INFO        (1 << 9)
> > >
> > >  #define UVC_CTRL_FLAG_GET_RANGE \
> > >       (UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_GET_MIN | \

-- 
Regards,

Laurent Pinchart

[PATCH 2/6] media: uvcvideo: Add UVC_CTRL_FLAG_ENTITY_GET_CUR

[Ricardo Ribalda]

This flag allows controls to get their current value from an entity
defined function instead of via a query to the USB device.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 17 +++++++++++++----
 include/uapi/linux/uvcvideo.h    |  1 +
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 7acdc055613b..0a8835742d49 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1001,10 +1001,19 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain,
 		return -EACCES;
 
 	if (!ctrl->loaded) {
-		ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, ctrl->entity->id,
-				chain->dev->intfnum, ctrl->info.selector,
-				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
-				ctrl->info.size);
+		if (ctrl->info.flags & UVC_CTRL_FLAG_ENTITY_GET_CUR) {
+			if (!ctrl->entity->get_cur)
+				return -EINVAL;
+			ret = ctrl->entity->get_cur(ctrl->entity,
+					ctrl->info.selector,
+					uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
+					ctrl->info.size);
+		} else {
+			ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, ctrl->entity->id,
+					     chain->dev->intfnum, ctrl->info.selector,
+					     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
+					     ctrl->info.size);
+		}
 		if (ret < 0)
 			return ret;
 
diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
index 69b636290c31..cb91797d2a09 100644
--- a/include/uapi/linux/uvcvideo.h
+++ b/include/uapi/linux/uvcvideo.h
@@ -32,6 +32,7 @@
 #define UVC_CTRL_FLAG_ASYNCHRONOUS	(1 << 8)
 /* Entity queries */
 #define UVC_CTRL_FLAG_ENTITY_GET_INFO	(1 << 9)
+#define UVC_CTRL_FLAG_ENTITY_GET_CUR	(1 << 10)
 
 #define UVC_CTRL_FLAG_GET_RANGE \
 	(UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_GET_MIN | \
-- 
2.29.0.rc1.297.gfa9743e501-goog

Re: [PATCH 2/6] media: uvcvideo: Add UVC_CTRL_FLAG_ENTITY_GET_CUR

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Thu, Oct 22, 2020 at 03:37:49PM +0200, Ricardo Ribalda wrote:
> This flag allows controls to get their current value from an entity
> defined function instead of via a query to the USB device.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 17 +++++++++++++----
>  include/uapi/linux/uvcvideo.h    |  1 +
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 7acdc055613b..0a8835742d49 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1001,10 +1001,19 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain,
>  		return -EACCES;
>  
>  	if (!ctrl->loaded) {
> -		ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, ctrl->entity->id,
> -				chain->dev->intfnum, ctrl->info.selector,
> -				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> -				ctrl->info.size);
> +		if (ctrl->info.flags & UVC_CTRL_FLAG_ENTITY_GET_CUR) {

Same question as for 2/6, do we need this flag ?

> +			if (!ctrl->entity->get_cur)
> +				return -EINVAL;
> +			ret = ctrl->entity->get_cur(ctrl->entity,
> +					ctrl->info.selector,
> +					uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> +					ctrl->info.size);
> +		} else {
> +			ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, ctrl->entity->id,
> +					     chain->dev->intfnum, ctrl->info.selector,
> +					     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> +					     ctrl->info.size);
> +		}
>  		if (ret < 0)
>  			return ret;
>  
> diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
> index 69b636290c31..cb91797d2a09 100644
> --- a/include/uapi/linux/uvcvideo.h
> +++ b/include/uapi/linux/uvcvideo.h
> @@ -32,6 +32,7 @@
>  #define UVC_CTRL_FLAG_ASYNCHRONOUS	(1 << 8)
>  /* Entity queries */
>  #define UVC_CTRL_FLAG_ENTITY_GET_INFO	(1 << 9)
> +#define UVC_CTRL_FLAG_ENTITY_GET_CUR	(1 << 10)
>  
>  #define UVC_CTRL_FLAG_GET_RANGE \
>  	(UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_GET_MIN | \

-- 
Regards,

Laurent Pinchart

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

[Ricardo Ribalda]

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 <ribalda@chromium.org>
---
 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

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

[Laurent Pinchart]

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 <ribalda@chromium.org>
> ---
>  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

[PATCH 4/6] media: uvcvideo: Add Privacy control based on EXT_GPIO

[Ricardo Ribalda]

Add a new control and mapping for Privacy controls connected to
UVC_GUID_EXT_GPIO_CONTROLLERs.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 20 ++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h |  2 +-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 913739915863..786498e66646 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -347,6 +347,16 @@ static const struct uvc_control_info uvc_ctrls[] = {
 				| UVC_CTRL_FLAG_RESTORE
 				| UVC_CTRL_FLAG_AUTO_UPDATE,
 	},
+	{
+		.entity		= UVC_GUID_EXT_GPIO_CONTROLLER,
+		.selector	= UVC_CT_PRIVACY_CONTROL,
+		.index		= 0,
+		.size		= 1,
+		.flags		= UVC_CTRL_FLAG_GET_CUR
+				| UVC_CTRL_FLAG_AUTO_UPDATE
+				| UVC_CTRL_FLAG_ENTITY_GET_INFO
+				| UVC_CTRL_FLAG_ENTITY_GET_CUR,
+	},
 };
 
 static const struct uvc_menu_info power_line_frequency_controls[] = {
@@ -735,6 +745,16 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 		.v4l2_type	= V4L2_CTRL_TYPE_BOOLEAN,
 		.data_type	= UVC_CTRL_DATA_TYPE_BOOLEAN,
 	},
+	{
+		.id		= V4L2_CID_PRIVACY,
+		.name		= "Privacy",
+		.entity		= UVC_GUID_EXT_GPIO_CONTROLLER,
+		.selector	= UVC_CT_PRIVACY_CONTROL,
+		.size		= 1,
+		.offset		= 0,
+		.v4l2_type	= V4L2_CTRL_TYPE_BOOLEAN,
+		.data_type	= UVC_CTRL_DATA_TYPE_BOOLEAN,
+	},
 };
 
 /* ------------------------------------------------------------------------
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 8e5a9fc35820..a493bc383d3e 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -58,7 +58,7 @@
 	 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}
+	 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

Re: [PATCH 4/6] media: uvcvideo: Add Privacy control based on EXT_GPIO

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Thu, Oct 22, 2020 at 03:37:51PM +0200, Ricardo Ribalda wrote:
> Add a new control and mapping for Privacy controls connected to
> UVC_GUID_EXT_GPIO_CONTROLLERs.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 20 ++++++++++++++++++++
>  drivers/media/usb/uvc/uvcvideo.h |  2 +-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 913739915863..786498e66646 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -347,6 +347,16 @@ static const struct uvc_control_info uvc_ctrls[] = {
>  				| UVC_CTRL_FLAG_RESTORE
>  				| UVC_CTRL_FLAG_AUTO_UPDATE,
>  	},
> +	{
> +		.entity		= UVC_GUID_EXT_GPIO_CONTROLLER,
> +		.selector	= UVC_CT_PRIVACY_CONTROL,
> +		.index		= 0,
> +		.size		= 1,
> +		.flags		= UVC_CTRL_FLAG_GET_CUR
> +				| UVC_CTRL_FLAG_AUTO_UPDATE
> +				| UVC_CTRL_FLAG_ENTITY_GET_INFO
> +				| UVC_CTRL_FLAG_ENTITY_GET_CUR,
> +	},
>  };
>  
>  static const struct uvc_menu_info power_line_frequency_controls[] = {
> @@ -735,6 +745,16 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  		.v4l2_type	= V4L2_CTRL_TYPE_BOOLEAN,
>  		.data_type	= UVC_CTRL_DATA_TYPE_BOOLEAN,
>  	},
> +	{
> +		.id		= V4L2_CID_PRIVACY,
> +		.name		= "Privacy",
> +		.entity		= UVC_GUID_EXT_GPIO_CONTROLLER,
> +		.selector	= UVC_CT_PRIVACY_CONTROL,
> +		.size		= 1,
> +		.offset		= 0,
> +		.v4l2_type	= V4L2_CTRL_TYPE_BOOLEAN,
> +		.data_type	= UVC_CTRL_DATA_TYPE_BOOLEAN,
> +	},
>  };
>  
>  /* ------------------------------------------------------------------------
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 8e5a9fc35820..a493bc383d3e 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -58,7 +58,7 @@
>  	 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}
> +	 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x01}

This belongs to the previous patch.

>  
>  #define UVC_GUID_FORMAT_MJPEG \
>  	{ 'M',  'J',  'P',  'G', 0x00, 0x00, 0x10, 0x00, \

-- 
Regards,

Laurent Pinchart

[PATCH 5/6] media: uvcvideo: Implement UVC_GPIO_UNIT

[Ricardo Ribalda]

Some devices can implement a physical switch to disable the input of the
camera on demand. Think of it like an elegant privacy sticker.

The system can read the status of the privacy switch via a GPIO.

It is important to know the status of the switch, e.g. to notify the
user when the camera will produce black frames and a videochat
application is used.

Since the uvc device is not aware of this pin (and it should't), we need
to implement a virtual entity that can interact with such pin.

The location of the GPIO is specified via acpi or DT. on the usb device Eg:

    Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
    {
        GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
            "\\_SB.PCI0.GPIO", 0x00, ResourceConsumer, ,
            )
            {   // Pin list
                0x0064
            }
    })
    Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
    {
        ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
        Package (0x01)
        {
            Package (0x02)
            {
                "privacy-gpio",
                Package (0x04)
                {
                    \_SB.PCI0.XHCI.RHUB.HS07,
                    Zero,
                    Zero,
                    One
                }
            }
        }
    })

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c   |  3 ++
 drivers/media/usb/uvc/uvc_driver.c | 72 ++++++++++++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  8 ++++
 3 files changed, 83 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 786498e66646..3a49a1326a90 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -2332,6 +2332,9 @@ 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_GPIO_UNIT) {
+			bmControls = entity->gpio.bmControls;
+			bControlSize = entity->gpio.bControlSize;
 		}
 
 		/* Remove bogus/blacklisted controls */
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index ddb9eaa11be7..180e503e900f 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/atomic.h>
+#include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
@@ -1440,6 +1441,58 @@ static int uvc_parse_control(struct uvc_device *dev)
 	return 0;
 }
 
+static int uvc_gpio_get_cur(struct uvc_entity *entity, u8 cs, void *data, u16 size)
+{
+	if ((cs != UVC_CT_PRIVACY_CONTROL) || (size < 1))
+		return -EINVAL;
+
+	*(uint8_t *)data = gpiod_get_value(entity->gpio.gpio_privacy);
+	return 0;
+}
+
+static int uvc_gpio_get_info(struct uvc_entity *entity, u8 cs, u8 *caps)
+{
+
+	if (cs != UVC_CT_PRIVACY_CONTROL)
+		return -EINVAL;
+
+	*caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
+	return 0;
+}
+
+static int uvc_parse_gpio(struct uvc_device *dev)
+{
+	struct uvc_entity *unit;
+	struct gpio_desc *gpio_privacy;
+	int irq;
+	int ret;
+
+	gpio_privacy = devm_gpiod_get_optional(&dev->udev->dev, "privacy", GPIOD_IN);
+
+	if (IS_ERR(gpio_privacy))
+		return PTR_ERR(gpio_privacy);
+
+	if (!gpio_privacy)
+		return 0;
+
+	unit = uvc_alloc_entity(UVC_GPIO_UNIT, 0xff, 1, 2);
+	if (!unit)
+		return -ENOMEM;
+
+	unit->gpio.gpio_privacy = gpio_privacy;
+	unit->gpio.bControlSize = 1;
+	unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
+	unit->gpio.bmControls[0] = 1;
+	unit->get_cur = uvc_gpio_get_cur;
+	unit->get_info = uvc_gpio_get_info;
+
+	sprintf(unit->name, "GPIO Unit");
+
+	list_add_tail(&unit->list, &dev->entities);
+
+	return 0;
+}
+
 /* ------------------------------------------------------------------------
  * UVC device scan
  */
@@ -1532,6 +1585,12 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
 
 		break;
 
+	case UVC_GPIO_UNIT:
+		if (uvc_trace_param & UVC_TRACE_PROBE)
+			printk(KERN_CONT " GPIO %d", entity->id);
+
+		break;
+
 	case UVC_TT_STREAMING:
 		if (UVC_ENTITY_IS_ITERM(entity)) {
 			if (uvc_trace_param & UVC_TRACE_PROBE)
@@ -1929,6 +1988,13 @@ static int uvc_scan_device(struct uvc_device *dev)
 		return -1;
 	}
 
+	/* Add GPIO entities to the first_chain */
+	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
+	list_for_each_entry(term, &dev->entities, list) {
+		if (UVC_ENTITY_TYPE(term) == UVC_GPIO_UNIT)
+			list_add_tail(&term->chain, &chain->entities);
+	}
+
 	return 0;
 }
 
@@ -2261,6 +2327,12 @@ static int uvc_probe(struct usb_interface *intf,
 		goto error;
 	}
 
+	/* Parse the associated GPIOs */
+	if (uvc_parse_gpio(dev) < 0) {
+		uvc_trace(UVC_TRACE_PROBE, "Unable to parse UVC GPIOs\n");
+		goto error;
+	}
+
 	uvc_printk(KERN_INFO, "Found UVC %u.%02x device %s (%04x:%04x)\n",
 		dev->uvc_version >> 8, dev->uvc_version & 0xff,
 		udev->product ? udev->product : "<unnamed>",
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index a493bc383d3e..7ca78005b6a9 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -6,6 +6,7 @@
 #error "The uvcvideo.h header is deprecated, use linux/uvcvideo.h instead."
 #endif /* __KERNEL__ */
 
+#include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/poll.h>
 #include <linux/usb.h>
@@ -37,6 +38,7 @@
 	(UVC_ENTITY_IS_TERM(entity) && \
 	((entity)->type & 0x8000) == UVC_TERM_OUTPUT)
 
+#define UVC_GPIO_UNIT 0x7ffe
 
 /* ------------------------------------------------------------------------
  * GUIDs
@@ -351,6 +353,12 @@ struct uvc_entity {
 			u8  *bmControls;
 			u8  *bmControlsType;
 		} extension;
+
+		struct {
+			u8  bControlSize;
+			u8  *bmControls;
+			struct gpio_desc *gpio_privacy;
+		} gpio;
 	};
 
 	u8 bNrInPins;
-- 
2.29.0.rc1.297.gfa9743e501-goog

Re: [PATCH 5/6] media: uvcvideo: Implement UVC_GPIO_UNIT

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Thu, Oct 22, 2020 at 03:37:52PM +0200, Ricardo Ribalda wrote:
> Some devices can implement a physical switch to disable the input of the
> camera on demand. Think of it like an elegant privacy sticker.
> 
> The system can read the status of the privacy switch via a GPIO.
> 
> It is important to know the status of the switch, e.g. to notify the
> user when the camera will produce black frames and a videochat
> application is used.
> 
> Since the uvc device is not aware of this pin (and it should't), we need

I'd argue that it should, we wouldn't have to deal with all this if the
switch was connected to the UVC device. This series is a hack to
workaround a bad hardware design :-)

> to implement a virtual entity that can interact with such pin.
> 
> The location of the GPIO is specified via acpi or DT. on the usb device Eg:

How does it look like for DT-based systems ? Do we need to add DT
bindings ?

> 
>     Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
>     {
>         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
>             "\\_SB.PCI0.GPIO", 0x00, ResourceConsumer, ,
>             )
>             {   // Pin list
>                 0x0064
>             }
>     })
>     Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
>     {
>         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
>         Package (0x01)
>         {
>             Package (0x02)
>             {
>                 "privacy-gpio",
>                 Package (0x04)
>                 {
>                     \_SB.PCI0.XHCI.RHUB.HS07,
>                     Zero,
>                     Zero,
>                     One

What do the last three values represent ?

>                 }
>             }
>         }
>     })

Can you add a bit of context to show in which ACPI device object this is
located (I assume \_SB.PCI0.XHCI.RHUB.HS07) ?

> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c   |  3 ++
>  drivers/media/usb/uvc/uvc_driver.c | 72 ++++++++++++++++++++++++++++++
>  drivers/media/usb/uvc/uvcvideo.h   |  8 ++++
>  3 files changed, 83 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 786498e66646..3a49a1326a90 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2332,6 +2332,9 @@ 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_GPIO_UNIT) {
> +			bmControls = entity->gpio.bmControls;
> +			bControlSize = entity->gpio.bControlSize;
>  		}
>  
>  		/* Remove bogus/blacklisted controls */
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index ddb9eaa11be7..180e503e900f 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/atomic.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
> @@ -1440,6 +1441,58 @@ static int uvc_parse_control(struct uvc_device *dev)
>  	return 0;
>  }
>  
> +static int uvc_gpio_get_cur(struct uvc_entity *entity, u8 cs, void *data, u16 size)

Line break at 80 columns please.

> +{
> +	if ((cs != UVC_CT_PRIVACY_CONTROL) || (size < 1))

No need for the inner parentheses.

> +		return -EINVAL;

Should we mimick the error value returned when querying the device with
an invalid control selector ? Same below.

> +
> +	*(uint8_t *)data = gpiod_get_value(entity->gpio.gpio_privacy);
> +	return 0;
> +}
> +
> +static int uvc_gpio_get_info(struct uvc_entity *entity, u8 cs, u8 *caps)
> +{
> +

Extra blank line.

> +	if (cs != UVC_CT_PRIVACY_CONTROL)
> +		return -EINVAL;
> +
> +	*caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> +	return 0;
> +}
> +
> +static int uvc_parse_gpio(struct uvc_device *dev)
> +{
> +	struct uvc_entity *unit;
> +	struct gpio_desc *gpio_privacy;
> +	int irq;
> +	int ret;
> +
> +	gpio_privacy = devm_gpiod_get_optional(&dev->udev->dev, "privacy", GPIOD_IN);

Line break at 80 columns please.

> +
> +	if (IS_ERR(gpio_privacy))
> +		return PTR_ERR(gpio_privacy);
> +
> +	if (!gpio_privacy)
> +		return 0;
> +
> +	unit = uvc_alloc_entity(UVC_GPIO_UNIT, 0xff, 1, 2);

Isn't there a risk, at least in theory, that entity ID 255 would be used
by a real UVC entity ? What are the implication of entity ID conflicts ?

It doesn't seem like the entity will be linked, does it need any pad ?

Why do you need two bytes of extra size, don't you use one only ?

> +	if (!unit)
> +		return -ENOMEM;
> +
> +	unit->gpio.gpio_privacy = gpio_privacy;
> +	unit->gpio.bControlSize = 1;
> +	unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> +	unit->gpio.bmControls[0] = 1;
> +	unit->get_cur = uvc_gpio_get_cur;
> +	unit->get_info = uvc_gpio_get_info;
> +
> +	sprintf(unit->name, "GPIO Unit");
> +
> +	list_add_tail(&unit->list, &dev->entities);
> +
> +	return 0;
> +}
> +
>  /* ------------------------------------------------------------------------
>   * UVC device scan
>   */
> @@ -1532,6 +1585,12 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
>  
>  		break;
>  
> +	case UVC_GPIO_UNIT:
> +		if (uvc_trace_param & UVC_TRACE_PROBE)
> +			printk(KERN_CONT " GPIO %d", entity->id);
> +
> +		break;
> +

Let's move this after UVC_TT_STREAMING.

>  	case UVC_TT_STREAMING:
>  		if (UVC_ENTITY_IS_ITERM(entity)) {
>  			if (uvc_trace_param & UVC_TRACE_PROBE)
> @@ -1929,6 +1988,13 @@ static int uvc_scan_device(struct uvc_device *dev)
>  		return -1;
>  	}
>  
> +	/* Add GPIO entities to the first_chain */

s/first_chain/first chain./

This leads to an interesting question. What if we have a UVC device with
two sensors ? There could be a different privacy GPIO for each of them
in theory. This would need to be reflected in the ACPI and DT bindings,
we would need to specify a GPIO per input terminal.

> +	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> +	list_for_each_entry(term, &dev->entities, list) {
> +		if (UVC_ENTITY_TYPE(term) == UVC_GPIO_UNIT)
> +			list_add_tail(&term->chain, &chain->entities);
> +	}

As this is done after calling uvc_scan_chain(), and thus after
uvc_scan_chain_entity(), do we need the previous hunk ?

Alternatively, we could hook up with the existing chain scanning
mechanism if the GPIO entity was linked to another entity. This may
however be difficult to implement.

> +
>  	return 0;
>  }
>  
> @@ -2261,6 +2327,12 @@ static int uvc_probe(struct usb_interface *intf,
>  		goto error;
>  	}
>  
> +	/* Parse the associated GPIOs */
> +	if (uvc_parse_gpio(dev) < 0) {
> +		uvc_trace(UVC_TRACE_PROBE, "Unable to parse UVC GPIOs\n");
> +		goto error;
> +	}
> +
>  	uvc_printk(KERN_INFO, "Found UVC %u.%02x device %s (%04x:%04x)\n",
>  		dev->uvc_version >> 8, dev->uvc_version & 0xff,
>  		udev->product ? udev->product : "<unnamed>",
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index a493bc383d3e..7ca78005b6a9 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -6,6 +6,7 @@
>  #error "The uvcvideo.h header is deprecated, use linux/uvcvideo.h instead."
>  #endif /* __KERNEL__ */
>  
> +#include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
>  #include <linux/poll.h>
>  #include <linux/usb.h>
> @@ -37,6 +38,7 @@
>  	(UVC_ENTITY_IS_TERM(entity) && \
>  	((entity)->type & 0x8000) == UVC_TERM_OUTPUT)
>  
> +#define UVC_GPIO_UNIT 0x7ffe

I'd name this UVC_EXT_GPIO_UNIT.

>  
>  /* ------------------------------------------------------------------------
>   * GUIDs
> @@ -351,6 +353,12 @@ struct uvc_entity {
>  			u8  *bmControls;
>  			u8  *bmControlsType;
>  		} extension;
> +
> +		struct {
> +			u8  bControlSize;
> +			u8  *bmControls;
> +			struct gpio_desc *gpio_privacy;
> +		} gpio;
>  	};
>  
>  	u8 bNrInPins;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/6] media: uvcvideo: Implement UVC_GPIO_UNIT

[Ricardo Ribalda]

HI Laurent

On Wed, Nov 4, 2020 at 12:59 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Thu, Oct 22, 2020 at 03:37:52PM +0200, Ricardo Ribalda wrote:
> > Some devices can implement a physical switch to disable the input of the
> > camera on demand. Think of it like an elegant privacy sticker.
> >
> > The system can read the status of the privacy switch via a GPIO.
> >
> > It is important to know the status of the switch, e.g. to notify the
> > user when the camera will produce black frames and a videochat
> > application is used.
> >
> > Since the uvc device is not aware of this pin (and it should't), we need
>
> I'd argue that it should, we wouldn't have to deal with all this if the
> switch was connected to the UVC device. This series is a hack to
> workaround a bad hardware design :-)

I have mixed feelings about this.

I like a design where the camera is independent from the uvc.
Otherwise a malicious camera could try to bypass the privacy pin (eg,
set the gpio as output and put a low value)

But you are right, this is just a hack.

>
> > to implement a virtual entity that can interact with such pin.
> >
> > The location of the GPIO is specified via acpi or DT. on the usb device Eg:
>
> How does it look like for DT-based systems ? Do we need to add DT
> bindings ?

I guess it is for us to define. I can prepare a patch with a DT
binding, although I do not have
a hardware with DT and this feature.

>
> >
> >     Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
> >     {
> >         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
> >             "\\_SB.PCI0.GPIO", 0x00, ResourceConsumer, ,
> >             )
> >             {   // Pin list
> >                 0x0064
> >             }
> >     })
> >     Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
> >     {
> >         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
> >         Package (0x01)
> >         {
> >             Package (0x02)
> >             {
> >                 "privacy-gpio",
> >                 Package (0x04)
> >                 {
> >                     \_SB.PCI0.XHCI.RHUB.HS07,
> >                     Zero,
> >                     Zero,
> >                     One
>
> What do the last three values represent ?
Package () { "name", Package () { ref, index, pin, active_low }}
Documentation/firmware-guide/acpi/gpio-properties.rst

>
> >                 }
> >             }
> >         }
> >     })
>
> Can you add a bit of context to show in which ACPI device object this is
> located (I assume \_SB.PCI0.XHCI.RHUB.HS07) ?
>
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c   |  3 ++
> >  drivers/media/usb/uvc/uvc_driver.c | 72 ++++++++++++++++++++++++++++++
> >  drivers/media/usb/uvc/uvcvideo.h   |  8 ++++
> >  3 files changed, 83 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 786498e66646..3a49a1326a90 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -2332,6 +2332,9 @@ 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_GPIO_UNIT) {
> > +                     bmControls = entity->gpio.bmControls;
> > +                     bControlSize = entity->gpio.bControlSize;
> >               }
> >
> >               /* Remove bogus/blacklisted controls */
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index ddb9eaa11be7..180e503e900f 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -7,6 +7,7 @@
> >   */
> >
> >  #include <linux/atomic.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/kernel.h>
> >  #include <linux/list.h>
> >  #include <linux/module.h>
> > @@ -1440,6 +1441,58 @@ static int uvc_parse_control(struct uvc_device *dev)
> >       return 0;
> >  }
> >
> > +static int uvc_gpio_get_cur(struct uvc_entity *entity, u8 cs, void *data, u16 size)
>
> Line break at 80 columns please.
>
> > +{
> > +     if ((cs != UVC_CT_PRIVACY_CONTROL) || (size < 1))
>
> No need for the inner parentheses.
>
> > +             return -EINVAL;
>
> Should we mimick the error value returned when querying the device with
> an invalid control selector ? Same below.

Not sure what you mean here. Don't we return  -EINVAL in that case ?

from uvc_query_ctrl()

switch (error) {
case 0:
/* Cannot happen - we received a STALL */
return -EPIPE;
case 1: /* Not ready */
return -EBUSY;
case 2: /* Wrong state */
return -EILSEQ;
case 3: /* Power */
return -EREMOTE;
case 4: /* Out of range */
return -ERANGE;
case 5: /* Invalid unit */
case 6: /* Invalid control
case 7: /* Invalid Request */
case 8: /* Invalid value within range */
return -EINVAL;
default: /* reserved or unknown */
break;

}

>
> > +
> > +     *(uint8_t *)data = gpiod_get_value(entity->gpio.gpio_privacy);
> > +     return 0;
> > +}
> > +
> > +static int uvc_gpio_get_info(struct uvc_entity *entity, u8 cs, u8 *caps)
> > +{
> > +
>
> Extra blank line.
>
> > +     if (cs != UVC_CT_PRIVACY_CONTROL)
> > +             return -EINVAL;
> > +
> > +     *caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> > +     return 0;
> > +}
> > +
> > +static int uvc_parse_gpio(struct uvc_device *dev)
> > +{
> > +     struct uvc_entity *unit;
> > +     struct gpio_desc *gpio_privacy;
> > +     int irq;
> > +     int ret;
> > +
> > +     gpio_privacy = devm_gpiod_get_optional(&dev->udev->dev, "privacy", GPIOD_IN);
>
> Line break at 80 columns please.
>
> > +
> > +     if (IS_ERR(gpio_privacy))
> > +             return PTR_ERR(gpio_privacy);
> > +
> > +     if (!gpio_privacy)
> > +             return 0;
> > +
> > +     unit = uvc_alloc_entity(UVC_GPIO_UNIT, 0xff, 1, 2);
>
> Isn't there a risk, at least in theory, that entity ID 255 would be used
> by a real UVC entity ? What are the implication of entity ID conflicts ?
>
> It doesn't seem like the entity will be linked, does it need any pad ?
>
> Why do you need two bytes of extra size, don't you use one only ?
>
> > +     if (!unit)
> > +             return -ENOMEM;
> > +
> > +     unit->gpio.gpio_privacy = gpio_privacy;
> > +     unit->gpio.bControlSize = 1;
> > +     unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> > +     unit->gpio.bmControls[0] = 1;
> > +     unit->get_cur = uvc_gpio_get_cur;
> > +     unit->get_info = uvc_gpio_get_info;
> > +
> > +     sprintf(unit->name, "GPIO Unit");
> > +
> > +     list_add_tail(&unit->list, &dev->entities);
> > +
> > +     return 0;
> > +}
> > +
> >  /* ------------------------------------------------------------------------
> >   * UVC device scan
> >   */
> > @@ -1532,6 +1585,12 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
> >
> >               break;
> >
> > +     case UVC_GPIO_UNIT:
> > +             if (uvc_trace_param & UVC_TRACE_PROBE)
> > +                     printk(KERN_CONT " GPIO %d", entity->id);
> > +
> > +             break;
> > +
>
> Let's move this after UVC_TT_STREAMING.
>
> >       case UVC_TT_STREAMING:
> >               if (UVC_ENTITY_IS_ITERM(entity)) {
> >                       if (uvc_trace_param & UVC_TRACE_PROBE)
> > @@ -1929,6 +1988,13 @@ static int uvc_scan_device(struct uvc_device *dev)
> >               return -1;
> >       }
> >
> > +     /* Add GPIO entities to the first_chain */
>
> s/first_chain/first chain./
>
> This leads to an interesting question. What if we have a UVC device with
> two sensors ? There could be a different privacy GPIO for each of them
> in theory. This would need to be reflected in the ACPI and DT bindings,
> we would need to specify a GPIO per input terminal.

My approach was to "contaminate" as little as possible the uvc driver
with a hack.

As of now I do not have any device with two sensors and an external
gpio. If we ever
go there we can decide what to do.

>
> > +     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > +     list_for_each_entry(term, &dev->entities, list) {
> > +             if (UVC_ENTITY_TYPE(term) == UVC_GPIO_UNIT)
> > +                     list_add_tail(&term->chain, &chain->entities);
> > +     }
>
> As this is done after calling uvc_scan_chain(), and thus after
> uvc_scan_chain_entity(), do we need the previous hunk ?
>
> Alternatively, we could hook up with the existing chain scanning
> mechanism if the GPIO entity was linked to another entity. This may
> however be difficult to implement.
>
> > +
> >       return 0;
> >  }
> >
> > @@ -2261,6 +2327,12 @@ static int uvc_probe(struct usb_interface *intf,
> >               goto error;
> >       }
> >
> > +     /* Parse the associated GPIOs */
> > +     if (uvc_parse_gpio(dev) < 0) {
> > +             uvc_trace(UVC_TRACE_PROBE, "Unable to parse UVC GPIOs\n");
> > +             goto error;
> > +     }
> > +
> >       uvc_printk(KERN_INFO, "Found UVC %u.%02x device %s (%04x:%04x)\n",
> >               dev->uvc_version >> 8, dev->uvc_version & 0xff,
> >               udev->product ? udev->product : "<unnamed>",
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index a493bc383d3e..7ca78005b6a9 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -6,6 +6,7 @@
> >  #error "The uvcvideo.h header is deprecated, use linux/uvcvideo.h instead."
> >  #endif /* __KERNEL__ */
> >
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/kernel.h>
> >  #include <linux/poll.h>
> >  #include <linux/usb.h>
> > @@ -37,6 +38,7 @@
> >       (UVC_ENTITY_IS_TERM(entity) && \
> >       ((entity)->type & 0x8000) == UVC_TERM_OUTPUT)
> >
> > +#define UVC_GPIO_UNIT 0x7ffe
>
> I'd name this UVC_EXT_GPIO_UNIT.
>
> >
> >  /* ------------------------------------------------------------------------
> >   * GUIDs
> > @@ -351,6 +353,12 @@ struct uvc_entity {
> >                       u8  *bmControls;
> >                       u8  *bmControlsType;
> >               } extension;
> > +
> > +             struct {
> > +                     u8  bControlSize;
> > +                     u8  *bmControls;
> > +                     struct gpio_desc *gpio_privacy;
> > +             } gpio;
> >       };
> >
> >       u8 bNrInPins;
>
> --
> Regards,
>
> Laurent Pinchart



--
Ricardo Ribalda

[PATCH 6/6] media: uvcvideo: Handle IRQs from the privacy_pin

[Ricardo Ribalda]

If the privacy pin produces an IRQ, read the gpio and notify userspace
via an event.

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

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 3a49a1326a90..00c41cba0f68 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1346,6 +1346,9 @@ static void uvc_ctrl_status_event_work(struct work_struct *work)
 
 	mutex_unlock(&chain->ctrl_mutex);
 
+	if (!w->urb)
+		return;
+
 	/* Resubmit the URB. */
 	w->urb->interval = dev->int_ep->desc.bInterval;
 	ret = usb_submit_urb(w->urb, GFP_KERNEL);
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 180e503e900f..d1260d131bd8 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1460,6 +1460,25 @@ static int uvc_gpio_get_info(struct uvc_entity *entity, u8 cs, u8 *caps)
 	return 0;
 }
 
+static irqreturn_t uvc_privacy_gpio_irq(int irq, void *data)
+{
+	struct uvc_device *dev = data;
+	struct uvc_video_chain *chain;
+	struct uvc_entity *term;
+	u8 value;
+
+	list_for_each_entry(chain, &dev->chains, list) {
+		list_for_each_entry(term, &dev->entities, list) {
+			if (UVC_ENTITY_TYPE(term) == UVC_GPIO_UNIT) {
+				value = gpiod_get_value(term->gpio.gpio_privacy);
+				uvc_ctrl_status_event(NULL, chain, term->controls, &value);
+			}
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
 static int uvc_parse_gpio(struct uvc_device *dev)
 {
 	struct uvc_entity *unit;
@@ -1490,6 +1509,17 @@ static int uvc_parse_gpio(struct uvc_device *dev)
 
 	list_add_tail(&unit->list, &dev->entities);
 
+	irq = gpiod_to_irq(gpio_privacy);
+
+	if (irq == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	if (irq < 0)
+		return 0;
+
+	ret = devm_request_irq(&dev->udev->dev, irq, uvc_privacy_gpio_irq, IRQF_SHARED,
+			       "uvc_privacy_gpio", dev);
+
 	return 0;
 }
 
-- 
2.29.0.rc1.297.gfa9743e501-goog

Re: [PATCH 6/6] media: uvcvideo: Handle IRQs from the privacy_pin

[Ricardo Ribalda]

Hi


On Thu, Oct 22, 2020 at 3:38 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> If the privacy pin produces an IRQ, read the gpio and notify userspace
> via an event.
>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c   |  3 +++
>  drivers/media/usb/uvc/uvc_driver.c | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 3a49a1326a90..00c41cba0f68 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1346,6 +1346,9 @@ static void uvc_ctrl_status_event_work(struct work_struct *work)
>
>         mutex_unlock(&chain->ctrl_mutex);
>
> +       if (!w->urb)
> +               return;
> +
>         /* Resubmit the URB. */
>         w->urb->interval = dev->int_ep->desc.bInterval;
>         ret = usb_submit_urb(w->urb, GFP_KERNEL);
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 180e503e900f..d1260d131bd8 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1460,6 +1460,25 @@ static int uvc_gpio_get_info(struct uvc_entity *entity, u8 cs, u8 *caps)
>         return 0;
>  }
>
> +static irqreturn_t uvc_privacy_gpio_irq(int irq, void *data)
> +{
> +       struct uvc_device *dev = data;
> +       struct uvc_video_chain *chain;
> +       struct uvc_entity *term;
> +       u8 value;
> +
> +       list_for_each_entry(chain, &dev->chains, list) {
> +               list_for_each_entry(term, &dev->entities, list) {
> +                       if (UVC_ENTITY_TYPE(term) == UVC_GPIO_UNIT) {
> +                               value = gpiod_get_value(term->gpio.gpio_privacy);
> +                               uvc_ctrl_status_event(NULL, chain, term->controls, &value);
> +                       }
> +               }
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
>  static int uvc_parse_gpio(struct uvc_device *dev)
>  {
>         struct uvc_entity *unit;
> @@ -1490,6 +1509,17 @@ static int uvc_parse_gpio(struct uvc_device *dev)
>
>         list_add_tail(&unit->list, &dev->entities);
>
> +       irq = gpiod_to_irq(gpio_privacy);
> +
> +       if (irq == -EPROBE_DEFER)
> +               return -EPROBE_DEFER;
> +
> +       if (irq < 0)
> +               return 0;
> +
> +       ret = devm_request_irq(&dev->udev->dev, irq, uvc_privacy_gpio_irq, IRQF_SHARED,
> +                              "uvc_privacy_gpio", dev);

Here it should say IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING instead
of IRQF_SHARED.

I will fix that on v2,

> +
>         return 0;
>  }
>
> --
> 2.29.0.rc1.297.gfa9743e501-goog
>

Re: [PATCH 6/6] media: uvcvideo: Handle IRQs from the privacy_pin

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Thu, Oct 22, 2020 at 03:37:53PM +0200, Ricardo Ribalda wrote:
> If the privacy pin produces an IRQ, read the gpio and notify userspace
> via an event.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c   |  3 +++
>  drivers/media/usb/uvc/uvc_driver.c | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 3a49a1326a90..00c41cba0f68 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1346,6 +1346,9 @@ static void uvc_ctrl_status_event_work(struct work_struct *work)
>  
>  	mutex_unlock(&chain->ctrl_mutex);
>  
> +	if (!w->urb)
> +		return;
> +
>  	/* Resubmit the URB. */
>  	w->urb->interval = dev->int_ep->desc.bInterval;
>  	ret = usb_submit_urb(w->urb, GFP_KERNEL);
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 180e503e900f..d1260d131bd8 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1460,6 +1460,25 @@ static int uvc_gpio_get_info(struct uvc_entity *entity, u8 cs, u8 *caps)
>  	return 0;
>  }
>  
> +static irqreturn_t uvc_privacy_gpio_irq(int irq, void *data)
> +{
> +	struct uvc_device *dev = data;
> +	struct uvc_video_chain *chain;
> +	struct uvc_entity *term;

The entity isn't a terminal, so I'd call the variable unit. I think
there was another occurrence of this issue in another patch in the
series.

> +	u8 value;
> +
> +	list_for_each_entry(chain, &dev->chains, list) {

This will effectively call uvc_ctrl_status_event() once per chain, is
that really what you were intending ?

> +		list_for_each_entry(term, &dev->entities, list) {
> +			if (UVC_ENTITY_TYPE(term) == UVC_GPIO_UNIT) {
> +				value = gpiod_get_value(term->gpio.gpio_privacy);
> +				uvc_ctrl_status_event(NULL, chain, term->controls, &value);

80 columns.

> +			}
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int uvc_parse_gpio(struct uvc_device *dev)
>  {
>  	struct uvc_entity *unit;
> @@ -1490,6 +1509,17 @@ static int uvc_parse_gpio(struct uvc_device *dev)
>  
>  	list_add_tail(&unit->list, &dev->entities);
>  
> +	irq = gpiod_to_irq(gpio_privacy);
> +

No need for a blank line.

And it looks like the local irq variable should be introduced in this
patch, not in the previous one. I think I'd squash this with 5/6.

> +	if (irq == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;



> +
> +	if (irq < 0)
> +		return 0;
> +
> +	ret = devm_request_irq(&dev->udev->dev, irq, uvc_privacy_gpio_irq, IRQF_SHARED,

80 columns.

> +			       "uvc_privacy_gpio", dev);

Do we need to handle failures ?

> +
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart