[PATCH v4 00/15] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows

[PATCH v4 00/15] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows

[Daniel Scally]

Hello all

v3:
https://lore.kernel.org/linux-media/20201224010907.263125-1-djrscally@gmail.com/T/#m37b831bb2b406917d6db5da9acf9ed35df65d72d
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
  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:
	- Incorporated Andy's patch fixing the ipu3-cio2 header

More details of changes on each patch.

Thanks
Dan

Andy Shevchenko (1):
  media: ipu3-cio2: Add headers that ipu3-cio2.h is direct user of

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                         | 181 +++++++++--
 drivers/media/pci/intel/ipu3/Kconfig          |  18 ++
 drivers/media/pci/intel/ipu3/Makefile         |   3 +
 drivers/media/pci/intel/ipu3/cio2-bridge.c    | 302 ++++++++++++++++++
 drivers/media/pci/intel/ipu3/cio2-bridge.h    | 125 ++++++++
 .../ipu3/{ipu3-cio2.c => ipu3-cio2-main.c}    |  33 ++
 drivers/media/pci/intel/ipu3/ipu3-cio2.h      |  24 ++
 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                        |   7 +
 include/media/v4l2-fwnode.h                   |  22 ++
 lib/test_printf.c                             |   4 +-
 16 files changed, 754 insertions(+), 38 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

[PATCH v4 01/15] software_node: Fix refcounts in software_node_get_next_child()

[Daniel Scally]

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 v4:

	- 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

[PATCH v4 02/15] media: ipu3-cio2: Add headers that ipu3-cio2.h is direct user of

[Daniel Scally]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

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

	- Incorporated so it can be used in a later patch - thanks Andy.
	  Not sure if this needs my Signed-off-by since I didn't write
	  any part of it. checkpatch.pl --strict doesn't complain at its
	  absence so I'm going with no; but let me know if I'm wrong.

 drivers/media/pci/intel/ipu3/ipu3-cio2.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
index ccf0b85ae36f..62187ab5ae43 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
@@ -4,8 +4,26 @@
 #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 <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>
+
+struct cio2_fbpt_entry;		/* defined here, after the first usage */
+struct pci_dev;
+
 #define CIO2_NAME					"ipu3-cio2"
 #define CIO2_DEVICE_NAME				"Intel IPU3 CIO2"
 #define CIO2_ENTITY_NAME				"ipu3-csi2"
-- 
2.25.1

[PATCH v4 03/15] property: Return true in fwnode_device_is_available for NULL ops

[Daniel Scally]

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 v4:

	- 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

[PATCH v4 04/15] property: Call fwnode_graph_get_endpoint_by_id() for fwnode->secondary

[Daniel Scally]

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 v4:

	- 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

[PATCH v4 05/15] software_node: Enforce parent before child ordering of nodes arrays

[Daniel Scally]

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 v4:

	- None

 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

[PATCH v4 06/15] software_node: unregister software_nodes in reverse order

[Daniel Scally]

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>
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 v4:

	- Changed the language of the comment to be easier to follow

 drivers/base/swnode.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index ade49173ff8d..1f43c51b431e 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -779,16 +779,23 @@ 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 members of the array are
+ * children of another member then the children must appear later in the list such
+ * that they are unregistered first.
  */
-void software_node_unregister_node_group(const struct software_node **node_group)
+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

[PATCH v4 07/15] include: fwnode.h: Define format macros for ports and endpoints

[Daniel Scally]

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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v4:

	- FORMAT -> FMT
	- Dropped the *_LEN macros, since we settled on using
	  strlen("port@") instead

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

diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 9506f8ec0974..72d36d46287d 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -32,6 +32,13 @@ struct fwnode_endpoint {
 	const struct fwnode_handle *local_fwnode;
 };
 
