All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] media: v4l2-async: Add waiting subdevices debugfs
@ 2021-01-04 17:48 Ezequiel Garcia
  2021-01-05  0:20 ` Sakari Ailus
  2021-01-05  3:46 ` Laurent Pinchart
  0 siblings, 2 replies; 8+ messages in thread
From: Ezequiel Garcia @ 2021-01-04 17:48 UTC (permalink / raw)
  To: linux-media, Hans Verkuil
  Cc: kernel, Laurent Pinchart, Sakari Ailus, Ezequiel Garcia

There is currently little to none 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:

Note that match-type "custom" prints no information.
Since there are no in-tree users of this match-type,
the implementation doesn't bother.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
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           |  9 ++++
 3 files changed, 80 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index e3ab003a6c85..d779808abb3b 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,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_CUSTOM:
+		seq_puts(s, " [custom]\n");
+		break;
+	case V4L2_ASYNC_MATCH_DEVNAME:
+		seq_printf(s, " [devname] %s\n", asd->match.device_name);
+		break;
+	case V4L2_ASYNC_MATCH_I2C:
+		seq_printf(s, " [i2c] %d-%04x\n", asd->match.i2c.adapter_id,
+			   asd->match.i2c.address);
+		break;
+	case V4L2_ASYNC_MATCH_FWNODE: {
+		struct fwnode_handle *fwnode = asd->match.fwnode;
+
+		if (fwnode_graph_is_endpoint(fwnode))
+			fwnode = fwnode_graph_get_port_parent(fwnode);
+
+		seq_printf(s, " [fwnode] dev=%s, node=%pfw\n",
+			   fwnode->dev ? dev_name(fwnode->dev) : "nil",
+			   fwnode);
+		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 0e04b5b2ebb0..abc85474bb3b 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -8,9 +8,11 @@
 #ifndef V4L2_ASYNC_H
 #define V4L2_ASYNC_H
 
+#include <linux/debugfs.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
 
+struct dentry;
 struct device;
 struct device_node;
 struct v4l2_device;
@@ -137,6 +139,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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] media: v4l2-async: Add waiting subdevices debugfs
  2021-01-04 17:48 [PATCH v2] media: v4l2-async: Add waiting subdevices debugfs Ezequiel Garcia
@ 2021-01-05  0:20 ` Sakari Ailus
  2021-01-07 20:58   ` Ezequiel Garcia
  2021-01-05  3:46 ` Laurent Pinchart
  1 sibling, 1 reply; 8+ messages in thread
From: Sakari Ailus @ 2021-01-05  0:20 UTC (permalink / raw)
  To: Ezequiel Garcia; +Cc: linux-media, Hans Verkuil, kernel, Laurent Pinchart

Hi Ezequiel,

On Mon, Jan 04, 2021 at 02:48:40PM -0300, Ezequiel Garcia wrote:
> There is currently little to none 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:
> 
> Note that match-type "custom" prints no information.
> Since there are no in-tree users of this match-type,
> the implementation doesn't bother.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
> 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           |  9 ++++
>  3 files changed, 80 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index e3ab003a6c85..d779808abb3b 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,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_CUSTOM:
> +		seq_puts(s, " [custom]\n");
> +		break;
> +	case V4L2_ASYNC_MATCH_DEVNAME:
> +		seq_printf(s, " [devname] %s\n", asd->match.device_name);
> +		break;
> +	case V4L2_ASYNC_MATCH_I2C:
> +		seq_printf(s, " [i2c] %d-%04x\n", asd->match.i2c.adapter_id,
> +			   asd->match.i2c.address);
> +		break;
> +	case V4L2_ASYNC_MATCH_FWNODE: {
> +		struct fwnode_handle *fwnode = asd->match.fwnode;
> +
> +		if (fwnode_graph_is_endpoint(fwnode))
> +			fwnode = fwnode_graph_get_port_parent(fwnode);

Could you print the endpoint node name as-is? That's what matching uses,
too. You'd need one more local variable for that I think.

> +
> +		seq_printf(s, " [fwnode] dev=%s, node=%pfw\n",
> +			   fwnode->dev ? dev_name(fwnode->dev) : "nil",
> +			   fwnode);
> +		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 0e04b5b2ebb0..abc85474bb3b 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -8,9 +8,11 @@
>  #ifndef V4L2_ASYNC_H
>  #define V4L2_ASYNC_H
>  
> +#include <linux/debugfs.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
>  
> +struct dentry;
>  struct device;
>  struct device_node;
>  struct v4l2_device;
> @@ -137,6 +139,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.
>   *

-- 
Regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] media: v4l2-async: Add waiting subdevices debugfs
  2021-01-04 17:48 [PATCH v2] media: v4l2-async: Add waiting subdevices debugfs Ezequiel Garcia
  2021-01-05  0:20 ` Sakari Ailus
