mbox series

[v7,00/12] Show privacy_gpio as a v4l2_ctrl

Message ID 20201223133528.55014-1-ribalda@chromium.org
Headers show
Series Show privacy_gpio as a v4l2_ctrl | expand

Message

Ricardo Ribalda Dec. 23, 2020, 1:35 p.m. UTC 
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.

v7: Thanks to all the comments from Laurent, Sakari and Joe
  - New patch from Joe: Rename debug functions
  - Rename direct handler to sync/async handler
  - Only launch events with IRQ
  - Use mutex on the stream_quirk
  - CodeStyle, spaces are my friends

v6: Thanks to all the comments from Laurent!
  - Remove multiple async_ctrls from v5, it is not needed
  - Split event handling in two parts, so it can be triggered without wq
  - Save pointer to the privacy entity in the main structure
  - Handle the quirk in a different location to avoid races
  - CodeStyle

v5: Thanks to all the comments from Laurent!
  - Allow multiple async_ctrls
  - Use dev_dbg() for uvc_trace
  - Major redesing of "Implement UVC_EXT_GPIO_UNIT"
  - Major redesing of "Implement UVC_QUIRK_PRIVACY_DURING_STREAM"

v4: Implement UVC_QUIRK_PRIVACY_DURING_STREAM

v3: Thanks to all the comments from Joe Perches
  - Rework of printk macros

v2: Thanks to all the comments from Laurent!
  - move guid to unit
  - support entities with no pads
  - CodeStyle
  - Irq handling
  - pr_cont
  - new ids

Joe Perches (1):
  media: uvcvideo: Rename debug functions

Ricardo Ribalda (11):
  media: uvcvideo: Move guid to entity
  media: uvcvideo: Allow extra entities
  media: uvcvideo: Allow entities with no pads
  media: uvcvideo: Provide sync and async uvc_ctrl_status_event
  media: uvcvideo: Allow entity-defined get_info and get_cur
  media: uvcvideo: Implement UVC_EXT_GPIO_UNIT
  media: uvcvideo: Add Privacy control based on EXT_GPIO
  media: uvcvideo: Use dev_ printk aliases
  media: uvcvideo: New macro uvc_trace_cont
  media: uvcvideo: use dev_printk() for uvc_trace()
  media: uvcvideo: Implement UVC_QUIRK_PRIVACY_DURING_STREAM

 drivers/media/usb/uvc/uvc_ctrl.c   | 179 +++++----
 drivers/media/usb/uvc/uvc_driver.c | 580 +++++++++++++++++++----------
 drivers/media/usb/uvc/uvc_entity.c |  11 +-
 drivers/media/usb/uvc/uvc_isight.c |  17 +-
 drivers/media/usb/uvc/uvc_queue.c  |   9 +-
 drivers/media/usb/uvc/uvc_status.c |  44 +--
 drivers/media/usb/uvc/uvc_v4l2.c   |  48 +--
 drivers/media/usb/uvc/uvc_video.c  | 189 ++++++----
 drivers/media/usb/uvc/uvcvideo.h   | 105 ++++--
 9 files changed, 736 insertions(+), 446 deletions(-)

Comments

Andy Shevchenko Dec. 24, 2020, 12:59 p.m. UTC | #1 
On Wed, Dec 23, 2020 at 3:39 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
>

> Replace all the uses of printk() and uvc_printk() with its

> equivalent dev_ alias macros.


> Modify uvc_warn_once() macro to use dev_info instead printk().


...

> +#define uvc_warn_once(_dev, warn, fmt, ...)                            \