+/*
+ * ports and endpoints defined as software_nodes should all follow a common
+ * naming scheme; use these macros to ensure commonality.
+ */
+#define SWNODE_GRAPH_PORT_NAME_FMT		"port@%u"
+#define SWNODE_GRAPH_ENDPOINT_NAME_FMT		"endpoint@%u"
+
 #define NR_FWNODE_REFERENCE_ARGS	8
 
 /**
-- 
2.25.1

[PATCH v4 08/15] software_node: Add support for fwnode_graph*() family of functions

[Daniel Scally]

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".

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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 v4:

	- Replaced the FWNODE_GRAPH_PORT_NAME_PREFIX_LEN macro with
	  strlen("port@") throughout
	- Added a check to software_node_graph_parse_endpoint() to ensure
	  the name of the endpoint's parent matches the expected port@n
	  format

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

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 1f43c51b431e..82f9d6326110 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -540,6 +540,116 @@ 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))) {
+		/*
+		 * fwnode ports have naming style "port@", so we search for any
+		 * children that follow that convention.
+		 */
+		if (!strncmp(to_swnode(port)->node->name, "port@",
+			     strlen("port@")))
+			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);
+	const char *parent_name = swnode->parent->node->name;
+	int ret;
+
+	if (!(strlen(parent_name) > strlen("port@")) ||
+	    strncmp(parent_name, "port@", strlen("port@")))
+		return -EINVAL;
+
+	/* Ports have naming style "port@n", we need to select the n */
+	ret = kstrtou32(parent_name + strlen("port@"),
+			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 +661,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

[PATCH v4 09/15] lib/test_printf.c: Use helper function to unwind array of software_nodes

[Daniel Scally]

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 v4:

	- 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

[PATCH v4 10/15] ipu3-cio2: Add T: entry to MAINTAINERS

[Daniel Scally]

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 v4:

	- 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

[PATCH v4 11/15] ipu3-cio2: Rename ipu3-cio2.c

[Daniel Scally]

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 v4:

	- 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

[PATCH v4 12/15] media: v4l2-core: v4l2-async: Check sd->fwnode->secondary in match_fwnode()

[Daniel Scally]

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 v4:

	- 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

[PATCH v4 13/15] acpi: Add acpi_dev_get_next_match_dev() and helper macro

[Daniel Scally]

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 v4:

	- None

 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

[PATCH v4 14/15] include: media: v4l2-fwnode: Include v4l2_fwnode_bus_type

[Daniel Scally]

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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v4:

	- Dropped the trailing comma from NR_OF_V4L2_FWNODE_BUS_TYPE

 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..77fd6a3ec308 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: BT.656 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

[PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Daniel Scally]

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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v4:

	- Added local definition of CIO2_MAX_LANES
	- Added some includes to cio2-bridge.h
	- Moved the inner loop of cio2_bridge_connect_sensors() to a
	  standalone function
	- Altered macros to make explicit assignments to members rather
	  than relying on position
	- A couple of minor format changes, mostly line wrapping

 MAINTAINERS                                   |   1 +
 drivers/media/pci/intel/ipu3/Kconfig          |  18 ++
 drivers/media/pci/intel/ipu3/Makefile         |   1 +
 drivers/media/pci/intel/ipu3/cio2-bridge.c    | 302 ++++++++++++++++++
 drivers/media/pci/intel/ipu3/cio2-bridge.h    | 125 ++++++++
 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  33 ++
 drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   6 +
 7 files changed, 486 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..96a2231b16ad 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 the 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..3a7bedb08f66
--- /dev/null
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
@@ -0,0 +1,302 @@
+// 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 IDs 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 < CIO2_MAX_LANES; 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),
+		 SWNODE_GRAPH_PORT_NAME_FMT, sensor->ssdb.link);
+	snprintf(sensor->node_names.port,
+		 sizeof(sensor->node_names.port),
+		 SWNODE_GRAPH_PORT_NAME_FMT, 0); /* Always port 0 */
+	snprintf(sensor->node_names.endpoint,
+		 sizeof(sensor->node_names.endpoint),
+		 SWNODE_GRAPH_ENDPOINT_NAME_FMT, 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_sensor(const struct cio2_sensor_config *cfg,
+				      struct cio2_bridge *bridge,
+				      struct pci_dev *cio2)
+{
+	struct fwnode_handle *fwnode;
+	struct cio2_sensor *sensor;
+	struct acpi_device *adev;
+	int ret;
+
+	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 > 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 0;
+
+err_free_swnodes:
+	software_node_unregister_nodes(sensor->swnodes);
+err_put_adev:
+	acpi_dev_put(sensor->adev);
+err_out:
+	return ret;
+}
+
+static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge,
+				       struct pci_dev *cio2)
+{
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
+		const struct cio2_sensor_config *cfg = &cio2_supported_sensors[i];
+
+		ret = cio2_bridge_connect_sensor(cfg, bridge, cio2);
+		if (ret)
+			goto err_unregister_sensors;
+	}
+
+	return 0;
+
+err_unregister_sensors:
+	cio2_bridge_unregister_sensors(bridge);
+	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..3ec4ed44aced
--- /dev/null
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
@@ -0,0 +1,125 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Author: Dan Scally <djrscally@gmail.com> */
+#ifndef __CIO2_BRIDGE_H
+#define __CIO2_BRIDGE_H
+
+#include <linux/property.h>
+#include <linux/types.h>
+
+#include "ipu3-cio2.h"
+
+#define CIO2_HID				"INT343E"
+#define CIO2_MAX_LANES				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) {		\
+		.name = _PORT,			\
+		.parent = _SENSOR_NODE,		\
+	})
+
+#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\
+	((const struct software_node) {		\
+		.name = _EP,			\
+		.parent = _PORT,		\
+		.properties = _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..50c7ea467795 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,22 @@ 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 62187ab5ae43..dc3e343a37fb 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
@@ -455,4 +455,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

Re: [PATCH v4 08/15] software_node: Add support for fwnode_graph*() family of functions

[Andy Shevchenko]

On Sun, Jan 03, 2021 at 11:12:28PM +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".

Couple of nitpicks below (can be considered as follow up, depends on yours and
maintainer wishes).

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 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 v4:
> 
> 	- Replaced the FWNODE_GRAPH_PORT_NAME_PREFIX_LEN macro with
> 	  strlen("port@") throughout
> 	- Added a check to software_node_graph_parse_endpoint() to ensure
> 	  the name of the endpoint's parent matches the expected port@n
> 	  format
> 
>  drivers/base/swnode.c | 116 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 115 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 1f43c51b431e..82f9d6326110 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -540,6 +540,116 @@ 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))) {
> +		/*
> +		 * fwnode ports have naming style "port@", so we search for any
> +		 * children that follow that convention.
> +		 */
> +		if (!strncmp(to_swnode(port)->node->name, "port@",
> +			     strlen("port@")))
> +			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);
> +	const char *parent_name = swnode->parent->node->name;
> +	int ret;
> +
> +	if (!(strlen(parent_name) > strlen("port@")) ||

A nit:

	if (strlen("port@") >= strlen(parent_name) ||

better to read

> +	    strncmp(parent_name, "port@", strlen("port@")))
> +		return -EINVAL;
> +
> +	/* Ports have naming style "port@n", we need to select the n */
> +	ret = kstrtou32(parent_name + strlen("port@"),
> +			10, &endpoint->port);

A nit:

	ret = kstrtou32(parent_name + strlen("port@"), 10, &endpoint->port);

(perhaps you need to adjust your editor settings, this still fits 80)

> +	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 +661,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

Re: [PATCH v4 08/15] software_node: Add support for fwnode_graph*() family of functions

[Daniel Scally]

Hi Andy

On 04/01/2021 10:22, Andy Shevchenko wrote:
> On Sun, Jan 03, 2021 at 11:12:28PM +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".
> Couple of nitpicks below (can be considered as follow up, depends on yours and
> maintainer wishes).
>
Thanks
> +static int
> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
> +				   struct fwnode_endpoint *endpoint)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	const char *parent_name = swnode->parent->node->name;
> +	int ret;
> +
> +	if (!(strlen(parent_name) > strlen("port@")) ||
> A nit:
>
> 	if (strlen("port@") >= strlen(parent_name) ||
>
> better to read

yeah agreed

>
>> +	    strncmp(parent_name, "port@", strlen("port@")))
>> +		return -EINVAL;
>> +
>> +	/* Ports have naming style "port@n", we need to select the n */
>> +	ret = kstrtou32(parent_name + strlen("port@"),
>> +			10, &endpoint->port);
> A nit:
>
> 	ret = kstrtou32(parent_name + strlen("port@"), 10, &endpoint->port);
>
> (perhaps you need to adjust your editor settings, this still fits 80)
Ah - my bad. Originally instead of parent_name there was
swnode->parent->node->name, which didn't fit. When I added the temp
variable I forgot to fix the line break - thanks.

Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Andy Shevchenko]