@ 2021-01-05  3:46 ` Laurent Pinchart
  2021-01-05 13:34   ` Ezequiel Garcia
  1 sibling, 1 reply; 8+ messages in thread
From: Laurent Pinchart @ 2021-01-05  3:46 UTC (permalink / raw)
  To: Ezequiel Garcia; +Cc: linux-media, Hans Verkuil, kernel, Sakari Ailus

Hi Ezequiel,

Thank you for the patch.

On Mon, Jan 04, 2021 at 02:48:40PM -0300, Ezequiel Garcia wrote:
> There is currently little to none 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:
> 
> Note that match-type "custom" prints no information.
> Since there are no in-tree users of this match-type,
> the implementation doesn't bother.

I wonder if we should drop it.

> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
> 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           |  9 ++++
>  3 files changed, 80 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index e3ab003a6c85..d779808abb3b 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,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_CUSTOM:
> +		seq_puts(s, " [custom]\n");
> +		break;
> +	case V4L2_ASYNC_MATCH_DEVNAME:
> +		seq_printf(s, " [devname] %s\n", asd->match.device_name);
> +		break;
> +	case V4L2_ASYNC_MATCH_I2C:
> +		seq_printf(s, " [i2c] %d-%04x\n", asd->match.i2c.adapter_id,
> +			   asd->match.i2c.address);
> +		break;
> +	case V4L2_ASYNC_MATCH_FWNODE: {
> +		struct fwnode_handle *fwnode = asd->match.fwnode;
> +
> +		if (fwnode_graph_is_endpoint(fwnode))
> +			fwnode = fwnode_graph_get_port_parent(fwnode);
> +
> +		seq_printf(s, " [fwnode] dev=%s, node=%pfw\n",
> +			   fwnode->dev ? dev_name(fwnode->dev) : "nil",
> +			   fwnode);

Apart from Sakari's comment related to printing the endpoint node (but
keeping the port's parent for ->dev),

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

> +		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 0e04b5b2ebb0..abc85474bb3b 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -8,9 +8,11 @@
>  #ifndef V4L2_ASYNC_H
>  #define V4L2_ASYNC_H
>  
> +#include <linux/debugfs.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
>  
> +struct dentry;
>  struct device;
>  struct device_node;
>  struct v4l2_device;
> @@ -137,6 +139,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
> 

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] media: v4l2-async: Add waiting subdevices debugfs
  2021-01-05  3:46 ` Laurent Pinchart
@ 2021-01-05 13:34   ` Ezequiel Garcia
  2021-01-05 14:20     ` Sakari Ailus
  0 siblings, 1 reply; 8+ messages in thread
From: Ezequiel Garcia @ 2021-01-05 13:34 UTC (permalink / raw)
  To: Laurent Pinchart, Sakari Ailus; +Cc: linux-media, Hans Verkuil, kernel

Hi Sakari, Laurent,

On Tue, 2021-01-05 at 05:46 +0200, Laurent Pinchart wrote:
> Hi Ezequiel,
> 
> Thank you for the patch.
> 
> On Mon, Jan 04, 2021 at 02:48:40PM -0300, Ezequiel Garcia wrote:
> > There is currently little to none 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:
> > 
> > Note that match-type "custom" prints no information.
> > Since there are no in-tree users of this match-type,
> > the implementation doesn't bother.
> 
> I wonder if we should drop it.
> 

That would make sense, unless we have some out-of-tree
users, and we want to support that.

Sakari, what do you think?

> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> > 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           |  9 ++++
> >  3 files changed, 80 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index e3ab003a6c85..d779808abb3b 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,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_CUSTOM:
> > +               seq_puts(s, " [custom]\n");
> > +               break;
> > +       case V4L2_ASYNC_MATCH_DEVNAME:
> > +               seq_printf(s, " [devname] %s\n", asd->match.device_name);
> > +               break;
> > +       case V4L2_ASYNC_MATCH_I2C:
> > +               seq_printf(s, " [i2c] %d-%04x\n", asd->match.i2c.adapter_id,
> > +                          asd->match.i2c.address);
> > +               break;
> > +       case V4L2_ASYNC_MATCH_FWNODE: {
> > +               struct fwnode_handle *fwnode = asd->match.fwnode;
> > +
> > +               if (fwnode_graph_is_endpoint(fwnode))
> > +                       fwnode = fwnode_graph_get_port_parent(fwnode);
> > +
> > +               seq_printf(s, " [fwnode] dev=%s, node=%pfw\n",
> > +                          fwnode->dev ? dev_name(fwnode->dev) : "nil",
> > +                          fwnode);
> 
> Apart from Sakari's comment related to printing the endpoint node (but
> keeping the port's parent for ->dev),
> 

Yes, makes some sense.

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

Thanks for reviewing,
Ezequiel


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] media: v4l2-async: Add waiting subdevices debugfs
  2021-01-05 13:34   ` Ezequiel Garcia
