[PATCH v6 0/8] Move device tree graph parsing helpers to drivers/of

[PATCH v6 0/8] Move device tree graph parsing helpers to drivers/of

[Philipp Zabel]

Hi,

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

Changes since v5:
 - Fixed spelling errors and a wrong device node name in the link section
 - Added parentless previous endpoint's full name to warning
 - Fixed documentation comment for of_graph_parse_endpoint
 - Unrolled for-loop in of_graph_get_remote_port_parent

Philipp Zabel (8):
  [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
  of: Warn if of_graph_parse_endpoint is called with the root node

 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                             | 167 ++++++++++++++++++++++++++
 include/linux/of_graph.h                      |  66 ++++++++++
 include/media/v4l2-of.h                       |  33 +----
 13 files changed, 399 insertions(+), 178 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/graph.txt
 create mode 100644 include/linux/of_graph.h

-- 
1.9.0.rc3

[PATCH v6 1/8] [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>
---
 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.9.0.rc3

[PATCH v6 2/8] 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
describes the generic bindings.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v5:
 - Fixed spelling errors and a wrong device node name in the link section
 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..1a69c07
--- /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 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-2 {
+        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.9.0.rc3

[PATCH v6 3/8] 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>
---
Changes since v5:
 - Added parentless previous endpoint's full name to warning
---
 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..b5e690b 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 %s has no parent node\n",
+			      __func__, prev->full_name))
 			return NULL;
 
 		/* Avoid dropping prev node refcount to 0. */
-- 
1.9.0.rc3

[PATCH v6 4/8] 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 b5e690b..a8e47d3 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 %s has no parent node\n",
-			      __func__, prev->full_name))
-			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 %s has no parent node\n",
+		      __func__, prev->full_name))
+		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.9.0.rc3

[PATCH v6 5/8] [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 v5:
 - Fixed documentation comment for of_graph_parse_endpoint
---
 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                             | 28 +++++++++++++++++++++++++++
 include/linux/of_graph.h                      | 20 +++++++++++++++++++
 include/media/v4l2-of.h                       |  8 ++------
 6 files changed, 59 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 a8e47d3..715144af 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1985,6 +1985,34 @@ 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
+ *
+ * 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.9.0.rc3

[PATCH v6 6/8] 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>
---
Changes since v5:
 - Unrolled for-loop in of_graph_get_remote_port_parent
---
 drivers/of/base.c | 42 +++++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 715144af..ffd0217 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2003,7 +2003,8 @@ int of_graph_parse_endpoint(const struct device_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);
+	if (port_node && !of_node_cmp(port_node->name, "port"))
+		of_property_read_u32(port_node, "reg", &endpoint->port);
 	of_property_read_u32(node, "reg", &endpoint->id);
 
 	of_node_put(port_node);
@@ -2034,8 +2035,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;
@@ -2046,8 +2052,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)
@@ -2062,6 +2066,10 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 	if (WARN_ONCE(!port, "%s(): endpoint %s has no parent node\n",
 		      __func__, prev->full_name))
 		return NULL;
+	if (port == parent) {
+		of_node_put(port);
+		return NULL;
+	}
 
 	/* Avoid dropping prev node refcount to 0. */
 	of_node_get(prev);
@@ -2097,18 +2105,21 @@ 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);
+	if (!np)
+		return NULL;
 