On Sun, Jan 03, 2021 at 11:12:35PM +0000, Daniel Scally 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 (I consider it's good enough as is, though).

> Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v4:
> 
> 	- Added local definition of CIO2_MAX_LANES
> 	- Added some includes to cio2-bridge.h
> 	- Moved the inner loop of cio2_bridge_connect_sensors() to a
> 	  standalone function
> 	- Altered macros to make explicit assignments to members rather
> 	  than relying on position
> 	- A couple of minor format changes, mostly line wrapping
> 
>  MAINTAINERS                                   |   1 +
>  drivers/media/pci/intel/ipu3/Kconfig          |  18 ++
>  drivers/media/pci/intel/ipu3/Makefile         |   1 +
>  drivers/media/pci/intel/ipu3/cio2-bridge.c    | 302 ++++++++++++++++++
>  drivers/media/pci/intel/ipu3/cio2-bridge.h    | 125 ++++++++
>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  33 ++
>  drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   6 +
>  7 files changed, 486 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..96a2231b16ad 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 the 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..3a7bedb08f66
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> @@ -0,0 +1,302 @@
> +// 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 IDs 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 < CIO2_MAX_LANES; 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),
> +		 SWNODE_GRAPH_PORT_NAME_FMT, sensor->ssdb.link);
> +	snprintf(sensor->node_names.port,
> +		 sizeof(sensor->node_names.port),
> +		 SWNODE_GRAPH_PORT_NAME_FMT, 0); /* Always port 0 */
> +	snprintf(sensor->node_names.endpoint,
> +		 sizeof(sensor->node_names.endpoint),
> +		 SWNODE_GRAPH_ENDPOINT_NAME_FMT, 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_sensor(const struct cio2_sensor_config *cfg,
> +				      struct cio2_bridge *bridge,
> +				      struct pci_dev *cio2)
> +{
> +	struct fwnode_handle *fwnode;
> +	struct cio2_sensor *sensor;
> +	struct acpi_device *adev;
> +	int ret;

> +	for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {

(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;

A nit: this would be better to be at (1) location.

Then possible to factor out the rest of the body of this loop as well.

(Also can be considered as a hint for the future improvement)

> +		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 > 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 0;
> +
> +err_free_swnodes:
> +	software_node_unregister_nodes(sensor->swnodes);
> +err_put_adev:
> +	acpi_dev_put(sensor->adev);
> +err_out:
> +	return ret;
> +}
> +
> +static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge,
> +				       struct pci_dev *cio2)
> +{
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
> +		const struct cio2_sensor_config *cfg = &cio2_supported_sensors[i];
> +
> +		ret = cio2_bridge_connect_sensor(cfg, bridge, cio2);
> +		if (ret)
> +			goto err_unregister_sensors;
> +	}
> +
> +	return 0;
> +
> +err_unregister_sensors:
> +	cio2_bridge_unregister_sensors(bridge);
> +	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..3ec4ed44aced
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Author: Dan Scally <djrscally@gmail.com> */
> +#ifndef __CIO2_BRIDGE_H
> +#define __CIO2_BRIDGE_H
> +
> +#include <linux/property.h>
> +#include <linux/types.h>
> +
> +#include "ipu3-cio2.h"
> +
> +#define CIO2_HID				"INT343E"
> +#define CIO2_MAX_LANES				4
> +#define MAX_NUM_LINK_FREQS			3
> +
> +#define CIO2_SENSOR_CONFIG(_HID, _NR, ...)	\
> +	{					\
> +		.hid = _HID,			\
> +		.nr_link_freqs = _NR,		\
> +		.link_freqs = { __VA_ARGS__ }	\
> +	}

Perhaps also good to declare it as a compound literal.

(Means to add (struct ...) to the initializer.

> +#define NODE_SENSOR(_HID, _PROPS)		\
> +	((const struct software_node) {		\
> +		.name = _HID,			\
> +		.properties = _PROPS,		\
> +	})
> +
> +#define NODE_PORT(_PORT, _SENSOR_NODE)		\
> +	((const struct software_node) {		\
> +		.name = _PORT,			\
> +		.parent = _SENSOR_NODE,		\
> +	})
> +
> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\
> +	((const struct software_node) {		\
> +		.name = _EP,			\
> +		.parent = _PORT,		\
> +		.properties = _PROPS,		\
> +	})

In all three I didn't get why you need outer parentheses. Without them it will
be well defined compound literal and should work as is.

> +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..50c7ea467795 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,22 @@ 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)) {

A nit:
I prefer form of

	r = cio2_check_fwnode_graph(fwnode);
	if (!r) { // alternatively if (r == 0), depends on maintainer's taste

> +		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 62187ab5ae43..dc3e343a37fb 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> @@ -455,4 +455,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; }

static inline?

> +#endif
> +
>  #endif
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 13/15] acpi: Add acpi_dev_get_next_match_dev() and helper macro

[Andy Shevchenko]

On Sun, Jan 03, 2021 at 11:12:33PM +0000, Daniel Scally wrote:
> 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.

I guess we need Rafael's blessing on this.

> 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 v4:
> 
> 	- None
> 
>  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
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 13/15] acpi: Add acpi_dev_get_next_match_dev() and helper macro

[Daniel Scally]

On 04/01/2021 12:42, Andy Shevchenko wrote:
> On Sun, Jan 03, 2021 at 11:12:33PM +0000, Daniel Scally wrote:
>> 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.
> I guess we need Rafael's blessing on this.
For this one yes

Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Daniel Scally]

Hi Andy