@ 2021-01-05 14:20     ` Sakari Ailus
  0 siblings, 0 replies; 8+ messages in thread
From: Sakari Ailus @ 2021-01-05 14:20 UTC (permalink / raw)
  To: Ezequiel Garcia; +Cc: Laurent Pinchart, linux-media, Hans Verkuil, kernel

Hi Ezequiel,

On Tue, Jan 05, 2021 at 10:34:14AM -0300, Ezequiel Garcia wrote:
> Hi Sakari, Laurent,
> 
> On Tue, 2021-01-05 at 05:46 +0200, Laurent Pinchart wrote:
> > Hi Ezequiel,
> > 
> > Thank you for the patch.
> > 
> > On Mon, Jan 04, 2021 at 02:48:40PM -0300, Ezequiel Garcia wrote:
> > > There is currently little to none 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:
> > > 
> > > Note that match-type "custom" prints no information.
> > > Since there are no in-tree users of this match-type,
> > > the implementation doesn't bother.
> > 
> > I wonder if we should drop it.
> > 
> 
> That would make sense, unless we have some out-of-tree
> users, and we want to support that.
> 
> Sakari, what do you think?

I think we could drop it. But I'd do that in a separate patch from this
one.

-- 
Regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] media: v4l2-async: Add waiting subdevices debugfs
  2021-01-05  0:20 ` Sakari Ailus
@ 2021-01-07 20:58   ` Ezequiel Garcia
  2021-01-07 21:12     ` Laurent Pinchart
  0 siblings, 1 reply; 8+ messages in thread
From: Ezequiel Garcia @ 2021-01-07 20:58 UTC (permalink / raw)
  To: Sakari Ailus, Laurent Pinchart; +Cc: linux-media, Hans Verkuil, kernel

Hi Sakari, Laurent,