> +do {                                                                   \

> +       if (!test_and_set_bit(warn, &(_dev)->warnings))                 \

> +               dev_info(&(_dev)->udev->dev, fmt, ##__VA_ARGS__);       \

> +} while (0)


...

Why not to use dev_warn_once() instead?

-- 
With Best Regards,
Andy Shevchenko
Laurent Pinchart Dec. 24, 2020, 1:50 p.m. UTC | #2 
Hi Andy,

On Thu, Dec 24, 2020 at 02:59:34PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 23, 2020 at 3:39 PM Ricardo Ribalda wrote:

> >

> > Replace all the uses of printk() and uvc_printk() with its

> > equivalent dev_ alias macros.

> 

> > Modify uvc_warn_once() macro to use dev_info instead printk().

> 

> ...

> 

> > +#define uvc_warn_once(_dev, warn, fmt, ...)                            \

> > +do {                                                                   \

> > +       if (!test_and_set_bit(warn, &(_dev)->warnings))                 \

> > +               dev_info(&(_dev)->udev->dev, fmt, ##__VA_ARGS__);       \

> > +} while (0)

> 

> ...

> 

> Why not to use dev_warn_once() instead?


uvc_warn_once() prints the warning once per device, not once globally.

-- 
Regards,

Laurent Pinchart
Laurent Pinchart Jan. 26, 2021, 3:38 p.m. UTC | #3 
Hi Ricardo,

Thank you for the patch.

On Wed, Dec 23, 2020 at 02:35:20PM +0100, Ricardo Ribalda wrote:
> Split the functionality of void uvc_ctrl_status_event_work in two, so it

> can be called by functions outside interrupt context and not part of an

> URB.

> 

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


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


> ---

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

>  drivers/media/usb/uvc/uvc_status.c |  3 ++-

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

>  3 files changed, 20 insertions(+), 12 deletions(-)

> 

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

> index 9f6174a10e73..4d43f4c3e349 100644

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

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

> @@ -1254,17 +1254,12 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,

>  	uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);

>  }

>  

> -static void uvc_ctrl_status_event_work(struct work_struct *work)

> +void uvc_ctrl_status_event(struct uvc_video_chain *chain,

> +			   struct uvc_control *ctrl, const u8 *data)

>  {

> -	struct uvc_device *dev = container_of(work, struct uvc_device,

> -					      async_ctrl.work);

> -	struct uvc_ctrl_work *w = &dev->async_ctrl;

> -	struct uvc_video_chain *chain = w->chain;

>  	struct uvc_control_mapping *mapping;

> -	struct uvc_control *ctrl = w->ctrl;

>  	struct uvc_fh *handle;

>  	unsigned int i;

> -	int ret;

>  

>  	mutex_lock(&chain->ctrl_mutex);

>  

> @@ -1272,7 +1267,7 @@ static void uvc_ctrl_status_event_work(struct work_struct *work)

>  	ctrl->handle = NULL;

>  

>  	list_for_each_entry(mapping, &ctrl->info.mappings, list) {

> -		s32 value = __uvc_ctrl_get_value(mapping, w->data);

> +		s32 value = __uvc_ctrl_get_value(mapping, data);

>  

>  		/*

>  		 * handle may be NULL here if the device sends auto-update

> @@ -1291,6 +1286,16 @@ static void uvc_ctrl_status_event_work(struct work_struct *work)

>  	}

>  

>  	mutex_unlock(&chain->ctrl_mutex);

> +}

> +

> +static void uvc_ctrl_status_event_work(struct work_struct *work)

> +{

> +	struct uvc_device *dev = container_of(work, struct uvc_device,

> +					      async_ctrl.work);

> +	struct uvc_ctrl_work *w = &dev->async_ctrl;

> +	int ret;

> +

> +	uvc_ctrl_status_event(w->chain, w->ctrl, w->data);

>  

>  	/* Resubmit the URB. */

>  	w->urb->interval = dev->int_ep->desc.bInterval;

> @@ -1300,8 +1305,8 @@ static void uvc_ctrl_status_event_work(struct work_struct *work)

>  			   ret);

>  }

>  

> -bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,

> -			   struct uvc_control *ctrl, const u8 *data)