On 04/01/2021 12:09, Andy Shevchenko wrote:
> On Sun, Jan 03, 2021 at 11:12:35PM +0000, Daniel Scally 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 (I consider it's good enough as is, though).
Thanks!
>> +static int cio2_bridge_connect_sensor(const struct cio2_sensor_config *cfg,
>> +				      struct cio2_bridge *bridge,
>> +				      struct pci_dev *cio2)
>> +{
>> +	struct fwnode_handle *fwnode;
>> +	struct cio2_sensor *sensor;
>> +	struct acpi_device *adev;
>> +	int ret;
>> +	for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> (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;
> A nit: this would be better to be at (1) location.


Yep, agreed

>
> Then possible to factor out the rest of the body of this loop as well.
>
> (Also can be considered as a hint for the future improvement)
Yeah I can look at this, there will probably be some future changes
anyway as we discover more details about the data in the SSDB buffer and
so on
> 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..3ec4ed44aced
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Author: Dan Scally <djrscally@gmail.com> */
> +#ifndef __CIO2_BRIDGE_H
> +#define __CIO2_BRIDGE_H
> +
> +#include <linux/property.h>
> +#include <linux/types.h>
> +
> +#include "ipu3-cio2.h"
> +
> +#define CIO2_HID				"INT343E"
> +#define CIO2_MAX_LANES				4
> +#define MAX_NUM_LINK_FREQS			3
> +
> +#define CIO2_SENSOR_CONFIG(_HID, _NR, ...)	\
> +	{					\
> +		.hid = _HID,			\
> +		.nr_link_freqs = _NR,		\
> +		.link_freqs = { __VA_ARGS__ }	\
> +	}
> Perhaps also good to declare it as a compound literal.
>
> (Means to add (struct ...) to the initializer.
>
Yep ok
>> +#define NODE_SENSOR(_HID, _PROPS)		\
>> +	((const struct software_node) {		\
>> +		.name = _HID,			\
>> +		.properties = _PROPS,		\
>> +	})
>> +
>> +#define NODE_PORT(_PORT, _SENSOR_NODE)		\
>> +	((const struct software_node) {		\
>> +		.name = _PORT,			\
>> +		.parent = _SENSOR_NODE,		\
>> +	})
>> +
>> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\
>> +	((const struct software_node) {		\
>> +		.name = _EP,			\
>> +		.parent = _PORT,		\
>> +		.properties = _PROPS,		\
>> +	})
> In all three I didn't get why you need outer parentheses. Without them it will
> be well defined compound literal and should work as is.
The code works fine, but checkpatch complains that macros with complex
values should be enclosed in parentheses. I guess now that I'm more
familiar with the code I'd call that a false-positive though, as nowhere
else in the kernel that I've seen encloses them the same way.
>>  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,22 @@ 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)) {
> A nit:
> I prefer form of
>
> 	r = cio2_check_fwnode_graph(fwnode);
> 	if (!r) { // alternatively if (r == 0), depends on maintainer's taste
This is fine by me; I can switch to that
>
>> +		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 62187ab5ae43..dc3e343a37fb 100644
>> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
>> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
>> @@ -455,4 +455,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; }
> static inline?

Ah, yes - thanks. Hadn't read about inline until now

Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Kieran Bingham]

Hi Dan,

On 03/01/2021 23:12, Daniel Scally 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.
> 
> Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v4:
> 
> 	- Added local definition of CIO2_MAX_LANES
> 	- Added some includes to cio2-bridge.h
> 	- Moved the inner loop of cio2_bridge_connect_sensors() to a
> 	  standalone function
> 	- Altered macros to make explicit assignments to members rather
> 	  than relying on position
> 	- A couple of minor format changes, mostly line wrapping
> 
>  MAINTAINERS                                   |   1 +
>  drivers/media/pci/intel/ipu3/Kconfig          |  18 ++
>  drivers/media/pci/intel/ipu3/Makefile         |   1 +
>  drivers/media/pci/intel/ipu3/cio2-bridge.c    | 302 ++++++++++++++++++
>  drivers/media/pci/intel/ipu3/cio2-bridge.h    | 125 ++++++++
>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  33 ++
>  drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   6 +
>  7 files changed, 486 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..96a2231b16ad 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 the 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..3a7bedb08f66
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> @@ -0,0 +1,302 @@
> +// 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 IDs 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),

I don't know if these are expressed anywhere else but would it be
helpful to add a comment, or indicator as to what the actual sensor is
that is represented by this HID?

I can make an assumption about what an OVTI2680 might be, but the
INT33BE is quite opaque. It's not clear what support that adds.

Unless no one cares what the sensor is that is, but I would anticipate
anyone looking here to add a new sensor might want to investigate what
was already in the table?



> +};
> +
> +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 < CIO2_MAX_LANES; i++)
> +		sensor->data_lanes[i] = i + 1;

Does something support lane swapping somewhere?
I assume this is just mapping each lane directly through.

Otherwise, I'm quite looking forwards to all of this ;-)

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

> +
> +	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),
> +		 SWNODE_GRAPH_PORT_NAME_FMT, sensor->ssdb.link);
> +	snprintf(sensor->node_names.port,
> +		 sizeof(sensor->node_names.port),
> +		 SWNODE_GRAPH_PORT_NAME_FMT, 0); /* Always port 0 */
> +	snprintf(sensor->node_names.endpoint,
> +		 sizeof(sensor->node_names.endpoint),
> +		 SWNODE_GRAPH_ENDPOINT_NAME_FMT, 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_sensor(const struct cio2_sensor_config *cfg,
> +				      struct cio2_bridge *bridge,
> +				      struct pci_dev *cio2)
> +{
> +	struct fwnode_handle *fwnode;
> +	struct cio2_sensor *sensor;
> +	struct acpi_device *adev;
> +	int ret;
> +
> +	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 > 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 0;
> +
> +err_free_swnodes:
> +	software_node_unregister_nodes(sensor->swnodes);
> +err_put_adev:
> +	acpi_dev_put(sensor->adev);
> +err_out:
> +	return ret;
> +}
> +
> +static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge,
> +				       struct pci_dev *cio2)
> +{
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
> +		const struct cio2_sensor_config *cfg = &cio2_supported_sensors[i];
> +
> +		ret = cio2_bridge_connect_sensor(cfg, bridge, cio2);
> +		if (ret)
> +			goto err_unregister_sensors;
> +	}
> +
> +	return 0;
> +
> +err_unregister_sensors:
> +	cio2_bridge_unregister_sensors(bridge);
> +	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..3ec4ed44aced
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Author: Dan Scally <djrscally@gmail.com> */
> +#ifndef __CIO2_BRIDGE_H
> +#define __CIO2_BRIDGE_H
> +
> +#include <linux/property.h>
> +#include <linux/types.h>
> +
> +#include "ipu3-cio2.h"
> +
> +#define CIO2_HID				"INT343E"
> +#define CIO2_MAX_LANES				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) {		\
> +		.name = _PORT,			\
> +		.parent = _SENSOR_NODE,		\
> +	})
> +
> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\
> +	((const struct software_node) {		\
> +		.name = _EP,			\
> +		.parent = _PORT,		\
> +		.properties = _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..50c7ea467795 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,22 @@ 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 62187ab5ae43..dc3e343a37fb 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> @@ -455,4 +455,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
> 

Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Andy Shevchenko]

On Mon, Jan 4, 2021 at 3:02 PM Daniel Scally <djrscally@gmail.com> wrote:
> On 04/01/2021 12:09, Andy Shevchenko wrote:
> > On Sun, Jan 03, 2021 at 11:12:35PM +0000, Daniel Scally wrote:

...

> >> +#define NODE_SENSOR(_HID, _PROPS)           \
> >> +    ((const struct software_node) {         \
> >> +            .name = _HID,                   \
> >> +            .properties = _PROPS,           \
> >> +    })
> >> +
> >> +#define NODE_PORT(_PORT, _SENSOR_NODE)              \
> >> +    ((const struct software_node) {         \
> >> +            .name = _PORT,                  \
> >> +            .parent = _SENSOR_NODE,         \
> >> +    })
> >> +
> >> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)   \
> >> +    ((const struct software_node) {         \
> >> +            .name = _EP,                    \
> >> +            .parent = _PORT,                \
> >> +            .properties = _PROPS,           \
> >> +    })
> > In all three I didn't get why you need outer parentheses. Without them it will
> > be well defined compound literal and should work as is.
> The code works fine, but checkpatch complains that macros with complex
> values should be enclosed in parentheses. I guess now that I'm more
> familiar with the code I'd call that a false-positive though, as nowhere
> else in the kernel that I've seen encloses them the same way.

