[PATCH] media: v4l2-async: Remove V4L2_ASYNC_MATCH_CUSTOM

[PATCH] media: v4l2-async: Remove V4L2_ASYNC_MATCH_CUSTOM

[Ezequiel Garcia]

Custom/driver-specific v4l2-async match support was introduced
in 2013, as V4L2_ASYNC_BUS_CUSTOM.

This type of match never had any user, so it's fair
to conclude it's not required and that safe for removal.
If the support is ever needed, it can always be restored.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/v4l2-async.c | 14 --------------
 include/media/v4l2-async.h           | 17 -----------------
 2 files changed, 31 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index e3ab003a6c85..3faf1d12d49d 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -139,16 +139,6 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
 	return true;
 }
 
-static bool match_custom(struct v4l2_async_notifier *notifier,
-			 struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
-{
-	if (!asd->match.custom.match)
-		/* Match always */
-		return true;
-
-	return asd->match.custom.match(sd->dev, asd);
-}
-
 static LIST_HEAD(subdev_list);
 static LIST_HEAD(notifier_list);
 static DEFINE_MUTEX(list_lock);
@@ -164,9 +154,6 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier,
 	list_for_each_entry(asd, &notifier->waiting, list) {
 		/* bus_type has been verified valid before */
 		switch (asd->match_type) {
-		case V4L2_ASYNC_MATCH_CUSTOM:
-			match = match_custom;
-			break;
 		case V4L2_ASYNC_MATCH_DEVNAME:
 			match = match_devname;
 			break;
@@ -467,7 +454,6 @@ static int v4l2_async_notifier_asd_valid(struct v4l2_async_notifier *notifier,
 		return -EINVAL;
 
 	switch (asd->match_type) {
-	case V4L2_ASYNC_MATCH_CUSTOM:
 	case V4L2_ASYNC_MATCH_DEVNAME:
 	case V4L2_ASYNC_MATCH_I2C:
 	case V4L2_ASYNC_MATCH_FWNODE:
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 0e04b5b2ebb0..8ed42188e7c9 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -21,8 +21,6 @@ struct v4l2_async_notifier;
  * enum v4l2_async_match_type - type of asynchronous subdevice logic to be used
  *	in order to identify a match
  *
- * @V4L2_ASYNC_MATCH_CUSTOM: Match will use the logic provided by &struct
- *	v4l2_async_subdev.match ops
  * @V4L2_ASYNC_MATCH_DEVNAME: Match will use the device name
  * @V4L2_ASYNC_MATCH_I2C: Match will check for I2C adapter ID and address
  * @V4L2_ASYNC_MATCH_FWNODE: Match will use firmware node
@@ -31,7 +29,6 @@ struct v4l2_async_notifier;
  * algorithm that will be used to match an asynchronous device.
  */
 enum v4l2_async_match_type {
-	V4L2_ASYNC_MATCH_CUSTOM,
 	V4L2_ASYNC_MATCH_DEVNAME,
 	V4L2_ASYNC_MATCH_I2C,
 	V4L2_ASYNC_MATCH_FWNODE,
@@ -58,15 +55,6 @@ enum v4l2_async_match_type {
  * @match.i2c.address:
  *		I2C address to be matched.
  *		Used if @match_type is %V4L2_ASYNC_MATCH_I2C.
- * @match.custom:
- *		Driver-specific match criteria.
- *		Used if @match_type is %V4L2_ASYNC_MATCH_CUSTOM.
- * @match.custom.match:
- *		Driver-specific match function to be used if
- *		%V4L2_ASYNC_MATCH_CUSTOM.
- * @match.custom.priv:
- *		Driver-specific private struct with match parameters
- *		to be used if %V4L2_ASYNC_MATCH_CUSTOM.
  * @asd_list:	used to add struct v4l2_async_subdev objects to the
  *		master notifier @asd_list
  * @list:	used to link struct v4l2_async_subdev objects, waiting to be
@@ -85,11 +73,6 @@ struct v4l2_async_subdev {
 			int adapter_id;
 			unsigned short address;
 		} i2c;
-		struct {
-			bool (*match)(struct device *dev,
-				      struct v4l2_async_subdev *sd);
-			void *priv;
-		} custom;
 	} match;
 
 	/* v4l2-async core private: not to be used by drivers */
-- 
2.29.2

[PATCH v3] media: v4l2-async: Add waiting subdevices debugfs

[Ezequiel Garcia]

There is currently little to no information available
about the reasons why a v4l2-async device hasn't
probed completely.

Inspired by the "devices_deferred" debugfs file,
add a file to list information about the subdevices
that are on waiting lists, for each notifier.

This is useful to debug v4l2-async subdevices
and notifiers, for instance when doing device bring-up.

For instance, a typical output would be:

$ cat /sys/kernel/debug/video4linux/pending_async_subdevices
ipu1_csi1:
 [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi1_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi1_mux
ipu1_csi0:
 [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi0_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi0_mux
imx6-mipi-csi2:
 [fwnode] dev=1-003c, node=/soc/bus@2100000/i2c@21a4000/camera@3c
imx-media:

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
--
v3:
* Drop MATCH_CUSTOM
* Use the actual fwnode, to print the real full path. endpoint name.
* Fix refcounted handle leak.
v2:
* Print fwnode path, as suggested by Sakari.
* Print the subdevices under their corresponding notifier.
* Rename the file as suggested by Laurent.
---
 drivers/media/v4l2-core/v4l2-async.c | 66 ++++++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-dev.c   |  5 +++
 include/media/v4l2-async.h           |  8 ++++
 3 files changed, 79 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 3faf1d12d49d..7c8a97205b3e 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
  */
 
+#include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
@@ -14,6 +15,7 @@
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 
@@ -823,3 +825,67 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
 	mutex_unlock(&list_lock);
 }
 EXPORT_SYMBOL(v4l2_async_unregister_subdev);
+
+static void print_waiting_subdev(struct seq_file *s,
+				 struct v4l2_async_subdev *asd)
+{
+	switch (asd->match_type) {
+	case V4L2_ASYNC_MATCH_DEVNAME:
+		seq_printf(s, " [devname] dev=%s\n", asd->match.device_name);
+		break;
+	case V4L2_ASYNC_MATCH_I2C:
+		seq_printf(s, " [i2c] dev=%d-%04x\n", asd->match.i2c.adapter_id,
+			   asd->match.i2c.address);
+		break;
+	case V4L2_ASYNC_MATCH_FWNODE: {
+		struct fwnode_handle *devnode, *fwnode = asd->match.fwnode;
+
+		devnode = fwnode_graph_is_endpoint(fwnode) ?
+			  fwnode_graph_get_port_parent(fwnode) :
+			  fwnode_handle_get(fwnode);
+
+		seq_printf(s, " [fwnode] dev=%s, node=%pfw\n",
+			   devnode->dev ? dev_name(devnode->dev) : "nil",
+			   fwnode);
+
+		fwnode_handle_put(devnode);
+		break;
+	}
+	}
+}
+
+static const char *
+v4l2_async_notifier_name(struct v4l2_async_notifier *notifier)
+{
+	if (notifier->v4l2_dev)
+		return notifier->v4l2_dev->name;
+	else if (notifier->sd)
+		return notifier->sd->name;
+	else
+		return "nil";
+}
+
+static int pending_subdevs_show(struct seq_file *s, void *data)
+{
+	struct v4l2_async_notifier *notif;
+	struct v4l2_async_subdev *asd;
+
+	mutex_lock(&list_lock);
+
+	list_for_each_entry(notif, &notifier_list, list) {
+		seq_printf(s, "%s:\n", v4l2_async_notifier_name(notif));
+		list_for_each_entry(asd, &notif->waiting, list)
+			print_waiting_subdev(s, asd);
+	}
+
+	mutex_unlock(&list_lock);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(pending_subdevs);
+
+void v4l2_async_debug_init(struct dentry *debugfs_dir)
+{
+	debugfs_create_file("pending_async_subdevices", 0444, debugfs_dir, NULL,
+			    &pending_subdevs_fops);
+}
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index a593ea0598b5..8d3813e6ab56 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -14,6 +14,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/debugfs.h>
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
@@ -37,6 +38,7 @@
 		       __func__, ##arg);				\
 } while (0)
 
+static struct dentry *v4l2_debugfs_dir;
 
 /*
  *	sysfs stuff
@@ -1113,6 +1115,8 @@ static int __init videodev_init(void)
 		return -EIO;
 	}
 
+	v4l2_debugfs_dir = debugfs_create_dir("video4linux", NULL);
+	v4l2_async_debug_init(v4l2_debugfs_dir);
 	return 0;
 }
 
@@ -1120,6 +1124,7 @@ static void __exit videodev_exit(void)
 {
 	dev_t dev = MKDEV(VIDEO_MAJOR, 0);
 
+	debugfs_remove_recursive(v4l2_debugfs_dir);
 	class_unregister(&video_class);
 	unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
 }
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 8ed42188e7c9..26296d4cb660 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -11,6 +11,7 @@
 #include <linux/list.h>
 #include <linux/mutex.h>
 
+struct dentry;
 struct device;
 struct device_node;
 struct v4l2_device;
@@ -120,6 +121,13 @@ struct v4l2_async_notifier {
 	struct list_head list;
 };
 
+/**
+ * v4l2_async_debug_init - Initialize debugging tools.
+ *
+ * @debugfs_dir: pointer to the parent debugfs &struct dentry
+ */
+void v4l2_async_debug_init(struct dentry *debugfs_dir);
+
 /**
  * v4l2_async_notifier_init - Initialize a notifier.
  *
-- 
2.29.2

Re: [PATCH] media: v4l2-async: Remove V4L2_ASYNC_MATCH_CUSTOM

[Laurent Pinchart]

Hi Ezequiel,

Thank you for the patch.

On Fri, Jan 08, 2021 at 02:17:27PM -0300, Ezequiel Garcia wrote:
> Custom/driver-specific v4l2-async match support was introduced
> in 2013, as V4L2_ASYNC_BUS_CUSTOM.
> 
> This type of match never had any user, so it's fair
> to conclude it's not required and that safe for removal.
> If the support is ever needed, it can always be restored.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

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

> ---
>  drivers/media/v4l2-core/v4l2-async.c | 14 --------------
>  include/media/v4l2-async.h           | 17 -----------------
>  2 files changed, 31 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index e3ab003a6c85..3faf1d12d49d 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -139,16 +139,6 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
>  	return true;
>  }
>  
> -static bool match_custom(struct v4l2_async_notifier *notifier,
> -			 struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> -{
> -	if (!asd->match.custom.match)
> -		/* Match always */
> -		return true;
> -
> -	return asd->match.custom.match(sd->dev, asd);
> -}
> -
>  static LIST_HEAD(subdev_list);
>  static LIST_HEAD(notifier_list);
>  static DEFINE_MUTEX(list_lock);
> @@ -164,9 +154,6 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier,
>  	list_for_each_entry(asd, &notifier->waiting, list) {
>  		/* bus_type has been verified valid before */
>  		switch (asd->match_type) {
> -		case V4L2_ASYNC_MATCH_CUSTOM:
> -			match = match_custom;
> -			break;
>  		case V4L2_ASYNC_MATCH_DEVNAME:
>  			match = match_devname;
>  			break;
> @@ -467,7 +454,6 @@ static int v4l2_async_notifier_asd_valid(struct v4l2_async_notifier *notifier,
>  		return -EINVAL;
>  
>  	switch (asd->match_type) {
> -	case V4L2_ASYNC_MATCH_CUSTOM:
>  	case V4L2_ASYNC_MATCH_DEVNAME:
>  	case V4L2_ASYNC_MATCH_I2C:
>  	case V4L2_ASYNC_MATCH_FWNODE:
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 0e04b5b2ebb0..8ed42188e7c9 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -21,8 +21,6 @@ struct v4l2_async_notifier;
>   * enum v4l2_async_match_type - type of asynchronous subdevice logic to be used
>   *	in order to identify a match
>   *
> - * @V4L2_ASYNC_MATCH_CUSTOM: Match will use the logic provided by &struct
> - *	v4l2_async_subdev.match ops
>   * @V4L2_ASYNC_MATCH_DEVNAME: Match will use the device name
>   * @V4L2_ASYNC_MATCH_I2C: Match will check for I2C adapter ID and address
>   * @V4L2_ASYNC_MATCH_FWNODE: Match will use firmware node
> @@ -31,7 +29,6 @@ struct v4l2_async_notifier;
>   * algorithm that will be used to match an asynchronous device.
>   */
>  enum v4l2_async_match_type {
> -	V4L2_ASYNC_MATCH_CUSTOM,
>  	V4L2_ASYNC_MATCH_DEVNAME,
>  	V4L2_ASYNC_MATCH_I2C,
>  	V4L2_ASYNC_MATCH_FWNODE,
> @@ -58,15 +55,6 @@ enum v4l2_async_match_type {
>   * @match.i2c.address:
>   *		I2C address to be matched.
>   *		Used if @match_type is %V4L2_ASYNC_MATCH_I2C.
> - * @match.custom:
> - *		Driver-specific match criteria.
> - *		Used if @match_type is %V4L2_ASYNC_MATCH_CUSTOM.
> - * @match.custom.match:
> - *		Driver-specific match function to be used if
> - *		%V4L2_ASYNC_MATCH_CUSTOM.
> - * @match.custom.priv:
> - *		Driver-specific private struct with match parameters
> - *		to be used if %V4L2_ASYNC_MATCH_CUSTOM.
>   * @asd_list:	used to add struct v4l2_async_subdev objects to the
>   *		master notifier @asd_list
>   * @list:	used to link struct v4l2_async_subdev objects, waiting to be
> @@ -85,11 +73,6 @@ struct v4l2_async_subdev {
>  			int adapter_id;
>  			unsigned short address;
>  		} i2c;
> -		struct {
> -			bool (*match)(struct device *dev,
> -				      struct v4l2_async_subdev *sd);
> -			void *priv;
> -		} custom;
>  	} match;
>  
>  	/* v4l2-async core private: not to be used by drivers */

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3] media: v4l2-async: Add waiting subdevices debugfs

[Sakari Ailus]

Hi Ezequiel,

On Fri, Jan 08, 2021 at 02:17:28PM -0300, Ezequiel Garcia wrote:
> There is currently little to no information available
> about the reasons why a v4l2-async device hasn't
> probed completely.
> 
> Inspired by the "devices_deferred" debugfs file,
> add a file to list information about the subdevices
> that are on waiting lists, for each notifier.
> 
> This is useful to debug v4l2-async subdevices
> and notifiers, for instance when doing device bring-up.
> 
> For instance, a typical output would be:
> 
> $ cat /sys/kernel/debug/video4linux/pending_async_subdevices
> ipu1_csi1:
>  [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi1_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi1_mux
> ipu1_csi0:
>  [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi0_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi0_mux
> imx6-mipi-csi2:
>  [fwnode] dev=1-003c, node=/soc/bus@2100000/i2c@21a4000/camera@3c
> imx-media:
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> --
> v3:
> * Drop MATCH_CUSTOM
> * Use the actual fwnode, to print the real full path. endpoint name.
> * Fix refcounted handle leak.
> v2:
> * Print fwnode path, as suggested by Sakari.
> * Print the subdevices under their corresponding notifier.
> * Rename the file as suggested by Laurent.

The patch itself seems fine. But lease put such notes below the '---' line
below going forward. What's above it will be part of the commit message.

> ---
>  drivers/media/v4l2-core/v4l2-async.c | 66 ++++++++++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-dev.c   |  5 +++
>  include/media/v4l2-async.h           |  8 ++++
>  3 files changed, 79 insertions(+)
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3] media: v4l2-async: Add waiting subdevices debugfs

[Hans Verkuil]

On 08/01/2021 18:17, Ezequiel Garcia wrote:
> There is currently little to no information available
> about the reasons why a v4l2-async device hasn't
> probed completely.
> 
> Inspired by the "devices_deferred" debugfs file,
> add a file to list information about the subdevices
> that are on waiting lists, for each notifier.
> 
> This is useful to debug v4l2-async subdevices
> and notifiers, for instance when doing device bring-up.
> 
> For instance, a typical output would be:
> 
> $ cat /sys/kernel/debug/video4linux/pending_async_subdevices
> ipu1_csi1:
>  [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi1_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi1_mux
> ipu1_csi0:
>  [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi0_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi0_mux
> imx6-mipi-csi2:
>  [fwnode] dev=1-003c, node=/soc/bus@2100000/i2c@21a4000/camera@3c
> imx-media:
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Nice, this should be quite useful.

Regards,

	Hans

> --
> v3:
> * Drop MATCH_CUSTOM
> * Use the actual fwnode, to print the real full path. endpoint name.
> * Fix refcounted handle leak.
> v2:
> * Print fwnode path, as suggested by Sakari.
> * Print the subdevices under their corresponding notifier.
> * Rename the file as suggested by Laurent.
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 66 ++++++++++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-dev.c   |  5 +++
>  include/media/v4l2-async.h           |  8 ++++
>  3 files changed, 79 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 3faf1d12d49d..7c8a97205b3e 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>   */
>  
> +#include <linux/debugfs.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
> @@ -14,6 +15,7 @@
>  #include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/seq_file.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  
> @@ -823,3 +825,67 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>  	mutex_unlock(&list_lock);
>  }
>  EXPORT_SYMBOL(v4l2_async_unregister_subdev);
> +
> +static void print_waiting_subdev(struct seq_file *s,
> +				 struct v4l2_async_subdev *asd)
> +{
> +	switch (asd->match_type) {
> +	case V4L2_ASYNC_MATCH_DEVNAME:
> +		seq_printf(s, " [devname] dev=%s\n", asd->match.device_name);
> +		break;
> +	case V4L2_ASYNC_MATCH_I2C:
> +		seq_printf(s, " [i2c] dev=%d-%04x\n", asd->match.i2c.adapter_id,
> +			   asd->match.i2c.address);
> +		break;
> +	case V4L2_ASYNC_MATCH_FWNODE: {
> +		struct fwnode_handle *devnode, *fwnode = asd->match.fwnode;
> +
> +		devnode = fwnode_graph_is_endpoint(fwnode) ?
> +			  fwnode_graph_get_port_parent(fwnode) :
> +			  fwnode_handle_get(fwnode);
> +
> +		seq_printf(s, " [fwnode] dev=%s, node=%pfw\n",
> +			   devnode->dev ? dev_name(devnode->dev) : "nil",
> +			   fwnode);
> +
> +		fwnode_handle_put(devnode);
> +		break;
> +	}
> +	}
> +}
> +
> +static const char *
> +v4l2_async_notifier_name(struct v4l2_async_notifier *notifier)
> +{
> +	if (notifier->v4l2_dev)
> +		return notifier->v4l2_dev->name;
> +	else if (notifier->sd)
> +		return notifier->sd->name;
> +	else
> +		return "nil";
> +}
> +
> +static int pending_subdevs_show(struct seq_file *s, void *data)
> +{
> +	struct v4l2_async_notifier *notif;
> +	struct v4l2_async_subdev *asd;
> +
> +	mutex_lock(&list_lock);
> +
> +	list_for_each_entry(notif, &notifier_list, list) {
> +		seq_printf(s, "%s:\n", v4l2_async_notifier_name(notif));
> +		list_for_each_entry(asd, &notif->waiting, list)
> +			print_waiting_subdev(s, asd);
> +	}
> +
> +	mutex_unlock(&list_lock);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(pending_subdevs);
> +
> +void v4l2_async_debug_init(struct dentry *debugfs_dir)
> +{
> +	debugfs_create_file("pending_async_subdevices", 0444, debugfs_dir, NULL,
> +			    &pending_subdevs_fops);
> +}
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index a593ea0598b5..8d3813e6ab56 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -14,6 +14,7 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> +#include <linux/debugfs.h>
>  #include <linux/module.h>
>  #include <linux/types.h>
>  #include <linux/kernel.h>
> @@ -37,6 +38,7 @@
>  		       __func__, ##arg);				\
>  } while (0)
>  
> +static struct dentry *v4l2_debugfs_dir;
>  
>  /*
>   *	sysfs stuff
> @@ -1113,6 +1115,8 @@ static int __init videodev_init(void)
>  		return -EIO;
>  	}
>  
> +	v4l2_debugfs_dir = debugfs_create_dir("video4linux", NULL);
> +	v4l2_async_debug_init(v4l2_debugfs_dir);
>  	return 0;
>  }
>  
> @@ -1120,6 +1124,7 @@ static void __exit videodev_exit(void)
>  {
>  	dev_t dev = MKDEV(VIDEO_MAJOR, 0);
>  
> +	debugfs_remove_recursive(v4l2_debugfs_dir);
>  	class_unregister(&video_class);
>  	unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
>  }
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 8ed42188e7c9..26296d4cb660 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -11,6 +11,7 @@
>  #include <linux/list.h>
>  #include <linux/mutex.h>
>  
> +struct dentry;
>  struct device;
>  struct device_node;
>  struct v4l2_device;
> @@ -120,6 +121,13 @@ struct v4l2_async_notifier {
>  	struct list_head list;
>  };
>  
> +/**
> + * v4l2_async_debug_init - Initialize debugging tools.
> + *
> + * @debugfs_dir: pointer to the parent debugfs &struct dentry
> + */
> +void v4l2_async_debug_init(struct dentry *debugfs_dir);
> +
>  /**
>   * v4l2_async_notifier_init - Initialize a notifier.
>   *
> 

Re: [PATCH v3] media: v4l2-async: Add waiting subdevices debugfs

[Ezequiel Garcia]

On Fri, 2021-01-15 at 01:36 +0200, Sakari Ailus wrote:
> Hi Ezequiel,
> 
> On Fri, Jan 08, 2021 at 02:17:28PM -0300, Ezequiel Garcia wrote:
> > There is currently little to no information available
> > about the reasons why a v4l2-async device hasn't
> > probed completely.
> > 
> > Inspired by the "devices_deferred" debugfs file,
> > add a file to list information about the subdevices
> > that are on waiting lists, for each notifier.
> > 
> > This is useful to debug v4l2-async subdevices
> > and notifiers, for instance when doing device bring-up.
> > 
> > For instance, a typical output would be:
> > 
> > $ cat /sys/kernel/debug/video4linux/pending_async_subdevices
> > ipu1_csi1:
> >  [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi1_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi1_mux
> > ipu1_csi0:
> >  [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi0_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi0_mux
> > imx6-mipi-csi2:
> >  [fwnode] dev=1-003c, node=/soc/bus@2100000/i2c@21a4000/camera@3c
> > imx-media:
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > --
> > v3:
> > * Drop MATCH_CUSTOM
> > * Use the actual fwnode, to print the real full path. endpoint name.
> > * Fix refcounted handle leak.
> > v2:
> > * Print fwnode path, as suggested by Sakari.
> > * Print the subdevices under their corresponding notifier.
> > * Rename the file as suggested by Laurent.
> 
> The patch itself seems fine. But lease put such notes below the '---' line
> below going forward. What's above it will be part of the commit message.
> 

Oops, sorry for the typo.

I have a v4 for this one, dropping MATCH_DEVNAME too.

Thanks,
Ezequiel

Re: [PATCH v3] media: v4l2-async: Add waiting subdevices debugfs

[Sakari Ailus]

On Fri, Jan 15, 2021 at 11:08:21AM -0300, Ezequiel Garcia wrote:
> On Fri, 2021-01-15 at 01:36 +0200, Sakari Ailus wrote:
> > Hi Ezequiel,
> > 
> > On Fri, Jan 08, 2021 at 02:17:28PM -0300, Ezequiel Garcia wrote:
> > > There is currently little to no information available
> > > about the reasons why a v4l2-async device hasn't
> > > probed completely.
> > > 
> > > Inspired by the "devices_deferred" debugfs file,
> > > add a file to list information about the subdevices
> > > that are on waiting lists, for each notifier.
> > > 
> > > This is useful to debug v4l2-async subdevices
> > > and notifiers, for instance when doing device bring-up.
> > > 
> > > For instance, a typical output would be:
> > > 
> > > $ cat /sys/kernel/debug/video4linux/pending_async_subdevices
> > > ipu1_csi1:
> > >  [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi1_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi1_mux
> > > ipu1_csi0:
> > >  [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi0_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi0_mux
> > > imx6-mipi-csi2:
> > >  [fwnode] dev=1-003c, node=/soc/bus@2100000/i2c@21a4000/camera@3c
> > > imx-media:
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > --
> > > v3:
> > > * Drop MATCH_CUSTOM
> > > * Use the actual fwnode, to print the real full path. endpoint name.
> > > * Fix refcounted handle leak.
> > > v2:
> > > * Print fwnode path, as suggested by Sakari.
> > > * Print the subdevices under their corresponding notifier.
> > > * Rename the file as suggested by Laurent.
> > 
> > The patch itself seems fine. But lease put such notes below the '---' line
> > below going forward. What's above it will be part of the commit message.
> > 
> 
> Oops, sorry for the typo.
> 
> I have a v4 for this one, dropping MATCH_DEVNAME too.

Could you remove it as a separate patch? It's unrelated to debugging.

-- 
Sakari Ailus

Re: [PATCH v3] media: v4l2-async: Add waiting subdevices debugfs

[Ezequiel Garcia]

On Fri, 2021-01-15 at 17:05 +0200, Sakari Ailus wrote:
> On Fri, Jan 15, 2021 at 11:08:21AM -0300, Ezequiel Garcia wrote:
> > On Fri, 2021-01-15 at 01:36 +0200, Sakari Ailus wrote:
> > > Hi Ezequiel,
> > > 
> > > On Fri, Jan 08, 2021 at 02:17:28PM -0300, Ezequiel Garcia wrote:
> > > > There is currently little to no information available
> > > > about the reasons why a v4l2-async device hasn't
> > > > probed completely.
> > > > 
> > > > Inspired by the "devices_deferred" debugfs file,
> > > > add a file to list information about the subdevices
> > > > that are on waiting lists, for each notifier.
> > > > 
> > > > This is useful to debug v4l2-async subdevices
> > > > and notifiers, for instance when doing device bring-up.
> > > > 
> > > > For instance, a typical output would be:
> > > > 
> > > > $ cat /sys/kernel/debug/video4linux/pending_async_subdevices
> > > > ipu1_csi1:
> > > >  [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi1_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi1_mux
> > > > ipu1_csi0:
> > > >  [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi0_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi0_mux
> > > > imx6-mipi-csi2:
> > > >  [fwnode] dev=1-003c, node=/soc/bus@2100000/i2c@21a4000/camera@3c
> > > > imx-media:
> > > > 
> > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > --
> > > > v3:
> > > > * Drop MATCH_CUSTOM
> > > > * Use the actual fwnode, to print the real full path. endpoint name.
> > > > * Fix refcounted handle leak.
> > > > v2:
> > > > * Print fwnode path, as suggested by Sakari.
> > > > * Print the subdevices under their corresponding notifier.
> > > > * Rename the file as suggested by Laurent.
> > > 
> > > The patch itself seems fine. But lease put such notes below the '---' line
> > > below going forward. What's above it will be part of the commit message.
> > > 
> > 
> > Oops, sorry for the typo.
> > 
> > I have a v4 for this one, dropping MATCH_DEVNAME too.
> 
> Could you remove it as a separate patch? It's unrelated to debugging.
> 

Yes, but still we won't be printing MATCH_DEVNAME types
so this patch needs a v4 dropping that.

I think it's worth it as it simplifies the patch a bit.

Thanks,
Ezequiel

[PATCH v4] media: v4l2-async: Add waiting subdevices debugfs

[Ezequiel Garcia]

There is currently little to no information available
about the reasons why a v4l2-async device hasn't
probed completely.

Inspired by the "devices_deferred" debugfs file,
add a file to list information about the subdevices
that are on waiting lists, for each notifier.

This is useful to debug v4l2-async subdevices
and notifiers, for instance when doing device bring-up.

For instance, a typical output would be:

$ cat /sys/kernel/debug/video4linux/pending_async_subdevices
ipu1_csi1:
 [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi1_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi1_mux
ipu1_csi0:
 [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi0_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi0_mux
imx6-mipi-csi2:
 [fwnode] dev=1-003c, node=/soc/bus@2100000/i2c@21a4000/camera@3c
imx-media:

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/v4l2-core/v4l2-async.c | 63 ++++++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-dev.c   |  5 +++
 include/media/v4l2-async.h           |  8 ++++
 3 files changed, 76 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index e3ab003a6c85..e35f18706792 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
  */
 
+#include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
@@ -14,6 +15,7 @@
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 
@@ -837,3 +839,64 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
 	mutex_unlock(&list_lock);
 }
 EXPORT_SYMBOL(v4l2_async_unregister_subdev);
+
+static void print_waiting_subdev(struct seq_file *s,
+				 struct v4l2_async_subdev *asd)
+{
+	switch (asd->match_type) {
+	case V4L2_ASYNC_MATCH_I2C:
+		seq_printf(s, " [i2c] dev=%d-%04x\n", asd->match.i2c.adapter_id,
+			   asd->match.i2c.address);
+		break;
+	case V4L2_ASYNC_MATCH_FWNODE: {
+		struct fwnode_handle *devnode, *fwnode = asd->match.fwnode;
+
+		devnode = fwnode_graph_is_endpoint(fwnode) ?
+			  fwnode_graph_get_port_parent(fwnode) :
+			  fwnode_handle_get(fwnode);
+
+		seq_printf(s, " [fwnode] dev=%s, node=%pfw\n",
+			   devnode->dev ? dev_name(devnode->dev) : "nil",
+			   fwnode);
+
+		fwnode_handle_put(devnode);
+		break;
+	}
+	}
+}
+
+static const char *
+v4l2_async_notifier_name(struct v4l2_async_notifier *notifier)
+{
+	if (notifier->v4l2_dev)
+		return notifier->v4l2_dev->name;
+	else if (notifier->sd)
+		return notifier->sd->name;
+	else
+		return "nil";
+}
+
+static int pending_subdevs_show(struct seq_file *s, void *data)
+{
+	struct v4l2_async_notifier *notif;
+	struct v4l2_async_subdev *asd;
+
+	mutex_lock(&list_lock);
+
+	list_for_each_entry(notif, &notifier_list, list) {
+		seq_printf(s, "%s:\n", v4l2_async_notifier_name(notif));
+		list_for_each_entry(asd, &notif->waiting, list)
+			print_waiting_subdev(s, asd);
+	}
+
+	mutex_unlock(&list_lock);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(pending_subdevs);
+
+void v4l2_async_debug_init(struct dentry *debugfs_dir)
+{
+	debugfs_create_file("pending_async_subdevices", 0444, debugfs_dir, NULL,
+			    &pending_subdevs_fops);
+}
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index f9cff033d0dc..b6a72d297775 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -14,6 +14,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/debugfs.h>
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
@@ -38,6 +39,7 @@
 		       __func__, ##arg);				\
 } while (0)
 
+static struct dentry *v4l2_debugfs_dir;
 
 /*
  *	sysfs stuff
@@ -1118,6 +1120,8 @@ static int __init videodev_init(void)
 		return -EIO;
 	}
 
+	v4l2_debugfs_dir = debugfs_create_dir("video4linux", NULL);
+	v4l2_async_debug_init(v4l2_debugfs_dir);
 	return 0;
 }
 
@@ -1125,6 +1129,7 @@ static void __exit videodev_exit(void)
 {
 	dev_t dev = MKDEV(VIDEO_MAJOR, 0);
 
+	debugfs_remove_recursive(v4l2_debugfs_dir);
 	class_unregister(&video_class);
 	unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
 }
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 0e04b5b2ebb0..243ac10a53c6 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -11,6 +11,7 @@
 #include <linux/list.h>
 #include <linux/mutex.h>
 
+struct dentry;
 struct device;
 struct device_node;
 struct v4l2_device;
@@ -137,6 +138,13 @@ struct v4l2_async_notifier {
 	struct list_head list;
 };
 
+/**
+ * v4l2_async_debug_init - Initialize debugging tools.
+ *
+ * @debugfs_dir: pointer to the parent debugfs &struct dentry
+ */
+void v4l2_async_debug_init(struct dentry *debugfs_dir);
+
 /**
  * v4l2_async_notifier_init - Initialize a notifier.
  *
-- 
2.29.2

Re: [PATCH] media: v4l2-async: Remove V4L2_ASYNC_MATCH_CUSTOM

[Kieran Bingham]

Hi Ezequiel,

On 08/01/2021 17:17, Ezequiel Garcia wrote:
> Custom/driver-specific v4l2-async match support was introduced
> in 2013, as V4L2_ASYNC_BUS_CUSTOM.
> 
> This type of match never had any user, so it's fair
> to conclude it's not required and that safe for removal.
> If the support is ever needed, it can always be restored.
> 

Simplify v4l2-async? Yes please!

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 14 --------------
>  include/media/v4l2-async.h           | 17 -----------------
>  2 files changed, 31 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index e3ab003a6c85..3faf1d12d49d 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -139,16 +139,6 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
>  	return true;
>  }
>  
> -static bool match_custom(struct v4l2_async_notifier *notifier,
> -			 struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> -{
> -	if (!asd->match.custom.match)
> -		/* Match always */
> -		return true;
> -
> -	return asd->match.custom.match(sd->dev, asd);
> -}
> -
>  static LIST_HEAD(subdev_list);
>  static LIST_HEAD(notifier_list);
>  static DEFINE_MUTEX(list_lock);
> @@ -164,9 +154,6 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier,
>  	list_for_each_entry(asd, &notifier->waiting, list) {
>  		/* bus_type has been verified valid before */
>  		switch (asd->match_type) {
> -		case V4L2_ASYNC_MATCH_CUSTOM:
> -			match = match_custom;
> -			break;
>  		case V4L2_ASYNC_MATCH_DEVNAME:
>  			match = match_devname;
>  			break;
> @@ -467,7 +454,6 @@ static int v4l2_async_notifier_asd_valid(struct v4l2_async_notifier *notifier,
>  		return -EINVAL;
>  
>  	switch (asd->match_type) {
> -	case V4L2_ASYNC_MATCH_CUSTOM:
>  	case V4L2_ASYNC_MATCH_DEVNAME:
>  	case V4L2_ASYNC_MATCH_I2C:
>  	case V4L2_ASYNC_MATCH_FWNODE:
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 0e04b5b2ebb0..8ed42188e7c9 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -21,8 +21,6 @@ struct v4l2_async_notifier;
>   * enum v4l2_async_match_type - type of asynchronous subdevice logic to be used
>   *	in order to identify a match
>   *
> - * @V4L2_ASYNC_MATCH_CUSTOM: Match will use the logic provided by &struct
> - *	v4l2_async_subdev.match ops
>   * @V4L2_ASYNC_MATCH_DEVNAME: Match will use the device name
>   * @V4L2_ASYNC_MATCH_I2C: Match will check for I2C adapter ID and address
>   * @V4L2_ASYNC_MATCH_FWNODE: Match will use firmware node
> @@ -31,7 +29,6 @@ struct v4l2_async_notifier;
>   * algorithm that will be used to match an asynchronous device.
>   */
>  enum v4l2_async_match_type {
> -	V4L2_ASYNC_MATCH_CUSTOM,
>  	V4L2_ASYNC_MATCH_DEVNAME,
>  	V4L2_ASYNC_MATCH_I2C,
>  	V4L2_ASYNC_MATCH_FWNODE,
> @@ -58,15 +55,6 @@ enum v4l2_async_match_type {
>   * @match.i2c.address:
>   *		I2C address to be matched.
>   *		Used if @match_type is %V4L2_ASYNC_MATCH_I2C.
> - * @match.custom:
> - *		Driver-specific match criteria.
> - *		Used if @match_type is %V4L2_ASYNC_MATCH_CUSTOM.
> - * @match.custom.match:
> - *		Driver-specific match function to be used if
> - *		%V4L2_ASYNC_MATCH_CUSTOM.
> - * @match.custom.priv:
> - *		Driver-specific private struct with match parameters
> - *		to be used if %V4L2_ASYNC_MATCH_CUSTOM.
>   * @asd_list:	used to add struct v4l2_async_subdev objects to the
>   *		master notifier @asd_list
>   * @list:	used to link struct v4l2_async_subdev objects, waiting to be
> @@ -85,11 +73,6 @@ struct v4l2_async_subdev {
>  			int adapter_id;
>  			unsigned short address;
>  		} i2c;
> -		struct {
> -			bool (*match)(struct device *dev,
> -				      struct v4l2_async_subdev *sd);
> -			void *priv;
> -		} custom;
>  	} match;
>  
>  	/* v4l2-async core private: not to be used by drivers */
> 

Re: [PATCH v4] media: v4l2-async: Add waiting subdevices debugfs

[Kieran Bingham]

Hi Ezequiel,

On 15/01/2021 19:14, Ezequiel Garcia wrote:
> There is currently little to no information available
> about the reasons why a v4l2-async device hasn't
> probed completely.
> 
> Inspired by the "devices_deferred" debugfs file,
> add a file to list information about the subdevices
> that are on waiting lists, for each notifier.
> 
> This is useful to debug v4l2-async subdevices
> and notifiers, for instance when doing device bring-up.
> 
> For instance, a typical output would be:
> 
> $ cat /sys/kernel/debug/video4linux/pending_async_subdevices
> ipu1_csi1:
>  [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi1_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi1_mux
> ipu1_csi0:
>  [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi0_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi0_mux
> imx6-mipi-csi2:
>  [fwnode] dev=1-003c, node=/soc/bus@2100000/i2c@21a4000/camera@3c
> imx-media:

Oh this is very exciting. I started looking at something like this
recently, hitting async failures, and this already looks better.

In other words, - Thank you!

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 63 ++++++++++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-dev.c   |  5 +++
>  include/media/v4l2-async.h           |  8 ++++
>  3 files changed, 76 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index e3ab003a6c85..e35f18706792 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>   */
>  
> +#include <linux/debugfs.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
> @@ -14,6 +15,7 @@
>  #include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/seq_file.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  
> @@ -837,3 +839,64 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>  	mutex_unlock(&list_lock);
>  }
>  EXPORT_SYMBOL(v4l2_async_unregister_subdev);
> +
> +static void print_waiting_subdev(struct seq_file *s,
> +				 struct v4l2_async_subdev *asd)
> +{
> +	switch (asd->match_type) {
> +	case V4L2_ASYNC_MATCH_I2C:
> +		seq_printf(s, " [i2c] dev=%d-%04x\n", asd->match.i2c.adapter_id,
> +			   asd->match.i2c.address);
> +		break;
> +	case V4L2_ASYNC_MATCH_FWNODE: {
> +		struct fwnode_handle *devnode, *fwnode = asd->match.fwnode;
> +
> +		devnode = fwnode_graph_is_endpoint(fwnode) ?
> +			  fwnode_graph_get_port_parent(fwnode) :
> +			  fwnode_handle_get(fwnode);
> +
> +		seq_printf(s, " [fwnode] dev=%s, node=%pfw\n",
> +			   devnode->dev ? dev_name(devnode->dev) : "nil",
> +			   fwnode);
> +
> +		fwnode_handle_put(devnode);
> +		break;
> +	}
> +	}
> +}
> +
> +static const char *
> +v4l2_async_notifier_name(struct v4l2_async_notifier *notifier)
> +{
> +	if (notifier->v4l2_dev)
> +		return notifier->v4l2_dev->name;
> +	else if (notifier->sd)
> +		return notifier->sd->name;
> +	else
> +		return "nil";
> +}
> +
> +static int pending_subdevs_show(struct seq_file *s, void *data)
> +{
> +	struct v4l2_async_notifier *notif;
> +	struct v4l2_async_subdev *asd;
> +
> +	mutex_lock(&list_lock);
> +
> +	list_for_each_entry(notif, &notifier_list, list) {
> +		seq_printf(s, "%s:\n", v4l2_async_notifier_name(notif));
> +		list_for_each_entry(asd, &notif->waiting, list)
> +			print_waiting_subdev(s, asd);
> +	}
> +
> +	mutex_unlock(&list_lock);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(pending_subdevs);
> +
> +void v4l2_async_debug_init(struct dentry *debugfs_dir)
> +{
> +	debugfs_create_file("pending_async_subdevices", 0444, debugfs_dir, NULL,
> +			    &pending_subdevs_fops);
> +}
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index f9cff033d0dc..b6a72d297775 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -14,6 +14,7 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> +#include <linux/debugfs.h>
>  #include <linux/module.h>
>  #include <linux/types.h>
>  #include <linux/kernel.h>
> @@ -38,6 +39,7 @@
>  		       __func__, ##arg);				\
>  } while (0)
>  
> +static struct dentry *v4l2_debugfs_dir;
>  
>  /*
>   *	sysfs stuff
> @@ -1118,6 +1120,8 @@ static int __init videodev_init(void)
>  		return -EIO;
>  	}
>  
> +	v4l2_debugfs_dir = debugfs_create_dir("video4linux", NULL);
> +	v4l2_async_debug_init(v4l2_debugfs_dir);
>  	return 0;
>  }
>  
> @@ -1125,6 +1129,7 @@ static void __exit videodev_exit(void)
>  {
>  	dev_t dev = MKDEV(VIDEO_MAJOR, 0);
>  
> +	debugfs_remove_recursive(v4l2_debugfs_dir);
>  	class_unregister(&video_class);
>  	unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
>  }
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 0e04b5b2ebb0..243ac10a53c6 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -11,6 +11,7 @@
>  #include <linux/list.h>
>  #include <linux/mutex.h>
>  
> +struct dentry;
>  struct device;
>  struct device_node;
>  struct v4l2_device;
> @@ -137,6 +138,13 @@ struct v4l2_async_notifier {
>  	struct list_head list;
>  };
>  
> +/**
> + * v4l2_async_debug_init - Initialize debugging tools.
> + *
> + * @debugfs_dir: pointer to the parent debugfs &struct dentry
> + */
> +void v4l2_async_debug_init(struct dentry *debugfs_dir);
> +
>  /**
>   * v4l2_async_notifier_init - Initialize a notifier.
>   *
> 

Re: [PATCH v4] media: v4l2-async: Add waiting subdevices debugfs

[Kieran Bingham]

Hi Ezequiel,

On 15/01/2021 19:47, Kieran Bingham wrote:
> Hi Ezequiel,
> 
> On 15/01/2021 19:14, Ezequiel Garcia wrote:
>> There is currently little to no information available
>> about the reasons why a v4l2-async device hasn't
>> probed completely.
>>
>> Inspired by the "devices_deferred" debugfs file,
>> add a file to list information about the subdevices
>> that are on waiting lists, for each notifier.
>>
>> This is useful to debug v4l2-async subdevices
>> and notifiers, for instance when doing device bring-up.
>>
>> For instance, a typical output would be:
>>
>> $ cat /sys/kernel/debug/video4linux/pending_async_subdevices
>> ipu1_csi1:
>>  [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi1_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi1_mux
>> ipu1_csi0:
>>  [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi0_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi0_mux
>> imx6-mipi-csi2:
>>  [fwnode] dev=1-003c, node=/soc/bus@2100000/i2c@21a4000/camera@3c
>> imx-media:
> 
> Oh this is very exciting. I started looking at something like this
> recently, hitting async failures, and this already looks better.
> 
> In other words, - Thank you!
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


Throwing in an extra tag:

> root@salvator-x:/home/kbingham# cat /sys/kernel/debug/video4linux/pending_async_subdevices 
> max9286 4-006c:
>  [fwnode] dev=15-0035, node=/soc/i2c@e66d8000/gmsl-deserializer@6c/i2c-mux/i2c@0/camera@35/port/endpoint
> max9286 4-004c:
>  [fwnode] dev=10-0031, node=/soc/i2c@e66d8000/gmsl-deserializer@4c/i2c-mux/i2c@0/camera@31/port/endpoint
>  [fwnode] dev=11-0032, node=/soc/i2c@e66d8000/gmsl-deserializer@4c/i2c-mux/i2c@1/camera@32/port/endpoint
>  [fwnode] dev=12-0033, node=/soc/i2c@e66d8000/gmsl-deserializer@4c/i2c-mux/i2c@2/camera@33/port/endpoint
>  [fwnode] dev=13-0034, node=/soc/i2c@e66d8000/gmsl-deserializer@4c/i2c-mux/i2c@3/camera@34/port/endpoint
> rcar-vin e6ef7000.video:
> rcar_csi2 feab0000.csi2:
> rcar_csi2 feaa0000.csi2:
> rcar_csi2 fea80000.csi2:


That looks good and really helpful to me ;-)

Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> 
>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/v4l2-core/v4l2-async.c | 63 ++++++++++++++++++++++++++++
>>  drivers/media/v4l2-core/v4l2-dev.c   |  5 +++
>>  include/media/v4l2-async.h           |  8 ++++
>>  3 files changed, 76 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>> index e3ab003a6c85..e35f18706792 100644
>> --- a/drivers/media/v4l2-core/v4l2-async.c
>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>> @@ -5,6 +5,7 @@
>>   * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>   */
>>  
>> +#include <linux/debugfs.h>
>>  #include <linux/device.h>
>>  #include <linux/err.h>
>>  #include <linux/i2c.h>
>> @@ -14,6 +15,7 @@
>>  #include <linux/mutex.h>
>>  #include <linux/of.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/seq_file.h>
>>  #include <linux/slab.h>
>>  #include <linux/types.h>
>>  
>> @@ -837,3 +839,64 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>>  	mutex_unlock(&list_lock);
>>  }
>>  EXPORT_SYMBOL(v4l2_async_unregister_subdev);
>> +
>> +static void print_waiting_subdev(struct seq_file *s,
>> +				 struct v4l2_async_subdev *asd)
>> +{
>> +	switch (asd->match_type) {
>> +	case V4L2_ASYNC_MATCH_I2C:
>> +		seq_printf(s, " [i2c] dev=%d-%04x\n", asd->match.i2c.adapter_id,
>> +			   asd->match.i2c.address);
>> +		break;
>> +	case V4L2_ASYNC_MATCH_FWNODE: {
>> +		struct fwnode_handle *devnode, *fwnode = asd->match.fwnode;
>> +
>> +		devnode = fwnode_graph_is_endpoint(fwnode) ?
>> +			  fwnode_graph_get_port_parent(fwnode) :
>> +			  fwnode_handle_get(fwnode);
>> +
>> +		seq_printf(s, " [fwnode] dev=%s, node=%pfw\n",
>> +			   devnode->dev ? dev_name(devnode->dev) : "nil",
>> +			   fwnode);
>> +
>> +		fwnode_handle_put(devnode);
>> +		break;
>> +	}
>> +	}
>> +}
>> +
>> +static const char *
>> +v4l2_async_notifier_name(struct v4l2_async_notifier *notifier)
>> +{
>> +	if (notifier->v4l2_dev)
>> +		return notifier->v4l2_dev->name;
>> +	else if (notifier->sd)
>> +		return notifier->sd->name;
>> +	else
>> +		return "nil";
>> +}
>> +
>> +static int pending_subdevs_show(struct seq_file *s, void *data)
>> +{
>> +	struct v4l2_async_notifier *notif;
>> +	struct v4l2_async_subdev *asd;
>> +
>> +	mutex_lock(&list_lock);
>> +
>> +	list_for_each_entry(notif, &notifier_list, list) {
>> +		seq_printf(s, "%s:\n", v4l2_async_notifier_name(notif));
>> +		list_for_each_entry(asd, &notif->waiting, list)
>> +			print_waiting_subdev(s, asd);
>> +	}
>> +
>> +	mutex_unlock(&list_lock);
>> +
>> +	return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(pending_subdevs);
>> +
>> +void v4l2_async_debug_init(struct dentry *debugfs_dir)
>> +{
>> +	debugfs_create_file("pending_async_subdevices", 0444, debugfs_dir, NULL,
>> +			    &pending_subdevs_fops);
>> +}
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index f9cff033d0dc..b6a72d297775 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -14,6 +14,7 @@
>>  
>>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>  
>> +#include <linux/debugfs.h>
>>  #include <linux/module.h>
>>  #include <linux/types.h>
>>  #include <linux/kernel.h>
>> @@ -38,6 +39,7 @@
>>  		       __func__, ##arg);				\
>>  } while (0)
>>  
>> +static struct dentry *v4l2_debugfs_dir;
>>  
>>  /*
>>   *	sysfs stuff
>> @@ -1118,6 +1120,8 @@ static int __init videodev_init(void)
>>  		return -EIO;
>>  	}
>>  
>> +	v4l2_debugfs_dir = debugfs_create_dir("video4linux", NULL);
>> +	v4l2_async_debug_init(v4l2_debugfs_dir);
>>  	return 0;
>>  }
>>  
>> @@ -1125,6 +1129,7 @@ static void __exit videodev_exit(void)
>>  {
>>  	dev_t dev = MKDEV(VIDEO_MAJOR, 0);
>>  
>> +	debugfs_remove_recursive(v4l2_debugfs_dir);
>>  	class_unregister(&video_class);
>>  	unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
>>  }
>> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
>> index 0e04b5b2ebb0..243ac10a53c6 100644
>> --- a/include/media/v4l2-async.h
>> +++ b/include/media/v4l2-async.h
>> @@ -11,6 +11,7 @@
>>  #include <linux/list.h>
>>  #include <linux/mutex.h>
>>  
>> +struct dentry;
>>  struct device;
>>  struct device_node;
>>  struct v4l2_device;
>> @@ -137,6 +138,13 @@ struct v4l2_async_notifier {
>>  	struct list_head list;
>>  };
>>  
>> +/**
>> + * v4l2_async_debug_init - Initialize debugging tools.
>> + *
>> + * @debugfs_dir: pointer to the parent debugfs &struct dentry
>> + */
>> +void v4l2_async_debug_init(struct dentry *debugfs_dir);
>> +
>>  /**
>>   * v4l2_async_notifier_init - Initialize a notifier.
>>   *
>>
>