[PATCH v5 0/7] Move device tree graph parsing helpers to drivers/of

[PATCH v5 0/7] Move device tree graph parsing helpers to drivers/of

[Philipp Zabel]

Hi,

this version of the OF graph helper move series addresses a few of
Grant's and Tomi's comments.

Changes since v4:
 - Moved graph helpers into drivers/of/base.c
 - Fixed endpoint parsing patch to update users
 - Improved documentation, emphasizing features that differentiate
   the graph bindings from simple phandle graphs. Made it clear that
   this is not necessarily specific to data connections
 - Added cleanups to of_graph_get_next_endpoint routine
 - Added simplified binding for single port devices

regards
Philipp

Philipp Zabel (7):
  [media] of: move graph helpers from drivers/media/v4l2-core to
    drivers/of
  Documentation: of: Document graph bindings
  of: Warn if of_graph_get_next_endpoint is called with the root node
  of: Reduce indentation in of_graph_get_next_endpoint
  [media] of: move common endpoint parsing to drivers/of
  of: Implement simplified graph binding for single port devices
  of: Document simplified graph binding for single port devices

 Documentation/devicetree/bindings/graph.txt   | 137 +++++++++++++++++++++
 drivers/media/i2c/adv7343.c                   |   4 +-
 drivers/media/i2c/mt9p031.c                   |   4 +-
 drivers/media/i2c/s5k5baf.c                   |   3 +-
 drivers/media/i2c/tvp514x.c                   |   3 +-
 drivers/media/i2c/tvp7002.c                   |   3 +-
 drivers/media/platform/exynos4-is/fimc-is.c   |   6 +-
 drivers/media/platform/exynos4-is/media-dev.c |  13 +-
 drivers/media/platform/exynos4-is/mipi-csis.c |   5 +-
 drivers/media/v4l2-core/v4l2-of.c             | 133 +--------------------
 drivers/of/base.c                             | 165 ++++++++++++++++++++++++++
 include/linux/of_graph.h                      |  66 +++++++++++
 include/media/v4l2-of.h                       |  33 +-----
 13 files changed, 397 insertions(+), 178 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/graph.txt
 create mode 100644 include/linux/of_graph.h

-- 
1.8.5.3

[PATCH v5 1/7] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of

[Philipp Zabel]

This patch moves the parsing helpers used to parse connected graphs
in the device tree, like the video interface bindings documented in
Documentation/devicetree/bindings/media/video-interfaces.txt, from
drivers/media/v4l2-core/v4l2-of.c into drivers/of/base.c.

This allows to reuse the same parser code from outside the V4L2
framework, most importantly from display drivers.
The functions v4l2_of_get_next_endpoint, v4l2_of_get_remote_port,
and v4l2_of_get_remote_port_parent are moved. They are renamed to
of_graph_get_next_endpoint, of_graph_get_remote_port, and
of_graph_get_remote_port_parent, respectively.
Since there are not that many current users yet, switch all of
them to the new functions right away.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v4:
 - Moved into drivers/of/base.c instead of creating of_graph.c
---
 drivers/media/i2c/adv7343.c                   |   4 +-
 drivers/media/i2c/mt9p031.c                   |   4 +-
 drivers/media/i2c/s5k5baf.c                   |   3 +-
 drivers/media/i2c/tvp514x.c                   |   3 +-
 drivers/media/i2c/tvp7002.c                   |   3 +-
 drivers/media/platform/exynos4-is/fimc-is.c   |   6 +-
 drivers/media/platform/exynos4-is/media-dev.c |   3 +-
 drivers/media/platform/exynos4-is/mipi-csis.c |   3 +-
 drivers/media/v4l2-core/v4l2-of.c             | 117 -------------------------
 drivers/of/base.c                             | 118 ++++++++++++++++++++++++++
 include/linux/of_graph.h                      |  46 ++++++++++
 include/media/v4l2-of.h                       |  25 +-----
 12 files changed, 182 insertions(+), 153 deletions(-)
 create mode 100644 include/linux/of_graph.h

diff --git a/drivers/media/i2c/adv7343.c b/drivers/media/i2c/adv7343.c
index d4e15a6..9d38f7b 100644
--- a/drivers/media/i2c/adv7343.c
+++ b/drivers/media/i2c/adv7343.c
@@ -26,12 +26,12 @@
 #include <linux/videodev2.h>
 #include <linux/uaccess.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 
 #include <media/adv7343.h>
 #include <media/v4l2-async.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ctrls.h>
-#include <media/v4l2-of.h>
 
 #include "adv7343_regs.h"
 
@@ -410,7 +410,7 @@ adv7343_get_pdata(struct i2c_client *client)
 	if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
 		return client->dev.platform_data;
 
-	np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
+	np = of_graph_get_next_endpoint(client->dev.of_node, NULL);
 	if (!np)
 		return NULL;
 
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index e5ddf47..192c4aa 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -21,6 +21,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
+#include <linux/of_graph.h>
 #include <linux/pm.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
@@ -29,7 +30,6 @@
 #include <media/mt9p031.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
-#include <media/v4l2-of.h>
 #include <media/v4l2-subdev.h>
 
 #include "aptina-pll.h"
@@ -943,7 +943,7 @@ mt9p031_get_pdata(struct i2c_client *client)
 	if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
 		return client->dev.platform_data;
 
-	np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
+	np = of_graph_get_next_endpoint(client->dev.of_node, NULL);
 	if (!np)
 		return NULL;
 
diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
index 77e10e0..2d768ef 100644
--- a/drivers/media/i2c/s5k5baf.c
+++ b/drivers/media/i2c/s5k5baf.c
@@ -21,6 +21,7 @@
 #include <linux/media.h>
 #include <linux/module.h>
 #include <linux/of_gpio.h>
+#include <linux/of_graph.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
@@ -1855,7 +1856,7 @@ static int s5k5baf_parse_device_node(struct s5k5baf *state, struct device *dev)
 	if (ret < 0)
 		return ret;
 
