On Tue, Dec 29, 2020 at 02:52:52PM -0300, Ezequiel Garcia wrote:

On Tue, 2020-12-29 at 16:14 +0200, Laurent Pinchart wrote:

quoted

Hi Ezequiel,

On Tue, Dec 29, 2020 at 07:16:41AM -0300, Ezequiel Garcia wrote:

quoted

On Mon, 2020-12-28 at 23:28 +0200, Laurent Pinchart wrote:

quoted

On Mon, Dec 28, 2020 at 08:35:20PM +0200, Sakari Ailus wrote:

quoted

On Mon, Dec 28, 2020 at 03:05:11PM -0300, Ezequiel Garcia wrote:

quoted

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/waiting_subdevices
[fwnode] 1-003c
[fwnode] 20e0000.iomuxc-gpr:ipu1_csi1_mux
[fwnode] 20e0000.iomuxc-gpr:ipu1_csi0_mux

It's possible to provide some more information, detecting
the type of fwnode and printing of-specific or acpi-specific
details. For now, the implementation is kept simple.

The rest of the debug information we're effectively providing through
kernel messages on DEBUG level (pr_debug/dev_dbg). Could we do the same
here?

Would just printing the names of the pending sub-devices at notifier
register and async subdevice register time be sufficient? That way you'd
also be fine with just dmesg output if you're asking someone to provide you
information from another system.

I think debugfs would be better. It can show the current state of an
async notifier in a single place, which is easier to parse than
reconstructing it from kernel messages and implicit knowledge of the
code. I'd expect users to have an easier time debugging probe issues
with such centralized information.

quoted

quoted

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

Lines up to 74 characters are fine. Only in Gerrit it's 60 or 40 or
whatever characters. ;-)

quoted

Signed-off-by: Ezequiel Garcia <redacted>
---
 drivers/media/v4l2-core/v4l2-async.c | 54 ++++++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-dev.c   |  5 +++
 include/media/v4l2-async.h           |  7 ++++
 3 files changed, 66 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index e3ab003a6c85..32cd1ecced97 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 [off-list ref]
  */
 
+#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,55 @@ 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);

Can we also print endpoint information ?

What endpoint information do you have in mind? I'm asking this
because I printed endpoint OF node full names, only to find
so many of them named "endpoint" :)

The port name and endpoint name would be useful. The full fwnode name
would be an acceptable way to print that I think.

Makes sense, and since we'd be parsing the fwnode subtype,
we'll be able to do something like:

[of] dev=%s, node=%s
[swnode] ...
[acpi] ...

Could you simply print the node name, %pfw?

It works independently of the firmware interface and contains all the
relevant information.

-- 
Sakari Ailus