[PATCH v4 0/9] Show privacy_gpio as a v4l2_ctrl

[PATCH v4 0/9] 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.

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

Ricardo Ribalda (9):
  media: uvcvideo: Move guid to entity
  media: uvcvideo: Allow external entities
  media: uvcvideo: Allow entities with no pads
  media: uvcvideo: 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: Implement UVC_QUIRK_PRIVACY_DURING_STREAM

 drivers/media/usb/uvc/uvc_ctrl.c   |  80 ++++---
 drivers/media/usb/uvc/uvc_driver.c | 322 +++++++++++++++++++++++------
 drivers/media/usb/uvc/uvc_entity.c |  10 +-
 drivers/media/usb/uvc/uvc_queue.c  |   3 +
 drivers/media/usb/uvc/uvc_status.c |  13 +-
 drivers/media/usb/uvc/uvc_video.c  |  51 ++---
 drivers/media/usb/uvc/uvcvideo.h   |  54 +++--
 7 files changed, 389 insertions(+), 144 deletions(-)

-- 
2.29.2.684.gfbc64c5ab5-goog

[PATCH v4 1/9] media: uvcvideo: Move guid to entity

[Ricardo Ribalda]

Instead of having multiple copies of the entity guid on the code, move
it to the entity structure.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c   | 30 ++++--------------------------
 drivers/media/usb/uvc/uvc_driver.c | 26 ++++++++++++++++++++++++--
 drivers/media/usb/uvc/uvcvideo.h   |  2 +-
 3 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 011e69427b7c..9f6174a10e73 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -826,31 +826,10 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping,
  * Terminal and unit management
  */
 
-static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
-static const u8 uvc_camera_guid[16] = UVC_GUID_UVC_CAMERA;
-static const u8 uvc_media_transport_input_guid[16] =
-	UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
-
 static int uvc_entity_match_guid(const struct uvc_entity *entity,
-	const u8 guid[16])
+				 const u8 guid[16])
 {
-	switch (UVC_ENTITY_TYPE(entity)) {
-	case UVC_ITT_CAMERA:
-		return memcmp(uvc_camera_guid, guid, 16) == 0;
-
-	case UVC_ITT_MEDIA_TRANSPORT_INPUT:
-		return memcmp(uvc_media_transport_input_guid, guid, 16) == 0;
-
-	case UVC_VC_PROCESSING_UNIT:
-		return memcmp(uvc_processing_guid, guid, 16) == 0;
-
-	case UVC_VC_EXTENSION_UNIT:
-		return memcmp(entity->extension.guidExtensionCode,
-			      guid, 16) == 0;
-
-	default:
-		return 0;
-	}
+	return memcmp(entity->guid, guid, sizeof(entity->guid)) == 0;
 }
 
 /* ------------------------------------------------------------------------
@@ -1776,8 +1755,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
 	if (data == NULL)
 		return -ENOMEM;
 
-	memcpy(info->entity, ctrl->entity->extension.guidExtensionCode,
-	       sizeof(info->entity));
+	memcpy(info->entity, ctrl->entity->guid, sizeof(info->entity));
 	info->index = ctrl->index;
 	info->selector = ctrl->index + 1;
 
@@ -1883,7 +1861,7 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
 
 	if (!found) {
 		uvc_trace(UVC_TRACE_CONTROL, "Control %pUl/%u not found.\n",
-			entity->extension.guidExtensionCode, xqry->selector);
+			entity->guid, xqry->selector);
 		return -ENOENT;
 	}
 
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index ddb9eaa11be7..4cdd65d252d9 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1019,6 +1019,11 @@ static int uvc_parse_streaming(struct uvc_device *dev,
 	return ret;
 }
 
+static const u8 uvc_camera_guid[16] = UVC_GUID_UVC_CAMERA;
+static const u8 uvc_media_transport_input_guid[16] =
+	UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
+static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
+
 static struct uvc_entity *uvc_alloc_entity(u16 type, u8 id,
 		unsigned int num_pads, unsigned int extra_size)
 {
@@ -1038,6 +1043,23 @@ static struct uvc_entity *uvc_alloc_entity(u16 type, u8 id,
 	entity->id = id;
 	entity->type = type;
 
+
+	/*
+	 * Set the GUID for standard entity types. For extension units, the GUID
+	 * is initialized by the caller.
+	 */
+	switch (type) {
+	case UVC_ITT_CAMERA:
+		memcpy(entity->guid, uvc_camera_guid, 16);
+		break;
+	case UVC_ITT_MEDIA_TRANSPORT_INPUT:
+		memcpy(entity->guid, uvc_media_transport_input_guid, 16);
+		break;
+	case UVC_VC_PROCESSING_UNIT:
+		memcpy(entity->guid, uvc_processing_guid, 16);
+		break;
+	}
+
 	entity->num_links = 0;
 	entity->num_pads = num_pads;
 	entity->pads = ((void *)(entity + 1)) + extra_size;
@@ -1109,7 +1131,7 @@ static int uvc_parse_vendor_control(struct uvc_device *dev,
 		if (unit == NULL)
 			return -ENOMEM;
 
-		memcpy(unit->extension.guidExtensionCode, &buffer[4], 16);
+		memcpy(unit->guid, &buffer[4], 16);
 		unit->extension.bNumControls = buffer[20];
 		memcpy(unit->baSourceID, &buffer[22], p);
 		unit->extension.bControlSize = buffer[22+p];
@@ -1368,7 +1390,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
 		if (unit == NULL)
 			return -ENOMEM;
 
-		memcpy(unit->extension.guidExtensionCode, &buffer[4], 16);
+		memcpy(unit->guid, &buffer[4], 16);
 		unit->extension.bNumControls = buffer[20];
 		memcpy(unit->baSourceID, &buffer[22], p);
 		unit->extension.bControlSize = buffer[22+p];
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index a3dfacf069c4..df7bf2d104a3 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -304,6 +304,7 @@ struct uvc_entity {
 	u8 id;
 	u16 type;
 	char name[64];
+	u8 guid[16];
 
 	/* Media controller-related fields. */
 	struct video_device *vdev;
@@ -342,7 +343,6 @@ struct uvc_entity {
 		} selector;
 
 		struct {
-			u8  guidExtensionCode[16];
 			u8  bNumControls;
 			u8  bControlSize;
 			u8  *bmControls;
-- 
2.29.2.684.gfbc64c5ab5-goog

[PATCH v4 2/9] media: uvcvideo: Allow external entities

[Ricardo Ribalda]

Increase the size of the id, to avoid collisions with external entities.

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

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 4cdd65d252d9..9f4451a2e0a6 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1024,7 +1024,7 @@ static const u8 uvc_media_transport_input_guid[16] =
 	UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
 static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
 
-static struct uvc_entity *uvc_alloc_entity(u16 type, u8 id,
+static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
 		unsigned int num_pads, unsigned int extra_size)
 {
 	struct uvc_entity *entity;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index df7bf2d104a3..00f985001c1d 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -301,7 +301,7 @@ struct uvc_entity {
 					 * chain. */
 	unsigned int flags;
 
-	u8 id;
+	u16 id; /* 0-255: usb entity. 256-65535: external entities */
 	u16 type;
 	char name[64];
 	u8 guid[16];
-- 
2.29.2.684.gfbc64c5ab5-goog

[PATCH v4 3/9] media: uvcvideo: Allow entities with no pads

[Ricardo Ribalda]

Avoid an underflow while calculating the number of inputs for entities
with zero pads.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 9f4451a2e0a6..534629ecd60d 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1033,7 +1033,11 @@ static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
 	unsigned int i;
 
 	extra_size = roundup(extra_size, sizeof(*entity->pads));
-	num_inputs = (type & UVC_TERM_OUTPUT) ? num_pads : num_pads - 1;
+	if (num_pads)
+		num_inputs = (type & UVC_TERM_OUTPUT) ? num_pads :
+								num_pads - 1;
+	else
+		num_inputs = 0;
 	size = sizeof(*entity) + extra_size + sizeof(*entity->pads) * num_pads
 	     + num_inputs;
 	entity = kzalloc(size, GFP_KERNEL);
@@ -1066,7 +1070,7 @@ static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
 
 	for (i = 0; i < num_inputs; ++i)
 		entity->pads[i].flags = MEDIA_PAD_FL_SINK;
-	if (!UVC_ENTITY_IS_OTERM(entity))
+	if (!UVC_ENTITY_IS_OTERM(entity) && num_pads)
 		entity->pads[num_pads-1].flags = MEDIA_PAD_FL_SOURCE;
 
 	entity->bNrInPins = num_inputs;
-- 
2.29.2.684.gfbc64c5ab5-goog

[PATCH v4 4/9] media: uvcvideo: Entity defined get_info and get_cur

[Ricardo Ribalda]

Allows controls to get their properties and 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 | 22 ++++++++++++++++++----
 drivers/media/usb/uvc/uvcvideo.h |  5 +++++
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 9f6174a10e73..531816762892 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -980,10 +980,20 @@ 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,
+		if (ctrl->entity->get_cur) {
+			ret = ctrl->entity->get_cur(chain->dev,
+				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;
 
@@ -1687,8 +1697,12 @@ 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->entity->get_info)
+		ret = ctrl->entity->get_info(dev, ctrl->entity,
+					     ctrl->info.selector, data);
+	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 00f985001c1d..ae464ba83f4f 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -353,6 +353,11 @@ struct uvc_entity {
 	u8 bNrInPins;
 	u8 *baSourceID;
 
+	int (*get_info)(struct uvc_device *dev, struct uvc_entity *entity,
+			u8 cs, u8 *caps);
+	int (*get_cur)(struct uvc_device *dev, struct uvc_entity *entity,
+		       u8 cs, void *data, u16 size);
+
 	unsigned int ncontrols;
 	struct uvc_control *controls;
 };
-- 
2.29.2.684.gfbc64c5ab5-goog

[PATCH v4 5/9] media: uvcvideo: Implement UVC_EXT_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:

  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   |   6 ++
 drivers/media/usb/uvc/uvc_driver.c | 106 +++++++++++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  12 ++++
 3 files changed, 124 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 531816762892..53da1d984883 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1302,6 +1302,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);
@@ -2286,6 +2289,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 534629ecd60d..498de09da07e 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;
@@ -1053,6 +1055,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;
@@ -1466,6 +1471,93 @@ static int uvc_parse_control(struct uvc_device *dev)
 	return 0;
 }
 
+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;
+
+	*(uint8_t *)data = gpiod_get_value(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_privacy_gpio_irq(int irq, void *data)
+{
+	struct uvc_device *dev = data;
+	struct uvc_video_chain *chain;
+	struct uvc_entity *unit;
+	u8 value;
+
+	/* GPIO entities are always on the first chain */
+	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
+	list_for_each_entry(unit, &dev->entities, list) {
+		if (UVC_ENTITY_TYPE(unit) != UVC_EXT_GPIO_UNIT)
+			continue;
+		value = gpiod_get_value(unit->gpio.gpio_privacy);
+		uvc_ctrl_status_event(NULL, chain, unit->controls, &value);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_HANDLED;
+}
+
+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_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
+	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);
+
+	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_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+			       "uvc_privacy_gpio", dev);
+	if (ret < 0)
+		dev_warn(&dev->udev->dev,
+		       "Unable to request uvc_privacy_gpio irq. Continuing\n");
+
+	return 0;
+}
+
 /* ------------------------------------------------------------------------
  * UVC device scan
  */
@@ -1917,6 +2009,7 @@ static int uvc_scan_device(struct uvc_device *dev)
 {
 	struct uvc_video_chain *chain;
 	struct uvc_entity *term;
+	struct uvc_entity *unit;
 
 	list_for_each_entry(term, &dev->entities, list) {
 		if (!UVC_ENTITY_IS_OTERM(term))
@@ -1955,6 +2048,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(unit, &dev->entities, list) {
+		if (UVC_ENTITY_TYPE(unit) == UVC_EXT_GPIO_UNIT)
+			list_add_tail(&unit->chain, &chain->entities);
+	}
+
 	return 0;
 }
 
@@ -2287,6 +2387,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 ae464ba83f4f..f87d14fb3f56 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,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, \
@@ -348,6 +354,12 @@ struct uvc_entity {
 			u8  *bmControls;
 			u8  *bmControlsType;
 		} extension;
+
+		struct {
+			u8  bControlSize;
+			u8  *bmControls;
+			struct gpio_desc *gpio_privacy;
+		} gpio;
 	};
 
 	u8 bNrInPins;
-- 
2.29.2.684.gfbc64c5ab5-goog

[PATCH v4 6/9] 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 | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 53da1d984883..511927e8b746 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -347,6 +347,14 @@ 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,
+	},
 };
 
 static const struct uvc_menu_info power_line_frequency_controls[] = {
@@ -735,6 +743,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,
+	},
 };
 
 /* ------------------------------------------------------------------------
-- 
2.29.2.684.gfbc64c5ab5-goog

[PATCH v4 7/9] media: uvcvideo: Use dev_ printk aliases

[Ricardo Ribalda]

Replace all the uses of printk() and uvc_printk() with its
equivalent dev_ alias macros.

They are more standard across the kernel tree and provide
more context about the error.

Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c   |  4 +-
 drivers/media/usb/uvc/uvc_driver.c | 80 ++++++++++++++++--------------
 drivers/media/usb/uvc/uvc_entity.c | 10 ++--
 drivers/media/usb/uvc/uvc_status.c | 13 ++---
 drivers/media/usb/uvc/uvc_video.c  | 51 ++++++++++---------
 drivers/media/usb/uvc/uvcvideo.h   | 25 ++++------
 6 files changed, 95 insertions(+), 88 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 511927e8b746..b78a52b6e193 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1327,8 +1327,8 @@ static void uvc_ctrl_status_event_work(struct work_struct *work)
 	w->urb->interval = dev->int_ep->desc.bInterval;
 	ret = usb_submit_urb(w->urb, GFP_KERNEL);
 	if (ret < 0)
-		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
-			   ret);
+		dev_err(&dev->udev->dev,
+			"Failed to resubmit status URB (%d).\n", ret);
 }
 
 bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 498de09da07e..4379916a6ac1 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -535,8 +535,8 @@ static int uvc_parse_format(struct uvc_device *dev,
 				sizeof(format->name));
 			format->fcc = fmtdesc->fcc;
 		} else {
-			uvc_printk(KERN_INFO, "Unknown video format %pUl\n",
-				&buffer[5]);
+			dev_info(&streaming->intf->dev,
+				 "Unknown video format %pUl\n", &buffer[5]);
 			snprintf(format->name, sizeof(format->name), "%pUl\n",
 				&buffer[5]);
 			format->fcc = 0;
@@ -1594,7 +1594,7 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
 	switch (UVC_ENTITY_TYPE(entity)) {
 	case UVC_VC_EXTENSION_UNIT:
 		if (uvc_trace_param & UVC_TRACE_PROBE)
-			printk(KERN_CONT " <- XU %d", entity->id);
+			pr_cont(" <- XU %d", entity->id);
 
 		if (entity->bNrInPins != 1) {
 			uvc_trace(UVC_TRACE_DESCR, "Extension unit %d has more "
@@ -1606,7 +1606,7 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
 
 	case UVC_VC_PROCESSING_UNIT:
 		if (uvc_trace_param & UVC_TRACE_PROBE)
-			printk(KERN_CONT " <- PU %d", entity->id);
+			pr_cont(" <- PU %d", entity->id);
 
 		if (chain->processing != NULL) {
 			uvc_trace(UVC_TRACE_DESCR, "Found multiple "
@@ -1619,7 +1619,7 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
 
 	case UVC_VC_SELECTOR_UNIT:
 		if (uvc_trace_param & UVC_TRACE_PROBE)
-			printk(KERN_CONT " <- SU %d", entity->id);
+			pr_cont(" <- SU %d", entity->id);
 
 		/* Single-input selector units are ignored. */
 		if (entity->bNrInPins == 1)
@@ -1638,7 +1638,7 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
 	case UVC_ITT_CAMERA:
 	case UVC_ITT_MEDIA_TRANSPORT_INPUT:
 		if (uvc_trace_param & UVC_TRACE_PROBE)
-			printk(KERN_CONT " <- IT %d\n", entity->id);
+			pr_cont(" <- IT %d\n", entity->id);
 
 		break;
 
@@ -1646,17 +1646,17 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
 	case UVC_OTT_DISPLAY:
 	case UVC_OTT_MEDIA_TRANSPORT_OUTPUT:
 		if (uvc_trace_param & UVC_TRACE_PROBE)
-			printk(KERN_CONT " OT %d", entity->id);
+			pr_cont(" OT %d", entity->id);
 
 		break;
 
 	case UVC_TT_STREAMING:
 		if (UVC_ENTITY_IS_ITERM(entity)) {
 			if (uvc_trace_param & UVC_TRACE_PROBE)
-				printk(KERN_CONT " <- IT %d\n", entity->id);
+				pr_cont(" <- IT %d\n", entity->id);
 		} else {
 			if (uvc_trace_param & UVC_TRACE_PROBE)
-				printk(KERN_CONT " OT %d", entity->id);
+				pr_cont(" OT %d", entity->id);
 		}
 
 		break;
@@ -1706,9 +1706,9 @@ static int uvc_scan_chain_forward(struct uvc_video_chain *chain,
 			list_add_tail(&forward->chain, &chain->entities);
 			if (uvc_trace_param & UVC_TRACE_PROBE) {
 				if (!found)
-					printk(KERN_CONT " (->");
+					pr_cont(" (->");
 
-				printk(KERN_CONT " XU %d", forward->id);
+				pr_cont(" XU %d", forward->id);
 				found = 1;
 			}
 			break;
@@ -1726,16 +1726,16 @@ static int uvc_scan_chain_forward(struct uvc_video_chain *chain,
 			list_add_tail(&forward->chain, &chain->entities);
 			if (uvc_trace_param & UVC_TRACE_PROBE) {
 				if (!found)
-					printk(KERN_CONT " (->");
+					pr_cont(" (->");
 
-				printk(KERN_CONT " OT %d", forward->id);
+				pr_cont(" OT %d", forward->id);
 				found = 1;
 			}
 			break;
 		}
 	}
 	if (found)
-		printk(KERN_CONT ")");
+		pr_cont(")");
 
 	return 0;
 }
@@ -1761,7 +1761,7 @@ static int uvc_scan_chain_backward(struct uvc_video_chain *chain,
 		}
 
 		if (uvc_trace_param & UVC_TRACE_PROBE)