> +bool uvc_ctrl_status_event_async(struct urb *urb, struct uvc_video_chain *chain,

> +				 struct uvc_control *ctrl, const u8 *data)

>  {

>  	struct uvc_device *dev = chain->dev;

>  	struct uvc_ctrl_work *w = &dev->async_ctrl;

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

> index 2bdb0ff203f8..3e26d82a906d 100644

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

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

> @@ -179,7 +179,8 @@ static bool uvc_event_control(struct urb *urb,

>  

>  	switch (status->bAttribute) {

>  	case UVC_CTRL_VALUE_CHANGE:

> -		return uvc_ctrl_status_event(urb, chain, ctrl, status->bValue);

> +		return uvc_ctrl_status_event_async(urb, chain, ctrl,

> +						   status->bValue);

>  

>  	case UVC_CTRL_INFO_CHANGE:

>  	case UVC_CTRL_FAILURE_CHANGE:

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

> index c50b0546901f..be784ed8354d 100644

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

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

> @@ -843,7 +843,9 @@ int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,

>  int uvc_ctrl_init_device(struct uvc_device *dev);

>  void uvc_ctrl_cleanup_device(struct uvc_device *dev);

>  int uvc_ctrl_restore_values(struct uvc_device *dev);

> -bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,

> +bool uvc_ctrl_status_event_async(struct urb *urb, struct uvc_video_chain *chain,

> +				 struct uvc_control *ctrl, const u8 *data);

> +void uvc_ctrl_status_event(struct uvc_video_chain *chain,

>  			   struct uvc_control *ctrl, const u8 *data);

>  

>  int uvc_ctrl_begin(struct uvc_video_chain *chain);


-- 
Regards,

Laurent Pinchart
Laurent Pinchart Jan. 26, 2021, 3:53 p.m. UTC | #4 
Hi Ricardo,

Thank you for the patch.

On Wed, Dec 23, 2020 at 02:35:22PM +0100, 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.

> 

> In some systems, the GPIO is connected to main SoC instead of the


s/to main/to the main/

> camera controller, with the connection reported by the system firmware

> (ACPI or DT). In that case, the UVC device isn't aware of the GPIO. We

> need to implement a virtual entity to handle the GPIO fully on the

> driver side.

> 

> For example, for ACPI-based systems, the GPIO is reported in the USB

> device object:

> 

>   Scope (\_SB.PCI0.XHCI.RHUB.HS07)

>   {

> 

> 	  /.../

> 

>     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 | 127 +++++++++++++++++++++++++++++

>  drivers/media/usb/uvc/uvc_entity.c |   1 +

>  drivers/media/usb/uvc/uvcvideo.h   |  16 ++++

>  4 files changed, 147 insertions(+)

> 

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

> index 1a5e85368af4..e0ab55583dd8 100644

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

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

> @@ -2291,6 +2291,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_EXT_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 c0c5f75ade40..b0bf93c44999 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>

> @@ -1020,6 +1021,7 @@ static int uvc_parse_streaming(struct uvc_device *dev,

>  }

>  

>  static const u8 uvc_camera_guid[16] = UVC_GUID_UVC_CAMERA;

> +static const u8 uvc_gpio_guid[16] = UVC_GUID_EXT_GPIO_CONTROLLER;

>  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;

> @@ -1051,6 +1053,9 @@ static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,

>  	 * is initialized by the caller.

>  	 */

>  	switch (type) {

> +	case UVC_EXT_GPIO_UNIT:

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

> +		break;

>  	case UVC_ITT_CAMERA:

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

>  		break;

> @@ -1464,6 +1469,108 @@ static int uvc_parse_control(struct uvc_device *dev)

>  	return 0;

>  }

>  

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

> + * Privacy GPIO

> + */

> +

> +static void uvc_gpio_event(struct uvc_device *dev)

> +{

> +	struct uvc_entity *unit = dev->gpio_unit;

> +	struct uvc_video_chain *chain;

> +	u8 new_val;

> +

> +	if (!unit)

> +		return;

> +

> +	new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);

> +

> +	/* GPIO entities are always on the first chain. */

> +	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);

> +	uvc_ctrl_status_event(chain, unit->controls, &new_val);

> +}

> +

> +static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,

> +			    u8 cs, void *data, u16 size)

> +{

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

> +		return -EINVAL;

> +

> +	*(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);

> +

> +	return 0;

> +}

> +

> +static int uvc_gpio_get_info(struct uvc_device *dev, 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 irqreturn_t uvc_gpio_irq(int irq, void *data)

> +{

> +	struct uvc_device *dev = data;

> +

> +	uvc_gpio_event(dev);


Should uvc_gpio_event() be inlined here ? If so I can make the
modification when applying, there's no need for a v8 just for this.

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


> +	return IRQ_HANDLED;

> +}

> +

> +static int uvc_gpio_parse(struct uvc_device *dev)

> +{

> +	struct uvc_entity *unit;

> +	struct gpio_desc *gpio_privacy;

> +	int irq;

> +

> +	gpio_privacy = devm_gpiod_get_optional(&dev->udev->dev, "privacy",

> +					       GPIOD_IN);

> +	if (IS_ERR_OR_NULL(gpio_privacy))

> +		return PTR_ERR_OR_ZERO(gpio_privacy);

> +

> +	unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);

> +	if (!unit)

> +		return -ENOMEM;

> +

> +	irq = gpiod_to_irq(gpio_privacy);

> +	if (irq < 0) {

> +		if (irq != EPROBE_DEFER)

> +			dev_err(&dev->udev->dev,

> +				"No IRQ for privacy GPIO (%d)\n", irq);

> +		return irq;

> +	}

> +

> +	unit->gpio.gpio_privacy = gpio_privacy;

> +	unit->gpio.irq = irq;

> +	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;

> +	strncpy(unit->name, "GPIO", sizeof(unit->name) - 1);

> +

> +	list_add_tail(&unit->list, &dev->entities);

> +

> +	dev->gpio_unit = unit;

> +

> +	return 0;

> +}

> +

> +static int uvc_gpio_init_irq(struct uvc_device *dev)

> +{

> +	struct uvc_entity *unit = dev->gpio_unit;

> +

> +	if (!unit || unit->gpio.irq < 0)

> +		return 0;

> +

> +	return devm_request_threaded_irq(&dev->udev->dev, unit->gpio.irq, NULL,

> +					 uvc_gpio_irq,

> +					 IRQF_ONESHOT | IRQF_TRIGGER_FALLING |

> +					 IRQF_TRIGGER_RISING,

> +					 "uvc_privacy_gpio", dev);

> +}

> +

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

>   * UVC device scan

>   */

> @@ -1953,6 +2060,13 @@ static int uvc_scan_device(struct uvc_device *dev)

>  		return -1;

>  	}