-	/* 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;
+	np = of_get_next_parent(np);
+	if (!np || of_node_cmp(np->name, "port") != 0)
+		return np;
+
+	np = of_get_next_parent(np);
+	if (!np || of_node_cmp(np->name, "ports") != 0)
+		return np;
+
+	return of_get_next_parent(np);
 }
 EXPORT_SYMBOL(of_graph_get_remote_port_parent);
 
@@ -2127,6 +2138,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.9.0.rc3

[PATCH v6 7/8] 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 1a69c07..e496547 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.9.0.rc3

[PATCH v6 8/8] of: Warn if of_graph_parse_endpoint is called with the root node

[Philipp Zabel]

If of_graph_parse_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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ffd0217..75de6bd 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1996,6 +1996,9 @@ int of_graph_parse_endpoint(const struct device_node *node,
 {
 	struct device_node *port_node = of_get_parent(node);
 
+	WARN_ONCE(!port_node, "%s(): endpoint %s has no parent node\n",
+		  __func__, node->full_name);
+
 	memset(endpoint, 0, sizeof(*endpoint));
 
 	endpoint->local_node = node;
-- 
1.9.0.rc3

Re: [PATCH v6 0/8] Move device tree graph parsing helpers to drivers/of

[Tomi Valkeinen]

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

Hi,

On 05/03/14 11:20, Philipp Zabel wrote:
> Hi,
> 
> this version of the OF graph helper move series further addresses a few of
> Tomi's and Sylwester's comments.
> 
> Changes since v5:
>  - Fixed spelling errors and a wrong device node name in the link section
>  - Added parentless previous endpoint's full name to warning
>  - Fixed documentation comment for of_graph_parse_endpoint
>  - Unrolled for-loop in of_graph_get_remote_port_parent
> 
> Philipp Zabel (8):
>   [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
>   of: Warn if of_graph_parse_endpoint is called with the root node

So, as I've pointed out, I don't agree with the API, as it's too limited
and I can't use it, but as this series is (mostly) about moving the
current API to a common place, it's fine for me.

Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 Tomi



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

Re: [PATCH v6 0/8] Move device tree graph parsing helpers to drivers/of

[Philipp Zabel]

Hi Tomi,

Am Mittwoch, den 05.03.2014, 13:35 +0200 schrieb Tomi Valkeinen:
> Hi,
> 
> On 05/03/14 11:20, Philipp Zabel wrote:
> > Hi,
> > 
> > this version of the OF graph helper move series further addresses a few of
> > Tomi's and Sylwester's comments.
> > 
> > Changes since v5:
> >  - Fixed spelling errors and a wrong device node name in the link section
> >  - Added parentless previous endpoint's full name to warning
> >  - Fixed documentation comment for of_graph_parse_endpoint
> >  - Unrolled for-loop in of_graph_get_remote_port_parent
> > 
> > Philipp Zabel (8):
> >   [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
> >   of: Warn if of_graph_parse_endpoint is called with the root node
> 
> So, as I've pointed out, I don't agree with the API, as it's too limited
> and I can't use it, but as this series is (mostly) about moving the
> current API to a common place, it's fine for me.
> 
> Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Thanks. I'll be happy to help expanding the API to parse ports
individually, once this gets accepted.

Mauro, Guennadi, are you fine with how this turned out? I'd like to get
your acks again, for the changed location.

regards
Philipp

Re: [PATCH v6 0/8] Move device tree graph parsing helpers to drivers/of

[Russell King - ARM Linux]

On Wed, Mar 05, 2014 at 03:42:34PM +0100, Philipp Zabel wrote:
> Am Mittwoch, den 05.03.2014, 13:35 +0200 schrieb Tomi Valkeinen:
> > Hi,
> > 
> > On 05/03/14 11:20, Philipp Zabel wrote:
> > > Hi,
> > > 
> > > this version of the OF graph helper move series further addresses a few of
> > > Tomi's and Sylwester's comments.
> > > 
> > > Changes since v5:
> > >  - Fixed spelling errors and a wrong device node name in the link section
> > >  - Added parentless previous endpoint's full name to warning
> > >  - Fixed documentation comment for of_graph_parse_endpoint
> > >  - Unrolled for-loop in of_graph_get_remote_port_parent
> > > 
> > > Philipp Zabel (8):
> > >   [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
> > >   of: Warn if of_graph_parse_endpoint is called with the root node
> > 
> > So, as I've pointed out, I don't agree with the API, as it's too limited
> > and I can't use it, but as this series is (mostly) about moving the
> > current API to a common place, it's fine for me.
> > 
> > Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> Thanks. I'll be happy to help expanding the API to parse ports
> individually, once this gets accepted.
> 
> Mauro, Guennadi, are you fine with how this turned out? I'd like to get
> your acks again, for the changed location.

I'll need those acks before I can even think about queuing up the
imx-drm bits.

Another way to deal with this is if this gets pulled into the V4L tree
from Philipp's git tree, I can also pull that in myself.  What mustn't
happen is for these to be committed independently as patches.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH v6 0/8] Move device tree graph parsing helpers to drivers/of

[Mauro Carvalho Chehab]

Em Thu, 06 Mar 2014 14:16:57 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> escreveu:

> On Wed, Mar 05, 2014 at 03:42:34PM +0100, Philipp Zabel wrote:
> > Am Mittwoch, den 05.03.2014, 13:35 +0200 schrieb Tomi Valkeinen:
> > > Hi,
> > > 
> > > On 05/03/14 11:20, Philipp Zabel wrote:
> > > > Hi,
> > > > 
> > > > this version of the OF graph helper move series further addresses a few of
> > > > Tomi's and Sylwester's comments.
> > > > 
> > > > Changes since v5:
> > > >  - Fixed spelling errors and a wrong device node name in the link section
> > > >  - Added parentless previous endpoint's full name to warning
> > > >  - Fixed documentation comment for of_graph_parse_endpoint
> > > >  - Unrolled for-loop in of_graph_get_remote_port_parent
> > > > 
> > > > Philipp Zabel (8):
> > > >   [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
> > > >   of: Warn if of_graph_parse_endpoint is called with the root node
> > > 
> > > So, as I've pointed out, I don't agree with the API, as it's too limited
> > > and I can't use it, but as this series is (mostly) about moving the
> > > current API to a common place, it's fine for me.
> > > 
> > > Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > 
> > Thanks. I'll be happy to help expanding the API to parse ports
> > individually, once this gets accepted.
> > 
> > Mauro, Guennadi, are you fine with how this turned out? I'd like to get
> > your acks again, for the changed location.

>From my side, there's nothing on such code that is V4L2 specific.
Moving it to drivers/of makes sense on my eyes.

Acked-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> 
> I'll need those acks before I can even think about queuing up the
> imx-drm bits.
> 
> Another way to deal with this is if this gets pulled into the V4L tree
> from Philipp's git tree, I can also pull that in myself.  What mustn't
> happen is for these to be committed independently as patches.

If everyone agrees, I actually prefer have this patch applied on my tree,
in order to avoid some potential merge conflicts at the merge window,
as we might have other drivers and changes there touching on those API
calls (I'm aware of a series of patches from Sylwester with some DT
stuff on it. Not sure if it would be affected by such changes or not).

-- 

Cheers,
Mauro

Re: [PATCH v6 0/8] Move device tree graph parsing helpers to drivers/of

[Russell King - ARM Linux]

On Wed, Mar 05, 2014 at 10:20:34AM +0100, Philipp Zabel wrote:
> this version of the OF graph helper move series further addresses a few of
> Tomi's and Sylwester's comments.

Philipp,

You mention in your other cover for imx-drm bits that this is available
via:

	git://git.pengutronix.de/git/pza/linux.git topic/of-graph

What is this tree based upon?

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH v6 0/8] Move device tree graph parsing helpers to drivers/of

[Philipp Zabel]

Am Donnerstag, den 06.03.2014, 15:24 +0000 schrieb Russell King - ARM
Linux:
> On Wed, Mar 05, 2014 at 10:20:34AM +0100, Philipp Zabel wrote:
> > this version of the OF graph helper move series further addresses a few of
> > Tomi's and Sylwester's comments.
> 
> Philipp,
> 
> You mention in your other cover for imx-drm bits that this is available
> via:
> 
> 	git://git.pengutronix.de/git/pza/linux.git topic/of-graph
> 
> What is this tree based upon?

It is based on v3.14-rc5.

regards
Philipp

Re: [PATCH v6 0/8] Move device tree graph parsing helpers to drivers/of

[Sylwester Nawrocki]

On 06/03/14 16:17, Mauro Carvalho Chehab wrote:
> Em Thu, 06 Mar 2014 14:16:57 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> escreveu:
> 
>> > On Wed, Mar 05, 2014 at 03:42:34PM +0100, Philipp Zabel wrote:
>>> > > Am Mittwoch, den 05.03.2014, 13:35 +0200 schrieb Tomi Valkeinen:
>>>> > > > Hi,
>>>> > > > 
>>>> > > > On 05/03/14 11:20, Philipp Zabel wrote:
>>>>> > > > > Hi,
>>>>> > > > > 
>>>>> > > > > this version of the OF graph helper move series further addresses a few of
>>>>> > > > > Tomi's and Sylwester's comments.
>>>>> > > > > 
>>>>> > > > > Changes since v5:
>>>>> > > > >  - Fixed spelling errors and a wrong device node name in the link section
>>>>> > > > >  - Added parentless previous endpoint's full name to warning
>>>>> > > > >  - Fixed documentation comment for of_graph_parse_endpoint
>>>>> > > > >  - Unrolled for-loop in of_graph_get_remote_port_parent
>>>>> > > > > 
>>>>> > > > > Philipp Zabel (8):
>>>>> > > > >   [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
>>>>> > > > >   of: Warn if of_graph_parse_endpoint is called with the root node
>>>> > > > 
>>>> > > > So, as I've pointed out, I don't agree with the API, as it's too limited
>>>> > > > and I can't use it, but as this series is (mostly) about moving the
>>>> > > > current API to a common place, it's fine for me.
>>>> > > > 
>>>> > > > Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> > > 
>>> > > Thanks. I'll be happy to help expanding the API to parse ports
>>> > > individually, once this gets accepted.
>>> > > 
>>> > > Mauro, Guennadi, are you fine with how this turned out? I'd like to get
>>> > > your acks again, for the changed location.
>
> From my side, there's nothing on such code that is V4L2 specific.
> Moving it to drivers/of makes sense on my eyes.
> 
> Acked-by: Mauro Carvalho Chehab <m.chehab@samsung.com>

I'm OK with patches 1...5, 8, so for these:

Acked-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Regarding the simplified version of the binding, I thought we should
leave 'port' instead of 'endpoint' node. This could cover more hardware
configurations. Are there any users of this simplified binding queued
for v3.15 ? If not, perhaps we can postpone it and discuss it a bit more
(sorry, couldn't find time to comment on that earlier) ?

>> > I'll need those acks before I can even think about queuing up the
>> > imx-drm bits.
>> > 
>> > Another way to deal with this is if this gets pulled into the V4L tree
>> > from Philipp's git tree, I can also pull that in myself.  What mustn't
>> > happen is for these to be committed independently as patches.
>
> If everyone agrees, I actually prefer have this patch applied on my tree,
> in order to avoid some potential merge conflicts at the merge window,
> as we might have other drivers and changes there touching on those API
> calls (I'm aware of a series of patches from Sylwester with some DT
> stuff on it. Not sure if it would be affected by such changes or not).

Yes, it's going to conflict with my patch series. I thought it could be
put onto a stable a topic branch, e.g. at git://linuxtv.org/media_tree.git,
which could be then pulled into the media master branch and anywhere
else it is needed ?

--
Regards,
Sylwester

Re: [PATCH v6 0/8] Move device tree graph parsing helpers to drivers/of

[Russell King - ARM Linux]

On Thu, Mar 06, 2014 at 04:39:39PM +0100, Philipp Zabel wrote:
> Am Donnerstag, den 06.03.2014, 15:24 +0000 schrieb Russell King - ARM
> Linux:
> > On Wed, Mar 05, 2014 at 10:20:34AM +0100, Philipp Zabel wrote:
> > > this version of the OF graph helper move series further addresses a few of
> > > Tomi's and Sylwester's comments.
> > 
> > Philipp,
> > 
> > You mention in your other cover for imx-drm bits that this is available
> > via:
> > 
> > 	git://git.pengutronix.de/git/pza/linux.git topic/of-graph
> > 
> > What is this tree based upon?
> 
> It is based on v3.14-rc5.

Great.  As everyone seems happy, can you both send me and Mauro a pull
request for this please?  Once Mauro says it's in his tree, I'll pull
it in for imx-drm stuff.

Thanks.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH v6 0/8] Move device tree graph parsing helpers to drivers/of

[Philipp Zabel]

Am Donnerstag, den 06.03.2014, 16:47 +0100 schrieb Sylwester Nawrocki:
> On 06/03/14 16:17, Mauro Carvalho Chehab wrote:
> > Em Thu, 06 Mar 2014 14:16:57 +0000
> > Russell King - ARM Linux <linux@arm.linux.org.uk> escreveu:
> >> > On Wed, Mar 05, 2014 at 03:42:34PM +0100, Philipp Zabel wrote:
> >>> > > Am Mittwoch, den 05.03.2014, 13:35 +0200 schrieb Tomi Valkeinen:
[...]
> >>>> > > > So, as I've pointed out, I don't agree with the API, as it's too limited
> >>>> > > > and I can't use it, but as this series is (mostly) about moving the
> >>>> > > > current API to a common place, it's fine for me.
> >>>> > > > 
> >>>> > > > Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >>> > > 
> >>> > > Thanks. I'll be happy to help expanding the API to parse ports
> >>> > > individually, once this gets accepted.
> >>> > > 
> >>> > > Mauro, Guennadi, are you fine with how this turned out? I'd like to get
> >>> > > your acks again, for the changed location.
> >
> > From my side, there's nothing on such code that is V4L2 specific.
> > Moving it to drivers/of makes sense on my eyes.
> > 
> > Acked-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> 
> I'm OK with patches 1...5, 8, so for these:
> 
> Acked-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 
> Regarding the simplified version of the binding, I thought we should
> leave 'port' instead of 'endpoint' node. This could cover more hardware
> configurations. Are there any users of this simplified binding queued
> for v3.15 ? If not, perhaps we can postpone it and discuss it a bit more
> (sorry, couldn't find time to comment on that earlier) ?

Since Tomi needs the separate port/endpoint iteration anyway, 
postponing the simple bindings shouldn't hurt. I'll (re)submit them
together in a second series.

> >> > I'll need those acks before I can even think about queuing up the
> >> > imx-drm bits.
> >> > 
> >> > Another way to deal with this is if this gets pulled into the V4L tree
> >> > from Philipp's git tree, I can also pull that in myself.  What mustn't
> >> > happen is for these to be committed independently as patches.
> >
> > If everyone agrees, I actually prefer have this patch applied on my tree,
> > in order to avoid some potential merge conflicts at the merge window,
> > as we might have other drivers and changes there touching on those API
> > calls (I'm aware of a series of patches from Sylwester with some DT
> > stuff on it. Not sure if it would be affected by such changes or not).
> 
> Yes, it's going to conflict with my patch series. I thought it could be
> put onto a stable a topic branch, e.g. at git://linuxtv.org/media_tree.git,
> which could be then pulled into the media master branch and anywhere
> else it is needed ?

Mauro, are you ok with handling the conflict in the merge, or should I
rebase on top of the media tree after you merged Sylwester's changes?

regards
Philipp

Re: [PATCH v6 0/8] Move device tree graph parsing helpers to drivers/of

[Sylwester Nawrocki]

On 06/03/14 17:21, Philipp Zabel wrote:
> Am Donnerstag, den 06.03.2014, 16:47 +0100 schrieb Sylwester Nawrocki:
>> On 06/03/14 16:17, Mauro Carvalho Chehab wrote:
>>> Em Thu, 06 Mar 2014 14:16:57 +0000
>>> Russell King - ARM Linux <linux@arm.linux.org.uk> escreveu:
>>>>> On Wed, Mar 05, 2014 at 03:42:34PM +0100, Philipp Zabel wrote:
>>>>>>> Am Mittwoch, den 05.03.2014, 13:35 +0200 schrieb Tomi Valkeinen:
> [...]
>>>>>>>>> So, as I've pointed out, I don't agree with the API, as it's too limited
>>>>>>>>> and I can't use it, but as this series is (mostly) about moving the
>>>>>>>>> current API to a common place, it's fine for me.
>>>>>>>>>
>>>>>>>>> Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>>>>>
>>>>>>> Thanks. I'll be happy to help expanding the API to parse ports
>>>>>>> individually, once this gets accepted.
>>>>>>>
>>>>>>> Mauro, Guennadi, are you fine with how this turned out? I'd like to get
>>>>>>> your acks again, for the changed location.
>>>
>>> From my side, there's nothing on such code that is V4L2 specific.
>>> Moving it to drivers/of makes sense on my eyes.
>>>
>>> Acked-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
>>
>> I'm OK with patches 1...5, 8, so for these:
>>
>> Acked-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>
>> Regarding the simplified version of the binding, I thought we should
>> leave 'port' instead of 'endpoint' node. This could cover more hardware
>> configurations. Are there any users of this simplified binding queued
>> for v3.15 ? If not, perhaps we can postpone it and discuss it a bit more
>> (sorry, couldn't find time to comment on that earlier) ?
> 
> Since Tomi needs the separate port/endpoint iteration anyway, 
> postponing the simple bindings shouldn't hurt. I'll (re)submit them
> together in a second series.

Ok, thanks.

>>>>> I'll need those acks before I can even think about queuing up the
>>>>> imx-drm bits.
>>>>>
>>>>> Another way to deal with this is if this gets pulled into the V4L tree
>>>>> from Philipp's git tree, I can also pull that in myself.  What mustn't
>>>>> happen is for these to be committed independently as patches.
>>>
>>> If everyone agrees, I actually prefer have this patch applied on my tree,
>>> in order to avoid some potential merge conflicts at the merge window,
>>> as we might have other drivers and changes there touching on those API
>>> calls (I'm aware of a series of patches from Sylwester with some DT
>>> stuff on it. Not sure if it would be affected by such changes or not).
>>
>> Yes, it's going to conflict with my patch series. I thought it could be
>> put onto a stable a topic branch, e.g. at git://linuxtv.org/media_tree.git,
>> which could be then pulled into the media master branch and anywhere
>> else it is needed ?
> 
> Mauro, are you ok with handling the conflict in the merge, or should I
> rebase on top of the media tree after you merged Sylwester's changes?

I could rebase and resolve any conflicts before sending my pull request
to Mauro. I don't think it's a good idea to rebase this series onto the
media tree, since it is touching drivers/of.

--
Regards,
Sylwester

Re: [PATCH v6 0/8] Move device tree graph parsing helpers to drivers/of

[Philipp Zabel]

Am Donnerstag, den 06.03.2014, 17:32 +0100 schrieb Sylwester Nawrocki:
> On 06/03/14 17:21, Philipp Zabel wrote:
> > Am Donnerstag, den 06.03.2014, 16:47 +0100 schrieb Sylwester Nawrocki:
> >> Yes, it's going to conflict with my patch series. I thought it could be
> >> put onto a stable a topic branch, e.g. at git://linuxtv.org/media_tree.git,
> >> which could be then pulled into the media master branch and anywhere
> >> else it is needed ?
> > 
> > Mauro, are you ok with handling the conflict in the merge, or should I
> > rebase on top of the media tree after you merged Sylwester's changes?
> 
> I could rebase and resolve any conflicts before sending my pull request
> to Mauro. I don't think it's a good idea to rebase this series onto the
> media tree, since it is touching drivers/of.

Excellent, thanks. I will send a pull request to Mauro and Russell
shortly.

regards
Philipp

Re: [PATCH v6 4/8] of: Reduce indentation in of_graph_get_next_endpoint

[Laurent Pinchart]

Hi Philipp,

Thank you for the patch.

I've submitted a fix for the of_graph_get_next_endpoint() function, but it 
hasn't been applied yet due to the patch series that contained it needing more 
work.

The patch is available at https://patchwork.linuxtv.org/patch/21946/. I can 
rebase it on top of this series, but I still wanted to let you know about it 
in case you would like to integrate it.

On Wednesday 05 March 2014 10:20:38 Philipp Zabel wrote:
> 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 b5e690b..a8e47d3 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 %s has no parent node\n",
> -			      __func__, prev->full_name))
> -			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 %s has no parent node\n",
> +		      __func__, prev->full_name))
> +		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);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 0/8] Move device tree graph parsing helpers to drivers/of

[Laurent Pinchart]

Hi Philipp,

Thank you for the patches.

On Thursday 06 March 2014 16:47:24 Sylwester Nawrocki wrote:
> On 06/03/14 16:17, Mauro Carvalho Chehab wrote:
> > Em Thu, 06 Mar 2014 14:16:57 +0000 Russell King - ARM Linux escreveu:
> >>> On Wed, Mar 05, 2014 at 03:42:34PM +0100, Philipp Zabel wrote:
> >>>>> Am Mittwoch, den 05.03.2014, 13:35 +0200 schrieb Tomi Valkeinen:
> >>>>>>> On 05/03/14 11:20, Philipp Zabel wrote:
> >>>>>>>>> Hi,
> >>>>>>>>> 
> >>>>>>>>> this version of the OF graph helper move series further addresses
> >>>>>>>>> a few of Tomi's and Sylwester's comments.
> >>>>>>>>> 
> >>>>>>>>> Changes since v5:
> >>>>>>>>>  - Fixed spelling errors and a wrong device node name in the link
> >>>>>>>>>  section
> >>>>>>>>>  - Added parentless previous endpoint's full name to warning
> >>>>>>>>>  - Fixed documentation comment for of_graph_parse_endpoint
> >>>>>>>>>  - Unrolled for-loop in of_graph_get_remote_port_parent
> >>>>>>>>> 
> >>>>>>>>> Philipp Zabel (8):
> >>>>>>>>>   [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
> >>>>>>>>>   of: Warn if of_graph_parse_endpoint is called with the root node
> >>>>>>> 
> >>>>>>> So, as I've pointed out, I don't agree with the API, as it's too
> >>>>>>> limited and I can't use it, but as this series is (mostly) about
> >>>>>>> moving the current API to a common place, it's fine for me.
> >>>>>>> 
> >>>>>>> Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >>>>> 
> >>>>> Thanks. I'll be happy to help expanding the API to parse ports
> >>>>> individually, once this gets accepted.
> >>>>> 
> >>>>> Mauro, Guennadi, are you fine with how this turned out? I'd like to
> >>>>> get your acks again, for the changed location.
> > 
> > From my side, there's nothing on such code that is V4L2 specific.
> > Moving it to drivers/of makes sense on my eyes.
> > 
> > Acked-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> 
> I'm OK with patches 1...5, 8, so for these:
> 
> Acked-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

For what it's worth given that you've sent a pull request already, for the 
same patches,

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 0/8] Move device tree graph parsing helpers to drivers/of

[Russell King - ARM Linux]

On Thu, Mar 06, 2014 at 05:50:30PM +0100, Philipp Zabel wrote:
> Am Donnerstag, den 06.03.2014, 17:32 +0100 schrieb Sylwester Nawrocki:
> > On 06/03/14 17:21, Philipp Zabel wrote:
> > > Am Donnerstag, den 06.03.2014, 16:47 +0100 schrieb Sylwester Nawrocki:
> > >> Yes, it's going to conflict with my patch series. I thought it could be
> > >> put onto a stable a topic branch, e.g. at git://linuxtv.org/media_tree.git,
> > >> which could be then pulled into the media master branch and anywhere
> > >> else it is needed ?
> > > 
> > > Mauro, are you ok with handling the conflict in the merge, or should I
> > > rebase on top of the media tree after you merged Sylwester's changes?
> > 
> > I could rebase and resolve any conflicts before sending my pull request
> > to Mauro. I don't think it's a good idea to rebase this series onto the
> > media tree, since it is touching drivers/of.
> 
> Excellent, thanks. I will send a pull request to Mauro and Russell
> shortly.

Unless we can find some way to resolve the problem which just happened
(Mauro applied the of-graph as patches to his tree inspite of this
discussion), I don't see the imx-drm DT changes being able to be pulled
in for the upcoming merge window - because otherwise it means that
whatever Mauro has applied your of-graph changes on top of in his tree
also gets pulled into my tree and Greg's tree.

I'm afraid this has turned into a total shambles, and I'm totally
hacked off at Mauro over this.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH v6 4/8] of: Reduce indentation in of_graph_get_next_endpoint

[Philipp Zabel]

Hi Laurent,

Am Freitag, den 07.03.2014, 01:12 +0100 schrieb Laurent Pinchart:
> Hi Philipp,
> 
> Thank you for the patch.
> 
> I've submitted a fix for the of_graph_get_next_endpoint() function, but it 
> hasn't been applied yet due to the patch series that contained it needing more 
> work.
>
> The patch is available at https://patchwork.linuxtv.org/patch/21946/. I can 
> rebase it on top of this series, but I still wanted to let you know about it 
> in case you would like to integrate it.

Thank you for the pointer. A pity about the timing, this will mostly
revert my indentation patch. I'd be glad if you could rebase on top of
the merged series.

While we look at of_graph_get_next_endpoint(), could you explain the
reason behind the extra reference count increase on the prev node:
	/*
	 * Avoid dropping prev node refcount to 0 when getting the next
	 * child below.
	 */
	of_node_get(prev);