I guess it is yet another false positive from checkpatch.
I would ignore its complaints.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Daniel Scally]

Hi Kieran

On 04/01/2021 13:35, Kieran Bingham wrote:
>> +/*
>> + * Extend this array with ACPI Hardware IDs 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),
> I don't know if these are expressed anywhere else but would it be
> helpful to add a comment, or indicator as to what the actual sensor is
> that is represented by this HID?
>
> I can make an assumption about what an OVTI2680 might be, but the
> INT33BE is quite opaque. It's not clear what support that adds.
>
> Unless no one cares what the sensor is that is, but I would anticipate
> anyone looking here to add a new sensor might want to investigate what
> was already in the table?

Yeah good point. I'll probably alternate comment and entry then, like:


+static const struct cio2_sensor_config cio2_supported_sensors[] = {
+	/* Sensor OVTI5693 */
+	CIO2_SENSOR_CONFIG("INT33BE", 0),
+	/* Sensor OVTI2680 */
+	CIO2_SENSOR_CONFIG("OVTI2680", 0),

As an inline comment won't fit for the sensors that we know link-frequencies for. That sound ok?

>> +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 < CIO2_MAX_LANES; i++)
>> +		sensor->data_lanes[i] = i + 1;
> Does something support lane swapping somewhere?
> I assume this is just mapping each lane directly through.

I think Sakari said remapping isn't supported in the CIO2 - so yeah this
is just mapping them directly

> Otherwise, I'm quite looking forwards to all of this ;-)
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Thanks very much!

Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Daniel Scally]

On 04/01/2021 13:38, Andy Shevchenko wrote:
> On Mon, Jan 4, 2021 at 3:02 PM Daniel Scally <djrscally@gmail.com> wrote:
>> On 04/01/2021 12:09, Andy Shevchenko wrote:
>>> On Sun, Jan 03, 2021 at 11:12:35PM +0000, Daniel Scally wrote:
> ...
>
>>>> +#define NODE_SENSOR(_HID, _PROPS)           \
>>>> +    ((const struct software_node) {         \
>>>> +            .name = _HID,                   \
>>>> +            .properties = _PROPS,           \
>>>> +    })
>>>> +
>>>> +#define NODE_PORT(_PORT, _SENSOR_NODE)              \
>>>> +    ((const struct software_node) {         \
>>>> +            .name = _PORT,                  \
>>>> +            .parent = _SENSOR_NODE,         \
>>>> +    })
>>>> +
>>>> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)   \
>>>> +    ((const struct software_node) {         \
>>>> +            .name = _EP,                    \
>>>> +            .parent = _PORT,                \
>>>> +            .properties = _PROPS,           \
>>>> +    })
>>> In all three I didn't get why you need outer parentheses. Without them it will
>>> be well defined compound literal and should work as is.
>> The code works fine, but checkpatch complains that macros with complex
>> values should be enclosed in parentheses. I guess now that I'm more
>> familiar with the code I'd call that a false-positive though, as nowhere
>> else in the kernel that I've seen encloses them the same way.
> I guess it is yet another false positive from checkpatch.
> I would ignore its complaints.
Will do so then

Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Andy Shevchenko]

On Mon, Jan 4, 2021 at 3:55 PM Daniel Scally <djrscally@gmail.com> wrote:
> On 04/01/2021 13:35, Kieran Bingham wrote:

...

> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
> +       /* Sensor OVTI5693 */
> +       CIO2_SENSOR_CONFIG("INT33BE", 0),
> +       /* Sensor OVTI2680 */
> +       CIO2_SENSOR_CONFIG("OVTI2680", 0),
>
> As an inline comment won't fit for the sensors that we know link-frequencies for. That sound ok?

At least to me it looks okay.

It seems for v5 we need Rafael's blessing on a few patches (driver
properties / swnode / ACPI) and we are fine.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 14/15] include: media: v4l2-fwnode: Include v4l2_fwnode_bus_type

[Andy Shevchenko]

On Sun, Jan 03, 2021 at 11:12:34PM +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.


Since it seems v5 will be the case, can you drop 'include: ' prefix in the
Subject line?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 07/15] include: fwnode.h: Define format macros for ports and endpoints

[Andy Shevchenko]

On Sun, Jan 03, 2021 at 11:12:27PM +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.

Since v5, can you modify subject prefix to be "device property: "?
Same for patches 3 and 4, please.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 07/15] include: fwnode.h: Define format macros for ports and endpoints

[Daniel Scally]

On 04/01/2021 14:24, Andy Shevchenko wrote:
> On Sun, Jan 03, 2021 at 11:12:27PM +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.
> Since v5, can you modify subject prefix to be "device property: "?
> Same for patches 3 and 4, please.
>
Will do, and for your last email too.

Re: [PATCH v4 13/15] acpi: Add acpi_dev_get_next_match_dev() and helper macro

[Andy Shevchenko]

On Sun, Jan 03, 2021 at 11:12:33PM +0000, Daniel Scally wrote:
> 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.

Since v5, also please update prefix in the Subject here to be "ACPI / bus: ".

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Kieran Bingham]

Hi Dan,

On 04/01/2021 13:55, Daniel Scally wrote:
> Hi Kieran
> 
> On 04/01/2021 13:35, Kieran Bingham wrote:
>>> +/*
>>> + * Extend this array with ACPI Hardware IDs 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),
>> I don't know if these are expressed anywhere else but would it be
>> helpful to add a comment, or indicator as to what the actual sensor is
>> that is represented by this HID?
>>
>> I can make an assumption about what an OVTI2680 might be, but the
>> INT33BE is quite opaque. It's not clear what support that adds.
>>
>> Unless no one cares what the sensor is that is, but I would anticipate
>> anyone looking here to add a new sensor might want to investigate what
>> was already in the table?
> 
> Yeah good point. I'll probably alternate comment and entry then, like:
> 
> 
> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
> +	/* Sensor OVTI5693 */
> +	CIO2_SENSOR_CONFIG("INT33BE", 0),
> +	/* Sensor OVTI2680 */
> +	CIO2_SENSOR_CONFIG("OVTI2680", 0),
> 
> As an inline comment won't fit for the sensors that we know link-frequencies for. That sound ok?

I might put the whole vendor name in, and no need to prefix 'Sensor' IMO.

+	/* Omnivision OV5693 */
+	CIO2_SENSOR_CONFIG("INT33BE", 0),
+	/* Omnivision OV2680 */
+	CIO2_SENSOR_CONFIG("OVTI2680", 0),

but otherwise, yes a comment the line before works for me, as you are
right - at the end would not be practical.


