linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 00/14] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows
@ 2020-12-24  1:08 Daniel Scally
  2020-12-24  1:08 ` [PATCH v3 01/14] software_node: Fix refcounts in software_node_get_next_child() Daniel Scally
                   ` (13 more replies)
  0 siblings, 14 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-24  1:08 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-media, devel
  Cc: rjw, lenb, gregkh, yong.zhi, sakari.ailus, bingbu.cao,
	tian.shu.qiu, mchehab, robert.moore, erik.kaneda, pmladek,
	rostedt, sergey.senozhatsky, andriy.shevchenko, linux,
	laurent.pinchart+renesas, jacopo+renesas, kieran.bingham+renesas,
	hverkuil-cisco, m.felsch, niklas.soderlund+renesas, slongerbeam,
	heikki.krogerus, linus.walleij

Hello all

v2:
https://lore.kernel.org/linux-media/20201217234337.1983732-1-djrscally@gmail.com/T/#md93fd090009b42a6a98aed892aff0d38cf07e0cd
v1:
https://lore.kernel.org/linux-media/20201130133129.1024662-1-djrscally@gmail.com/T/#m91934e12e3d033da2e768e952ea3b4a125ee3e67
The RFC version before that:
https://lore.kernel.org/linux-media/20201019225903.14276-1-djrscally@gmail.com/

This series is to start adding support for webcams on laptops with ACPI tables
designed for use with CIO2 on Windows. This problem has two main parts; the
first part, which is handled in this series, is extending the ipu3-cio2
driver to allow for patching the firmware via software_nodes if endpoints
aren't defined by ACPI. The second is adding a new driver to handle power,
clocks and GPIO pins defined in DSDT tables in an awkward way. I decided to
split that second part out from this series, and instead give it its own
series (a v2 of which should land "soon"). The reasons for that are:

1. It's a logically separate change anyway
2. The recipients list was getting really long and
3. That probably meant that handling merge for all of this in one go was
   going to be impractically awkward.

I'm hopeful that most or all of this series could get picked up for 5.12.
We touch a few different areas (listed below), but I think the easiest
approach would be to merge everything through media tree. Rafael, Greg,
Mauro and Sergey; are you ok with that plan, or would you prefer a
different approach? Mauro; if that plan is ok (and of course assuming that
the rest of the patches are acked by their respective maintainers) could
we get a dedicated feature branch just in case the following series ends
up being ready in time too?

lib (with an ack already)
  lib/test_printf.c: Use helper function to unwind array of
    software_nodes

base
  software_node: Fix refcounts in software_node_get_next_child()
  property: Return true in fwnode_device_is_available for NULL ops
  property: Call fwnode_graph_get_endpoint_by_id() for fwnode->secondary
  software_node: Enforce parent before child ordering of nodes arrays
  software_node: unregister software_nodes in reverse order
  include: fwnode.h: Define format macros for ports and endpoints

acpi
  acpi: Add acpi_dev_get_next_match_dev() and helper macro

media
  media: v4l2-core: v4l2-async: Check sd->fwnode->secondary in
    match_fwnode()
  ipu3-cio2: Add T: entry to MAINTAINERS
  ipu3-cio2: Rename ipu3-cio2.c
  ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  include: media: v4l2-fwnode: Include v4l2_fwnode_bus_type

Series-level changelog:
	- Added patch 13/14 to define an enum in media/v4l2-fwnode.h
	- Added patch 06/14 to define name formats for fwnode graph
	  ports and endpoints

More details of changes on each patch.

Merry Christmas everyone!

Dan

Daniel Scally (13):
  software_node: Fix refcounts in software_node_get_next_child()
  property: Return true in fwnode_device_is_available for NULL ops
  property: Call fwnode_graph_get_endpoint_by_id() for fwnode->secondary
  software_node: Enforce parent before child ordering of nodes arrays
  software_node: unregister software_nodes in reverse order
  include: fwnode.h: Define format macros for ports and endpoints
  lib/test_printf.c: Use helper function to unwind array of
    software_nodes
  ipu3-cio2: Add T: entry to MAINTAINERS
  ipu3-cio2: Rename ipu3-cio2.c
  media: v4l2-core: v4l2-async: Check sd->fwnode->secondary in
    match_fwnode()
  acpi: Add acpi_dev_get_next_match_dev() and helper macro
  include: media: v4l2-fwnode: Include v4l2_fwnode_bus_type
  ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

Heikki Krogerus (1):
  software_node: Add support for fwnode_graph*() family of functions

 MAINTAINERS                                   |   2 +
 drivers/acpi/utils.c                          |  30 +-
 drivers/base/property.c                       |  15 +-
 drivers/base/swnode.c                         | 174 +++++++++--
 drivers/media/pci/intel/ipu3/Kconfig          |  18 ++
 drivers/media/pci/intel/ipu3/Makefile         |   3 +
 drivers/media/pci/intel/ipu3/cio2-bridge.c    | 272 ++++++++++++++++++
 drivers/media/pci/intel/ipu3/cio2-bridge.h    | 122 ++++++++
 .../ipu3/{ipu3-cio2.c => ipu3-cio2-main.c}    |  34 +++
 drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   6 +
 drivers/media/v4l2-core/v4l2-async.c          |   8 +
 drivers/media/v4l2-core/v4l2-fwnode.c         |  11 -
 include/acpi/acpi_bus.h                       |   7 +
 include/linux/fwnode.h                        |  13 +
 include/media/v4l2-fwnode.h                   |  22 ++
 lib/test_printf.c                             |   4 +-
 16 files changed, 704 insertions(+), 37 deletions(-)
 create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
 create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
 rename drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} (98%)

-- 
2.25.1


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

* [PATCH v3 01/14] software_node: Fix refcounts in software_node_get_next_child()
  2020-12-24  1:08 [PATCH v3 00/14] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
@ 2020-12-24  1:08 ` Daniel Scally
  2020-12-24  1:08 ` [PATCH v3 02/14] property: Return true in fwnode_device_is_available for NULL ops Daniel Scally
                   ` (12 subsequent siblings)
  13 siblings, 0 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-24  1:08 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-media, devel
  Cc: rjw, lenb, gregkh, yong.zhi, sakari.ailus, bingbu.cao,
	tian.shu.qiu, mchehab, robert.moore, erik.kaneda, pmladek,
	rostedt, sergey.senozhatsky, andriy.shevchenko, linux,
	laurent.pinchart+renesas, jacopo+renesas, kieran.bingham+renesas,
	hverkuil-cisco, m.felsch, niklas.soderlund+renesas, slongerbeam,
	heikki.krogerus, linus.walleij, Laurent Pinchart

The software_node_get_next_child() function currently does not hold
references to the child software_node that it finds or put the ref that
is held against the old child - fix that.

Fixes: 59abd83672f7 ("drivers: base: Introducing software nodes to the firmware node framework")
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3
	- None

 drivers/base/swnode.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 010828fc785b..615a0c93e116 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -443,14 +443,18 @@ software_node_get_next_child(const struct fwnode_handle *fwnode,
 	struct swnode *c = to_swnode(child);
 
 	if (!p || list_empty(&p->children) ||
-	    (c && list_is_last(&c->entry, &p->children)))
+	    (c && list_is_last(&c->entry, &p->children))) {
+		fwnode_handle_put(child);
 		return NULL;
+	}
 
 	if (c)
 		c = list_next_entry(c, entry);
 	else
 		c = list_first_entry(&p->children, struct swnode, entry);
-	return &c->fwnode;
+
+	fwnode_handle_put(child);
+	return fwnode_handle_get(&c->fwnode);
 }
 
 static struct fwnode_handle *
-- 
2.25.1


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

* [PATCH v3 02/14] property: Return true in fwnode_device_is_available for NULL ops
  2020-12-24  1:08 [PATCH v3 00/14] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
  2020-12-24  1:08 ` [PATCH v3 01/14] software_node: Fix refcounts in software_node_get_next_child() Daniel Scally
@ 2020-12-24  1:08 ` Daniel Scally
  2020-12-24  1:08 ` [PATCH v3 03/14] property: Call fwnode_graph_get_endpoint_by_id() for fwnode->secondary Daniel Scally
                   ` (11 subsequent siblings)
  13 siblings, 0 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-24  1:08 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-media, devel
  Cc: rjw, lenb, gregkh, yong.zhi, sakari.ailus, bingbu.cao,
	tian.shu.qiu, mchehab, robert.moore, erik.kaneda, pmladek,
	rostedt, sergey.senozhatsky, andriy.shevchenko, linux,
	laurent.pinchart+renesas, jacopo+renesas, kieran.bingham+renesas,
	hverkuil-cisco, m.felsch, niklas.soderlund+renesas, slongerbeam,
	heikki.krogerus, linus.walleij, Laurent Pinchart

Some types of fwnode_handle do not implement the device_is_available()
check, such as those created by software_nodes. There isn't really a
meaningful way to check for the availability of a device that doesn't
actually exist, so if the check isn't implemented just assume that the
"device" is present.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3
	- None

 drivers/base/property.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 4c43d30145c6..bc9c634df6df 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -785,9 +785,15 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put);
 /**
  * fwnode_device_is_available - check if a device is available for use
  * @fwnode: Pointer to the fwnode of the device.
+ *
+ * For fwnode node types that don't implement the .device_is_available()
+ * operation, this function returns true.
  */
 bool fwnode_device_is_available(const struct fwnode_handle *fwnode)
 {
+	if (!fwnode_has_op(fwnode, device_is_available))
+		return true;
+
 	return fwnode_call_bool_op(fwnode, device_is_available);
 }
 EXPORT_SYMBOL_GPL(fwnode_device_is_available);
-- 
2.25.1


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

* [PATCH v3 03/14] property: Call fwnode_graph_get_endpoint_by_id() for fwnode->secondary
  2020-12-24  1:08 [PATCH v3 00/14] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
  2020-12-24  1:08 ` [PATCH v3 01/14] software_node: Fix refcounts in software_node_get_next_child() Daniel Scally
  2020-12-24  1:08 ` [PATCH v3 02/14] property: Return true in fwnode_device_is_available for NULL ops Daniel Scally
@ 2020-12-24  1:08 ` Daniel Scally
  2020-12-24  1:08 ` [PATCH v3 04/14] software_node: Enforce parent before child ordering of nodes arrays Daniel Scally
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-24  1:08 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-media, devel
  Cc: rjw, lenb, gregkh, yong.zhi, sakari.ailus, bingbu.cao,
	tian.shu.qiu, mchehab, robert.moore, erik.kaneda, pmladek,
	rostedt, sergey.senozhatsky, andriy.shevchenko, linux,
	laurent.pinchart+renesas, jacopo+renesas, kieran.bingham+renesas,
	hverkuil-cisco, m.felsch, niklas.soderlund+renesas, slongerbeam,
	heikki.krogerus, linus.walleij, Laurent Pinchart

This function is used to find fwnode endpoints against a device. In
some instances those endpoints are software nodes which are children of
fwnode->secondary. Add support to fwnode_graph_get_endpoint_by_id() to
find those endpoints by recursively calling itself passing the ptr to
fwnode->secondary in the event no endpoint is found for the primary.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3
	- None

 drivers/base/property.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index bc9c634df6df..ddba75d90af2 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1163,7 +1163,14 @@ fwnode_graph_get_endpoint_by_id(const struct fwnode_handle *fwnode,
 		best_ep_id = fwnode_ep.id;
 	}
 
-	return best_ep;
+	if (best_ep)
+		return best_ep;
+
+	if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
+		return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port,
+						       endpoint, flags);
+
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(fwnode_graph_get_endpoint_by_id);
 
-- 
2.25.1


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

* [PATCH v3 04/14] software_node: Enforce parent before child ordering of nodes arrays
  2020-12-24  1:08 [PATCH v3 00/14] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
                   ` (2 preceding siblings ...)
  2020-12-24  1:08 ` [PATCH v3 03/14] property: Call fwnode_graph_get_endpoint_by_id() for fwnode->secondary Daniel Scally
@ 2020-12-24  1:08 ` Daniel Scally
  2020-12-24  1:08 ` [PATCH v3 05/14] software_node: unregister software_nodes in reverse order Daniel Scally
                   ` (9 subsequent siblings)
  13 siblings, 0 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-24  1:08 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-media, devel
  Cc: rjw, lenb, gregkh, yong.zhi, sakari.ailus, bingbu.cao,
	tian.shu.qiu, mchehab, robert.moore, erik.kaneda, pmladek,
	rostedt, sergey.senozhatsky, andriy.shevchenko, linux,
	laurent.pinchart+renesas, jacopo+renesas, kieran.bingham+renesas,
	hverkuil-cisco, m.felsch, niklas.soderlund+renesas, slongerbeam,
	heikki.krogerus, linus.walleij, Laurent Pinchart

Registering software_nodes with the .parent member set to point to a
currently unregistered software_node has the potential for problems,
so enforce parent -> child ordering in arrays passed in to
software_node_register_nodes().

Software nodes that are children of another software node should be
unregistered before their parent. To allow easy unregistering of an array
of software_nodes ordered parent to child, reverse the order in which
software_node_unregister_nodes() unregisters software_nodes.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3
	- kerneldoc comment cleanup

 drivers/base/swnode.c | 42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 615a0c93e116..ade49173ff8d 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -692,7 +692,11 @@ swnode_register(const struct software_node *node, struct swnode *parent,
  * software_node_register_nodes - Register an array of software nodes
  * @nodes: Zero terminated array of software nodes to be registered
  *
- * Register multiple software nodes at once.
+ * Register multiple software nodes at once. If any node in the array
+ * has its .parent pointer set (which can only be to another software_node),
+ * then its parent **must** have been registered before it is; either outside
+ * of this function or by ordering the array such that parent comes before
+ * child.
  */
 int software_node_register_nodes(const struct software_node *nodes)
 {
@@ -700,14 +704,23 @@ int software_node_register_nodes(const struct software_node *nodes)
 	int i;
 
 	for (i = 0; nodes[i].name; i++) {
-		ret = software_node_register(&nodes[i]);
-		if (ret) {
-			software_node_unregister_nodes(nodes);
-			return ret;
+		const struct software_node *parent = nodes[i].parent;
+
+		if (parent && !software_node_to_swnode(parent)) {
+			ret = -EINVAL;
+			goto err_unregister_nodes;
 		}
+
+		ret = software_node_register(&nodes[i]);
+		if (ret)
+			goto err_unregister_nodes;
 	}
 
 	return 0;
+
+err_unregister_nodes:
+	software_node_unregister_nodes(nodes);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(software_node_register_nodes);
 
@@ -715,18 +728,23 @@ EXPORT_SYMBOL_GPL(software_node_register_nodes);
  * software_node_unregister_nodes - Unregister an array of software nodes
  * @nodes: Zero terminated array of software nodes to be unregistered
  *
- * Unregister multiple software nodes at once.
+ * Unregister multiple software nodes at once. If parent pointers are set up
+ * in any of the software nodes then the array **must** be ordered such that
+ * parents come before their children.
  *
- * NOTE: Be careful using this call if the nodes had parent pointers set up in
- * them before registering.  If so, it is wiser to remove the nodes
- * individually, in the correct order (child before parent) instead of relying
- * on the sequential order of the list of nodes in the array.
+ * NOTE: If you are uncertain whether the array is ordered such that
+ * parents will be unregistered before their children, it is wiser to
+ * remove the nodes individually, in the correct order (child before
+ * parent).
  */
 void software_node_unregister_nodes(const struct software_node *nodes)
 {
-	int i;
+	unsigned int i = 0;
+
+	while (nodes[i].name)
+		i++;
 
-	for (i = 0; nodes[i].name; i++)
+	while (i--)
 		software_node_unregister(&nodes[i]);
 }
 EXPORT_SYMBOL_GPL(software_node_unregister_nodes);
-- 
2.25.1


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

* [PATCH v3 05/14] software_node: unregister software_nodes in reverse order
  2020-12-24  1:08 [PATCH v3 00/14] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
                   ` (3 preceding siblings ...)
  2020-12-24  1:08 ` [PATCH v3 04/14] software_node: Enforce parent before child ordering of nodes arrays Daniel Scally
@ 2020-12-24  1:08 ` Daniel Scally
  2020-12-24 12:13   ` Andy Shevchenko
  2020-12-28 16:34   ` Sakari Ailus
  2020-12-24  1:08 ` [PATCH v3 06/14] include: fwnode.h: Define format macros for ports and endpoints Daniel Scally
                   ` (8 subsequent siblings)
  13 siblings, 2 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-24  1:08 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-media, devel
  Cc: rjw, lenb, gregkh, yong.zhi, sakari.ailus, bingbu.cao,
	tian.shu.qiu, mchehab, robert.moore, erik.kaneda, pmladek,
	rostedt, sergey.senozhatsky, andriy.shevchenko, linux,
	laurent.pinchart+renesas, jacopo+renesas, kieran.bingham+renesas,
	hverkuil-cisco, m.felsch, niklas.soderlund+renesas, slongerbeam,
	heikki.krogerus, linus.walleij, kernel test robot, Dan Carpenter,
	Laurent Pinchart

To maintain consistency with software_node_unregister_nodes(), reverse
the order in which the software_node_unregister_node_group() function
unregisters nodes.

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3
	- Fixed the dereference of the terminating NULL entry
	- Comment cleanup

 drivers/base/swnode.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index ade49173ff8d..2d07eb04c6c8 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -779,16 +779,22 @@ EXPORT_SYMBOL_GPL(software_node_register_node_group);
  * software_node_unregister_node_group - Unregister a group of software nodes
  * @node_group: NULL terminated array of software node pointers to be unregistered
  *
- * Unregister multiple software nodes at once.
+ * Unregister multiple software nodes at once. The array will be unwound in
+ * reverse order (i.e. last entry first) and thus if any member of the array
+ * has its .parent member set then they should appear later in the array such
+ * that they are unregistered first.
  */
 void software_node_unregister_node_group(const struct software_node **node_group)
 {
-	unsigned int i;
+	unsigned int i = 0;
 
 	if (!node_group)
 		return;
 
-	for (i = 0; node_group[i]; i++)
+	while (node_group[i])
+		i++;
+
+	while (i--)
 		software_node_unregister(node_group[i]);
 }
 EXPORT_SYMBOL_GPL(software_node_unregister_node_group);
-- 
2.25.1


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

* [PATCH v3 06/14] include: fwnode.h: Define format macros for ports and endpoints
  2020-12-24  1:08 [PATCH v3 00/14] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
                   ` (4 preceding siblings ...)
  2020-12-24  1:08 ` [PATCH v3 05/14] software_node: unregister software_nodes in reverse order Daniel Scally
@ 2020-12-24  1:08 ` Daniel Scally
  2020-12-24 12:17   ` Andy Shevchenko
  2020-12-28 16:30   ` Sakari Ailus
  2020-12-24  1:09 ` [PATCH v3 07/14] software_node: Add support for fwnode_graph*() family of functions Daniel Scally
                   ` (7 subsequent siblings)
  13 siblings, 2 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-24  1:08 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-media, devel
  Cc: rjw, lenb, gregkh, yong.zhi, sakari.ailus, bingbu.cao,
	tian.shu.qiu, mchehab, robert.moore, erik.kaneda, pmladek,
	rostedt, sergey.senozhatsky, andriy.shevchenko, linux,
	laurent.pinchart+renesas, jacopo+renesas, kieran.bingham+renesas,
	hverkuil-cisco, m.felsch, niklas.soderlund+renesas, slongerbeam,
	heikki.krogerus, linus.walleij

OF, ACPI and software_nodes all implement graphs including nodes for ports
and endpoints. These are all intended to be named with a common schema,
as "port@n" and "endpoint@n" where n is an unsigned int representing the
index of the node. To ensure commonality across the subsystems, provide a
set of macros to define the format.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3
	- Patch introduced

 include/linux/fwnode.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 9506f8ec0974..52889efceb7d 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -32,6 +32,19 @@ struct fwnode_endpoint {
 	const struct fwnode_handle *local_fwnode;
 };
 
+/*
+ * ports and endpoints defined in OF, ACPI and as software_nodes should all
+ * follow a common naming scheme; use these macros to ensure commonality across
+ * the subsystems.
+ *
+ * The *PREFIX_LEN macros refer to the length of the "port@" and "endpoint@"
+ * sections of the naming scheme.
+ */
+#define FWNODE_GRAPH_PORT_NAME_FORMAT		"port@%u"
+#define FWNODE_GRAPH_PORT_NAME_PREFIX_LEN	5
+#define FWNODE_GRAPH_ENDPOINT_NAME_FORMAT	"endpoint@%u"
+#define FWNODE_GRAPH_ENDPOINT_PREFIX_LEN	9
+
 #define NR_FWNODE_REFERENCE_ARGS	8
 
 /**
-- 
2.25.1


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

* [PATCH v3 07/14] software_node: Add support for fwnode_graph*() family of functions
  2020-12-24  1:08 [PATCH v3 00/14] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
                   ` (5 preceding siblings ...)
  2020-12-24  1:08 ` [PATCH v3 06/14] include: fwnode.h: Define format macros for ports and endpoints Daniel Scally
@ 2020-12-24  1:09 ` Daniel Scally
  2020-12-24 12:24   ` Andy Shevchenko
  2020-12-24 12:53   ` Laurent Pinchart
  2020-12-24  1:09 ` [PATCH v3 08/14] lib/test_printf.c: Use helper function to unwind array of software_nodes Daniel Scally
                   ` (6 subsequent siblings)
  13 siblings, 2 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-24  1:09 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-media, devel
  Cc: rjw, lenb, gregkh, yong.zhi, sakari.ailus, bingbu.cao,
	tian.shu.qiu, mchehab, robert.moore, erik.kaneda, pmladek,
	rostedt, sergey.senozhatsky, andriy.shevchenko, linux,
	laurent.pinchart+renesas, jacopo+renesas, kieran.bingham+renesas,
	hverkuil-cisco, m.felsch, niklas.soderlund+renesas, slongerbeam,
	heikki.krogerus, linus.walleij

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

This implements the remaining .graph_* callbacks in the
fwnode operations structure for the software nodes. That makes
the fwnode_graph*() functions available in the drivers also
when software nodes are used.

The implementation tries to mimic the "OF graph" as much as
possible, but there is no support for the "reg" device
property. The ports will need to have the index in their
name which starts with "port@" (for example "port@0", "port@1",
...) and endpoints will use the index of the software node
that is given to them during creation. The port nodes can
also be grouped under a specially named "ports" subnode,
just like in DT, if necessary.

The remote-endpoints are reference properties under the
endpoint nodes that are named "remote-endpoint".

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Co-developed-by: Daniel Scally <djrscally@gmail.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3
	- Changed software_node_get_next_endpoint() to drop the variable
	named "old"
	- Used the macros defined in 06/14 instead of magic numbers
	- Added some comments to explain behaviour a little where it's unclear

 drivers/base/swnode.c | 112 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 111 insertions(+), 1 deletion(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 2d07eb04c6c8..ff690029060d 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -540,6 +540,112 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 	return 0;
 }
 
+static struct fwnode_handle *
+swnode_graph_find_next_port(const struct fwnode_handle *parent,
+			    struct fwnode_handle *port)
+{
+	struct fwnode_handle *old = port;
+
+	while ((port = software_node_get_next_child(parent, old))) {
+		/*
+		 * ports have naming style "port@n", so we search for children
+		 * that follow that convention (but without assuming anything
+		 * about the index number)
+		 */
+		if (!strncmp(to_swnode(port)->node->name, "port@",
+			     FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
+			return port;
+		old = port;
+	}
+
+	return NULL;
+}
+
+static struct fwnode_handle *
+software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
+				      struct fwnode_handle *endpoint)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	struct fwnode_handle *parent;
+	struct fwnode_handle *port;
+
+	if (!swnode)
+		return NULL;
+
+	if (endpoint) {
+		port = software_node_get_parent(endpoint);
+		parent = software_node_get_parent(port);
+	} else {
+		parent = software_node_get_named_child_node(fwnode, "ports");
+		if (!parent)
+			parent = software_node_get(&swnode->fwnode);
+
+		port = swnode_graph_find_next_port(parent, NULL);
+	}
+
+	for (; port; port = swnode_graph_find_next_port(parent, port)) {
+		endpoint = software_node_get_next_child(port, endpoint);
+		if (endpoint) {
+			fwnode_handle_put(port);
+			break;
+		}
+	}
+
+	fwnode_handle_put(parent);
+
+	return endpoint;
+}
+
+static struct fwnode_handle *
+software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	const struct software_node_ref_args *ref;
+	const struct property_entry *prop;
+
+	if (!swnode)
+		return NULL;
+
+	prop = property_entry_get(swnode->node->properties, "remote-endpoint");
+	if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
+		return NULL;
+
+	ref = prop->pointer;
+
+	return software_node_get(software_node_fwnode(ref[0].node));
+}
+
+static struct fwnode_handle *
+software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+
+	swnode = swnode->parent;
+	if (swnode && !strcmp(swnode->node->name, "ports"))
+		swnode = swnode->parent;
+
+	return swnode ? software_node_get(&swnode->fwnode) : NULL;
+}
+
+static int
+software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
+				   struct fwnode_endpoint *endpoint)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	int ret;
+
+	/* Ports have naming style "port@n", we need to select the n */
+	ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
+			10, &endpoint->port);
+	if (ret)
+		return ret;
+
+	endpoint->id = swnode->id;
+	endpoint->local_fwnode = fwnode;
+
+	return 0;
+}
+
 static const struct fwnode_operations software_node_ops = {
 	.get = software_node_get,
 	.put = software_node_put,
@@ -551,7 +657,11 @@ static const struct fwnode_operations software_node_ops = {
 	.get_parent = software_node_get_parent,
 	.get_next_child_node = software_node_get_next_child,
 	.get_named_child_node = software_node_get_named_child_node,
-	.get_reference_args = software_node_get_reference_args
+	.get_reference_args = software_node_get_reference_args,
+	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
+	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
+	.graph_get_port_parent = software_node_graph_get_port_parent,
+	.graph_parse_endpoint = software_node_graph_parse_endpoint,
 };
 
 /* -------------------------------------------------------------------------- */
-- 
2.25.1


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

* [PATCH v3 08/14] lib/test_printf.c: Use helper function to unwind array of software_nodes
  2020-12-24  1:08 [PATCH v3 00/14] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
                   ` (6 preceding siblings ...)
  2020-12-24  1:09 ` [PATCH v3 07/14] software_node: Add support for fwnode_graph*() family of functions Daniel Scally
@ 2020-12-24  1:09 ` Daniel Scally
  2020-12-24  1:09 ` [PATCH v3 09/14] ipu3-cio2: Add T: entry to MAINTAINERS Daniel Scally
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-24  1:09 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-media, devel
  Cc: rjw, lenb, gregkh, yong.zhi, sakari.ailus, bingbu.cao,
	tian.shu.qiu, mchehab, robert.moore, erik.kaneda, pmladek,
	rostedt, sergey.senozhatsky, andriy.shevchenko, linux,
	laurent.pinchart+renesas, jacopo+renesas, kieran.bingham+renesas,
	hverkuil-cisco, m.felsch, niklas.soderlund+renesas, slongerbeam,
	heikki.krogerus, linus.walleij, Laurent Pinchart

Use the software_node_unregister_nodes() helper function to unwind this
array in a cleaner way.

Acked-by: Petr Mladek <pmladek@suse.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3
	- None

 lib/test_printf.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 7ac87f18a10f..7d60f24240a4 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -644,9 +644,7 @@ static void __init fwnode_pointer(void)
 	test(second_name, "%pfwP", software_node_fwnode(&softnodes[1]));
 	test(third_name, "%pfwP", software_node_fwnode(&softnodes[2]));
 
-	software_node_unregister(&softnodes[2]);
-	software_node_unregister(&softnodes[1]);
-	software_node_unregister(&softnodes[0]);
+	software_node_unregister_nodes(softnodes);
 }
 
 static void __init