This unfortunately makes using the function in for_each style macros a
hassle. If that part wasn't there and all users that want to keep using
prev after the call were expected to increase refcount themselves,
we could have a
#define of_graph_for_each_endpoint(parent, endpoint) \
	for (endpoint = of_graph_get_next_endpoint(parent, NULL); \
	     endpoint != NULL; \
	     endpoint = of_graph_get_next_endpoint(parent, endpoint))

regards
Philipp

Re: [PATCH v6 1/8] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of

[Grant Likely]

On Wed,  5 Mar 2014 10:20:35 +0100, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> 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>

This patch is fine because it only moves code, but I'm not providing an
ack yet.  I'll supply one when I'm happy with the collection of fixups.

g.

Re: [PATCH v6 2/8] Documentation: of: Document graph bindings

[Grant Likely]

On Wed,  5 Mar 2014 10:20:36 +0100, Philipp Zabel <p.zabel@pengutronix.de> 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
> describes the generic bindings.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

See my comments on the previous version. My concerns are the handling of
the optional 'ports' node and the usage of reverse links.

g.

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

[Grant Likely]

On Wed,  5 Mar 2014 10:20:37 +0100, Philipp Zabel <p.zabel@pengutronix.de> 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>

Acked-by: Grant Likely <grant.likely@linaro.org>