-	node_ep = v4l2_of_get_next_endpoint(node, NULL);
+	node_ep = of_graph_get_next_endpoint(node, NULL);
 	if (!node_ep) {
 		dev_err(dev, "no endpoint defined at node %s\n",
 			node->full_name);
diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
index 83d85df..ca00117 100644
--- a/drivers/media/i2c/tvp514x.c
+++ b/drivers/media/i2c/tvp514x.c
@@ -36,6 +36,7 @@
 #include <linux/module.h>
 #include <linux/v4l2-mediabus.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 
 #include <media/v4l2-async.h>
 #include <media/v4l2-device.h>
@@ -1068,7 +1069,7 @@ tvp514x_get_pdata(struct i2c_client *client)
 	if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
 		return client->dev.platform_data;
 
-	endpoint = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
+	endpoint = of_graph_get_next_endpoint(client->dev.of_node, NULL);
 	if (!endpoint)
 		return NULL;
 
diff --git a/drivers/media/i2c/tvp7002.c b/drivers/media/i2c/tvp7002.c
index 912e1cc..c4e1e2c 100644
--- a/drivers/media/i2c/tvp7002.c
+++ b/drivers/media/i2c/tvp7002.c
@@ -30,6 +30,7 @@
 #include <linux/videodev2.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/v4l2-dv-timings.h>
 #include <media/tvp7002.h>
 #include <media/v4l2-async.h>
@@ -957,7 +958,7 @@ tvp7002_get_pdata(struct i2c_client *client)
 	if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
 		return client->dev.platform_data;
 
-	endpoint = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
+	endpoint = of_graph_get_next_endpoint(client->dev.of_node, NULL);
 	if (!endpoint)
 		return NULL;
 
diff --git a/drivers/media/platform/exynos4-is/fimc-is.c b/drivers/media/platform/exynos4-is/fimc-is.c
index 13a4228..9bdfa45 100644
--- a/drivers/media/platform/exynos4-is/fimc-is.c
+++ b/drivers/media/platform/exynos4-is/fimc-is.c
@@ -24,13 +24,13 @@
 #include <linux/i2c.h>
 #include <linux/of_irq.h>
 #include <linux/of_address.h>
+#include <linux/of_graph.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/videodev2.h>
-#include <media/v4l2-of.h>
 #include <media/videobuf2-dma-contig.h>
 
 #include "media-dev.h"
@@ -167,10 +167,10 @@ static int fimc_is_parse_sensor_config(struct fimc_is_sensor *sensor,
 	u32 tmp = 0;
 	int ret;
 
-	np = v4l2_of_get_next_endpoint(np, NULL);
+	np = of_graph_get_next_endpoint(np, NULL);
 	if (!np)
 		return -ENXIO;
-	np = v4l2_of_get_remote_port(np);
+	np = of_graph_get_remote_port(np);
 	if (!np)
 		return -ENXIO;
 
diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index c1bce17..d0f82da 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -20,6 +20,7 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/of_device.h>
+#include <linux/of_graph.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/types.h>
@@ -473,7 +474,7 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
 
 	pd->mux_id = (endpoint.port - 1) & 0x1;
 
-	rem = v4l2_of_get_remote_port_parent(ep);
+	rem = of_graph_get_remote_port_parent(ep);
 	of_node_put(ep);
 	if (rem == NULL) {
 		v4l2_info(&fmd->v4l2_dev, "Remote device at %s not found\n",
diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c
index f3c3591..fd1ae65 100644
--- a/drivers/media/platform/exynos4-is/mipi-csis.c
+++ b/drivers/media/platform/exynos4-is/mipi-csis.c
@@ -20,6 +20,7 @@
 #include <linux/memory.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_data/mipi-csis.h>
 #include <linux/platform_device.h>
@@ -762,7 +763,7 @@ static int s5pcsis_parse_dt(struct platform_device *pdev,
 				 &state->max_num_lanes))
 		return -EINVAL;
 
-	node = v4l2_of_get_next_endpoint(node, NULL);
+	node = of_graph_get_next_endpoint(node, NULL);
 	if (!node) {
 		dev_err(&pdev->dev, "No port node at %s\n",
 				pdev->dev.of_node->full_name);
diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
index 42e3e8a..f919db3 100644
--- a/drivers/media/v4l2-core/v4l2-of.c
+++ b/drivers/media/v4l2-core/v4l2-of.c
@@ -152,120 +152,3 @@ int v4l2_of_parse_endpoint(const struct device_node *node,
 	return 0;
 }
 EXPORT_SYMBOL(v4l2_of_parse_endpoint);
-
-/**
- * v4l2_of_get_next_endpoint() - get next endpoint node
- * @parent: pointer to the parent device node
- * @prev: previous endpoint node, or NULL to get first
- *
- * Return: An 'endpoint' node pointer with refcount incremented. Refcount
- * of the passed @prev node is not decremented, the caller have to use
- * of_node_put() on it when done.
- */
-struct device_node *v4l2_of_get_next_endpoint(const struct device_node *parent,
-					struct device_node *prev)
-{
-	struct device_node *endpoint;
-	struct device_node *port = NULL;
-
-	if (!parent)
-		return NULL;
-
-	if (!prev) {
-		struct device_node *node;
-		/*
-		 * It's the first call, we have to find a port subnode
-		 * within this node or within an optional 'ports' node.
-		 */
-		node = of_get_child_by_name(parent, "ports");
-		if (node)
-			parent = node;
-
-		port = of_get_child_by_name(parent, "port");
-
-		if (port) {
-			/* Found a port, get an endpoint. */
-			endpoint = of_get_next_child(port, NULL);
-			of_node_put(port);
-		} else {
-			endpoint = NULL;
-		}
-
-		if (!endpoint)
-			pr_err("%s(): no endpoint nodes specified for %s\n",
-			       __func__, parent->full_name);
-		of_node_put(node);
-	} else {
-		port = of_get_parent(prev);
-		if (!port)
-			/* Hm, has someone given us the root node ?... */
-			return NULL;
-
-		/* Avoid dropping prev node refcount to 0. */
-		of_node_get(prev);
-		endpoint = of_get_next_child(port, prev);
-		if (endpoint) {
-			of_node_put(port);
-			return endpoint;
-		}
-
-		/* No more endpoints under this port, try the next one. */
-		do {
-			port = of_get_next_child(parent, port);
-			if (!port)
-				return NULL;
-		} while (of_node_cmp(port->name, "port"));
-
-		/* Pick up the first endpoint in this port. */
-		endpoint = of_get_next_child(port, NULL);
-		of_node_put(port);
-	}
-
-	return endpoint;
-}
-EXPORT_SYMBOL(v4l2_of_get_next_endpoint);
-
-/**
- * v4l2_of_get_remote_port_parent() - get remote port's parent node
- * @node: pointer to a local endpoint device_node
- *
- * Return: Remote device node associated with remote endpoint node linked
- *	   to @node. Use of_node_put() on it when done.
- */
-struct device_node *v4l2_of_get_remote_port_parent(
-			       const struct device_node *node)
-{
-	struct device_node *np;
-	unsigned int depth;
-
-	/* Get remote endpoint node. */
-	np = of_parse_phandle(node, "remote-endpoint", 0);
-
-	/* Walk 3 levels up only if there is 'ports' node. */
-	for (depth = 3; depth && np; depth--) {
-		np = of_get_next_parent(np);
-		if (depth == 2 && of_node_cmp(np->name, "ports"))
-			break;
-	}
-	return np;
-}
-EXPORT_SYMBOL(v4l2_of_get_remote_port_parent);
-
-/**
- * v4l2_of_get_remote_port() - get remote port node
- * @node: pointer to a local endpoint device_node
- *
- * Return: Remote port node associated with remote endpoint node linked
- *	   to @node. Use of_node_put() on it when done.
- */
-struct device_node *v4l2_of_get_remote_port(const struct device_node *node)
-{
-	struct device_node *np;
-
-	/* Get remote endpoint node. */
-	np = of_parse_phandle(node, "remote-endpoint", 0);
-	if (!np)
-		return NULL;
-	return of_get_next_parent(np);
-}
-EXPORT_SYMBOL(v4l2_of_get_remote_port);
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 89e888a..b2f223f 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -21,6 +21,7 @@
 #include <linux/cpu.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
 #include <linux/proc_fs.h>
@@ -1982,3 +1983,120 @@ struct device_node *of_find_next_cache_node(const struct device_node *np)
 
 	return NULL;
 }
+
+/**
+ * of_graph_get_next_endpoint() - get next endpoint node
+ * @parent: pointer to the parent device node
+ * @prev: previous endpoint node, or NULL to get first
+ *
+ * Return: An 'endpoint' node pointer with refcount incremented. Refcount
+ * of the passed @prev node is not decremented, the caller have to use
+ * of_node_put() on it when done.
+ */
+struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
+					struct device_node *prev)
+{
+	struct device_node *endpoint;
+	struct device_node *port = NULL;
+
+	if (!parent)
+		return NULL;
+
+	if (!prev) {
+		struct device_node *node;
+		/*
+		 * It's the first call, we have to find a port subnode
+		 * within this node or within an optional 'ports' node.
+		 */
+		node = of_get_child_by_name(parent, "ports");
+		if (node)
+			parent = node;
+
+		port = of_get_child_by_name(parent, "port");
+
+		if (port) {
+			/* Found a port, get an endpoint. */
+			endpoint = of_get_next_child(port, NULL);
+			of_node_put(port);
+		} else {
+			endpoint = NULL;
+		}
+
+		if (!endpoint)
+			pr_err("%s(): no endpoint nodes specified for %s\n",
+			       __func__, parent->full_name);
+		of_node_put(node);
+	} else {
+		port = of_get_parent(prev);
+		if (!port)
+			/* Hm, has someone given us the root node ?... */
+			return NULL;
+
+		/* Avoid dropping prev node refcount to 0. */
+		of_node_get(prev);
+		endpoint = of_get_next_child(port, prev);
+		if (endpoint) {
+			of_node_put(port);
+			return endpoint;
+		}
+
+		/* No more endpoints under this port, try the next one. */
+		do {
+			port = of_get_next_child(parent, port);
+			if (!port)
+				return NULL;
+		} while (of_node_cmp(port->name, "port"));
+
+		/* Pick up the first endpoint in this port. */
+		endpoint = of_get_next_child(port, NULL);
+		of_node_put(port);
+	}
+
+	return endpoint;
+}
+EXPORT_SYMBOL(of_graph_get_next_endpoint);
+
+/**
+ * of_graph_get_remote_port_parent() - get remote port's parent node
+ * @node: pointer to a local endpoint device_node
+ *
+ * Return: Remote device node associated with remote endpoint node linked
+ *	   to @node. Use of_node_put() on it when done.
+ */
+struct device_node *of_graph_get_remote_port_parent(
+			       const struct device_node *node)
+{
+	struct device_node *np;
+	unsigned int depth;
+
+	/* Get remote endpoint node. */
+	np = of_parse_phandle(node, "remote-endpoint", 0);
+
+	/* Walk 3 levels up only if there is 'ports' node. */
+	for (depth = 3; depth && np; depth--) {
+		np = of_get_next_parent(np);
+		if (depth == 2 && of_node_cmp(np->name, "ports"))
+			break;
+	}
+	return np;
+}
+EXPORT_SYMBOL(of_graph_get_remote_port_parent);
+
+/**
+ * of_graph_get_remote_port() - get remote port node
+ * @node: pointer to a local endpoint device_node
+ *
+ * Return: Remote port node associated with remote endpoint node linked
+ *	   to @node. Use of_node_put() on it when done.
+ */
+struct device_node *of_graph_get_remote_port(const struct device_node *node)
+{
+	struct device_node *np;
+
+	/* Get remote endpoint node. */
+	np = of_parse_phandle(node, "remote-endpoint", 0);
+	if (!np)
+		return NULL;
+	return of_get_next_parent(np);
+}
+EXPORT_SYMBOL(of_graph_get_remote_port);
diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
new file mode 100644
index 0000000..3bbeb60
--- /dev/null
+++ b/include/linux/of_graph.h
@@ -0,0 +1,46 @@
+/*
+ * OF graph binding parsing helpers
+ *
+ * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd.
+ * Author: Sylwester Nawrocki <s.nawrocki@samsung.com>
+ *
+ * Copyright (C) 2012 Renesas Electronics Corp.
+ * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ */
+#ifndef __LINUX_OF_GRAPH_H
+#define __LINUX_OF_GRAPH_H
+
+#ifdef CONFIG_OF
+struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
+					struct device_node *previous);
+struct device_node *of_graph_get_remote_port_parent(
+					const struct device_node *node);
+struct device_node *of_graph_get_remote_port(const struct device_node *node);
+#else
+
+static inline struct device_node *of_graph_get_next_endpoint(
+					const struct device_node *parent,
+					struct device_node *previous)
+{
+	return NULL;
+}
+
+static inline struct device_node *of_graph_get_remote_port_parent(
+					const struct device_node *node)
+{
+	return NULL;
+}
+
+static inline struct device_node *of_graph_get_remote_port(
+					const struct device_node *node)
+{
+	return NULL;
+}
+
+#endif /* CONFIG_OF */
+
+#endif /* __LINUX_OF_GRAPH_H */
diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
index 541cea4..3a49735 100644
--- a/include/media/v4l2-of.h
+++ b/include/media/v4l2-of.h
@@ -17,6 +17,7 @@
 #include <linux/list.h>
 #include <linux/types.h>
 #include <linux/errno.h>
+#include <linux/of_graph.h>
 
 #include <media/v4l2-mediabus.h>
 
@@ -72,11 +73,6 @@ struct v4l2_of_endpoint {
 #ifdef CONFIG_OF
 int v4l2_of_parse_endpoint(const struct device_node *node,
 			   struct v4l2_of_endpoint *endpoint);
-struct device_node *v4l2_of_get_next_endpoint(const struct device_node *parent,
-					struct device_node *previous);
-struct device_node *v4l2_of_get_remote_port_parent(
-					const struct device_node *node);
-struct device_node *v4l2_of_get_remote_port(const struct device_node *node);
 #else /* CONFIG_OF */
 
 static inline int v4l2_of_parse_endpoint(const struct device_node *node,
@@ -85,25 +81,6 @@ static inline int v4l2_of_parse_endpoint(const struct device_node *node,
 	return -ENOSYS;
 }
 
-static inline struct device_node *v4l2_of_get_next_endpoint(
-					const struct device_node *parent,
-					struct device_node *previous)
-{
-	return NULL;
-}
-
-static inline struct device_node *v4l2_of_get_remote_port_parent(
-					const struct device_node *node)
-{
-	return NULL;
-}
-
-static inline struct device_node *v4l2_of_get_remote_port(
-					const struct device_node *node)
-{
-	return NULL;
-}
-
 #endif /* CONFIG_OF */
 
 #endif /* _V4L2_OF_H */
-- 
1.8.5.3

[PATCH v5 2/7] Documentation: of: Document graph bindings

[Philipp Zabel]

The device tree graph bindings as used by V4L2 and documented in
Documentation/device-tree/bindings/media/video-interfaces.txt contain
generic parts that are not media specific but could be useful for any
subsystem with data flow between multiple devices. This document
describe the generic bindings.

Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
Changes since v4:
 - Differentiate from graphs made by simple phandle links
 - Do not mention data flow except in video-interfaces example
 - 
---
 Documentation/devicetree/bindings/graph.txt | 129 ++++++++++++++++++++++++++++
 1 file changed, 129 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/graph.txt

diff --git a/Documentation/devicetree/bindings/graph.txt b/Documentation/devicetree/bindings/graph.txt
new file mode 100644
index 0000000..554865b
--- /dev/null
+++ b/Documentation/devicetree/bindings/graph.txt
@@ -0,0 +1,129 @@
+Common bindings for device graphs
+
+General concept
+---------------
+
+The hierarchical organisation of the device tree is well suited to describe
+control flow to devices, but there can be more complex connections between
+devices that work together to form a logical compound device, following an
+arbitrarily complex graph.
+There already is a simple directed graph between devices tree nodes using
+phandle properties pointing to other nodes to describe connections that
+can not be inferred from device tree parent-child relationships. The device
+tree graph bindings described herein abstract more complex devices that can
+have multiple specifiable ports, each of which can be linked to one or more
+ports of other devices.
+
+These common bindings do not contain any information about the direction of
+type of the connections, they just map their existence. Specific properties
+may be described by specialized bindings depending on the type of connection.
+
+To see how this binding applies to video pipelines, for example, see
+Documentation/device-tree/bindings/media/video-interfaces.txt.
+Here the ports describe data interfaces, and the links between them are
+the connecting data buses. A single port with multiple connections can
+correspond to multiple devices being connected to the same physical bus.
+
+Organisation of ports and endpoints
+-----------------------------------
+
+Ports are described by child 'port' nodes contained in the device node.
+Each port node contains an 'endpoint' subnode for each remote device port
+connected to this port. If a single port is connected to more than one
+remote device, an 'endpoint' child node must be provided for each link.
+If more than one port is present in a device node or there is more than one
+endpoint at a port, or a port node needs to be associated with a selected
+hardware interface, a common scheme using '#address-cells', '#size-cells'
+and 'reg' properties is used number the nodes.
+
+device {
+        ...
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        port@0 {
+	        #address-cells = <1>;
+	        #size-cells = <0>;
+		reg = <0>;
+
+                endpoint@0 {
+			reg = <0>;
+			...
+		};
+                endpoint@1 {
+			reg = <1>;
+			...
+		};
+        };
+
+        port@1 {
+		reg = <1>;
+
+		endpoint { ... };
+	};
+};
+
+All 'port' nodes can be grouped under an optional 'ports' node, which
+allows to specify #address-cells, #size-cells properties for the 'port'
+nodes independently from any other child device nodes a device might
+have.
+
+device {
+        ...
+        ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                        ...
+                        endpoint@0 { ... };
+                        endpoint@1 { ... };
+                };
+
+                port@1 { ... };
+        };
+};
+
+Links between endpoints
+-----------------------
+
+Each endpoint should contain a 'remote-endpoint' phandle property that points
+to the corresponding endpoint in the port of the remote device. In turn, the
+remote endpoint should contain a 'remote-endpoint' property. If it has one,
+it must not point to another than the local endpoint. Two endpoints with their
+'remote-endpoint' phandles pointing at each other form a link between the
+containing ports.
+
+device_1 {
+        port {
+                device_1_output: endpoint {
+                        remote-endpoint = <&device_2_input>;
+                };
+        };
+};
+
+device_1 {
+        port {
+                device_2_input: endpoint {
+                        remote-endpoint = <&device_1_output>;
+                };
+        };
+};
+
+
+Required properties
+-------------------
+
+If there is more than one 'port' or more than one 'endpoint' node or 'reg'
+property is present in port and/or endpoint nodes the following properties
+are required in a relevant parent node:
+
+ - #address-cells : number of cells required to define port/endpoint
+                    identifier, should be 1.
+ - #size-cells    : should be zero.
+
+Optional endpoint properties
+----------------------------
+
+- remote-endpoint: phandle to an 'endpoint' subnode of a remote device node.
+
-- 
1.8.5.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/7] Documentation: of: Document graph bindings