>>> +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 < CIO2_MAX_LANES; i++)
>>> +		sensor->data_lanes[i] = i + 1;
>> Does something support lane swapping somewhere?
>> I assume this is just mapping each lane directly through.
> 
> I think Sakari said remapping isn't supported in the CIO2 - so yeah this
> is just mapping them directly

So is this needed? Or is it some future compatibility thing?

I haven't seen where it's used yet, but I'm not too worried about it
though, just not sure what value an array of [1, 2, 3, 4] gives if it's
constant...


>> Otherwise, I'm quite looking forwards to all of this ;-)
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Thanks very much!

--
Kieran




-- 
Regards
--
Kieran

Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Daniel Scally]

Hi Kieran

On 04/01/2021 15:13, Kieran Bingham wrote:
> Hi Dan,
>
> On 04/01/2021 13:55, Daniel Scally wrote:
>> Hi Kieran
>>
>> On 04/01/2021 13:35, Kieran Bingham wrote:
>>>> +/*
>>>> + * Extend this array with ACPI Hardware IDs 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),
>>> I don't know if these are expressed anywhere else but would it be
>>> helpful to add a comment, or indicator as to what the actual sensor is
>>> that is represented by this HID?
>>>
>>> I can make an assumption about what an OVTI2680 might be, but the
>>> INT33BE is quite opaque. It's not clear what support that adds.
>>>
>>> Unless no one cares what the sensor is that is, but I would anticipate
>>> anyone looking here to add a new sensor might want to investigate what
>>> was already in the table?
>> Yeah good point. I'll probably alternate comment and entry then, like:
>>
>>
>> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
>> +	/* Sensor OVTI5693 */
>> +	CIO2_SENSOR_CONFIG("INT33BE", 0),
>> +	/* Sensor OVTI2680 */
>> +	CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>
>> As an inline comment won't fit for the sensors that we know link-frequencies for. That sound ok?
> I might put the whole vendor name in, and no need to prefix 'Sensor' IMO.
>
> +	/* Omnivision OV5693 */
> +	CIO2_SENSOR_CONFIG("INT33BE", 0),
> +	/* Omnivision OV2680 */
> +	CIO2_SENSOR_CONFIG("OVTI2680", 0),
>
> but otherwise, yes a comment the line before works for me, as you are
> right - at the end would not be practical.
Works for me
>>>> +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 < CIO2_MAX_LANES; i++)
>>>> +		sensor->data_lanes[i] = i + 1;
>>> Does something support lane swapping somewhere?
>>> I assume this is just mapping each lane directly through.
>> I think Sakari said remapping isn't supported in the CIO2 - so yeah this
>> is just mapping them directly
> So is this needed? Or is it some future compatibility thing?
>
> I haven't seen where it's used yet, but I'm not too worried about it
> though, just not sure what value an array of [1, 2, 3, 4] gives if it's
> constant...

The endpoints need to have the data-lanes property which passes an array
of data lanes, but there may well be a better way of doing this. I'm
just using the lanes member of the ssdb data structure to tell the
property how many members of the array to look at:


+    sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
+                    sensor->prop_names.data_lanes,
+                    sensor->data_lanes,
+                    sensor->ssdb.lanes);


So if sensor->ssdb.lanes is 2, even though it's passed a pointer to the
first member of an array of 4 members, the size calculation of that
macro limits it to just those in use. I.E. if sensor->ssdb.lanes is 2
then the property will be given the size 2 * sizeof(u32), and so when
its parsed only [1, 2] will be read.

Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Kieran Bingham]

Hi Dan,

On 04/01/2021 15:31, Daniel Scally wrote:
> Hi Kieran
> 
> On 04/01/2021 15:13, Kieran Bingham wrote:
>> Hi Dan,
>>
>> On 04/01/2021 13:55, Daniel Scally wrote:
>>> Hi Kieran
>>>
>>> On 04/01/2021 13:35, Kieran Bingham wrote:
>>>>> +/*
>>>>> + * Extend this array with ACPI Hardware IDs 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),
>>>> I don't know if these are expressed anywhere else but would it be
>>>> helpful to add a comment, or indicator as to what the actual sensor is
>>>> that is represented by this HID?
>>>>
>>>> I can make an assumption about what an OVTI2680 might be, but the
>>>> INT33BE is quite opaque. It's not clear what support that adds.
>>>>
>>>> Unless no one cares what the sensor is that is, but I would anticipate
>>>> anyone looking here to add a new sensor might want to investigate what
>>>> was already in the table?
>>> Yeah good point. I'll probably alternate comment and entry then, like:
>>>
>>>
>>> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
>>> +	/* Sensor OVTI5693 */
>>> +	CIO2_SENSOR_CONFIG("INT33BE", 0),
>>> +	/* Sensor OVTI2680 */
>>> +	CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>>
>>> As an inline comment won't fit for the sensors that we know link-frequencies for. That sound ok?
>> I might put the whole vendor name in, and no need to prefix 'Sensor' IMO.
>>
>> +	/* Omnivision OV5693 */
>> +	CIO2_SENSOR_CONFIG("INT33BE", 0),
>> +	/* Omnivision OV2680 */
>> +	CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>
>> but otherwise, yes a comment the line before works for me, as you are
>> right - at the end would not be practical.
> Works for me
>>>>> +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 < CIO2_MAX_LANES; i++)
>>>>> +		sensor->data_lanes[i] = i + 1;
>>>> Does something support lane swapping somewhere?
>>>> I assume this is just mapping each lane directly through.
>>> I think Sakari said remapping isn't supported in the CIO2 - so yeah this
>>> is just mapping them directly
>> So is this needed? Or is it some future compatibility thing?
>>
>> I haven't seen where it's used yet, but I'm not too worried about it
>> though, just not sure what value an array of [1, 2, 3, 4] gives if it's
>> constant...
> 
> The endpoints need to have the data-lanes property which passes an array
> of data lanes, but there may well be a better way of doing this. I'm
> just using the lanes member of the ssdb data structure to tell the
> property how many members of the array to look at:
> 
> 
> +    sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
> +                    sensor->prop_names.data_lanes,
> +                    sensor->data_lanes,
> +                    sensor->ssdb.lanes);
> 
> 
> So if sensor->ssdb.lanes is 2, even though it's passed a pointer to the
> first member of an array of 4 members, the size calculation of that
> macro limits it to just those in use. I.E. if sensor->ssdb.lanes is 2
> then the property will be given the size 2 * sizeof(u32), and so when
> its parsed only [1, 2] will be read.


Aha, I see, ok - so we are populating an array of [1, 2, 3, 4] for each
sensor that we add.

What about creating the data_lanes once as a const static array and
mapping to that?

/*
 * Map the lane arrangement, which is fixed for the IPU3.
 */
static const int data_lanes[CIO2_MAX_LANES] = { 1, 2, 3, 4 };