> ---
> Changes since v5:
>  - Added parentless previous endpoint's full name to warning
> ---
>  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..b5e690b 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 %s has no parent node\n",
> +			      __func__, prev->full_name))
>  			return NULL;
>  
>  		/* Avoid dropping prev node refcount to 0. */
> -- 
> 1.9.0.rc3
> 

Re: [PATCH v6 4/8] of: Reduce indentation in of_graph_get_next_endpoint

[Grant Likely]

On Wed,  5 Mar 2014 10:20:38 +0100, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> 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>

Acked-by: Grant Likely <grant.likely@linaro.org>

> ---
>  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 b5e690b..a8e47d3 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 %s has no parent node\n",
> -			      __func__, prev->full_name))
> -			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 %s has no parent node\n",
> +		      __func__, prev->full_name))
> +		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.9.0.rc3
> 

Re: [PATCH v6 5/8] [media] of: move common endpoint parsing to drivers/of

[Grant Likely]

On Wed,  5 Mar 2014 10:20:39 +0100, Philipp Zabel <p.zabel@pengutronix.de> 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>

Seems okay.

Acked-by: Grant Likely <grant.likely@linaro.org>

> ---
> Changes since v5:
>  - Fixed documentation comment for of_graph_parse_endpoint
> ---
>  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                             | 28 +++++++++++++++++++++++++++
>  include/linux/of_graph.h                      | 20 +++++++++++++++++++
>  include/media/v4l2-of.h                       |  8 ++------
>  6 files changed, 59 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 a8e47d3..715144af 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1985,6 +1985,34 @@ 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
> + *
> + * 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.9.0.rc3
> 

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