-- 
2.25.1


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

* [PATCH v3 09/14] ipu3-cio2: Add T: entry to MAINTAINERS
  2020-12-24  1:08 [PATCH v3 00/14] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
                   ` (7 preceding siblings ...)
  2020-12-24  1:09 ` [PATCH v3 08/14] lib/test_printf.c: Use helper function to unwind array of software_nodes Daniel Scally
@ 2020-12-24  1:09 ` Daniel Scally
  2020-12-24  1:09 ` [PATCH v3 10/14] ipu3-cio2: Rename ipu3-cio2.c Daniel Scally
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-24  1:09 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-media, devel
  Cc: rjw, lenb, gregkh, yong.zhi, sakari.ailus, bingbu.cao,
	tian.shu.qiu, mchehab, robert.moore, erik.kaneda, pmladek,
	rostedt, sergey.senozhatsky, andriy.shevchenko, linux,
	laurent.pinchart+renesas, jacopo+renesas, kieran.bingham+renesas,
	hverkuil-cisco, m.felsch, niklas.soderlund+renesas, slongerbeam,
	heikki.krogerus, linus.walleij, Laurent Pinchart

Development for the ipu3-cio2 driver is taking place in media_tree, but
there's no T: entry in MAINTAINERS to denote that - rectify that oversight

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3
	- None

 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 80881fb36404..16b544624577 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8946,6 +8946,7 @@ M:	Bingbu Cao <bingbu.cao@intel.com>
 R:	Tianshu Qiu <tian.shu.qiu@intel.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
+T:	git git://linuxtv.org/media_tree.git
 F:	Documentation/userspace-api/media/v4l/pixfmt-srggb10-ipu3.rst
 F:	drivers/media/pci/intel/ipu3/
 
-- 
2.25.1


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

* [PATCH v3 10/14] ipu3-cio2: Rename ipu3-cio2.c
  2020-12-24  1:08 [PATCH v3 00/14] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
                   ` (8 preceding siblings ...)
  2020-12-24  1:09 ` [PATCH v3 09/14] ipu3-cio2: Add T: entry to MAINTAINERS Daniel Scally
@ 2020-12-24  1:09 ` Daniel Scally
  2020-12-24  1:09 ` [PATCH v3 11/14] media: v4l2-core: v4l2-async: Check sd->fwnode->secondary in match_fwnode() Daniel Scally
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-24  1:09 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-media, devel
  Cc: rjw, lenb, gregkh, yong.zhi, sakari.ailus, bingbu.cao,
	tian.shu.qiu, mchehab, robert.moore, erik.kaneda, pmladek,
	rostedt, sergey.senozhatsky, andriy.shevchenko, linux,
	laurent.pinchart+renesas, jacopo+renesas, kieran.bingham+renesas,
	hverkuil-cisco, m.felsch, niklas.soderlund+renesas, slongerbeam,
	heikki.krogerus, linus.walleij, Laurent Pinchart

ipu3-cio2 driver needs extending with multiple files; rename the main
source file and specify the renamed file in Makefile to accommodate that.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3
	- None

 drivers/media/pci/intel/ipu3/Makefile                          | 2 ++
 drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} | 0
 2 files changed, 2 insertions(+)
 rename drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} (100%)

diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile
index 98ddd5beafe0..429d516452e4 100644
--- a/drivers/media/pci/intel/ipu3/Makefile
+++ b/drivers/media/pci/intel/ipu3/Makefile
@@ -1,2 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
+
+ipu3-cio2-y += ipu3-cio2-main.o
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
similarity index 100%
rename from drivers/media/pci/intel/ipu3/ipu3-cio2.c
rename to drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
-- 
2.25.1


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

* [PATCH v3 11/14] media: v4l2-core: v4l2-async: Check sd->fwnode->secondary in match_fwnode()
  2020-12-24  1:08 [PATCH v3 00/14] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
                   ` (9 preceding siblings ...)
  2020-12-24  1:09 ` [PATCH v3 10/14] ipu3-cio2: Rename ipu3-cio2.c Daniel Scally
@ 2020-12-24  1:09 ` Daniel Scally
  2020-12-24  1:09 ` [PATCH v3 12/14] acpi: Add acpi_dev_get_next_match_dev() and helper macro Daniel Scally
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-24  1:09 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-media, devel
  Cc: rjw, lenb, gregkh, yong.zhi, sakari.ailus, bingbu.cao,
	tian.shu.qiu, mchehab, robert.moore, erik.kaneda, pmladek,
	rostedt, sergey.senozhatsky, andriy.shevchenko, linux,
	laurent.pinchart+renesas, jacopo+renesas, kieran.bingham+renesas,
	hverkuil-cisco, m.felsch, niklas.soderlund+renesas, slongerbeam,
	heikki.krogerus, linus.walleij, Laurent Pinchart

Where the fwnode graph is comprised of software_nodes, these will be
assigned as the secondary to dev->fwnode. Check the v4l2_subdev's fwnode
for a secondary and attempt to match against it during match_fwnode() to
accommodate that possibility.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3
	- None

 drivers/media/v4l2-core/v4l2-async.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index e3ab003a6c85..9dd896d085ec 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -87,6 +87,14 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
 	if (sd->fwnode == asd->match.fwnode)
 		return true;
 
+	/*
+	 * Check the same situation for any possible secondary assigned to the
+	 * subdev's fwnode
+	 */
+	if (!IS_ERR_OR_NULL(sd->fwnode->secondary) &&
+	    sd->fwnode->secondary == asd->match.fwnode)
+		return true;
+
 	/*
 	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
 	 * endpoint or a device. If they're of the same type, there's no match.
-- 
2.25.1


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

* [PATCH v3 12/14] acpi: Add acpi_dev_get_next_match_dev() and helper macro
  2020-12-24  1:08 [PATCH v3 00/14] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
                   ` (10 preceding siblings ...)
  2020-12-24  1:09 ` [PATCH v3 11/14] media: v4l2-core: v4l2-async: Check sd->fwnode->secondary in match_fwnode() Daniel Scally
@ 2020-12-24  1:09 ` Daniel Scally
  2020-12-24  1:09 ` [PATCH v3 13/14] include: media: v4l2-fwnode: Include v4l2_fwnode_bus_type Daniel Scally
  2020-12-24  1:09 ` [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver Daniel Scally
  13 siblings, 0 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-24  1:09 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-media, devel
  Cc: rjw, lenb, gregkh, yong.zhi, sakari.ailus, bingbu.cao,
	tian.shu.qiu, mchehab, robert.moore, erik.kaneda, pmladek,
	rostedt, sergey.senozhatsky, andriy.shevchenko, linux,
	laurent.pinchart+renesas, jacopo+renesas, kieran.bingham+renesas,
	hverkuil-cisco, m.felsch, niklas.soderlund+renesas, slongerbeam,
	heikki.krogerus, linus.walleij

To ensure we handle situations in which multiple sensors of the same
model (and therefore _HID) are present in a system, we need to be able
to iterate over devices matching a known _HID but unknown _UID and _HRV
 - add acpi_dev_get_next_match_dev() to accommodate that possibility and
change acpi_dev_get_first_match_dev() to simply call the new function
with a NULL starting point. Add an iterator macro for convenience.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3
	- prefixed referenced to the arguments with "@" in comment

 drivers/acpi/utils.c    | 30 ++++++++++++++++++++++++++----
 include/acpi/acpi_bus.h |  7 +++++++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index d5411a166685..ddca1550cce6 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -843,12 +843,13 @@ bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
 EXPORT_SYMBOL(acpi_dev_present);
 
 /**
- * acpi_dev_get_first_match_dev - Return the first match of ACPI device
+ * acpi_dev_get_next_match_dev - Return the next match of ACPI device
+ * @adev: Pointer to the previous acpi_device matching this @hid, @uid and @hrv
  * @hid: Hardware ID of the device.
  * @uid: Unique ID of the device, pass NULL to not check _UID
  * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
  *
- * Return the first match of ACPI device if a matching device was present
+ * Return the next match of ACPI device if another matching device was present
  * at the moment of invocation, or NULL otherwise.
  *
  * The caller is responsible to call put_device() on the returned device.
@@ -856,8 +857,9 @@ EXPORT_SYMBOL(acpi_dev_present);
  * See additional information in acpi_dev_present() as well.
  */
 struct acpi_device *
-acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
+acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv)
 {
+	struct device *start = adev ? &adev->dev : NULL;
 	struct acpi_dev_match_info match = {};
 	struct device *dev;
 
@@ -865,9 +867,29 @@ acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
 	match.uid = uid;
 	match.hrv = hrv;
 
-	dev = bus_find_device(&acpi_bus_type, NULL, &match, acpi_dev_match_cb);
+	dev = bus_find_device(&acpi_bus_type, start, &match, acpi_dev_match_cb);
 	return dev ? to_acpi_device(dev) : NULL;
 }
+EXPORT_SYMBOL(acpi_dev_get_next_match_dev);
+
+/**
+ * acpi_dev_get_first_match_dev - Return the first match of ACPI device
+ * @hid: Hardware ID of the device.
+ * @uid: Unique ID of the device, pass NULL to not check _UID
+ * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
+ *
+ * Return the first match of ACPI device if a matching device was present
+ * at the moment of invocation, or NULL otherwise.
+ *
+ * The caller is responsible to call put_device() on the returned device.
+ *
+ * See additional information in acpi_dev_present() as well.
+ */
+struct acpi_device *
+acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
+{
+	return acpi_dev_get_next_match_dev(NULL, hid, uid, hrv);
+}
 EXPORT_SYMBOL(acpi_dev_get_first_match_dev);
 
 /*
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index a3abcc4b7d9f..0a028ba967d3 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -688,9 +688,16 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
 
 bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2);
 
+struct acpi_device *
+acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
 struct acpi_device *
 acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv);
 
+#define for_each_acpi_dev_match(adev, hid, uid, hrv)			\
+	for (adev = acpi_dev_get_first_match_dev(hid, uid, hrv);	\
+	     adev;							\
+	     adev = acpi_dev_get_next_match_dev(adev, hid, uid, hrv))
+
 static inline void acpi_dev_put(struct acpi_device *adev)
 {
 	put_device(&adev->dev);
-- 
2.25.1


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

* [PATCH v3 13/14] include: media: v4l2-fwnode: Include v4l2_fwnode_bus_type
  2020-12-24  1:08 [PATCH v3 00/14] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
                   ` (11 preceding siblings ...)
  2020-12-24  1:09 ` [PATCH v3 12/14] acpi: Add acpi_dev_get_next_match_dev() and helper macro Daniel Scally
@ 2020-12-24  1:09 ` Daniel Scally
  2020-12-24 12:32   ` Andy Shevchenko
  2020-12-24 12:58   ` Laurent Pinchart
  2020-12-24  1:09 ` [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver Daniel Scally
  13 siblings, 2 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-24  1:09 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-media, devel
  Cc: rjw, lenb, gregkh, yong.zhi, sakari.ailus, bingbu.cao,
	tian.shu.qiu, mchehab, robert.moore, erik.kaneda, pmladek,
	rostedt, sergey.senozhatsky, andriy.shevchenko, linux,
	laurent.pinchart+renesas, jacopo+renesas, kieran.bingham+renesas,
	hverkuil-cisco, m.felsch, niklas.soderlund+renesas, slongerbeam,
	heikki.krogerus, linus.walleij

V4L2 fwnode bus types are enumerated in v4l2-fwnode.c, meaning they aren't
available to the rest of the kernel. Move the enum to the corresponding
header so that I can use the label to refer to those values.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3
	- Patch introduced

 drivers/media/v4l2-core/v4l2-fwnode.c | 11 -----------
 include/media/v4l2-fwnode.h           | 22 ++++++++++++++++++++++
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 5353e37eb950..c1c2b3060532 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -28,17 +28,6 @@
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
-enum v4l2_fwnode_bus_type {
-	V4L2_FWNODE_BUS_TYPE_GUESS = 0,
-	V4L2_FWNODE_BUS_TYPE_CSI2_CPHY,
-	V4L2_FWNODE_BUS_TYPE_CSI1,
-	V4L2_FWNODE_BUS_TYPE_CCP2,
-	V4L2_FWNODE_BUS_TYPE_CSI2_DPHY,
-	V4L2_FWNODE_BUS_TYPE_PARALLEL,
-	V4L2_FWNODE_BUS_TYPE_BT656,
-	NR_OF_V4L2_FWNODE_BUS_TYPE,
-};
-
 static const struct v4l2_fwnode_bus_conv {
 	enum v4l2_fwnode_bus_type fwnode_bus_type;
 	enum v4l2_mbus_type mbus_type;
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index 4365430eea6f..d306a28bda96 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -213,6 +213,28 @@ struct v4l2_fwnode_connector {
 	} connector;
 };
 
+/**
+ * enum v4l2_fwnode_bus_type - Video bus types defined by firmware properties
+ * @V4L2_FWNODE_BUS_TYPE_GUESS: Default value if no bus-type fwnode property
+ * @V4L2_FWNODE_BUS_TYPE_CSI2_CPHY: MIPI CSI-2 bus, C-PHY physical layer
+ * @V4L2_FWNODE_BUS_TYPE_CSI1: MIPI CSI-1 bus
+ * @V4L2_FWNODE_BUS_TYPE_CCP2: SMIA Compact Camera Port 2 bus
+ * @V4L2_FWNODE_BUS_TYPE_CSI2_DPHY: MIPI CSI-2 bus, D-PHY physical layer
+ * @V4L2_FWNODE_BUS_TYPE_PARALLEL: Camera Parallel Interface bus
+ * @V4L2_FWNODE_BUS_TYPE_BT656: BT656 video format bus-type
+ * @NR_OF_V4L2_FWNODE_BUS_TYPE: Number of bus-types
+ */
+enum v4l2_fwnode_bus_type {
+	V4L2_FWNODE_BUS_TYPE_GUESS = 0,
+	V4L2_FWNODE_BUS_TYPE_CSI2_CPHY,
+	V4L2_FWNODE_BUS_TYPE_CSI1,
+	V4L2_FWNODE_BUS_TYPE_CCP2,
+	V4L2_FWNODE_BUS_TYPE_CSI2_DPHY,
+	V4L2_FWNODE_BUS_TYPE_PARALLEL,
+	V4L2_FWNODE_BUS_TYPE_BT656,
+	NR_OF_V4L2_FWNODE_BUS_TYPE,
+};
+
 /**
  * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
  * @fwnode: pointer to the endpoint's fwnode handle
-- 
2.25.1


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

* [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2020-12-24  1:08 [PATCH v3 00/14] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
                   ` (12 preceding siblings ...)
  2020-12-24  1:09 ` [PATCH v3 13/14] include: media: v4l2-fwnode: Include v4l2_fwnode_bus_type Daniel Scally
@ 2020-12-24  1:09 ` Daniel Scally
  2020-12-24 12:54   ` Andy Shevchenko
  13 siblings, 1 reply; 58+ messages in thread
From: Daniel Scally @ 2020-12-24  1:09 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-media, devel
  Cc: rjw, lenb, gregkh, yong.zhi, sakari.ailus, bingbu.cao,
	tian.shu.qiu, mchehab, robert.moore, erik.kaneda, pmladek,
	rostedt, sergey.senozhatsky, andriy.shevchenko, linux,
	laurent.pinchart+renesas, jacopo+renesas, kieran.bingham+renesas,
	hverkuil-cisco, m.felsch, niklas.soderlund+renesas, slongerbeam,
	heikki.krogerus, linus.walleij, Jordan Hand, Laurent Pinchart

Currently on platforms designed for Windows, connections between CIO2 and
sensors are not properly defined in DSDT. This patch extends the ipu3-cio2
driver to compensate by building software_node connections, parsing the
connection properties from the sensor's SSDB buffer.

Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3
	- Used Laurent's suggestion to simplify initing the property names
	- Wrapped some lines
	- Fixed return and error handling for cio2_bridge_read_acpi_buffer()
	- Returned an error if more sensors than available ports are detected
	- Used defines for port/endpoint name formats and the bus-type property
	- Some bits of cleanup

 MAINTAINERS                                   |   1 +
 drivers/media/pci/intel/ipu3/Kconfig          |  18 ++
 drivers/media/pci/intel/ipu3/Makefile         |   1 +
 drivers/media/pci/intel/ipu3/cio2-bridge.c    | 272 ++++++++++++++++++
 drivers/media/pci/intel/ipu3/cio2-bridge.h    | 122 ++++++++
 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  34 +++
 drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   6 +
 7 files changed, 454 insertions(+)
 create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
 create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 16b544624577..e7784b4bc8ea 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8943,6 +8943,7 @@ INTEL IPU3 CSI-2 CIO2 DRIVER
 M:	Yong Zhi <yong.zhi@intel.com>
 M:	Sakari Ailus <sakari.ailus@linux.intel.com>
 M:	Bingbu Cao <bingbu.cao@intel.com>
+M:	Dan Scally <djrscally@gmail.com>
 R:	Tianshu Qiu <tian.shu.qiu@intel.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig
index 82d7f17e6a02..dcf5c4b74673 100644
--- a/drivers/media/pci/intel/ipu3/Kconfig
+++ b/drivers/media/pci/intel/ipu3/Kconfig
@@ -16,3 +16,21 @@ config VIDEO_IPU3_CIO2
 	  Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
 	  connected camera.
 	  The module will be called ipu3-cio2.
+
+config CIO2_BRIDGE
+	bool "IPU3 CIO2 Sensors Bridge"
+	depends on VIDEO_IPU3_CIO2
+	help
+	  This extension provides an API for the ipu3-cio2 driver to create
+	  connections to cameras that are hidden in SSDB buffer in ACPI. It
+	  can be used to enable support for cameras in detachable / hybrid
+	  devices that ship with Windows.
+
+	  Say Y here if your device is a detachable / hybrid laptop that comes
+	  with Windows installed by the OEM, for example:
+
+		- Microsoft Surface models (except Surface Pro 3)
+		- The Lenovo Miix line (for example the 510, 520, 710 and 720)
+		- Dell 7285
+
+	  If in doubt, say N here.
diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile
index 429d516452e4..933777e6ea8a 100644
--- a/drivers/media/pci/intel/ipu3/Makefile
+++ b/drivers/media/pci/intel/ipu3/Makefile
@@ -2,3 +2,4 @@
 obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
 
 ipu3-cio2-y += ipu3-cio2-main.o
+ipu3-cio2-$(CONFIG_CIO2_BRIDGE) += cio2-bridge.o
diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
new file mode 100644
index 000000000000..3f4ae172fd25
--- /dev/null
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
@@ -0,0 +1,272 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Author: Dan Scally <djrscally@gmail.com> */
+
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/pci.h>
+#include <linux/property.h>
+#include <media/v4l2-fwnode.h>
+
+#include "cio2-bridge.h"
+
+/*
+ * Extend this array with ACPI Hardware ID's of devices known to be working
+ * plus the number of link-frequencies expected by their drivers, along with
+ * the frequency values in hertz. This is somewhat opportunistic way of adding
+ * support for this for now in the hopes of a better source for the information
+ * (possibly some encoded value in the SSDB buffer that we're unaware of)
+ * becoming apparent in the future.
+ *
+ * Do not add an entry for a sensor that is not actually supported.
+ */
+static const struct cio2_sensor_config cio2_supported_sensors[] = {
+	CIO2_SENSOR_CONFIG("INT33BE", 0),
+	CIO2_SENSOR_CONFIG("OVTI2680", 0),
+};
+
+static const struct cio2_property_names prop_names = {
+	.clock_frequency = "clock-frequency",
+	.rotation = "rotation",
+	.bus_type = "bus-type",
+	.data_lanes = "data-lanes",
+	.remote_endpoint = "remote-endpoint",
+	.link_frequencies = "link-frequencies",
+};
+
+static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
+					void *data, u32 size)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	acpi_status status;
+	int ret = 0;
+
+	status = acpi_evaluate_object(adev->handle, id, NULL, &buffer);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	obj = buffer.pointer;
+	if (!obj) {
+		dev_err(&adev->dev, "Couldn't locate ACPI buffer\n");
+		return -ENODEV;
+	}
+
+	if (obj->type != ACPI_TYPE_BUFFER) {
+		dev_err(&adev->dev, "Not an ACPI buffer\n");
+		ret = -ENODEV;
+		goto out_free_buff;
+	}
+
+	if (obj->buffer.length > size) {
+		dev_err(&adev->dev, "Given buffer is too small\n");
+		ret = -EINVAL;
+		goto out_free_buff;
+	}
+
+	memcpy(data, obj->buffer.pointer, obj->buffer.length);
+
+out_free_buff:
+	kfree(buffer.pointer);
+	return ret;
+}
+
+static void cio2_bridge_create_fwnode_properties(struct cio2_sensor *sensor,
+						 const struct cio2_sensor_config *cfg)
+{
+	unsigned int i;
+
+	sensor->prop_names = prop_names;
+
+	for (i = 0; i < 4; i++)
+		sensor->data_lanes[i] = i + 1;
+
+	sensor->local_ref[0].node = &sensor->swnodes[SWNODE_CIO2_ENDPOINT];
+	sensor->remote_ref[0].node = &sensor->swnodes[SWNODE_SENSOR_ENDPOINT];
+
+	sensor->dev_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.clock_frequency,
+						       sensor->ssdb.mclkspeed);
+	sensor->dev_properties[1] = PROPERTY_ENTRY_U8(sensor->prop_names.rotation,
+						      sensor->ssdb.degree);
+
+	sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type,
+						      V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
+	sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
+								sensor->data_lanes,
+								sensor->ssdb.lanes);
+	sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
+							    sensor->local_ref);
+
+	if (cfg->nr_link_freqs > 0)
+		sensor->ep_properties[3] = PROPERTY_ENTRY_U64_ARRAY_LEN(
+						sensor->prop_names.link_frequencies,
+						cfg->link_freqs,
+						cfg->nr_link_freqs);
+
+	sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
+								  sensor->data_lanes,
+								  sensor->ssdb.lanes);
+	sensor->cio2_properties[1] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
+							      sensor->remote_ref);
+}
+
+static void cio2_bridge_init_swnode_names(struct cio2_sensor *sensor)
+{
+	snprintf(sensor->node_names.remote_port, sizeof(sensor->node_names.remote_port),
+		 FWNODE_GRAPH_PORT_NAME_FORMAT, sensor->ssdb.link);
+	snprintf(sensor->node_names.port, sizeof(sensor->node_names.port),
+		 FWNODE_GRAPH_PORT_NAME_FORMAT, 0); /* Always port 0 */
+	snprintf(sensor->node_names.endpoint, sizeof(sensor->node_names.endpoint),
+		 FWNODE_GRAPH_ENDPOINT_NAME_FORMAT, 0); /* And endpoint 0 */
+}
+
+static void cio2_bridge_create_connection_swnodes(struct cio2_bridge *bridge,
+						  struct cio2_sensor *sensor)
+{
+	struct software_node *nodes = sensor->swnodes;
+
+	cio2_bridge_init_swnode_names(sensor);
+
+	nodes[SWNODE_SENSOR_HID] = NODE_SENSOR(sensor->name,
+					       sensor->dev_properties);
+	nodes[SWNODE_SENSOR_PORT] = NODE_PORT(sensor->node_names.port,
+					      &nodes[SWNODE_SENSOR_HID]);
+	nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
+						      &nodes[SWNODE_SENSOR_PORT],
+						      sensor->ep_properties);
+	nodes[SWNODE_CIO2_PORT] = NODE_PORT(sensor->node_names.remote_port,
+					    &bridge->cio2_hid_node);
+	nodes[SWNODE_CIO2_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
+						    &nodes[SWNODE_CIO2_PORT],
+						    sensor->cio2_properties);
+}
+
+static void cio2_bridge_unregister_sensors(struct cio2_bridge *bridge)
+{
+	struct cio2_sensor *sensor;
+	unsigned int i;
+
+	for (i = 0; i < bridge->n_sensors; i++) {
+		sensor = &bridge->sensors[i];
+		software_node_unregister_nodes(sensor->swnodes);
+		acpi_dev_put(sensor->adev);
+	}
+}
+
+static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge,
+				       struct pci_dev *cio2)
+{
+	struct fwnode_handle *fwnode;
+	struct cio2_sensor *sensor;
+	struct acpi_device *adev;
+	unsigned int i;
+	int ret = 0;
+
+	for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
+		const struct cio2_sensor_config *cfg = &cio2_supported_sensors[i];
+
+		for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
+			if (bridge->n_sensors >= CIO2_NUM_PORTS) {
+				dev_err(&cio2->dev, "Exceeded available CIO2 ports\n");
+				cio2_bridge_unregister_sensors(bridge);
+				ret = -EINVAL;
+				goto err_out;
+			}
+
+			if (!adev->status.enabled)
+				continue;
+
+			sensor = &bridge->sensors[bridge->n_sensors];
+			sensor->adev = adev;
+			strscpy(sensor->name, cfg->hid, sizeof(sensor->name));
+
+			ret = cio2_bridge_read_acpi_buffer(adev, "SSDB",
+							   &sensor->ssdb,
+							   sizeof(sensor->ssdb));
+			if (ret)
+				goto err_put_adev;
+
+			if (sensor->ssdb.lanes > 4) {
+				dev_err(&adev->dev,
+					"Number of lanes in SSDB is invalid\n");
+				ret = -EINVAL;
+				goto err_put_adev;
+			}
+
+			cio2_bridge_create_fwnode_properties(sensor, cfg);
+			cio2_bridge_create_connection_swnodes(bridge, sensor);
+
+			ret = software_node_register_nodes(sensor->swnodes);
+			if (ret)
+				goto err_put_adev;
+
+			fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_SENSOR_HID]);
+			if (!fwnode) {
+				ret = -ENODEV;
+				goto err_free_swnodes;
+			}
+
+			adev->fwnode.secondary = fwnode;
+
+			dev_info(&cio2->dev, "Found supported sensor %s\n",
+				 acpi_dev_name(adev));
+
+			bridge->n_sensors++;
+		}
+	}
+
+	return ret;
+
+err_free_swnodes:
+	software_node_unregister_nodes(sensor->swnodes);
+err_put_adev:
+	acpi_dev_put(sensor->adev);
+err_out:
+	return ret;
+}
+
+int cio2_bridge_init(struct pci_dev *cio2)
+{
+	struct device *dev = &cio2->dev;
+	struct fwnode_handle *fwnode;
+	struct cio2_bridge *bridge;
+	int ret;
+
+	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
+	if (!bridge)
+		return -ENOMEM;
+
+	strscpy(bridge->cio2_node_name, CIO2_HID, sizeof(bridge->cio2_node_name));
+	bridge->cio2_hid_node.name = bridge->cio2_node_name;
+
+	ret = software_node_register(&bridge->cio2_hid_node);
+	if (ret < 0) {
+		dev_err(dev, "Failed to register the CIO2 HID node\n");
+		goto err_free_bridge;
+	}
+
+	ret = cio2_bridge_connect_sensors(bridge, cio2);
+	if (ret || bridge->n_sensors == 0)
+		goto err_unregister_cio2;
+
+	dev_info(dev, "Connected %d cameras\n", bridge->n_sensors);
+
+	fwnode = software_node_fwnode(&bridge->cio2_hid_node);
+	if (!fwnode) {
+		dev_err(dev, "Error getting fwnode from cio2 software_node\n");
+		ret = -ENODEV;
+		goto err_unregister_sensors;
+	}
+
+	set_secondary_fwnode(dev, fwnode);
+
+	return 0;
+
+err_unregister_sensors:
+	cio2_bridge_unregister_sensors(bridge);
+err_unregister_cio2:
+	software_node_unregister(&bridge->cio2_hid_node);
+err_free_bridge:
+	kfree(bridge);
+
+	return ret;
+}
diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.h b/drivers/media/pci/intel/ipu3/cio2-bridge.h
new file mode 100644
index 000000000000..004b608f322f
--- /dev/null
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
@@ -0,0 +1,122 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Author: Dan Scally <djrscally@gmail.com> */
+#ifndef __CIO2_BRIDGE_H
+#define __CIO2_BRIDGE_H
+
+#include <linux/property.h>
+
+#define CIO2_HID				"INT343E"
+#define CIO2_NUM_PORTS				4
+#define MAX_NUM_LINK_FREQS			3
+
+#define CIO2_SENSOR_CONFIG(_HID, _NR, ...)	\
+	{					\
+		.hid = _HID,			\
+		.nr_link_freqs = _NR,		\
+		.link_freqs = { __VA_ARGS__ }	\
+	}
+
+#define NODE_SENSOR(_HID, _PROPS)		\
+	((const struct software_node) {		\
+		.name = _HID,			\
+		.properties = _PROPS,		\
+	})
+
+#define NODE_PORT(_PORT, _SENSOR_NODE)		\
+	((const struct software_node) {		\
+		_PORT,				\
+		_SENSOR_NODE,			\
+	})
+
+#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\
+	((const struct software_node) {		\
+		_EP,				\
+		_PORT,				\
+		_PROPS,				\
+	})
+
+enum cio2_sensor_swnodes {
+	SWNODE_SENSOR_HID,
+	SWNODE_SENSOR_PORT,
+	SWNODE_SENSOR_ENDPOINT,
+	SWNODE_CIO2_PORT,
+	SWNODE_CIO2_ENDPOINT,
+	SWNODE_COUNT,
+};
+
+/* Data representation as it is in ACPI SSDB buffer */
+struct cio2_sensor_ssdb {
+	u8 version;
+	u8 sku;
+	u8 guid_csi2[16];
+	u8 devfunction;
+	u8 bus;
+	u32 dphylinkenfuses;
+	u32 clockdiv;
+	u8 link;
+	u8 lanes;
+	u32 csiparams[10];
+	u32 maxlanespeed;
+	u8 sensorcalibfileidx;
+	u8 sensorcalibfileidxInMBZ[3];
+	u8 romtype;
+	u8 vcmtype;
+	u8 platforminfo;
+	u8 platformsubinfo;
+	u8 flash;
+	u8 privacyled;
+	u8 degree;
+	u8 mipilinkdefined;
+	u32 mclkspeed;
+	u8 controllogicid;
+	u8 reserved1[3];
+	u8 mclkport;
+	u8 reserved2[13];
+} __packed;
+
+struct cio2_property_names {
+	char clock_frequency[16];
+	char rotation[9];
+	char bus_type[9];
+	char data_lanes[11];
+	char remote_endpoint[16];
+	char link_frequencies[17];
+};
+
+struct cio2_node_names {
+	char port[7];
+	char endpoint[11];
+	char remote_port[7];
+};
+
+struct cio2_sensor_config {
+	const char *hid;
+	const u8 nr_link_freqs;
+	const u64 link_freqs[MAX_NUM_LINK_FREQS];
+};
+
+struct cio2_sensor {
+	char name[ACPI_ID_LEN];
+	struct acpi_device *adev;
+
+	struct software_node swnodes[6];
+	struct cio2_node_names node_names;
+
+	u32 data_lanes[4];
+	struct cio2_sensor_ssdb ssdb;
+	struct cio2_property_names prop_names;
+	struct property_entry ep_properties[5];
+	struct property_entry dev_properties[3];
+	struct property_entry cio2_properties[3];
+	struct software_node_ref_args local_ref[1];
+	struct software_node_ref_args remote_ref[1];
+};
+
+struct cio2_bridge {
+	char cio2_node_name[ACPI_ID_LEN];
+	struct software_node cio2_hid_node;
+	unsigned int n_sensors;
+	struct cio2_sensor sensors[CIO2_NUM_PORTS];
+};
+
+#endif
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
index 36e354ecf71e..68ff28abc6a3 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
@@ -1702,11 +1702,28 @@ static void cio2_queues_exit(struct cio2_device *cio2)
 		cio2_queue_exit(cio2, &cio2->queue[i]);
 }
 