...

   sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
                    sensor->prop_names.data_lanes,
                    data_lanes,
                    sensor->ssdb.lanes);
...

Then we don't need the loop to populate the array for each sensor
anymore, or the data_lanes in the sensor struct?

-- 
Regards
--
Kieran

Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Daniel Scally]

Hi Kieran

On 04/01/2021 16:13, Kieran Bingham wrote:
> Hi Dan,
> 
> On 04/01/2021 15:31, Daniel Scally wrote:
>> Hi Kieran
>>
>> On 04/01/2021 15:13, Kieran Bingham wrote:
>>> Hi Dan,
>>>
>>> On 04/01/2021 13:55, Daniel Scally wrote:
>>>> Hi Kieran
>>>>
>>>> On 04/01/2021 13:35, Kieran Bingham wrote:
>>>>>> +/*
>>>>>> + * Extend this array with ACPI Hardware IDs 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),
>>>>> I don't know if these are expressed anywhere else but would it be
>>>>> helpful to add a comment, or indicator as to what the actual sensor is
>>>>> that is represented by this HID?
>>>>>
>>>>> I can make an assumption about what an OVTI2680 might be, but the
>>>>> INT33BE is quite opaque. It's not clear what support that adds.
>>>>>
>>>>> Unless no one cares what the sensor is that is, but I would anticipate
>>>>> anyone looking here to add a new sensor might want to investigate what
>>>>> was already in the table?
>>>> Yeah good point. I'll probably alternate comment and entry then, like:
>>>>
>>>>
>>>> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
>>>> +	/* Sensor OVTI5693 */
>>>> +	CIO2_SENSOR_CONFIG("INT33BE", 0),
>>>> +	/* Sensor OVTI2680 */
>>>> +	CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>>>
>>>> As an inline comment won't fit for the sensors that we know link-frequencies for. That sound ok?
>>> I might put the whole vendor name in, and no need to prefix 'Sensor' IMO.
>>>
>>> +	/* Omnivision OV5693 */
>>> +	CIO2_SENSOR_CONFIG("INT33BE", 0),
>>> +	/* Omnivision OV2680 */
>>> +	CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>>
>>> but otherwise, yes a comment the line before works for me, as you are
>>> right - at the end would not be practical.
>> Works for me
>>>>>> +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 < CIO2_MAX_LANES; i++)
>>>>>> +		sensor->data_lanes[i] = i + 1;
>>>>> Does something support lane swapping somewhere?
>>>>> I assume this is just mapping each lane directly through.
>>>> I think Sakari said remapping isn't supported in the CIO2 - so yeah this
>>>> is just mapping them directly
>>> So is this needed? Or is it some future compatibility thing?
>>>
>>> I haven't seen where it's used yet, but I'm not too worried about it
>>> though, just not sure what value an array of [1, 2, 3, 4] gives if it's
>>> constant...
>>
>> The endpoints need to have the data-lanes property which passes an array
>> of data lanes, but there may well be a better way of doing this. I'm
>> just using the lanes member of the ssdb data structure to tell the
>> property how many members of the array to look at:
>>
>>
>> +    sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
>> +                    sensor->prop_names.data_lanes,
>> +                    sensor->data_lanes,
>> +                    sensor->ssdb.lanes);
>>
>>
>> So if sensor->ssdb.lanes is 2, even though it's passed a pointer to the
>> first member of an array of 4 members, the size calculation of that
>> macro limits it to just those in use. I.E. if sensor->ssdb.lanes is 2
>> then the property will be given the size 2 * sizeof(u32), and so when
>> its parsed only [1, 2] will be read.
> 
> 
> Aha, I see, ok - so we are populating an array of [1, 2, 3, 4] for each
> sensor that we add.
> 
> What about creating the data_lanes once as a const static array and
> mapping to that?
> 
> /*
>  * Map the lane arrangement, which is fixed for the IPU3.
>  */
> static const int data_lanes[CIO2_MAX_LANES] = { 1, 2, 3, 4 };


Can't do exactly this; the bridge needs to store everything on heap
incase the module is unloaded, but I could move the data_lanes array to
the struct cio2_bridge instead of against each sensor and then we're
only doing it once.

> ...
> 
>    sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
>                     sensor->prop_names.data_lanes,
>                     data_lanes,
>                     sensor->ssdb.lanes);
> ...
> 
> Then we don't need the loop to populate the array for each sensor
> anymore, or the data_lanes in the sensor struct?
> 

Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Kieran Bingham]

Hi Dan,

On 04/01/2021 22:02, Daniel Scally wrote:
>>>>> On 04/01/2021 13:35, Kieran Bingham wrote:
>>>>>>> +/*
>>>>>>> + * Extend this array with ACPI Hardware IDs 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),
>>>>>> I don't know if these are expressed anywhere else but would it be
>>>>>> helpful to add a comment, or indicator as to what the actual sensor is
>>>>>> that is represented by this HID?
>>>>>>
>>>>>> I can make an assumption about what an OVTI2680 might be, but the
>>>>>> INT33BE is quite opaque. It's not clear what support that adds.
>>>>>>
>>>>>> Unless no one cares what the sensor is that is, but I would anticipate
>>>>>> anyone looking here to add a new sensor might want to investigate what
>>>>>> was already in the table?
>>>>> Yeah good point. I'll probably alternate comment and entry then, like:
>>>>>
>>>>>
>>>>> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
>>>>> +	/* Sensor OVTI5693 */
>>>>> +	CIO2_SENSOR_CONFIG("INT33BE", 0),
>>>>> +	/* Sensor OVTI2680 */
>>>>> +	CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>>>>
>>>>> As an inline comment won't fit for the sensors that we know link-frequencies for. That sound ok?
>>>> I might put the whole vendor name in, and no need to prefix 'Sensor' IMO.
>>>>
>>>> +	/* Omnivision OV5693 */
>>>> +	CIO2_SENSOR_CONFIG("INT33BE", 0),
>>>> +	/* Omnivision OV2680 */
>>>> +	CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>>>
>>>> but otherwise, yes a comment the line before works for me, as you are
>>>> right - at the end would not be practical.
>>> Works for me
>>>>>>> +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 < CIO2_MAX_LANES; i++)
>>>>>>> +		sensor->data_lanes[i] = i + 1;
>>>>>> Does something support lane swapping somewhere?
>>>>>> I assume this is just mapping each lane directly through.
>>>>> I think Sakari said remapping isn't supported in the CIO2 - so yeah this
>>>>> is just mapping them directly
>>>> So is this needed? Or is it some future compatibility thing?
>>>>
>>>> I haven't seen where it's used yet, but I'm not too worried about it
>>>> though, just not sure what value an array of [1, 2, 3, 4] gives if it's
>>>> constant...
>>>
>>> The endpoints need to have the data-lanes property which passes an array
>>> of data lanes, but there may well be a better way of doing this. I'm
>>> just using the lanes member of the ssdb data structure to tell the
>>> property how many members of the array to look at:
>>>
>>>
>>> +    sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
>>> +                    sensor->prop_names.data_lanes,
>>> +                    sensor->data_lanes,
>>> +                    sensor->ssdb.lanes);
>>>
>>>
>>> So if sensor->ssdb.lanes is 2, even though it's passed a pointer to the
>>> first member of an array of 4 members, the size calculation of that
>>> macro limits it to just those in use. I.E. if sensor->ssdb.lanes is 2
>>> then the property will be given the size 2 * sizeof(u32), and so when
>>> its parsed only [1, 2] will be read.
>>
>>
>> Aha, I see, ok - so we are populating an array of [1, 2, 3, 4] for each
>> sensor that we add.
>>
>> What about creating the data_lanes once as a const static array and
>> mapping to that?
>>
>> /*
>>  * Map the lane arrangement, which is fixed for the IPU3.
>>  */
>> static const int data_lanes[CIO2_MAX_LANES] = { 1, 2, 3, 4 };
> 
> 
> Can't do exactly this; the bridge needs to store everything on heap
> incase the module is unloaded, but I could move the data_lanes array to
> the struct cio2_bridge instead of against each sensor and then we're
> only doing it once.