[Grant Likely]

On Wed,  5 Mar 2014 10:20:40 +0100, Philipp Zabel <p.zabel@pengutronix.de> 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.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Ergh... I think this is too loosely defined. The caller really should be
explicit about which behaviour it needs. I'll listen to arguments
though if you can make a strong argument.

g.

> --- > Changes since v5:
>  - Unrolled for-loop in of_graph_get_remote_port_parent
> ---
>  drivers/of/base.c | 42 +++++++++++++++++++++++++++++-------------
>  1 file changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 715144af..ffd0217 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2003,7 +2003,8 @@ int of_graph_parse_endpoint(const struct device_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);
> +	if (port_node && !of_node_cmp(port_node->name, "port"))
> +		of_property_read_u32(port_node, "reg", &endpoint->port);
>  	of_property_read_u32(node, "reg", &endpoint->id);
>  
>  	of_node_put(port_node);
> @@ -2034,8 +2035,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;
> @@ -2046,8 +2052,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)
> @@ -2062,6 +2066,10 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
>  	if (WARN_ONCE(!port, "%s(): endpoint %s has no parent node\n",
>  		      __func__, prev->full_name))
>  		return NULL;
> +	if (port == parent) {
> +		of_node_put(port);
> +		return NULL;
> +	}
>  
>  	/* Avoid dropping prev node refcount to 0. */
>  	of_node_get(prev);
> @@ -2097,18 +2105,21 @@ 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);
> +	if (!np)
> +		return NULL;
>  
> -	/* 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;
> +	np = of_get_next_parent(np);
> +	if (!np || of_node_cmp(np->name, "port") != 0)
> +		return np;
> +
> +	np = of_get_next_parent(np);
> +	if (!np || of_node_cmp(np->name, "ports") != 0)
> +		return np;
> +
> +	return of_get_next_parent(np);
>  }
>  EXPORT_SYMBOL(of_graph_get_remote_port_parent);
>  
> @@ -2127,6 +2138,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.9.0.rc3
> 

Re: [PATCH v6 0/8] Move device tree graph parsing helpers to drivers/of

[Grant Likely]

On Thu, 06 Mar 2014 12:17:21 -0300, Mauro Carvalho Chehab <m.chehab@samsung.com> wrote:
> Em Thu, 06 Mar 2014 14:16:57 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> escreveu:
> 
> > On Wed, Mar 05, 2014 at 03:42:34PM +0100, Philipp Zabel wrote:
> > > Am Mittwoch, den 05.03.2014, 13:35 +0200 schrieb Tomi Valkeinen:
> > > > Hi,
> > > > 
> > > > On 05/03/14 11:20, Philipp Zabel wrote:
> > > > > Hi,
> > > > > 
> > > > > this version of the OF graph helper move series further addresses a few of
> > > > > Tomi's and Sylwester's comments.
> > > > > 
> > > > > Changes since v5:
> > > > >  - Fixed spelling errors and a wrong device node name in the link section
> > > > >  - Added parentless previous endpoint's full name to warning
> > > > >  - Fixed documentation comment for of_graph_parse_endpoint
> > > > >  - Unrolled for-loop in of_graph_get_remote_port_parent
> > > > > 
> > > > > Philipp Zabel (8):
> > > > >   [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
> > > > >   of: Warn if of_graph_parse_endpoint is called with the root node
> > > > 
> > > > So, as I've pointed out, I don't agree with the API, as it's too limited
> > > > and I can't use it, but as this series is (mostly) about moving the
> > > > current API to a common place, it's fine for me.
> > > > 
> > > > Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > > 
> > > Thanks. I'll be happy to help expanding the API to parse ports
> > > individually, once this gets accepted.
> > > 
> > > Mauro, Guennadi, are you fine with how this turned out? I'd like to get
> > > your acks again, for the changed location.
> 
> From my side, there's nothing on such code that is V4L2 specific.
> Moving it to drivers/of makes sense on my eyes.
> 
> Acked-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> > 
> > I'll need those acks before I can even think about queuing up the
> > imx-drm bits.
> > 
> > Another way to deal with this is if this gets pulled into the V4L tree
> > from Philipp's git tree, I can also pull that in myself.  What mustn't
> > happen is for these to be committed independently as patches.
> 
> If everyone agrees, I actually prefer have this patch applied on my tree,
> in order to avoid some potential merge conflicts at the merge window,
> as we might have other drivers and changes there touching on those API
> calls (I'm aware of a series of patches from Sylwester with some DT
> stuff on it. Not sure if it would be affected by such changes or not).

No. I disagree. Aside from the api changes this is primarily a drivers/of
series. I should go via me or Rob Herring. If you're concerned about
resolving conflicts then I can put it into a separate branch that you
can merge into your tree also.

g.

Re: [PATCH v6 0/8] Move device tree graph parsing helpers to drivers/of

[Grant Likely]

On Thu, 6 Mar 2014 15:50:18 +0000, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> On Thu, Mar 06, 2014 at 04:39:39PM +0100, Philipp Zabel wrote:
> > Am Donnerstag, den 06.03.2014, 15:24 +0000 schrieb Russell King - ARM
> > Linux:
> > > On Wed, Mar 05, 2014 at 10:20:34AM +0100, Philipp Zabel wrote:
> > > > this version of the OF graph helper move series further addresses a few of
> > > > Tomi's and Sylwester's comments.
> > > 
> > > Philipp,
> > > 
> > > You mention in your other cover for imx-drm bits that this is available
> > > via:
> > > 
> > > 	git://git.pengutronix.de/git/pza/linux.git topic/of-graph
> > > 
> > > What is this tree based upon?
> > 
> > It is based on v3.14-rc5.
> 
> Great.  As everyone seems happy, can you both send me and Mauro a pull
> request for this please?  Once Mauro says it's in his tree, I'll pull
> it in for imx-drm stuff.
> 
> Thanks.

I'm really not. There are still unresolved issues.

g.

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

[Philipp Zabel]

On Fri, Mar 07, 2014 at 06:38:02PM +0000, Grant Likely wrote:
> On Wed,  5 Mar 2014 10:20:40 +0100, Philipp Zabel <p.zabel@pengutronix.de> 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.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> Ergh... I think this is too loosely defined. The caller really should be
> explicit about which behaviour it needs. I'll listen to arguments
> though if you can make a strong argument.

I have dropped this patch and the corresponding documentation patch for
now. This simplification is a separate issue from the move and there is
no consensus yet.
Basically the main issue with the port { endpoint { remote-endpoint=... } }
binding is that it is very verbose if you just need a single link.
Instead of removing the port node, we could also remove the endpoint node
and have the remote-endpoint property direcly in the port node.

regards
Philipp

Re: [PATCH v6 2/8] Documentation: of: Document graph bindings

[Philipp Zabel]

Hi Grant,

Am Freitag, den 07.03.2014, 18:27 +0000 schrieb Grant Likely:
> On Wed,  5 Mar 2014 10:20:36 +0100, Philipp Zabel <p.zabel@pengutronix.de> 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
> > describes the generic bindings.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> See my comments on the previous version. My concerns are the handling of
> the optional 'ports' node and the usage of reverse links.

would this change address your concern about the reverse links? As the
preexisting video-interfaces.txt bindings mandate the reverse links, I
worry about introducing a second, subtly different binding. It should be
noted somewhere in video-interfaces.txt that the reverse links are
deprecated for the but still supported by the code for backwards
compatibility.

diff --git a/Documentation/devicetree/bindings/graph.txt b/Documentation/devicetree/bindings/graph.txt
index 1a69c07..eb6cae5 100644
--- a/Documentation/devicetree/bindings/graph.txt
+++ b/Documentation/devicetree/bindings/graph.txt
@@ -87,12 +87,13 @@ device {
 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.
+Two endpoint nodes form a link between the two ports they are contained in
+if one contains a 'remote-endpoint' phandle property, pointing to the other
+endpoint. The endpoint pointed to should not contain a 'remote-endpoint'
+property itself. Which direction the phandle should point in depends on the
+device type. In general, links should be pointing outwards from central
+devices that provide DMA memory interfaces, such as display controller,
+video capture interface, or serial digital audio interface cores.
 
 device-1 {
         port {
@@ -104,8 +105,8 @@ device-1 {
 
 device-2 {
         port {
-                device_2_input: endpoint {
-                        remote-endpoint = <&device_1_output>;
+                device_2_input: endpoint { };
+                       /* no remote-endpoint, this endpoint is pointed at */
                 };
         };
 };