[Sylwester Nawrocki]

Hi Philipp,

Just couple minor comments...

On 02/27/2014 06:35 PM, Philipp Zabel wrote:
> The device tree graph bindings as used by V4L2 and documented in
> Documentation/device-tree/bindings/media/video-interfaces.txt contain
> generic parts that are not media specific but could be useful for any
> subsystem with data flow between multiple devices. This document
> describe the generic bindings.

s/describe/describes/

> Signed-off-by: Philipp Zabel<p.zabel@pengutronix.de>
> ---
> Changes since v4:
>   - Differentiate from graphs made by simple phandle links
>   - Do not mention data flow except in video-interfaces example
>   -
> ---
>   Documentation/devicetree/bindings/graph.txt | 129 ++++++++++++++++++++++++++++
>   1 file changed, 129 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/graph.txt
>
> diff --git a/Documentation/devicetree/bindings/graph.txt b/Documentation/devicetree/bindings/graph.txt
> new file mode 100644
> index 0000000..554865b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/graph.txt
> @@ -0,0 +1,129 @@
> +Common bindings for device graphs
> +
> +General concept
> +---------------
> +
> +The hierarchical organisation of the device tree is well suited to describe
> +control flow to devices, but there can be more complex connections between
> +devices that work together to form a logical compound device, following an
> +arbitrarily complex graph.
> +There already is a simple directed graph between devices tree nodes using
> +phandle properties pointing to other nodes to describe connections that
> +can not be inferred from device tree parent-child relationships. The device
> +tree graph bindings described herein abstract more complex devices that can
> +have multiple specifiable ports, each of which can be linked to one or more
> +ports of other devices.
> +
> +These common bindings do not contain any information about the direction of

s/of/or/ ?

> +type of the connections, they just map their existence. Specific properties
> +may be described by specialized bindings depending on the type of connection.
> +
> +To see how this binding applies to video pipelines, for example, see
> +Documentation/device-tree/bindings/media/video-interfaces.txt.
> +Here the ports describe data interfaces, and the links between them are
> +the connecting data buses. A single port with multiple connections can
> +correspond to multiple devices being connected to the same physical bus.
> +
> +Organisation of ports and endpoints
> +-----------------------------------
> +
> +Ports are described by child 'port' nodes contained in the device node.
> +Each port node contains an 'endpoint' subnode for each remote device port
> +connected to this port. If a single port is connected to more than one
> +remote device, an 'endpoint' child node must be provided for each link.
> +If more than one port is present in a device node or there is more than one
> +endpoint at a port, or a port node needs to be associated with a selected
> +hardware interface, a common scheme using '#address-cells', '#size-cells'
> +and 'reg' properties is used number the nodes.
> +
> +device {
> +        ...
> +        #address-cells =<1>;
> +        #size-cells =<0>;
> +
> +        port@0 {
> +	        #address-cells =<1>;
> +	        #size-cells =<0>;
> +		reg =<0>;
> +
> +                endpoint@0 {
> +			reg =<0>;
> +			...
> +		};
> +                endpoint@1 {
> +			reg =<1>;
> +			...
> +		};
> +        };
> +
> +        port@1 {
> +		reg =<1>;
> +
> +		endpoint { ... };
> +	};
> +};
> +
> +All 'port' nodes can be grouped under an optional 'ports' node, which
> +allows to specify #address-cells, #size-cells properties for the 'port'
> +nodes independently from any other child device nodes a device might
> +have.
> +
> +device {
> +        ...
> +        ports {
> +                #address-cells =<1>;
> +                #size-cells =<0>;
> +
> +                port@0 {
> +                        ...
> +                        endpoint@0 { ... };
> +                        endpoint@1 { ... };
> +                };
> +
> +                port@1 { ... };
> +        };
> +};
> +
> +Links between endpoints
> +-----------------------
> +
> +Each endpoint should contain a 'remote-endpoint' phandle property that points
> +to the corresponding endpoint in the port of the remote device. In turn, the
> +remote endpoint should contain a 'remote-endpoint' property. If it has one,
> +it must not point to another than the local endpoint. Two endpoints with their
> +'remote-endpoint' phandles pointing at each other form a link between the
> +containing ports.
> +
> +device_1 {
> +        port {
> +                device_1_output: endpoint {
> +                        remote-endpoint =<&device_2_input>;
> +                };
> +        };
> +};
> +
> +device_1 {

s/device_1/device_2/
But it might be better to use dashes instead of underscores
for the node names.

> +        port {
> +                device_2_input: endpoint {
> +                        remote-endpoint =<&device_1_output>;
> +                };
> +        };
> +};
> +
> +
> +Required properties
> +-------------------
> +
> +If there is more than one 'port' or more than one 'endpoint' node or 'reg'
> +property is present in port and/or endpoint nodes the following properties
> +are required in a relevant parent node:
> +
> + - #address-cells : number of cells required to define port/endpoint
> +                    identifier, should be 1.
> + - #size-cells    : should be zero.
> +
> +Optional endpoint properties
> +----------------------------
> +
> +- remote-endpoint: phandle to an 'endpoint' subnode of a remote device node.
> +

--
Regards,
Sylwester

Re: [PATCH v5 2/7] Documentation: of: Document graph bindings

[Philipp Zabel]

Hi Sylwester,

Am Freitag, den 28.02.2014, 22:08 +0100 schrieb Sylwester Nawrocki:
> Hi Philipp,
> 
> Just couple minor comments...

Thanks, I'll fix all of those.

> On 02/27/2014 06:35 PM, Philipp Zabel wrote:
> > The device tree graph bindings as used by V4L2 and documented in
> > Documentation/device-tree/bindings/media/video-interfaces.txt contain
> > generic parts that are not media specific but could be useful for any
> > subsystem with data flow between multiple devices. This document
> > describe the generic bindings.
> 
> s/describe/describes/

[...]

> > +These common bindings do not contain any information about the direction of
> 
> s/of/or/ ?

[...]