On Tue, 2021-01-05 at 02:20 +0200, Sakari Ailus wrote:
> Hi Ezequiel,
> 
> On Mon, Jan 04, 2021 at 02:48:40PM -0300, Ezequiel Garcia wrote:
> > There is currently little to none 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:
> > 
> > Note that match-type "custom" prints no information.
> > Since there are no in-tree users of this match-type,
> > the implementation doesn't bother.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> > 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           |  9 ++++
> >  3 files changed, 80 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index e3ab003a6c85..d779808abb3b 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,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_CUSTOM:
> > +               seq_puts(s, " [custom]\n");
> > +               break;
> > +       case V4L2_ASYNC_MATCH_DEVNAME:
> > +               seq_printf(s, " [devname] %s\n", asd->match.device_name);
> > +               break;
> > +       case V4L2_ASYNC_MATCH_I2C:
> > +               seq_printf(s, " [i2c] %d-%04x\n", asd->match.i2c.adapter_id,
> > +                          asd->match.i2c.address);
> > +               break;
> > +       case V4L2_ASYNC_MATCH_FWNODE: {
> > +               struct fwnode_handle *fwnode = asd->match.fwnode;
> > +
> > +               if (fwnode_graph_is_endpoint(fwnode))
> > +                       fwnode = fwnode_graph_get_port_parent(fwnode);
> 
> Could you print the endpoint node name as-is? That's what matching uses,
> too. You'd need one more local variable for that I think.
> 

Since we are printing the full path for the node, how about this:

                devnode = fwnode_graph_is_endpoint(fwnode) ?                     
                          fwnode_graph_get_port_parent(fwnode) : fwnode;         
                                                                                 
                seq_printf(s, " [fwnode] dev=%s, node=%pfw\n",                   
                           devnode->dev ? dev_name(devnode->dev) : "nil",        
                           fwnode);                                              

The parent is used only to print a more useful dev_name,
but otherwise the actual full path is used,
whether it's an endpoint or not.

Ezequiel 



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] media: v4l2-async: Add waiting subdevices debugfs
  2021-01-07 20:58   ` Ezequiel Garcia
@ 2021-01-07 21:12     ` Laurent Pinchart
  2021-01-07 22:08       ` Sakari Ailus
  0 siblings, 1 reply; 8+ messages in thread
From: Laurent Pinchart @ 2021-01-07 21:12 UTC (permalink / raw)
  To: Ezequiel Garcia; +Cc: Sakari Ailus, linux-media, Hans Verkuil, kernel

Hi Ezequiel,

On Thu, Jan 07, 2021 at 05:58:04PM -0300, Ezequiel Garcia wrote:
> On Tue, 2021-01-05 at 02:20 +0200, Sakari Ailus wrote:
> > On Mon, Jan 04, 2021 at 02:48:40PM -0300, Ezequiel Garcia wrote:
> > > There is currently little to none 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:
> > > 
> > > Note that match-type "custom" prints no information.
> > > Since there are no in-tree users of this match-type,
> > > the implementation doesn't bother.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > ---
> > > 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           |  9 ++++
> > >  3 files changed, 80 insertions(+)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > index e3ab003a6c85..d779808abb3b 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,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_CUSTOM:
> > > +               seq_puts(s, " [custom]\n");
> > > +               break;
> > > +       case V4L2_ASYNC_MATCH_DEVNAME:
> > > +               seq_printf(s, " [devname] %s\n", asd->match.device_name);
> > > +               break;
> > > +       case V4L2_ASYNC_MATCH_I2C:
> > > +               seq_printf(s, " [i2c] %d-%04x\n", asd->match.i2c.adapter_id,
> > > +                          asd->match.i2c.address);
> > > +               break;
> > > +       case V4L2_ASYNC_MATCH_FWNODE: {
> > > +               struct fwnode_handle *fwnode = asd->match.fwnode;
> > > +
> > > +               if (fwnode_graph_is_endpoint(fwnode))
> > > +                       fwnode = fwnode_graph_get_port_parent(fwnode);
> > 
> > Could you print the endpoint node name as-is? That's what matching uses,
> > too. You'd need one more local variable for that I think.
> > 
> 
> Since we are printing the full path for the node, how about this:
> 
>                 devnode = fwnode_graph_is_endpoint(fwnode) ?                     
>                           fwnode_graph_get_port_parent(fwnode) : fwnode;         
>                                                                                  
>                 seq_printf(s, " [fwnode] dev=%s, node=%pfw\n",                   
>                            devnode->dev ? dev_name(devnode->dev) : "nil",        
>                            fwnode);                                              
> 
> The parent is used only to print a more useful dev_name,
> but otherwise the actual full path is used,
> whether it's an endpoint or not.

With the refcount issue handled (fwnode_graph_get_port_parent() takes a
reference), it looks good to me.

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] media: v4l2-async: Add waiting subdevices debugfs
  2021-01-07 21:12     ` Laurent Pinchart