+static bool cio2_check_fwnode_graph(struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *endpoint;
+
+	if (IS_ERR_OR_NULL(fwnode))
+		return false;
+
+	endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
+	if (endpoint) {
+		fwnode_handle_put(endpoint);
+		return true;
+	}
+
+	return cio2_check_fwnode_graph(fwnode->secondary);
+}
+
 /**************** PCI interface ****************/
 
 static int cio2_pci_probe(struct pci_dev *pci_dev,
 			  const struct pci_device_id *id)
 {
+	struct fwnode_handle *fwnode = dev_fwnode(&pci_dev->dev);
 	struct cio2_device *cio2;
 	int r;
 
@@ -1715,6 +1732,23 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 		return -ENOMEM;
 	cio2->pci_dev = pci_dev;
 
+	/*
+	 * On some platforms no connections to sensors are defined in firmware,
+	 * if the device has no endpoints then we can try to build those as
+	 * software_nodes parsed from SSDB.
+	 */
+	if (!cio2_check_fwnode_graph(fwnode)) {
+		if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) {
+			dev_err(&pci_dev->dev,
+				"fwnode graph has no endpoints connected\n");
+			return -EINVAL;
+		}
+
+		r = cio2_bridge_init(pci_dev);
+		if (r)
+			return r;
+	}
+
 	r = pcim_enable_device(pci_dev);
 	if (r) {
 		dev_err(&pci_dev->dev, "failed to enable device (%d)\n", r);
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
index ccf0b85ae36f..520a27c9cdad 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
@@ -437,4 +437,10 @@ static inline struct cio2_queue *vb2q_to_cio2_queue(struct vb2_queue *vq)
 	return container_of(vq, struct cio2_queue, vbq);
 }
 
+#if IS_ENABLED(CONFIG_CIO2_BRIDGE)
+int cio2_bridge_init(struct pci_dev *cio2);
+#else
+int cio2_bridge_init(struct pci_dev *cio2) { return 0; }
+#endif
+
 #endif
-- 
2.25.1


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

* Re: [PATCH v3 05/14] software_node: unregister software_nodes in reverse order
  2020-12-24  1:08 ` [PATCH v3 05/14] software_node: unregister software_nodes in reverse order Daniel Scally
@ 2020-12-24 12:13   ` Andy Shevchenko
  2020-12-24 14:00     ` Daniel Scally
  2020-12-28 16:34   ` Sakari Ailus
  1 sibling, 1 reply; 58+ messages in thread
From: Andy Shevchenko @ 2020-12-24 12:13 UTC (permalink / raw)
  To: Daniel Scally
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Sakari Ailus, Bingbu Cao,
	Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
	Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, kernel test robot, Dan Carpenter,
	Laurent Pinchart

On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally <djrscally@gmail.com> wrote:
>
> To maintain consistency with software_node_unregister_nodes(), reverse
> the order in which the software_node_unregister_node_group() function
> unregisters nodes.

...

> - * Unregister multiple software nodes at once.
> + * Unregister multiple software nodes at once. The array will be unwound in
> + * reverse order (i.e. last entry first) and thus if any member of the array
> + * has its .parent member set then they should appear later in the array such
> + * that they are unregistered first.

I'm, as being not a native speaker, a bit confused by this comment.
The idea is that children are unregistered first. Can you try to make
it more clear maybe?

>   */


-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v3 06/14] include: fwnode.h: Define format macros for ports and endpoints
  2020-12-24  1:08 ` [PATCH v3 06/14] include: fwnode.h: Define format macros for ports and endpoints Daniel Scally
@ 2020-12-24 12:17   ` Andy Shevchenko
  2020-12-24 12:41     ` Laurent Pinchart
  2020-12-28 16:30   ` Sakari Ailus
  1 sibling, 1 reply; 58+ messages in thread
From: Andy Shevchenko @ 2020-12-24 12:17 UTC (permalink / raw)
  To: Daniel Scally
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Sakari Ailus, Bingbu Cao,
	Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
	Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij

On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally <djrscally@gmail.com> wrote:
>
> OF, ACPI and software_nodes all implement graphs including nodes for ports
> and endpoints. These are all intended to be named with a common schema,
> as "port@n" and "endpoint@n" where n is an unsigned int representing the
> index of the node. To ensure commonality across the subsystems, provide a
> set of macros to define the format.

Nitpicks below, but in general that's what I meant, thanks!

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
(after addressing nitpicks)

> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v3
>         - Patch introduced
>
>  include/linux/fwnode.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 9506f8ec0974..52889efceb7d 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -32,6 +32,19 @@ struct fwnode_endpoint {
>         const struct fwnode_handle *local_fwnode;
>  };
>
> +/*
> + * ports and endpoints defined in OF, ACPI and as software_nodes should all
> + * follow a common naming scheme; use these macros to ensure commonality across
> + * the subsystems.
> + *
> + * The *PREFIX_LEN macros refer to the length of the "port@" and "endpoint@"

*PREFIX_LEN -> *_PREFIX_LEN

> + * sections of the naming scheme.
> + */
> +#define FWNODE_GRAPH_PORT_NAME_FORMAT          "port@%u"
> +#define FWNODE_GRAPH_PORT_NAME_PREFIX_LEN      5
> +#define FWNODE_GRAPH_ENDPOINT_NAME_FORMAT      "endpoint@%u"
> +#define FWNODE_GRAPH_ENDPOINT_PREFIX_LEN       9

_FORMAT -> _FMT (however, V4L2 guys may correct me, because IIRC _FMT
suffix is also used for other things in v4l2.

>  #define NR_FWNODE_REFERENCE_ARGS       8
>
>  /**

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v3 07/14] software_node: Add support for fwnode_graph*() family of functions
  2020-12-24  1:09 ` [PATCH v3 07/14] software_node: Add support for fwnode_graph*() family of functions Daniel Scally
@ 2020-12-24 12:24   ` Andy Shevchenko
  2020-12-24 12:55     ` Laurent Pinchart
  2020-12-26 23:47     ` Daniel Scally
  2020-12-24 12:53   ` Laurent Pinchart
  1 sibling, 2 replies; 58+ messages in thread
From: Andy Shevchenko @ 2020-12-24 12:24 UTC (permalink / raw)
  To: Daniel Scally
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Sakari Ailus, Bingbu Cao,
	Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
	Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij

On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally <djrscally@gmail.com> wrote:
>
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>
> This implements the remaining .graph_* callbacks in the

.graph_* ==> ->graph_*() ?

> fwnode operations structure for the software nodes. That makes
> the fwnode_graph*() functions available in the drivers also

graph*() -> graph_*() ?

> when software nodes are used.
>
> The implementation tries to mimic the "OF graph" as much as
> possible, but there is no support for the "reg" device
> property. The ports will need to have the index in their
> name which starts with "port@" (for example "port@0", "port@1",

> ...)

Looks not good, perhaps move to the previous line, or move port@1 example here?

> and endpoints will use the index of the software node
> that is given to them during creation. The port nodes can
> also be grouped under a specially named "ports" subnode,
> just like in DT, if necessary.
>
> The remote-endpoints are reference properties under the
> endpoint nodes that are named "remote-endpoint".

Few nitpicks here and there, after addressing them,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Co-developed-by: Daniel Scally <djrscally@gmail.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v3
>         - Changed software_node_get_next_endpoint() to drop the variable
>         named "old"
>         - Used the macros defined in 06/14 instead of magic numbers
>         - Added some comments to explain behaviour a little where it's unclear
>
>  drivers/base/swnode.c | 112 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 111 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 2d07eb04c6c8..ff690029060d 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -540,6 +540,112 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
>         return 0;
>  }
>
> +static struct fwnode_handle *
> +swnode_graph_find_next_port(const struct fwnode_handle *parent,
> +                           struct fwnode_handle *port)
> +{
> +       struct fwnode_handle *old = port;
> +
> +       while ((port = software_node_get_next_child(parent, old))) {
> +               /*
> +                * ports have naming style "port@n", so we search for children
> +                * that follow that convention (but without assuming anything
> +                * about the index number)
> +                */

> +               if (!strncmp(to_swnode(port)->node->name, "port@",

You may use here corresponding _FMT macro.

> +                            FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
> +                       return port;
> +               old = port;
> +       }
> +
> +       return NULL;
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
> +                                     struct fwnode_handle *endpoint)
> +{
> +       struct swnode *swnode = to_swnode(fwnode);
> +       struct fwnode_handle *parent;
> +       struct fwnode_handle *port;
> +
> +       if (!swnode)
> +               return NULL;
> +
> +       if (endpoint) {
> +               port = software_node_get_parent(endpoint);
> +               parent = software_node_get_parent(port);
> +       } else {
> +               parent = software_node_get_named_child_node(fwnode, "ports");
> +               if (!parent)
> +                       parent = software_node_get(&swnode->fwnode);
> +
> +               port = swnode_graph_find_next_port(parent, NULL);
> +       }
> +
> +       for (; port; port = swnode_graph_find_next_port(parent, port)) {
> +               endpoint = software_node_get_next_child(port, endpoint);
> +               if (endpoint) {
> +                       fwnode_handle_put(port);
> +                       break;
> +               }
> +       }
> +
> +       fwnode_handle_put(parent);
> +
> +       return endpoint;
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
> +{
> +       struct swnode *swnode = to_swnode(fwnode);
> +       const struct software_node_ref_args *ref;
> +       const struct property_entry *prop;
> +
> +       if (!swnode)
> +               return NULL;
> +
> +       prop = property_entry_get(swnode->node->properties, "remote-endpoint");
> +       if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
> +               return NULL;
> +
> +       ref = prop->pointer;
> +
> +       return software_node_get(software_node_fwnode(ref[0].node));
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
> +{
> +       struct swnode *swnode = to_swnode(fwnode);
> +
> +       swnode = swnode->parent;
> +       if (swnode && !strcmp(swnode->node->name, "ports"))
> +               swnode = swnode->parent;
> +
> +       return swnode ? software_node_get(&swnode->fwnode) : NULL;
> +}
> +
> +static int
> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
> +                                  struct fwnode_endpoint *endpoint)
> +{
> +       struct swnode *swnode = to_swnode(fwnode);
> +       int ret;
> +
> +       /* Ports have naming style "port@n", we need to select the n */

> +       ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,

Maybe a temporary variable?

  unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN;
  ...
  ret = kstrtou32(swnode->parent->node->name + prefix_len,


> +                       10, &endpoint->port);
> +       if (ret)
> +               return ret;
> +
> +       endpoint->id = swnode->id;
> +       endpoint->local_fwnode = fwnode;
> +
> +       return 0;
> +}
> +
>  static const struct fwnode_operations software_node_ops = {
>         .get = software_node_get,
>         .put = software_node_put,
> @@ -551,7 +657,11 @@ static const struct fwnode_operations software_node_ops = {
>         .get_parent = software_node_get_parent,
>         .get_next_child_node = software_node_get_next_child,
>         .get_named_child_node = software_node_get_named_child_node,
> -       .get_reference_args = software_node_get_reference_args
> +       .get_reference_args = software_node_get_reference_args,
> +       .graph_get_next_endpoint = software_node_graph_get_next_endpoint,
> +       .graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
> +       .graph_get_port_parent = software_node_graph_get_port_parent,
> +       .graph_parse_endpoint = software_node_graph_parse_endpoint,
>  };
>
>  /* -------------------------------------------------------------------------- */
> --
> 2.25.1
>


-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v3 13/14] include: media: v4l2-fwnode: Include v4l2_fwnode_bus_type
  2020-12-24  1:09 ` [PATCH v3 13/14] include: media: v4l2-fwnode: Include v4l2_fwnode_bus_type Daniel Scally
@ 2020-12-24 12:32   ` Andy Shevchenko
  2020-12-26 23:14     ` Daniel Scally
  2020-12-24 12:58   ` Laurent Pinchart
  1 sibling, 1 reply; 58+ messages in thread
From: Andy Shevchenko @ 2020-12-24 12:32 UTC (permalink / raw)
  To: Daniel Scally
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Sakari Ailus, Bingbu Cao,
	Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
	Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij

On Thu, Dec 24, 2020 at 3:13 AM Daniel Scally <djrscally@gmail.com> wrote:
>
> V4L2 fwnode bus types are enumerated in v4l2-fwnode.c, meaning they aren't
> available to the rest of the kernel. Move the enum to the corresponding
> header so that I can use the label to refer to those values.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
One nitpick below, though.

> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v3
>         - Patch introduced
>
>  drivers/media/v4l2-core/v4l2-fwnode.c | 11 -----------
>  include/media/v4l2-fwnode.h           | 22 ++++++++++++++++++++++
>  2 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 5353e37eb950..c1c2b3060532 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -28,17 +28,6 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>
> -enum v4l2_fwnode_bus_type {
> -       V4L2_FWNODE_BUS_TYPE_GUESS = 0,
> -       V4L2_FWNODE_BUS_TYPE_CSI2_CPHY,
> -       V4L2_FWNODE_BUS_TYPE_CSI1,
> -       V4L2_FWNODE_BUS_TYPE_CCP2,
> -       V4L2_FWNODE_BUS_TYPE_CSI2_DPHY,
> -       V4L2_FWNODE_BUS_TYPE_PARALLEL,
> -       V4L2_FWNODE_BUS_TYPE_BT656,
> -       NR_OF_V4L2_FWNODE_BUS_TYPE,
> -};
> -
>  static const struct v4l2_fwnode_bus_conv {
>         enum v4l2_fwnode_bus_type fwnode_bus_type;
>         enum v4l2_mbus_type mbus_type;
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 4365430eea6f..d306a28bda96 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -213,6 +213,28 @@ struct v4l2_fwnode_connector {
>         } connector;
>  };
>
> +/**
> + * enum v4l2_fwnode_bus_type - Video bus types defined by firmware properties
> + * @V4L2_FWNODE_BUS_TYPE_GUESS: Default value if no bus-type fwnode property
> + * @V4L2_FWNODE_BUS_TYPE_CSI2_CPHY: MIPI CSI-2 bus, C-PHY physical layer
> + * @V4L2_FWNODE_BUS_TYPE_CSI1: MIPI CSI-1 bus
> + * @V4L2_FWNODE_BUS_TYPE_CCP2: SMIA Compact Camera Port 2 bus
> + * @V4L2_FWNODE_BUS_TYPE_CSI2_DPHY: MIPI CSI-2 bus, D-PHY physical layer
> + * @V4L2_FWNODE_BUS_TYPE_PARALLEL: Camera Parallel Interface bus
> + * @V4L2_FWNODE_BUS_TYPE_BT656: BT656 video format bus-type
> + * @NR_OF_V4L2_FWNODE_BUS_TYPE: Number of bus-types
> + */
> +enum v4l2_fwnode_bus_type {
> +       V4L2_FWNODE_BUS_TYPE_GUESS = 0,
> +       V4L2_FWNODE_BUS_TYPE_CSI2_CPHY,
> +       V4L2_FWNODE_BUS_TYPE_CSI1,
> +       V4L2_FWNODE_BUS_TYPE_CCP2,
> +       V4L2_FWNODE_BUS_TYPE_CSI2_DPHY,
> +       V4L2_FWNODE_BUS_TYPE_PARALLEL,
> +       V4L2_FWNODE_BUS_TYPE_BT656,

> +       NR_OF_V4L2_FWNODE_BUS_TYPE,

I see that comma is in the original line, but I think it's a good time
to remove it from this line. Since it's a terminator line we might
prevent potential issues during review (by a different diff look) and
at compile time (if anything comes after it).

> +};
> +
>  /**
>   * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
>   * @fwnode: pointer to the endpoint's fwnode handle
> --
> 2.25.1
>


-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v3 06/14] include: fwnode.h: Define format macros for ports and endpoints
  2020-12-24 12:17   ` Andy Shevchenko
@ 2020-12-24 12:41     ` Laurent Pinchart
  0 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-12-24 12:41 UTC (permalink / raw)
  To: Daniel Scally
  Cc: Andy Shevchenko, Linux Kernel Mailing List,
	ACPI Devel Maling List, Linux Media Mailing List, devel,
	Rafael J. Wysocki, Len Brown, Greg Kroah-Hartman, Yong Zhi,
	Sakari Ailus, Bingbu Cao, Tian Shu Qiu, Mauro Carvalho Chehab,
	Robert Moore, Erik Kaneda, Petr Mladek, Steven Rostedt,
	Sergey Senozhatsky, Andy Shevchenko, Rasmus Villemoes,
	Laurent Pinchart, Jacopo Mondi, kieran.bingham+renesas,
	Hans Verkuil, Marco Felsch, niklas.soderlund+renesas,
	Steve Longerbeam, Krogerus, Heikki, Linus Walleij

Hi Daniel,

Thank you for the patch.

On Thu, Dec 24, 2020 at 02:17:07PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally wrote:
> >
> > OF, ACPI and software_nodes all implement graphs including nodes for ports
> > and endpoints. These are all intended to be named with a common schema,
> > as "port@n" and "endpoint@n" where n is an unsigned int representing the
> > index of the node. To ensure commonality across the subsystems, provide a
> > set of macros to define the format.
> 
> Nitpicks below, but in general that's what I meant, thanks!
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> (after addressing nitpicks)
> 
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Daniel Scally <djrscally@gmail.com>
> > ---
> > Changes in v3
> >         - Patch introduced
> >
> >  include/linux/fwnode.h | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> > index 9506f8ec0974..52889efceb7d 100644
> > --- a/include/linux/fwnode.h
> > +++ b/include/linux/fwnode.h
> > @@ -32,6 +32,19 @@ struct fwnode_endpoint {
> >         const struct fwnode_handle *local_fwnode;
> >  };
> >
> > +/*
> > + * ports and endpoints defined in OF, ACPI and as software_nodes should all
> > + * follow a common naming scheme; use these macros to ensure commonality across
> > + * the subsystems.
> > + *
> > + * The *PREFIX_LEN macros refer to the length of the "port@" and "endpoint@"
> 
> *PREFIX_LEN -> *_PREFIX_LEN
> 
> > + * sections of the naming scheme.
> > + */
> > +#define FWNODE_GRAPH_PORT_NAME_FORMAT          "port@%u"
> > +#define FWNODE_GRAPH_PORT_NAME_PREFIX_LEN      5
> > +#define FWNODE_GRAPH_ENDPOINT_NAME_FORMAT      "endpoint@%u"
> > +#define FWNODE_GRAPH_ENDPOINT_PREFIX_LEN       9
> 
> _FORMAT -> _FMT (however, V4L2 guys may correct me, because IIRC _FMT
> suffix is also used for other things in v4l2.

This isn't related to V4L2, so it doesn't matter much :-) I personally
prefer spelling names out in full when that wouldn't result in too long
lines, but it's really a matter of personal preference, I don't mind
either way.

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

> >  #define NR_FWNODE_REFERENCE_ARGS       8
> >
> >  /**

-- 
Regards,

Laurent Pinchart

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

* Re: [PATCH v3 07/14] software_node: Add support for fwnode_graph*() family of functions
  2020-12-24  1:09 ` [PATCH v3 07/14] software_node: Add support for fwnode_graph*() family of functions Daniel Scally
  2020-12-24 12:24   ` Andy Shevchenko
@ 2020-12-24 12:53   ` Laurent Pinchart
  2020-12-24 14:24     ` Daniel Scally
  1 sibling, 1 reply; 58+ messages in thread
From: Laurent Pinchart @ 2020-12-24 12:53 UTC (permalink / raw)
  To: Daniel Scally
  Cc: linux-kernel, linux-acpi, linux-media, devel, rjw, lenb, gregkh,
	yong.zhi, sakari.ailus, bingbu.cao, tian.shu.qiu, mchehab,
	robert.moore, erik.kaneda, pmladek, rostedt, sergey.senozhatsky,
	andriy.shevchenko, linux, laurent.pinchart+renesas,
	jacopo+renesas, kieran.bingham+renesas, hverkuil-cisco, m.felsch,
	niklas.soderlund+renesas, slongerbeam, heikki.krogerus,
	linus.walleij

Hi Daniel,

Thank you for the patch.

On Thu, Dec 24, 2020 at 01:09:00AM +0000, Daniel Scally wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> This implements the remaining .graph_* callbacks in the
> fwnode operations structure for the software nodes. That makes
> the fwnode_graph*() functions available in the drivers also
> when software nodes are used.
> 
> The implementation tries to mimic the "OF graph" as much as
> possible, but there is no support for the "reg" device
> property. The ports will need to have the index in their
> name which starts with "port@" (for example "port@0", "port@1",
> ...) and endpoints will use the index of the software node
> that is given to them during creation. The port nodes can
> also be grouped under a specially named "ports" subnode,
> just like in DT, if necessary.
> 
> The remote-endpoints are reference properties under the
> endpoint nodes that are named "remote-endpoint".
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Co-developed-by: Daniel Scally <djrscally@gmail.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v3
> 	- Changed software_node_get_next_endpoint() to drop the variable
> 	named "old"
> 	- Used the macros defined in 06/14 instead of magic numbers
> 	- Added some comments to explain behaviour a little where it's unclear
> 
>  drivers/base/swnode.c | 112 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 111 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 2d07eb04c6c8..ff690029060d 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -540,6 +540,112 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
>  	return 0;
>  }
>  
> +static struct fwnode_handle *
> +swnode_graph_find_next_port(const struct fwnode_handle *parent,
> +			    struct fwnode_handle *port)
> +{
> +	struct fwnode_handle *old = port;
> +
> +	while ((port = software_node_get_next_child(parent, old))) {
> +		/*
> +		 * ports have naming style "port@n", so we search for children
> +		 * that follow that convention (but without assuming anything
> +		 * about the index number)
> +		 */
> +		if (!strncmp(to_swnode(port)->node->name, "port@",
> +			     FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))

I would either add a macro to replace the prefix ("port@"), or drop
FWNODE_GRAPH_PORT_NAME_PREFIX_LEN. I think this is the worst of both
worlds, the string and its length are defined in two different places
:-)

I would personally drop the macro, but I don't mind either way as long
as the string and its length are defined in the same place.

> +			return port;
> +		old = port;
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
> +				      struct fwnode_handle *endpoint)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	struct fwnode_handle *parent;
> +	struct fwnode_handle *port;
> +
> +	if (!swnode)
> +		return NULL;
> +
> +	if (endpoint) {
> +		port = software_node_get_parent(endpoint);
> +		parent = software_node_get_parent(port);
> +	} else {
> +		parent = software_node_get_named_child_node(fwnode, "ports");
> +		if (!parent)
> +			parent = software_node_get(&swnode->fwnode);
> +
> +		port = swnode_graph_find_next_port(parent, NULL);
> +	}
> +
> +	for (; port; port = swnode_graph_find_next_port(parent, port)) {
> +		endpoint = software_node_get_next_child(port, endpoint);
> +		if (endpoint) {
> +			fwnode_handle_put(port);
> +			break;
> +		}
> +	}
> +
> +	fwnode_handle_put(parent);
> +
> +	return endpoint;
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	const struct software_node_ref_args *ref;
> +	const struct property_entry *prop;
> +
> +	if (!swnode)
> +		return NULL;
> +
> +	prop = property_entry_get(swnode->node->properties, "remote-endpoint");
> +	if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
> +		return NULL;
> +
> +	ref = prop->pointer;
> +
> +	return software_node_get(software_node_fwnode(ref[0].node));
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +
> +	swnode = swnode->parent;
> +	if (swnode && !strcmp(swnode->node->name, "ports"))
> +		swnode = swnode->parent;
> +
> +	return swnode ? software_node_get(&swnode->fwnode) : NULL;
> +}
> +
> +static int
> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
> +				   struct fwnode_endpoint *endpoint)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	int ret;
> +
> +	/* Ports have naming style "port@n", we need to select the n */
> +	ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
> +			10, &endpoint->port);