> > +device_1 {
> > +        port {
> > +                device_1_output: endpoint {
> > +                        remote-endpoint =<&device_2_input>;
> > +                };
> > +        };
> > +};
> > +
> > +device_1 {
> 
> s/device_1/device_2/
> But it might be better to use dashes instead of underscores
> for the node names.

regards
Philipp

[PATCH v5 3/7] of: Warn if of_graph_get_next_endpoint is called with the root node

[Philipp Zabel]

If of_graph_get_next_endpoint is given a parentless node instead of an
endpoint node, it is clearly a bug.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/of/base.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index b2f223f..6e650cf 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2028,8 +2028,8 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 		of_node_put(node);
 	} else {
 		port = of_get_parent(prev);
-		if (!port)
-			/* Hm, has someone given us the root node ?... */
+		if (WARN_ONCE(!port, "%s(): endpoint has no parent node\n",
+			      __func__))
 			return NULL;
 
 		/* Avoid dropping prev node refcount to 0. */
-- 
1.8.5.3

Re: [PATCH v5 3/7] of: Warn if of_graph_get_next_endpoint is called with the root node

[Sylwester Nawrocki]

On 02/27/2014 06:35 PM, Philipp Zabel wrote:
> If of_graph_get_next_endpoint is given a parentless node instead of an
> endpoint node, it is clearly a bug.
>
> Signed-off-by: Philipp Zabel<p.zabel@pengutronix.de>
> ---
>   drivers/of/base.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index b2f223f..6e650cf 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2028,8 +2028,8 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
>   		of_node_put(node);
>   	} else {
>   		port = of_get_parent(prev);
> -		if (!port)
> -			/* Hm, has someone given us the root node ?... */
> +		if (WARN_ONCE(!port, "%s(): endpoint has no parent node\n",
> +			      __func__))

Perhaps we can add more information to this error log, e.g.

		if (WARN_ONCE(!port, "%s(): endpoint %s has no parent node\n",
			      __func__, prev->full_name))
?
>   			return NULL;
>
>   		/* Avoid dropping prev node refcount to 0. */

Re: [PATCH v5 3/7] of: Warn if of_graph_get_next_endpoint is called with the root node

[Philipp Zabel]

Am Freitag, den 28.02.2014, 22:09 +0100 schrieb Sylwester Nawrocki:
> On 02/27/2014 06:35 PM, Philipp Zabel wrote:
> > If of_graph_get_next_endpoint is given a parentless node instead of an
> > endpoint node, it is clearly a bug.
> >
> > Signed-off-by: Philipp Zabel<p.zabel@pengutronix.de>
> > ---
> >   drivers/of/base.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index b2f223f..6e650cf 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -2028,8 +2028,8 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
> >   		of_node_put(node);
> >   	} else {
> >   		port = of_get_parent(prev);
> > -		if (!port)
> > -			/* Hm, has someone given us the root node ?... */
> > +		if (WARN_ONCE(!port, "%s(): endpoint has no parent node\n",
> > +			      __func__))
> 
> Perhaps we can add more information to this error log, e.g.
>
> 		if (WARN_ONCE(!port, "%s(): endpoint %s has no parent node\n",
> 			      __func__, prev->full_name))
> ?

Yes, I'll include this change next time.

thanks
Philipp

[PATCH v5 4/7] of: Reduce indentation in of_graph_get_next_endpoint

[Philipp Zabel]

A 'return endpoint;' at the end of the (!prev) case allows to
reduce the indentation level of the (prev) case.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/of/base.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 6e650cf..8ecca7a 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2026,32 +2026,34 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 			pr_err("%s(): no endpoint nodes specified for %s\n",
 			       __func__, parent->full_name);
 		of_node_put(node);
-	} else {
-		port = of_get_parent(prev);
-		if (WARN_ONCE(!port, "%s(): endpoint has no parent node\n",
-			      __func__))
-			return NULL;
 
-		/* Avoid dropping prev node refcount to 0. */
-		of_node_get(prev);
-		endpoint = of_get_next_child(port, prev);
-		if (endpoint) {
-			of_node_put(port);
-			return endpoint;
-		}
+		return endpoint;
+	}
 
-		/* No more endpoints under this port, try the next one. */
-		do {
-			port = of_get_next_child(parent, port);
-			if (!port)
-				return NULL;
-		} while (of_node_cmp(port->name, "port"));
+	port = of_get_parent(prev);
+	if (WARN_ONCE(!port, "%s(): endpoint has no parent node\n",
+		      __func__))
+		return NULL;
 
-		/* Pick up the first endpoint in this port. */
-		endpoint = of_get_next_child(port, NULL);
+	/* Avoid dropping prev node refcount to 0. */
+	of_node_get(prev);
+	endpoint = of_get_next_child(port, prev);
+	if (endpoint) {
 		of_node_put(port);
+		return endpoint;
 	}
 
+	/* No more endpoints under this port, try the next one. */
+	do {
+		port = of_get_next_child(parent, port);
+		if (!port)
+			return NULL;
+	} while (of_node_cmp(port->name, "port"));
+
+	/* Pick up the first endpoint in this port. */
+	endpoint = of_get_next_child(port, NULL);
+	of_node_put(port);
+
 	return endpoint;
 }
 EXPORT_SYMBOL(of_graph_get_next_endpoint);
-- 
1.8.5.3

[PATCH v5 5/7] [media] of: move common endpoint parsing to drivers/of

[Philipp Zabel]

This patch adds a new struct of_endpoint which is then embedded in struct
v4l2_of_endpoint and contains the endpoint properties that are not V4L2
(or even media) specific: the port number, endpoint id, local device tree
node and remote endpoint phandle. of_graph_parse_endpoint parses those
properties and is used by v4l2_of_parse_endpoint, which just adds the
V4L2 MBUS information to the containing v4l2_of_endpoint structure.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v4:
 - Fixed users of struct v4l2_of_endpoint
 - Removed left-over #include <media/of_graph.h> from v4l2-of.h
---
 drivers/media/platform/exynos4-is/media-dev.c | 10 ++++-----
 drivers/media/platform/exynos4-is/mipi-csis.c |  2 +-
 drivers/media/v4l2-core/v4l2-of.c             | 16 +++-----------
 drivers/of/base.c                             | 31 +++++++++++++++++++++++++++
 include/linux/of_graph.h                      | 20 +++++++++++++++++
 include/media/v4l2-of.h                       |  8 ++-----
 6 files changed, 62 insertions(+), 25 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index d0f82da..04d6ecd 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -469,10 +469,10 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
 		return 0;
 
 	v4l2_of_parse_endpoint(ep, &endpoint);
-	if (WARN_ON(endpoint.port == 0) || index >= FIMC_MAX_SENSORS)
+	if (WARN_ON(endpoint.base.port == 0) || index >= FIMC_MAX_SENSORS)
 		return -EINVAL;
 
-	pd->mux_id = (endpoint.port - 1) & 0x1;
+	pd->mux_id = (endpoint.base.port - 1) & 0x1;
 
 	rem = of_graph_get_remote_port_parent(ep);
 	of_node_put(ep);
@@ -494,13 +494,13 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
 		return -EINVAL;
 	}
 