>  

> +	/* Add GPIO entity to the first chain. */

> +	if (dev->gpio_unit) {

> +		chain = list_first_entry(&dev->chains,

> +					 struct uvc_video_chain, list);

> +		list_add_tail(&dev->gpio_unit->chain, &chain->entities);

> +	}

> +

>  	return 0;

>  }

>  

> @@ -2285,6 +2399,12 @@ static int uvc_probe(struct usb_interface *intf,

>  		goto error;

>  	}

>  

> +	/* Parse the associated GPIOs. */

> +	if (uvc_gpio_parse(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>",

> @@ -2329,6 +2449,13 @@ static int uvc_probe(struct usb_interface *intf,

>  			"supported.\n", ret);

>  	}

>  

> +	ret = uvc_gpio_init_irq(dev);

> +	if (ret < 0) {

> +		dev_err(&dev->udev->dev,

> +			"Unable to request privacy GPIO IRQ (%d)\n", ret);

> +		goto error;

> +	}

> +

>  	uvc_trace(UVC_TRACE_PROBE, "UVC device initialized.\n");

>  	usb_enable_autosuspend(udev);

>  	return 0;

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

> index ca3a9c2eec27..6a9ba5b498db 100644

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

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

> @@ -105,6 +105,7 @@ static int uvc_mc_init_entity(struct uvc_video_chain *chain,

>  		case UVC_OTT_DISPLAY:

>  		case UVC_OTT_MEDIA_TRANSPORT_OUTPUT:

>  		case UVC_EXTERNAL_VENDOR_SPECIFIC:

> +		case UVC_EXT_GPIO_UNIT:

>  		default:

>  			function = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;

>  			break;

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

> index 6465711fe5bb..4211531a3558 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/atomic.h>

>  #include <linux/kernel.h>

>  #include <linux/poll.h>

>  #include <linux/usb.h>

> @@ -37,6 +38,8 @@

>  	(UVC_ENTITY_IS_TERM(entity) && \

>  	((entity)->type & 0x8000) == UVC_TERM_OUTPUT)

>  

> +#define UVC_EXT_GPIO_UNIT		0x7ffe

> +#define UVC_EXT_GPIO_UNIT_ID		0x100

>  

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

>   * GUIDs

> @@ -56,6 +59,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 \

> +	{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, \

> +	 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x03}

>  

>  #define UVC_GUID_FORMAT_MJPEG \

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

> @@ -212,6 +218,7 @@

>   * Structures.

>   */

>  

> +struct gpio_desc;

>  struct uvc_device;

>  

>  /* TODO: Put the most frequently accessed fields at the beginning of

> @@ -353,6 +360,13 @@ struct uvc_entity {

>  			u8  *bmControls;

>  			u8  *bmControlsType;

>  		} extension;

> +

> +		struct {

> +			u8  bControlSize;

> +			u8  *bmControls;

> +			struct gpio_desc *gpio_privacy;

> +			int irq;

> +		} gpio;

>  	};

>  

>  	u8 bNrInPins;

> @@ -690,6 +704,8 @@ struct uvc_device {

>  		struct uvc_control *ctrl;

>  		const void *data;

>  	} async_ctrl;

> +

> +	struct uvc_entity *gpio_unit;

>  };

>  

>  enum uvc_handle_state {


-- 
Regards,

Laurent Pinchart
Laurent Pinchart Jan. 26, 2021, 3:56 p.m. UTC | #5 
On Tue, Jan 26, 2021 at 05:53:19PM +0200, Laurent Pinchart wrote:
> Hi Ricardo,

> 

> Thank you for the patch.

> 

> On Wed, Dec 23, 2020 at 02:35:22PM +0100, 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.

> > 

> > In some systems, the GPIO is connected to main SoC instead of the

> 

> s/to main/to the main/

> 

> > camera controller, with the connection reported by the system firmware

> > (ACPI or DT). In that case, the UVC device isn't aware of the GPIO. We

> > need to implement a virtual entity to handle the GPIO fully on the

> > driver side.

> > 

> > For example, for ACPI-based systems, the GPIO is reported in the USB

> > device object:

> > 

> >   Scope (\_SB.PCI0.XHCI.RHUB.HS07)

> >   {

> > 

> > 	  /.../

> > 

> >     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 | 127 +++++++++++++++++++++++++++++

> >  drivers/media/usb/uvc/uvc_entity.c |   1 +

> >  drivers/media/usb/uvc/uvcvideo.h   |  16 ++++

> >  4 files changed, 147 insertions(+)

> > 

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

> > index 1a5e85368af4..e0ab55583dd8 100644

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

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

> > @@ -2291,6 +2291,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_EXT_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 c0c5f75ade40..b0bf93c44999 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>

> > @@ -1020,6 +1021,7 @@ static int uvc_parse_streaming(struct uvc_device *dev,

> >  }

> >  

> >  static const u8 uvc_camera_guid[16] = UVC_GUID_UVC_CAMERA;

> > +static const u8 uvc_gpio_guid[16] = UVC_GUID_EXT_GPIO_CONTROLLER;

> >  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;

> > @@ -1051,6 +1053,9 @@ static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,

> >  	 * is initialized by the caller.

> >  	 */

> >  	switch (type) {

> > +	case UVC_EXT_GPIO_UNIT:

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

> > +		break;

> >  	case UVC_ITT_CAMERA:

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

> >  		break;

> > @@ -1464,6 +1469,108 @@ static int uvc_parse_control(struct uvc_device *dev)

> >  	return 0;

> >  }

> >  

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

> > + * Privacy GPIO

> > + */

> > +

> > +static void uvc_gpio_event(struct uvc_device *dev)

> > +{

> > +	struct uvc_entity *unit = dev->gpio_unit;

> > +	struct uvc_video_chain *chain;

> > +	u8 new_val;

> > +

> > +	if (!unit)

> > +		return;

> > +

> > +	new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);

> > +

> > +	/* GPIO entities are always on the first chain. */

> > +	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);

> > +	uvc_ctrl_status_event(chain, unit->controls, &new_val);

> > +}