-			printk(KERN_CONT " <- IT");
+			pr_cont(" <- IT");
 
 		chain->selector = entity;
 		for (i = 0; i < entity->bNrInPins; ++i) {
@@ -1782,14 +1782,14 @@ static int uvc_scan_chain_backward(struct uvc_video_chain *chain,
 			}
 
 			if (uvc_trace_param & UVC_TRACE_PROBE)
-				printk(KERN_CONT " %d", term->id);
+				pr_cont(" %d", term->id);
 
 			list_add_tail(&term->chain, &chain->entities);
 			uvc_scan_chain_forward(chain, term, entity);
 		}
 
 		if (uvc_trace_param & UVC_TRACE_PROBE)
-			printk(KERN_CONT "\n");
+			pr_cont("\n");
 
 		id = 0;
 		break;
@@ -2044,7 +2044,7 @@ static int uvc_scan_device(struct uvc_device *dev)
 		uvc_scan_fallback(dev);
 
 	if (list_empty(&dev->chains)) {
-		uvc_printk(KERN_INFO, "No valid video chain found.\n");
+		dev_info(&dev->udev->dev, "No valid video chain found.\n");
 		return -1;
 	}
 
@@ -2203,8 +2203,9 @@ int uvc_register_video_device(struct uvc_device *dev,
 
 	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
 	if (ret < 0) {
-		uvc_printk(KERN_ERR, "Failed to register %s device (%d).\n",
-			   v4l2_type_names[type], ret);
+		dev_err(&stream->intf->dev,
+			"Failed to register %s device (%d).\n",
+			v4l2_type_names[type], ret);
 		return ret;
 	}
 
@@ -2220,8 +2221,8 @@ static int uvc_register_video(struct uvc_device *dev,
 	/* Initialize the streaming interface with default parameters. */
 	ret = uvc_video_init(stream);
 	if (ret < 0) {
-		uvc_printk(KERN_ERR, "Failed to initialize the device (%d).\n",
-			   ret);
+		dev_err(&stream->intf->dev,
+			"Failed to initialize the device (%d).\n", ret);
 		return ret;
 	}
 
@@ -2255,8 +2256,9 @@ static int uvc_register_terms(struct uvc_device *dev,
 
 		stream = uvc_stream_by_id(dev, term->id);
 		if (stream == NULL) {
-			uvc_printk(KERN_INFO, "No streaming interface found "
-				   "for terminal %u.", term->id);
+			dev_info(&dev->udev->dev,
+				 "No streaming interface found for terminal %u.",
+				 term->id);
 			continue;
 		}
 
@@ -2289,8 +2291,8 @@ static int uvc_register_chains(struct uvc_device *dev)
 #ifdef CONFIG_MEDIA_CONTROLLER
 		ret = uvc_mc_register_entities(chain);
 		if (ret < 0)
-			uvc_printk(KERN_INFO,
-				   "Failed to register entities (%d).\n", ret);
+			dev_info(&dev->udev->dev,
+				 "Failed to register entities (%d).\n", ret);
 #endif
 	}
 
@@ -2393,17 +2395,19 @@ static int uvc_probe(struct usb_interface *intf,
 		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>",
-		le16_to_cpu(udev->descriptor.idVendor),
-		le16_to_cpu(udev->descriptor.idProduct));
+	dev_info(&dev->udev->dev,
+		 "Found UVC %u.%02x device %s (%04x:%04x)\n",
+		 dev->uvc_version >> 8, dev->uvc_version & 0xff,
+		 udev->product ? udev->product : "<unnamed>",
+		 le16_to_cpu(udev->descriptor.idVendor),
+		 le16_to_cpu(udev->descriptor.idProduct));
 
 	if (dev->quirks != dev->info->quirks) {
-		uvc_printk(KERN_INFO, "Forcing device quirks to 0x%x by module "
-			"parameter for testing purpose.\n", dev->quirks);
-		uvc_printk(KERN_INFO, "Please report required quirks to the "
-			"linux-uvc-devel mailing list.\n");
+		dev_info(&dev->udev->dev,
+			 "Forcing device quirks to 0x%x by module parameter for testing purpose.\n",
+			 dev->quirks);
+		dev_info(&dev->udev->dev,
+			 "Please report required quirks to the linux-uvc-devel mailing list.\n");
 	}
 
 	/* Register the V4L2 device. */
@@ -2432,9 +2436,9 @@ static int uvc_probe(struct usb_interface *intf,
 
 	/* Initialize the interrupt URB. */
 	if ((ret = uvc_status_init(dev)) < 0) {
-		uvc_printk(KERN_INFO, "Unable to initialize the status "
-			"endpoint (%d), status interrupt will not be "
-			"supported.\n", ret);
+		dev_info(&dev->udev->dev,
+			 "Unable to initialize the status endpoint (%d), status interrupt will not be supported.\n",
+			 ret);
 	}
 
 	uvc_trace(UVC_TRACE_PROBE, "UVC device initialized.\n");
diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c
index ca3a9c2eec27..a13bee5aee56 100644
--- a/drivers/media/usb/uvc/uvc_entity.c
+++ b/drivers/media/usb/uvc/uvc_entity.c
@@ -139,8 +139,9 @@ int uvc_mc_register_entities(struct uvc_video_chain *chain)
 	list_for_each_entry(entity, &chain->entities, chain) {
 		ret = uvc_mc_init_entity(chain, entity);
 		if (ret < 0) {
-			uvc_printk(KERN_INFO, "Failed to initialize entity for "
-				   "entity %u\n", entity->id);
+			dev_info(&chain->dev->udev->dev,
+				 "Failed to initialize entity for entity %u\n",
+				 entity->id);
 			return ret;
 		}
 	}
@@ -148,8 +149,9 @@ int uvc_mc_register_entities(struct uvc_video_chain *chain)
 	list_for_each_entry(entity, &chain->entities, chain) {
 		ret = uvc_mc_create_links(chain, entity);
 		if (ret < 0) {
-			uvc_printk(KERN_INFO, "Failed to create links for "
-				   "entity %u\n", entity->id);
+			dev_info(&chain->dev->udev->dev,
+				 "Failed to create links for entity %u\n",
+				 entity->id);
 			return ret;
 		}
 	}
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index 2bdb0ff203f8..36fa196a9258 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -208,8 +208,9 @@ static void uvc_status_complete(struct urb *urb)
 		return;
 
 	default:
-		uvc_printk(KERN_WARNING, "Non-zero status (%d) in status "
-			"completion handler.\n", urb->status);
+		dev_warn(&dev->udev->dev,
+			 "Non-zero status (%d) in status completion handler.\n",
+			 urb->status);
 		return;
 	}
 
@@ -243,10 +244,10 @@ static void uvc_status_complete(struct urb *urb)
 
 	/* Resubmit the URB. */
 	urb->interval = dev->int_ep->desc.bInterval;
-	if ((ret = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
-		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
-			ret);
-	}
+	ret = usb_submit_urb(urb, GFP_ATOMIC);
+	if (ret < 0)
+		dev_err(&dev->udev->dev,
+			"Failed to resubmit status URB (%d).\n", ret);
 }
 
 int uvc_status_init(struct uvc_device *dev)
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index a6a441d92b94..a4a0f3556986 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -76,9 +76,9 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
 	if (likely(ret == size))
 		return 0;
 
-	uvc_printk(KERN_ERR,
-		   "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
-		   uvc_query_name(query), cs, unit, ret, size);
+	dev_err(&dev->udev->dev,
+		"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
+		uvc_query_name(query), cs, unit, ret, size);
 
 	if (ret != -EPIPE)
 		return ret;
@@ -254,9 +254,9 @@ static int uvc_get_video_ctrl(struct uvc_streaming *stream,
 		ret = -EIO;
 		goto out;
 	} else if (ret != size) {
-		uvc_printk(KERN_ERR, "Failed to query (%u) UVC %s control : "
-			"%d (exp. %u).\n", query, probe ? "probe" : "commit",
-			ret, size);
+		dev_err(&stream->intf->dev,
+			"Failed to query (%u) UVC %s control : %d (exp. %u).\n",
+			query, probe ? "probe" : "commit", ret, size);
 		ret = -EIO;
 		goto out;
 	}
@@ -334,9 +334,9 @@ static int uvc_set_video_ctrl(struct uvc_streaming *stream,
 		probe ? UVC_VS_PROBE_CONTROL : UVC_VS_COMMIT_CONTROL, data,
 		size, uvc_timeout_param);
 	if (ret != size) {
-		uvc_printk(KERN_ERR, "Failed to set UVC %s control : "
-			"%d (exp. %u).\n", probe ? "probe" : "commit",
-			ret, size);
+		dev_err(&stream->intf->dev,
+			"Failed to set UVC %s control : %d (exp. %u).\n",
+			probe ? "probe" : "commit", ret, size);
 		ret = -EIO;
 	}
 
@@ -1119,9 +1119,10 @@ static void uvc_video_copy_data_work(struct work_struct *work)
 	}
 
 	ret = usb_submit_urb(uvc_urb->urb, GFP_KERNEL);
-	if (ret < 0)
-		uvc_printk(KERN_ERR, "Failed to resubmit video URB (%d).\n",
-			   ret);
+	if (ret < 0) {
+		dev_err(&uvc_urb->stream->intf->dev,
+			"Failed to resubmit video URB (%d).\n", ret);
+	}
 }
 
 static void uvc_video_decode_data(struct uvc_urb *uvc_urb,
@@ -1507,8 +1508,9 @@ static void uvc_video_complete(struct urb *urb)
 		break;
 
 	default:
-		uvc_printk(KERN_WARNING, "Non-zero status (%d) in video "
-			"completion handler.\n", urb->status);
+		dev_warn(&stream->intf->dev,
+			 "Non-zero status (%d) in video completion handler.\n",
+			 urb->status);
 		fallthrough;
 	case -ENOENT:		/* usb_poison_urb() called. */
 		if (stream->frozen)
@@ -1545,9 +1547,8 @@ static void uvc_video_complete(struct urb *urb)
 	if (!uvc_urb->async_operations) {
 		ret = usb_submit_urb(uvc_urb->urb, GFP_ATOMIC);
 		if (ret < 0)
-			uvc_printk(KERN_ERR,
-				   "Failed to resubmit video URB (%d).\n",
-				   ret);
+			dev_err(&stream->intf->dev,
+				"Failed to resubmit video URB (%d).\n", ret);
 		return;
 	}
 
@@ -1893,8 +1894,9 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
 	for_each_uvc_urb(uvc_urb, stream) {
 		ret = usb_submit_urb(uvc_urb->urb, gfp_flags);
 		if (ret < 0) {
-			uvc_printk(KERN_ERR, "Failed to submit URB %u (%d).\n",
-				   uvc_urb_index(uvc_urb), ret);
+			dev_err(&stream->intf->dev,
+				"Failed to submit URB %u (%d).\n",
+				uvc_urb_index(uvc_urb), ret);
 			uvc_video_stop_transfer(stream, 1);
 			return ret;
 		}
@@ -1989,7 +1991,8 @@ int uvc_video_init(struct uvc_streaming *stream)
 	int ret;
 
 	if (stream->nformats == 0) {
-		uvc_printk(KERN_INFO, "No supported video formats found.\n");
+		dev_info(&stream->intf->dev,
+			 "No supported video formats found.\n");
 		return -EINVAL;
 	}
 
@@ -2029,8 +2032,8 @@ int uvc_video_init(struct uvc_streaming *stream)
 	}
 
 	if (format->nframes == 0) {
-		uvc_printk(KERN_INFO, "No frame descriptor found for the "
-			"default format.\n");
+		dev_info(&stream->intf->dev,
+			 "No frame descriptor found for the default format.\n");
 		return -EINVAL;
 	}
 
@@ -2064,8 +2067,8 @@ int uvc_video_init(struct uvc_streaming *stream)
 		if (stream->intf->num_altsetting == 1)
 			stream->decode = uvc_video_encode_bulk;
 		else {
-			uvc_printk(KERN_INFO, "Isochronous endpoints are not "
-				"supported for video output devices.\n");
+			dev_info(&stream->intf->dev,
+				 "Isochronous endpoints are not supported for video output devices.\n");
 			return -EINVAL;
 		}
 	}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index f87d14fb3f56..d8e2f27bf576 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -742,20 +742,17 @@ extern unsigned int uvc_trace_param;
 extern unsigned int uvc_timeout_param;
 extern unsigned int uvc_hw_timestamps_param;
 
-#define uvc_trace(flag, msg...) \
-	do { \
-		if (uvc_trace_param & flag) \
-			printk(KERN_DEBUG "uvcvideo: " msg); \
-	} while (0)
-
-#define uvc_warn_once(dev, warn, msg...) \
-	do { \
-		if (!test_and_set_bit(warn, &dev->warnings)) \
-			printk(KERN_INFO "uvcvideo: " msg); \
-	} while (0)
-
-#define uvc_printk(level, msg...) \
-	printk(level "uvcvideo: " msg)
+#define uvc_trace(flag, fmt, ...)					\
+do {									\
+	if (uvc_trace_param & flag)					\
+		printk(KERN_DEBUG "uvcvideo: " fmt, ##__VA_ARGS__);	\
+} while (0)
+
+#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)
 
 /* --------------------------------------------------------------------------
  * Internal functions.
-- 
2.29.2.684.gfbc64c5ab5-goog

[PATCH v4 8/9] media: uvcvideo: New macro uvc_trace_cont

[Ricardo Ribalda]

Remove all the duplicated code around pr_cont with a new macro.

Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 57 +++++++++++-------------------
 drivers/media/usb/uvc/uvcvideo.h   |  6 ++++
 2 files changed, 27 insertions(+), 36 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 4379916a6ac1..e49491250e87 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1593,8 +1593,7 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
 {
 	switch (UVC_ENTITY_TYPE(entity)) {
 	case UVC_VC_EXTENSION_UNIT:
-		if (uvc_trace_param & UVC_TRACE_PROBE)
-			pr_cont(" <- XU %d", entity->id);
+		uvc_trace_cont(UVC_TRACE_PROBE, " <- XU %d", entity->id);
 
 		if (entity->bNrInPins != 1) {
 			uvc_trace(UVC_TRACE_DESCR, "Extension unit %d has more "
@@ -1605,8 +1604,7 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
 		break;
 
 	case UVC_VC_PROCESSING_UNIT:
-		if (uvc_trace_param & UVC_TRACE_PROBE)
-			pr_cont(" <- PU %d", entity->id);
+		uvc_trace_cont(UVC_TRACE_PROBE, " <- PU %d", entity->id);
 
 		if (chain->processing != NULL) {
 			uvc_trace(UVC_TRACE_DESCR, "Found multiple "
@@ -1618,8 +1616,7 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
 		break;
 
 	case UVC_VC_SELECTOR_UNIT:
-		if (uvc_trace_param & UVC_TRACE_PROBE)
-			pr_cont(" <- SU %d", entity->id);
+		uvc_trace_cont(UVC_TRACE_PROBE, " <- SU %d", entity->id);
 
 		/* Single-input selector units are ignored. */
 		if (entity->bNrInPins == 1)
@@ -1637,27 +1634,22 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
 	case UVC_ITT_VENDOR_SPECIFIC:
 	case UVC_ITT_CAMERA:
 	case UVC_ITT_MEDIA_TRANSPORT_INPUT:
-		if (uvc_trace_param & UVC_TRACE_PROBE)
-			pr_cont(" <- IT %d\n", entity->id);
+		uvc_trace_cont(UVC_TRACE_PROBE, " <- IT %d\n", entity->id);
 
 		break;
 
 	case UVC_OTT_VENDOR_SPECIFIC:
 	case UVC_OTT_DISPLAY:
 	case UVC_OTT_MEDIA_TRANSPORT_OUTPUT:
-		if (uvc_trace_param & UVC_TRACE_PROBE)
-			pr_cont(" OT %d", entity->id);
+		uvc_trace_cont(UVC_TRACE_PROBE, " OT %d", entity->id);
 
 		break;
 
 	case UVC_TT_STREAMING:
-		if (UVC_ENTITY_IS_ITERM(entity)) {
-			if (uvc_trace_param & UVC_TRACE_PROBE)
-				pr_cont(" <- IT %d\n", entity->id);
-		} else {
-			if (uvc_trace_param & UVC_TRACE_PROBE)
-				pr_cont(" OT %d", entity->id);
-		}
+		if (UVC_ENTITY_IS_ITERM(entity))
+			uvc_trace_cont(UVC_TRACE_PROBE, " <- IT %d\n", entity->id);
+		else
+			uvc_trace_cont(UVC_TRACE_PROBE, " OT %d", entity->id);
 
 		break;
 
@@ -1704,13 +1696,11 @@ static int uvc_scan_chain_forward(struct uvc_video_chain *chain,
 			}
 
 			list_add_tail(&forward->chain, &chain->entities);
-			if (uvc_trace_param & UVC_TRACE_PROBE) {
-				if (!found)
-					pr_cont(" (->");
+			if (!found)
+				uvc_trace_cont(UVC_TRACE_PROBE, " (->");
 
-				pr_cont(" XU %d", forward->id);
-				found = 1;
-			}
+			uvc_trace_cont(UVC_TRACE_PROBE, " XU %d", forward->id);
+			found = 1;
 			break;
 
 		case UVC_OTT_VENDOR_SPECIFIC:
@@ -1724,18 +1714,16 @@ static int uvc_scan_chain_forward(struct uvc_video_chain *chain,
 			}
 
 			list_add_tail(&forward->chain, &chain->entities);
-			if (uvc_trace_param & UVC_TRACE_PROBE) {
-				if (!found)
-					pr_cont(" (->");
+			if (!found)
+				uvc_trace_cont(UVC_TRACE_PROBE, " (->");
 
-				pr_cont(" OT %d", forward->id);
-				found = 1;
-			}
+			uvc_trace_cont(UVC_TRACE_PROBE, " OT %d", forward->id);
+			found = 1;
 			break;
 		}
 	}
 	if (found)
-		pr_cont(")");
+		uvc_trace_cont(UVC_TRACE_PROBE, ")");
 
 	return 0;
 }
@@ -1760,8 +1748,7 @@ static int uvc_scan_chain_backward(struct uvc_video_chain *chain,
 			break;
 		}
 
-		if (uvc_trace_param & UVC_TRACE_PROBE)
-			pr_cont(" <- IT");
+		uvc_trace_cont(UVC_TRACE_PROBE, " <- IT");
 
 		chain->selector = entity;
 		for (i = 0; i < entity->bNrInPins; ++i) {
@@ -1781,15 +1768,13 @@ static int uvc_scan_chain_backward(struct uvc_video_chain *chain,
 				return -EINVAL;
 			}
 
-			if (uvc_trace_param & UVC_TRACE_PROBE)
-				pr_cont(" %d", term->id);
+			uvc_trace_cont(UVC_TRACE_PROBE, " %d", term->id);
 
 			list_add_tail(&term->chain, &chain->entities);
 			uvc_scan_chain_forward(chain, term, entity);
 		}
 