-	if (fimc_input_is_parallel(endpoint.port)) {
+	if (fimc_input_is_parallel(endpoint.base.port)) {
 		if (endpoint.bus_type == V4L2_MBUS_PARALLEL)
 			pd->sensor_bus_type = FIMC_BUS_TYPE_ITU_601;
 		else
 			pd->sensor_bus_type = FIMC_BUS_TYPE_ITU_656;
 		pd->flags = endpoint.bus.parallel.flags;
-	} else if (fimc_input_is_mipi_csi(endpoint.port)) {
+	} else if (fimc_input_is_mipi_csi(endpoint.base.port)) {
 		/*
 		 * MIPI CSI-2: only input mux selection and
 		 * the sensor's clock frequency is needed.
@@ -508,7 +508,7 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
 		pd->sensor_bus_type = FIMC_BUS_TYPE_MIPI_CSI2;
 	} else {
 		v4l2_err(&fmd->v4l2_dev, "Wrong port id (%u) at node %s\n",
-			 endpoint.port, rem->full_name);
+			 endpoint.base.port, rem->full_name);
 	}
 	/*
 	 * For FIMC-IS handled sensors, that are placed under i2c-isp device
diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c
index fd1ae65..3678ba5 100644
--- a/drivers/media/platform/exynos4-is/mipi-csis.c
+++ b/drivers/media/platform/exynos4-is/mipi-csis.c
@@ -772,7 +772,7 @@ static int s5pcsis_parse_dt(struct platform_device *pdev,
 	/* Get port node and validate MIPI-CSI channel id. */
 	v4l2_of_parse_endpoint(node, &endpoint);
 
-	state->index = endpoint.port - FIMC_INPUT_MIPI_CSI2_0;
+	state->index = endpoint.base.port - FIMC_INPUT_MIPI_CSI2_0;
 	if (state->index < 0 || state->index >= CSIS_MAX_ENTITIES)
 		return -ENXIO;
 
diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
index f919db3..b4ed9a9 100644
--- a/drivers/media/v4l2-core/v4l2-of.c
+++ b/drivers/media/v4l2-core/v4l2-of.c
@@ -127,17 +127,9 @@ static void v4l2_of_parse_parallel_bus(const struct device_node *node,
 int v4l2_of_parse_endpoint(const struct device_node *node,
 			   struct v4l2_of_endpoint *endpoint)
 {
-	struct device_node *port_node = of_get_parent(node);
-
-	memset(endpoint, 0, offsetof(struct v4l2_of_endpoint, head));
-
-	endpoint->local_node = node;
-	/*
-	 * It doesn't matter whether the two calls below succeed.
-	 * If they don't then the default value 0 is used.
-	 */
-	of_property_read_u32(port_node, "reg", &endpoint->port);
-	of_property_read_u32(node, "reg", &endpoint->id);
+	of_graph_parse_endpoint(node, &endpoint->base);
+	endpoint->bus_type = 0;
+	memset(&endpoint->bus, 0, sizeof(endpoint->bus));
 
 	v4l2_of_parse_csi_bus(node, endpoint);
 	/*
@@ -147,8 +139,6 @@ int v4l2_of_parse_endpoint(const struct device_node *node,
 	if (endpoint->bus.mipi_csi2.flags == 0)
 		v4l2_of_parse_parallel_bus(node, endpoint);
 
-	of_node_put(port_node);
-
 	return 0;
 }
 EXPORT_SYMBOL(v4l2_of_parse_endpoint);
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 8ecca7a..ba3cfca 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1985,6 +1985,37 @@ struct device_node *of_find_next_cache_node(const struct device_node *np)
 }
 
 /**
+ * of_graph_parse_endpoint() - parse common endpoint node properties
+ * @node: pointer to endpoint device_node
+ * @endpoint: pointer to the OF endpoint data structure
+ *
+ * All properties are optional. If none are found, we don't set any flags.
+ * This means the port has a static configuration and no properties have
+ * to be specified explicitly.
+ * The caller should hold a reference to @node.
+ */
+int of_graph_parse_endpoint(const struct device_node *node,
+			    struct of_endpoint *endpoint)
+{
+	struct device_node *port_node = of_get_parent(node);
+
+	memset(endpoint, 0, sizeof(*endpoint));
+
+	endpoint->local_node = node;
+	/*
+	 * It doesn't matter whether the two calls below succeed.
+	 * If they don't then the default value 0 is used.
+	 */
+	of_property_read_u32(port_node, "reg", &endpoint->port);
+	of_property_read_u32(node, "reg", &endpoint->id);
+
+	of_node_put(port_node);
+
+	return 0;
+}
+EXPORT_SYMBOL(of_graph_parse_endpoint);
+
+/**
  * of_graph_get_next_endpoint() - get next endpoint node
  * @parent: pointer to the parent device node
  * @prev: previous endpoint node, or NULL to get first
diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
index 3bbeb60..2b233db 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h
@@ -14,7 +14,21 @@
 #ifndef __LINUX_OF_GRAPH_H
 #define __LINUX_OF_GRAPH_H
 
+/**
+ * struct of_endpoint - the OF graph endpoint data structure
+ * @port: identifier (value of reg property) of a port this endpoint belongs to
+ * @id: identifier (value of reg property) of this endpoint
+ * @local_node: pointer to device_node of this endpoint
+ */
+struct of_endpoint {
+	unsigned int port;
+	unsigned int id;
+	const struct device_node *local_node;
+};
+
 #ifdef CONFIG_OF
+int of_graph_parse_endpoint(const struct device_node *node,
+				struct of_endpoint *endpoint);
 struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 					struct device_node *previous);
 struct device_node *of_graph_get_remote_port_parent(
@@ -22,6 +36,12 @@ struct device_node *of_graph_get_remote_port_parent(
 struct device_node *of_graph_get_remote_port(const struct device_node *node);
 #else
 
+static inline int of_graph_parse_endpoint(const struct device_node *node,
+					struct of_endpoint *endpoint);
+{
+	return -ENOSYS;
+}
+
 static inline struct device_node *of_graph_get_next_endpoint(
 					const struct device_node *parent,
 					struct device_node *previous)
diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
index 3a49735..70fa7b7 100644
--- a/include/media/v4l2-of.h
+++ b/include/media/v4l2-of.h
@@ -51,17 +51,13 @@ struct v4l2_of_bus_parallel {
 
 /**
  * struct v4l2_of_endpoint - the endpoint data structure
- * @port: identifier (value of reg property) of a port this endpoint belongs to
- * @id: identifier (value of reg property) of this endpoint
- * @local_node: pointer to device_node of this endpoint
+ * @base: struct of_endpoint containing port, id, and local of_node
  * @bus_type: bus type
  * @bus: bus configuration data structure
  * @head: list head for this structure
  */
 struct v4l2_of_endpoint {
-	unsigned int port;
-	unsigned int id;
-	const struct device_node *local_node;
+	struct of_endpoint base;
 	enum v4l2_mbus_type bus_type;
 	union {
 		struct v4l2_of_bus_parallel parallel;
-- 
1.8.5.3

Re: [PATCH v5 5/7] [media] of: move common endpoint parsing to drivers/of

[Sylwester Nawrocki]

On 02/27/2014 06:35 PM, Philipp Zabel wrote:
> This patch adds a new struct of_endpoint which is then embedded in struct
> v4l2_of_endpoint and contains the endpoint properties that are not V4L2
> (or even media) specific: the port number, endpoint id, local device tree
> node and remote endpoint phandle. of_graph_parse_endpoint parses those
> properties and is used by v4l2_of_parse_endpoint, which just adds the
> V4L2 MBUS information to the containing v4l2_of_endpoint structure.
>
> Signed-off-by: Philipp Zabel<p.zabel@pengutronix.de>
> ---
> Changes since v4:
>   - Fixed users of struct v4l2_of_endpoint
>   - Removed left-over #include<media/of_graph.h>  from v4l2-of.h
> ---
>   drivers/media/platform/exynos4-is/media-dev.c | 10 ++++-----
>   drivers/media/platform/exynos4-is/mipi-csis.c |  2 +-
>   drivers/media/v4l2-core/v4l2-of.c             | 16 +++-----------
>   drivers/of/base.c                             | 31 +++++++++++++++++++++++++++
>   include/linux/of_graph.h                      | 20 +++++++++++++++++
>   include/media/v4l2-of.h                       |  8 ++-----
>   6 files changed, 62 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> index d0f82da..04d6ecd 100644
> --- a/drivers/media/platform/exynos4-is/media-dev.c
> +++ b/drivers/media/platform/exynos4-is/media-dev.c
> @@ -469,10 +469,10 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
>   		return 0;
>
>   	v4l2_of_parse_endpoint(ep,&endpoint);
> -	if (WARN_ON(endpoint.port == 0) || index>= FIMC_MAX_SENSORS)
> +	if (WARN_ON(endpoint.base.port == 0) || index>= FIMC_MAX_SENSORS)
>   		return -EINVAL;
>
> -	pd->mux_id = (endpoint.port - 1)&  0x1;
> +	pd->mux_id = (endpoint.base.port - 1)&  0x1;
>
>   	rem = of_graph_get_remote_port_parent(ep);
>   	of_node_put(ep);
> @@ -494,13 +494,13 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
>   		return -EINVAL;
>   	}
>
> -	if (fimc_input_is_parallel(endpoint.port)) {
> +	if (fimc_input_is_parallel(endpoint.base.port)) {
>   		if (endpoint.bus_type == V4L2_MBUS_PARALLEL)
>   			pd->sensor_bus_type = FIMC_BUS_TYPE_ITU_601;
>   		else
>   			pd->sensor_bus_type = FIMC_BUS_TYPE_ITU_656;
>   		pd->flags = endpoint.bus.parallel.flags;
> -	} else if (fimc_input_is_mipi_csi(endpoint.port)) {
> +	} else if (fimc_input_is_mipi_csi(endpoint.base.port)) {
>   		/*
>   		 * MIPI CSI-2: only input mux selection and
>   		 * the sensor's clock frequency is needed.
> @@ -508,7 +508,7 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
>   		pd->sensor_bus_type = FIMC_BUS_TYPE_MIPI_CSI2;
>   	} else {
>   		v4l2_err(&fmd->v4l2_dev, "Wrong port id (%u) at node %s\n",
> -			 endpoint.port, rem->full_name);
> +			 endpoint.base.port, rem->full_name);
>   	}
>   	/*
>   	 * For FIMC-IS handled sensors, that are placed under i2c-isp device
> diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c
> index fd1ae65..3678ba5 100644
> --- a/drivers/media/platform/exynos4-is/mipi-csis.c
> +++ b/drivers/media/platform/exynos4-is/mipi-csis.c
> @@ -772,7 +772,7 @@ static int s5pcsis_parse_dt(struct platform_device *pdev,
>   	/* Get port node and validate MIPI-CSI channel id. */
>   	v4l2_of_parse_endpoint(node,&endpoint);
>
> -	state->index = endpoint.port - FIMC_INPUT_MIPI_CSI2_0;
> +	state->index = endpoint.base.port - FIMC_INPUT_MIPI_CSI2_0;
>   	if (state->index<  0 || state->index>= CSIS_MAX_ENTITIES)
>   		return -ENXIO;
>
> diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
> index f919db3..b4ed9a9 100644
> --- a/drivers/media/v4l2-core/v4l2-of.c
> +++ b/drivers/media/v4l2-core/v4l2-of.c
> @@ -127,17 +127,9 @@ static void v4l2_of_parse_parallel_bus(const struct device_node *node,
>   int v4l2_of_parse_endpoint(const struct device_node *node,
>   			   struct v4l2_of_endpoint *endpoint)
>   {
> -	struct device_node *port_node = of_get_parent(node);
> -
> -	memset(endpoint, 0, offsetof(struct v4l2_of_endpoint, head));
> -
> -	endpoint->local_node = node;
> -	/*
> -	 * It doesn't matter whether the two calls below succeed.
> -	 * If they don't then the default value 0 is used.
> -	 */
> -	of_property_read_u32(port_node, "reg",&endpoint->port);
> -	of_property_read_u32(node, "reg",&endpoint->id);
> +	of_graph_parse_endpoint(node,&endpoint->base);
> +	endpoint->bus_type = 0;
> +	memset(&endpoint->bus, 0, sizeof(endpoint->bus));
>
>   	v4l2_of_parse_csi_bus(node, endpoint);
>   	/*
> @@ -147,8 +139,6 @@ int v4l2_of_parse_endpoint(const struct device_node *node,
>   	if (endpoint->bus.mipi_csi2.flags == 0)
>   		v4l2_of_parse_parallel_bus(node, endpoint);
>
> -	of_node_put(port_node);
> -
>   	return 0;
>   }
>   EXPORT_SYMBOL(v4l2_of_parse_endpoint);
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 8ecca7a..ba3cfca 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1985,6 +1985,37 @@ struct device_node *of_find_next_cache_node(const struct device_node *np)
>   }
>
>   /**
> + * of_graph_parse_endpoint() - parse common endpoint node properties
> + * @node: pointer to endpoint device_node
> + * @endpoint: pointer to the OF endpoint data structure
> + *
> + * All properties are optional. If none are found, we don't set any flags.
> + * This means the port has a static configuration and no properties have
> + * to be specified explicitly.

I don't think these two sentences are needed, it's all described in the
DT binding documentation. And struct of_endpoint doesn't contain any
"flags" field.

> + * The caller should hold a reference to @node.
> + */
> +int of_graph_parse_endpoint(const struct device_node *node,
> +			    struct of_endpoint *endpoint)
> +{
> +	struct device_node *port_node = of_get_parent(node);
> +
> +	memset(endpoint, 0, sizeof(*endpoint));
> +
> +	endpoint->local_node = node;
> +	/*
> +	 * It doesn't matter whether the two calls below succeed.
> +	 * If they don't then the default value 0 is used.
> +	 */
> +	of_property_read_u32(port_node, "reg",&endpoint->port);
> +	of_property_read_u32(node, "reg",&endpoint->id);
> +
> +	of_node_put(port_node);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(of_graph_parse_endpoint);
> +
> +/**
>    * of_graph_get_next_endpoint() - get next endpoint node
>    * @parent: pointer to the parent device node
>    * @prev: previous endpoint node, or NULL to get first
> diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
> index 3bbeb60..2b233db 100644
> --- a/include/linux/of_graph.h
> +++ b/include/linux/of_graph.h
> @@ -14,7 +14,21 @@
>   #ifndef __LINUX_OF_GRAPH_H
>   #define __LINUX_OF_GRAPH_H
>
> +/**
> + * struct of_endpoint - the OF graph endpoint data structure
> + * @port: identifier (value of reg property) of a port this endpoint belongs to
> + * @id: identifier (value of reg property) of this endpoint
> + * @local_node: pointer to device_node of this endpoint
> + */
> +struct of_endpoint {
> +	unsigned int port;
> +	unsigned int id;
> +	const struct device_node *local_node;
> +};
> +
>   #ifdef CONFIG_OF
> +int of_graph_parse_endpoint(const struct device_node *node,
> +				struct of_endpoint *endpoint);
>   struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
>   					struct device_node *previous);
>   struct device_node *of_graph_get_remote_port_parent(
> @@ -22,6 +36,12 @@ struct device_node *of_graph_get_remote_port_parent(
>   struct device_node *of_graph_get_remote_port(const struct device_node *node);
>   #else
>
> +static inline int of_graph_parse_endpoint(const struct device_node *node,
> +					struct of_endpoint *endpoint);
> +{
> +	return -ENOSYS;
> +}
> +
>   static inline struct device_node *of_graph_get_next_endpoint(
>   					const struct device_node *parent,
>   					struct device_node *previous)
> diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
> index 3a49735..70fa7b7 100644
> --- a/include/media/v4l2-of.h
> +++ b/include/media/v4l2-of.h
> @@ -51,17 +51,13 @@ struct v4l2_of_bus_parallel {
>
>   /**
>    * struct v4l2_of_endpoint - the endpoint data structure
> - * @port: identifier (value of reg property) of a port this endpoint belongs to
> - * @id: identifier (value of reg property) of this endpoint
> - * @local_node: pointer to device_node of this endpoint
> + * @base: struct of_endpoint containing port, id, and local of_node
>    * @bus_type: bus type
>    * @bus: bus configuration data structure
>    * @head: list head for this structure
>    */
>   struct v4l2_of_endpoint {
> -	unsigned int port;
> -	unsigned int id;
> -	const struct device_node *local_node;
> +	struct of_endpoint base;
>   	enum v4l2_mbus_type bus_type;
>   	union {
>   		struct v4l2_of_bus_parallel parallel;

Otherwise looks good to me.

Re: [PATCH v5 5/7] [media] of: move common endpoint parsing to drivers/of

[Philipp Zabel]

Am Freitag, den 28.02.2014, 22:09 +0100 schrieb Sylwester Nawrocki:
[...]
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -1985,6 +1985,37 @@ struct device_node *of_find_next_cache_node(const struct device_node *np)
> >   }
> >
> >   /**
> > + * of_graph_parse_endpoint() - parse common endpoint node properties
> > + * @node: pointer to endpoint device_node
> > + * @endpoint: pointer to the OF endpoint data structure
> > + *
> > + * All properties are optional. If none are found, we don't set any flags.
> > + * This means the port has a static configuration and no properties have
> > + * to be specified explicitly.
> 
> I don't think these two sentences are needed, it's all described in the
> DT binding documentation. And struct of_endpoint doesn't contain any
> "flags" field.

Thanks, I'll remove them.

regards
Philipp

Re: [PATCH v5 5/7] [media] of: move common endpoint parsing to drivers/of

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 4781 bytes --]

Hi Philipp,

On 27/02/14 19:35, Philipp Zabel wrote:
> This patch adds a new struct of_endpoint which is then embedded in struct
> v4l2_of_endpoint and contains the endpoint properties that are not V4L2
> (or even media) specific: the port number, endpoint id, local device tree
> node and remote endpoint phandle. of_graph_parse_endpoint parses those
> properties and is used by v4l2_of_parse_endpoint, which just adds the
> V4L2 MBUS information to the containing v4l2_of_endpoint structure.

<snip>

> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 8ecca7a..ba3cfca 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1985,6 +1985,37 @@ struct device_node *of_find_next_cache_node(const struct device_node *np)
>  }
>  
>  /**
> + * of_graph_parse_endpoint() - parse common endpoint node properties
> + * @node: pointer to endpoint device_node
> + * @endpoint: pointer to the OF endpoint data structure
> + *
> + * All properties are optional. If none are found, we don't set any flags.
> + * This means the port has a static configuration and no properties have
> + * to be specified explicitly.
> + * The caller should hold a reference to @node.
> + */
> +int of_graph_parse_endpoint(const struct device_node *node,
> +			    struct of_endpoint *endpoint)
> +{
> +	struct device_node *port_node = of_get_parent(node);

Can port_node be NULL? Probably only if something is quite wrong, but
maybe it's safer to return error in that case.

> +	memset(endpoint, 0, sizeof(*endpoint));
> +
> +	endpoint->local_node = node;
> +	/*
> +	 * It doesn't matter whether the two calls below succeed.
> +	 * If they don't then the default value 0 is used.
> +	 */
> +	of_property_read_u32(port_node, "reg", &endpoint->port);
> +	of_property_read_u32(node, "reg", &endpoint->id);

If the endpoint does not have 'port' as parent (i.e. the shortened
format), the above will return the 'reg' of the device node (with
'device node' I mean the main node, with 'compatible' property).

And generally speaking, if struct of_endpoint is needed, maybe it would
be better to return the struct of_endpoint when iterating the ports and
endpoints. That way there's no need to do parsing "afterwards", trying
to figure out if there's a parent port node, but the information is
already at hand.

> +
> +	of_node_put(port_node);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(of_graph_parse_endpoint);
> +
> +/**
>   * of_graph_get_next_endpoint() - get next endpoint node
>   * @parent: pointer to the parent device node
>   * @prev: previous endpoint node, or NULL to get first
> diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
> index 3bbeb60..2b233db 100644
> --- a/include/linux/of_graph.h
> +++ b/include/linux/of_graph.h
> @@ -14,7 +14,21 @@
>  #ifndef __LINUX_OF_GRAPH_H
>  #define __LINUX_OF_GRAPH_H
>  
> +/**
> + * struct of_endpoint - the OF graph endpoint data structure
> + * @port: identifier (value of reg property) of a port this endpoint belongs to
> + * @id: identifier (value of reg property) of this endpoint
> + * @local_node: pointer to device_node of this endpoint
> + */
> +struct of_endpoint {
> +	unsigned int port;
> +	unsigned int id;
> +	const struct device_node *local_node;
> +};

A few thoughts about the iteration, and the API in general.

In the omapdss version I separated iterating ports and endpoints, for
the two reasons:

1) I think there are cases where you may want to have properties in the
port node, for things that are common for all the port's endpoints.

2) if there are multiple ports, I think the driver code is cleaner if
you first take the port, decide what port that is and maybe call a
sub-function, and then iterate the endpoints for that port only.

Both of those are possible with the API in the series, but not very cleanly.

Also, if you just want to iterate the endpoints, it's easy to implement
a helper using the separate port and endpoint iterations.


Then, about the get_remote functions: I think there should be only one
function for that purpose, one that returns the device node that
contains the remote endpoint.

My reasoning is that the ports and endpoints, and their contents, should
be private to the device. So the only use to get the remote is to get
the actual device, to see if it's been probed, or maybe get some video
API for that device.

If the driver model used has some kind of master-driver, which goes
through all the display entities, I think the above is still valid. When
the master-driver follows the remote-link, it still needs to first get
the main device node, as the ports and endpoints make no sense without
the context of the main device node.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH v5 5/7] [media] of: move common endpoint parsing to drivers/of

[Philipp Zabel]

Hi Tomi,

Am Dienstag, den 04.03.2014, 10:58 +0200 schrieb Tomi Valkeinen:
[...]
> > +int of_graph_parse_endpoint(const struct device_node *node,
> > +			    struct of_endpoint *endpoint)
> > +{
> > +	struct device_node *port_node = of_get_parent(node);
> 
> Can port_node be NULL? Probably only if something is quite wrong, but
> maybe it's safer to return error in that case.

both of_property_read_u32 and of_node_put can handle port_node == NULL.
I'll add a WARN_ONCE here as for of_graph_get_next_endpoint and continue
on.

> > +	memset(endpoint, 0, sizeof(*endpoint));
> > +
> > +	endpoint->local_node = node;
> > +	/*
> > +	 * It doesn't matter whether the two calls below succeed.
> > +	 * If they don't then the default value 0 is used.
> > +	 */
> > +	of_property_read_u32(port_node, "reg", &endpoint->port);
> > +	of_property_read_u32(node, "reg", &endpoint->id);
> 
> If the endpoint does not have 'port' as parent (i.e. the shortened
> format), the above will return the 'reg' of the device node (with
> 'device node' I mean the main node, with 'compatible' property).

Yes, a check for the port_node name is in order.

> And generally speaking, if struct of_endpoint is needed, maybe it would
> be better to return the struct of_endpoint when iterating the ports and
> endpoints. That way there's no need to do parsing "afterwards", trying
> to figure out if there's a parent port node, but the information is
> already at hand.

I'd like to keep the iteration separate from parsing so we can
eventually introduce a for_each_endpoint_of_node helper macro around
of_graph_get_next_endpoint.

[...]
> A few thoughts about the iteration, and the API in general.
> 
> In the omapdss version I separated iterating ports and endpoints, for
> the two reasons:
> 
> 1) I think there are cases where you may want to have properties in the
> port node, for things that are common for all the port's endpoints.
>
> 2) if there are multiple ports, I think the driver code is cleaner if
> you first take the port, decide what port that is and maybe call a
> sub-function, and then iterate the endpoints for that port only.

It depends a bit on whether you are actually iterating over individual
ports, or if you are just walking the whole endpoint graph to find
remote devices that have to be added to the component master's waiting
list, for example.

> Both of those are possible with the API in the series, but not very cleanly.
>
> Also, if you just want to iterate the endpoints, it's easy to implement
> a helper using the separate port and endpoint iterations.

I started out to move an existing (albeit lightly used) API to a common
place so others can use it and improve upon it, too. I'm happy to pile
on fixes directly in this series, but could we separate the improvement
step from the move, for the bigger modifications?

I had no immediate use for the port iteration, so I have taken no steps
to add a function for this. I see no problem to add this later when
somebody needs it, or even rewrite of_graph_get_next_endpoint to use it
if it is feasible. Iterating over endpoints on a given port needs no
helper, as you can just use for_each_child_of_node.

> Then, about the get_remote functions: I think there should be only one
> function for that purpose, one that returns the device node that
> contains the remote endpoint.
> 
> My reasoning is that the ports and endpoints, and their contents, should
> be private to the device. So the only use to get the remote is to get
> the actual device, to see if it's been probed, or maybe get some video
> API for that device.

of_graph_get_remote_port currently is used in the exynos4-is/fimc-is.c
v4l2 driver to get the mipi-csi channel id from the remote port, and
I've started using it in imx-drm-core.c for two cases:
- given an endpoint on the encoder, find the remote port connected to
  it, get the associated drm_crtc, to obtain its the drm_crtc_mask
  for encoder->possible_crtcs.
- given an encoder and a connected drm_crtc, walk all endpoints to find
  the remote port associated with the drm_crtc, and then use the local
  endpoint parent port to determine multiplexer settings.

> If the driver model used has some kind of master-driver, which goes
> through all the display entities, I think the above is still valid. When
> the master-driver follows the remote-link, it still needs to first get
> the main device node, as the ports and endpoints make no sense without
> the context of the main device node.

I'm not sure about this. I might just need the remote port node
associated with a remote drm_crtc or drm_encoder structure to find out
which local endpoint I should look at to retrieve configuration.

regards
Philipp

Re: [PATCH v5 5/7] [media] of: move common endpoint parsing to drivers/of

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 6001 bytes --]

On 04/03/14 13:36, Philipp Zabel wrote:
> Hi Tomi,
> 
> Am Dienstag, den 04.03.2014, 10:58 +0200 schrieb Tomi Valkeinen:
> [...]
>>> +int of_graph_parse_endpoint(const struct device_node *node,
>>> +			    struct of_endpoint *endpoint)
>>> +{
>>> +	struct device_node *port_node = of_get_parent(node);
>>
>> Can port_node be NULL? Probably only if something is quite wrong, but
>> maybe it's safer to return error in that case.
> 
> both of_property_read_u32 and of_node_put can handle port_node == NULL.
> I'll add a WARN_ONCE here as for of_graph_get_next_endpoint and continue
> on.

Isn't it better to return an error?

>> And generally speaking, if struct of_endpoint is needed, maybe it would
>> be better to return the struct of_endpoint when iterating the ports and
>> endpoints. That way there's no need to do parsing "afterwards", trying
>> to figure out if there's a parent port node, but the information is
>> already at hand.
> 
> I'd like to keep the iteration separate from parsing so we can
> eventually introduce a for_each_endpoint_of_node helper macro around
> of_graph_get_next_endpoint.
> 
> [...]
>> A few thoughts about the iteration, and the API in general.
>>
>> In the omapdss version I separated iterating ports and endpoints, for
>> the two reasons:
>>
>> 1) I think there are cases where you may want to have properties in the
>> port node, for things that are common for all the port's endpoints.
>>
>> 2) if there are multiple ports, I think the driver code is cleaner if
>> you first take the port, decide what port that is and maybe call a
>> sub-function, and then iterate the endpoints for that port only.
> 
> It depends a bit on whether you are actually iterating over individual
> ports, or if you are just walking the whole endpoint graph to find
> remote devices that have to be added to the component master's waiting
> list, for example.

True, but the latter is easily implemented using the separate
port/endpoint iteration. So I see it as a more powerful API.

>> Both of those are possible with the API in the series, but not very cleanly.
>>
>> Also, if you just want to iterate the endpoints, it's easy to implement
>> a helper using the separate port and endpoint iterations.
> 
> I started out to move an existing (albeit lightly used) API to a common
> place so others can use it and improve upon it, too. I'm happy to pile
> on fixes directly in this series, but could we separate the improvement
> step from the move, for the bigger modifications?

Yes, I understand that. What I wonder is that which is easier: make it a
public API now, more or less as it was in v4l2, or make it a public API
only when all the improvements we can think of have been made.

So my fear is that the API is now made public, and you and others start
to use it. But I can't use it, as I need things like separate
port/endpoint iteration. I need to add those, which also means that I
need to change all the users of the API, making the task more difficult
than I'd like.

However, this is more of "thinking out loud" than "I don't like the
series". It's a good series =).

> I had no immediate use for the port iteration, so I have taken no steps
> to add a function for this. I see no problem to add this later when
> somebody needs it, or even rewrite of_graph_get_next_endpoint to use it
> if it is feasible. Iterating over endpoints on a given port needs no
> helper, as you can just use for_each_child_of_node.

I would have a helper, which should do some sanity checks, like that the
node names are "endpoint".

>> Then, about the get_remote functions: I think there should be only one
>> function for that purpose, one that returns the device node that
>> contains the remote endpoint.
>>
>> My reasoning is that the ports and endpoints, and their contents, should
>> be private to the device. So the only use to get the remote is to get
>> the actual device, to see if it's been probed, or maybe get some video
>> API for that device.
> 
> of_graph_get_remote_port currently is used in the exynos4-is/fimc-is.c
> v4l2 driver to get the mipi-csi channel id from the remote port, and
> I've started using it in imx-drm-core.c for two cases:
> - given an endpoint on the encoder, find the remote port connected to
>   it, get the associated drm_crtc, to obtain its the drm_crtc_mask
>   for encoder->possible_crtcs.
> - given an encoder and a connected drm_crtc, walk all endpoints to find
>   the remote port associated with the drm_crtc, and then use the local
>   endpoint parent port to determine multiplexer settings.

Ok.

In omapdss each driver handles only the ports and endpoints defined for
its device, and they can be considered private to that device. The only
reason to look for the remote endpoint is to find the remote device. To
me the omapdss model makes sense, and feels logical and sane =). So I
have to say I'm not really familiar with the model you're using.

Of course, the different get_remove_* funcs do no harm, even if we have
a bunch of them. My point was only about enforcing the correct use of
the model, where "correct" is of course subjective =).

>> If the driver model used has some kind of master-driver, which goes
>> through all the display entities, I think the above is still valid. When
>> the master-driver follows the remote-link, it still needs to first get
>> the main device node, as the ports and endpoints make no sense without
>> the context of the main device node.
> 
> I'm not sure about this. I might just need the remote port node
> associated with a remote drm_crtc or drm_encoder structure to find out
> which local endpoint I should look at to retrieve configuration.

Ok. I guess if you have a fixed model for the video pipeline elements it
works out. For omapdss we don't know what's there on the remote side, as
we can have arbitrary amount of video pipeline elements.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH v5 5/7] [media] of: move common endpoint parsing to drivers/of

[Philipp Zabel]

Am Dienstag, den 04.03.2014, 14:21 +0200 schrieb Tomi Valkeinen:
> On 04/03/14 13:36, Philipp Zabel wrote:
[...]
> >> Can port_node be NULL? Probably only if something is quite wrong, but
> >> maybe it's safer to return error in that case.
> > 
> > both of_property_read_u32 and of_node_put can handle port_node == NULL.
> > I'll add a WARN_ONCE here as for of_graph_get_next_endpoint and continue
> > on.
> 
> Isn't it better to return an error?

I am not sure. We can still correctly parse the endpoint properties of a
parentless node. All current users ignore the return value anyway. So as
long as we still do the memset and and set local_node and id, returning
an error effectively won't change the current behaviour.

[...]
> > It depends a bit on whether you are actually iterating over individual
> > ports, or if you are just walking the whole endpoint graph to find
> > remote devices that have to be added to the component master's waiting
> > list, for example.
> 
> True, but the latter is easily implemented using the separate
> port/endpoint iteration. So I see it as a more powerful API.

Indeed. I see no problem in adding an of_graph_get_next_port function.
But I'd like to keep the current of_graph_get_next_endpoint function
iterating over all endpoints of the device.

> >> Both of those are possible with the API in the series, but not very cleanly.
> >>
> >> Also, if you just want to iterate the endpoints, it's easy to implement
> >> a helper using the separate port and endpoint iterations.
> > 
> > I started out to move an existing (albeit lightly used) API to a common
> > place so others can use it and improve upon it, too. I'm happy to pile
> > on fixes directly in this series, but could we separate the improvement
> > step from the move, for the bigger modifications?
> 
> Yes, I understand that. What I wonder is that which is easier: make it a
> public API now, more or less as it was in v4l2, or make it a public API
> only when all the improvements we can think of have been made.
> 
> So my fear is that the API is now made public, and you and others start
> to use it.

And I fear that this series might outgrow maintainer attention spans if
I keep adding to it.

> But I can't use it, as I need things like separate
> port/endpoint iteration. I need to add those, which also means that I
> need to change all the users of the API, making the task more difficult
> than I'd like.
>
> However, this is more of "thinking out loud" than "I don't like the
> series". It's a good series =).

Thanks. How about I follow this up with a split port/endpoint parsing
helpers after I get some acks?

> > I had no immediate use for the port iteration, so I have taken no steps
> > to add a function for this. I see no problem to add this later when
> > somebody needs it, or even rewrite of_graph_get_next_endpoint to use it
> > if it is feasible. Iterating over endpoints on a given port needs no
> > helper, as you can just use for_each_child_of_node.
> 
> I would have a helper, which should do some sanity checks, like that the
> node names are "endpoint".

I'd prefer this to be a generic function of_get_next_child_by_name and
possibly a macro for_each_named_child_of_node wrapping that.

[...]
> In omapdss each driver handles only the ports and endpoints defined for
> its device, and they can be considered private to that device. The only
> reason to look for the remote endpoint is to find the remote device. To
> me the omapdss model makes sense, and feels logical and sane =). So I
> have to say I'm not really familiar with the model you're using.

The main difference I see is that a single IPU device will have two port
nodes handled by the DRM driver and two port nodes handled by the V4L2
driver, so we can't go back up to the IPU device tree node and iterate
over all its ports in either the DRM or V4L2 drivers.
You could argue that all the device tree parsing should be done from the
IPU drivers, and the DRM and V4L2 drivers should use preparsed internal
graph structures. But then we are getting into using struct media_entity
in DRM drivers territory, and rather not go there right now.

regards
Philipp

Re: [PATCH v5 5/7] [media] of: move common endpoint parsing to drivers/of

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]

On 04/03/14 17:47, Philipp Zabel wrote:
> Am Dienstag, den 04.03.2014, 14:21 +0200 schrieb Tomi Valkeinen:
>> On 04/03/14 13:36, Philipp Zabel wrote:
> [...]
>>>> Can port_node be NULL? Probably only if something is quite wrong, but
>>>> maybe it's safer to return error in that case.
>>>
>>> both of_property_read_u32 and of_node_put can handle port_node == NULL.
>>> I'll add a WARN_ONCE here as for of_graph_get_next_endpoint and continue
>>> on.
>>
>> Isn't it better to return an error?
> 
> I am not sure. We can still correctly parse the endpoint properties of a
> parentless node. All current users ignore the return value anyway. So as
> long as we still do the memset and and set local_node and id, returning
> an error effectively won't change the current behaviour.

Is the parentless node case an error or not? If it's not, we can handle
it silently and return 0, no WARN needed. If it is an error, we should
return an error, even if nobody is currently handling that (which is a
bug in itself, as the function does return an error value, and callers
should handle it).

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH v5 5/7] [media] of: move common endpoint parsing to drivers/of

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 2534 bytes --]

Hi Tomi,

On Tuesday 04 March 2014 14:21:09 Tomi Valkeinen wrote:
> On 04/03/14 13:36, Philipp Zabel wrote:
> > Am Dienstag, den 04.03.2014, 10:58 +0200 schrieb Tomi Valkeinen:
> > [...]

[snip]

> >> Then, about the get_remote functions: I think there should be only one
> >> function for that purpose, one that returns the device node that
> >> contains the remote endpoint.
> >> 
> >> My reasoning is that the ports and endpoints, and their contents, should
> >> be private to the device. So the only use to get the remote is to get
> >> the actual device, to see if it's been probed, or maybe get some video
> >> API for that device.
> > 
> > of_graph_get_remote_port currently is used in the exynos4-is/fimc-is.c
> > v4l2 driver to get the mipi-csi channel id from the remote port, and
> > I've started using it in imx-drm-core.c for two cases:
> > - given an endpoint on the encoder, find the remote port connected to
> >   it, get the associated drm_crtc, to obtain its the drm_crtc_mask
> >   for encoder->possible_crtcs.
> > 
> > - given an encoder and a connected drm_crtc, walk all endpoints to find
> >   the remote port associated with the drm_crtc, and then use the local
> >   endpoint parent port to determine multiplexer settings.
> 
> Ok.
> 
> In omapdss each driver handles only the ports and endpoints defined for
> its device, and they can be considered private to that device. The only
> reason to look for the remote endpoint is to find the remote device. To
> me the omapdss model makes sense, and feels logical and sane =). So I
> have to say I'm not really familiar with the model you're using.

I agree with you that most of the content of the remote endpoint should be 
considered private to the remote device and not accessed by the local device 
driver. There is, however, one piece of information from the remote endpoint 
useful to the local device driver, it's the remote port identifier. This can 
be expressed by a phandle, a remote port number, a media entity pad pointer, 
or any other mean, but the information is useful for the local device driver 
to communicate with the remote device driver. For instance a driver could use 
it to ask its video stream source to start the video stream on a given port.

> Of course, the different get_remove_* funcs do no harm, even if we have
> a bunch of them. My point was only about enforcing the correct use of
> the model, where "correct" is of course subjective =).

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH v5 5/7] [media] of: move common endpoint parsing to drivers/of