> > +

> > +static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,

> > +			    u8 cs, void *data, u16 size)

> > +{

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

> > +		return -EINVAL;

> > +

> > +	*(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);

> > +

> > +	return 0;

> > +}

> > +

> > +static int uvc_gpio_get_info(struct uvc_device *dev, 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 irqreturn_t uvc_gpio_irq(int irq, void *data)

> > +{

> > +	struct uvc_device *dev = data;

> > +

> > +	uvc_gpio_event(dev);

> 

> Should uvc_gpio_event() be inlined here ? If so I can make the

> modification when applying, there's no need for a v8 just for this.


I've seen the function is called in a different location later in this
series, please ignore the noise.

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

> 

> > +	return IRQ_HANDLED;

> > +}

> > +

> > +static int uvc_gpio_parse(struct uvc_device *dev)

> > +{

> > +	struct uvc_entity *unit;

> > +	struct gpio_desc *gpio_privacy;

> > +	int irq;

> > +

> > +	gpio_privacy = devm_gpiod_get_optional(&dev->udev->dev, "privacy",

> > +					       GPIOD_IN);

> > +	if (IS_ERR_OR_NULL(gpio_privacy))

> > +		return PTR_ERR_OR_ZERO(gpio_privacy);

> > +

> > +	unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);

> > +	if (!unit)

> > +		return -ENOMEM;

> > +

> > +	irq = gpiod_to_irq(gpio_privacy);

> > +	if (irq < 0) {

> > +		if (irq != EPROBE_DEFER)

> > +			dev_err(&dev->udev->dev,

> > +				"No IRQ for privacy GPIO (%d)\n", irq);

> > +		return irq;

> > +	}

> > +

> > +	unit->gpio.gpio_privacy = gpio_privacy;

> > +	unit->gpio.irq = irq;

> > +	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;

> > +	strncpy(unit->name, "GPIO", sizeof(unit->name) - 1);

> > +

> > +	list_add_tail(&unit->list, &dev->entities);

> > +

> > +	dev->gpio_unit = unit;

> > +

> > +	return 0;

> > +}