-		if (uvc_trace_param & UVC_TRACE_PROBE)
-			pr_cont("\n");
+		uvc_trace_cont(UVC_TRACE_PROBE, "\n");
 
 		id = 0;
 		break;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index d8e2f27bf576..2b5ba4b02d3a 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -748,6 +748,12 @@ do {									\
 		printk(KERN_DEBUG "uvcvideo: " fmt, ##__VA_ARGS__);	\
 } while (0)
 
+#define uvc_trace_cont(flag, fmt, ...)					\
+do {									\
+	if (uvc_trace_param & flag)					\
+		pr_cont(fmt, ##__VA_ARGS__);				\
+} while (0)
+
 #define uvc_warn_once(_dev, warn, fmt, ...)				\
 do {									\
 	if (!test_and_set_bit(warn, &(_dev)->warnings))			\
-- 
2.29.2.684.gfbc64c5ab5-goog

[PATCH v4 9/9] media: uvcvideo: Implement UVC_QUIRK_PRIVACY_DURING_STREAM

[Ricardo Ribalda]

Some devices, can only read the privacy_pin if the device is
streaming.

This patch implement a quirk for such devices, in order to avoid invalid
reads and/or spurious events.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 97 ++++++++++++++++++++++++++----
 drivers/media/usb/uvc/uvc_queue.c  |  3 +
 drivers/media/usb/uvc/uvcvideo.h   |  6 ++
 3 files changed, 94 insertions(+), 12 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index e49491250e87..61313019e226 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/dmi.h>
 #include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
@@ -1471,13 +1472,39 @@ static int uvc_parse_control(struct uvc_device *dev)
 	return 0;
 }
 
+static bool uvc_ext_gpio_is_streaming(struct uvc_device *dev)
+{
+	struct uvc_streaming *streaming;
+
+	list_for_each_entry(streaming, &dev->streams, list) {
+		if (uvc_queue_streaming(&streaming->queue))
+			return true;
+	}
+
+	return false;
+}
+
+/* Update the cached value and return true if it has changed */
+static bool uvc_gpio_update_value(struct uvc_entity *unit, u8 *new_val)
+{
+	*new_val = gpiod_get_value(unit->gpio.gpio_privacy);
+
+	return atomic_xchg(&unit->gpio.gpio_privacy_value, *new_val) !=
+								      *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;
 
-	*(uint8_t *)data = gpiod_get_value(entity->gpio.gpio_privacy);
+	if ((dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) &&
+	    !uvc_ext_gpio_is_streaming(dev))
+		return -EBUSY;
+
+	uvc_gpio_update_value(entity, (uint8_t *)data);
+
 	return 0;
 }
 
@@ -1491,26 +1518,69 @@ static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
 	return 0;
 }
 
-static irqreturn_t uvc_privacy_gpio_irq(int irq, void *data)
+static struct uvc_entity *uvc_find_ext_gpio_unit(struct uvc_device *dev)
 {
-	struct uvc_device *dev = data;
-	struct uvc_video_chain *chain;
 	struct uvc_entity *unit;
-	u8 value;
 
-	/* GPIO entities are always on the first chain */
-	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
 	list_for_each_entry(unit, &dev->entities, list) {
-		if (UVC_ENTITY_TYPE(unit) != UVC_EXT_GPIO_UNIT)
-			continue;
-		value = gpiod_get_value(unit->gpio.gpio_privacy);
-		uvc_ctrl_status_event(NULL, chain, unit->controls, &value);
-		return IRQ_HANDLED;
+		if (UVC_ENTITY_TYPE(unit) == UVC_EXT_GPIO_UNIT)
+			return unit;
 	}
 
+	return unit;
+}
+
+void uvc_privacy_gpio_event(struct uvc_device *dev)
+{
+	struct uvc_entity *unit;
+	struct uvc_video_chain *chain;
+	u8 new_value;
+
+	unit = uvc_find_ext_gpio_unit(dev);
+	if (WARN_ONCE(!unit, "Unable to find entity ext_gpio_unit"))
+		return;
+
+	if (!uvc_gpio_update_value(unit, &new_value))
+		return;
+
+	/* GPIO entities are always on the first chain */
+	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
+	uvc_ctrl_status_event(NULL, chain, unit->controls, &new_value);
+}
+
+static irqreturn_t uvc_privacy_gpio_irq(int irq, void *data)
+{
+	struct uvc_device *dev = data;
+
+	/* Ignore privacy events during streamoff */
+	if (dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM)
+		if (!uvc_ext_gpio_is_streaming(dev))
+			return IRQ_HANDLED;
+
+	uvc_privacy_gpio_event(dev);
+
 	return IRQ_HANDLED;
 }
 
+static const struct dmi_system_id privacy_valid_during_streamon[] = {
+	{
+		.ident = "HP Elite c1030 Chromebook",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Jinlon"),
+		},
+	},
+	{
+		.ident = "HP Pro c640 Chromebook",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Dratini"),
+		},
+	},
+	{ } /* terminate list */
+};
+
+
 static int uvc_parse_gpio(struct uvc_device *dev)
 {
 	struct uvc_entity *unit;
@@ -1545,6 +1615,9 @@ static int uvc_parse_gpio(struct uvc_device *dev)
 	if (irq == -EPROBE_DEFER)
 		return -EPROBE_DEFER;
 
+	if (dmi_check_system(privacy_valid_during_streamon))
+		dev->quirks |= UVC_QUIRK_PRIVACY_DURING_STREAM;
+
 	if (irq < 0)
 		return 0;
 
diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index cd60c6c1749e..e800d491303f 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -337,9 +337,12 @@ int uvc_dequeue_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf,
 int uvc_queue_streamon(struct uvc_video_queue *queue, enum v4l2_buf_type type)
 {
 	int ret;
+	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
 
 	mutex_lock(&queue->mutex);
 	ret = vb2_streamon(&queue->queue, type);
+	if (stream->dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM)
+		uvc_privacy_gpio_event(stream->dev);
 	mutex_unlock(&queue->mutex);
 
 	return ret;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 2b5ba4b02d3a..2a95b3ed3ea8 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/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/poll.h>
@@ -209,6 +210,7 @@
 #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
 #define UVC_QUIRK_FORCE_Y8		0x00000800
 #define UVC_QUIRK_FORCE_BPP		0x00001000
+#define UVC_QUIRK_PRIVACY_DURING_STREAM	0x00002000
 
 /* Format flags */
 #define UVC_FMT_FLAG_COMPRESSED		0x00000001
@@ -359,6 +361,7 @@ struct uvc_entity {
 			u8  bControlSize;
 			u8  *bmControls;
 			struct gpio_desc *gpio_privacy;
+			atomic_t  gpio_privacy_value;
 		} gpio;
 	};
 
@@ -815,6 +818,9 @@ extern const struct v4l2_file_operations uvc_fops;
 int uvc_mc_register_entities(struct uvc_video_chain *chain);
 void uvc_mc_cleanup_entity(struct uvc_entity *entity);
 
+/* Privacy gpio */
+void uvc_privacy_gpio_event(struct uvc_device *dev);
+
 /* Video */
 int uvc_video_init(struct uvc_streaming *stream);
 int uvc_video_suspend(struct uvc_streaming *stream);
-- 
2.29.2.684.gfbc64c5ab5-goog

Re: [PATCH v4 1/9] media: uvcvideo: Move guid to entity

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Tue, Dec 15, 2020 at 04:44:31PM +0100, Ricardo Ribalda wrote:
> Instead of having multiple copies of the entity guid on the code, move
> it to the entity structure.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c   | 30 ++++--------------------------
>  drivers/media/usb/uvc/uvc_driver.c | 26 ++++++++++++++++++++++++--
>  drivers/media/usb/uvc/uvcvideo.h   |  2 +-
>  3 files changed, 29 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 011e69427b7c..9f6174a10e73 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -826,31 +826,10 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping,
>   * Terminal and unit management
>   */
>  
> -static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
> -static const u8 uvc_camera_guid[16] = UVC_GUID_UVC_CAMERA;
> -static const u8 uvc_media_transport_input_guid[16] =
> -	UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
> -
>  static int uvc_entity_match_guid(const struct uvc_entity *entity,
> -	const u8 guid[16])
> +				 const u8 guid[16])
>  {
> -	switch (UVC_ENTITY_TYPE(entity)) {
> -	case UVC_ITT_CAMERA:
> -		return memcmp(uvc_camera_guid, guid, 16) == 0;
> -
> -	case UVC_ITT_MEDIA_TRANSPORT_INPUT:
> -		return memcmp(uvc_media_transport_input_guid, guid, 16) == 0;
> -
> -	case UVC_VC_PROCESSING_UNIT:
> -		return memcmp(uvc_processing_guid, guid, 16) == 0;
> -
> -	case UVC_VC_EXTENSION_UNIT:
> -		return memcmp(entity->extension.guidExtensionCode,
> -			      guid, 16) == 0;
> -
> -	default:
> -		return 0;
> -	}
> +	return memcmp(entity->guid, guid, sizeof(entity->guid)) == 0;
>  }
>  
>  /* ------------------------------------------------------------------------
> @@ -1776,8 +1755,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
>  	if (data == NULL)
>  		return -ENOMEM;
>  
> -	memcpy(info->entity, ctrl->entity->extension.guidExtensionCode,
> -	       sizeof(info->entity));
> +	memcpy(info->entity, ctrl->entity->guid, sizeof(info->entity));
>  	info->index = ctrl->index;
>  	info->selector = ctrl->index + 1;
>  
> @@ -1883,7 +1861,7 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
>  
>  	if (!found) {
>  		uvc_trace(UVC_TRACE_CONTROL, "Control %pUl/%u not found.\n",
> -			entity->extension.guidExtensionCode, xqry->selector);
> +			entity->guid, xqry->selector);
>  		return -ENOENT;
>  	}
>  
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index ddb9eaa11be7..4cdd65d252d9 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1019,6 +1019,11 @@ static int uvc_parse_streaming(struct uvc_device *dev,
>  	return ret;
>  }
>  
> +static const u8 uvc_camera_guid[16] = UVC_GUID_UVC_CAMERA;
> +static const u8 uvc_media_transport_input_guid[16] =
> +	UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
> +static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
> +
>  static struct uvc_entity *uvc_alloc_entity(u16 type, u8 id,
>  		unsigned int num_pads, unsigned int extra_size)
>  {
> @@ -1038,6 +1043,23 @@ static struct uvc_entity *uvc_alloc_entity(u16 type, u8 id,
>  	entity->id = id;
>  	entity->type = type;
>  
> +

Nearly there, just one blank line to remove :-) I'll fix this when
applying.

> +	/*
> +	 * Set the GUID for standard entity types. For extension units, the GUID
> +	 * is initialized by the caller.
> +	 */
> +	switch (type) {
> +	case UVC_ITT_CAMERA:
> +		memcpy(entity->guid, uvc_camera_guid, 16);
> +		break;
> +	case UVC_ITT_MEDIA_TRANSPORT_INPUT:
> +		memcpy(entity->guid, uvc_media_transport_input_guid, 16);
> +		break;
> +	case UVC_VC_PROCESSING_UNIT:
> +		memcpy(entity->guid, uvc_processing_guid, 16);
> +		break;
> +	}
> +
>  	entity->num_links = 0;
>  	entity->num_pads = num_pads;
>  	entity->pads = ((void *)(entity + 1)) + extra_size;
> @@ -1109,7 +1131,7 @@ static int uvc_parse_vendor_control(struct uvc_device *dev,
>  		if (unit == NULL)
>  			return -ENOMEM;
>  
> -		memcpy(unit->extension.guidExtensionCode, &buffer[4], 16);
> +		memcpy(unit->guid, &buffer[4], 16);
>  		unit->extension.bNumControls = buffer[20];
>  		memcpy(unit->baSourceID, &buffer[22], p);
>  		unit->extension.bControlSize = buffer[22+p];
> @@ -1368,7 +1390,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
>  		if (unit == NULL)
>  			return -ENOMEM;
>  
> -		memcpy(unit->extension.guidExtensionCode, &buffer[4], 16);
> +		memcpy(unit->guid, &buffer[4], 16);
>  		unit->extension.bNumControls = buffer[20];
>  		memcpy(unit->baSourceID, &buffer[22], p);
>  		unit->extension.bControlSize = buffer[22+p];
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index a3dfacf069c4..df7bf2d104a3 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -304,6 +304,7 @@ struct uvc_entity {
>  	u8 id;
>  	u16 type;
>  	char name[64];
> +	u8 guid[16];
>  
>  	/* Media controller-related fields. */
>  	struct video_device *vdev;
> @@ -342,7 +343,6 @@ struct uvc_entity {
>  		} selector;
>  
>  		struct {
> -			u8  guidExtensionCode[16];
>  			u8  bNumControls;
>  			u8  bControlSize;
>  			u8  *bmControls;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/9] media: uvcvideo: Allow entities with no pads

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Tue, Dec 15, 2020 at 04:44:33PM +0100, Ricardo Ribalda wrote:
> Avoid an underflow while calculating the number of inputs for entities
> with zero pads.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 9f4451a2e0a6..534629ecd60d 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1033,7 +1033,11 @@ static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
>  	unsigned int i;
>  
>  	extra_size = roundup(extra_size, sizeof(*entity->pads));
> -	num_inputs = (type & UVC_TERM_OUTPUT) ? num_pads : num_pads - 1;
> +	if (num_pads)
> +		num_inputs = (type & UVC_TERM_OUTPUT) ? num_pads :
> +								num_pads - 1;

This is a weird indentation. How about the following ?

		num_inputs = type & UVC_TERM_OUTPUT ? num_pads : num_pads - 1;

I can fix this when applying.

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

> +	else
> +		num_inputs = 0;
>  	size = sizeof(*entity) + extra_size + sizeof(*entity->pads) * num_pads
>  	     + num_inputs;
>  	entity = kzalloc(size, GFP_KERNEL);
> @@ -1066,7 +1070,7 @@ static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
>  
>  	for (i = 0; i < num_inputs; ++i)
>  		entity->pads[i].flags = MEDIA_PAD_FL_SINK;
> -	if (!UVC_ENTITY_IS_OTERM(entity))
> +	if (!UVC_ENTITY_IS_OTERM(entity) && num_pads)
>  		entity->pads[num_pads-1].flags = MEDIA_PAD_FL_SOURCE;
>  
>  	entity->bNrInPins = num_inputs;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 4/9] media: uvcvideo: Entity defined get_info and get_cur

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

Maybe s/Entity defined/Allow entity-defined/ in the subject line ?

On Tue, Dec 15, 2020 at 04:44:34PM +0100, Ricardo Ribalda wrote:
> Allows controls to get their properties and 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 | 22 ++++++++++++++++++----
>  drivers/media/usb/uvc/uvcvideo.h |  5 +++++
>  2 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 9f6174a10e73..531816762892 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -980,10 +980,20 @@ 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,
> +		if (ctrl->entity->get_cur) {
> +			ret = ctrl->entity->get_cur(chain->dev,
> +				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);

We could possibly set entity->get_cur by default to a function wrapping
uvc_query_ctrl() to avoid the condition here. This isn't mandatory for
now though, but I'll toy with it on top of the series to avoid storing
function pointers in a non-const structure.

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

> +		}
>  		if (ret < 0)
>  			return ret;
>  
> @@ -1687,8 +1697,12 @@ 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->entity->get_info)
> +		ret = ctrl->entity->get_info(dev, ctrl->entity,
> +					     ctrl->info.selector, data);
> +	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 00f985001c1d..ae464ba83f4f 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -353,6 +353,11 @@ struct uvc_entity {
>  	u8 bNrInPins;
>  	u8 *baSourceID;
>  
> +	int (*get_info)(struct uvc_device *dev, struct uvc_entity *entity,
> +			u8 cs, u8 *caps);
> +	int (*get_cur)(struct uvc_device *dev, struct uvc_entity *entity,
> +		       u8 cs, void *data, u16 size);
> +
>  	unsigned int ncontrols;
>  	struct uvc_control *controls;
>  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 5/9] media: uvcvideo: Implement UVC_EXT_GPIO_UNIT

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Tue, Dec 15, 2020 at 04:44:35PM +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.
> 
> Since the uvc device is not aware of this pin (and it should't), we need

s/should't/shouldn't/

But I don't agree, if this is part of the camera, it should be
implemented in the camera firmware. I understand that it's not possible
(or desirable) with the hardware architecture you're dealing with
though. How about

"In some systems, the GPIO is connected to main SoC instead of the
camera controller, with the connected 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:"

> 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:
> 
>   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   |   6 ++
>  drivers/media/usb/uvc/uvc_driver.c | 106 +++++++++++++++++++++++++++++
>  drivers/media/usb/uvc/uvcvideo.h   |  12 ++++
>  3 files changed, 124 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 531816762892..53da1d984883 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1302,6 +1302,9 @@ static void uvc_ctrl_status_event_work(struct work_struct *work)
>  
>  	mutex_unlock(&chain->ctrl_mutex);
>  
> +	if (!w->urb)
> +		return;

A small comment to explain why would be useful.

> +
>  	/* Resubmit the URB. */
>  	w->urb->interval = dev->int_ep->desc.bInterval;
>  	ret = usb_submit_urb(w->urb, GFP_KERNEL);
> @@ -2286,6 +2289,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 534629ecd60d..498de09da07e 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;
> @@ -1053,6 +1055,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;
> @@ -1466,6 +1471,93 @@ static int uvc_parse_control(struct uvc_device *dev)
>  	return 0;
>  }

Can we add

/* -----------------------------------------------------------------------------
 * Privacy GPIO
 */

here ?