[Eric Boxer]

[-- Attachment #1: Type: text/plain, Size: 2525 bytes --]

Eric Boxer liked your message with Boxer. On March 6, 2014 at 5:59:17 PM CST, Laurent Pinchart  wrote:Hi Tomi,On Tuesday 04 March 2014 14:21:09 Tomi Valkeinen wrote:> On 04/03/14 13:36, Philipp Zabel wrote:> > Am Dienstag, den 04.03.2014, 10:58 +0200 schrieb Tomi Valkeinen:> > [...][snip]> >> Then, about the get_remote functions: I think there should be only one> >> function for that purpose, one that returns the device node that> >> contains the remote endpoint.> >> > >> My reasoning is that the ports and endpoints, and their contents, should> >> be private to the device. So the only use to get the remote is to get> >> the actual device, to see if it's been probed, or maybe get some video> >> API for that device.> > > > of_graph_get_remote_port currently is used in the exynos4-is/fimc-is.c> > v4l2 driver to get the mipi-csi channel id from the remote port, and> > I've started using it in imx-drm-core.c for two cases:> > - given an endpoint on the encoder, find the remote port connected to> > it, get the associated drm_crtc, to obtain its the drm_crtc_mask> > for encoder->possible_crtcs.> > > > - given an encoder and a connected drm_crtc, walk all endpoints to find> > the remote port associated with the drm_crtc, and then use the local> > endpoint parent port to determine multiplexer settings.> > Ok.> > In omapdss each driver handles only the ports and endpoints defined for> its device, and they can be considered private to that device. The only> reason to look for the remote endpoint is to find the remote device. To> me the omapdss model makes sense, and feels logical and sane =). So I> have to say I'm not really familiar with the model you're using.I agree with you that most of the content of the remote endpoint should be considered private to the remote device and not accessed by the local device driver. There is, however, one piece of information from the remote endpoint useful to the local device driver, it's the remote port identifier. This can be expressed by a phandle, a remote port number, a media entity pad pointer, or any other mean, but the information is useful for the local device driver to communicate with the remote device driver. For instance a driver could use it to ask its video stream source to start the video stream on a given port.> Of course, the different get_remove_* funcs do no harm, even if we have> a bunch of them. My point was only about enforcing the correct use of> the model, where "correct" is of course subjective =).-- Regards,Laurent Pinchart     