> > +

> > +static int uvc_gpio_init_irq(struct uvc_device *dev)

> > +{

> > +	struct uvc_entity *unit = dev->gpio_unit;

> > +

> > +	if (!unit || unit->gpio.irq < 0)

> > +		return 0;

> > +

> > +	return devm_request_threaded_irq(&dev->udev->dev, unit->gpio.irq, NULL,

> > +					 uvc_gpio_irq,

> > +					 IRQF_ONESHOT | IRQF_TRIGGER_FALLING |

> > +					 IRQF_TRIGGER_RISING,

> > +					 "uvc_privacy_gpio", dev);

> > +}

> > +

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

> >   * UVC device scan

> >   */

> > @@ -1953,6 +2060,13 @@ static int uvc_scan_device(struct uvc_device *dev)

> >  		return -1;

> >  	}

> >  

> > +	/* Add GPIO entity to the first chain. */

> > +	if (dev->gpio_unit) {

> > +		chain = list_first_entry(&dev->chains,

> > +					 struct uvc_video_chain, list);

> > +		list_add_tail(&dev->gpio_unit->chain, &chain->entities);

> > +	}

> > +

> >  	return 0;

> >  }

> >  

> > @@ -2285,6 +2399,12 @@ static int uvc_probe(struct usb_interface *intf,

> >  		goto error;

> >  	}