Same here.

I wonder if we should add a check to ensure parent->node->name is long
enough (and possibly even start with the right prefix), as otherwise the
pointer passed to kstrtou32() may be past the end of the string. Maybe
this is overkill, if we can rely on the fact that software nodes have
correct names.

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

> +	if (ret)
> +		return ret;
> +
> +	endpoint->id = swnode->id;
> +	endpoint->local_fwnode = fwnode;
> +
> +	return 0;
> +}
> +
>  static const struct fwnode_operations software_node_ops = {
>  	.get = software_node_get,
>  	.put = software_node_put,
> @@ -551,7 +657,11 @@ static const struct fwnode_operations software_node_ops = {
>  	.get_parent = software_node_get_parent,
>  	.get_next_child_node = software_node_get_next_child,
>  	.get_named_child_node = software_node_get_named_child_node,
> -	.get_reference_args = software_node_get_reference_args
> +	.get_reference_args = software_node_get_reference_args,
> +	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
> +	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
> +	.graph_get_port_parent = software_node_graph_get_port_parent,
> +	.graph_parse_endpoint = software_node_graph_parse_endpoint,
>  };
>  
>  /* -------------------------------------------------------------------------- */

-- 
Regards,

Laurent Pinchart

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

* Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2020-12-24  1:09 ` [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver Daniel Scally
@ 2020-12-24 12:54   ` Andy Shevchenko
  2020-12-26 23:23     ` Daniel Scally
  2020-12-28 17:05     ` Sakari Ailus
  0 siblings, 2 replies; 58+ messages in thread
From: Andy Shevchenko @ 2020-12-24 12:54 UTC (permalink / raw)
  To: Daniel Scally
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Sakari Ailus, Bingbu Cao,
	Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
	Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, Jordan Hand, Laurent Pinchart

On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally <djrscally@gmail.com> wrote:
>
> Currently on platforms designed for Windows, connections between CIO2 and
> sensors are not properly defined in DSDT. This patch extends the ipu3-cio2
> driver to compensate by building software_node connections, parsing the
> connection properties from the sensor's SSDB buffer.

Few nitpicks below, after addressing them
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v3
>         - Used Laurent's suggestion to simplify initing the property names
>         - Wrapped some lines
>         - Fixed return and error handling for cio2_bridge_read_acpi_buffer()
>         - Returned an error if more sensors than available ports are detected
>         - Used defines for port/endpoint name formats and the bus-type property
>         - Some bits of cleanup
>
>  MAINTAINERS                                   |   1 +
>  drivers/media/pci/intel/ipu3/Kconfig          |  18 ++
>  drivers/media/pci/intel/ipu3/Makefile         |   1 +
>  drivers/media/pci/intel/ipu3/cio2-bridge.c    | 272 ++++++++++++++++++
>  drivers/media/pci/intel/ipu3/cio2-bridge.h    | 122 ++++++++
>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  34 +++
>  drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   6 +
>  7 files changed, 454 insertions(+)
>  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
>  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 16b544624577..e7784b4bc8ea 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8943,6 +8943,7 @@ INTEL IPU3 CSI-2 CIO2 DRIVER
>  M:     Yong Zhi <yong.zhi@intel.com>
>  M:     Sakari Ailus <sakari.ailus@linux.intel.com>
>  M:     Bingbu Cao <bingbu.cao@intel.com>
> +M:     Dan Scally <djrscally@gmail.com>
>  R:     Tianshu Qiu <tian.shu.qiu@intel.com>
>  L:     linux-media@vger.kernel.org
>  S:     Maintained
> diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig
> index 82d7f17e6a02..dcf5c4b74673 100644
> --- a/drivers/media/pci/intel/ipu3/Kconfig
> +++ b/drivers/media/pci/intel/ipu3/Kconfig
> @@ -16,3 +16,21 @@ config VIDEO_IPU3_CIO2
>           Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
>           connected camera.
>           The module will be called ipu3-cio2.
> +
> +config CIO2_BRIDGE
> +       bool "IPU3 CIO2 Sensors Bridge"
> +       depends on VIDEO_IPU3_CIO2
> +       help
> +         This extension provides an API for the ipu3-cio2 driver to create
> +         connections to cameras that are hidden in SSDB buffer in ACPI. It

in the

> +         can be used to enable support for cameras in detachable / hybrid
> +         devices that ship with Windows.
> +
> +         Say Y here if your device is a detachable / hybrid laptop that comes
> +         with Windows installed by the OEM, for example:
> +
> +               - Microsoft Surface models (except Surface Pro 3)
> +               - The Lenovo Miix line (for example the 510, 520, 710 and 720)
> +               - Dell 7285
> +
> +         If in doubt, say N here.
> diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile
> index 429d516452e4..933777e6ea8a 100644
> --- a/drivers/media/pci/intel/ipu3/Makefile
> +++ b/drivers/media/pci/intel/ipu3/Makefile
> @@ -2,3 +2,4 @@
>  obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
>
>  ipu3-cio2-y += ipu3-cio2-main.o
> +ipu3-cio2-$(CONFIG_CIO2_BRIDGE) += cio2-bridge.o
> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> new file mode 100644
> index 000000000000..3f4ae172fd25
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> @@ -0,0 +1,272 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Author: Dan Scally <djrscally@gmail.com> */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/pci.h>
> +#include <linux/property.h>
> +#include <media/v4l2-fwnode.h>
> +
> +#include "cio2-bridge.h"
> +
> +/*
> + * Extend this array with ACPI Hardware ID's of devices known to be working

ID's -> IDs ?

> + * plus the number of link-frequencies expected by their drivers, along with
> + * the frequency values in hertz. This is somewhat opportunistic way of adding
> + * support for this for now in the hopes of a better source for the information
> + * (possibly some encoded value in the SSDB buffer that we're unaware of)
> + * becoming apparent in the future.
> + *
> + * Do not add an entry for a sensor that is not actually supported.
> + */
> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
> +       CIO2_SENSOR_CONFIG("INT33BE", 0),
> +       CIO2_SENSOR_CONFIG("OVTI2680", 0),
> +};
> +
> +static const struct cio2_property_names prop_names = {
> +       .clock_frequency = "clock-frequency",
> +       .rotation = "rotation",
> +       .bus_type = "bus-type",
> +       .data_lanes = "data-lanes",
> +       .remote_endpoint = "remote-endpoint",
> +       .link_frequencies = "link-frequencies",
> +};
> +
> +static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
> +                                       void *data, u32 size)
> +{
> +       struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +       union acpi_object *obj;
> +       acpi_status status;
> +       int ret = 0;
> +
> +       status = acpi_evaluate_object(adev->handle, id, NULL, &buffer);
> +       if (ACPI_FAILURE(status))
> +               return -ENODEV;
> +
> +       obj = buffer.pointer;
> +       if (!obj) {
> +               dev_err(&adev->dev, "Couldn't locate ACPI buffer\n");
> +               return -ENODEV;
> +       }
> +
> +       if (obj->type != ACPI_TYPE_BUFFER) {
> +               dev_err(&adev->dev, "Not an ACPI buffer\n");
> +               ret = -ENODEV;
> +               goto out_free_buff;
> +       }
> +
> +       if (obj->buffer.length > size) {
> +               dev_err(&adev->dev, "Given buffer is too small\n");
> +               ret = -EINVAL;
> +               goto out_free_buff;
> +       }
> +
> +       memcpy(data, obj->buffer.pointer, obj->buffer.length);
> +
> +out_free_buff:
> +       kfree(buffer.pointer);
> +       return ret;
> +}
> +
> +static void cio2_bridge_create_fwnode_properties(struct cio2_sensor *sensor,
> +                                                const struct cio2_sensor_config *cfg)
> +{
> +       unsigned int i;
> +
> +       sensor->prop_names = prop_names;
> +
> +       for (i = 0; i < 4; i++)

4 here and below, can we have a local define for them, like

  #define CIO2_MAX_LANES  4

> +               sensor->data_lanes[i] = i + 1;
> +
> +       sensor->local_ref[0].node = &sensor->swnodes[SWNODE_CIO2_ENDPOINT];
> +       sensor->remote_ref[0].node = &sensor->swnodes[SWNODE_SENSOR_ENDPOINT];
> +
> +       sensor->dev_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.clock_frequency,
> +                                                      sensor->ssdb.mclkspeed);
> +       sensor->dev_properties[1] = PROPERTY_ENTRY_U8(sensor->prop_names.rotation,
> +                                                     sensor->ssdb.degree);
> +
> +       sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type,
> +                                                     V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
> +       sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
> +                                                               sensor->data_lanes,
> +                                                               sensor->ssdb.lanes);
> +       sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
> +                                                           sensor->local_ref);
> +
> +       if (cfg->nr_link_freqs > 0)
> +               sensor->ep_properties[3] = PROPERTY_ENTRY_U64_ARRAY_LEN(
> +                                               sensor->prop_names.link_frequencies,
> +                                               cfg->link_freqs,
> +                                               cfg->nr_link_freqs);
> +
> +       sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
> +                                                                 sensor->data_lanes,
> +                                                                 sensor->ssdb.lanes);
> +       sensor->cio2_properties[1] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
> +                                                             sensor->remote_ref);
> +}
> +
> +static void cio2_bridge_init_swnode_names(struct cio2_sensor *sensor)
> +{
> +       snprintf(sensor->node_names.remote_port, sizeof(sensor->node_names.remote_port),
> +                FWNODE_GRAPH_PORT_NAME_FORMAT, sensor->ssdb.link);
> +       snprintf(sensor->node_names.port, sizeof(sensor->node_names.port),
> +                FWNODE_GRAPH_PORT_NAME_FORMAT, 0); /* Always port 0 */
> +       snprintf(sensor->node_names.endpoint, sizeof(sensor->node_names.endpoint),
> +                FWNODE_GRAPH_ENDPOINT_NAME_FORMAT, 0); /* And endpoint 0 */
> +}
> +
> +static void cio2_bridge_create_connection_swnodes(struct cio2_bridge *bridge,
> +                                                 struct cio2_sensor *sensor)
> +{
> +       struct software_node *nodes = sensor->swnodes;
> +
> +       cio2_bridge_init_swnode_names(sensor);
> +
> +       nodes[SWNODE_SENSOR_HID] = NODE_SENSOR(sensor->name,
> +                                              sensor->dev_properties);
> +       nodes[SWNODE_SENSOR_PORT] = NODE_PORT(sensor->node_names.port,
> +                                             &nodes[SWNODE_SENSOR_HID]);
> +       nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
> +                                                     &nodes[SWNODE_SENSOR_PORT],
> +                                                     sensor->ep_properties);
> +       nodes[SWNODE_CIO2_PORT] = NODE_PORT(sensor->node_names.remote_port,
> +                                           &bridge->cio2_hid_node);
> +       nodes[SWNODE_CIO2_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
> +                                                   &nodes[SWNODE_CIO2_PORT],
> +                                                   sensor->cio2_properties);
> +}
> +
> +static void cio2_bridge_unregister_sensors(struct cio2_bridge *bridge)
> +{
> +       struct cio2_sensor *sensor;
> +       unsigned int i;
> +
> +       for (i = 0; i < bridge->n_sensors; i++) {
> +               sensor = &bridge->sensors[i];
> +               software_node_unregister_nodes(sensor->swnodes);
> +               acpi_dev_put(sensor->adev);
> +       }
> +}
> +
> +static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge,
> +                                      struct pci_dev *cio2)
> +{
> +       struct fwnode_handle *fwnode;
> +       struct cio2_sensor *sensor;
> +       struct acpi_device *adev;
> +       unsigned int i;
> +       int ret = 0;

You may drop this assignment and...

> +       for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
> +               const struct cio2_sensor_config *cfg = &cio2_supported_sensors[i];
> +
> +               for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> +                       if (bridge->n_sensors >= CIO2_NUM_PORTS) {
> +                               dev_err(&cio2->dev, "Exceeded available CIO2 ports\n");
> +                               cio2_bridge_unregister_sensors(bridge);
> +                               ret = -EINVAL;
> +                               goto err_out;
> +                       }
> +
> +                       if (!adev->status.enabled)
> +                               continue;
> +
> +                       sensor = &bridge->sensors[bridge->n_sensors];
> +                       sensor->adev = adev;
> +                       strscpy(sensor->name, cfg->hid, sizeof(sensor->name));
> +
> +                       ret = cio2_bridge_read_acpi_buffer(adev, "SSDB",
> +                                                          &sensor->ssdb,
> +                                                          sizeof(sensor->ssdb));
> +                       if (ret)
> +                               goto err_put_adev;
> +
> +                       if (sensor->ssdb.lanes > 4) {

CIO2_MAX_LANES

> +                               dev_err(&adev->dev,
> +                                       "Number of lanes in SSDB is invalid\n");
> +                               ret = -EINVAL;
> +                               goto err_put_adev;
> +                       }
> +
> +                       cio2_bridge_create_fwnode_properties(sensor, cfg);
> +                       cio2_bridge_create_connection_swnodes(bridge, sensor);
> +
> +                       ret = software_node_register_nodes(sensor->swnodes);
> +                       if (ret)
> +                               goto err_put_adev;
> +
> +                       fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_SENSOR_HID]);
> +                       if (!fwnode) {
> +                               ret = -ENODEV;
> +                               goto err_free_swnodes;
> +                       }
> +
> +                       adev->fwnode.secondary = fwnode;
> +
> +                       dev_info(&cio2->dev, "Found supported sensor %s\n",
> +                                acpi_dev_name(adev));
> +
> +                       bridge->n_sensors++;
> +               }
> +       }

> +       return ret;

...use here

  return 0;

directly.

> +err_free_swnodes:
> +       software_node_unregister_nodes(sensor->swnodes);
> +err_put_adev:
> +       acpi_dev_put(sensor->adev);
> +err_out:
> +       return ret;
> +}
> +
> +int cio2_bridge_init(struct pci_dev *cio2)
> +{
> +       struct device *dev = &cio2->dev;
> +       struct fwnode_handle *fwnode;
> +       struct cio2_bridge *bridge;
> +       int ret;
> +
> +       bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> +       if (!bridge)
> +               return -ENOMEM;
> +
> +       strscpy(bridge->cio2_node_name, CIO2_HID, sizeof(bridge->cio2_node_name));
> +       bridge->cio2_hid_node.name = bridge->cio2_node_name;
> +
> +       ret = software_node_register(&bridge->cio2_hid_node);
> +       if (ret < 0) {
> +               dev_err(dev, "Failed to register the CIO2 HID node\n");
> +               goto err_free_bridge;
> +       }
> +
> +       ret = cio2_bridge_connect_sensors(bridge, cio2);
> +       if (ret || bridge->n_sensors == 0)
> +               goto err_unregister_cio2;
> +
> +       dev_info(dev, "Connected %d cameras\n", bridge->n_sensors);
> +
> +       fwnode = software_node_fwnode(&bridge->cio2_hid_node);
> +       if (!fwnode) {
> +               dev_err(dev, "Error getting fwnode from cio2 software_node\n");
> +               ret = -ENODEV;
> +               goto err_unregister_sensors;
> +       }
> +
> +       set_secondary_fwnode(dev, fwnode);
> +
> +       return 0;
> +
> +err_unregister_sensors:
> +       cio2_bridge_unregister_sensors(bridge);
> +err_unregister_cio2:
> +       software_node_unregister(&bridge->cio2_hid_node);
> +err_free_bridge:
> +       kfree(bridge);
> +
> +       return ret;
> +}
> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.h b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> new file mode 100644
> index 000000000000..004b608f322f
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> @@ -0,0 +1,122 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Author: Dan Scally <djrscally@gmail.com> */
> +#ifndef __CIO2_BRIDGE_H
> +#define __CIO2_BRIDGE_H
> +
> +#include <linux/property.h>
> +
> +#define CIO2_HID                               "INT343E"
> +#define CIO2_NUM_PORTS                         4
> +#define MAX_NUM_LINK_FREQS                     3
> +
> +#define CIO2_SENSOR_CONFIG(_HID, _NR, ...)     \
> +       {                                       \
> +               .hid = _HID,                    \
> +               .nr_link_freqs = _NR,           \
> +               .link_freqs = { __VA_ARGS__ }   \
> +       }
> +
> +#define NODE_SENSOR(_HID, _PROPS)              \
> +       ((const struct software_node) {         \
> +               .name = _HID,                   \
> +               .properties = _PROPS,           \
> +       })
> +
> +#define NODE_PORT(_PORT, _SENSOR_NODE)         \
> +       ((const struct software_node) {         \
> +               _PORT,                          \
> +               _SENSOR_NODE,                   \
> +       })
> +
> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)      \
> +       ((const struct software_node) {         \
> +               _EP,                            \
> +               _PORT,                          \
> +               _PROPS,                         \
> +       })
> +
> +enum cio2_sensor_swnodes {
> +       SWNODE_SENSOR_HID,
> +       SWNODE_SENSOR_PORT,
> +       SWNODE_SENSOR_ENDPOINT,
> +       SWNODE_CIO2_PORT,
> +       SWNODE_CIO2_ENDPOINT,

> +       SWNODE_COUNT,

No comma?

> +};
> +
> +/* Data representation as it is in ACPI SSDB buffer */
> +struct cio2_sensor_ssdb {
> +       u8 version;
> +       u8 sku;
> +       u8 guid_csi2[16];
> +       u8 devfunction;
> +       u8 bus;
> +       u32 dphylinkenfuses;
> +       u32 clockdiv;
> +       u8 link;
> +       u8 lanes;
> +       u32 csiparams[10];
> +       u32 maxlanespeed;
> +       u8 sensorcalibfileidx;
> +       u8 sensorcalibfileidxInMBZ[3];
> +       u8 romtype;
> +       u8 vcmtype;
> +       u8 platforminfo;
> +       u8 platformsubinfo;
> +       u8 flash;
> +       u8 privacyled;
> +       u8 degree;
> +       u8 mipilinkdefined;
> +       u32 mclkspeed;
> +       u8 controllogicid;
> +       u8 reserved1[3];
> +       u8 mclkport;
> +       u8 reserved2[13];
> +} __packed;
> +
> +struct cio2_property_names {
> +       char clock_frequency[16];
> +       char rotation[9];
> +       char bus_type[9];
> +       char data_lanes[11];
> +       char remote_endpoint[16];
> +       char link_frequencies[17];
> +};
> +
> +struct cio2_node_names {
> +       char port[7];
> +       char endpoint[11];
> +       char remote_port[7];
> +};
> +
> +struct cio2_sensor_config {
> +       const char *hid;
> +       const u8 nr_link_freqs;
> +       const u64 link_freqs[MAX_NUM_LINK_FREQS];
> +};
> +
> +struct cio2_sensor {
> +       char name[ACPI_ID_LEN];
> +       struct acpi_device *adev;
> +
> +       struct software_node swnodes[6];
> +       struct cio2_node_names node_names;
> +
> +       u32 data_lanes[4];
> +       struct cio2_sensor_ssdb ssdb;
> +       struct cio2_property_names prop_names;
> +       struct property_entry ep_properties[5];
> +       struct property_entry dev_properties[3];
> +       struct property_entry cio2_properties[3];
> +       struct software_node_ref_args local_ref[1];
> +       struct software_node_ref_args remote_ref[1];
> +};
> +
> +struct cio2_bridge {
> +       char cio2_node_name[ACPI_ID_LEN];
> +       struct software_node cio2_hid_node;
> +       unsigned int n_sensors;
> +       struct cio2_sensor sensors[CIO2_NUM_PORTS];
> +};
> +
> +#endif
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> index 36e354ecf71e..68ff28abc6a3 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> @@ -1702,11 +1702,28 @@ static void cio2_queues_exit(struct cio2_device *cio2)
>                 cio2_queue_exit(cio2, &cio2->queue[i]);
>  }
>
> +static bool cio2_check_fwnode_graph(struct fwnode_handle *fwnode)
> +{
> +       struct fwnode_handle *endpoint;
> +
> +       if (IS_ERR_OR_NULL(fwnode))
> +               return false;
> +
> +       endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> +       if (endpoint) {
> +               fwnode_handle_put(endpoint);
> +               return true;
> +       }
> +
> +       return cio2_check_fwnode_graph(fwnode->secondary);
> +}
> +
>  /**************** PCI interface ****************/
>
>  static int cio2_pci_probe(struct pci_dev *pci_dev,
>                           const struct pci_device_id *id)
>  {
> +       struct fwnode_handle *fwnode = dev_fwnode(&pci_dev->dev);
>         struct cio2_device *cio2;
>         int r;
>
> @@ -1715,6 +1732,23 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>                 return -ENOMEM;
>         cio2->pci_dev = pci_dev;
>
> +       /*
> +        * On some platforms no connections to sensors are defined in firmware,
> +        * if the device has no endpoints then we can try to build those as
> +        * software_nodes parsed from SSDB.
> +        */
> +       if (!cio2_check_fwnode_graph(fwnode)) {
> +               if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) {

> +                       dev_err(&pci_dev->dev,
> +                               "fwnode graph has no endpoints connected\n");

One line?

> +                       return -EINVAL;
> +               }
> +
> +               r = cio2_bridge_init(pci_dev);
> +               if (r)
> +                       return r;
> +       }
> +
>         r = pcim_enable_device(pci_dev);
>         if (r) {
>                 dev_err(&pci_dev->dev, "failed to enable device (%d)\n", r);
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> index ccf0b85ae36f..520a27c9cdad 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> @@ -437,4 +437,10 @@ static inline struct cio2_queue *vb2q_to_cio2_queue(struct vb2_queue *vq)
>         return container_of(vq, struct cio2_queue, vbq);
>  }
>
> +#if IS_ENABLED(CONFIG_CIO2_BRIDGE)
> +int cio2_bridge_init(struct pci_dev *cio2);
> +#else
> +int cio2_bridge_init(struct pci_dev *cio2) { return 0; }
> +#endif
> +
>  #endif
> --
> 2.25.1
>


-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v3 07/14] software_node: Add support for fwnode_graph*() family of functions
  2020-12-24 12:24   ` Andy Shevchenko
@ 2020-12-24 12:55     ` Laurent Pinchart
  2020-12-24 13:03       ` Andy Shevchenko
  2020-12-26 23:47     ` Daniel Scally
  1 sibling, 1 reply; 58+ messages in thread
From: Laurent Pinchart @ 2020-12-24 12:55 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Daniel Scally, Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Sakari Ailus, Bingbu Cao,
	Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
	Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij

Hi Andy,

On Thu, Dec 24, 2020 at 02:24:12PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally wrote:
> >
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >
> > This implements the remaining .graph_* callbacks in the
> 
> .graph_* ==> ->graph_*() ?
> 
> > fwnode operations structure for the software nodes. That makes
> > the fwnode_graph*() functions available in the drivers also
> 
> graph*() -> graph_*() ?
> 
> > when software nodes are used.
> >
> > The implementation tries to mimic the "OF graph" as much as
> > possible, but there is no support for the "reg" device
> > property. The ports will need to have the index in their
> > name which starts with "port@" (for example "port@0", "port@1",
> 
> > ...)
> 
> Looks not good, perhaps move to the previous line, or move port@1 example here?
> 
> > and endpoints will use the index of the software node
> > that is given to them during creation. The port nodes can
> > also be grouped under a specially named "ports" subnode,
> > just like in DT, if necessary.
> >
> > The remote-endpoints are reference properties under the
> > endpoint nodes that are named "remote-endpoint".
> 
> Few nitpicks here and there, after addressing them,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Co-developed-by: Daniel Scally <djrscally@gmail.com>
> > Signed-off-by: Daniel Scally <djrscally@gmail.com>
> > ---
> > Changes in v3
> >         - Changed software_node_get_next_endpoint() to drop the variable
> >         named "old"
> >         - Used the macros defined in 06/14 instead of magic numbers
> >         - Added some comments to explain behaviour a little where it's unclear
> >
> >  drivers/base/swnode.c | 112 +++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 111 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> > index 2d07eb04c6c8..ff690029060d 100644
> > --- a/drivers/base/swnode.c
> > +++ b/drivers/base/swnode.c
> > @@ -540,6 +540,112 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
> >         return 0;
> >  }
> >
> > +static struct fwnode_handle *
> > +swnode_graph_find_next_port(const struct fwnode_handle *parent,
> > +                           struct fwnode_handle *port)
> > +{
> > +       struct fwnode_handle *old = port;
> > +
> > +       while ((port = software_node_get_next_child(parent, old))) {
> > +               /*
> > +                * ports have naming style "port@n", so we search for children
> > +                * that follow that convention (but without assuming anything
> > +                * about the index number)
> > +                */
> 
> > +               if (!strncmp(to_swnode(port)->node->name, "port@",
> 
> You may use here corresponding _FMT macro.
> 
> > +                            FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
> > +                       return port;
> > +               old = port;
> > +       }
> > +
> > +       return NULL;
> > +}
> > +
> > +static struct fwnode_handle *
> > +software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
> > +                                     struct fwnode_handle *endpoint)
> > +{
> > +       struct swnode *swnode = to_swnode(fwnode);
> > +       struct fwnode_handle *parent;
> > +       struct fwnode_handle *port;
> > +
> > +       if (!swnode)
> > +               return NULL;
> > +
> > +       if (endpoint) {
> > +               port = software_node_get_parent(endpoint);
> > +               parent = software_node_get_parent(port);
> > +       } else {
> > +               parent = software_node_get_named_child_node(fwnode, "ports");
> > +               if (!parent)
> > +                       parent = software_node_get(&swnode->fwnode);
> > +
> > +               port = swnode_graph_find_next_port(parent, NULL);
> > +       }
> > +
> > +       for (; port; port = swnode_graph_find_next_port(parent, port)) {
> > +               endpoint = software_node_get_next_child(port, endpoint);
> > +               if (endpoint) {
> > +                       fwnode_handle_put(port);
> > +                       break;
> > +               }
> > +       }
> > +
> > +       fwnode_handle_put(parent);
> > +
> > +       return endpoint;
> > +}
> > +
> > +static struct fwnode_handle *
> > +software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
> > +{
> > +       struct swnode *swnode = to_swnode(fwnode);
> > +       const struct software_node_ref_args *ref;
> > +       const struct property_entry *prop;
> > +
> > +       if (!swnode)
> > +               return NULL;
> > +
> > +       prop = property_entry_get(swnode->node->properties, "remote-endpoint");
> > +       if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
> > +               return NULL;
> > +
> > +       ref = prop->pointer;
> > +
> > +       return software_node_get(software_node_fwnode(ref[0].node));
> > +}
> > +
> > +static struct fwnode_handle *
> > +software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
> > +{
> > +       struct swnode *swnode = to_swnode(fwnode);
> > +
> > +       swnode = swnode->parent;
> > +       if (swnode && !strcmp(swnode->node->name, "ports"))
> > +               swnode = swnode->parent;
> > +
> > +       return swnode ? software_node_get(&swnode->fwnode) : NULL;
> > +}
> > +
> > +static int
> > +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
> > +                                  struct fwnode_endpoint *endpoint)
> > +{
> > +       struct swnode *swnode = to_swnode(fwnode);
> > +       int ret;
> > +
> > +       /* Ports have naming style "port@n", we need to select the n */
> 
> > +       ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
> 
> Maybe a temporary variable?
> 
>   unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN;
>   ...
>   ret = kstrtou32(swnode->parent->node->name + prefix_len,

Honestly I'm wondering if those macros don't hinder readability. I'd
rather write

	+ strlen("port@")

and let the compiler optimize this to a compile-time constant.

> > +                       10, &endpoint->port);
> > +       if (ret)
> > +               return ret;
> > +
> > +       endpoint->id = swnode->id;
> > +       endpoint->local_fwnode = fwnode;
> > +
> > +       return 0;
> > +}
> > +
> >  static const struct fwnode_operations software_node_ops = {
> >         .get = software_node_get,
> >         .put = software_node_put,
> > @@ -551,7 +657,11 @@ static const struct fwnode_operations software_node_ops = {
> >         .get_parent = software_node_get_parent,
> >         .get_next_child_node = software_node_get_next_child,
> >         .get_named_child_node = software_node_get_named_child_node,
> > -       .get_reference_args = software_node_get_reference_args
> > +       .get_reference_args = software_node_get_reference_args,
> > +       .graph_get_next_endpoint = software_node_graph_get_next_endpoint,
> > +       .graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
> > +       .graph_get_port_parent = software_node_graph_get_port_parent,
> > +       .graph_parse_endpoint = software_node_graph_parse_endpoint,
> >  };
> >
> >  /* -------------------------------------------------------------------------- */

-- 
Regards,

Laurent Pinchart

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

* Re: [PATCH v3 13/14] include: media: v4l2-fwnode: Include v4l2_fwnode_bus_type
  2020-12-24  1:09 ` [PATCH v3 13/14] include: media: v4l2-fwnode: Include v4l2_fwnode_bus_type Daniel Scally
  2020-12-24 12:32   ` Andy Shevchenko
@ 2020-12-24 12:58   ` Laurent Pinchart
  1 sibling, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-12-24 12:58 UTC (permalink / raw)
  To: Daniel Scally
  Cc: linux-kernel, linux-acpi, linux-media, devel, rjw, lenb, gregkh,
	yong.zhi, sakari.ailus, bingbu.cao, tian.shu.qiu, mchehab,
	robert.moore, erik.kaneda, pmladek, rostedt, sergey.senozhatsky,
	andriy.shevchenko, linux, laurent.pinchart+renesas,
	jacopo+renesas, kieran.bingham+renesas, hverkuil-cisco, m.felsch,
	niklas.soderlund+renesas, slongerbeam, heikki.krogerus,
	linus.walleij

Hi Daniel,

Thank you for the patch.

On Thu, Dec 24, 2020 at 01:09:06AM +0000, Daniel Scally wrote:
> V4L2 fwnode bus types are enumerated in v4l2-fwnode.c, meaning they aren't
> available to the rest of the kernel. Move the enum to the corresponding
> header so that I can use the label to refer to those values.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v3
> 	- Patch introduced
> 
>  drivers/media/v4l2-core/v4l2-fwnode.c | 11 -----------
>  include/media/v4l2-fwnode.h           | 22 ++++++++++++++++++++++
>  2 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 5353e37eb950..c1c2b3060532 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -28,17 +28,6 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
> -enum v4l2_fwnode_bus_type {
> -	V4L2_FWNODE_BUS_TYPE_GUESS = 0,
> -	V4L2_FWNODE_BUS_TYPE_CSI2_CPHY,
> -	V4L2_FWNODE_BUS_TYPE_CSI1,
> -	V4L2_FWNODE_BUS_TYPE_CCP2,
> -	V4L2_FWNODE_BUS_TYPE_CSI2_DPHY,
> -	V4L2_FWNODE_BUS_TYPE_PARALLEL,
> -	V4L2_FWNODE_BUS_TYPE_BT656,
> -	NR_OF_V4L2_FWNODE_BUS_TYPE,
> -};
> -
>  static const struct v4l2_fwnode_bus_conv {
>  	enum v4l2_fwnode_bus_type fwnode_bus_type;
>  	enum v4l2_mbus_type mbus_type;
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 4365430eea6f..d306a28bda96 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -213,6 +213,28 @@ struct v4l2_fwnode_connector {
>  	} connector;
>  };
>  
> +/**
> + * enum v4l2_fwnode_bus_type - Video bus types defined by firmware properties
> + * @V4L2_FWNODE_BUS_TYPE_GUESS: Default value if no bus-type fwnode property
> + * @V4L2_FWNODE_BUS_TYPE_CSI2_CPHY: MIPI CSI-2 bus, C-PHY physical layer
> + * @V4L2_FWNODE_BUS_TYPE_CSI1: MIPI CSI-1 bus
> + * @V4L2_FWNODE_BUS_TYPE_CCP2: SMIA Compact Camera Port 2 bus
> + * @V4L2_FWNODE_BUS_TYPE_CSI2_DPHY: MIPI CSI-2 bus, D-PHY physical layer
> + * @V4L2_FWNODE_BUS_TYPE_PARALLEL: Camera Parallel Interface bus
> + * @V4L2_FWNODE_BUS_TYPE_BT656: BT656 video format bus-type

s/BT656 video/BT.656 video/

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

> + * @NR_OF_V4L2_FWNODE_BUS_TYPE: Number of bus-types
> + */
> +enum v4l2_fwnode_bus_type {
> +	V4L2_FWNODE_BUS_TYPE_GUESS = 0,
> +	V4L2_FWNODE_BUS_TYPE_CSI2_CPHY,
> +	V4L2_FWNODE_BUS_TYPE_CSI1,
> +	V4L2_FWNODE_BUS_TYPE_CCP2,
> +	V4L2_FWNODE_BUS_TYPE_CSI2_DPHY,
> +	V4L2_FWNODE_BUS_TYPE_PARALLEL,
> +	V4L2_FWNODE_BUS_TYPE_BT656,
> +	NR_OF_V4L2_FWNODE_BUS_TYPE,
> +};
> +
>  /**
>   * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
>   * @fwnode: pointer to the endpoint's fwnode handle

-- 
Regards,

Laurent Pinchart

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

* Re: [PATCH v3 07/14] software_node: Add support for fwnode_graph*() family of functions
  2020-12-24 12:55     ` Laurent Pinchart
@ 2020-12-24 13:03       ` Andy Shevchenko
  2020-12-24 14:21         ` Daniel Scally
  0 siblings, 1 reply; 58+ messages in thread
From: Andy Shevchenko @ 2020-12-24 13:03 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Daniel Scally, Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Sakari Ailus, Bingbu Cao,
	Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
	Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij

On Thu, Dec 24, 2020 at 2:55 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Thu, Dec 24, 2020 at 02:24:12PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally wrote:

...

> > > +               if (!strncmp(to_swnode(port)->node->name, "port@",
> >
> > You may use here corresponding _FMT macro.
> >
> > > +                            FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
> > > +                       return port;

...

> > > +       /* Ports have naming style "port@n", we need to select the n */
> >
> > > +       ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
> >
> > Maybe a temporary variable?
> >
> >   unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN;
> >   ...
> >   ret = kstrtou32(swnode->parent->node->name + prefix_len,
>
> Honestly I'm wondering if those macros don't hinder readability. I'd
> rather write
>
>         + strlen("port@")

Works for me, since the compiler optimizes this away to be a plain constant.

> and let the compiler optimize this to a compile-time constant.
>
> > > +                       10, &endpoint->port);
> > > +       if (ret)
> > > +               return ret;


-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v3 05/14] software_node: unregister software_nodes in reverse order
  2020-12-24 12:13   ` Andy Shevchenko
@ 2020-12-24 14:00     ` Daniel Scally
  2020-12-24 14:12       ` Andy Shevchenko
  0 siblings, 1 reply; 58+ messages in thread
From: Daniel Scally @ 2020-12-24 14:00 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Sakari Ailus, Bingbu Cao,
	Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
	Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, kernel test robot, Dan Carpenter,
	Laurent Pinchart

On 24/12/2020 12:13, Andy Shevchenko wrote:
> On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally <djrscally@gmail.com> wrote:
>>
>> To maintain consistency with software_node_unregister_nodes(), reverse
>> the order in which the software_node_unregister_node_group() function
>> unregisters nodes.
> 
> ...
> 
>> - * Unregister multiple software nodes at once.
>> + * Unregister multiple software nodes at once. The array will be unwound in
>> + * reverse order (i.e. last entry first) and thus if any member of the array
>> + * has its .parent member set then they should appear later in the array such
>> + * that they are unregistered first.
> 
> I'm, as being not a native speaker, a bit confused by this comment.
> The idea is that children are unregistered first. Can you try to make
> it more clear maybe?

Sure, how about:

The array will be unwound in reverse order (i.e. last entry first). If
any member of the array is a child of another member then the child must
appear later in the array than their parent, so that they are
unregistered first.

?

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

* Re: [PATCH v3 05/14] software_node: unregister software_nodes in reverse order
  2020-12-24 14:00     ` Daniel Scally
@ 2020-12-24 14:12       ` Andy Shevchenko
  2020-12-24 14:14         ` Daniel Scally
  0 siblings, 1 reply; 58+ messages in thread
From: Andy Shevchenko @ 2020-12-24 14:12 UTC (permalink / raw)
  To: Daniel Scally
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Sakari Ailus, Bingbu Cao,
	Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
	Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, kernel test robot, Dan Carpenter,
	Laurent Pinchart

On Thu, Dec 24, 2020 at 4:00 PM Daniel Scally <djrscally@gmail.com> wrote:
> On 24/12/2020 12:13, Andy Shevchenko wrote:
> > On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally <djrscally@gmail.com> wrote:

...

> >> + * Unregister multiple software nodes at once. The array will be unwound in
> >> + * reverse order (i.e. last entry first) and thus if any member of the array
> >> + * has its .parent member set then they should appear later in the array such
> >> + * that they are unregistered first.
> >
> > I'm, as being not a native speaker, a bit confused by this comment.
> > The idea is that children are unregistered first. Can you try to make
> > it more clear maybe?
>
> Sure, how about:
>
> The array will be unwound in reverse order (i.e. last entry first). If
> any member of the array is a child of another member then the child must

children ?

> appear later in the array than their parent, so that they are
> unregistered first.

I think with the above change it will be better, yes.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v3 05/14] software_node: unregister software_nodes in reverse order
  2020-12-24 14:12       ` Andy Shevchenko
@ 2020-12-24 14:14         ` Daniel Scally
  2020-12-24 18:36           ` David Laight
  0 siblings, 1 reply; 58+ messages in thread
From: Daniel Scally @ 2020-12-24 14:14 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Sakari Ailus, Bingbu Cao,
	Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
	Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, kernel test robot, Dan Carpenter,
	Laurent Pinchart


On 24/12/2020 14:12, Andy Shevchenko wrote:
> On Thu, Dec 24, 2020 at 4:00 PM Daniel Scally <djrscally@gmail.com> wrote:
>> On 24/12/2020 12:13, Andy Shevchenko wrote:
>>> On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally <djrscally@gmail.com> wrote:
> ...
>
>>>> + * Unregister multiple software nodes at once. The array will be unwound in
>>>> + * reverse order (i.e. last entry first) and thus if any member of the array
>>>> + * has its .parent member set then they should appear later in the array such
>>>> + * that they are unregistered first.
>>> I'm, as being not a native speaker, a bit confused by this comment.
>>> The idea is that children are unregistered first. Can you try to make
>>> it more clear maybe?
>> Sure, how about:
>>
>> The array will be unwound in reverse order (i.e. last entry first). If
>> any member of the array is a child of another member then the child must
> children ?

Yes, you are right of course.

>
>> appear later in the array than their parent, so that they are
>> unregistered first.
> I think with the above change it will be better, yes.
>
Ok, done.

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

* Re: [PATCH v3 07/14] software_node: Add support for fwnode_graph*() family of functions
  2020-12-24 13:03       ` Andy Shevchenko
@ 2020-12-24 14:21         ` Daniel Scally
  2020-12-28 16:41           ` Sakari Ailus
  0 siblings, 1 reply; 58+ messages in thread
From: Daniel Scally @ 2020-12-24 14:21 UTC (permalink / raw)
  To: Andy Shevchenko, Laurent Pinchart
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Sakari Ailus, Bingbu Cao,
	Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
	Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij

Hi Andy, Laurent

> On Thu, Dec 24, 2020 at 2:55 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> On Thu, Dec 24, 2020 at 02:24:12PM +0200, Andy Shevchenko wrote:
>>> On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally wrote:
> 
> ...
> 
>>>> +               if (!strncmp(to_swnode(port)->node->name, "port@",
>>>
>>> You may use here corresponding _FMT macro.
>>>
>>>> +                            FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
>>>> +                       return port;
> 
> ...
> 
>>>> +       /* Ports have naming style "port@n", we need to select the n */
>>>
>>>> +       ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
>>>
>>> Maybe a temporary variable?
>>>
>>>   unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN;
>>>   ...
>>>   ret = kstrtou32(swnode->parent->node->name + prefix_len,
>>
>> Honestly I'm wondering if those macros don't hinder readability. I'd
>> rather write
>>
>>         + strlen("port@")
> 
> Works for me, since the compiler optimizes this away to be a plain constant.

Well, how about instead of the LEN macro, we have:

#define FWNODE_GRAPH_PORT_NAME_PREFIX	"port@"
#define FWNODE_GRAPH_PORT_NAME_FMT FWNODE_GRAPH_PORT_NAME_PREFIX "%u"

And then it's still maintainable in one place but (I think) slightly
less clunky, since we can do strlen(FWNODE_GRAPH_PORT_NAME_PREFIX)

Or we can do strlen("port@"), I'm good either way :)

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

* Re: [PATCH v3 07/14] software_node: Add support for fwnode_graph*() family of functions
  2020-12-24 12:53   ` Laurent Pinchart
@ 2020-12-24 14:24     ` Daniel Scally
  0 siblings, 0 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-24 14:24 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-kernel, linux-acpi, linux-media, devel, rjw, lenb, gregkh,
	yong.zhi, sakari.ailus, bingbu.cao, tian.shu.qiu, mchehab,
	robert.moore, erik.kaneda, pmladek, rostedt, sergey.senozhatsky,
	andriy.shevchenko, linux, laurent.pinchart+renesas,
	jacopo+renesas, kieran.bingham+renesas, hverkuil-cisco, m.felsch,
	niklas.soderlund+renesas, slongerbeam, heikki.krogerus,
	linus.walleij

On 24/12/2020 12:53, Laurent Pinchart wrote:
>> +	while ((port = software_node_get_next_child(parent, old))) {
>> +		/*
>> +		 * ports have naming style "port@n", so we search for children
>> +		 * that follow that convention (but without assuming anything
>> +		 * about the index number)
>> +		 */
>> +		if (!strncmp(to_swnode(port)->node->name, "port@",
>> +			     FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
> 
> I would either add a macro to replace the prefix ("port@"), or drop
> FWNODE_GRAPH_PORT_NAME_PREFIX_LEN. I think this is the worst of both
> worlds, the string and its length are defined in two different places
> :-)
> 
> I would personally drop the macro, but I don't mind either way as long
> as the string and its length are defined in the same place.

OK, pending outcome of the discussion in the other thread I'll do both
things the same way - whatever the decision there is.


>> +static int
>> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>> +				   struct fwnode_endpoint *endpoint)
>> +{
>> +	struct swnode *swnode = to_swnode(fwnode);
>> +	int ret;
>> +
>> +	/* Ports have naming style "port@n", we need to select the n */
>> +	ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
>> +			10, &endpoint->port);
> 
> Same here.
> 
> I wonder if we should add a check to ensure parent->node->name is long
> enough (and possibly even start with the right prefix), as otherwise the
> pointer passed to kstrtou32() may be past the end of the string. Maybe
> this is overkill, if we can rely on the fact that software nodes have
> correct names.

Not necessarily actually; ports yes but endpoints no, so I think the
danger is there. I'll add the check.

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

Thanks!


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

* RE: [PATCH v3 05/14] software_node: unregister software_nodes in reverse order
  2020-12-24 14:14         ` Daniel Scally
@ 2020-12-24 18:36           ` David Laight
  2020-12-28 10:15             ` Andy Shevchenko
  0 siblings, 1 reply; 58+ messages in thread
From: David Laight @ 2020-12-24 18:36 UTC (permalink / raw)
  To: 'Daniel Scally', Andy Shevchenko
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Sakari Ailus, Bingbu Cao,
	Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
	Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, kernel test robot, Dan Carpenter,
	Laurent Pinchart

From: Daniel Scally 
> Sent: 24 December 2020 14:14
...
> >> The array will be unwound in reverse order (i.e. last entry first). If
> >> any member of the array is a child of another member then the child must
> > children ?
> 
> Yes, you are right of course.

The second 'child' is a back-reference to 'any member' so is singular
so 'child' is correct.
'the child' could be replaced by 'it'

You could have:
   If any members of the array are children of another member then the
   children must appear later in the list.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

* Re: [PATCH v3 13/14] include: media: v4l2-fwnode: Include v4l2_fwnode_bus_type
  2020-12-24 12:32   ` Andy Shevchenko
@ 2020-12-26 23:14     ` Daniel Scally
  0 siblings, 0 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-26 23:14 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Sakari Ailus, Bingbu Cao,
	Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
	Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij

Hi Andy

On 24/12/2020 12:32, Andy Shevchenko wrote:
> On Thu, Dec 24, 2020 at 3:13 AM Daniel Scally <djrscally@gmail.com> wrote:

> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thank you

>> +/**
>> + * enum v4l2_fwnode_bus_type - Video bus types defined by firmware properties
>> + * @V4L2_FWNODE_BUS_TYPE_GUESS: Default value if no bus-type fwnode property
>> + * @V4L2_FWNODE_BUS_TYPE_CSI2_CPHY: MIPI CSI-2 bus, C-PHY physical layer
>> + * @V4L2_FWNODE_BUS_TYPE_CSI1: MIPI CSI-1 bus
>> + * @V4L2_FWNODE_BUS_TYPE_CCP2: SMIA Compact Camera Port 2 bus
>> + * @V4L2_FWNODE_BUS_TYPE_CSI2_DPHY: MIPI CSI-2 bus, D-PHY physical layer
>> + * @V4L2_FWNODE_BUS_TYPE_PARALLEL: Camera Parallel Interface bus
>> + * @V4L2_FWNODE_BUS_TYPE_BT656: BT656 video format bus-type
>> + * @NR_OF_V4L2_FWNODE_BUS_TYPE: Number of bus-types
>> + */
>> +enum v4l2_fwnode_bus_type {
>> +       V4L2_FWNODE_BUS_TYPE_GUESS = 0,
>> +       V4L2_FWNODE_BUS_TYPE_CSI2_CPHY,
>> +       V4L2_FWNODE_BUS_TYPE_CSI1,
>> +       V4L2_FWNODE_BUS_TYPE_CCP2,
>> +       V4L2_FWNODE_BUS_TYPE_CSI2_DPHY,
>> +       V4L2_FWNODE_BUS_TYPE_PARALLEL,
>> +       V4L2_FWNODE_BUS_TYPE_BT656,
> 
>> +       NR_OF_V4L2_FWNODE_BUS_TYPE,
> 
> I see that comma is in the original line, but I think it's a good time
> to remove it from this line. Since it's a terminator line we might
> prevent potential issues during review (by a different diff look) and
> at compile time (if anything comes after it).

Fair enough, I've removed it.


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

* Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2020-12-24 12:54   ` Andy Shevchenko
@ 2020-12-26 23:23     ` Daniel Scally
  2020-12-28 17:05     ` Sakari Ailus
  1 sibling, 0 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-26 23:23 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Sakari Ailus, Bingbu Cao,
	Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
	Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, Jordan Hand, Laurent Pinchart

Hi Andy

On 24/12/2020 12:54, Andy Shevchenko wrote:
> On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally <djrscally@gmail.com> wrote:
>>
>> Currently on platforms designed for Windows, connections between CIO2 and
>> sensors are not properly defined in DSDT. This patch extends the ipu3-cio2
>> driver to compensate by building software_node connections, parsing the
>> connection properties from the sensor's SSDB buffer.
> 
> Few nitpicks below, after addressing them
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thanks, and for all the time you've put into this series

>> +/*
>> + * Extend this array with ACPI Hardware ID's of devices known to be working
> 
> ID's -> IDs ?

Yes, turns out I'm pretty bad at apostrophes.

>> +static void cio2_bridge_create_fwnode_properties(struct cio2_sensor *sensor,
>> +                                                const struct cio2_sensor_config *cfg)
>> +{
>> +       unsigned int i;
>> +
>> +       sensor->prop_names = prop_names;
>> +
>> +       for (i = 0; i < 4; i++)
> 
> 4 here and below, can we have a local define for them, like
> 
>   #define CIO2_MAX_LANES  4

Done and for the other place it's mentioned below.

>> +static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge,
>> +                                      struct pci_dev *cio2)
>> +{
>> +       struct fwnode_handle *fwnode;
>> +       struct cio2_sensor *sensor;
>> +       struct acpi_device *adev;
>> +       unsigned int i;
>> +       int ret = 0;
> 
> You may drop this assignment and...
> 
...
> 
>> +       return ret;
> 
> ...use here
> 
>   return 0;
> 
> directly.

Done

>> +enum cio2_sensor_swnodes {
>> +       SWNODE_SENSOR_HID,
>> +       SWNODE_SENSOR_PORT,
>> +       SWNODE_SENSOR_ENDPOINT,
>> +       SWNODE_CIO2_PORT,
>> +       SWNODE_CIO2_ENDPOINT,
> 
>> +       SWNODE_COUNT,
> 
> No comma?

Done

>> @@ -1715,6 +1732,23 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>>                 return -ENOMEM;
>>         cio2->pci_dev = pci_dev;
>>
>> +       /*
>> +        * On some platforms no connections to sensors are defined in firmware,
>> +        * if the device has no endpoints then we can try to build those as
>> +        * software_nodes parsed from SSDB.
>> +        */
>> +       if (!cio2_check_fwnode_graph(fwnode)) {
>> +               if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) {
> 
>> +                       dev_err(&pci_dev->dev,
>> +                               "fwnode graph has no endpoints connected\n");
> 
> One line?

Done

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

* Re: [PATCH v3 07/14] software_node: Add support for fwnode_graph*() family of functions
  2020-12-24 12:24   ` Andy Shevchenko
  2020-12-24 12:55     ` Laurent Pinchart
@ 2020-12-26 23:47     ` Daniel Scally
  1 sibling, 0 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-26 23:47 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Sakari Ailus, Bingbu Cao,
	Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
	Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij

Hi Andy

On 24/12/2020 12:24, Andy Shevchenko wrote:
> On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally <djrscally@gmail.com> wrote:
>>
>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>
>> This implements the remaining .graph_* callbacks in the
> 
> .graph_* ==> ->graph_*() ?
> 
>> fwnode operations structure for the software nodes. That makes
>> the fwnode_graph*() functions available in the drivers also
> 
> graph*() -> graph_*() ?

Both done.

>> when software nodes are used.
>>
>> The implementation tries to mimic the "OF graph" as much as
>> possible, but there is no support for the "reg" device
>> property. The ports will need to have the index in their
>> name which starts with "port@" (for example "port@0", "port@1",
> 
>> ...)
> 
> Looks not good, perhaps move to the previous line, or move port@1 example here?

Fixed - by expanding the lines to ~75 chars

> Few nitpicks here and there, after addressing them,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thank you!

>> +static struct fwnode_handle *
>> +swnode_graph_find_next_port(const struct fwnode_handle *parent,
>> +                           struct fwnode_handle *port)
>> +{
>> +       struct fwnode_handle *old = port;
>> +
>> +       while ((port = software_node_get_next_child(parent, old))) {
>> +               /*
>> +                * ports have naming style "port@n", so we search for children
>> +                * that follow that convention (but without assuming anything
>> +                * about the index number)
>> +                */
> 
>> +               if (!strncmp(to_swnode(port)->node->name, "port@",
> 
> You may use here corresponding _FMT macro.

>> +static int
>> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>> +                                  struct fwnode_endpoint *endpoint)
>> +{
>> +       struct swnode *swnode = to_swnode(fwnode);
>> +       int ret;
>> +
>> +       /* Ports have naming style "port@n", we need to select the n */
> 
>> +       ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
> 
> Maybe a temporary variable?
> 
>   unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN;
>   ...
>   ret = kstrtou32(swnode->parent->node->name + prefix_len,


Both discussed after Laurent's reply I think.

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

* Re: [PATCH v3 05/14] software_node: unregister software_nodes in reverse order
  2020-12-24 18:36           ` David Laight
@ 2020-12-28 10:15             ` Andy Shevchenko
  2020-12-28 21:17               ` Daniel Scally
  0 siblings, 1 reply; 58+ messages in thread
From: Andy Shevchenko @ 2020-12-28 10:15 UTC (permalink / raw)
  To: David Laight
  Cc: 'Daniel Scally',
	Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Sakari Ailus, Bingbu Cao,
	Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
	Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, kernel test robot, Dan Carpenter,
	Laurent Pinchart

On Thu, Dec 24, 2020 at 06:36:10PM +0000, David Laight wrote:
> From: Daniel Scally 
> > Sent: 24 December 2020 14:14
> ...
> > >> The array will be unwound in reverse order (i.e. last entry first). If
> > >> any member of the array is a child of another member then the child must
> > > children ?
> > 
> > Yes, you are right of course.
> 
> The second 'child' is a back-reference to 'any member' so is singular
> so 'child' is correct.
> 'the child' could be replaced by 'it'
> 
> You could have:
>    If any members of the array are children of another member then the
>    children must appear later in the list.

Works for me!
Dan, can you consider David's proposal?

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v3 06/14] include: fwnode.h: Define format macros for ports and endpoints
  2020-12-24  1:08 ` [PATCH v3 06/14] include: fwnode.h: Define format macros for ports and endpoints Daniel Scally
  2020-12-24 12:17   ` Andy Shevchenko
@ 2020-12-28 16:30   ` Sakari Ailus
  2020-12-28 17:11     ` Sakari Ailus
  1 sibling, 1 reply; 58+ messages in thread
From: Sakari Ailus @ 2020-12-28 16:30 UTC (permalink / raw)
  To: Daniel Scally
  Cc: linux-kernel, linux-acpi, linux-media, devel, rjw, lenb, gregkh,
	yong.zhi, bingbu.cao, tian.shu.qiu, mchehab, robert.moore,
	erik.kaneda, pmladek, rostedt, sergey.senozhatsky,
	andriy.shevchenko, linux, laurent.pinchart+renesas,
	jacopo+renesas, kieran.bingham+renesas, hverkuil-cisco, m.felsch,
	niklas.soderlund+renesas, slongerbeam, heikki.krogerus,
	linus.walleij

Hi Daniel, Andy,

On Thu, Dec 24, 2020 at 01:08:59AM +0000, Daniel Scally wrote:
> OF, ACPI and software_nodes all implement graphs including nodes for ports
> and endpoints. These are all intended to be named with a common schema,
> as "port@n" and "endpoint@n" where n is an unsigned int representing the
> index of the node. To ensure commonality across the subsystems, provide a
> set of macros to define the format.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v3
> 	- Patch introduced
> 
>  include/linux/fwnode.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 9506f8ec0974..52889efceb7d 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -32,6 +32,19 @@ struct fwnode_endpoint {
>  	const struct fwnode_handle *local_fwnode;
>  };
>  
> +/*
> + * ports and endpoints defined in OF, ACPI and as software_nodes should all
> + * follow a common naming scheme; use these macros to ensure commonality across
> + * the subsystems.
> + *
> + * The *PREFIX_LEN macros refer to the length of the "port@" and "endpoint@"
> + * sections of the naming scheme.
> + */
> +#define FWNODE_GRAPH_PORT_NAME_FORMAT		"port@%u"
> +#define FWNODE_GRAPH_PORT_NAME_PREFIX_LEN	5
> +#define FWNODE_GRAPH_ENDPOINT_NAME_FORMAT	"endpoint@%u"
> +#define FWNODE_GRAPH_ENDPOINT_PREFIX_LEN	9
> +
>  #define NR_FWNODE_REFERENCE_ARGS	8

I'd keep such definitions local to the swnode implementation as neither
ACPI nor DT have an apparent need for them. They do use the naming, but
don't appear to format such strings.

-- 
Regards,

Sakari Ailus

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

* Re: [PATCH v3 05/14] software_node: unregister software_nodes in reverse order
  2020-12-24  1:08 ` [PATCH v3 05/14] software_node: unregister software_nodes in reverse order Daniel Scally
  2020-12-24 12:13   ` Andy Shevchenko
@ 2020-12-28 16:34   ` Sakari Ailus
  2020-12-28 17:47     ` Andy Shevchenko
  1 sibling, 1 reply; 58+ messages in thread
From: Sakari Ailus @ 2020-12-28 16:34 UTC (permalink / raw)
  To: Daniel Scally
  Cc: linux-kernel, linux-acpi, linux-media, devel, rjw, lenb, gregkh,
	yong.zhi, bingbu.cao, tian.shu.qiu, mchehab, robert.moore,
	erik.kaneda, pmladek, rostedt, sergey.senozhatsky,
	andriy.shevchenko, linux, laurent.pinchart+renesas,
	jacopo+renesas, kieran.bingham+renesas, hverkuil-cisco, m.felsch,
	niklas.soderlund+renesas, slongerbeam, heikki.krogerus,
	linus.walleij, kernel test robot, Dan Carpenter,
	Laurent Pinchart

Hi Daniel,

On Thu, Dec 24, 2020 at 01:08:58AM +0000, Daniel Scally wrote:
> To maintain consistency with software_node_unregister_nodes(), reverse
> the order in which the software_node_unregister_node_group() function
> unregisters nodes.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v3
> 	- Fixed the dereference of the terminating NULL entry
> 	- Comment cleanup
> 
>  drivers/base/swnode.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index ade49173ff8d..2d07eb04c6c8 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -779,16 +779,22 @@ EXPORT_SYMBOL_GPL(software_node_register_node_group);
>   * software_node_unregister_node_group - Unregister a group of software nodes
>   * @node_group: NULL terminated array of software node pointers to be unregistered
>   *
> - * Unregister multiple software nodes at once.
> + * Unregister multiple software nodes at once. The array will be unwound in
> + * reverse order (i.e. last entry first) and thus if any member of the array
> + * has its .parent member set then they should appear later in the array such
> + * that they are unregistered first.
>   */
>  void software_node_unregister_node_group(const struct software_node **node_group)

With this line wrapped,

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

>  {
> -	unsigned int i;
> +	unsigned int i = 0;
>  
>  	if (!node_group)
>  		return;
>  
> -	for (i = 0; node_group[i]; i++)
> +	while (node_group[i])
> +		i++;
> +
> +	while (i--)
>  		software_node_unregister(node_group[i]);
>  }
>  EXPORT_SYMBOL_GPL(software_node_unregister_node_group);

-- 
Kind regards,

Sakari Ailus

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

* Re: [PATCH v3 07/14] software_node: Add support for fwnode_graph*() family of functions
  2020-12-24 14:21         ` Daniel Scally
@ 2020-12-28 16:41           ` Sakari Ailus
  2020-12-28 21:37             ` Daniel Scally
  0 siblings, 1 reply; 58+ messages in thread
From: Sakari Ailus @ 2020-12-28 16:41 UTC (permalink / raw)
  To: Daniel Scally
  Cc: Andy Shevchenko, Laurent Pinchart, Linux Kernel Mailing List,
	ACPI Devel Maling List, Linux Media Mailing List, devel,
	Rafael J. Wysocki, Len Brown, Greg Kroah-Hartman, Yong Zhi,
	Bingbu Cao, Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore,
	Erik Kaneda, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
	Andy Shevchenko, Rasmus Villemoes, Laurent Pinchart,
	Jacopo Mondi, kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij

Hi Daniel,

On Thu, Dec 24, 2020 at 02:21:15PM +0000, Daniel Scally wrote:
> Hi Andy, Laurent
> 
> > On Thu, Dec 24, 2020 at 2:55 PM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> >> On Thu, Dec 24, 2020 at 02:24:12PM +0200, Andy Shevchenko wrote:
> >>> On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally wrote:
> > 
> > ...
> > 
> >>>> +               if (!strncmp(to_swnode(port)->node->name, "port@",
> >>>
> >>> You may use here corresponding _FMT macro.
> >>>
> >>>> +                            FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
> >>>> +                       return port;
> > 
> > ...
> > 
> >>>> +       /* Ports have naming style "port@n", we need to select the n */
> >>>
> >>>> +       ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
> >>>
> >>> Maybe a temporary variable?
> >>>
> >>>   unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN;
> >>>   ...
> >>>   ret = kstrtou32(swnode->parent->node->name + prefix_len,
> >>
> >> Honestly I'm wondering if those macros don't hinder readability. I'd
> >> rather write
> >>
> >>         + strlen("port@")
> > 
> > Works for me, since the compiler optimizes this away to be a plain constant.
> 
> Well, how about instead of the LEN macro, we have:
> 
> #define FWNODE_GRAPH_PORT_NAME_PREFIX	"port@"
> #define FWNODE_GRAPH_PORT_NAME_FMT FWNODE_GRAPH_PORT_NAME_PREFIX "%u"
> 
> And then it's still maintainable in one place but (I think) slightly
> less clunky, since we can do strlen(FWNODE_GRAPH_PORT_NAME_PREFIX)
> 
> Or we can do strlen("port@"), I'm good either way :)

I'd be in favour of using strlen("port@") here.

At least for now. I think refactoring the use of such strings could be a
separate set at another time, if that's seen as the way to go.

-- 
Kind regards,

Sakari Ailus

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

* Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2020-12-24 12:54   ` Andy Shevchenko
  2020-12-26 23:23     ` Daniel Scally
@ 2020-12-28 17:05     ` Sakari Ailus
  2020-12-28 22:37       ` Daniel Scally
  1 sibling, 1 reply; 58+ messages in thread
From: Sakari Ailus @ 2020-12-28 17:05 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Daniel Scally, Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Bingbu Cao, Tian Shu Qiu,
	Mauro Carvalho Chehab, Robert Moore, Erik Kaneda, Petr Mladek,
	Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, Jordan Hand, Laurent Pinchart

Hi Andy, Daniel,

On Thu, Dec 24, 2020 at 02:54:44PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally <djrscally@gmail.com> wrote:
> >
> > Currently on platforms designed for Windows, connections between CIO2 and
> > sensors are not properly defined in DSDT. This patch extends the ipu3-cio2
> > driver to compensate by building software_node connections, parsing the
> > connection properties from the sensor's SSDB buffer.
> 
> Few nitpicks below, after addressing them
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> > Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Daniel Scally <djrscally@gmail.com>
> > ---
> > Changes in v3
> >         - Used Laurent's suggestion to simplify initing the property names
> >         - Wrapped some lines
> >         - Fixed return and error handling for cio2_bridge_read_acpi_buffer()
> >         - Returned an error if more sensors than available ports are detected
> >         - Used defines for port/endpoint name formats and the bus-type property
> >         - Some bits of cleanup
> >
> >  MAINTAINERS                                   |   1 +
> >  drivers/media/pci/intel/ipu3/Kconfig          |  18 ++
> >  drivers/media/pci/intel/ipu3/Makefile         |   1 +
> >  drivers/media/pci/intel/ipu3/cio2-bridge.c    | 272 ++++++++++++++++++
> >  drivers/media/pci/intel/ipu3/cio2-bridge.h    | 122 ++++++++
> >  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  34 +++
> >  drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   6 +
> >  7 files changed, 454 insertions(+)
> >  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
> >  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 16b544624577..e7784b4bc8ea 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -8943,6 +8943,7 @@ INTEL IPU3 CSI-2 CIO2 DRIVER
> >  M:     Yong Zhi <yong.zhi@intel.com>
> >  M:     Sakari Ailus <sakari.ailus@linux.intel.com>
> >  M:     Bingbu Cao <bingbu.cao@intel.com>
> > +M:     Dan Scally <djrscally@gmail.com>
> >  R:     Tianshu Qiu <tian.shu.qiu@intel.com>
> >  L:     linux-media@vger.kernel.org
> >  S:     Maintained
> > diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig
> > index 82d7f17e6a02..dcf5c4b74673 100644
> > --- a/drivers/media/pci/intel/ipu3/Kconfig
> > +++ b/drivers/media/pci/intel/ipu3/Kconfig
> > @@ -16,3 +16,21 @@ config VIDEO_IPU3_CIO2
> >           Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
> >           connected camera.
> >           The module will be called ipu3-cio2.
> > +
> > +config CIO2_BRIDGE
> > +       bool "IPU3 CIO2 Sensors Bridge"
> > +       depends on VIDEO_IPU3_CIO2
> > +       help
> > +         This extension provides an API for the ipu3-cio2 driver to create
> > +         connections to cameras that are hidden in SSDB buffer in ACPI. It
> 
> in the
> 
> > +         can be used to enable support for cameras in detachable / hybrid
> > +         devices that ship with Windows.
> > +
> > +         Say Y here if your device is a detachable / hybrid laptop that comes
> > +         with Windows installed by the OEM, for example:
> > +
> > +               - Microsoft Surface models (except Surface Pro 3)
> > +               - The Lenovo Miix line (for example the 510, 520, 710 and 720)
> > +               - Dell 7285
> > +
> > +         If in doubt, say N here.
> > diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile
> > index 429d516452e4..933777e6ea8a 100644
> > --- a/drivers/media/pci/intel/ipu3/Makefile
> > +++ b/drivers/media/pci/intel/ipu3/Makefile
> > @@ -2,3 +2,4 @@
> >  obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
> >
> >  ipu3-cio2-y += ipu3-cio2-main.o
> > +ipu3-cio2-$(CONFIG_CIO2_BRIDGE) += cio2-bridge.o
> > diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> > new file mode 100644
> > index 000000000000..3f4ae172fd25
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> > @@ -0,0 +1,272 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Author: Dan Scally <djrscally@gmail.com> */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/device.h>
> > +#include <linux/pci.h>
> > +#include <linux/property.h>
> > +#include <media/v4l2-fwnode.h>
> > +
> > +#include "cio2-bridge.h"
> > +
> > +/*
> > + * Extend this array with ACPI Hardware ID's of devices known to be working
> 
> ID's -> IDs ?
> 
> > + * plus the number of link-frequencies expected by their drivers, along with
> > + * the frequency values in hertz. This is somewhat opportunistic way of adding
> > + * support for this for now in the hopes of a better source for the information
> > + * (possibly some encoded value in the SSDB buffer that we're unaware of)
> > + * becoming apparent in the future.
> > + *
> > + * Do not add an entry for a sensor that is not actually supported.
> > + */
> > +static const struct cio2_sensor_config cio2_supported_sensors[] = {
> > +       CIO2_SENSOR_CONFIG("INT33BE", 0),
> > +       CIO2_SENSOR_CONFIG("OVTI2680", 0),
> > +};
> > +
> > +static const struct cio2_property_names prop_names = {
> > +       .clock_frequency = "clock-frequency",
> > +       .rotation = "rotation",
> > +       .bus_type = "bus-type",
> > +       .data_lanes = "data-lanes",
> > +       .remote_endpoint = "remote-endpoint",
> > +       .link_frequencies = "link-frequencies",
> > +};
> > +
> > +static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
> > +                                       void *data, u32 size)
> > +{
> > +       struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > +       union acpi_object *obj;
> > +       acpi_status status;
> > +       int ret = 0;
> > +
> > +       status = acpi_evaluate_object(adev->handle, id, NULL, &buffer);
> > +       if (ACPI_FAILURE(status))
> > +               return -ENODEV;
> > +
> > +       obj = buffer.pointer;
> > +       if (!obj) {
> > +               dev_err(&adev->dev, "Couldn't locate ACPI buffer\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       if (obj->type != ACPI_TYPE_BUFFER) {
> > +               dev_err(&adev->dev, "Not an ACPI buffer\n");
> > +               ret = -ENODEV;
> > +               goto out_free_buff;
> > +       }
> > +
> > +       if (obj->buffer.length > size) {
> > +               dev_err(&adev->dev, "Given buffer is too small\n");
> > +               ret = -EINVAL;
> > +               goto out_free_buff;
> > +       }
> > +
> > +       memcpy(data, obj->buffer.pointer, obj->buffer.length);
> > +
> > +out_free_buff:
> > +       kfree(buffer.pointer);
> > +       return ret;
> > +}
> > +
> > +static void cio2_bridge_create_fwnode_properties(struct cio2_sensor *sensor,
> > +                                                const struct cio2_sensor_config *cfg)
> > +{
> > +       unsigned int i;
> > +
> > +       sensor->prop_names = prop_names;
> > +
> > +       for (i = 0; i < 4; i++)
> 
> 4 here and below, can we have a local define for them, like
> 
>   #define CIO2_MAX_LANES  4
> 
> > +               sensor->data_lanes[i] = i + 1;
> > +
> > +       sensor->local_ref[0].node = &sensor->swnodes[SWNODE_CIO2_ENDPOINT];
> > +       sensor->remote_ref[0].node = &sensor->swnodes[SWNODE_SENSOR_ENDPOINT];
> > +
> > +       sensor->dev_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.clock_frequency,
> > +                                                      sensor->ssdb.mclkspeed);
> > +       sensor->dev_properties[1] = PROPERTY_ENTRY_U8(sensor->prop_names.rotation,
> > +                                                     sensor->ssdb.degree);
> > +
> > +       sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type,
> > +                                                     V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
> > +       sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
> > +                                                               sensor->data_lanes,
> > +                                                               sensor->ssdb.lanes);
> > +       sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
> > +                                                           sensor->local_ref);
> > +
> > +       if (cfg->nr_link_freqs > 0)
> > +               sensor->ep_properties[3] = PROPERTY_ENTRY_U64_ARRAY_LEN(
> > +                                               sensor->prop_names.link_frequencies,
> > +                                               cfg->link_freqs,
> > +                                               cfg->nr_link_freqs);
> > +
> > +       sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
> > +                                                                 sensor->data_lanes,
> > +                                                                 sensor->ssdb.lanes);
> > +       sensor->cio2_properties[1] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
> > +                                                             sensor->remote_ref);
> > +}
> > +
> > +static void cio2_bridge_init_swnode_names(struct cio2_sensor *sensor)
> > +{
> > +       snprintf(sensor->node_names.remote_port, sizeof(sensor->node_names.remote_port),
> > +                FWNODE_GRAPH_PORT_NAME_FORMAT, sensor->ssdb.link);
> > +       snprintf(sensor->node_names.port, sizeof(sensor->node_names.port),
> > +                FWNODE_GRAPH_PORT_NAME_FORMAT, 0); /* Always port 0 */
> > +       snprintf(sensor->node_names.endpoint, sizeof(sensor->node_names.endpoint),
> > +                FWNODE_GRAPH_ENDPOINT_NAME_FORMAT, 0); /* And endpoint 0 */

Please wrap before 80, there's no need here to do otherwise. You could
argue about cio2_bridge_create_fwnode_properties() though. I might just
wrap that, too.

Applies to the rest of the patch.

> > +}
> > +
> > +static void cio2_bridge_create_connection_swnodes(struct cio2_bridge *bridge,
> > +                                                 struct cio2_sensor *sensor)
> > +{
> > +       struct software_node *nodes = sensor->swnodes;
> > +
> > +       cio2_bridge_init_swnode_names(sensor);
> > +
> > +       nodes[SWNODE_SENSOR_HID] = NODE_SENSOR(sensor->name,
> > +                                              sensor->dev_properties);
> > +       nodes[SWNODE_SENSOR_PORT] = NODE_PORT(sensor->node_names.port,
> > +                                             &nodes[SWNODE_SENSOR_HID]);
> > +       nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
> > +                                                     &nodes[SWNODE_SENSOR_PORT],
> > +                                                     sensor->ep_properties);
> > +       nodes[SWNODE_CIO2_PORT] = NODE_PORT(sensor->node_names.remote_port,
> > +                                           &bridge->cio2_hid_node);
> > +       nodes[SWNODE_CIO2_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
> > +                                                   &nodes[SWNODE_CIO2_PORT],
> > +                                                   sensor->cio2_properties);
> > +}
> > +
> > +static void cio2_bridge_unregister_sensors(struct cio2_bridge *bridge)
> > +{
> > +       struct cio2_sensor *sensor;
> > +       unsigned int i;
> > +
> > +       for (i = 0; i < bridge->n_sensors; i++) {
> > +               sensor = &bridge->sensors[i];
> > +               software_node_unregister_nodes(sensor->swnodes);
> > +               acpi_dev_put(sensor->adev);
> > +       }
> > +}
> > +
> > +static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge,
> > +                                      struct pci_dev *cio2)
> > +{
> > +       struct fwnode_handle *fwnode;
> > +       struct cio2_sensor *sensor;
> > +       struct acpi_device *adev;
> > +       unsigned int i;
> > +       int ret = 0;
> 
> You may drop this assignment and...
> 
> > +       for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
> > +               const struct cio2_sensor_config *cfg = &cio2_supported_sensors[i];

You could move the inner loop into a new function called e.g.
cio2_bridge_connect_sensor.

> > +
> > +               for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> > +                       if (bridge->n_sensors >= CIO2_NUM_PORTS) {
> > +                               dev_err(&cio2->dev, "Exceeded available CIO2 ports\n");
> > +                               cio2_bridge_unregister_sensors(bridge);
> > +                               ret = -EINVAL;
> > +                               goto err_out;
> > +                       }
> > +
> > +                       if (!adev->status.enabled)
> > +                               continue;
> > +
> > +                       sensor = &bridge->sensors[bridge->n_sensors];
> > +                       sensor->adev = adev;
> > +                       strscpy(sensor->name, cfg->hid, sizeof(sensor->name));
> > +
> > +                       ret = cio2_bridge_read_acpi_buffer(adev, "SSDB",
> > +                                                          &sensor->ssdb,
> > +                                                          sizeof(sensor->ssdb));
> > +                       if (ret)
> > +                               goto err_put_adev;
> > +
> > +                       if (sensor->ssdb.lanes > 4) {
> 
> CIO2_MAX_LANES
> 
> > +                               dev_err(&adev->dev,
> > +                                       "Number of lanes in SSDB is invalid\n");
> > +                               ret = -EINVAL;
> > +                               goto err_put_adev;
> > +                       }
> > +
> > +                       cio2_bridge_create_fwnode_properties(sensor, cfg);
> > +                       cio2_bridge_create_connection_swnodes(bridge, sensor);
> > +
> > +                       ret = software_node_register_nodes(sensor->swnodes);
> > +                       if (ret)
> > +                               goto err_put_adev;
> > +
> > +                       fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_SENSOR_HID]);
> > +                       if (!fwnode) {
> > +                               ret = -ENODEV;
> > +                               goto err_free_swnodes;
> > +                       }
> > +
> > +                       adev->fwnode.secondary = fwnode;
> > +
> > +                       dev_info(&cio2->dev, "Found supported sensor %s\n",
> > +                                acpi_dev_name(adev));
> > +
> > +                       bridge->n_sensors++;
> > +               }
> > +       }
> 
> > +       return ret;
> 
> ...use here
> 
>   return 0;
> 
> directly.
> 
> > +err_free_swnodes:
> > +       software_node_unregister_nodes(sensor->swnodes);
> > +err_put_adev:
> > +       acpi_dev_put(sensor->adev);
> > +err_out:
> > +       return ret;
> > +}
> > +
> > +int cio2_bridge_init(struct pci_dev *cio2)
> > +{
> > +       struct device *dev = &cio2->dev;
> > +       struct fwnode_handle *fwnode;
> > +       struct cio2_bridge *bridge;
> > +       int ret;
> > +
> > +       bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> > +       if (!bridge)
> > +               return -ENOMEM;
> > +
> > +       strscpy(bridge->cio2_node_name, CIO2_HID, sizeof(bridge->cio2_node_name));
> > +       bridge->cio2_hid_node.name = bridge->cio2_node_name;
> > +
> > +       ret = software_node_register(&bridge->cio2_hid_node);
> > +       if (ret < 0) {
> > +               dev_err(dev, "Failed to register the CIO2 HID node\n");
> > +               goto err_free_bridge;
> > +       }
> > +
> > +       ret = cio2_bridge_connect_sensors(bridge, cio2);
> > +       if (ret || bridge->n_sensors == 0)
> > +               goto err_unregister_cio2;
> > +
> > +       dev_info(dev, "Connected %d cameras\n", bridge->n_sensors);
> > +
> > +       fwnode = software_node_fwnode(&bridge->cio2_hid_node);
> > +       if (!fwnode) {
> > +               dev_err(dev, "Error getting fwnode from cio2 software_node\n");
> > +               ret = -ENODEV;
> > +               goto err_unregister_sensors;
> > +       }
> > +
> > +       set_secondary_fwnode(dev, fwnode);
> > +
> > +       return 0;
> > +
> > +err_unregister_sensors:
> > +       cio2_bridge_unregister_sensors(bridge);
> > +err_unregister_cio2:
> > +       software_node_unregister(&bridge->cio2_hid_node);
> > +err_free_bridge:
> > +       kfree(bridge);
> > +
> > +       return ret;
> > +}
> > diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.h b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> > new file mode 100644
> > index 000000000000..004b608f322f
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> > @@ -0,0 +1,122 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Author: Dan Scally <djrscally@gmail.com> */
> > +#ifndef __CIO2_BRIDGE_H
> > +#define __CIO2_BRIDGE_H
> > +
> > +#include <linux/property.h>
> > +
> > +#define CIO2_HID                               "INT343E"
> > +#define CIO2_NUM_PORTS                         4

This is already defined in ipu3-cio2.h. Could you include that instead?

> > +#define MAX_NUM_LINK_FREQS                     3
> > +
> > +#define CIO2_SENSOR_CONFIG(_HID, _NR, ...)     \
> > +       {                                       \
> > +               .hid = _HID,                    \
> > +               .nr_link_freqs = _NR,           \
> > +               .link_freqs = { __VA_ARGS__ }   \
> > +       }
> > +
> > +#define NODE_SENSOR(_HID, _PROPS)              \
> > +       ((const struct software_node) {         \
> > +               .name = _HID,                   \
> > +               .properties = _PROPS,           \
> > +       })
> > +
> > +#define NODE_PORT(_PORT, _SENSOR_NODE)         \
> > +       ((const struct software_node) {         \
> > +               _PORT,                          \
> > +               _SENSOR_NODE,                   \

Could you use explicit assignments to fields here, please?

> > +       })
> > +
> > +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)      \
> > +       ((const struct software_node) {         \
> > +               _EP,                            \
> > +               _PORT,                          \
> > +               _PROPS,                         \

Ditto.

> > +       })
> > +
> > +enum cio2_sensor_swnodes {
> > +       SWNODE_SENSOR_HID,
> > +       SWNODE_SENSOR_PORT,
> > +       SWNODE_SENSOR_ENDPOINT,
> > +       SWNODE_CIO2_PORT,
> > +       SWNODE_CIO2_ENDPOINT,
> 
> > +       SWNODE_COUNT,
> 
> No comma?
> 
> > +};
> > +
> > +/* Data representation as it is in ACPI SSDB buffer */
> > +struct cio2_sensor_ssdb {
> > +       u8 version;
> > +       u8 sku;
> > +       u8 guid_csi2[16];
> > +       u8 devfunction;
> > +       u8 bus;
> > +       u32 dphylinkenfuses;
> > +       u32 clockdiv;
> > +       u8 link;
> > +       u8 lanes;
> > +       u32 csiparams[10];
> > +       u32 maxlanespeed;
> > +       u8 sensorcalibfileidx;
> > +       u8 sensorcalibfileidxInMBZ[3];
> > +       u8 romtype;
> > +       u8 vcmtype;
> > +       u8 platforminfo;
> > +       u8 platformsubinfo;
> > +       u8 flash;
> > +       u8 privacyled;
> > +       u8 degree;
> > +       u8 mipilinkdefined;
> > +       u32 mclkspeed;
> > +       u8 controllogicid;
> > +       u8 reserved1[3];
> > +       u8 mclkport;
> > +       u8 reserved2[13];
> > +} __packed;
> > +
> > +struct cio2_property_names {
> > +       char clock_frequency[16];
> > +       char rotation[9];
> > +       char bus_type[9];
> > +       char data_lanes[11];
> > +       char remote_endpoint[16];
> > +       char link_frequencies[17];
> > +};
> > +
> > +struct cio2_node_names {
> > +       char port[7];
> > +       char endpoint[11];
> > +       char remote_port[7];
> > +};
> > +
> > +struct cio2_sensor_config {
> > +       const char *hid;
> > +       const u8 nr_link_freqs;
> > +       const u64 link_freqs[MAX_NUM_LINK_FREQS];
> > +};
> > +
> > +struct cio2_sensor {
> > +       char name[ACPI_ID_LEN];
> > +       struct acpi_device *adev;
> > +
> > +       struct software_node swnodes[6];
> > +       struct cio2_node_names node_names;
> > +
> > +       u32 data_lanes[4];
> > +       struct cio2_sensor_ssdb ssdb;
> > +       struct cio2_property_names prop_names;
> > +       struct property_entry ep_properties[5];
> > +       struct property_entry dev_properties[3];
> > +       struct property_entry cio2_properties[3];
> > +       struct software_node_ref_args local_ref[1];
> > +       struct software_node_ref_args remote_ref[1];
> > +};
> > +
> > +struct cio2_bridge {
> > +       char cio2_node_name[ACPI_ID_LEN];
> > +       struct software_node cio2_hid_node;
> > +       unsigned int n_sensors;
> > +       struct cio2_sensor sensors[CIO2_NUM_PORTS];
> > +};
> > +
> > +#endif
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> > index 36e354ecf71e..68ff28abc6a3 100644
> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> > @@ -1702,11 +1702,28 @@ static void cio2_queues_exit(struct cio2_device *cio2)
> >                 cio2_queue_exit(cio2, &cio2->queue[i]);
> >  }
> >
> > +static bool cio2_check_fwnode_graph(struct fwnode_handle *fwnode)
> > +{
> > +       struct fwnode_handle *endpoint;
> > +
> > +       if (IS_ERR_OR_NULL(fwnode))
> > +               return false;
> > +
> > +       endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > +       if (endpoint) {
> > +               fwnode_handle_put(endpoint);
> > +               return true;
> > +       }
> > +
> > +       return cio2_check_fwnode_graph(fwnode->secondary);
> > +}
> > +
> >  /**************** PCI interface ****************/
> >
> >  static int cio2_pci_probe(struct pci_dev *pci_dev,
> >                           const struct pci_device_id *id)
> >  {
> > +       struct fwnode_handle *fwnode = dev_fwnode(&pci_dev->dev);
> >         struct cio2_device *cio2;
> >         int r;
> >
> > @@ -1715,6 +1732,23 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
> >                 return -ENOMEM;
> >         cio2->pci_dev = pci_dev;
> >
> > +       /*
> > +        * On some platforms no connections to sensors are defined in firmware,
> > +        * if the device has no endpoints then we can try to build those as
> > +        * software_nodes parsed from SSDB.
> > +        */
> > +       if (!cio2_check_fwnode_graph(fwnode)) {
> > +               if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) {
> 
> > +                       dev_err(&pci_dev->dev,
> > +                               "fwnode graph has no endpoints connected\n");
> 
> One line?

Hmm. That'll be over 80 anyway, but now it's less so.

-- 
Kind regards,

Sakari Ailus

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

* Re: [PATCH v3 06/14] include: fwnode.h: Define format macros for ports and endpoints
  2020-12-28 16:30   ` Sakari Ailus
@ 2020-12-28 17:11     ` Sakari Ailus
  2020-12-28 21:36       ` Daniel Scally
  0 siblings, 1 reply; 58+ messages in thread
From: Sakari Ailus @ 2020-12-28 17:11 UTC (permalink / raw)
  To: Daniel Scally
  Cc: linux-kernel, linux-acpi, linux-media, devel, rjw, lenb, gregkh,
	yong.zhi, bingbu.cao, tian.shu.qiu, mchehab, robert.moore,
	erik.kaneda, pmladek, rostedt, sergey.senozhatsky,
	andriy.shevchenko, linux, laurent.pinchart+renesas,
	jacopo+renesas, kieran.bingham+renesas, hverkuil-cisco, m.felsch,
	niklas.soderlund+renesas, slongerbeam, heikki.krogerus,
	linus.walleij

On Mon, Dec 28, 2020 at 06:30:24PM +0200, Sakari Ailus wrote:
> Hi Daniel, Andy,
> 
> On Thu, Dec 24, 2020 at 01:08:59AM +0000, Daniel Scally wrote:
> > OF, ACPI and software_nodes all implement graphs including nodes for ports
> > and endpoints. These are all intended to be named with a common schema,
> > as "port@n" and "endpoint@n" where n is an unsigned int representing the
> > index of the node. To ensure commonality across the subsystems, provide a
> > set of macros to define the format.
> > 
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Daniel Scally <djrscally@gmail.com>
> > ---
> > Changes in v3
> > 	- Patch introduced
> > 
> >  include/linux/fwnode.h | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> > index 9506f8ec0974..52889efceb7d 100644
> > --- a/include/linux/fwnode.h
> > +++ b/include/linux/fwnode.h
> > @@ -32,6 +32,19 @@ struct fwnode_endpoint {
> >  	const struct fwnode_handle *local_fwnode;
> >  };
> >  
> > +/*
> > + * ports and endpoints defined in OF, ACPI and as software_nodes should all
> > + * follow a common naming scheme; use these macros to ensure commonality across
> > + * the subsystems.
> > + *
> > + * The *PREFIX_LEN macros refer to the length of the "port@" and "endpoint@"
> > + * sections of the naming scheme.
> > + */
> > +#define FWNODE_GRAPH_PORT_NAME_FORMAT		"port@%u"
> > +#define FWNODE_GRAPH_PORT_NAME_PREFIX_LEN	5
> > +#define FWNODE_GRAPH_ENDPOINT_NAME_FORMAT	"endpoint@%u"
> > +#define FWNODE_GRAPH_ENDPOINT_PREFIX_LEN	9
> > +
> >  #define NR_FWNODE_REFERENCE_ARGS	8
> 
> I'd keep such definitions local to the swnode implementation as neither
> ACPI nor DT have an apparent need for them. They do use the naming, but
> don't appear to format such strings.

Ah, I noticed these are used by the later patches. Please ignore that
comment then.

But these are still definitions, so I'd use SWNODE prefix rather than
FWNODE. That should also be reflected in the comment.

ACPI does not have a format port concept so the definitions that are being
used can be said to be specific to Linux.

-- 
Kind regards,

Sakari Ailus

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

* Re: [PATCH v3 05/14] software_node: unregister software_nodes in reverse order
  2020-12-28 16:34   ` Sakari Ailus
@ 2020-12-28 17:47     ` Andy Shevchenko
  0 siblings, 0 replies; 58+ messages in thread
From: Andy Shevchenko @ 2020-12-28 17:47 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Daniel Scally, linux-kernel, linux-acpi, linux-media, devel, rjw,
	lenb, gregkh, yong.zhi, bingbu.cao, tian.shu.qiu, mchehab,
	robert.moore, erik.kaneda, pmladek, rostedt, sergey.senozhatsky,
	linux, laurent.pinchart+renesas, jacopo+renesas,
	kieran.bingham+renesas, hverkuil-cisco, m.felsch,
	niklas.soderlund+renesas, slongerbeam, heikki.krogerus,
	linus.walleij, kernel test robot, Dan Carpenter,
	Laurent Pinchart

On Mon, Dec 28, 2020 at 06:34:10PM +0200, Sakari Ailus wrote:
> On Thu, Dec 24, 2020 at 01:08:58AM +0000, Daniel Scally wrote:

...

> >  void software_node_unregister_node_group(const struct software_node **node_group)
> 
> With this line wrapped,

Why? It's only one character behind 80 and wrapping it will decrease
readability. Moreover, documentation has explicit exceptions for such cases.


-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v3 05/14] software_node: unregister software_nodes in reverse order
  2020-12-28 10:15             ` Andy Shevchenko
@ 2020-12-28 21:17               ` Daniel Scally
  0 siblings, 0 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-28 21:17 UTC (permalink / raw)
  To: Andy Shevchenko, David Laight
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Sakari Ailus, Bingbu Cao,
	Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
	Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, kernel test robot, Dan Carpenter,
	Laurent Pinchart



On 28/12/2020 10:15, Andy Shevchenko wrote:
> On Thu, Dec 24, 2020 at 06:36:10PM +0000, David Laight wrote:
>> From: Daniel Scally 
>>> Sent: 24 December 2020 14:14
>> ...
>>>>> The array will be unwound in reverse order (i.e. last entry first). If
>>>>> any member of the array is a child of another member then the child must
>>>> children ?
>>>
>>> Yes, you are right of course.
>>
>> The second 'child' is a back-reference to 'any member' so is singular
>> so 'child' is correct.
>> 'the child' could be replaced by 'it'
>>
>> You could have:
>>    If any members of the array are children of another member then the
>>    children must appear later in the list.
> 
> Works for me!
> Dan, can you consider David's proposal?

Yep - done, thanks David

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

* Re: [PATCH v3 06/14] include: fwnode.h: Define format macros for ports and endpoints
  2020-12-28 17:11     ` Sakari Ailus
@ 2020-12-28 21:36       ` Daniel Scally
  0 siblings, 0 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-28 21:36 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-kernel, linux-acpi, linux-media, devel, rjw, lenb, gregkh,
	yong.zhi, bingbu.cao, tian.shu.qiu, mchehab, robert.moore,
	erik.kaneda, pmladek, rostedt, sergey.senozhatsky,
	andriy.shevchenko, linux, laurent.pinchart+renesas,
	jacopo+renesas, kieran.bingham+renesas, hverkuil-cisco, m.felsch,
	niklas.soderlund+renesas, slongerbeam, heikki.krogerus,
	linus.walleij

Hi Sakari

On 28/12/2020 17:11, Sakari Ailus wrote:
> On Mon, Dec 28, 2020 at 06:30:24PM +0200, Sakari Ailus wrote:
>> Hi Daniel, Andy,
>>
>> On Thu, Dec 24, 2020 at 01:08:59AM +0000, Daniel Scally wrote:
>>> OF, ACPI and software_nodes all implement graphs including nodes for ports
>>> and endpoints. These are all intended to be named with a common schema,
>>> as "port@n" and "endpoint@n" where n is an unsigned int representing the
>>> index of the node. To ensure commonality across the subsystems, provide a
>>> set of macros to define the format.
>>>
>>> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>> ---
>>> Changes in v3
>>> 	- Patch introduced
>>>
>>>  include/linux/fwnode.h | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
>>> index 9506f8ec0974..52889efceb7d 100644
>>> --- a/include/linux/fwnode.h
>>> +++ b/include/linux/fwnode.h
>>> @@ -32,6 +32,19 @@ struct fwnode_endpoint {
>>>  	const struct fwnode_handle *local_fwnode;
>>>  };
>>>  
>>> +/*
>>> + * ports and endpoints defined in OF, ACPI and as software_nodes should all
>>> + * follow a common naming scheme; use these macros to ensure commonality across
>>> + * the subsystems.
>>> + *
>>> + * The *PREFIX_LEN macros refer to the length of the "port@" and "endpoint@"
>>> + * sections of the naming scheme.
>>> + */
>>> +#define FWNODE_GRAPH_PORT_NAME_FORMAT		"port@%u"
>>> +#define FWNODE_GRAPH_PORT_NAME_PREFIX_LEN	5
>>> +#define FWNODE_GRAPH_ENDPOINT_NAME_FORMAT	"endpoint@%u"
>>> +#define FWNODE_GRAPH_ENDPOINT_PREFIX_LEN	9
>>> +
>>>  #define NR_FWNODE_REFERENCE_ARGS	8
>>
>> I'd keep such definitions local to the swnode implementation as neither
>> ACPI nor DT have an apparent need for them. They do use the naming, but
>> don't appear to format such strings.
> 
> Ah, I noticed these are used by the later patches. Please ignore that
> comment then.
> 
> But these are still definitions, so I'd use SWNODE prefix rather than
> FWNODE. That should also be reflected in the comment.

Fine by me; I shall change that.

> ACPI does not have a format port concept so the definitions that are being
> used can be said to be specific to Linux.
> 

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

* Re: [PATCH v3 07/14] software_node: Add support for fwnode_graph*() family of functions
  2020-12-28 16:41           ` Sakari Ailus
@ 2020-12-28 21:37             ` Daniel Scally
  0 siblings, 0 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-28 21:37 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Andy Shevchenko, Laurent Pinchart, Linux Kernel Mailing List,
	ACPI Devel Maling List, Linux Media Mailing List, devel,
	Rafael J. Wysocki, Len Brown, Greg Kroah-Hartman, Yong Zhi,
	Bingbu Cao, Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore,
	Erik Kaneda, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
	Andy Shevchenko, Rasmus Villemoes, Laurent Pinchart,
	Jacopo Mondi, kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij

Hi Sakari

On 28/12/2020 16:41, Sakari Ailus wrote:
> Hi Daniel,
> 
> On Thu, Dec 24, 2020 at 02:21:15PM +0000, Daniel Scally wrote:
>> Hi Andy, Laurent
>>
>>> On Thu, Dec 24, 2020 at 2:55 PM Laurent Pinchart
>>> <laurent.pinchart@ideasonboard.com> wrote:
>>>> On Thu, Dec 24, 2020 at 02:24:12PM +0200, Andy Shevchenko wrote:
>>>>> On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally wrote:
>>>
>>> ...
>>>
>>>>>> +               if (!strncmp(to_swnode(port)->node->name, "port@",
>>>>>
>>>>> You may use here corresponding _FMT macro.
>>>>>
>>>>>> +                            FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
>>>>>> +                       return port;
>>>
>>> ...
>>>
>>>>>> +       /* Ports have naming style "port@n", we need to select the n */
>>>>>
>>>>>> +       ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
>>>>>
>>>>> Maybe a temporary variable?
>>>>>
>>>>>   unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN;
>>>>>   ...
>>>>>   ret = kstrtou32(swnode->parent->node->name + prefix_len,
>>>>
>>>> Honestly I'm wondering if those macros don't hinder readability. I'd
>>>> rather write
>>>>
>>>>         + strlen("port@")
>>>
>>> Works for me, since the compiler optimizes this away to be a plain constant.
>>
>> Well, how about instead of the LEN macro, we have:
>>
>> #define FWNODE_GRAPH_PORT_NAME_PREFIX	"port@"
>> #define FWNODE_GRAPH_PORT_NAME_FMT FWNODE_GRAPH_PORT_NAME_PREFIX "%u"
>>
>> And then it's still maintainable in one place but (I think) slightly
>> less clunky, since we can do strlen(FWNODE_GRAPH_PORT_NAME_PREFIX)
>>
>> Or we can do strlen("port@"), I'm good either way :)
> 
> I'd be in favour of using strlen("port@") here.
> 
> At least for now. I think refactoring the use of such strings could be a
> separate set at another time, if that's seen as the way to go.

Alright - thanks. In that case I'll switch to strlen("port@0"), and
FWNODE_GRAPH_PORT_NAME_LEN can be dropped then I think.

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

* Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2020-12-28 17:05     ` Sakari Ailus
@ 2020-12-28 22:37       ` Daniel Scally
  2020-12-28 22:55         ` Andy Shevchenko
  2021-01-02 17:07         ` Sakari Ailus
  0 siblings, 2 replies; 58+ messages in thread
From: Daniel Scally @ 2020-12-28 22:37 UTC (permalink / raw)
  To: Sakari Ailus, Andy Shevchenko
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Bingbu Cao, Tian Shu Qiu,
	Mauro Carvalho Chehab, Robert Moore, Erik Kaneda, Petr Mladek,
	Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, Jordan Hand, Laurent Pinchart

Hi Sakari, thanks for comments

On 28/12/2020 17:05, Sakari Ailus wrote:
> Hi Andy, Daniel,
> 
> On Thu, Dec 24, 2020 at 02:54:44PM +0200, Andy Shevchenko wrote:
>>> +static void cio2_bridge_init_swnode_names(struct cio2_sensor *sensor)
>>> +{
>>> +       snprintf(sensor->node_names.remote_port, sizeof(sensor->node_names.remote_port),
>>> +                FWNODE_GRAPH_PORT_NAME_FORMAT, sensor->ssdb.link);
>>> +       snprintf(sensor->node_names.port, sizeof(sensor->node_names.port),
>>> +                FWNODE_GRAPH_PORT_NAME_FORMAT, 0); /* Always port 0 */
>>> +       snprintf(sensor->node_names.endpoint, sizeof(sensor->node_names.endpoint),
>>> +                FWNODE_GRAPH_ENDPOINT_NAME_FORMAT, 0); /* And endpoint 0 */
> 
> Please wrap before 80, there's no need here to do otherwise. You could
> argue about cio2_bridge_create_fwnode_properties() though. I might just
> wrap that, too.
> 
> Applies to the rest of the patch.

I shall wrap such cases then - I thought I read somewhere that the
wrapped line needed to be shorter than the parent which is why I wrapped
after 80...but I can't find the reference now so possibly I imagined that.

>>> +static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge,
>>> +                                      struct pci_dev *cio2)
>>> +{
>>> +       struct fwnode_handle *fwnode;
>>> +       struct cio2_sensor *sensor;
>>> +       struct acpi_device *adev;
>>> +       unsigned int i;
>>> +       int ret = 0;
>>
>> You may drop this assignment and...
>>
>>> +       for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
>>> +               const struct cio2_sensor_config *cfg = &cio2_supported_sensors[i];
> 
> You could move the inner loop into a new function called e.g.
> cio2_bridge_connect_sensor.

Yeah good idea, I'll do that.

>>> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.h b/drivers/media/pci/intel/ipu3/cio2-bridge.h
>>> new file mode 100644
>>> index 000000000000..004b608f322f
>>> --- /dev/null
>>> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
>>> @@ -0,0 +1,122 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/* Author: Dan Scally <djrscally@gmail.com> */
>>> +#ifndef __CIO2_BRIDGE_H
>>> +#define __CIO2_BRIDGE_H
>>> +
>>> +#include <linux/property.h>
>>> +
>>> +#define CIO2_HID                               "INT343E"
>>> +#define CIO2_NUM_PORTS                         4
> 
> This is already defined in ipu3-cio2.h. Could you include that instead?

Yes; but I'd need to also include media/v4l2-device.h and
media/videobuf2-dma-sg.h (they're included in ipu3-cio2-main.c at the
moment). It didn't seem worth it; but I can move those two includes from
the .c to the .h and then include ipu3-cio2.h in cio2-bridge.h

Which do you prefer?

>>> +#define MAX_NUM_LINK_FREQS                     3
>>> +
>>> +#define CIO2_SENSOR_CONFIG(_HID, _NR, ...)     \
>>> +       {                                       \
>>> +               .hid = _HID,                    \
>>> +               .nr_link_freqs = _NR,           \
>>> +               .link_freqs = { __VA_ARGS__ }   \
>>> +       }
>>> +
>>> +#define NODE_SENSOR(_HID, _PROPS)              \
>>> +       ((const struct software_node) {         \
>>> +               .name = _HID,                   \
>>> +               .properties = _PROPS,           \
>>> +       })
>>> +
>>> +#define NODE_PORT(_PORT, _SENSOR_NODE)         \
>>> +       ((const struct software_node) {         \
>>> +               _PORT,                          \
>>> +               _SENSOR_NODE,                   \
> 
> Could you use explicit assignments to fields here, please?
> 
>>> +       })
>>> +
>>> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)      \
>>> +       ((const struct software_node) {         \
>>> +               _EP,                            \
>>> +               _PORT,                          \
>>> +               _PROPS,                         \
> 
> Ditto.
> 

Will do


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

* Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2020-12-28 22:37       ` Daniel Scally
@ 2020-12-28 22:55         ` Andy Shevchenko
  2020-12-28 22:56           ` Andy Shevchenko
                             ` (2 more replies)
  2021-01-02 17:07         ` Sakari Ailus
  1 sibling, 3 replies; 58+ messages in thread
From: Andy Shevchenko @ 2020-12-28 22:55 UTC (permalink / raw)
  To: Daniel Scally
  Cc: Sakari Ailus, Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Bingbu Cao, Tian Shu Qiu,
	Mauro Carvalho Chehab, Robert Moore, Erik Kaneda, Petr Mladek,
	Steven Rostedt, Sergey Senozhatsky, Rasmus Villemoes,
	Laurent Pinchart, Jacopo Mondi, kieran.bingham+renesas,
	Hans Verkuil, Marco Felsch, niklas.soderlund+renesas,
	Steve Longerbeam, Krogerus, Heikki, Linus Walleij, Jordan Hand,
	Laurent Pinchart

On Mon, Dec 28, 2020 at 10:37:38PM +0000, Daniel Scally wrote:
> On 28/12/2020 17:05, Sakari Ailus wrote:
> > On Thu, Dec 24, 2020 at 02:54:44PM +0200, Andy Shevchenko wrote:

...

> >>> +#include <linux/property.h>
> >>> +
> >>> +#define CIO2_HID                               "INT343E"
> >>> +#define CIO2_NUM_PORTS                         4
> > 
> > This is already defined in ipu3-cio2.h. Could you include that instead?
> 
> Yes; but I'd need to also include media/v4l2-device.h and
> media/videobuf2-dma-sg.h (they're included in ipu3-cio2-main.c at the
> moment). It didn't seem worth it; but I can move those two includes from
> the .c to the .h and then include ipu3-cio2.h in cio2-bridge.h
> 
> Which do you prefer?

Actually ipu3-cio2.h misses a lot of inclusions (like mutex.h which I
immediately noticed when scrolled over data types). I think here should be a
compromise variant, split out something like ipu3-cio2-defs.h which can be
included in both ipu3-cio2.h and cio2-bridge.h.

And cio2-bridge.h needs more inclusions like types.h.

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2020-12-28 22:55         ` Andy Shevchenko
@ 2020-12-28 22:56           ` Andy Shevchenko
  2020-12-28 23:07           ` Laurent Pinchart
  2020-12-28 23:30           ` Daniel Scally
  2 siblings, 0 replies; 58+ messages in thread
From: Andy Shevchenko @ 2020-12-28 22:56 UTC (permalink / raw)
  To: Daniel Scally
  Cc: Sakari Ailus, Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Bingbu Cao, Tian Shu Qiu,
	Mauro Carvalho Chehab, Robert Moore, Erik Kaneda, Petr Mladek,
	Steven Rostedt, Sergey Senozhatsky, Rasmus Villemoes,
	Laurent Pinchart, Jacopo Mondi, kieran.bingham+renesas,
	Hans Verkuil, Marco Felsch, niklas.soderlund+renesas,
	Steve Longerbeam, Krogerus, Heikki, Linus Walleij, Jordan Hand,
	Laurent Pinchart

On Tue, Dec 29, 2020 at 12:55:45AM +0200, Andy Shevchenko wrote:
> On Mon, Dec 28, 2020 at 10:37:38PM +0000, Daniel Scally wrote:
> > On 28/12/2020 17:05, Sakari Ailus wrote:
> > > On Thu, Dec 24, 2020 at 02:54:44PM +0200, Andy Shevchenko wrote:
> 
> ...
> 
> > >>> +#include <linux/property.h>
> > >>> +
> > >>> +#define CIO2_HID                               "INT343E"
> > >>> +#define CIO2_NUM_PORTS                         4
> > > 
> > > This is already defined in ipu3-cio2.h. Could you include that instead?
> > 
> > Yes; but I'd need to also include media/v4l2-device.h and
> > media/videobuf2-dma-sg.h (they're included in ipu3-cio2-main.c at the
> > moment). It didn't seem worth it; but I can move those two includes from
> > the .c to the .h and then include ipu3-cio2.h in cio2-bridge.h
> > 
> > Which do you prefer?
> 
> Actually ipu3-cio2.h misses a lot of inclusions (like mutex.h which I
> immediately noticed when scrolled over data types). I think here should be a
> compromise variant, split out something like ipu3-cio2-defs.h which can be

Seems like cio2-defs.h more plausible name.

> included in both ipu3-cio2.h and cio2-bridge.h.
> 
> And cio2-bridge.h needs more inclusions like types.h.

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2020-12-28 22:55         ` Andy Shevchenko
  2020-12-28 22:56           ` Andy Shevchenko
@ 2020-12-28 23:07           ` Laurent Pinchart
  2020-12-28 23:54             ` Andy Shevchenko
  2020-12-28 23:30           ` Daniel Scally
  2 siblings, 1 reply; 58+ messages in thread
From: Laurent Pinchart @ 2020-12-28 23:07 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Daniel Scally, Sakari Ailus, Linux Kernel Mailing List,
	ACPI Devel Maling List, Linux Media Mailing List, devel,
	Rafael J. Wysocki, Len Brown, Greg Kroah-Hartman, Yong Zhi,
	Bingbu Cao, Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore,
	Erik Kaneda, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, Jordan Hand

On Tue, Dec 29, 2020 at 12:55:44AM +0200, Andy Shevchenko wrote:
> On Mon, Dec 28, 2020 at 10:37:38PM +0000, Daniel Scally wrote:
> > On 28/12/2020 17:05, Sakari Ailus wrote:
> > > On Thu, Dec 24, 2020 at 02:54:44PM +0200, Andy Shevchenko wrote:
> 
> ...
> 
> > >>> +#include <linux/property.h>
> > >>> +
> > >>> +#define CIO2_HID                               "INT343E"
> > >>> +#define CIO2_NUM_PORTS                         4
> > > 
> > > This is already defined in ipu3-cio2.h. Could you include that instead?
> > 
> > Yes; but I'd need to also include media/v4l2-device.h and
> > media/videobuf2-dma-sg.h (they're included in ipu3-cio2-main.c at the
> > moment). It didn't seem worth it; but I can move those two includes from
> > the .c to the .h and then include ipu3-cio2.h in cio2-bridge.h
> > 
> > Which do you prefer?
> 
> Actually ipu3-cio2.h misses a lot of inclusions (like mutex.h which I
> immediately noticed when scrolled over data types).

Then ipu3-cio2.h should be fixed :-)

> I think here should be a compromise variant, split out something like
> ipu3-cio2-defs.h which can be included in both ipu3-cio2.h and
> cio2-bridge.h.
> 
> And cio2-bridge.h needs more inclusions like types.h.

-- 
Regards,

Laurent Pinchart

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

* Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2020-12-28 22:55         ` Andy Shevchenko
  2020-12-28 22:56           ` Andy Shevchenko
  2020-12-28 23:07           ` Laurent Pinchart
@ 2020-12-28 23:30           ` Daniel Scally
  2020-12-28 23:47             ` Andy Shevchenko
  2 siblings, 1 reply; 58+ messages in thread
From: Daniel Scally @ 2020-12-28 23:30 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Sakari Ailus, Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Bingbu Cao, Tian Shu Qiu,
	Mauro Carvalho Chehab, Robert Moore, Erik Kaneda, Petr Mladek,
	Steven Rostedt, Sergey Senozhatsky, Rasmus Villemoes,
	Laurent Pinchart, Jacopo Mondi, kieran.bingham+renesas,
	Hans Verkuil, Marco Felsch, niklas.soderlund+renesas,
	Steve Longerbeam, Krogerus, Heikki, Linus Walleij, Jordan Hand,
	Laurent Pinchart

Hi Andy

On 28/12/2020 22:55, Andy Shevchenko wrote:
> On Mon, Dec 28, 2020 at 10:37:38PM +0000, Daniel Scally wrote:
>> On 28/12/2020 17:05, Sakari Ailus wrote:
>>> On Thu, Dec 24, 2020 at 02:54:44PM +0200, Andy Shevchenko wrote:
> ...
>
>>>>> +#include <linux/property.h>
>>>>> +
>>>>> +#define CIO2_HID                               "INT343E"
>>>>> +#define CIO2_NUM_PORTS                         4
>>> This is already defined in ipu3-cio2.h. Could you include that instead?
>> Yes; but I'd need to also include media/v4l2-device.h and
>> media/videobuf2-dma-sg.h (they're included in ipu3-cio2-main.c at the
>> moment). It didn't seem worth it; but I can move those two includes from
>> the .c to the .h and then include ipu3-cio2.h in cio2-bridge.h
>>
>> Which do you prefer?
> Actually ipu3-cio2.h misses a lot of inclusions (like mutex.h which I
> immediately noticed when scrolled over data types). I think here should be a
> compromise variant, split out something like ipu3-cio2-defs.h which can be
> included in both ipu3-cio2.h and cio2-bridge.h.


And just including all the things that need to be in both files you mean?

>
> And cio2-bridge.h needs more inclusions like types.h.


Added this in there too


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

* Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2020-12-28 23:30           ` Daniel Scally
@ 2020-12-28 23:47             ` Andy Shevchenko
  0 siblings, 0 replies; 58+ messages in thread
From: Andy Shevchenko @ 2020-12-28 23:47 UTC (permalink / raw)
  To: Daniel Scally
  Cc: Sakari Ailus, Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Media Mailing List, devel, Rafael J. Wysocki, Len Brown,
	Greg Kroah-Hartman, Yong Zhi, Bingbu Cao, Tian Shu Qiu,
	Mauro Carvalho Chehab, Robert Moore, Erik Kaneda, Petr Mladek,
	Steven Rostedt, Sergey Senozhatsky, Rasmus Villemoes,
	Laurent Pinchart, Jacopo Mondi, kieran.bingham+renesas,
	Hans Verkuil, Marco Felsch, niklas.soderlund+renesas,
	Steve Longerbeam, Krogerus, Heikki, Linus Walleij, Jordan Hand,
	Laurent Pinchart

On Tue, Dec 29, 2020 at 1:30 AM Daniel Scally <djrscally@gmail.com> wrote:
>
> Hi Andy
>
> On 28/12/2020 22:55, Andy Shevchenko wrote:
> > On Mon, Dec 28, 2020 at 10:37:38PM +0000, Daniel Scally wrote:
> >> On 28/12/2020 17:05, Sakari Ailus wrote:
> >>> On Thu, Dec 24, 2020 at 02:54:44PM +0200, Andy Shevchenko wrote:
> > ...
> >
> >>>>> +#include <linux/property.h>
> >>>>> +
> >>>>> +#define CIO2_HID                               "INT343E"
> >>>>> +#define CIO2_NUM_PORTS                         4
> >>> This is already defined in ipu3-cio2.h. Could you include that instead?
> >> Yes; but I'd need to also include media/v4l2-device.h and
> >> media/videobuf2-dma-sg.h (they're included in ipu3-cio2-main.c at the
> >> moment). It didn't seem worth it; but I can move those two includes from
> >> the .c to the .h and then include ipu3-cio2.h in cio2-bridge.h
> >>
> >> Which do you prefer?

> > I think here should be a
> > compromise variant, split out something like ipu3-cio2-defs.h which can be
> > included in both ipu3-cio2.h and cio2-bridge.h.
>
> And just including all the things that need to be in both files you mean?

Something which may be logically grouped together. It may include
something which cio2-bridge doesn't need, but at least it will be in
one place (for example, if you move one CIO2_PCI_* constant, that
means you better move all, or so, the rest CIO2_PCI_* constants as
well).


-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2020-12-28 23:07           ` Laurent Pinchart
@ 2020-12-28 23:54             ` Andy Shevchenko
  2020-12-29  0:07               ` Laurent Pinchart
  0 siblings, 1 reply; 58+ messages in thread
From: Andy Shevchenko @ 2020-12-28 23:54 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Daniel Scally, Sakari Ailus, Linux Kernel Mailing List,
	ACPI Devel Maling List, Linux Media Mailing List, devel,
	Rafael J. Wysocki, Len Brown, Greg Kroah-Hartman, Yong Zhi,
	Bingbu Cao, Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore,
	Erik Kaneda, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, Jordan Hand

On Tue, Dec 29, 2020 at 1:08 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Tue, Dec 29, 2020 at 12:55:44AM +0200, Andy Shevchenko wrote:
> > On Mon, Dec 28, 2020 at 10:37:38PM +0000, Daniel Scally wrote:
> > > On 28/12/2020 17:05, Sakari Ailus wrote:

...

> > > Which do you prefer?
> >
> > Actually ipu3-cio2.h misses a lot of inclusions (like mutex.h which I
> > immediately noticed when scrolled over data types).
>
> Then ipu3-cio2.h should be fixed :-)

Below is a draft patch (it is possible mangled, due to Gmail). Can you
look at it and tell me what you think?
I believe some headers can be removed, but I have no idea about header
inclusion guarantees that v4l2 provides.

From 10fa6c7ff66ded35a246677ffe20c677e8453f5b3 Mon Sep 17 00:00:00 2001
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: Tue, 29 Dec 2020 01:42:03 +0200
Subject: [PATCH 1/1] media: ipu3-cio2: Add headers that ipu3-cio2.h is direct
 user of

Add headers that ipu3-cio2.h is direct user of.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
index ccf0b85ae36f..9ea154c50ba1 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
@@ -4,8 +4,25 @@
 #ifndef __IPU3_CIO2_H
 #define __IPU3_CIO2_H

+#include <linux/bits.h>
+#include <linux/dma-mapping.h>
+#include <linux/kernel.h>
+#include <linux/mutex.h>
 #include <linux/types.h>

+#include <asm/page.h>
+
+#include <linux/videodev2.h>
+
+#include <media/media-device.h>
+#include <media/media-entity.h>
+#include <media/v4l2-async.h>
+#include <media/v4l2-dev.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-subdev.h>
+#include <media/videobuf2-core.h>
+#include <media/videobuf2-v4l2.h>
+
 #define CIO2_NAME "ipu3-cio2"
 #define CIO2_DEVICE_NAME "Intel IPU3 CIO2"
 #define CIO2_ENTITY_NAME "ipu3-csi2"
@@ -325,6 +342,8 @@ struct csi2_bus_info {
  u32 lanes;
 };

+struct cio2_fbpt_entry;
+
 struct cio2_queue {
  /* mutex to be used by vb2_queue */
  struct mutex lock;
@@ -355,6 +374,8 @@ struct cio2_queue {
  atomic_t bufs_queued;
 };

+struct pci_dev;
+
 struct cio2_device {
  struct pci_dev *pci_dev;
  void __iomem *base;
-- 
2.29.2


-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2020-12-28 23:54             ` Andy Shevchenko
@ 2020-12-29  0:07               ` Laurent Pinchart
  2020-12-30 20:47                 ` Andy Shevchenko
  0 siblings, 1 reply; 58+ messages in thread
From: Laurent Pinchart @ 2020-12-29  0:07 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Daniel Scally, Sakari Ailus, Linux Kernel Mailing List,
	ACPI Devel Maling List, Linux Media Mailing List, devel,
	Rafael J. Wysocki, Len Brown, Greg Kroah-Hartman, Yong Zhi,
	Bingbu Cao, Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore,
	Erik Kaneda, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, Jordan Hand

Hi Andy,

On Tue, Dec 29, 2020 at 01:54:59AM +0200, Andy Shevchenko wrote:
> On Tue, Dec 29, 2020 at 1:08 AM Laurent Pinchart wrote:
> >
> > On Tue, Dec 29, 2020 at 12:55:44AM +0200, Andy Shevchenko wrote:
> > > On Mon, Dec 28, 2020 at 10:37:38PM +0000, Daniel Scally wrote:
> > > > On 28/12/2020 17:05, Sakari Ailus wrote:
> 
> ...
> 
> > > > Which do you prefer?
> > >
> > > Actually ipu3-cio2.h misses a lot of inclusions (like mutex.h which I
> > > immediately noticed when scrolled over data types).
> >
> > Then ipu3-cio2.h should be fixed :-)
> 
> Below is a draft patch (it is possible mangled, due to Gmail). Can you
> look at it and tell me what you think?

Thank you for the patch.

> I believe some headers can be removed, but I have no idea about header
> inclusion guarantees that v4l2 provides.
> 
> From 10fa6c7ff66ded35a246677ffe20c677e8453f5b3 Mon Sep 17 00:00:00 2001
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Date: Tue, 29 Dec 2020 01:42:03 +0200
> Subject: [PATCH 1/1] media: ipu3-cio2: Add headers that ipu3-cio2.h is direct
>  user of
> 
> Add headers that ipu3-cio2.h is direct user of.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> index ccf0b85ae36f..9ea154c50ba1 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> @@ -4,8 +4,25 @@
>  #ifndef __IPU3_CIO2_H
>  #define __IPU3_CIO2_H
> 
> +#include <linux/bits.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/kernel.h>
> +#include <linux/mutex.h>
>  #include <linux/types.h>
> 
> +#include <asm/page.h>
> +
> +#include <linux/videodev2.h>

I think this can be dropped.

> +
> +#include <media/media-device.h>
> +#include <media/media-entity.h>
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/videobuf2-core.h>
> +#include <media/videobuf2-v4l2.h>
> +
>  #define CIO2_NAME "ipu3-cio2"
>  #define CIO2_DEVICE_NAME "Intel IPU3 CIO2"
>  #define CIO2_ENTITY_NAME "ipu3-csi2"
> @@ -325,6 +342,8 @@ struct csi2_bus_info {
>   u32 lanes;
>  };
> 
> +struct cio2_fbpt_entry;
> +
>  struct cio2_queue {
>   /* mutex to be used by vb2_queue */
>   struct mutex lock;
> @@ -355,6 +374,8 @@ struct cio2_queue {
>   atomic_t bufs_queued;
>  };
> 
> +struct pci_dev;
> +

How about grouping all forward declarations at the top ?

Otherwise this looks good,

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

If I was maintaining this driver I would likely move the structure
definitions to ipu3-cio2.c though.

>  struct cio2_device {
>   struct pci_dev *pci_dev;
>   void __iomem *base;

-- 
Regards,

Laurent Pinchart

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

* Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2020-12-29  0:07               ` Laurent Pinchart
@ 2020-12-30 20:47                 ` Andy Shevchenko
  0 siblings, 0 replies; 58+ messages in thread
From: Andy Shevchenko @ 2020-12-30 20:47 UTC (permalink / raw)
  To: Laurent Pinchart, Andy Shevchenko
  Cc: Daniel Scally, Sakari Ailus, Linux Kernel Mailing List,
	ACPI Devel Maling List, Linux Media Mailing List, devel,
	Rafael J. Wysocki, Len Brown, Greg Kroah-Hartman, Yong Zhi,
	Bingbu Cao, Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore,
	Erik Kaneda, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
	Rasmus Villemoes, Laurent Pinchart, Jacopo Mondi,
	kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, Jordan Hand

On Tue, Dec 29, 2020 at 2:07 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tue, Dec 29, 2020 at 01:54:59AM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 29, 2020 at 1:08 AM Laurent Pinchart wrote:

...

> > +#include <linux/videodev2.h>
>
> I think this can be dropped.

I dropped above (I noticed it's included by a half of the headers listed below.

> > +#include <media/media-device.h>
> > +#include <media/media-entity.h>
> > +#include <media/v4l2-async.h>
> > +#include <media/v4l2-dev.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-subdev.h>
> > +#include <media/videobuf2-core.h>
> > +#include <media/videobuf2-v4l2.h>

...

> How about grouping all forward declarations at the top ?

Done.

> Otherwise this looks good,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!
I just sent a formal patch with your tag included.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2020-12-28 22:37       ` Daniel Scally
  2020-12-28 22:55         ` Andy Shevchenko
@ 2021-01-02 17:07         ` Sakari Ailus
  2021-01-02 17:12           ` Daniel Scally
  1 sibling, 1 reply; 58+ messages in thread
From: Sakari Ailus @ 2021-01-02 17:07 UTC (permalink / raw)
  To: Daniel Scally
  Cc: Andy Shevchenko, Linux Kernel Mailing List,
	ACPI Devel Maling List, Linux Media Mailing List, devel,
	Rafael J. Wysocki, Len Brown, Greg Kroah-Hartman, Yong Zhi,
	Bingbu Cao, Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore,
	Erik Kaneda, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
	Andy Shevchenko, Rasmus Villemoes, Laurent Pinchart,
	Jacopo Mondi, kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, Jordan Hand, Laurent Pinchart

Hi Daniel,

On Mon, Dec 28, 2020 at 10:37:38PM +0000, Daniel Scally wrote:
> >>> +#define CIO2_NUM_PORTS                         4
> > 
> > This is already defined in ipu3-cio2.h. Could you include that instead?
> 
> Yes; but I'd need to also include media/v4l2-device.h and
> media/videobuf2-dma-sg.h (they're included in ipu3-cio2-main.c at the
> moment). It didn't seem worth it; but I can move those two includes from
> the .c to the .h and then include ipu3-cio2.h in cio2-bridge.h
> 
> Which do you prefer?

Seems you got answers already... :-) splitting the header in two seems good
to me. But IMO it doesn't have to be a part of this set.

-- 
Sakari Ailus

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

* Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2021-01-02 17:07         ` Sakari Ailus
@ 2021-01-02 17:12           ` Daniel Scally
  2021-01-02 17:21             ` Sakari Ailus
  2021-01-02 17:24             ` Sakari Ailus
  0 siblings, 2 replies; 58+ messages in thread
From: Daniel Scally @ 2021-01-02 17:12 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Andy Shevchenko, Linux Kernel Mailing List,
	ACPI Devel Maling List, Linux Media Mailing List, devel,
	Rafael J. Wysocki, Len Brown, Greg Kroah-Hartman, Yong Zhi,
	Bingbu Cao, Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore,
	Erik Kaneda, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
	Andy Shevchenko, Rasmus Villemoes, Laurent Pinchart,
	Jacopo Mondi, kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, Jordan Hand, Laurent Pinchart

Hi Sakari

On 02/01/2021 17:07, Sakari Ailus wrote:
> Hi Daniel,
>
> On Mon, Dec 28, 2020 at 10:37:38PM +0000, Daniel Scally wrote:
>>>>> +#define CIO2_NUM_PORTS                         4
>>> This is already defined in ipu3-cio2.h. Could you include that instead?
>> Yes; but I'd need to also include media/v4l2-device.h and
>> media/videobuf2-dma-sg.h (they're included in ipu3-cio2-main.c at the
>> moment). It didn't seem worth it; but I can move those two includes from
>> the .c to the .h and then include ipu3-cio2.h in cio2-bridge.h
>>
>> Which do you prefer?
> Seems you got answers already... :-) splitting the header in two seems good
> to me. But IMO it doesn't have to be a part of this set.
>
Yeah I've been hesitating over this; if we chose not to do it in this
set though, how would you want me to deal with the double definition of
CIO2_NUM_PORTS

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

* Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2021-01-02 17:12           ` Daniel Scally
@ 2021-01-02 17:21             ` Sakari Ailus
  2021-01-02 17:24             ` Sakari Ailus
  1 sibling, 0 replies; 58+ messages in thread
From: Sakari Ailus @ 2021-01-02 17:21 UTC (permalink / raw)
  To: Daniel Scally
  Cc: Andy Shevchenko, Linux Kernel Mailing List,
	ACPI Devel Maling List, Linux Media Mailing List, devel,
	Rafael J. Wysocki, Len Brown, Greg Kroah-Hartman, Yong Zhi,
	Bingbu Cao, Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore,
	Erik Kaneda, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
	Andy Shevchenko, Rasmus Villemoes, Laurent Pinchart,
	Jacopo Mondi, kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, Jordan Hand, Laurent Pinchart

On Sat, Jan 02, 2021 at 05:12:47PM +0000, Daniel Scally wrote:
> Hi Sakari
> 
> On 02/01/2021 17:07, Sakari Ailus wrote:
> > Hi Daniel,
> >
> > On Mon, Dec 28, 2020 at 10:37:38PM +0000, Daniel Scally wrote:
> >>>>> +#define CIO2_NUM_PORTS                         4
> >>> This is already defined in ipu3-cio2.h. Could you include that instead?
> >> Yes; but I'd need to also include media/v4l2-device.h and
> >> media/videobuf2-dma-sg.h (they're included in ipu3-cio2-main.c at the
> >> moment). It didn't seem worth it; but I can move those two includes from
> >> the .c to the .h and then include ipu3-cio2.h in cio2-bridge.h
> >>
> >> Which do you prefer?
> > Seems you got answers already... :-) splitting the header in two seems good
> > to me. But IMO it doesn't have to be a part of this set.
> >
> Yeah I've been hesitating over this; if we chose not to do it in this
> set though, how would you want me to deal with the double definition of
> CIO2_NUM_PORTS

How about prepending Andy's patch to the set and including the header?

-- 
Sakari Ailus

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

* Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2021-01-02 17:12           ` Daniel Scally
  2021-01-02 17:21             ` Sakari Ailus
@ 2021-01-02 17:24             ` Sakari Ailus
  2021-01-02 21:23               ` Daniel Scally
  1 sibling, 1 reply; 58+ messages in thread
From: Sakari Ailus @ 2021-01-02 17:24 UTC (permalink / raw)
  To: Daniel Scally
  Cc: Andy Shevchenko, Linux Kernel Mailing List,
	ACPI Devel Maling List, Linux Media Mailing List, devel,
	Rafael J. Wysocki, Len Brown, Greg Kroah-Hartman, Yong Zhi,
	Bingbu Cao, Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore,
	Erik Kaneda, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
	Andy Shevchenko, Rasmus Villemoes, Laurent Pinchart,
	Jacopo Mondi, kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, Jordan Hand, Laurent Pinchart

On Sat, Jan 02, 2021 at 05:12:47PM +0000, Daniel Scally wrote:
> Hi Sakari
> 
> On 02/01/2021 17:07, Sakari Ailus wrote:
> > Hi Daniel,
> >
> > On Mon, Dec 28, 2020 at 10:37:38PM +0000, Daniel Scally wrote:
> >>>>> +#define CIO2_NUM_PORTS                         4
> >>> This is already defined in ipu3-cio2.h. Could you include that instead?
> >> Yes; but I'd need to also include media/v4l2-device.h and
> >> media/videobuf2-dma-sg.h (they're included in ipu3-cio2-main.c at the
> >> moment). It didn't seem worth it; but I can move those two includes from
> >> the .c to the .h and then include ipu3-cio2.h in cio2-bridge.h
> >>
> >> Which do you prefer?
> > Seems you got answers already... :-) splitting the header in two seems good
> > to me. But IMO it doesn't have to be a part of this set.
> >
> Yeah I've been hesitating over this; if we chose not to do it in this
> set though, how would you want me to deal with the double definition of
> CIO2_NUM_PORTS

The patch is here:

<URL:https://patchwork.linuxtv.org/project/linux-media/patch/20201230204405.62892-1-andriy.shevchenko@linux.intel.com/>

I guess Andy forgot to cc you.

-- 
Sakari Ailus

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

* Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
  2021-01-02 17:24             ` Sakari Ailus
@ 2021-01-02 21:23               ` Daniel Scally
  0 siblings, 0 replies; 58+ messages in thread
From: Daniel Scally @ 2021-01-02 21:23 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Andy Shevchenko, Linux Kernel Mailing List,
	ACPI Devel Maling List, Linux Media Mailing List, devel,
	Rafael J. Wysocki, Len Brown, Greg Kroah-Hartman, Yong Zhi,
	Bingbu Cao, Tian Shu Qiu, Mauro Carvalho Chehab, Robert Moore,
	Erik Kaneda, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
	Andy Shevchenko, Rasmus Villemoes, Laurent Pinchart,
	Jacopo Mondi, kieran.bingham+renesas, Hans Verkuil, Marco Felsch,
	niklas.soderlund+renesas, Steve Longerbeam, Krogerus, Heikki,
	Linus Walleij, Jordan Hand, Laurent Pinchart

Hi Sakari

On 02/01/2021 17:24, Sakari Ailus wrote:
> On Sat, Jan 02, 2021 at 05:12:47PM +0000, Daniel Scally wrote:
>> Hi Sakari
>>
>> On 02/01/2021 17:07, Sakari Ailus wrote:
>>> Hi Daniel,
>>>
>>> On Mon, Dec 28, 2020 at 10:37:38PM +0000, Daniel Scally wrote:
>>>>>>> +#define CIO2_NUM_PORTS                         4
>>>>> This is already defined in ipu3-cio2.h. Could you include that instead?
>>>> Yes; but I'd need to also include media/v4l2-device.h and
>>>> media/videobuf2-dma-sg.h (they're included in ipu3-cio2-main.c at the
>>>> moment). It didn't seem worth it; but I can move those two includes from
>>>> the .c to the .h and then include ipu3-cio2.h in cio2-bridge.h
>>>>
>>>> Which do you prefer?
>>> Seems you got answers already... :-) splitting the header in two seems good
>>> to me. But IMO it doesn't have to be a part of this set.
>>>
>> Yeah I've been hesitating over this; if we chose not to do it in this
>> set though, how would you want me to deal with the double definition of
>> CIO2_NUM_PORTS
> The patch is here:
>
> <URL:https://patchwork.linuxtv.org/project/linux-media/patch/20201230204405.62892-1-andriy.shevchenko@linux.intel.com/>
>
> I guess Andy forgot to cc you.
>
Ah - thanks, I'd not read back through the list yet. I'll look at this
tonight then.

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

end of thread, other threads:[~2021-01-02 21:24 UTC | newest]

Thread overview: 58+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-24  1:08 [PATCH v3 00/14] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
2020-12-24  1:08 ` [PATCH v3 01/14] software_node: Fix refcounts in software_node_get_next_child() Daniel Scally
2020-12-24  1:08 ` [PATCH v3 02/14] property: Return true in fwnode_device_is_available for NULL ops Daniel Scally
2020-12-24  1:08 ` [PATCH v3 03/14] property: Call fwnode_graph_get_endpoint_by_id() for fwnode->secondary Daniel Scally
2020-12-24  1:08 ` [PATCH v3 04/14] software_node: Enforce parent before child ordering of nodes arrays Daniel Scally
2020-12-24  1:08 ` [PATCH v3 05/14] software_node: unregister software_nodes in reverse order Daniel Scally
2020-12-24 12:13   ` Andy Shevchenko
2020-12-24 14:00     ` Daniel Scally
2020-12-24 14:12       ` Andy Shevchenko
2020-12-24 14:14         ` Daniel Scally
2020-12-24 18:36           ` David Laight
2020-12-28 10:15             ` Andy Shevchenko
2020-12-28 21:17               ` Daniel Scally
2020-12-28 16:34   ` Sakari Ailus
2020-12-28 17:47     ` Andy Shevchenko
2020-12-24  1:08 ` [PATCH v3 06/14] include: fwnode.h: Define format macros for ports and endpoints Daniel Scally
2020-12-24 12:17   ` Andy Shevchenko
2020-12-24 12:41     ` Laurent Pinchart
2020-12-28 16:30   ` Sakari Ailus
2020-12-28 17:11     ` Sakari Ailus
2020-12-28 21:36       ` Daniel Scally
2020-12-24  1:09 ` [PATCH v3 07/14] software_node: Add support for fwnode_graph*() family of functions Daniel Scally
2020-12-24 12:24   ` Andy Shevchenko
2020-12-24 12:55     ` Laurent Pinchart
2020-12-24 13:03       ` Andy Shevchenko
2020-12-24 14:21         ` Daniel Scally
2020-12-28 16:41           ` Sakari Ailus
2020-12-28 21:37             ` Daniel Scally
2020-12-26 23:47     ` Daniel Scally
2020-12-24 12:53   ` Laurent Pinchart
2020-12-24 14:24     ` Daniel Scally
2020-12-24  1:09 ` [PATCH v3 08/14] lib/test_printf.c: Use helper function to unwind array of software_nodes Daniel Scally
2020-12-24  1:09 ` [PATCH v3 09/14] ipu3-cio2: Add T: entry to MAINTAINERS Daniel Scally
2020-12-24  1:09 ` [PATCH v3 10/14] ipu3-cio2: Rename ipu3-cio2.c Daniel Scally
2020-12-24  1:09 ` [PATCH v3 11/14] media: v4l2-core: v4l2-async: Check sd->fwnode->secondary in match_fwnode() Daniel Scally
2020-12-24  1:09 ` [PATCH v3 12/14] acpi: Add acpi_dev_get_next_match_dev() and helper macro Daniel Scally
2020-12-24  1:09 ` [PATCH v3 13/14] include: media: v4l2-fwnode: Include v4l2_fwnode_bus_type Daniel Scally
2020-12-24 12:32   ` Andy Shevchenko
2020-12-26 23:14     ` Daniel Scally
2020-12-24 12:58   ` Laurent Pinchart
2020-12-24  1:09 ` [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver Daniel Scally
2020-12-24 12:54   ` Andy Shevchenko
2020-12-26 23:23     ` Daniel Scally
2020-12-28 17:05     ` Sakari Ailus
2020-12-28 22:37       ` Daniel Scally
2020-12-28 22:55         ` Andy Shevchenko
2020-12-28 22:56           ` Andy Shevchenko
2020-12-28 23:07           ` Laurent Pinchart
2020-12-28 23:54             ` Andy Shevchenko
2020-12-29  0:07               ` Laurent Pinchart
2020-12-30 20:47                 ` Andy Shevchenko
2020-12-28 23:30           ` Daniel Scally
2020-12-28 23:47             ` Andy Shevchenko
2021-01-02 17:07         ` Sakari Ailus
2021-01-02 17:12           ` Daniel Scally
2021-01-02 17:21             ` Sakari Ailus
2021-01-02 17:24             ` Sakari Ailus
2021-01-02 21:23               ` Daniel Scally

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).