@ 2021-01-07 22:08       ` Sakari Ailus
  0 siblings, 0 replies; 8+ messages in thread
From: Sakari Ailus @ 2021-01-07 22:08 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Ezequiel Garcia, linux-media, Hans Verkuil, kernel

On Thu, Jan 07, 2021 at 11:12:32PM +0200, Laurent Pinchart wrote:
> Hi Ezequiel,
> 
> On Thu, Jan 07, 2021 at 05:58:04PM -0300, Ezequiel Garcia wrote:
> > On Tue, 2021-01-05 at 02:20 +0200, Sakari Ailus wrote:
> > > On Mon, Jan 04, 2021 at 02:48:40PM -0300, Ezequiel Garcia wrote:
> > > > There is currently little to none 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:
> > > > 
> > > > Note that match-type "custom" prints no information.
> > > > Since there are no in-tree users of this match-type,
> > > > the implementation doesn't bother.
> > > > 
> > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > > ---
> > > > 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           |  9 ++++
> > > >  3 files changed, 80 insertions(+)
> > > > 
> > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > > index e3ab003a6c85..d779808abb3b 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,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_CUSTOM:
> > > > +               seq_puts(s, " [custom]\n");
> > > > +               break;
> > > > +       case V4L2_ASYNC_MATCH_DEVNAME:
> > > > +               seq_printf(s, " [devname] %s\n", asd->match.device_name);
> > > > +               break;
> > > > +       case V4L2_ASYNC_MATCH_I2C:
> > > > +               seq_printf(s, " [i2c] %d-%04x\n", asd->match.i2c.adapter_id,
> > > > +                          asd->match.i2c.address);
> > > > +               break;
> > > > +       case V4L2_ASYNC_MATCH_FWNODE: {
> > > > +               struct fwnode_handle *fwnode = asd->match.fwnode;
> > > > +
> > > > +               if (fwnode_graph_is_endpoint(fwnode))
> > > > +                       fwnode = fwnode_graph_get_port_parent(fwnode);
> > > 
> > > Could you print the endpoint node name as-is? That's what matching uses,
> > > too. You'd need one more local variable for that I think.
> > > 
> > 
> > Since we are printing the full path for the node, how about this:
> > 
> >                 devnode = fwnode_graph_is_endpoint(fwnode) ?                     
> >                           fwnode_graph_get_port_parent(fwnode) : fwnode;         
> >                                                                                  
> >                 seq_printf(s, " [fwnode] dev=%s, node=%pfw\n",                   
> >                            devnode->dev ? dev_name(devnode->dev) : "nil",        
> >                            fwnode);                                              
> > 
> > The parent is used only to print a more useful dev_name,
> > but otherwise the actual full path is used,
> > whether it's an endpoint or not.
> 
> With the refcount issue handled (fwnode_graph_get_port_parent() takes a
> reference), it looks good to me.

Agreed. I'd just use fwnode_handle_get() on the right-most operand, and do
fwnode_handle_put() when done.

-- 
Regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-01-07 22:10 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-04 17:48 [PATCH v2] media: v4l2-async: Add waiting subdevices debugfs Ezequiel Garcia
2021-01-05  0:20 ` Sakari Ailus
2021-01-07 20:58   ` Ezequiel Garcia
2021-01-07 21:12     ` Laurent Pinchart
2021-01-07 22:08       ` Sakari Ailus
2021-01-05  3:46 ` Laurent Pinchart
2021-01-05 13:34   ` Ezequiel Garcia
2021-01-05 14:20     ` Sakari Ailus

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.