> +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;
> +
> +	*(uint8_t *)data = gpiod_get_value(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_privacy_gpio_irq(int irq, void *data)
> +{
> +	struct uvc_device *dev = data;
> +	struct uvc_video_chain *chain;
> +	struct uvc_entity *unit;
> +	u8 value;
> +
> +	/* GPIO entities are always on the first chain */


s/chain/chain./

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

Can we iterate over entities in the chain instead ?

I'd be actually tempted to pass the pointer to the entity as data. We
would however need to add a chain pointer to uvc_entity. Not sure if
it's worth it, up to you.

> +		if (UVC_ENTITY_TYPE(unit) != UVC_EXT_GPIO_UNIT)
> +			continue;
> +		value = gpiod_get_value(unit->gpio.gpio_privacy);

Could this sleep ? Should we use a threaded IRQ ?

> +		uvc_ctrl_status_event(NULL, chain, unit->controls, &value);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int uvc_parse_gpio(struct uvc_device *dev)

How about making the naming scheme consistent, by using a uvc_gpio_
prefix for all four functions ?

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

This could be folded in

	if (IS_ERR_OR_NULL(gpio_privacy))
		return PTR_ERR_OR_ZERO(gpio_privacy);

Up to you.

> +
> +	unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> +	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");

Other unit names don't contain "Unit", should this be just "GPIO" ?

> +
> +	list_add_tail(&unit->list, &dev->entities);
> +
> +	irq = gpiod_to_irq(gpio_privacy);
> +	if (irq == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;

Can this happen in practice ?

> +
> +	if (irq < 0)
> +		return 0;

So this will succeed, with the GPIO unit created, but it won't be
operational. Is this desired ? Shouldn't we at least print a warning ?

I also wonder if we should create the GPIO unit in this cases. Wouldn't
it be more confusing for userspace to see the privacy control being
exposed, but without it being functional ? Maybe the call to
gpiod_to_irq should be moved to before uvc_alloc_entity() ?

> +
> +	ret = devm_request_irq(&dev->udev->dev, irq, uvc_privacy_gpio_irq,
> +			       IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> +			       "uvc_privacy_gpio", dev);
> +	if (ret < 0)
> +		dev_warn(&dev->udev->dev,
> +		       "Unable to request uvc_privacy_gpio irq. Continuing\n");

s/uvc_privacy_gpio irq/privacy GPIO IRQ/ ?

There's also a race condition, as soon as the IRQ is requested, it could
fire, and the driver isn't fully initialized. Walking the chain in the
IRQ handler while entities are added to the chain could lead to a crash.
I see two options, either adding a flag to tell that initialization is
complete and returning from the interrupt handler when the flag isn't
set, or moving IRQ registration to a separate function, called later
during the initialization. I think I have a preference for the latter.

> +
> +	return 0;
> +}
> +
>  /* ------------------------------------------------------------------------
>   * UVC device scan
>   */
> @@ -1917,6 +2009,7 @@ static int uvc_scan_device(struct uvc_device *dev)
>  {
>  	struct uvc_video_chain *chain;
>  	struct uvc_entity *term;
> +	struct uvc_entity *unit;
>  
>  	list_for_each_entry(term, &dev->entities, list) {
>  		if (!UVC_ENTITY_IS_OTERM(term))
> @@ -1955,6 +2048,13 @@ static int uvc_scan_device(struct uvc_device *dev)
>  		return -1;
>  	}
>  
> +	/* Add GPIO entities to the first chain */

s/chain/chain./

> +	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> +	list_for_each_entry(unit, &dev->entities, list) {
> +		if (UVC_ENTITY_TYPE(unit) == UVC_EXT_GPIO_UNIT)
> +			list_add_tail(&unit->chain, &chain->entities);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -2287,6 +2387,12 @@ static int uvc_probe(struct usb_interface *intf,
>  		goto error;
>  	}
>  
> +	/* Parse the associated GPIOs */

s/GPIOs/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 ae464ba83f4f..f87d14fb3f56 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>

We can use a forward declaration of struct gpio_desc in this file.

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

Maybe a couple of tabs to align this with the other defines above ?

>  
>  /* ------------------------------------------------------------------------
>   * 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, \
> @@ -348,6 +354,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 v4 2/9] media: uvcvideo: Allow external entities

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Tue, Dec 15, 2020 at 04:44:32PM +0100, Ricardo Ribalda wrote:
> Increase the size of the id, to avoid collisions with external entities.

Could you expand the commit message a bit to explain what external
entities are ?

> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 2 +-
>  drivers/media/usb/uvc/uvcvideo.h   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 4cdd65d252d9..9f4451a2e0a6 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1024,7 +1024,7 @@ static const u8 uvc_media_transport_input_guid[16] =
>  	UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
>  static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
>  
> -static struct uvc_entity *uvc_alloc_entity(u16 type, u8 id,
> +static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
>  		unsigned int num_pads, unsigned int extra_size)
>  {
>  	struct uvc_entity *entity;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index df7bf2d104a3..00f985001c1d 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -301,7 +301,7 @@ struct uvc_entity {
>  					 * chain. */
>  	unsigned int flags;
>  
> -	u8 id;
> +	u16 id; /* 0-255: usb entity. 256-65535: external entities */

How about

	/*
	 * Entities exposed by the UVC device use IDs 0-255, extra entities
	 * implemented by the driver (such as the GPIO entity) use IDs 256 and
	 * up.
	 */

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

>  	u16 type;
>  	char name[64];
>  	u8 guid[16];

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Tue, Dec 15, 2020 at 04:44:36PM +0100, 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>

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

> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 53da1d984883..511927e8b746 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -347,6 +347,14 @@ 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,
> +	},
>  };
>  
>  static const struct uvc_menu_info power_line_frequency_controls[] = {
> @@ -735,6 +743,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,
> +	},
>  };
>  
>  /* ------------------------------------------------------------------------

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 7/9] media: uvcvideo: Use dev_ printk aliases

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Tue, Dec 15, 2020 at 04:44:37PM +0100, Ricardo Ribalda wrote:
> Replace all the uses of printk() and uvc_printk() with its
> equivalent dev_ alias macros.
> 
> They are more standard across the kernel tree and provide
> more context about the error.

A very welcome change !

There's one printk() left in uvc_init() which can't be replaced with
dev_*, but we could simply drop it. There's also another instance in
uvc_ctrl_restore_values() that seems to have been missed by this patch.

The commit message doesn't mention the change from KERN_CONT to
pr_cont(). I would either mention it here, or maybe better move it to
the next patch that replaces the pr_cont() calls.

> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c   |  4 +-
>  drivers/media/usb/uvc/uvc_driver.c | 80 ++++++++++++++++--------------
>  drivers/media/usb/uvc/uvc_entity.c | 10 ++--
>  drivers/media/usb/uvc/uvc_status.c | 13 ++---
>  drivers/media/usb/uvc/uvc_video.c  | 51 ++++++++++---------
>  drivers/media/usb/uvc/uvcvideo.h   | 25 ++++------
>  6 files changed, 95 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 511927e8b746..b78a52b6e193 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1327,8 +1327,8 @@ static void uvc_ctrl_status_event_work(struct work_struct *work)
>  	w->urb->interval = dev->int_ep->desc.bInterval;
>  	ret = usb_submit_urb(w->urb, GFP_KERNEL);
>  	if (ret < 0)
> -		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
> -			   ret);
> +		dev_err(&dev->udev->dev,
> +			"Failed to resubmit status URB (%d).\n", ret);
>  }
>  
>  bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 498de09da07e..4379916a6ac1 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -535,8 +535,8 @@ static int uvc_parse_format(struct uvc_device *dev,
>  				sizeof(format->name));
>  			format->fcc = fmtdesc->fcc;
>  		} else {
> -			uvc_printk(KERN_INFO, "Unknown video format %pUl\n",
> -				&buffer[5]);
> +			dev_info(&streaming->intf->dev,
> +				 "Unknown video format %pUl\n", &buffer[5]);
>  			snprintf(format->name, sizeof(format->name), "%pUl\n",
>  				&buffer[5]);
>  			format->fcc = 0;
> @@ -1594,7 +1594,7 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
>  	switch (UVC_ENTITY_TYPE(entity)) {
>  	case UVC_VC_EXTENSION_UNIT:
>  		if (uvc_trace_param & UVC_TRACE_PROBE)
> -			printk(KERN_CONT " <- XU %d", entity->id);
> +			pr_cont(" <- XU %d", entity->id);
>  
>  		if (entity->bNrInPins != 1) {
>  			uvc_trace(UVC_TRACE_DESCR, "Extension unit %d has more "
> @@ -1606,7 +1606,7 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
>  
>  	case UVC_VC_PROCESSING_UNIT:
>  		if (uvc_trace_param & UVC_TRACE_PROBE)
> -			printk(KERN_CONT " <- PU %d", entity->id);
> +			pr_cont(" <- PU %d", entity->id);
>  
>  		if (chain->processing != NULL) {
>  			uvc_trace(UVC_TRACE_DESCR, "Found multiple "
> @@ -1619,7 +1619,7 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
>  
>  	case UVC_VC_SELECTOR_UNIT:
>  		if (uvc_trace_param & UVC_TRACE_PROBE)
> -			printk(KERN_CONT " <- SU %d", entity->id);
> +			pr_cont(" <- SU %d", entity->id);
>  
>  		/* Single-input selector units are ignored. */
>  		if (entity->bNrInPins == 1)
> @@ -1638,7 +1638,7 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
>  	case UVC_ITT_CAMERA:
>  	case UVC_ITT_MEDIA_TRANSPORT_INPUT:
>  		if (uvc_trace_param & UVC_TRACE_PROBE)
> -			printk(KERN_CONT " <- IT %d\n", entity->id);
> +			pr_cont(" <- IT %d\n", entity->id);
>  
>  		break;
>  
> @@ -1646,17 +1646,17 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
>  	case UVC_OTT_DISPLAY:
>  	case UVC_OTT_MEDIA_TRANSPORT_OUTPUT:
>  		if (uvc_trace_param & UVC_TRACE_PROBE)
> -			printk(KERN_CONT " OT %d", entity->id);
> +			pr_cont(" OT %d", entity->id);
>  
>  		break;
>  
>  	case UVC_TT_STREAMING:
>  		if (UVC_ENTITY_IS_ITERM(entity)) {
>  			if (uvc_trace_param & UVC_TRACE_PROBE)
> -				printk(KERN_CONT " <- IT %d\n", entity->id);
> +				pr_cont(" <- IT %d\n", entity->id);
>  		} else {
>  			if (uvc_trace_param & UVC_TRACE_PROBE)
> -				printk(KERN_CONT " OT %d", entity->id);
> +				pr_cont(" OT %d", entity->id);
>  		}
>  
>  		break;
> @@ -1706,9 +1706,9 @@ static int uvc_scan_chain_forward(struct uvc_video_chain *chain,
>  			list_add_tail(&forward->chain, &chain->entities);
>  			if (uvc_trace_param & UVC_TRACE_PROBE) {
>  				if (!found)
> -					printk(KERN_CONT " (->");
> +					pr_cont(" (->");
>  
> -				printk(KERN_CONT " XU %d", forward->id);
> +				pr_cont(" XU %d", forward->id);
>  				found = 1;
>  			}
>  			break;
> @@ -1726,16 +1726,16 @@ static int uvc_scan_chain_forward(struct uvc_video_chain *chain,
>  			list_add_tail(&forward->chain, &chain->entities);
>  			if (uvc_trace_param & UVC_TRACE_PROBE) {
>  				if (!found)
> -					printk(KERN_CONT " (->");
> +					pr_cont(" (->");
>  
> -				printk(KERN_CONT " OT %d", forward->id);
> +				pr_cont(" OT %d", forward->id);
>  				found = 1;
>  			}
>  			break;
>  		}
>  	}
>  	if (found)
> -		printk(KERN_CONT ")");
> +		pr_cont(")");
>  
>  	return 0;
>  }
> @@ -1761,7 +1761,7 @@ static int uvc_scan_chain_backward(struct uvc_video_chain *chain,
>  		}
>  
>  		if (uvc_trace_param & UVC_TRACE_PROBE)
> -			printk(KERN_CONT " <- IT");
> +			pr_cont(" <- IT");
>  
>  		chain->selector = entity;
>  		for (i = 0; i < entity->bNrInPins; ++i) {
> @@ -1782,14 +1782,14 @@ static int uvc_scan_chain_backward(struct uvc_video_chain *chain,
>  			}
>  
>  			if (uvc_trace_param & UVC_TRACE_PROBE)
> -				printk(KERN_CONT " %d", term->id);
> +				pr_cont(" %d", term->id);
>  
>  			list_add_tail(&term->chain, &chain->entities);
>  			uvc_scan_chain_forward(chain, term, entity);
>  		}
>  
>  		if (uvc_trace_param & UVC_TRACE_PROBE)
> -			printk(KERN_CONT "\n");
> +			pr_cont("\n");
>  
>  		id = 0;
>  		break;
> @@ -2044,7 +2044,7 @@ static int uvc_scan_device(struct uvc_device *dev)
>  		uvc_scan_fallback(dev);
>  
>  	if (list_empty(&dev->chains)) {
> -		uvc_printk(KERN_INFO, "No valid video chain found.\n");
> +		dev_info(&dev->udev->dev, "No valid video chain found.\n");
>  		return -1;
>  	}
>  
> @@ -2203,8 +2203,9 @@ int uvc_register_video_device(struct uvc_device *dev,
>  
>  	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>  	if (ret < 0) {
> -		uvc_printk(KERN_ERR, "Failed to register %s device (%d).\n",
> -			   v4l2_type_names[type], ret);
> +		dev_err(&stream->intf->dev,
> +			"Failed to register %s device (%d).\n",
> +			v4l2_type_names[type], ret);
>  		return ret;
>  	}
>  
> @@ -2220,8 +2221,8 @@ static int uvc_register_video(struct uvc_device *dev,
>  	/* Initialize the streaming interface with default parameters. */
>  	ret = uvc_video_init(stream);
>  	if (ret < 0) {
> -		uvc_printk(KERN_ERR, "Failed to initialize the device (%d).\n",
> -			   ret);
> +		dev_err(&stream->intf->dev,
> +			"Failed to initialize the device (%d).\n", ret);
>  		return ret;
>  	}
>  
> @@ -2255,8 +2256,9 @@ static int uvc_register_terms(struct uvc_device *dev,
>  
>  		stream = uvc_stream_by_id(dev, term->id);
>  		if (stream == NULL) {
> -			uvc_printk(KERN_INFO, "No streaming interface found "
> -				   "for terminal %u.", term->id);
> +			dev_info(&dev->udev->dev,
> +				 "No streaming interface found for terminal %u.",
> +				 term->id);
>  			continue;
>  		}
>  
> @@ -2289,8 +2291,8 @@ static int uvc_register_chains(struct uvc_device *dev)
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  		ret = uvc_mc_register_entities(chain);
>  		if (ret < 0)
> -			uvc_printk(KERN_INFO,
> -				   "Failed to register entities (%d).\n", ret);
> +			dev_info(&dev->udev->dev,
> +				 "Failed to register entities (%d).\n", ret);
>  #endif
>  	}
>  
> @@ -2393,17 +2395,19 @@ static int uvc_probe(struct usb_interface *intf,
>  		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>",
> -		le16_to_cpu(udev->descriptor.idVendor),
> -		le16_to_cpu(udev->descriptor.idProduct));
> +	dev_info(&dev->udev->dev,
> +		 "Found UVC %u.%02x device %s (%04x:%04x)\n",

This holds on a single line.

> +		 dev->uvc_version >> 8, dev->uvc_version & 0xff,
> +		 udev->product ? udev->product : "<unnamed>",
> +		 le16_to_cpu(udev->descriptor.idVendor),
> +		 le16_to_cpu(udev->descriptor.idProduct));
>  
>  	if (dev->quirks != dev->info->quirks) {
> -		uvc_printk(KERN_INFO, "Forcing device quirks to 0x%x by module "
> -			"parameter for testing purpose.\n", dev->quirks);
> -		uvc_printk(KERN_INFO, "Please report required quirks to the "
> -			"linux-uvc-devel mailing list.\n");
> +		dev_info(&dev->udev->dev,
> +			 "Forcing device quirks to 0x%x by module parameter for testing purpose.\n",
> +			 dev->quirks);
> +		dev_info(&dev->udev->dev,
> +			 "Please report required quirks to the linux-uvc-devel mailing list.\n");
>  	}
>  
>  	/* Register the V4L2 device. */
> @@ -2432,9 +2436,9 @@ static int uvc_probe(struct usb_interface *intf,
>  
>  	/* Initialize the interrupt URB. */
>  	if ((ret = uvc_status_init(dev)) < 0) {
> -		uvc_printk(KERN_INFO, "Unable to initialize the status "
> -			"endpoint (%d), status interrupt will not be "
> -			"supported.\n", ret);
> +		dev_info(&dev->udev->dev,
> +			 "Unable to initialize the status endpoint (%d), status interrupt will not be supported.\n",
> +			 ret);
>  	}
>  
>  	uvc_trace(UVC_TRACE_PROBE, "UVC device initialized.\n");
> diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c
> index ca3a9c2eec27..a13bee5aee56 100644
> --- a/drivers/media/usb/uvc/uvc_entity.c
> +++ b/drivers/media/usb/uvc/uvc_entity.c
> @@ -139,8 +139,9 @@ int uvc_mc_register_entities(struct uvc_video_chain *chain)
>  	list_for_each_entry(entity, &chain->entities, chain) {
>  		ret = uvc_mc_init_entity(chain, entity);
>  		if (ret < 0) {
> -			uvc_printk(KERN_INFO, "Failed to initialize entity for "
> -				   "entity %u\n", entity->id);
> +			dev_info(&chain->dev->udev->dev,
> +				 "Failed to initialize entity for entity %u\n",
> +				 entity->id);
>  			return ret;
>  		}
>  	}
> @@ -148,8 +149,9 @@ int uvc_mc_register_entities(struct uvc_video_chain *chain)
>  	list_for_each_entry(entity, &chain->entities, chain) {
>  		ret = uvc_mc_create_links(chain, entity);
>  		if (ret < 0) {
> -			uvc_printk(KERN_INFO, "Failed to create links for "
> -				   "entity %u\n", entity->id);
> +			dev_info(&chain->dev->udev->dev,
> +				 "Failed to create links for entity %u\n",
> +				 entity->id);
>  			return ret;
>  		}
>  	}
> diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
> index 2bdb0ff203f8..36fa196a9258 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -208,8 +208,9 @@ static void uvc_status_complete(struct urb *urb)
>  		return;
>  
>  	default:
> -		uvc_printk(KERN_WARNING, "Non-zero status (%d) in status "
> -			"completion handler.\n", urb->status);
> +		dev_warn(&dev->udev->dev,
> +			 "Non-zero status (%d) in status completion handler.\n",
> +			 urb->status);
>  		return;
>  	}
>  
> @@ -243,10 +244,10 @@ static void uvc_status_complete(struct urb *urb)
>  
>  	/* Resubmit the URB. */
>  	urb->interval = dev->int_ep->desc.bInterval;
> -	if ((ret = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
> -		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
> -			ret);
> -	}
> +	ret = usb_submit_urb(urb, GFP_ATOMIC);
> +	if (ret < 0)
> +		dev_err(&dev->udev->dev,
> +			"Failed to resubmit status URB (%d).\n", ret);
>  }
>  
>  int uvc_status_init(struct uvc_device *dev)
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index a6a441d92b94..a4a0f3556986 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -76,9 +76,9 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
>  	if (likely(ret == size))
>  		return 0;
>  
> -	uvc_printk(KERN_ERR,
> -		   "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> -		   uvc_query_name(query), cs, unit, ret, size);
> +	dev_err(&dev->udev->dev,
> +		"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> +		uvc_query_name(query), cs, unit, ret, size);
>  
>  	if (ret != -EPIPE)
>  		return ret;
> @@ -254,9 +254,9 @@ static int uvc_get_video_ctrl(struct uvc_streaming *stream,
>  		ret = -EIO;
>  		goto out;
>  	} else if (ret != size) {
> -		uvc_printk(KERN_ERR, "Failed to query (%u) UVC %s control : "
> -			"%d (exp. %u).\n", query, probe ? "probe" : "commit",
> -			ret, size);
> +		dev_err(&stream->intf->dev,
> +			"Failed to query (%u) UVC %s control : %d (exp. %u).\n",
> +			query, probe ? "probe" : "commit", ret, size);
>  		ret = -EIO;
>  		goto out;
>  	}
> @@ -334,9 +334,9 @@ static int uvc_set_video_ctrl(struct uvc_streaming *stream,
>  		probe ? UVC_VS_PROBE_CONTROL : UVC_VS_COMMIT_CONTROL, data,
>  		size, uvc_timeout_param);
>  	if (ret != size) {
> -		uvc_printk(KERN_ERR, "Failed to set UVC %s control : "
> -			"%d (exp. %u).\n", probe ? "probe" : "commit",
> -			ret, size);
> +		dev_err(&stream->intf->dev,
> +			"Failed to set UVC %s control : %d (exp. %u).\n",
> +			probe ? "probe" : "commit", ret, size);
>  		ret = -EIO;
>  	}
>  
> @@ -1119,9 +1119,10 @@ static void uvc_video_copy_data_work(struct work_struct *work)
>  	}
>  
>  	ret = usb_submit_urb(uvc_urb->urb, GFP_KERNEL);
> -	if (ret < 0)
> -		uvc_printk(KERN_ERR, "Failed to resubmit video URB (%d).\n",
> -			   ret);
> +	if (ret < 0) {
> +		dev_err(&uvc_urb->stream->intf->dev,
> +			"Failed to resubmit video URB (%d).\n", ret);
> +	}

No need for curly braces.

>  }
>  
>  static void uvc_video_decode_data(struct uvc_urb *uvc_urb,
> @@ -1507,8 +1508,9 @@ static void uvc_video_complete(struct urb *urb)
>  		break;
>  
>  	default:
> -		uvc_printk(KERN_WARNING, "Non-zero status (%d) in video "
> -			"completion handler.\n", urb->status);
> +		dev_warn(&stream->intf->dev,
> +			 "Non-zero status (%d) in video completion handler.\n",
> +			 urb->status);
>  		fallthrough;
>  	case -ENOENT:		/* usb_poison_urb() called. */
>  		if (stream->frozen)
> @@ -1545,9 +1547,8 @@ static void uvc_video_complete(struct urb *urb)
>  	if (!uvc_urb->async_operations) {
>  		ret = usb_submit_urb(uvc_urb->urb, GFP_ATOMIC);
>  		if (ret < 0)
> -			uvc_printk(KERN_ERR,
> -				   "Failed to resubmit video URB (%d).\n",
> -				   ret);
> +			dev_err(&stream->intf->dev,
> +				"Failed to resubmit video URB (%d).\n", ret);
>  		return;
>  	}
>  
> @@ -1893,8 +1894,9 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
>  	for_each_uvc_urb(uvc_urb, stream) {
>  		ret = usb_submit_urb(uvc_urb->urb, gfp_flags);
>  		if (ret < 0) {
> -			uvc_printk(KERN_ERR, "Failed to submit URB %u (%d).\n",
> -				   uvc_urb_index(uvc_urb), ret);
> +			dev_err(&stream->intf->dev,
> +				"Failed to submit URB %u (%d).\n",
> +				uvc_urb_index(uvc_urb), ret);
>  			uvc_video_stop_transfer(stream, 1);
>  			return ret;
>  		}
> @@ -1989,7 +1991,8 @@ int uvc_video_init(struct uvc_streaming *stream)
>  	int ret;
>  
>  	if (stream->nformats == 0) {
> -		uvc_printk(KERN_INFO, "No supported video formats found.\n");
> +		dev_info(&stream->intf->dev,
> +			 "No supported video formats found.\n");
>  		return -EINVAL;
>  	}
>  
> @@ -2029,8 +2032,8 @@ int uvc_video_init(struct uvc_streaming *stream)
>  	}
>  
>  	if (format->nframes == 0) {
> -		uvc_printk(KERN_INFO, "No frame descriptor found for the "
> -			"default format.\n");
> +		dev_info(&stream->intf->dev,
> +			 "No frame descriptor found for the default format.\n");
>  		return -EINVAL;
>  	}
>  
> @@ -2064,8 +2067,8 @@ int uvc_video_init(struct uvc_streaming *stream)
>  		if (stream->intf->num_altsetting == 1)
>  			stream->decode = uvc_video_encode_bulk;
>  		else {
> -			uvc_printk(KERN_INFO, "Isochronous endpoints are not "
> -				"supported for video output devices.\n");
> +			dev_info(&stream->intf->dev,
> +				 "Isochronous endpoints are not supported for video output devices.\n");
>  			return -EINVAL;
>  		}
>  	}
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index f87d14fb3f56..d8e2f27bf576 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -742,20 +742,17 @@ extern unsigned int uvc_trace_param;
>  extern unsigned int uvc_timeout_param;
>  extern unsigned int uvc_hw_timestamps_param;
>  
> -#define uvc_trace(flag, msg...) \
> -	do { \
> -		if (uvc_trace_param & flag) \
> -			printk(KERN_DEBUG "uvcvideo: " msg); \
> -	} while (0)
> -
> -#define uvc_warn_once(dev, warn, msg...) \
> -	do { \
> -		if (!test_and_set_bit(warn, &dev->warnings)) \
> -			printk(KERN_INFO "uvcvideo: " msg); \
> -	} while (0)
> -
> -#define uvc_printk(level, msg...) \
> -	printk(level "uvcvideo: " msg)
> +#define uvc_trace(flag, fmt, ...)					\
> +do {									\
> +	if (uvc_trace_param & flag)					\
> +		printk(KERN_DEBUG "uvcvideo: " fmt, ##__VA_ARGS__);	\
> +} while (0)

It would be nice to use dev_printk(KERN_DEBUG, dev, ...) :-) (in a
separate patch).

> +
> +#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)

Could you please mention these changes in the commit message ?

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

>  
>  /* --------------------------------------------------------------------------
>   * Internal functions.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 8/9] media: uvcvideo: New macro uvc_trace_cont

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Tue, Dec 15, 2020 at 04:44:38PM +0100, Ricardo Ribalda wrote:
> Remove all the duplicated code around pr_cont with a new macro.
> 
> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

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

> ---
>  drivers/media/usb/uvc/uvc_driver.c | 57 +++++++++++-------------------
>  drivers/media/usb/uvc/uvcvideo.h   |  6 ++++
>  2 files changed, 27 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 4379916a6ac1..e49491250e87 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1593,8 +1593,7 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
>  {
>  	switch (UVC_ENTITY_TYPE(entity)) {
>  	case UVC_VC_EXTENSION_UNIT:
> -		if (uvc_trace_param & UVC_TRACE_PROBE)
> -			pr_cont(" <- XU %d", entity->id);
> +		uvc_trace_cont(UVC_TRACE_PROBE, " <- XU %d", entity->id);
>  
>  		if (entity->bNrInPins != 1) {
>  			uvc_trace(UVC_TRACE_DESCR, "Extension unit %d has more "
> @@ -1605,8 +1604,7 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
>  		break;
>  
>  	case UVC_VC_PROCESSING_UNIT:
> -		if (uvc_trace_param & UVC_TRACE_PROBE)
> -			pr_cont(" <- PU %d", entity->id);
> +		uvc_trace_cont(UVC_TRACE_PROBE, " <- PU %d", entity->id);
>  
>  		if (chain->processing != NULL) {
>  			uvc_trace(UVC_TRACE_DESCR, "Found multiple "
> @@ -1618,8 +1616,7 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
>  		break;
>  
>  	case UVC_VC_SELECTOR_UNIT:
> -		if (uvc_trace_param & UVC_TRACE_PROBE)
> -			pr_cont(" <- SU %d", entity->id);
> +		uvc_trace_cont(UVC_TRACE_PROBE, " <- SU %d", entity->id);
>  
>  		/* Single-input selector units are ignored. */
>  		if (entity->bNrInPins == 1)
> @@ -1637,27 +1634,22 @@ static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
>  	case UVC_ITT_VENDOR_SPECIFIC:
>  	case UVC_ITT_CAMERA:
>  	case UVC_ITT_MEDIA_TRANSPORT_INPUT:
> -		if (uvc_trace_param & UVC_TRACE_PROBE)
> -			pr_cont(" <- IT %d\n", entity->id);
> +		uvc_trace_cont(UVC_TRACE_PROBE, " <- IT %d\n", entity->id);
>  
>  		break;
>  
>  	case UVC_OTT_VENDOR_SPECIFIC:
>  	case UVC_OTT_DISPLAY:
>  	case UVC_OTT_MEDIA_TRANSPORT_OUTPUT:
> -		if (uvc_trace_param & UVC_TRACE_PROBE)
> -			pr_cont(" OT %d", entity->id);
> +		uvc_trace_cont(UVC_TRACE_PROBE, " OT %d", entity->id);
>  
>  		break;
>  
>  	case UVC_TT_STREAMING:
> -		if (UVC_ENTITY_IS_ITERM(entity)) {
> -			if (uvc_trace_param & UVC_TRACE_PROBE)
> -				pr_cont(" <- IT %d\n", entity->id);
> -		} else {
> -			if (uvc_trace_param & UVC_TRACE_PROBE)
> -				pr_cont(" OT %d", entity->id);
> -		}
> +		if (UVC_ENTITY_IS_ITERM(entity))
> +			uvc_trace_cont(UVC_TRACE_PROBE, " <- IT %d\n", entity->id);
> +		else
> +			uvc_trace_cont(UVC_TRACE_PROBE, " OT %d", entity->id);
>  
>  		break;
>  
> @@ -1704,13 +1696,11 @@ static int uvc_scan_chain_forward(struct uvc_video_chain *chain,
>  			}
>  
>  			list_add_tail(&forward->chain, &chain->entities);
> -			if (uvc_trace_param & UVC_TRACE_PROBE) {
> -				if (!found)
> -					pr_cont(" (->");
> +			if (!found)
> +				uvc_trace_cont(UVC_TRACE_PROBE, " (->");
>  
> -				pr_cont(" XU %d", forward->id);
> -				found = 1;
> -			}
> +			uvc_trace_cont(UVC_TRACE_PROBE, " XU %d", forward->id);
> +			found = 1;
>  			break;
>  
>  		case UVC_OTT_VENDOR_SPECIFIC:
> @@ -1724,18 +1714,16 @@ static int uvc_scan_chain_forward(struct uvc_video_chain *chain,
>  			}
>  
>  			list_add_tail(&forward->chain, &chain->entities);
> -			if (uvc_trace_param & UVC_TRACE_PROBE) {
> -				if (!found)
> -					pr_cont(" (->");
> +			if (!found)
> +				uvc_trace_cont(UVC_TRACE_PROBE, " (->");
>  
> -				pr_cont(" OT %d", forward->id);
> -				found = 1;
> -			}
> +			uvc_trace_cont(UVC_TRACE_PROBE, " OT %d", forward->id);
> +			found = 1;
>  			break;
>  		}
>  	}
>  	if (found)
> -		pr_cont(")");
> +		uvc_trace_cont(UVC_TRACE_PROBE, ")");
>  
>  	return 0;
>  }
> @@ -1760,8 +1748,7 @@ static int uvc_scan_chain_backward(struct uvc_video_chain *chain,
>  			break;
>  		}
>  
> -		if (uvc_trace_param & UVC_TRACE_PROBE)
> -			pr_cont(" <- IT");
> +		uvc_trace_cont(UVC_TRACE_PROBE, " <- IT");
>  
>  		chain->selector = entity;
>  		for (i = 0; i < entity->bNrInPins; ++i) {
> @@ -1781,15 +1768,13 @@ static int uvc_scan_chain_backward(struct uvc_video_chain *chain,
>  				return -EINVAL;
>  			}
>  
> -			if (uvc_trace_param & UVC_TRACE_PROBE)
> -				pr_cont(" %d", term->id);
> +			uvc_trace_cont(UVC_TRACE_PROBE, " %d", term->id);
>  
>  			list_add_tail(&term->chain, &chain->entities);
>  			uvc_scan_chain_forward(chain, term, entity);
>  		}
>  
> -		if (uvc_trace_param & UVC_TRACE_PROBE)
> -			pr_cont("\n");
> +		uvc_trace_cont(UVC_TRACE_PROBE, "\n");
>  
>  		id = 0;
>  		break;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index d8e2f27bf576..2b5ba4b02d3a 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -748,6 +748,12 @@ do {									\
>  		printk(KERN_DEBUG "uvcvideo: " fmt, ##__VA_ARGS__);	\
>  } while (0)
>  
> +#define uvc_trace_cont(flag, fmt, ...)					\
> +do {									\
> +	if (uvc_trace_param & flag)					\
> +		pr_cont(fmt, ##__VA_ARGS__);				\
> +} while (0)
> +
>  #define uvc_warn_once(_dev, warn, fmt, ...)				\
>  do {									\
>  	if (!test_and_set_bit(warn, &(_dev)->warnings))			\

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 9/9] media: uvcvideo: Implement UVC_QUIRK_PRIVACY_DURING_STREAM

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Tue, Dec 15, 2020 at 04:44:39PM +0100, Ricardo Ribalda wrote:
> Some devices, can only read the privacy_pin if the device is
> streaming.

:-(

> This patch implement a quirk for such devices, in order to avoid invalid
> reads and/or spurious events.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 97 ++++++++++++++++++++++++++----
>  drivers/media/usb/uvc/uvc_queue.c  |  3 +
>  drivers/media/usb/uvc/uvcvideo.h   |  6 ++
>  3 files changed, 94 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index e49491250e87..61313019e226 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/dmi.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> @@ -1471,13 +1472,39 @@ static int uvc_parse_control(struct uvc_device *dev)
>  	return 0;
>  }
>  
> +static bool uvc_ext_gpio_is_streaming(struct uvc_device *dev)
> +{
> +	struct uvc_streaming *streaming;
> +
> +	list_for_each_entry(streaming, &dev->streams, list) {
> +		if (uvc_queue_streaming(&streaming->queue))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +/* Update the cached value and return true if it has changed */
> +static bool uvc_gpio_update_value(struct uvc_entity *unit, u8 *new_val)
> +{
> +	*new_val = gpiod_get_value(unit->gpio.gpio_privacy);
> +
> +	return atomic_xchg(&unit->gpio.gpio_privacy_value, *new_val) !=
> +								      *new_val;

That's a weird indentation. Also, as the left hand side modifies
*new_val, does C guarantee the order in which the two operands to != are
evaluated ? Could the code be written in an easier to read way ?

> +}
> +
>  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;
>  
> -	*(uint8_t *)data = gpiod_get_value(entity->gpio.gpio_privacy);
> +	if ((dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) &&
> +	    !uvc_ext_gpio_is_streaming(dev))
> +		return -EBUSY;
> +
> +	uvc_gpio_update_value(entity, (uint8_t *)data);
> +
>  	return 0;
>  }
>  
> @@ -1491,26 +1518,69 @@ static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
>  	return 0;
>  }
>  
> -static irqreturn_t uvc_privacy_gpio_irq(int irq, void *data)
> +static struct uvc_entity *uvc_find_ext_gpio_unit(struct uvc_device *dev)
>  {
> -	struct uvc_device *dev = data;
> -	struct uvc_video_chain *chain;
>  	struct uvc_entity *unit;
> -	u8 value;
>  
> -	/* GPIO entities are always on the first chain */
> -	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
>  	list_for_each_entry(unit, &dev->entities, list) {
> -		if (UVC_ENTITY_TYPE(unit) != UVC_EXT_GPIO_UNIT)
> -			continue;
> -		value = gpiod_get_value(unit->gpio.gpio_privacy);
> -		uvc_ctrl_status_event(NULL, chain, unit->controls, &value);
> -		return IRQ_HANDLED;
> +		if (UVC_ENTITY_TYPE(unit) == UVC_EXT_GPIO_UNIT)
> +			return unit;
>  	}
>  
> +	return unit;
> +}
> +
> +void uvc_privacy_gpio_event(struct uvc_device *dev)
> +{
> +	struct uvc_entity *unit;
> +	struct uvc_video_chain *chain;
> +	u8 new_value;
> +
> +	unit = uvc_find_ext_gpio_unit(dev);
> +	if (WARN_ONCE(!unit, "Unable to find entity ext_gpio_unit"))
> +		return;
> +
> +	if (!uvc_gpio_update_value(unit, &new_value))
> +		return;

If VIDIOC_G_CTRL() is called before the IRQ is processed, this
uvc_gpio_update_value() call will return false, and no event will be
generated. I don't think that's right, and even should be generated
every time the control changes.

> +
> +	/* GPIO entities are always on the first chain */
> +	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> +	uvc_ctrl_status_event(NULL, chain, unit->controls, &new_value);
> +}
> +
> +static irqreturn_t uvc_privacy_gpio_irq(int irq, void *data)
> +{
> +	struct uvc_device *dev = data;
> +
> +	/* Ignore privacy events during streamoff */
> +	if (dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM)
> +		if (!uvc_ext_gpio_is_streaming(dev))
> +			return IRQ_HANDLED;

	if (dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) {
		if (!uvc_ext_gpio_is_streaming(dev))
			return IRQ_HANDLED;
	}

There's a potential race condition with VIDIOC_STREAMON and
VIDIOC_STREAMOFF. Could you explain what the device does exactly when
not streaming ? As the GPIO isn't tied to the UVC controller, how comes
the streaming state influences it ? Any hope the firmware could be fixed
instead ?

> +
> +	uvc_privacy_gpio_event(dev);
> +
>  	return IRQ_HANDLED;
>  }
>  
> +static const struct dmi_system_id privacy_valid_during_streamon[] = {
> +	{
> +		.ident = "HP Elite c1030 Chromebook",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Jinlon"),
> +		},
> +	},
> +	{
> +		.ident = "HP Pro c640 Chromebook",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Dratini"),
> +		},
> +	},
> +	{ } /* terminate list */
> +};
> +
> +
>  static int uvc_parse_gpio(struct uvc_device *dev)
>  {
>  	struct uvc_entity *unit;
> @@ -1545,6 +1615,9 @@ static int uvc_parse_gpio(struct uvc_device *dev)
>  	if (irq == -EPROBE_DEFER)
>  		return -EPROBE_DEFER;
>  
> +	if (dmi_check_system(privacy_valid_during_streamon))
> +		dev->quirks |= UVC_QUIRK_PRIVACY_DURING_STREAM;
> +
>  	if (irq < 0)
>  		return 0;
>  
> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> index cd60c6c1749e..e800d491303f 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -337,9 +337,12 @@ int uvc_dequeue_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf,
>  int uvc_queue_streamon(struct uvc_video_queue *queue, enum v4l2_buf_type type)
>  {
>  	int ret;
> +	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
>  
>  	mutex_lock(&queue->mutex);
>  	ret = vb2_streamon(&queue->queue, type);
> +	if (stream->dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM)
> +		uvc_privacy_gpio_event(stream->dev);
>  	mutex_unlock(&queue->mutex);
>  
>  	return ret;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 2b5ba4b02d3a..2a95b3ed3ea8 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/gpio/consumer.h>
>  #include <linux/kernel.h>
>  #include <linux/poll.h>
> @@ -209,6 +210,7 @@
>  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
>  #define UVC_QUIRK_FORCE_Y8		0x00000800
>  #define UVC_QUIRK_FORCE_BPP		0x00001000
> +#define UVC_QUIRK_PRIVACY_DURING_STREAM	0x00002000
>  
>  /* Format flags */
>  #define UVC_FMT_FLAG_COMPRESSED		0x00000001
> @@ -359,6 +361,7 @@ struct uvc_entity {
>  			u8  bControlSize;
>  			u8  *bmControls;
>  			struct gpio_desc *gpio_privacy;
> +			atomic_t  gpio_privacy_value;
>  		} gpio;
>  	};
>  
> @@ -815,6 +818,9 @@ extern const struct v4l2_file_operations uvc_fops;
>  int uvc_mc_register_entities(struct uvc_video_chain *chain);
>  void uvc_mc_cleanup_entity(struct uvc_entity *entity);
>  
> +/* Privacy gpio */
> +void uvc_privacy_gpio_event(struct uvc_device *dev);
> +
>  /* Video */
>  int uvc_video_init(struct uvc_streaming *stream);
>  int uvc_video_suspend(struct uvc_streaming *stream);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 5/9] media: uvcvideo: Implement UVC_EXT_GPIO_UNIT