[-- Attachment #2: Type: text/html, Size: 3278 bytes --]

[PATCH v5 6/7] of: Implement simplified graph binding for single port devices

[Philipp Zabel]

For simple devices with only one port, it can be made implicit.
The endpoint node can be a direct child of the device node.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/of/base.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ba3cfca..7d9c62b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2037,8 +2037,13 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 		struct device_node *node;
 		/*
 		 * It's the first call, we have to find a port subnode
-		 * within this node or within an optional 'ports' node.
+		 * within this node or within an optional 'ports' node,
+		 * or at least a single endpoint.
 		 */
+		endpoint = of_get_child_by_name(parent, "endpoint");
+		if (endpoint)
+			return endpoint;
+
 		node = of_get_child_by_name(parent, "ports");
 		if (node)
 			parent = node;
@@ -2049,8 +2054,6 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 			/* Found a port, get an endpoint. */
 			endpoint = of_get_next_child(port, NULL);
 			of_node_put(port);
-		} else {
-			endpoint = NULL;
 		}
 
 		if (!endpoint)
@@ -2065,6 +2068,10 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 	if (WARN_ONCE(!port, "%s(): endpoint has no parent node\n",
 		      __func__))
 		return NULL;
+	if (port == parent) {
+		of_node_put(port);
+		return NULL;
+	}
 
 	/* Avoid dropping prev node refcount to 0. */
 	of_node_get(prev);