Ahh, yes I remember reading about that already.

It maybe worth adding a comment about that in this file, to prevent
other people from 'optimising' things out in 5 years ...

It probably doesn't make much difference in that case if it's per sensor
or per bridge. But indeed at least in the bridge it's only created once.

--
Kieran


> 
>> ...
>>
>>    sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
>>                     sensor->prop_names.data_lanes,
>>                     data_lanes,
>>                     sensor->ssdb.lanes);
>> ...
>>
>> Then we don't need the loop to populate the array for each sensor
>> anymore, or the data_lanes in the sensor struct?
>>
> 

-- 
Regards
--
Kieran

Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Daniel Scally]

Morning Kieran

On 05/01/2021 06:55, Kieran Bingham wrote:
> Hi Dan,
>
> On 04/01/2021 22:02, Daniel Scally wrote:
>>>>>> On 04/01/2021 13:35, Kieran Bingham wrote:
>>>>>>>> +/*
>>>>>>>> + * Extend this array with ACPI Hardware IDs 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),
>>>>>>> I don't know if these are expressed anywhere else but would it be
>>>>>>> helpful to add a comment, or indicator as to what the actual sensor is
>>>>>>> that is represented by this HID?
>>>>>>>
>>>>>>> I can make an assumption about what an OVTI2680 might be, but the
>>>>>>> INT33BE is quite opaque. It's not clear what support that adds.
>>>>>>>
>>>>>>> Unless no one cares what the sensor is that is, but I would anticipate
>>>>>>> anyone looking here to add a new sensor might want to investigate what
>>>>>>> was already in the table?
>>>>>> Yeah good point. I'll probably alternate comment and entry then, like:
>>>>>>
>>>>>>
>>>>>> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
>>>>>> +	/* Sensor OVTI5693 */
>>>>>> +	CIO2_SENSOR_CONFIG("INT33BE", 0),
>>>>>> +	/* Sensor OVTI2680 */
>>>>>> +	CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>>>>>
>>>>>> As an inline comment won't fit for the sensors that we know link-frequencies for. That sound ok?
>>>>> I might put the whole vendor name in, and no need to prefix 'Sensor' IMO.
>>>>>
>>>>> +	/* Omnivision OV5693 */
>>>>> +	CIO2_SENSOR_CONFIG("INT33BE", 0),
>>>>> +	/* Omnivision OV2680 */
>>>>> +	CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>>>>
>>>>> but otherwise, yes a comment the line before works for me, as you are
>>>>> right - at the end would not be practical.
>>>> Works for me
>>>>>>>> +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 < CIO2_MAX_LANES; i++)
>>>>>>>> +		sensor->data_lanes[i] = i + 1;
>>>>>>> Does something support lane swapping somewhere?
>>>>>>> I assume this is just mapping each lane directly through.
>>>>>> I think Sakari said remapping isn't supported in the CIO2 - so yeah this
>>>>>> is just mapping them directly
>>>>> So is this needed? Or is it some future compatibility thing?
>>>>>
>>>>> I haven't seen where it's used yet, but I'm not too worried about it
>>>>> though, just not sure what value an array of [1, 2, 3, 4] gives if it's
>>>>> constant...
>>>> The endpoints need to have the data-lanes property which passes an array
>>>> of data lanes, but there may well be a better way of doing this. I'm
>>>> just using the lanes member of the ssdb data structure to tell the
>>>> property how many members of the array to look at:
>>>>
>>>>
>>>> +    sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
>>>> +                    sensor->prop_names.data_lanes,
>>>> +                    sensor->data_lanes,
>>>> +                    sensor->ssdb.lanes);
>>>>
>>>>
>>>> So if sensor->ssdb.lanes is 2, even though it's passed a pointer to the
>>>> first member of an array of 4 members, the size calculation of that
>>>> macro limits it to just those in use. I.E. if sensor->ssdb.lanes is 2
>>>> then the property will be given the size 2 * sizeof(u32), and so when
>>>> its parsed only [1, 2] will be read.
>>>
>>> Aha, I see, ok - so we are populating an array of [1, 2, 3, 4] for each
>>> sensor that we add.
>>>
>>> What about creating the data_lanes once as a const static array and
>>> mapping to that?
>>>
>>> /*
>>>  * Map the lane arrangement, which is fixed for the IPU3.
>>>  */
>>> static const int data_lanes[CIO2_MAX_LANES] = { 1, 2, 3, 4 };
>>
>> Can't do exactly this; the bridge needs to store everything on heap
>> incase the module is unloaded, but I could move the data_lanes array to
>> the struct cio2_bridge instead of against each sensor and then we're
>> only doing it once.
> Ahh, yes I remember reading about that already.
>
> It maybe worth adding a comment about that in this file, to prevent
> other people from 'optimising' things out in 5 years ...
>
> It probably doesn't make much difference in that case if it's per sensor
> or per bridge. But indeed at least in the bridge it's only created once.
Yep ok; I moved it there and I'll add a comment explaining why it's done
a bit weird.
> --
> Kieran
>
>
>>> ...
>>>
>>>    sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
>>>                     sensor->prop_names.data_lanes,
>>>                     data_lanes,
>>>                     sensor->ssdb.lanes);
>>> ...
>>>
>>> Then we don't need the loop to populate the array for each sensor
>>> anymore, or the data_lanes in the sensor struct?
>>>