[Ricardo Ribalda]

HI Laurent

Thanks for your review!

On Sun, Dec 20, 2020 at 5:49 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Tue, Dec 15, 2020 at 04:44:35PM +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.
> >
> > Since the uvc device is not aware of this pin (and it should't), we need
>
> s/should't/shouldn't/
>
> But I don't agree, if this is part of the camera, it should be
> implemented in the camera firmware. I understand that it's not possible
> (or desirable) with the hardware architecture you're dealing with
> though. How about
>
> "In some systems, the GPIO is connected to main SoC instead of the
> camera controller, with the connected 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:"
>
> > 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:
> >
> >   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   |   6 ++
> >  drivers/media/usb/uvc/uvc_driver.c | 106 +++++++++++++++++++++++++++++
> >  drivers/media/usb/uvc/uvcvideo.h   |  12 ++++
> >  3 files changed, 124 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 531816762892..53da1d984883 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1302,6 +1302,9 @@ static void uvc_ctrl_status_event_work(struct work_struct *work)
> >
> >       mutex_unlock(&chain->ctrl_mutex);
> >
> > +     if (!w->urb)
> > +             return;
>
> A small comment to explain why would be useful.
>
> > +
> >       /* Resubmit the URB. */
> >       w->urb->interval = dev->int_ep->desc.bInterval;
> >       ret = usb_submit_urb(w->urb, GFP_KERNEL);
> > @@ -2286,6 +2289,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 534629ecd60d..498de09da07e 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;
> > @@ -1053,6 +1055,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;
> > @@ -1466,6 +1471,93 @@ static int uvc_parse_control(struct uvc_device *dev)
> >       return 0;
> >  }
>
> Can we add
>
> /* -----------------------------------------------------------------------------
>  * Privacy GPIO
>  */
>
> here ?
>
> > +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;
> > +
> > +     *(uint8_t *)data = gpiod_get_value(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_privacy_gpio_irq(int irq, void *data)
> > +{
> > +     struct uvc_device *dev = data;
> > +     struct uvc_video_chain *chain;
> > +     struct uvc_entity *unit;
> > +     u8 value;
> > +
> > +     /* GPIO entities are always on the first chain */
>
>
> s/chain/chain./
>
> > +     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > +     list_for_each_entry(unit, &dev->entities, list) {
>
> Can we iterate over entities in the chain instead ?
>
> I'd be actually tempted to pass the pointer to the entity as data. We
> would however need to add a chain pointer to uvc_entity. Not sure if
> it's worth it, up to you.
>
> > +             if (UVC_ENTITY_TYPE(unit) != UVC_EXT_GPIO_UNIT)
> > +                     continue;
> > +             value = gpiod_get_value(unit->gpio.gpio_privacy);
>
> Could this sleep ? Should we use a threaded IRQ ?

get_value() should not sleep, but there are platforms that may sleep
to get the irq.

In order to support them I have converted to threaded_irq in the next version
and called gpio_get_value_cansleep()

>
> > +             uvc_ctrl_status_event(NULL, chain, unit->controls, &value);
> > +             return IRQ_HANDLED;
> > +     }
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static int uvc_parse_gpio(struct uvc_device *dev)
>
> How about making the naming scheme consistent, by using a uvc_gpio_
> prefix for all four functions ?
>
> > +{
> > +     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;
>
> This could be folded in
>
>         if (IS_ERR_OR_NULL(gpio_privacy))
>                 return PTR_ERR_OR_ZERO(gpio_privacy);
>
> Up to you.
>
> > +
> > +     unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > +     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");
>
> Other unit names don't contain "Unit", should this be just "GPIO" ?
>
> > +
> > +     list_add_tail(&unit->list, &dev->entities);
> > +
> > +     irq = gpiod_to_irq(gpio_privacy);
> > +     if (irq == -EPROBE_DEFER)
> > +             return -EPROBE_DEFER;
>
> Can this happen in practice ?

Yes :(, Eg:
If the irq is handled by an GPIO expansion via I2C, and it has not
been probed yet.

>
> > +
> > +     if (irq < 0)
> > +             return 0;
>
> So this will succeed, with the GPIO unit created, but it won't be
> operational. Is this desired ? Shouldn't we at least print a warning ?

If there is no IRQ, the unit is still functional, but instead of
sending events when the gpio changes, the user will have to poll it.

>
> I also wonder if we should create the GPIO unit in this cases. Wouldn't
> it be more confusing for userspace to see the privacy control being
> exposed, but without it being functional ? Maybe the call to
> gpiod_to_irq should be moved to before uvc_alloc_entity() ?
>
> > +
> > +     ret = devm_request_irq(&dev->udev->dev, irq, uvc_privacy_gpio_irq,
> > +                            IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> > +                            "uvc_privacy_gpio", dev);
> > +     if (ret < 0)
> > +             dev_warn(&dev->udev->dev,
> > +                    "Unable to request uvc_privacy_gpio irq. Continuing\n");
>
> s/uvc_privacy_gpio irq/privacy GPIO IRQ/ ?
>
> There's also a race condition, as soon as the IRQ is requested, it could
> fire, and the driver isn't fully initialized. Walking the chain in the
> IRQ handler while entities are added to the chain could lead to a crash.
> I see two options, either adding a flag to tell that initialization is
> complete and returning from the interrupt handler when the flag isn't
> set, or moving IRQ registration to a separate function, called later
> during the initialization. I think I have a preference for the latter.
>
> > +
> > +     return 0;
> > +}
> > +
> >  /* ------------------------------------------------------------------------
> >   * UVC device scan
> >   */
> > @@ -1917,6 +2009,7 @@ static int uvc_scan_device(struct uvc_device *dev)
> >  {
> >       struct uvc_video_chain *chain;
> >       struct uvc_entity *term;
> > +     struct uvc_entity *unit;
> >
> >       list_for_each_entry(term, &dev->entities, list) {
> >               if (!UVC_ENTITY_IS_OTERM(term))
> > @@ -1955,6 +2048,13 @@ static int uvc_scan_device(struct uvc_device *dev)
> >               return -1;
> >       }
> >
> > +     /* Add GPIO entities to the first chain */
>
> s/chain/chain./
>
> > +     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > +     list_for_each_entry(unit, &dev->entities, list) {
> > +             if (UVC_ENTITY_TYPE(unit) == UVC_EXT_GPIO_UNIT)
> > +                     list_add_tail(&unit->chain, &chain->entities);
> > +     }
> > +
> >       return 0;
> >  }
> >
> > @@ -2287,6 +2387,12 @@ static int uvc_probe(struct usb_interface *intf,
> >               goto error;
> >       }
> >
> > +     /* Parse the associated GPIOs */
>
> s/GPIOs/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 ae464ba83f4f..f87d14fb3f56 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>
>
> We can use a forward declaration of struct gpio_desc in this file.
>
> >  #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
>
> Maybe a couple of tabs to align this with the other defines above ?
>
> >
> >  /* ------------------------------------------------------------------------
> >   * 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, \
> > @@ -348,6 +354,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

Re: [PATCH v4 5/9] media: uvcvideo: Implement UVC_EXT_GPIO_UNIT

[Laurent Pinchart]

Hi Ricardo,

On Sun, Dec 20, 2020 at 11:56:12PM +0100, Ricardo Ribalda wrote:
> On Sun, Dec 20, 2020 at 5:49 PM Laurent Pinchart wrote:
> > On Tue, Dec 15, 2020 at 04:44:35PM +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.
> > >
> > > Since the uvc device is not aware of this pin (and it should't), we need
> >
> > s/should't/shouldn't/
> >
> > But I don't agree, if this is part of the camera, it should be
> > implemented in the camera firmware. I understand that it's not possible
> > (or desirable) with the hardware architecture you're dealing with
> > though. How about
> >
> > "In some systems, the GPIO is connected to main SoC instead of the
> > camera controller, with the connected 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:"
> >
> > > 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:
> > >
> > >   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   |   6 ++
> > >  drivers/media/usb/uvc/uvc_driver.c | 106 +++++++++++++++++++++++++++++
> > >  drivers/media/usb/uvc/uvcvideo.h   |  12 ++++
> > >  3 files changed, 124 insertions(+)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index 531816762892..53da1d984883 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -1302,6 +1302,9 @@ static void uvc_ctrl_status_event_work(struct work_struct *work)
> > >
> > >       mutex_unlock(&chain->ctrl_mutex);
> > >
> > > +     if (!w->urb)
> > > +             return;
> >
> > A small comment to explain why would be useful.
> >
> > > +
> > >       /* Resubmit the URB. */
> > >       w->urb->interval = dev->int_ep->desc.bInterval;
> > >       ret = usb_submit_urb(w->urb, GFP_KERNEL);
> > > @@ -2286,6 +2289,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 534629ecd60d..498de09da07e 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;
> > > @@ -1053,6 +1055,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;
> > > @@ -1466,6 +1471,93 @@ static int uvc_parse_control(struct uvc_device *dev)
> > >       return 0;
> > >  }
> >
> > Can we add
> >
> > /* -----------------------------------------------------------------------------
> >  * Privacy GPIO
> >  */
> >
> > here ?
> >
> > > +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;
> > > +
> > > +     *(uint8_t *)data = gpiod_get_value(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_privacy_gpio_irq(int irq, void *data)
> > > +{
> > > +     struct uvc_device *dev = data;
> > > +     struct uvc_video_chain *chain;
> > > +     struct uvc_entity *unit;
> > > +     u8 value;
> > > +
> > > +     /* GPIO entities are always on the first chain */
> >
> >
> > s/chain/chain./
> >
> > > +     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > > +     list_for_each_entry(unit, &dev->entities, list) {
> >
> > Can we iterate over entities in the chain instead ?
> >
> > I'd be actually tempted to pass the pointer to the entity as data. We
> > would however need to add a chain pointer to uvc_entity. Not sure if
> > it's worth it, up to you.
> >
> > > +             if (UVC_ENTITY_TYPE(unit) != UVC_EXT_GPIO_UNIT)
> > > +                     continue;
> > > +             value = gpiod_get_value(unit->gpio.gpio_privacy);
> >
> > Could this sleep ? Should we use a threaded IRQ ?
> 
> get_value() should not sleep, but there are platforms that may sleep
> to get the irq.
> 
> In order to support them I have converted to threaded_irq in the next version
> and called gpio_get_value_cansleep()
> 
> > > +             uvc_ctrl_status_event(NULL, chain, unit->controls, &value);
> > > +             return IRQ_HANDLED;
> > > +     }
> > > +
> > > +     return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int uvc_parse_gpio(struct uvc_device *dev)
> >
> > How about making the naming scheme consistent, by using a uvc_gpio_
> > prefix for all four functions ?
> >
> > > +{
> > > +     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;
> >
> > This could be folded in
> >
> >         if (IS_ERR_OR_NULL(gpio_privacy))
> >                 return PTR_ERR_OR_ZERO(gpio_privacy);
> >
> > Up to you.
> >
> > > +
> > > +     unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > > +     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");
> >
> > Other unit names don't contain "Unit", should this be just "GPIO" ?
> >
> > > +
> > > +     list_add_tail(&unit->list, &dev->entities);
> > > +
> > > +     irq = gpiod_to_irq(gpio_privacy);
> > > +     if (irq == -EPROBE_DEFER)
> > > +             return -EPROBE_DEFER;
> >
> > Can this happen in practice ?
> 
> Yes :(, Eg:
> If the irq is handled by an GPIO expansion via I2C, and it has not
> been probed yet.

Won't the devm_gpiod_get_optional() call fail with -EPROBE_DEFER in that
case ?

> > > +
> > > +     if (irq < 0)
> > > +             return 0;
> >
> > So this will succeed, with the GPIO unit created, but it won't be
> > operational. Is this desired ? Shouldn't we at least print a warning ?
> 
> If there is no IRQ, the unit is still functional, but instead of
> sending events when the gpio changes, the user will have to poll it.

Good point.

> > I also wonder if we should create the GPIO unit in this cases. Wouldn't
> > it be more confusing for userspace to see the privacy control being
> > exposed, but without it being functional ? Maybe the call to
> > gpiod_to_irq should be moved to before uvc_alloc_entity() ?
> >
> > > +
> > > +     ret = devm_request_irq(&dev->udev->dev, irq, uvc_privacy_gpio_irq,
> > > +                            IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> > > +                            "uvc_privacy_gpio", dev);
> > > +     if (ret < 0)
> > > +             dev_warn(&dev->udev->dev,
> > > +                    "Unable to request uvc_privacy_gpio irq. Continuing\n");
> >
> > s/uvc_privacy_gpio irq/privacy GPIO IRQ/ ?
> >
> > There's also a race condition, as soon as the IRQ is requested, it could
> > fire, and the driver isn't fully initialized. Walking the chain in the
> > IRQ handler while entities are added to the chain could lead to a crash.
> > I see two options, either adding a flag to tell that initialization is
> > complete and returning from the interrupt handler when the flag isn't
> > set, or moving IRQ registration to a separate function, called later
> > during the initialization. I think I have a preference for the latter.
> >
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  /* ------------------------------------------------------------------------
> > >   * UVC device scan
> > >   */
> > > @@ -1917,6 +2009,7 @@ static int uvc_scan_device(struct uvc_device *dev)
> > >  {
> > >       struct uvc_video_chain *chain;
> > >       struct uvc_entity *term;
> > > +     struct uvc_entity *unit;
> > >
> > >       list_for_each_entry(term, &dev->entities, list) {
> > >               if (!UVC_ENTITY_IS_OTERM(term))
> > > @@ -1955,6 +2048,13 @@ static int uvc_scan_device(struct uvc_device *dev)
> > >               return -1;
> > >       }
> > >
> > > +     /* Add GPIO entities to the first chain */
> >
> > s/chain/chain./
> >
> > > +     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > > +     list_for_each_entry(unit, &dev->entities, list) {
> > > +             if (UVC_ENTITY_TYPE(unit) == UVC_EXT_GPIO_UNIT)
> > > +                     list_add_tail(&unit->chain, &chain->entities);
> > > +     }
> > > +
> > >       return 0;
> > >  }
> > >
> > > @@ -2287,6 +2387,12 @@ static int uvc_probe(struct usb_interface *intf,
> > >               goto error;
> > >       }
> > >
> > > +     /* Parse the associated GPIOs */
> >
> > s/GPIOs/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 ae464ba83f4f..f87d14fb3f56 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>
> >
> > We can use a forward declaration of struct gpio_desc in this file.
> >
> > >  #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
> >
> > Maybe a couple of tabs to align this with the other defines above ?
> >
> > >
> > >  /* ------------------------------------------------------------------------
> > >   * 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, \
> > > @@ -348,6 +354,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 v4 5/9] media: uvcvideo: Implement UVC_EXT_GPIO_UNIT

[Ricardo Ribalda]

HI Laurent

On Mon, Dec 21, 2020 at 12:24 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> On Sun, Dec 20, 2020 at 11:56:12PM +0100, Ricardo Ribalda wrote:
> > On Sun, Dec 20, 2020 at 5:49 PM Laurent Pinchart wrote:
> > > On Tue, Dec 15, 2020 at 04:44:35PM +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.
> > > >
> > > > Since the uvc device is not aware of this pin (and it should't), we need
> > >
> > > s/should't/shouldn't/
> > >
> > > But I don't agree, if this is part of the camera, it should be
> > > implemented in the camera firmware. I understand that it's not possible
> > > (or desirable) with the hardware architecture you're dealing with
> > > though. How about
> > >
> > > "In some systems, the GPIO is connected to main SoC instead of the
> > > camera controller, with the connected 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:"
> > >
> > > > 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:
> > > >
> > > >   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   |   6 ++
> > > >  drivers/media/usb/uvc/uvc_driver.c | 106 +++++++++++++++++++++++++++++
> > > >  drivers/media/usb/uvc/uvcvideo.h   |  12 ++++
> > > >  3 files changed, 124 insertions(+)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > index 531816762892..53da1d984883 100644
> > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > @@ -1302,6 +1302,9 @@ static void uvc_ctrl_status_event_work(struct work_struct *work)
> > > >
> > > >       mutex_unlock(&chain->ctrl_mutex);
> > > >
> > > > +     if (!w->urb)
> > > > +             return;
> > >
> > > A small comment to explain why would be useful.
> > >
> > > > +
> > > >       /* Resubmit the URB. */
> > > >       w->urb->interval = dev->int_ep->desc.bInterval;
> > > >       ret = usb_submit_urb(w->urb, GFP_KERNEL);
> > > > @@ -2286,6 +2289,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 534629ecd60d..498de09da07e 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;
> > > > @@ -1053,6 +1055,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;
> > > > @@ -1466,6 +1471,93 @@ static int uvc_parse_control(struct uvc_device *dev)
> > > >       return 0;
> > > >  }
> > >
> > > Can we add
> > >
> > > /* -----------------------------------------------------------------------------
> > >  * Privacy GPIO
> > >  */
> > >
> > > here ?
> > >
> > > > +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;
> > > > +
> > > > +     *(uint8_t *)data = gpiod_get_value(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_privacy_gpio_irq(int irq, void *data)
> > > > +{
> > > > +     struct uvc_device *dev = data;
> > > > +     struct uvc_video_chain *chain;
> > > > +     struct uvc_entity *unit;
> > > > +     u8 value;
> > > > +
> > > > +     /* GPIO entities are always on the first chain */
> > >
> > >
> > > s/chain/chain./
> > >
> > > > +     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > > > +     list_for_each_entry(unit, &dev->entities, list) {
> > >
> > > Can we iterate over entities in the chain instead ?
> > >
> > > I'd be actually tempted to pass the pointer to the entity as data. We
> > > would however need to add a chain pointer to uvc_entity. Not sure if
> > > it's worth it, up to you.
> > >
> > > > +             if (UVC_ENTITY_TYPE(unit) != UVC_EXT_GPIO_UNIT)
> > > > +                     continue;
> > > > +             value = gpiod_get_value(unit->gpio.gpio_privacy);
> > >
> > > Could this sleep ? Should we use a threaded IRQ ?
> >
> > get_value() should not sleep, but there are platforms that may sleep
> > to get the irq.
> >
> > In order to support them I have converted to threaded_irq in the next version
> > and called gpio_get_value_cansleep()
> >
> > > > +             uvc_ctrl_status_event(NULL, chain, unit->controls, &value);
> > > > +             return IRQ_HANDLED;
> > > > +     }
> > > > +
> > > > +     return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +static int uvc_parse_gpio(struct uvc_device *dev)
> > >
> > > How about making the naming scheme consistent, by using a uvc_gpio_
> > > prefix for all four functions ?
> > >
> > > > +{
> > > > +     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;
> > >
> > > This could be folded in
> > >
> > >         if (IS_ERR_OR_NULL(gpio_privacy))
> > >                 return PTR_ERR_OR_ZERO(gpio_privacy);
> > >
> > > Up to you.
> > >
> > > > +
> > > > +     unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > > > +     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");
> > >
> > > Other unit names don't contain "Unit", should this be just "GPIO" ?
> > >
> > > > +
> > > > +     list_add_tail(&unit->list, &dev->entities);
> > > > +
> > > > +     irq = gpiod_to_irq(gpio_privacy);
> > > > +     if (irq == -EPROBE_DEFER)
> > > > +             return -EPROBE_DEFER;
> > >
> > > Can this happen in practice ?
> >
> > Yes :(, Eg:
> > If the irq is handled by an GPIO expansion via I2C, and it has not
> > been probed yet.
>
> Won't the devm_gpiod_get_optional() call fail with -EPROBE_DEFER in that
> case ?

One thing is the gpio, and the other is the irq, the irq can also be
chained to another gpio that produces the irq...

Probably overkilled, but it is just two lines.

>
> > > > +
> > > > +     if (irq < 0)
> > > > +             return 0;
> > >
> > > So this will succeed, with the GPIO unit created, but it won't be
> > > operational. Is this desired ? Shouldn't we at least print a warning ?
> >
> > If there is no IRQ, the unit is still functional, but instead of
> > sending events when the gpio changes, the user will have to poll it.
>
> Good point.
>
> > > I also wonder if we should create the GPIO unit in this cases. Wouldn't
> > > it be more confusing for userspace to see the privacy control being
> > > exposed, but without it being functional ? Maybe the call to
> > > gpiod_to_irq should be moved to before uvc_alloc_entity() ?
> > >
> > > > +
> > > > +     ret = devm_request_irq(&dev->udev->dev, irq, uvc_privacy_gpio_irq,
> > > > +                            IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> > > > +                            "uvc_privacy_gpio", dev);
> > > > +     if (ret < 0)
> > > > +             dev_warn(&dev->udev->dev,
> > > > +                    "Unable to request uvc_privacy_gpio irq. Continuing\n");
> > >
> > > s/uvc_privacy_gpio irq/privacy GPIO IRQ/ ?
> > >
> > > There's also a race condition, as soon as the IRQ is requested, it could
> > > fire, and the driver isn't fully initialized. Walking the chain in the
> > > IRQ handler while entities are added to the chain could lead to a crash.
> > > I see two options, either adding a flag to tell that initialization is
> > > complete and returning from the interrupt handler when the flag isn't
> > > set, or moving IRQ registration to a separate function, called later
> > > during the initialization. I think I have a preference for the latter.
> > >
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > >  /* ------------------------------------------------------------------------
> > > >   * UVC device scan
> > > >   */
> > > > @@ -1917,6 +2009,7 @@ static int uvc_scan_device(struct uvc_device *dev)
> > > >  {
> > > >       struct uvc_video_chain *chain;
> > > >       struct uvc_entity *term;
> > > > +     struct uvc_entity *unit;
> > > >
> > > >       list_for_each_entry(term, &dev->entities, list) {
> > > >               if (!UVC_ENTITY_IS_OTERM(term))
> > > > @@ -1955,6 +2048,13 @@ static int uvc_scan_device(struct uvc_device *dev)
> > > >               return -1;
> > > >       }
> > > >
> > > > +     /* Add GPIO entities to the first chain */
> > >
> > > s/chain/chain./
> > >
> > > > +     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > > > +     list_for_each_entry(unit, &dev->entities, list) {
> > > > +             if (UVC_ENTITY_TYPE(unit) == UVC_EXT_GPIO_UNIT)
> > > > +                     list_add_tail(&unit->chain, &chain->entities);
> > > > +     }
> > > > +
> > > >       return 0;
> > > >  }
> > > >
> > > > @@ -2287,6 +2387,12 @@ static int uvc_probe(struct usb_interface *intf,
> > > >               goto error;
> > > >       }
> > > >
> > > > +     /* Parse the associated GPIOs */
> > >
> > > s/GPIOs/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 ae464ba83f4f..f87d14fb3f56 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>
> > >
> > > We can use a forward declaration of struct gpio_desc in this file.
> > >
> > > >  #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
> > >
> > > Maybe a couple of tabs to align this with the other defines above ?
> > >
> > > >
> > > >  /* ------------------------------------------------------------------------
> > > >   * 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, \
> > > > @@ -348,6 +354,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

Re: [PATCH v4 9/9] media: uvcvideo: Implement UVC_QUIRK_PRIVACY_DURING_STREAM

[Ricardo Ribalda]

HI

On Sun, Dec 20, 2020 at 6:22 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Tue, Dec 15, 2020 at 04:44:39PM +0100, Ricardo Ribalda wrote:
> > Some devices, can only read the privacy_pin if the device is
> > streaming.
>
> :-(
:"-(


>
> > This patch implement a quirk for such devices, in order to avoid invalid
> > reads and/or spurious events.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 97 ++++++++++++++++++++++++++----
> >  drivers/media/usb/uvc/uvc_queue.c  |  3 +
> >  drivers/media/usb/uvc/uvcvideo.h   |  6 ++
> >  3 files changed, 94 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index e49491250e87..61313019e226 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/dmi.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/kernel.h>
> >  #include <linux/list.h>
> > @@ -1471,13 +1472,39 @@ static int uvc_parse_control(struct uvc_device *dev)
> >       return 0;
> >  }
> >
> > +static bool uvc_ext_gpio_is_streaming(struct uvc_device *dev)
> > +{
> > +     struct uvc_streaming *streaming;
> > +
> > +     list_for_each_entry(streaming, &dev->streams, list) {
> > +             if (uvc_queue_streaming(&streaming->queue))
> > +                     return true;
> > +     }
> > +
> > +     return false;
> > +}
> > +
> > +/* Update the cached value and return true if it has changed */
> > +static bool uvc_gpio_update_value(struct uvc_entity *unit, u8 *new_val)
> > +{
> > +     *new_val = gpiod_get_value(unit->gpio.gpio_privacy);
> > +
> > +     return atomic_xchg(&unit->gpio.gpio_privacy_value, *new_val) !=
> > +                                                                   *new_val;
>
> That's a weird indentation. Also, as the left hand side modifies
> *new_val, does C guarantee the order in which the two operands to != are
> evaluated ? Could the code be written in an easier to read way ?
>
> > +}
> > +
> >  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;
> >
> > -     *(uint8_t *)data = gpiod_get_value(entity->gpio.gpio_privacy);
> > +     if ((dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) &&
> > +         !uvc_ext_gpio_is_streaming(dev))
> > +             return -EBUSY;
> > +
> > +     uvc_gpio_update_value(entity, (uint8_t *)data);
> > +
> >       return 0;
> >  }
> >
> > @@ -1491,26 +1518,69 @@ static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> >       return 0;
> >  }
> >
> > -static irqreturn_t uvc_privacy_gpio_irq(int irq, void *data)
> > +static struct uvc_entity *uvc_find_ext_gpio_unit(struct uvc_device *dev)
> >  {
> > -     struct uvc_device *dev = data;
> > -     struct uvc_video_chain *chain;
> >       struct uvc_entity *unit;
> > -     u8 value;
> >
> > -     /* GPIO entities are always on the first chain */
> > -     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> >       list_for_each_entry(unit, &dev->entities, list) {
> > -             if (UVC_ENTITY_TYPE(unit) != UVC_EXT_GPIO_UNIT)
> > -                     continue;
> > -             value = gpiod_get_value(unit->gpio.gpio_privacy);
> > -             uvc_ctrl_status_event(NULL, chain, unit->controls, &value);
> > -             return IRQ_HANDLED;
> > +             if (UVC_ENTITY_TYPE(unit) == UVC_EXT_GPIO_UNIT)
> > +                     return unit;
> >       }
> >
> > +     return unit;
> > +}
> > +
> > +void uvc_privacy_gpio_event(struct uvc_device *dev)
> > +{
> > +     struct uvc_entity *unit;
> > +     struct uvc_video_chain *chain;
> > +     u8 new_value;
> > +
> > +     unit = uvc_find_ext_gpio_unit(dev);
> > +     if (WARN_ONCE(!unit, "Unable to find entity ext_gpio_unit"))
> > +             return;
> > +
> > +     if (!uvc_gpio_update_value(unit, &new_value))
> > +             return;
>
> If VIDIOC_G_CTRL() is called before the IRQ is processed, this
> uvc_gpio_update_value() call will return false, and no event will be
> generated. I don't think that's right, and even should be generated
> every time the control changes.
>
I was almost sure that get_cur had also the events wired.... but no.

> > +
> > +     /* GPIO entities are always on the first chain */
> > +     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > +     uvc_ctrl_status_event(NULL, chain, unit->controls, &new_value);
> > +}
> > +
> > +static irqreturn_t uvc_privacy_gpio_irq(int irq, void *data)
> > +{
> > +     struct uvc_device *dev = data;
> > +
> > +     /* Ignore privacy events during streamoff */
> > +     if (dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM)
> > +             if (!uvc_ext_gpio_is_streaming(dev))
> > +                     return IRQ_HANDLED;
>
>         if (dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) {
>                 if (!uvc_ext_gpio_is_streaming(dev))
>                         return IRQ_HANDLED;
>         }
>
> There's a potential race condition with VIDIOC_STREAMON and
> VIDIOC_STREAMOFF. Could you explain what the device does exactly when
> not streaming ? As the GPIO isn't tied to the UVC controller, how comes
> the streaming state influences it ? Any hope the firmware could be fixed
> instead ?

In the affected devices, the privacy_pin is an output of the camera
module instead of an independent pin.

When the camera is not streaming, the camera does not drive the pin,
so the system reads whatever pull-up, pull-down the pin is configured
by default in the firmware/hardware.

Unfortunately the only way to fix it would be to change the module, or
the firmware of the module, and neigher things are not feasable :(. So
we have to use a quirk for them.
Future models have this fixed.

Regarding the race condition... The use of the atomic_t was to avoid
that race, but what I did not realise was that by default get_cur does
not send an event... So I am working on a new series, which also
includes a fix for the async_control wq.

Thanks for your review!!!

>
> > +
> > +     uvc_privacy_gpio_event(dev);
> > +
> >       return IRQ_HANDLED;
> >  }
> >
> > +static const struct dmi_system_id privacy_valid_during_streamon[] = {
> > +     {
> > +             .ident = "HP Elite c1030 Chromebook",
> > +             .matches = {
> > +                     DMI_MATCH(DMI_SYS_VENDOR, "HP"),
> > +                     DMI_MATCH(DMI_PRODUCT_NAME, "Jinlon"),
> > +             },
> > +     },
> > +     {
> > +             .ident = "HP Pro c640 Chromebook",
> > +             .matches = {
> > +                     DMI_MATCH(DMI_SYS_VENDOR, "HP"),
> > +                     DMI_MATCH(DMI_PRODUCT_NAME, "Dratini"),
> > +             },
> > +     },
> > +     { } /* terminate list */
> > +};
> > +
> > +
> >  static int uvc_parse_gpio(struct uvc_device *dev)
> >  {
> >       struct uvc_entity *unit;
> > @@ -1545,6 +1615,9 @@ static int uvc_parse_gpio(struct uvc_device *dev)
> >       if (irq == -EPROBE_DEFER)
> >               return -EPROBE_DEFER;
> >
> > +     if (dmi_check_system(privacy_valid_during_streamon))
> > +             dev->quirks |= UVC_QUIRK_PRIVACY_DURING_STREAM;
> > +
> >       if (irq < 0)
> >               return 0;
> >
> > diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> > index cd60c6c1749e..e800d491303f 100644
> > --- a/drivers/media/usb/uvc/uvc_queue.c
> > +++ b/drivers/media/usb/uvc/uvc_queue.c
> > @@ -337,9 +337,12 @@ int uvc_dequeue_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf,
> >  int uvc_queue_streamon(struct uvc_video_queue *queue, enum v4l2_buf_type type)
> >  {
> >       int ret;
> > +     struct uvc_streaming *stream = uvc_queue_to_stream(queue);
> >
> >       mutex_lock(&queue->mutex);
> >       ret = vb2_streamon(&queue->queue, type);
> > +     if (stream->dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM)
> > +             uvc_privacy_gpio_event(stream->dev);
> >       mutex_unlock(&queue->mutex);
> >
> >       return ret;
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 2b5ba4b02d3a..2a95b3ed3ea8 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/gpio/consumer.h>
> >  #include <linux/kernel.h>
> >  #include <linux/poll.h>
> > @@ -209,6 +210,7 @@
> >  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT      0x00000400
> >  #define UVC_QUIRK_FORCE_Y8           0x00000800
> >  #define UVC_QUIRK_FORCE_BPP          0x00001000
> > +#define UVC_QUIRK_PRIVACY_DURING_STREAM      0x00002000
> >
> >  /* Format flags */
> >  #define UVC_FMT_FLAG_COMPRESSED              0x00000001
> > @@ -359,6 +361,7 @@ struct uvc_entity {
> >                       u8  bControlSize;
> >                       u8  *bmControls;
> >                       struct gpio_desc *gpio_privacy;
> > +                     atomic_t  gpio_privacy_value;
> >               } gpio;
> >       };
> >
> > @@ -815,6 +818,9 @@ extern const struct v4l2_file_operations uvc_fops;
> >  int uvc_mc_register_entities(struct uvc_video_chain *chain);
> >  void uvc_mc_cleanup_entity(struct uvc_entity *entity);
> >
> > +/* Privacy gpio */
> > +void uvc_privacy_gpio_event(struct uvc_device *dev);
> > +
> >  /* Video */
> >  int uvc_video_init(struct uvc_streaming *stream);
> >  int uvc_video_suspend(struct uvc_streaming *stream);
>
> --
> Regards,
>
> Laurent Pinchart



--
Ricardo Ribalda

Re: [PATCH v4 9/9] media: uvcvideo: Implement UVC_QUIRK_PRIVACY_DURING_STREAM

[Laurent Pinchart]

Hi Ricardo,

On Mon, Dec 21, 2020 at 02:10:18AM +0100, Ricardo Ribalda wrote:
> On Sun, Dec 20, 2020 at 6:22 PM Laurent Pinchart wrote:
> > On Tue, Dec 15, 2020 at 04:44:39PM +0100, Ricardo Ribalda wrote:
> > > Some devices, can only read the privacy_pin if the device is
> > > streaming.
> >
> > :-(
> 
> :"-(
> 
> > > This patch implement a quirk for such devices, in order to avoid invalid
> > > reads and/or spurious events.
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_driver.c | 97 ++++++++++++++++++++++++++----
> > >  drivers/media/usb/uvc/uvc_queue.c  |  3 +
> > >  drivers/media/usb/uvc/uvcvideo.h   |  6 ++
> > >  3 files changed, 94 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index e49491250e87..61313019e226 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/dmi.h>
> > >  #include <linux/gpio/consumer.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/list.h>
> > > @@ -1471,13 +1472,39 @@ static int uvc_parse_control(struct uvc_device *dev)
> > >       return 0;
> > >  }
> > >
> > > +static bool uvc_ext_gpio_is_streaming(struct uvc_device *dev)
> > > +{
> > > +     struct uvc_streaming *streaming;
> > > +
> > > +     list_for_each_entry(streaming, &dev->streams, list) {
> > > +             if (uvc_queue_streaming(&streaming->queue))
> > > +                     return true;
> > > +     }
> > > +
> > > +     return false;
> > > +}
> > > +
> > > +/* Update the cached value and return true if it has changed */
> > > +static bool uvc_gpio_update_value(struct uvc_entity *unit, u8 *new_val)
> > > +{
> > > +     *new_val = gpiod_get_value(unit->gpio.gpio_privacy);
> > > +
> > > +     return atomic_xchg(&unit->gpio.gpio_privacy_value, *new_val) !=
> > > +                                                                   *new_val;
> >
> > That's a weird indentation. Also, as the left hand side modifies
> > *new_val, does C guarantee the order in which the two operands to != are
> > evaluated ? Could the code be written in an easier to read way ?
> >
> > > +}
> > > +
> > >  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;
> > >
> > > -     *(uint8_t *)data = gpiod_get_value(entity->gpio.gpio_privacy);
> > > +     if ((dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) &&
> > > +         !uvc_ext_gpio_is_streaming(dev))
> > > +             return -EBUSY;
> > > +
> > > +     uvc_gpio_update_value(entity, (uint8_t *)data);
> > > +
> > >       return 0;
> > >  }
> > >
> > > @@ -1491,26 +1518,69 @@ static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> > >       return 0;
> > >  }
> > >
> > > -static irqreturn_t uvc_privacy_gpio_irq(int irq, void *data)
> > > +static struct uvc_entity *uvc_find_ext_gpio_unit(struct uvc_device *dev)
> > >  {
> > > -     struct uvc_device *dev = data;
> > > -     struct uvc_video_chain *chain;
> > >       struct uvc_entity *unit;
> > > -     u8 value;
> > >
> > > -     /* GPIO entities are always on the first chain */
> > > -     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > >       list_for_each_entry(unit, &dev->entities, list) {
> > > -             if (UVC_ENTITY_TYPE(unit) != UVC_EXT_GPIO_UNIT)
> > > -                     continue;
> > > -             value = gpiod_get_value(unit->gpio.gpio_privacy);
> > > -             uvc_ctrl_status_event(NULL, chain, unit->controls, &value);
> > > -             return IRQ_HANDLED;
> > > +             if (UVC_ENTITY_TYPE(unit) == UVC_EXT_GPIO_UNIT)
> > > +                     return unit;
> > >       }
> > >
> > > +     return unit;
> > > +}
> > > +
> > > +void uvc_privacy_gpio_event(struct uvc_device *dev)
> > > +{
> > > +     struct uvc_entity *unit;
> > > +     struct uvc_video_chain *chain;
> > > +     u8 new_value;
> > > +
> > > +     unit = uvc_find_ext_gpio_unit(dev);
> > > +     if (WARN_ONCE(!unit, "Unable to find entity ext_gpio_unit"))
> > > +             return;
> > > +
> > > +     if (!uvc_gpio_update_value(unit, &new_value))
> > > +             return;
> >
> > If VIDIOC_G_CTRL() is called before the IRQ is processed, this
> > uvc_gpio_update_value() call will return false, and no event will be
> > generated. I don't think that's right, and even should be generated
> > every time the control changes.
> >
> I was almost sure that get_cur had also the events wired.... but no.
> 
> > > +
> > > +     /* GPIO entities are always on the first chain */
> > > +     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > > +     uvc_ctrl_status_event(NULL, chain, unit->controls, &new_value);
> > > +}
> > > +
> > > +static irqreturn_t uvc_privacy_gpio_irq(int irq, void *data)
> > > +{
> > > +     struct uvc_device *dev = data;
> > > +
> > > +     /* Ignore privacy events during streamoff */
> > > +     if (dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM)
> > > +             if (!uvc_ext_gpio_is_streaming(dev))
> > > +                     return IRQ_HANDLED;
> >
> >         if (dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) {
> >                 if (!uvc_ext_gpio_is_streaming(dev))
> >                         return IRQ_HANDLED;
> >         }
> >
> > There's a potential race condition with VIDIOC_STREAMON and
> > VIDIOC_STREAMOFF. Could you explain what the device does exactly when
> > not streaming ? As the GPIO isn't tied to the UVC controller, how comes
> > the streaming state influences it ? Any hope the firmware could be fixed
> > instead ?
> 
> In the affected devices, the privacy_pin is an output of the camera
> module instead of an independent pin.

So the privacy switch is an input of a camera module, which then output
its state on a GPIO instead of exposing it through the UVC privacy
control ? Amazing design !

> When the camera is not streaming, the camera does not drive the pin,
> so the system reads whatever pull-up, pull-down the pin is configured
> by default in the firmware/hardware.
> 
> Unfortunately the only way to fix it would be to change the module, or
> the firmware of the module, and neigher things are not feasable :(. So
> we have to use a quirk for them.
> Future models have this fixed.

Changing the hardware, obviously. Changing the firmware of the module...
Someone needs to be scolded along the development chain, can you take
care of that at least ? ;-)

Thinking more about this, do you have a use case for knowing the state
of the privacy switch when not streaming ?

> Regarding the race condition... The use of the atomic_t was to avoid
> that race, but what I did not realise was that by default get_cur does
> not send an event... So I am working on a new series, which also
> includes a fix for the async_control wq.
> 
> Thanks for your review!!!
> 
> > > +
> > > +     uvc_privacy_gpio_event(dev);
> > > +
> > >       return IRQ_HANDLED;
> > >  }
> > >
> > > +static const struct dmi_system_id privacy_valid_during_streamon[] = {
> > > +     {
> > > +             .ident = "HP Elite c1030 Chromebook",
> > > +             .matches = {
> > > +                     DMI_MATCH(DMI_SYS_VENDOR, "HP"),
> > > +                     DMI_MATCH(DMI_PRODUCT_NAME, "Jinlon"),
> > > +             },
> > > +     },
> > > +     {
> > > +             .ident = "HP Pro c640 Chromebook",
> > > +             .matches = {
> > > +                     DMI_MATCH(DMI_SYS_VENDOR, "HP"),
> > > +                     DMI_MATCH(DMI_PRODUCT_NAME, "Dratini"),
> > > +             },
> > > +     },
> > > +     { } /* terminate list */
> > > +};
> > > +
> > > +
> > >  static int uvc_parse_gpio(struct uvc_device *dev)
> > >  {
> > >       struct uvc_entity *unit;
> > > @@ -1545,6 +1615,9 @@ static int uvc_parse_gpio(struct uvc_device *dev)
> > >       if (irq == -EPROBE_DEFER)
> > >               return -EPROBE_DEFER;
> > >
> > > +     if (dmi_check_system(privacy_valid_during_streamon))
> > > +             dev->quirks |= UVC_QUIRK_PRIVACY_DURING_STREAM;
> > > +
> > >       if (irq < 0)
> > >               return 0;
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> > > index cd60c6c1749e..e800d491303f 100644
> > > --- a/drivers/media/usb/uvc/uvc_queue.c
> > > +++ b/drivers/media/usb/uvc/uvc_queue.c
> > > @@ -337,9 +337,12 @@ int uvc_dequeue_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf,
> > >  int uvc_queue_streamon(struct uvc_video_queue *queue, enum v4l2_buf_type type)
> > >  {
> > >       int ret;
> > > +     struct uvc_streaming *stream = uvc_queue_to_stream(queue);
> > >
> > >       mutex_lock(&queue->mutex);
> > >       ret = vb2_streamon(&queue->queue, type);
> > > +     if (stream->dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM)
> > > +             uvc_privacy_gpio_event(stream->dev);
> > >       mutex_unlock(&queue->mutex);
> > >
> > >       return ret;
> > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > index 2b5ba4b02d3a..2a95b3ed3ea8 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/gpio/consumer.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/poll.h>
> > > @@ -209,6 +210,7 @@
> > >  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT      0x00000400
> > >  #define UVC_QUIRK_FORCE_Y8           0x00000800
> > >  #define UVC_QUIRK_FORCE_BPP          0x00001000
> > > +#define UVC_QUIRK_PRIVACY_DURING_STREAM      0x00002000
> > >
> > >  /* Format flags */
> > >  #define UVC_FMT_FLAG_COMPRESSED              0x00000001
> > > @@ -359,6 +361,7 @@ struct uvc_entity {
> > >                       u8  bControlSize;
> > >                       u8  *bmControls;
> > >                       struct gpio_desc *gpio_privacy;
> > > +                     atomic_t  gpio_privacy_value;
> > >               } gpio;
> > >       };
> > >
> > > @@ -815,6 +818,9 @@ extern const struct v4l2_file_operations uvc_fops;
> > >  int uvc_mc_register_entities(struct uvc_video_chain *chain);
> > >  void uvc_mc_cleanup_entity(struct uvc_entity *entity);
> > >
> > > +/* Privacy gpio */
> > > +void uvc_privacy_gpio_event(struct uvc_device *dev);
> > > +
> > >  /* Video */
> > >  int uvc_video_init(struct uvc_streaming *stream);
> > >  int uvc_video_suspend(struct uvc_streaming *stream);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 9/9] media: uvcvideo: Implement UVC_QUIRK_PRIVACY_DURING_STREAM

[Ricardo Ribalda]

Hi Laurent

On Mon, Dec 21, 2020 at 3:08 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> On Mon, Dec 21, 2020 at 02:10:18AM +0100, Ricardo Ribalda wrote:
> > On Sun, Dec 20, 2020 at 6:22 PM Laurent Pinchart wrote:
> > > On Tue, Dec 15, 2020 at 04:44:39PM +0100, Ricardo Ribalda wrote:
> > > > Some devices, can only read the privacy_pin if the device is
> > > > streaming.
> > >
> > > :-(
> >
> > :"-(
> >
> > > > This patch implement a quirk for such devices, in order to avoid invalid
> > > > reads and/or spurious events.
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_driver.c | 97 ++++++++++++++++++++++++++----
> > > >  drivers/media/usb/uvc/uvc_queue.c  |  3 +
> > > >  drivers/media/usb/uvc/uvcvideo.h   |  6 ++
> > > >  3 files changed, 94 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > index e49491250e87..61313019e226 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/dmi.h>
> > > >  #include <linux/gpio/consumer.h>
> > > >  #include <linux/kernel.h>
> > > >  #include <linux/list.h>
> > > > @@ -1471,13 +1472,39 @@ static int uvc_parse_control(struct uvc_device *dev)
> > > >       return 0;
> > > >  }
> > > >
> > > > +static bool uvc_ext_gpio_is_streaming(struct uvc_device *dev)
> > > > +{
> > > > +     struct uvc_streaming *streaming;
> > > > +
> > > > +     list_for_each_entry(streaming, &dev->streams, list) {
> > > > +             if (uvc_queue_streaming(&streaming->queue))
> > > > +                     return true;
> > > > +     }
> > > > +
> > > > +     return false;
> > > > +}
> > > > +
> > > > +/* Update the cached value and return true if it has changed */
> > > > +static bool uvc_gpio_update_value(struct uvc_entity *unit, u8 *new_val)
> > > > +{
> > > > +     *new_val = gpiod_get_value(unit->gpio.gpio_privacy);
> > > > +
> > > > +     return atomic_xchg(&unit->gpio.gpio_privacy_value, *new_val) !=
> > > > +                                                                   *new_val;
> > >
> > > That's a weird indentation. Also, as the left hand side modifies
> > > *new_val, does C guarantee the order in which the two operands to != are
> > > evaluated ? Could the code be written in an easier to read way ?
> > >
> > > > +}
> > > > +
> > > >  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;
> > > >
> > > > -     *(uint8_t *)data = gpiod_get_value(entity->gpio.gpio_privacy);
> > > > +     if ((dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) &&
> > > > +         !uvc_ext_gpio_is_streaming(dev))
> > > > +             return -EBUSY;
> > > > +
> > > > +     uvc_gpio_update_value(entity, (uint8_t *)data);
> > > > +
> > > >       return 0;
> > > >  }
> > > >
> > > > @@ -1491,26 +1518,69 @@ static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> > > >       return 0;
> > > >  }
> > > >
> > > > -static irqreturn_t uvc_privacy_gpio_irq(int irq, void *data)
> > > > +static struct uvc_entity *uvc_find_ext_gpio_unit(struct uvc_device *dev)
> > > >  {
> > > > -     struct uvc_device *dev = data;
> > > > -     struct uvc_video_chain *chain;
> > > >       struct uvc_entity *unit;
> > > > -     u8 value;
> > > >
> > > > -     /* GPIO entities are always on the first chain */
> > > > -     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > > >       list_for_each_entry(unit, &dev->entities, list) {
> > > > -             if (UVC_ENTITY_TYPE(unit) != UVC_EXT_GPIO_UNIT)
> > > > -                     continue;
> > > > -             value = gpiod_get_value(unit->gpio.gpio_privacy);
> > > > -             uvc_ctrl_status_event(NULL, chain, unit->controls, &value);
> > > > -             return IRQ_HANDLED;
> > > > +             if (UVC_ENTITY_TYPE(unit) == UVC_EXT_GPIO_UNIT)
> > > > +                     return unit;
> > > >       }
> > > >
> > > > +     return unit;
> > > > +}
> > > > +
> > > > +void uvc_privacy_gpio_event(struct uvc_device *dev)
> > > > +{
> > > > +     struct uvc_entity *unit;
> > > > +     struct uvc_video_chain *chain;
> > > > +     u8 new_value;
> > > > +
> > > > +     unit = uvc_find_ext_gpio_unit(dev);
> > > > +     if (WARN_ONCE(!unit, "Unable to find entity ext_gpio_unit"))
> > > > +             return;
> > > > +
> > > > +     if (!uvc_gpio_update_value(unit, &new_value))
> > > > +             return;
> > >
> > > If VIDIOC_G_CTRL() is called before the IRQ is processed, this
> > > uvc_gpio_update_value() call will return false, and no event will be
> > > generated. I don't think that's right, and even should be generated
> > > every time the control changes.
> > >
> > I was almost sure that get_cur had also the events wired.... but no.
> >
> > > > +
> > > > +     /* GPIO entities are always on the first chain */
> > > > +     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > > > +     uvc_ctrl_status_event(NULL, chain, unit->controls, &new_value);
> > > > +}
> > > > +
> > > > +static irqreturn_t uvc_privacy_gpio_irq(int irq, void *data)
> > > > +{
> > > > +     struct uvc_device *dev = data;
> > > > +
> > > > +     /* Ignore privacy events during streamoff */
> > > > +     if (dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM)
> > > > +             if (!uvc_ext_gpio_is_streaming(dev))
> > > > +                     return IRQ_HANDLED;
> > >
> > >         if (dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) {
> > >                 if (!uvc_ext_gpio_is_streaming(dev))
> > >                         return IRQ_HANDLED;
> > >         }
> > >
> > > There's a potential race condition with VIDIOC_STREAMON and
> > > VIDIOC_STREAMOFF. Could you explain what the device does exactly when
> > > not streaming ? As the GPIO isn't tied to the UVC controller, how comes
> > > the streaming state influences it ? Any hope the firmware could be fixed
> > > instead ?
> >
> > In the affected devices, the privacy_pin is an output of the camera
> > module instead of an independent pin.
>
> So the privacy switch is an input of a camera module, which then output
> its state on a GPIO instead of exposing it through the UVC privacy
> control ? Amazing design !

Dont kill the messenger...

>
> > When the camera is not streaming, the camera does not drive the pin,
> > so the system reads whatever pull-up, pull-down the pin is configured
> > by default in the firmware/hardware.
> >
> > Unfortunately the only way to fix it would be to change the module, or
> > the firmware of the module, and neigher things are not feasable :(. So
> > we have to use a quirk for them.
> > Future models have this fixed.
>
> Changing the hardware, obviously. Changing the firmware of the module...
> Someone needs to be scolded along the development chain, can you take
> care of that at least ? ;-)

Will do my best ;)

>
> Thinking more about this, do you have a use case for knowing the state
> of the privacy switch when not streaming ?

Notify user space that the switch has toggled, to provide visual
feedback of the action.

>
> > Regarding the race condition... The use of the atomic_t was to avoid
> > that race, but what I did not realise was that by default get_cur does
> > not send an event... So I am working on a new series, which also
> > includes a fix for the async_control wq.
> >
> > Thanks for your review!!!
> >
> > > > +
> > > > +     uvc_privacy_gpio_event(dev);
> > > > +
> > > >       return IRQ_HANDLED;
> > > >  }
> > > >
> > > > +static const struct dmi_system_id privacy_valid_during_streamon[] = {
> > > > +     {
> > > > +             .ident = "HP Elite c1030 Chromebook",
> > > > +             .matches = {
> > > > +                     DMI_MATCH(DMI_SYS_VENDOR, "HP"),
> > > > +                     DMI_MATCH(DMI_PRODUCT_NAME, "Jinlon"),
> > > > +             },
> > > > +     },
> > > > +     {
> > > > +             .ident = "HP Pro c640 Chromebook",
> > > > +             .matches = {
> > > > +                     DMI_MATCH(DMI_SYS_VENDOR, "HP"),
> > > > +                     DMI_MATCH(DMI_PRODUCT_NAME, "Dratini"),
> > > > +             },
> > > > +     },
> > > > +     { } /* terminate list */
> > > > +};
> > > > +
> > > > +
> > > >  static int uvc_parse_gpio(struct uvc_device *dev)
> > > >  {
> > > >       struct uvc_entity *unit;
> > > > @@ -1545,6 +1615,9 @@ static int uvc_parse_gpio(struct uvc_device *dev)
> > > >       if (irq == -EPROBE_DEFER)
> > > >               return -EPROBE_DEFER;
> > > >
> > > > +     if (dmi_check_system(privacy_valid_during_streamon))
> > > > +             dev->quirks |= UVC_QUIRK_PRIVACY_DURING_STREAM;
> > > > +
> > > >       if (irq < 0)
> > > >               return 0;
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> > > > index cd60c6c1749e..e800d491303f 100644
> > > > --- a/drivers/media/usb/uvc/uvc_queue.c
> > > > +++ b/drivers/media/usb/uvc/uvc_queue.c
> > > > @@ -337,9 +337,12 @@ int uvc_dequeue_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf,
> > > >  int uvc_queue_streamon(struct uvc_video_queue *queue, enum v4l2_buf_type type)
> > > >  {
> > > >       int ret;
> > > > +     struct uvc_streaming *stream = uvc_queue_to_stream(queue);
> > > >
> > > >       mutex_lock(&queue->mutex);
> > > >       ret = vb2_streamon(&queue->queue, type);
> > > > +     if (stream->dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM)
> > > > +             uvc_privacy_gpio_event(stream->dev);
> > > >       mutex_unlock(&queue->mutex);
> > > >
> > > >       return ret;
> > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > index 2b5ba4b02d3a..2a95b3ed3ea8 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/gpio/consumer.h>
> > > >  #include <linux/kernel.h>
> > > >  #include <linux/poll.h>
> > > > @@ -209,6 +210,7 @@
> > > >  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT      0x00000400
> > > >  #define UVC_QUIRK_FORCE_Y8           0x00000800
> > > >  #define UVC_QUIRK_FORCE_BPP          0x00001000
> > > > +#define UVC_QUIRK_PRIVACY_DURING_STREAM      0x00002000
> > > >
> > > >  /* Format flags */
> > > >  #define UVC_FMT_FLAG_COMPRESSED              0x00000001
> > > > @@ -359,6 +361,7 @@ struct uvc_entity {
> > > >                       u8  bControlSize;
> > > >                       u8  *bmControls;
> > > >                       struct gpio_desc *gpio_privacy;
> > > > +                     atomic_t  gpio_privacy_value;
> > > >               } gpio;
> > > >       };
> > > >
> > > > @@ -815,6 +818,9 @@ extern const struct v4l2_file_operations uvc_fops;
> > > >  int uvc_mc_register_entities(struct uvc_video_chain *chain);
> > > >  void uvc_mc_cleanup_entity(struct uvc_entity *entity);
> > > >
> > > > +/* Privacy gpio */
> > > > +void uvc_privacy_gpio_event(struct uvc_device *dev);
> > > > +
> > > >  /* Video */
> > > >  int uvc_video_init(struct uvc_streaming *stream);
> > > >  int uvc_video_suspend(struct uvc_streaming *stream);
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda