[PATCH v2 0/7] Show privacy_gpio as a v4l2_ctrl

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

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 (7):
  media: uvcvideo: Use pr_cont() macro
  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

 drivers/media/usb/uvc/uvc_ctrl.c   |  73 ++++++++-----
 drivers/media/usb/uvc/uvc_driver.c | 166 +++++++++++++++++++++++++----
 drivers/media/usb/uvc/uvcvideo.h   |  19 +++-
 3 files changed, 207 insertions(+), 51 deletions(-)

-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH v2 1/7] media: uvcvideo: Use pr_cont() macro

[Ricardo Ribalda]

Replace all the uses of printk(KERN_CONT ... with pr_cont().

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

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index ddb9eaa11be7..9fc0b600eab1 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1476,7 +1476,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 "
@@ -1488,7 +1488,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 "
@@ -1501,7 +1501,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)
@@ -1520,7 +1520,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;
 
@@ -1528,17 +1528,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;
@@ -1588,9 +1588,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;
@@ -1608,16 +1608,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;
 }
@@ -1643,7 +1643,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) {
@@ -1664,14 +1664,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;
-- 
2.29.1.341.ge80a0c044ae-goog

Re: [PATCH v2 1/7] media: uvcvideo: Use pr_cont() macro

[Joe Perches]

On Wed, 2020-11-04 at 19:07 +0100, Ricardo Ribalda wrote:
> Replace all the uses of printk(KERN_CONT ... with pr_cont().

Perhaps remove the uvc_printk macro and uses and use the more
common pr_fmt and pr_<level> mechanisms.

Something like:
---
 drivers/media/usb/uvc/uvc_ctrl.c   |  5 +++--
 drivers/media/usb/uvc/uvc_driver.c | 35 +++++++++++++++------------------
 drivers/media/usb/uvc/uvc_entity.c | 10 ++++++----
 drivers/media/usb/uvc/uvc_status.c |  9 +++++----
 drivers/media/usb/uvc/uvc_video.c  | 40 ++++++++++++++++----------------------
 drivers/media/usb/uvc/uvcvideo.h   | 25 +++++++++++-------------
 6 files changed, 58 insertions(+), 66 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index f479d8971dfb..e0b273697d49 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -6,6 +6,8 @@
  *          Laurent Pinchart (laurent.pinchart@ideasonboard.com)
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
@@ -1317,8 +1319,7 @@ 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);
+		pr_err("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 ddb9eaa11be7..03d68e2b6b70 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -6,6 +6,8 @@
  *          Laurent Pinchart (laurent.pinchart@ideasonboard.com)
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/atomic.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
@@ -534,8 +536,7 @@ 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]);
+			pr_info("Unknown video format %pUl\n", &buffer[5]);
 			snprintf(format->name, sizeof(format->name), "%pUl\n",
 				&buffer[5]);
 			format->fcc = 0;
@@ -1925,7 +1926,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");
+		pr_info("No valid video chain found\n");
 		return -1;
 	}
 
@@ -2077,8 +2078,8 @@ 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);
+		pr_err("Failed to register %s device (%d)\n",
+		       v4l2_type_names[type], ret);
 		return ret;
 	}
 
@@ -2094,8 +2095,7 @@ 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);
+		pr_err("Failed to initialize the device (%d)\n", ret);
 		return ret;
 	}
 
@@ -2129,8 +2129,8 @@ 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);
+			pr_info("No streaming interface found for terminal %u\n",
+				term->id);
 			continue;
 		}
 
@@ -2163,8 +2163,7 @@ 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);
+			pr_info("Failed to register entities (%d)\n", ret);
 #endif
 	}
 
@@ -2261,17 +2260,16 @@ static int uvc_probe(struct usb_interface *intf,
 		goto error;
 	}
 
-	uvc_printk(KERN_INFO, "Found UVC %u.%02x device %s (%04x:%04x)\n",
+	pr_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));
 
 	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");
+		pr_info("Forcing device quirks to 0x%x by module parameter for testing purpose\n",
+			dev->quirks);
+		pr_info("Please report required quirks to the linux-uvc-devel mailing list\n");
 	}
 
 	/* Register the V4L2 device. */
@@ -2300,9 +2298,8 @@ 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);
+		pr_info("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..a5c11cde273c 100644
--- a/drivers/media/usb/uvc/uvc_entity.c
+++ b/drivers/media/usb/uvc/uvc_entity.c
@@ -6,6 +6,8 @@
  *          Laurent Pinchart (laurent.pinchart@ideasonboard.com)
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/videodev2.h>
@@ -139,8 +141,8 @@ 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);
+			pr_info("Failed to initialize entity for entity %u\n",
+				entity->id);
 			return ret;
 		}
 	}
@@ -148,8 +150,8 @@ 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);
+			pr_info("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..4c84525ada71 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -6,6 +6,8 @@
  *          Laurent Pinchart (laurent.pinchart@ideasonboard.com)
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/kernel.h>
 #include <linux/input.h>
 #include <linux/slab.h>
@@ -208,8 +210,8 @@ 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);
+		pr_warn("Non-zero status (%d) in status completion handler\n",
+			urb->status);
 		return;
 	}
 
@@ -244,8 +246,7 @@ 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);
+		pr_err("Failed to resubmit status URB (%d)\n", ret);
 	}
 }
 
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index a6a441d92b94..6d48bcdeac16 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -6,6 +6,8 @@
  *          Laurent Pinchart (laurent.pinchart@ideasonboard.com)
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
@@ -76,9 +78,8 @@ 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);
+	pr_err("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 +255,8 @@ 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);
+		pr_err("Failed to query (%u) UVC %s control : %d (exp. %u)\n",
+		       query, probe ? "probe" : "commit", ret, size);
 		ret = -EIO;
 		goto out;
 	}
@@ -334,9 +334,8 @@ 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);
+		pr_err("Failed to set UVC %s control : %d (exp. %u)\n",
+		       probe ? "probe" : "commit", ret, size);
 		ret = -EIO;
 	}
 
@@ -1120,8 +1119,7 @@ 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);
+		pr_err("Failed to resubmit video URB (%d)\n", ret);
 }
 
 static void uvc_video_decode_data(struct uvc_urb *uvc_urb,
@@ -1507,8 +1505,8 @@ 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);
+		pr_warn("Non-zero status (%d) in video completion handler\n",
+			urb->status);
 		fallthrough;
 	case -ENOENT:		/* usb_poison_urb() called. */
 		if (stream->frozen)
@@ -1545,9 +1543,7 @@ 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);
+			pr_err("Failed to resubmit video URB (%d)\n", ret);
 		return;
 	}
 
@@ -1893,8 +1889,8 @@ 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);
+			pr_err("Failed to submit URB %u (%d)\n",
+			       uvc_urb_index(uvc_urb), ret);
 			uvc_video_stop_transfer(stream, 1);
 			return ret;
 		}
@@ -1989,7 +1985,7 @@ int uvc_video_init(struct uvc_streaming *stream)
 	int ret;
 
 	if (stream->nformats == 0) {
-		uvc_printk(KERN_INFO, "No supported video formats found.\n");
+		pr_info("No supported video formats found\n");
 		return -EINVAL;
 	}
 
@@ -2029,8 +2025,7 @@ 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");
+		pr_info("No frame descriptor found for the default format\n");
 		return -EINVAL;
 	}
 
@@ -2064,8 +2059,7 @@ 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");
+			pr_info("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 a3dfacf069c4..724dec20444a 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -725,20 +725,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))			\
+		pr_info(fmt, ##__VA_ARGS__);				\
+} while (0)
 
 /* --------------------------------------------------------------------------
  * Internal functions.

Re: [PATCH v2 1/7] media: uvcvideo: Use pr_cont() macro

[Laurent Pinchart]

Hi Joe,

On Wed, Nov 04, 2020 at 11:29:30AM -0800, Joe Perches wrote:
> On Wed, 2020-11-04 at 19:07 +0100, Ricardo Ribalda wrote:
> > Replace all the uses of printk(KERN_CONT ... with pr_cont().
> 
> Perhaps remove the uvc_printk macro and uses and use the more
> common pr_fmt and pr_<level> mechanisms.

I'd actually go for dev_* instead, to give some context. It's fairly
common to have multiple UVC devices connected to a system, so printing
the device name would be useful. It can still be wrapped with
uvc_printk() if we want to wrap the cast from uvc_device to a struct
device (we should actually try to get the device corresponding to the
USB interface where available, so we should use uvc_streaming->intf->dev
where possible, and fallback to uvc_device->udev->dev otherwise), or
drop the wrapper completely.

> Something like:
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c   |  5 +++--
>  drivers/media/usb/uvc/uvc_driver.c | 35 +++++++++++++++------------------
>  drivers/media/usb/uvc/uvc_entity.c | 10 ++++++----
>  drivers/media/usb/uvc/uvc_status.c |  9 +++++----
>  drivers/media/usb/uvc/uvc_video.c  | 40 ++++++++++++++++----------------------
>  drivers/media/usb/uvc/uvcvideo.h   | 25 +++++++++++-------------
>  6 files changed, 58 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index f479d8971dfb..e0b273697d49 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -6,6 +6,8 @@
>   *          Laurent Pinchart (laurent.pinchart@ideasonboard.com)
>   */
>  
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
> @@ -1317,8 +1319,7 @@ 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);
> +		pr_err("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 ddb9eaa11be7..03d68e2b6b70 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -6,6 +6,8 @@
>   *          Laurent Pinchart (laurent.pinchart@ideasonboard.com)
>   */
>  
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/atomic.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> @@ -534,8 +536,7 @@ 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]);
> +			pr_info("Unknown video format %pUl\n", &buffer[5]);
>  			snprintf(format->name, sizeof(format->name), "%pUl\n",
>  				&buffer[5]);
>  			format->fcc = 0;
> @@ -1925,7 +1926,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");
> +		pr_info("No valid video chain found\n");
>  		return -1;
>  	}
>  
> @@ -2077,8 +2078,8 @@ 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);
> +		pr_err("Failed to register %s device (%d)\n",
> +		       v4l2_type_names[type], ret);
>  		return ret;
>  	}
>  
> @@ -2094,8 +2095,7 @@ 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);
> +		pr_err("Failed to initialize the device (%d)\n", ret);
>  		return ret;
>  	}
>  
> @@ -2129,8 +2129,8 @@ 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);
> +			pr_info("No streaming interface found for terminal %u\n",
> +				term->id);
>  			continue;
>  		}
>  
> @@ -2163,8 +2163,7 @@ 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);
> +			pr_info("Failed to register entities (%d)\n", ret);
>  #endif
>  	}
>  
> @@ -2261,17 +2260,16 @@ static int uvc_probe(struct usb_interface *intf,
>  		goto error;
>  	}
>  
> -	uvc_printk(KERN_INFO, "Found UVC %u.%02x device %s (%04x:%04x)\n",
> +	pr_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));
>  
>  	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");
> +		pr_info("Forcing device quirks to 0x%x by module parameter for testing purpose\n",
> +			dev->quirks);
> +		pr_info("Please report required quirks to the linux-uvc-devel mailing list\n");
>  	}
>  
>  	/* Register the V4L2 device. */
> @@ -2300,9 +2298,8 @@ 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);
> +		pr_info("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..a5c11cde273c 100644
> --- a/drivers/media/usb/uvc/uvc_entity.c
> +++ b/drivers/media/usb/uvc/uvc_entity.c
> @@ -6,6 +6,8 @@
>   *          Laurent Pinchart (laurent.pinchart@ideasonboard.com)
>   */
>  
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/videodev2.h>
> @@ -139,8 +141,8 @@ 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);
> +			pr_info("Failed to initialize entity for entity %u\n",
> +				entity->id);
>  			return ret;
>  		}
>  	}
> @@ -148,8 +150,8 @@ 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);
> +			pr_info("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..4c84525ada71 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -6,6 +6,8 @@
>   *          Laurent Pinchart (laurent.pinchart@ideasonboard.com)
>   */
>  
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/kernel.h>
>  #include <linux/input.h>
>  #include <linux/slab.h>
> @@ -208,8 +210,8 @@ 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);
> +		pr_warn("Non-zero status (%d) in status completion handler\n",
> +			urb->status);
>  		return;
>  	}
>  
> @@ -244,8 +246,7 @@ 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);
> +		pr_err("Failed to resubmit status URB (%d)\n", ret);
>  	}
>  }
>  
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index a6a441d92b94..6d48bcdeac16 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -6,6 +6,8 @@
>   *          Laurent Pinchart (laurent.pinchart@ideasonboard.com)
>   */
>  
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
> @@ -76,9 +78,8 @@ 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);
> +	pr_err("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 +255,8 @@ 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);
> +		pr_err("Failed to query (%u) UVC %s control : %d (exp. %u)\n",
> +		       query, probe ? "probe" : "commit", ret, size);
>  		ret = -EIO;
>  		goto out;
>  	}
> @@ -334,9 +334,8 @@ 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);
> +		pr_err("Failed to set UVC %s control : %d (exp. %u)\n",
> +		       probe ? "probe" : "commit", ret, size);
>  		ret = -EIO;
>  	}
>  
> @@ -1120,8 +1119,7 @@ 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);
> +		pr_err("Failed to resubmit video URB (%d)\n", ret);
>  }
>  
>  static void uvc_video_decode_data(struct uvc_urb *uvc_urb,
> @@ -1507,8 +1505,8 @@ 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);
> +		pr_warn("Non-zero status (%d) in video completion handler\n",
> +			urb->status);
>  		fallthrough;
>  	case -ENOENT:		/* usb_poison_urb() called. */
>  		if (stream->frozen)
> @@ -1545,9 +1543,7 @@ 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);
> +			pr_err("Failed to resubmit video URB (%d)\n", ret);
>  		return;
>  	}
>  
> @@ -1893,8 +1889,8 @@ 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);
> +			pr_err("Failed to submit URB %u (%d)\n",
> +			       uvc_urb_index(uvc_urb), ret);
>  			uvc_video_stop_transfer(stream, 1);
>  			return ret;
>  		}
> @@ -1989,7 +1985,7 @@ int uvc_video_init(struct uvc_streaming *stream)
>  	int ret;
>  
>  	if (stream->nformats == 0) {
> -		uvc_printk(KERN_INFO, "No supported video formats found.\n");
> +		pr_info("No supported video formats found\n");
>  		return -EINVAL;
>  	}
>  
> @@ -2029,8 +2025,7 @@ 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");
> +		pr_info("No frame descriptor found for the default format\n");
>  		return -EINVAL;
>  	}
>  
> @@ -2064,8 +2059,7 @@ 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");
> +			pr_info("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 a3dfacf069c4..724dec20444a 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -725,20 +725,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))			\
> +		pr_info(fmt, ##__VA_ARGS__);				\
> +} while (0)
>  
>  /* --------------------------------------------------------------------------
>   * Internal functions.
> 
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/7] media: uvcvideo: Use pr_cont() macro

[Joe Perches]

On Wed, 2020-11-04 at 23:42 +0200, Laurent Pinchart wrote:
> Hi Joe,

Hi Laurent.

> On Wed, Nov 04, 2020 at 11:29:30AM -0800, Joe Perches wrote:
> > On Wed, 2020-11-04 at 19:07 +0100, Ricardo Ribalda wrote:
> > > Replace all the uses of printk(KERN_CONT ... with pr_cont().
> > 
> > Perhaps remove the uvc_printk macro and uses and use the more
> > common pr_fmt and pr_<level> mechanisms.
> 
> I'd actually go for dev_* instead, to give some context. It's fairly
> common to have multiple UVC devices connected to a system, so printing
> the device name would be useful. It can still be wrapped with
> uvc_printk() if we want to wrap the cast from uvc_device to a struct
> device (we should actually try to get the device corresponding to the
> USB interface where available, so we should use uvc_streaming->intf->dev
> where possible, and fallback to uvc_device->udev->dev otherwise), or
> drop the wrapper completely.

Of course yes.  I was not going to look around and update the existing
call sites to find whatever controlling uvc_device * or other struct *
to a real device that exists though.

It's not even clear from the changes that an appropriate pointer to
some struct exists in all the functions.

That's work for someone that knows the actual subsystem and I do not.

cheers, Joe

Re: [PATCH v2 1/7] media: uvcvideo: Use pr_cont() macro

[Ricardo Ribalda]

Hi

On Wed, Nov 4, 2020 at 10:51 PM Joe Perches <joe@perches.com> wrote:
>
> On Wed, 2020-11-04 at 23:42 +0200, Laurent Pinchart wrote:
> > Hi Joe,
>
> Hi Laurent.
>
> > On Wed, Nov 04, 2020 at 11:29:30AM -0800, Joe Perches wrote:
> > > On Wed, 2020-11-04 at 19:07 +0100, Ricardo Ribalda wrote:
> > > > Replace all the uses of printk(KERN_CONT ... with pr_cont().
> > >
> > > Perhaps remove the uvc_printk macro and uses and use the more
> > > common pr_fmt and pr_<level> mechanisms.
> >
> > I'd actually go for dev_* instead, to give some context. It's fairly
> > common to have multiple UVC devices connected to a system, so printing
> > the device name would be useful. It can still be wrapped with
> > uvc_printk() if we want to wrap the cast from uvc_device to a struct
> > device (we should actually try to get the device corresponding to the
> > USB interface where available, so we should use uvc_streaming->intf->dev
> > where possible, and fallback to uvc_device->udev->dev otherwise), or
> > drop the wrapper completely.
>
> Of course yes.  I was not going to look around and update the existing
> call sites to find whatever controlling uvc_device * or other struct *
> to a real device that exists though.
>
> It's not even clear from the changes that an appropriate pointer to
> some struct exists in all the functions.
>
> That's work for someone that knows the actual subsystem and I do not.

I have updated my tree with the dev_ variants

https://github.com/ribalda/linux/commit/b8785fd8efb4f2e5bbf5d0f2df3e0d69a5439015

will send it to the tree when I get more feedback from the other patches.

Thanks!

>
> cheers, Joe
>


-- 
Ricardo Ribalda

Re: [PATCH v2 1/7] media: uvcvideo: Use pr_cont() macro

[Joe Perches]

On Wed, 2020-11-04 at 23:31 +0100, Ricardo Ribalda wrote:

> I have updated my tree with the dev_ variants
> 
> https://github.com/ribalda/linux/commit/b8785fd8efb4f2e5bbf5d0f2df3e0d69a5439015

I looked at this link and was confused so you made me look.
I think you meant:

https://github.com/ribalda/linux/commit/83cb6eb3a9f7bd1954acbfb4fb3d56ddf54bce73

Re: [PATCH v2 1/7] media: uvcvideo: Use pr_cont() macro

[Ricardo Ribalda]

Hi Joe

On Thu, Nov 5, 2020 at 12:00 AM Joe Perches <joe@perches.com> wrote:
>
> On Wed, 2020-11-04 at 23:31 +0100, Ricardo Ribalda wrote:
>
> > I have updated my tree with the dev_ variants
> >
> > https://github.com/ribalda/linux/commit/b8785fd8efb4f2e5bbf5d0f2df3e0d69a5439015
>
> I looked at this link and was confused so you made me look.
> I think you meant:
>
> https://github.com/ribalda/linux/commit/83cb6eb3a9f7bd1954acbfb4fb3d56ddf54bce73

Yes, thanks :) Sorry about that

This is why I should be away from a keyboard after 23:00 :)
>
>


-- 
Ricardo Ribalda

Re: [PATCH v2 1/7] media: uvcvideo: Use pr_cont() macro

[Joe Perches]

On Thu, 2020-11-05 at 00:01 +0100, Ricardo Ribalda wrote:
> Hi Joe
> 
> On Thu, Nov 5, 2020 at 12:00 AM Joe Perches <joe@perches.com> wrote:
> > 
> > On Wed, 2020-11-04 at 23:31 +0100, Ricardo Ribalda wrote:
> > 
> > > I have updated my tree with the dev_ variants
> > > 
> > > https://github.com/ribalda/linux/commit/b8785fd8efb4f2e5bbf5d0f2df3e0d69a5439015
> > 
> > I looked at this link and was confused so you made me look.
> > I think you meant:
> > 
> > https://github.com/ribalda/linux/commit/83cb6eb3a9f7bd1954acbfb4fb3d56ddf54bce73
> 
> Yes, thanks :) Sorry about that
> 
> This is why I should be away from a keyboard after 23:00 :)

Sleep is good.
There are lots of sleep deprived people here in the US today though.

It looks as if all the pr_cont uses in the code are odd and repetitive.

Perhaps it'd be sensible to add something like:

#define uvc_trace_cont(flag, fmt, ...)					\
do {									\
	if (uvc_trace_param & (flag))					\
		pr_cont(fmt, ##__VA_ARGS__);				\
} while (0)

and change all the uses like:

-               if (uvc_trace_param & UVC_TRACE_PROBE)
-                       printk(KERN_CONT " <- SU %d", entity->id);
+               uvc_trace_cont(UVC_TRACE_PROBE, " <- SU %d", entity->id);

Re: [PATCH v2 1/7] media: uvcvideo: Use pr_cont() macro

[Ricardo Ribalda]

Hi Joe

On Thu, Nov 5, 2020 at 12:59 AM Joe Perches <joe@perches.com> wrote:
>
> On Thu, 2020-11-05 at 00:01 +0100, Ricardo Ribalda wrote:
> > Hi Joe
> >
> > On Thu, Nov 5, 2020 at 12:00 AM Joe Perches <joe@perches.com> wrote:
> > >
> > > On Wed, 2020-11-04 at 23:31 +0100, Ricardo Ribalda wrote:
> > >
> > > > I have updated my tree with the dev_ variants
> > > >
> > > > https://github.com/ribalda/linux/commit/b8785fd8efb4f2e5bbf5d0f2df3e0d69a5439015
> > >
> > > I looked at this link and was confused so you made me look.
> > > I think you meant:
> > >
> > > https://github.com/ribalda/linux/commit/83cb6eb3a9f7bd1954acbfb4fb3d56ddf54bce73
> >
> > Yes, thanks :) Sorry about that
> >
> > This is why I should be away from a keyboard after 23:00 :)
>
> Sleep is good.
> There are lots of sleep deprived people here in the US today though.

Today and tomorrow and the day after. Seems like you are not going to
sleep for a week if you want a final result.

>
> It looks as if all the pr_cont uses in the code are odd and repetitive.
>
> Perhaps it'd be sensible to add something like:

Looks like a great idea. Queued for my v3

https://github.com/ribalda/linux/commit/1623b648331d7f69c8a9f6b199e119f6c8ee61c6

I let Laurent decide if we should squash with the previous patch or not.

Thanks!

>
> #define uvc_trace_cont(flag, fmt, ...)                                  \
> do {                                                                    \
>         if (uvc_trace_param & (flag))                                   \
>                 pr_cont(fmt, ##__VA_ARGS__);                            \
> } while (0)
>
> and change all the uses like:
>
> -               if (uvc_trace_param & UVC_TRACE_PROBE)
> -                       printk(KERN_CONT " <- SU %d", entity->id);
> +               uvc_trace_cont(UVC_TRACE_PROBE, " <- SU %d", entity->id);
>
>


-- 
Ricardo Ribalda

Re: [PATCH v2 1/7] media: uvcvideo: Use pr_cont() macro

[Joe Perches]

On Thu, 2020-11-05 at 10:50 +0100, Ricardo Ribalda wrote:
> Hi Joe

Rehi Ricardo.
 
> On Thu, Nov 5, 2020 at 12:59 AM Joe Perches <joe@perches.com> wrote:
> > It looks as if all the pr_cont uses in the code are odd and repetitive.
> > 
> > Perhaps it'd be sensible to add something like:
> 
> Looks like a great idea. Queued for my v3
> 
> https://github.com/ribalda/linux/commit/1623b648331d7f69c8a9f6b199e119f6c8ee61c6
> 
> I let Laurent decide if we should squash with the previous patch or not.
> 
> Thanks!
> 
> > 
> > #define uvc_trace_cont(flag, fmt, ...)                                  \
> > do {                                                                    \
> >         if (uvc_trace_param & (flag))                                   \
> >                 pr_cont(fmt, ##__VA_ARGS__);                            \
> > } while (0)
> > 
> > and change all the uses like:
> > 
> > -               if (uvc_trace_param & UVC_TRACE_PROBE)
> > -                       printk(KERN_CONT " <- SU %d", entity->id);
> > +               uvc_trace_cont(UVC_TRACE_PROBE, " <- SU %d", entity->id);
> > 

While I know the only uses of uvc_trace_cont are with UVC_TRACE_PROBE,
there are other existing uvc_trace(flag, ...) uses.

I think the uvc_trace_cont mechanism should require a flag and not
assume UVC_TRACE_PROBE so it's possible, even though currently unused,
for other uvc_trace calls to be continued.

$ git grep -Poh '\buvc_trace\(\w+' drivers/media | sort | uniq -c
      1 uvc_trace(flag
      6 uvc_trace(UVC_TRACE_CALLS
      1 uvc_trace(UVC_TRACE_CAPTURE
      2 uvc_trace(UVC_TRACE_CLOCK
     15 uvc_trace(UVC_TRACE_CONTROL
     42 uvc_trace(UVC_TRACE_DESCR
      6 uvc_trace(UVC_TRACE_FORMAT
     12 uvc_trace(UVC_TRACE_FRAME
      7 uvc_trace(UVC_TRACE_PROBE
      1 uvc_trace(UVC_TRACE_STATS
      6 uvc_trace(UVC_TRACE_STATUS
      4 uvc_trace(UVC_TRACE_SUSPEND
      6 uvc_trace(UVC_TRACE_VIDEO

Re: [PATCH v2 1/7] media: uvcvideo: Use pr_cont() macro

[Ricardo Ribalda]

Hi Joe

On Thu, Nov 5, 2020 at 7:58 PM Joe Perches <joe@perches.com> wrote:
>
> On Thu, 2020-11-05 at 10:50 +0100, Ricardo Ribalda wrote:
> > Hi Joe
>
> Rehi Ricardo.
>
> > On Thu, Nov 5, 2020 at 12:59 AM Joe Perches <joe@perches.com> wrote:
> > > It looks as if all the pr_cont uses in the code are odd and repetitive.
> > >
> > > Perhaps it'd be sensible to add something like:
> >
> > Looks like a great idea. Queued for my v3
> >
> > https://github.com/ribalda/linux/commit/1623b648331d7f69c8a9f6b199e119f6c8ee61c6
> >
> > I let Laurent decide if we should squash with the previous patch or not.
> >
> > Thanks!
> >
> > >
> > > #define uvc_trace_cont(flag, fmt, ...)                                  \
> > > do {                                                                    \
> > >         if (uvc_trace_param & (flag))                                   \
> > >                 pr_cont(fmt, ##__VA_ARGS__);                            \
> > > } while (0)
> > >
> > > and change all the uses like:
> > >
> > > -               if (uvc_trace_param & UVC_TRACE_PROBE)
> > > -                       printk(KERN_CONT " <- SU %d", entity->id);
> > > +               uvc_trace_cont(UVC_TRACE_PROBE, " <- SU %d", entity->id);
> > >
>
> While I know the only uses of uvc_trace_cont are with UVC_TRACE_PROBE,
> there are other existing uvc_trace(flag, ...) uses.
>
> I think the uvc_trace_cont mechanism should require a flag and not
> assume UVC_TRACE_PROBE so it's possible, even though currently unused,
> for other uvc_trace calls to be continued.
>
> $ git grep -Poh '\buvc_trace\(\w+' drivers/media | sort | uniq -c
>       1 uvc_trace(flag
>       6 uvc_trace(UVC_TRACE_CALLS
>       1 uvc_trace(UVC_TRACE_CAPTURE
>       2 uvc_trace(UVC_TRACE_CLOCK
>      15 uvc_trace(UVC_TRACE_CONTROL
>      42 uvc_trace(UVC_TRACE_DESCR
>       6 uvc_trace(UVC_TRACE_FORMAT
>      12 uvc_trace(UVC_TRACE_FRAME
>       7 uvc_trace(UVC_TRACE_PROBE
>       1 uvc_trace(UVC_TRACE_STATS
>       6 uvc_trace(UVC_TRACE_STATUS
>       4 uvc_trace(UVC_TRACE_SUSPEND
>       6 uvc_trace(UVC_TRACE_VIDEO
>
>

There you are:

https://github.com/ribalda/linux/commit/7b3e2214dde38dc4534eec7399a4053881c2589e

Thanks!

-- 
Ricardo Ribalda

Re: [PATCH v2 1/7] media: uvcvideo: Use pr_cont() macro

[Ricardo Ribalda]

Hi Joe


On Wed, Nov 4, 2020 at 8:29 PM Joe Perches <joe@perches.com> wrote:
>
> On Wed, 2020-11-04 at 19:07 +0100, Ricardo Ribalda wrote:
> > Replace all the uses of printk(KERN_CONT ... with pr_cont().
>
> Perhaps remove the uvc_printk macro and uses and use the more
> common pr_fmt and pr_<level> mechanisms.

I have made the suggested changes and pushed them to my working branch

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

once the other patches are reviewed I send it to the mailing list.

Thanks!

>
> Something like:
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c   |  5 +++--
>  drivers/media/usb/uvc/uvc_driver.c | 35 +++++++++++++++------------------
>  drivers/media/usb/uvc/uvc_entity.c | 10 ++++++----
>  drivers/media/usb/uvc/uvc_status.c |  9 +++++----
>  drivers/media/usb/uvc/uvc_video.c  | 40 ++++++++++++++++----------------------
>  drivers/media/usb/uvc/uvcvideo.h   | 25 +++++++++++-------------
>  6 files changed, 58 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index f479d8971dfb..e0b273697d49 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -6,6 +6,8 @@
>   *          Laurent Pinchart (laurent.pinchart@ideasonboard.com)
>   */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
> @@ -1317,8 +1319,7 @@ 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);
> +               pr_err("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 ddb9eaa11be7..03d68e2b6b70 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -6,6 +6,8 @@
>   *          Laurent Pinchart (laurent.pinchart@ideasonboard.com)
>   */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/atomic.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> @@ -534,8 +536,7 @@ 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]);
> +                       pr_info("Unknown video format %pUl\n", &buffer[5]);
>                         snprintf(format->name, sizeof(format->name), "%pUl\n",
>                                 &buffer[5]);
>                         format->fcc = 0;
> @@ -1925,7 +1926,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");
> +               pr_info("No valid video chain found\n");
>                 return -1;
>         }
>
> @@ -2077,8 +2078,8 @@ 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);
> +               pr_err("Failed to register %s device (%d)\n",
> +                      v4l2_type_names[type], ret);
>                 return ret;
>         }
>
> @@ -2094,8 +2095,7 @@ 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);
> +               pr_err("Failed to initialize the device (%d)\n", ret);
>                 return ret;
>         }
>
> @@ -2129,8 +2129,8 @@ 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);
> +                       pr_info("No streaming interface found for terminal %u\n",
> +                               term->id);
>                         continue;
>                 }
>
> @@ -2163,8 +2163,7 @@ 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);
> +                       pr_info("Failed to register entities (%d)\n", ret);
>  #endif
>         }
>
> @@ -2261,17 +2260,16 @@ static int uvc_probe(struct usb_interface *intf,
>                 goto error;
>         }
>
> -       uvc_printk(KERN_INFO, "Found UVC %u.%02x device %s (%04x:%04x)\n",
> +       pr_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));
>
>         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");
> +               pr_info("Forcing device quirks to 0x%x by module parameter for testing purpose\n",
> +                       dev->quirks);
> +               pr_info("Please report required quirks to the linux-uvc-devel mailing list\n");
>         }
>
>         /* Register the V4L2 device. */
> @@ -2300,9 +2298,8 @@ 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);
> +               pr_info("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..a5c11cde273c 100644
> --- a/drivers/media/usb/uvc/uvc_entity.c
> +++ b/drivers/media/usb/uvc/uvc_entity.c
> @@ -6,6 +6,8 @@
>   *          Laurent Pinchart (laurent.pinchart@ideasonboard.com)
>   */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/videodev2.h>
> @@ -139,8 +141,8 @@ 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);
> +                       pr_info("Failed to initialize entity for entity %u\n",
> +                               entity->id);
>                         return ret;
>                 }
>         }
> @@ -148,8 +150,8 @@ 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);
> +                       pr_info("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..4c84525ada71 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -6,6 +6,8 @@
>   *          Laurent Pinchart (laurent.pinchart@ideasonboard.com)
>   */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/kernel.h>
>  #include <linux/input.h>
>  #include <linux/slab.h>
> @@ -208,8 +210,8 @@ 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);
> +               pr_warn("Non-zero status (%d) in status completion handler\n",
> +                       urb->status);
>                 return;
>         }
>
> @@ -244,8 +246,7 @@ 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);
> +               pr_err("Failed to resubmit status URB (%d)\n", ret);
>         }
>  }
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index a6a441d92b94..6d48bcdeac16 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -6,6 +6,8 @@
>   *          Laurent Pinchart (laurent.pinchart@ideasonboard.com)
>   */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
> @@ -76,9 +78,8 @@ 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);
> +       pr_err("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 +255,8 @@ 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);
> +               pr_err("Failed to query (%u) UVC %s control : %d (exp. %u)\n",
> +                      query, probe ? "probe" : "commit", ret, size);
>                 ret = -EIO;
>                 goto out;
>         }
> @@ -334,9 +334,8 @@ 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);
> +               pr_err("Failed to set UVC %s control : %d (exp. %u)\n",
> +                      probe ? "probe" : "commit", ret, size);
>                 ret = -EIO;
>         }
>
> @@ -1120,8 +1119,7 @@ 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);
> +               pr_err("Failed to resubmit video URB (%d)\n", ret);
>  }
>
>  static void uvc_video_decode_data(struct uvc_urb *uvc_urb,
> @@ -1507,8 +1505,8 @@ 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);
> +               pr_warn("Non-zero status (%d) in video completion handler\n",
> +                       urb->status);
>                 fallthrough;
>         case -ENOENT:           /* usb_poison_urb() called. */
>                 if (stream->frozen)
> @@ -1545,9 +1543,7 @@ 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);
> +                       pr_err("Failed to resubmit video URB (%d)\n", ret);
>                 return;
>         }
>
> @@ -1893,8 +1889,8 @@ 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);
> +                       pr_err("Failed to submit URB %u (%d)\n",
> +                              uvc_urb_index(uvc_urb), ret);
>                         uvc_video_stop_transfer(stream, 1);
>                         return ret;
>                 }
> @@ -1989,7 +1985,7 @@ int uvc_video_init(struct uvc_streaming *stream)
>         int ret;
>
>         if (stream->nformats == 0) {
> -               uvc_printk(KERN_INFO, "No supported video formats found.\n");
> +               pr_info("No supported video formats found\n");
>                 return -EINVAL;
>         }
>
> @@ -2029,8 +2025,7 @@ 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");
> +               pr_info("No frame descriptor found for the default format\n");
>                 return -EINVAL;
>         }
>
> @@ -2064,8 +2059,7 @@ 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");
> +                       pr_info("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 a3dfacf069c4..724dec20444a 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -725,20 +725,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))                  \
> +               pr_info(fmt, ##__VA_ARGS__);                            \
> +} while (0)
>
>  /* --------------------------------------------------------------------------
>   * Internal functions.
>
>


-- 
Ricardo Ribalda

[PATCH v2 2/7] 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.

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

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

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

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

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

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

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

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

> +
>  	entity->num_links = 0;
>  	entity->num_pads = num_pads;
>  	entity->pads = ((void *)(entity + 1)) + extra_size;
> @@ -1109,7 +1126,7 @@ static int uvc_parse_vendor_control(struct uvc_device *dev,
>  		if (unit == NULL)
>  			return -ENOMEM;
>  
> -		memcpy(unit->extension.guidExtensionCode, &buffer[4], 16);
> +		memcpy(unit->guid, &buffer[4], 16);
>  		unit->extension.bNumControls = buffer[20];
>  		memcpy(unit->baSourceID, &buffer[22], p);
>  		unit->extension.bControlSize = buffer[22+p];
> @@ -1368,7 +1385,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
>  		if (unit == NULL)
>  			return -ENOMEM;
>  
> -		memcpy(unit->extension.guidExtensionCode, &buffer[4], 16);
> +		memcpy(unit->guid, &buffer[4], 16);
>  		unit->extension.bNumControls = buffer[20];
>  		memcpy(unit->baSourceID, &buffer[22], p);
>  		unit->extension.bControlSize = buffer[22+p];
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index a3dfacf069c4..df7bf2d104a3 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -304,6 +304,7 @@ struct uvc_entity {
>  	u8 id;
>  	u16 type;
>  	char name[64];
> +	u8 guid[16];
>  
>  	/* Media controller-related fields. */
>  	struct video_device *vdev;
> @@ -342,7 +343,6 @@ struct uvc_entity {
>  		} selector;
>  
>  		struct {
> -			u8  guidExtensionCode[16];
>  			u8  bNumControls;
>  			u8  bControlSize;
>  			u8  *bmControls;

-- 
Regards,

Laurent Pinchart

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

[Ricardo Ribalda]

Hi Laurent

Thanks for the review

On Fri, Nov 6, 2020 at 7:06 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Wed, Nov 04, 2020 at 07:07:29PM +0100, Ricardo Ribalda wrote:
> > Instead of having multiple copies of the entity guid on the code, move
> > it to the entity structure.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c   | 30 ++++--------------------------
> >  drivers/media/usb/uvc/uvc_driver.c | 21 +++++++++++++++++++--
> >  drivers/media/usb/uvc/uvcvideo.h   |  2 +-
> >  3 files changed, 24 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index f479d8971dfb..0e480b75e724 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -826,31 +826,10 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping,
> >   * Terminal and unit management
> >   */
> >
> > -static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
> > -static const u8 uvc_camera_guid[16] = UVC_GUID_UVC_CAMERA;
> > -static const u8 uvc_media_transport_input_guid[16] =
> > -     UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
> > -
> >  static int uvc_entity_match_guid(const struct uvc_entity *entity,
> > -     const u8 guid[16])
> > +                              const u8 guid[16])
> >  {
> > -     switch (UVC_ENTITY_TYPE(entity)) {
> > -     case UVC_ITT_CAMERA:
> > -             return memcmp(uvc_camera_guid, guid, 16) == 0;
> > -
> > -     case UVC_ITT_MEDIA_TRANSPORT_INPUT:
> > -             return memcmp(uvc_media_transport_input_guid, guid, 16) == 0;
> > -
> > -     case UVC_VC_PROCESSING_UNIT:
> > -             return memcmp(uvc_processing_guid, guid, 16) == 0;
> > -
> > -     case UVC_VC_EXTENSION_UNIT:
> > -             return memcmp(entity->extension.guidExtensionCode,
> > -                           guid, 16) == 0;
> > -
> > -     default:
> > -             return 0;
> > -     }
> > +     return memcmp(entity->guid, guid, sizeof(entity->guid)) == 0;
> >  }
> >
> >  /* ------------------------------------------------------------------------
> > @@ -1776,8 +1755,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
> >       if (data == NULL)
> >               return -ENOMEM;
> >
> > -     memcpy(info->entity, ctrl->entity->extension.guidExtensionCode,
> > -            sizeof(info->entity));
> > +     memcpy(info->entity, ctrl->entity->guid, sizeof(info->entity));
> >       info->index = ctrl->index;
> >       info->selector = ctrl->index + 1;
> >
> > @@ -1883,7 +1861,7 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
> >
> >       if (!found) {
> >               uvc_trace(UVC_TRACE_CONTROL, "Control %pUl/%u not found.\n",
> > -                     entity->extension.guidExtensionCode, xqry->selector);
> > +                     entity->guid, xqry->selector);
> >               return -ENOENT;
> >       }
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 9fc0b600eab1..77fea26faa9a 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1019,6 +1019,11 @@ static int uvc_parse_streaming(struct uvc_device *dev,
> >       return ret;
> >  }
> >
> > +static const u8 uvc_camera_guid[16] = UVC_GUID_UVC_CAMERA;
> > +static const u8 uvc_media_transport_input_guid[16] =
> > +     UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
> > +static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
> > +
> >  static struct uvc_entity *uvc_alloc_entity(u16 type, u8 id,
> >               unsigned int num_pads, unsigned int extra_size)
> >  {
> > @@ -1038,6 +1043,18 @@ static struct uvc_entity *uvc_alloc_entity(u16 type, u8 id,
> >       entity->id = id;
> >       entity->type = type;
> >
> > +     switch (type) {
> > +     case UVC_ITT_CAMERA:
> > +             memcpy(entity->guid, uvc_camera_guid, 16);
> > +             break;
> > +     case UVC_ITT_MEDIA_TRANSPORT_INPUT:
> > +             memcpy(entity->guid, uvc_media_transport_input_guid, 16);
> > +             break;
> > +     case UVC_VC_PROCESSING_UNIT:
> > +             memcpy(entity->guid, uvc_processing_guid, 16);
> > +             break;
> > +     }
>
> Given that the GUID is set in uvc_parse_vendor_control() and
> uvc_parse_standard_control() for extension units, I'm wondering if it
> would make sense to move it there for the other entity types too. Up to
> you. Otherwise, I'd add the following comment above the switch:
>
>         /*
>          * Set the GUID for standard entity types. For extension units, the GUID
>          * is initialized by the caller.
>          */

I added the comment. So far I am working on

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

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

Thanks!

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > +
> >       entity->num_links = 0;
> >       entity->num_pads = num_pads;
> >       entity->pads = ((void *)(entity + 1)) + extra_size;
> > @@ -1109,7 +1126,7 @@ static int uvc_parse_vendor_control(struct uvc_device *dev,
> >               if (unit == NULL)
> >                       return -ENOMEM;
> >
> > -             memcpy(unit->extension.guidExtensionCode, &buffer[4], 16);
> > +             memcpy(unit->guid, &buffer[4], 16);
> >               unit->extension.bNumControls = buffer[20];
> >               memcpy(unit->baSourceID, &buffer[22], p);
> >               unit->extension.bControlSize = buffer[22+p];
> > @@ -1368,7 +1385,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
> >               if (unit == NULL)
> >                       return -ENOMEM;
> >
> > -             memcpy(unit->extension.guidExtensionCode, &buffer[4], 16);
> > +             memcpy(unit->guid, &buffer[4], 16);
> >               unit->extension.bNumControls = buffer[20];
> >               memcpy(unit->baSourceID, &buffer[22], p);
> >               unit->extension.bControlSize = buffer[22+p];
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index a3dfacf069c4..df7bf2d104a3 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -304,6 +304,7 @@ struct uvc_entity {
> >       u8 id;
> >       u16 type;
> >       char name[64];
> > +     u8 guid[16];
> >
> >       /* Media controller-related fields. */
> >       struct video_device *vdev;
> > @@ -342,7 +343,6 @@ struct uvc_entity {
> >               } selector;
> >
> >               struct {
> > -                     u8  guidExtensionCode[16];
> >                       u8  bNumControls;
> >                       u8  bControlSize;
> >                       u8  *bmControls;
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

[PATCH v2 3/7] 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 77fea26faa9a..7a1b2decccc8 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.1.341.ge80a0c044ae-goog

[PATCH v2 4/7] 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 7a1b2decccc8..27acc221f0d6 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);
@@ -1061,7 +1065,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.1.341.ge80a0c044ae-goog

[PATCH v2 5/7] 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 | 21 +++++++++++++++++----
 drivers/media/usb/uvc/uvcvideo.h |  3 +++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 0e480b75e724..076f63af1031 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -980,10 +980,19 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain,
 		return -EACCES;
 
 	if (!ctrl->loaded) {
-		ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, ctrl->entity->id,
-				chain->dev->intfnum, ctrl->info.selector,
+		if (ctrl->entity->get_cur) {
+			ret = ctrl->entity->get_cur(ctrl->entity,
+				ctrl->info.selector,
 				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
 				ctrl->info.size);
+		} else {
+			ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
+				ctrl->entity->id,
+				chain->dev->intfnum,
+				ctrl->info.selector,
+				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
+				ctrl->info.size);
+		}
 		if (ret < 0)
 			return ret;
 
@@ -1687,8 +1696,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(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..776b083ed466 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -353,6 +353,9 @@ struct uvc_entity {
 	u8 bNrInPins;
 	u8 *baSourceID;
 
+	int (*get_info)(struct uvc_entity *entity, u8 cs, u8 *caps);
+	int (*get_cur)(struct uvc_entity *entity, u8 cs, void *data, u16 size);
+
 	unsigned int ncontrols;
 	struct uvc_control *controls;
 };
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH v2 6/7] 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 | 105 +++++++++++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  12 ++++
 3 files changed, 123 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 076f63af1031..f9382f8c10a8 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1301,6 +1301,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);
@@ -2285,6 +2288,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 27acc221f0d6..bb977b333752 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;
@@ -1048,6 +1050,9 @@ static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
 	entity->type = type;
 
 	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;
@@ -1461,6 +1466,92 @@ static int uvc_parse_control(struct uvc_device *dev)
 	return 0;
 }
 
+static int uvc_gpio_get_cur(struct uvc_entity *entity, u8 cs, void *data,
+			    u16 size)
+{
+	if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
+		return -EINVAL;
+
+	*(uint8_t *)data = gpiod_get_value(entity->gpio.gpio_privacy);
+	return 0;
+}
+
+static int uvc_gpio_get_info(struct uvc_entity *entity, u8 cs, u8 *caps)
+{
+	if (cs != UVC_CT_PRIVACY_CONTROL)
+		return -EINVAL;
+
+	*caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
+	return 0;
+}
+
+static 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
  */
@@ -1912,6 +2003,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))
@@ -1950,6 +2042,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;
 }
 
@@ -2282,6 +2381,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 776b083ed466..689f95ff4f12 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.1.341.ge80a0c044ae-goog

[PATCH v2 7/7] 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 f9382f8c10a8..285a7a5e15d2 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.1.341.ge80a0c044ae-goog