regards
Philipp

Re: [PATCH v6 2/8] Documentation: of: Document graph bindings

[Laurent Pinchart]

Hi Philipp,

On Monday 10 March 2014 10:28:10 Philipp Zabel wrote:
> Hi Grant,
> 
> Am Freitag, den 07.03.2014, 18:27 +0000 schrieb Grant Likely:
> > On Wed,  5 Mar 2014 10:20:36 +0100, 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
> > > describes the generic bindings.
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > 
> > See my comments on the previous version. My concerns are the handling of
> > the optional 'ports' node and the usage of reverse links.
> 
> would this change address your concern about the reverse links? As the
> preexisting video-interfaces.txt bindings mandate the reverse links, I
> worry about introducing a second, subtly different binding. It should be
> noted somewhere in video-interfaces.txt that the reverse links are
> deprecated for the but still supported by the code for backwards
> compatibility.

I'm very much against removing the reverse links. Without them the graph will 
become much more complex to parse. You can try to convince me, but for now I'm 
afraid it's a NACK.

> diff --git a/Documentation/devicetree/bindings/graph.txt
> b/Documentation/devicetree/bindings/graph.txt index 1a69c07..eb6cae5 100644
> --- a/Documentation/devicetree/bindings/graph.txt
> +++ b/Documentation/devicetree/bindings/graph.txt
> @@ -87,12 +87,13 @@ device {
>  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.
> +Two endpoint nodes form a link between the two ports they are contained in
> +if one contains a 'remote-endpoint' phandle property, pointing to the other
> +endpoint. The endpoint pointed to should not contain a 'remote-endpoint'
> +property itself. Which direction the phandle should point in depends on
> the +device type. In general, links should be pointing outwards from
> central +devices that provide DMA memory interfaces, such as display
> controller, +video capture interface, or serial digital audio interface
> cores.
> 
>  device-1 {
>          port {
> @@ -104,8 +105,8 @@ device-1 {
> 
>  device-2 {
>          port {
> -                device_2_input: endpoint {
> -                        remote-endpoint = <&device_1_output>;
> +                device_2_input: endpoint { };
> +                       /* no remote-endpoint, this endpoint is pointed at
> */ };
>          };
>  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 2/8] Documentation: of: Document graph bindings

[Philipp Zabel]

Am Montag, den 10.03.2014, 12:37 +0100 schrieb Laurent Pinchart:
> Hi Philipp,
> 
> On Monday 10 March 2014 10:28:10 Philipp Zabel wrote:
> > Hi Grant,
> > 
> > Am Freitag, den 07.03.2014, 18:27 +0000 schrieb Grant Likely:
> > > On Wed,  5 Mar 2014 10:20:36 +0100, 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
> > > > describes the generic bindings.
> > > > 
> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > 
> > > See my comments on the previous version. My concerns are the handling of
> > > the optional 'ports' node and the usage of reverse links.
> > 
> > would this change address your concern about the reverse links? As the
> > preexisting video-interfaces.txt bindings mandate the reverse links, I
> > worry about introducing a second, subtly different binding. It should be
> > noted somewhere in video-interfaces.txt that the reverse links are
> > deprecated for the but still supported by the code for backwards
> > compatibility.
> 
> I'm very much against removing the reverse links. Without them the graph will 
> become much more complex to parse. You can try to convince me, but for now I'm 
> afraid it's a NACK.

For the record, I'd prefer to keep them, too. Besides the parsing
complexity, it just feels more natural to take both ends and connect
them together. If phandles are only permitted to point in one direction,
there's always the additional question which direction is the right one.

Assume, for example, the following setup of two SoC digital audio
interfaces connected to an audio codec and a bluetooth chip,
respectively. The audio codec has a second audio interface that is
connected directly to the bluetooth chip for headset operation:

,-------.     ,--------.
| dai1 [0]---[0] codec |
`-------'  ,-[1]       |
           |  `--------'
           |  ,-----.
,-------.  `-[1] bt |
| dai2 [0]---[0]    |
`-------'     `-----´

How to decide which direction the codec:1 <-->  bt:1 link should point
in?

regards
Philipp

Re: [PATCH v6 4/8] of: Reduce indentation in of_graph_get_next_endpoint

[Laurent Pinchart]

Hi Philipp,

On Friday 07 March 2014 18:40:54 Philipp Zabel wrote:
> Am Freitag, den 07.03.2014, 01:12 +0100 schrieb Laurent Pinchart:
> > Hi Philipp,
> > 
> > Thank you for the patch.
> > 
> > I've submitted a fix for the of_graph_get_next_endpoint() function, but it
> > hasn't been applied yet due to the patch series that contained it needing
> > more work.
> > 
> > The patch is available at https://patchwork.linuxtv.org/patch/21946/. I
> > can rebase it on top of this series, but I still wanted to let you know
> > about it in case you would like to integrate it.
> 
> Thank you for the pointer. A pity about the timing, this will mostly
> revert my indentation patch. I'd be glad if you could rebase on top of
> the merged series.
> 
> While we look at of_graph_get_next_endpoint(), could you explain the
> reason behind the extra reference count increase on the prev node:
>
> 	/*
> 	 * Avoid dropping prev node refcount to 0 when getting the next
> 	 * child below.
> 	 */
> 	of_node_get(prev);
>
> This unfortunately makes using the function in for_each style macros a
> hassle. If that part wasn't there and all users that want to keep using
> prev after the call were expected to increase refcount themselves,
> we could have a
>
> #define of_graph_for_each_endpoint(parent, endpoint) \
> 	for (endpoint = of_graph_get_next_endpoint(parent, NULL); \
> 	     endpoint != NULL; \
> 	     endpoint = of_graph_get_next_endpoint(parent, endpoint))

I don't know what the exact design decision was (Sylwester might know), but I 
suspect it's mostly about historical reasons. I see no reason that would 
prevent modifying the current behaviour to make a for-each loop easier to 
implement.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 4/8] of: Reduce indentation in of_graph_get_next_endpoint

[Philipp Zabel]

Hi Laurent,

Am Montag, den 10.03.2014, 20:19 +0100 schrieb Laurent Pinchart:
> On Friday 07 March 2014 18:40:54 Philipp Zabel wrote:
> > While we look at of_graph_get_next_endpoint(), could you explain the
> > reason behind the extra reference count increase on the prev node:
> >
> > 	/*
> > 	 * Avoid dropping prev node refcount to 0 when getting the next
> > 	 * child below.
> > 	 */
> > 	of_node_get(prev);
> >
> > This unfortunately makes using the function in for_each style macros a
> > hassle. If that part wasn't there and all users that want to keep using
> > prev after the call were expected to increase refcount themselves,
> > we could have a
> >
> > #define of_graph_for_each_endpoint(parent, endpoint) \
> > 	for (endpoint = of_graph_get_next_endpoint(parent, NULL); \
> > 	     endpoint != NULL; \
> > 	     endpoint = of_graph_get_next_endpoint(parent, endpoint))
> 
> I don't know what the exact design decision was (Sylwester might know), but I 
> suspect it's mostly about historical reasons. I see no reason that would 
> prevent modifying the current behaviour to make a for-each loop easier to 
> implement.

Thanks, I'll include a patch to change this in the next round, then.

regards
Philipp