@@ -2105,9 +2112,11 @@ struct device_node *of_graph_get_remote_port_parent(
 	/* Get remote endpoint node. */
 	np = of_parse_phandle(node, "remote-endpoint", 0);
 
-	/* Walk 3 levels up only if there is 'ports' node. */
+	/* Walk 3 levels up only if there is 'ports' node */
 	for (depth = 3; depth && np; depth--) {
 		np = of_get_next_parent(np);
+		if (depth == 3 && of_node_cmp(np->name, "port"))
+			break;
 		if (depth == 2 && of_node_cmp(np->name, "ports"))
 			break;
 	}
@@ -2130,6 +2139,11 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node)
 	np = of_parse_phandle(node, "remote-endpoint", 0);
 	if (!np)
 		return NULL;
-	return of_get_next_parent(np);
+	np = of_get_next_parent(np);
+	if (of_node_cmp(np->name, "port")) {
+		of_node_put(np);
+		return NULL;
+	}
+	return np;
 }
 EXPORT_SYMBOL(of_graph_get_remote_port);
-- 
1.8.5.3

Re: [PATCH v5 6/7] of: Implement simplified graph binding for single port devices

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1082 bytes --]

On 27/02/14 19:35, Philipp Zabel wrote:
> For simple devices with only one port, it can be made implicit.
> The endpoint node can be a direct child of the device node.

<snip>

> @@ -2105,9 +2112,11 @@ struct device_node *of_graph_get_remote_port_parent(
>  	/* Get remote endpoint node. */
>  	np = of_parse_phandle(node, "remote-endpoint", 0);
>  
> -	/* Walk 3 levels up only if there is 'ports' node. */
> +	/* Walk 3 levels up only if there is 'ports' node */
>  	for (depth = 3; depth && np; depth--) {
>  		np = of_get_next_parent(np);
> +		if (depth == 3 && of_node_cmp(np->name, "port"))
> +			break;
>  		if (depth == 2 && of_node_cmp(np->name, "ports"))
>  			break;
>  	}

This function becomes a bit funny. Would it be clearer just to do
something like:

	np = of_parse_phandle(node, "remote-endpoint", 0);

	np = of_get_next_parent(np);
	if (of_node_cmp(np->name, "port") != 0)
		return np;

	np = of_get_next_parent(np);
	if (of_node_cmp(np->name, "ports") != 0)
		return np;

	np = of_get_next_parent(np);
	return np;

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH v5 6/7] of: Implement simplified graph binding for single port devices

[Philipp Zabel]

Am Dienstag, den 04.03.2014, 11:06 +0200 schrieb Tomi Valkeinen:
> On 27/02/14 19:35, Philipp Zabel wrote:
> > For simple devices with only one port, it can be made implicit.
> > The endpoint node can be a direct child of the device node.
> 
> <snip>
> 
> > @@ -2105,9 +2112,11 @@ struct device_node *of_graph_get_remote_port_parent(
> >  	/* Get remote endpoint node. */
> >  	np = of_parse_phandle(node, "remote-endpoint", 0);
> >  
> > -	/* Walk 3 levels up only if there is 'ports' node. */
> > +	/* Walk 3 levels up only if there is 'ports' node */
> >  	for (depth = 3; depth && np; depth--) {
> >  		np = of_get_next_parent(np);
> > +		if (depth == 3 && of_node_cmp(np->name, "port"))
> > +			break;
> >  		if (depth == 2 && of_node_cmp(np->name, "ports"))
> >  			break;
> >  	}
> 
> This function becomes a bit funny. Would it be clearer just to do
> something like:
> 
> 	np = of_parse_phandle(node, "remote-endpoint", 0);
> 
> 	np = of_get_next_parent(np);
> 	if (of_node_cmp(np->name, "port") != 0)
> 		return np;
> 
> 	np = of_get_next_parent(np);
> 	if (of_node_cmp(np->name, "ports") != 0)
> 		return np;
> 
> 	np = of_get_next_parent(np);
> 	return np;

I'm not sure if this was initially crafted to reduce the number of
function calls, but rolled out it certainly is easier to read. I'll
change this as you suggest.

thanks
Philipp

[PATCH v5 7/7] of: Document simplified graph binding for single port devices

[Philipp Zabel]

For simple devices with only one port, it can be made implicit.
The endpoint node can be a direct child of the device node.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 Documentation/devicetree/bindings/graph.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/graph.txt b/Documentation/devicetree/bindings/graph.txt
index 554865b..6dfaff8 100644
--- a/Documentation/devicetree/bindings/graph.txt
+++ b/Documentation/devicetree/bindings/graph.txt
@@ -84,6 +84,14 @@ device {
         };
 };
 
+For devices with only a single port and a single endpoint, this can be further
+simplified by making the port implicit, and adding the endpoint node as a direct
+child of the device node.
+
+device {
+	endpoint { ... };
+};
+
 Links between endpoints
 -----------------------
 
-- 
1.8.5.3