> >  

> > +	/* Parse the associated GPIOs. */

> > +	if (uvc_gpio_parse(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>",

> > @@ -2329,6 +2449,13 @@ static int uvc_probe(struct usb_interface *intf,

> >  			"supported.\n", ret);

> >  	}

> >  

> > +	ret = uvc_gpio_init_irq(dev);

> > +	if (ret < 0) {

> > +		dev_err(&dev->udev->dev,

> > +			"Unable to request privacy GPIO IRQ (%d)\n", ret);

> > +		goto error;

> > +	}

> > +

> >  	uvc_trace(UVC_TRACE_PROBE, "UVC device initialized.\n");

> >  	usb_enable_autosuspend(udev);

> >  	return 0;

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

> > index ca3a9c2eec27..6a9ba5b498db 100644

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

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

> > @@ -105,6 +105,7 @@ static int uvc_mc_init_entity(struct uvc_video_chain *chain,

> >  		case UVC_OTT_DISPLAY:

> >  		case UVC_OTT_MEDIA_TRANSPORT_OUTPUT:

> >  		case UVC_EXTERNAL_VENDOR_SPECIFIC:

> > +		case UVC_EXT_GPIO_UNIT:

> >  		default:

> >  			function = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;

> >  			break;

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

> > index 6465711fe5bb..4211531a3558 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/atomic.h>

> >  #include <linux/kernel.h>

> >  #include <linux/poll.h>

> >  #include <linux/usb.h>

> > @@ -37,6 +38,8 @@

> >  	(UVC_ENTITY_IS_TERM(entity) && \

> >  	((entity)->type & 0x8000) == UVC_TERM_OUTPUT)

> >  

> > +#define UVC_EXT_GPIO_UNIT		0x7ffe

> > +#define UVC_EXT_GPIO_UNIT_ID		0x100

> >  

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

> >   * GUIDs

> > @@ -56,6 +59,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 \

> > +	{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, \

> > +	 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x03}

> >  

> >  #define UVC_GUID_FORMAT_MJPEG \

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

> > @@ -212,6 +218,7 @@

> >   * Structures.

> >   */

> >  

> > +struct gpio_desc;

> >  struct uvc_device;

> >  

> >  /* TODO: Put the most frequently accessed fields at the beginning of

> > @@ -353,6 +360,13 @@ struct uvc_entity {

> >  			u8  *bmControls;

> >  			u8  *bmControlsType;

> >  		} extension;

> > +

> > +		struct {

> > +			u8  bControlSize;

> > +			u8  *bmControls;

> > +			struct gpio_desc *gpio_privacy;

> > +			int irq;

> > +		} gpio;

> >  	};

> >  

> >  	u8 bNrInPins;

> > @@ -690,6 +704,8 @@ struct uvc_device {

> >  		struct uvc_control *ctrl;

> >  		const void *data;

> >  	} async_ctrl;

> > +

> > +	struct uvc_entity *gpio_unit;

> >  };

> >  

> >  enum uvc_handle_state {


-- 
Regards,

Laurent Pinchart