[PATCH v4 0/3] Move device tree graph parsing helpers to drivers/of

[PATCH v4 0/3] Move device tree graph parsing helpers to drivers/of

[Philipp Zabel]

Hi,

this version moves the graph helpers to drivers/of again instead of
drivers/media. Since the location changed again, I have dropped the
Acks. A second patch is added that splits out the common parts from
v4l2_of_parse_endpoint into of_graph_parse_endpoint and I have added
a binding description Documentation/devicetree/bindings/graph.txt.

Changes since v3:
 - Moved back to drivers/of
 - Added DT binding documentation

regards
Philipp

Philipp Zabel (3):
  [media] of: move graph helpers from drivers/media/v4l2-core to
    drivers/of
  [media] of: move common endpoint parsing to drivers/of
  Documentation: of: Document graph bindings

 Documentation/devicetree/bindings/graph.txt   |  98 +++++++++++++++
 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             | 133 +--------------------
 drivers/of/Makefile                           |   1 +
 drivers/of/of_graph.c                         | 166 ++++++++++++++++++++++++++
 include/linux/of_graph.h                      |  66 ++++++++++
 include/media/v4l2-of.h                       |  34 +-----
 14 files changed, 355 insertions(+), 172 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/graph.txt
 create mode 100644 drivers/of/of_graph.c
 create mode 100644 include/linux/of_graph.h

-- 
1.8.5.3

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

[Philipp Zabel]

From: Philipp Zabel <philipp.zabel@gmail.com>

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 to drivers/of.

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

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v3:
 - Moved back to drivers/of
---
 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/Makefile                           |   1 +
 drivers/of/of_graph.c                         | 134 ++++++++++++++++++++++++++
 include/linux/of_graph.h                      |  46 +++++++++
 include/media/v4l2-of.h                       |  25 +----
 13 files changed, 199 insertions(+), 153 deletions(-)
 create mode 100644 drivers/of/of_graph.c
 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/Makefile b/drivers/of/Makefile
index efd0510..b4a4bc7 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
 obj-$(CONFIG_OF_PCI)	+= of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_MTD)	+= of_mtd.o
+obj-$(CONFIG_OF)	+= of_graph.o
diff --git a/drivers/of/of_graph.c b/drivers/of/of_graph.c
new file mode 100644
index 0000000..267d8f7
--- /dev/null
+++ b/drivers/of/of_graph.c
@@ -0,0 +1,134 @@
+/*
+ * OF graph binding parsing library
+ *
+ * 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.
+ */
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/types.h>
+
+/**
+ * of_graph_get_next_endpoint() - get next endpoint node
+ * @parent: pointer to the parent device node
+ * @prev: previous endpoint node, or NULL to get first
+ *
+ * Return: An 'endpoint' node pointer with refcount incremented. Refcount
+ * of the passed @prev node is not decremented, the caller have to use
+ * of_node_put() on it when done.
+ */
+struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
+					struct device_node *prev)
+{
+	struct device_node *endpoint;
+	struct device_node *port = NULL;
+
+	if (!parent)
+		return NULL;
+
+	if (!prev) {
+		struct device_node *node;
+		/*
+		 * It's the first call, we have to find a port subnode
+		 * within this node or within an optional 'ports' node.
+		 */
+		node = of_get_child_by_name(parent, "ports");
+		if (node)
+			parent = node;
+
+		port = of_get_child_by_name(parent, "port");
+
+		if (port) {
+			/* Found a port, get an endpoint. */
+			endpoint = of_get_next_child(port, NULL);
+			of_node_put(port);
+		} else {
+			endpoint = NULL;
+		}
+
+		if (!endpoint)
+			pr_err("%s(): no endpoint nodes specified for %s\n",
+			       __func__, parent->full_name);
+		of_node_put(node);
+	} else {
+		port = of_get_parent(prev);
+		if (!port)
+			/* Hm, has someone given us the root node ?... */
+			return NULL;
+
+		/* Avoid dropping prev node refcount to 0. */
+		of_node_get(prev);
+		endpoint = of_get_next_child(port, prev);
+		if (endpoint) {
+			of_node_put(port);
+			return endpoint;
+		}
+
+		/* No more endpoints under this port, try the next one. */
+		do {
+			port = of_get_next_child(parent, port);
+			if (!port)
+				return NULL;
+		} while (of_node_cmp(port->name, "port"));
+
+		/* Pick up the first endpoint in this port. */
+		endpoint = of_get_next_child(port, NULL);
+		of_node_put(port);
+	}
+
+	return endpoint;
+}
+EXPORT_SYMBOL(of_graph_get_next_endpoint);
+
+/**
+ * of_graph_get_remote_port_parent() - get remote port's parent node
+ * @node: pointer to a local endpoint device_node
+ *
+ * Return: Remote device node associated with remote endpoint node linked
+ *	   to @node. Use of_node_put() on it when done.
+ */
+struct device_node *of_graph_get_remote_port_parent(
+			       const struct device_node *node)
+{
+	struct device_node *np;
+	unsigned int depth;
+
+	/* Get remote endpoint node. */
+	np = of_parse_phandle(node, "remote-endpoint", 0);
+
+	/* Walk 3 levels up only if there is 'ports' node. */
+	for (depth = 3; depth && np; depth--) {
+		np = of_get_next_parent(np);
+		if (depth == 2 && of_node_cmp(np->name, "ports"))
+			break;
+	}
+	return np;
+}
+EXPORT_SYMBOL(of_graph_get_remote_port_parent);
+
+/**
+ * of_graph_get_remote_port() - get remote port node
+ * @node: pointer to a local endpoint device_node
+ *
+ * Return: Remote port node associated with remote endpoint node linked
+ *	   to @node. Use of_node_put() on it when done.
+ */
+struct device_node *of_graph_get_remote_port(const struct device_node *node)
+{
+	struct device_node *np;
+
+	/* Get remote endpoint node. */
+	np = of_parse_phandle(node, "remote-endpoint", 0);
+	if (!np)
+		return NULL;
+	return of_get_next_parent(np);
+}
+EXPORT_SYMBOL(of_graph_get_remote_port);
diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
new file mode 100644
index 0000000..3bbeb60
--- /dev/null
+++ b/include/linux/of_graph.h
@@ -0,0 +1,46 @@
+/*
+ * OF graph binding parsing helpers
+ *
+ * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd.
+ * Author: Sylwester Nawrocki <s.nawrocki@samsung.com>
+ *
+ * Copyright (C) 2012 Renesas Electronics Corp.
+ * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ */
+#ifndef __LINUX_OF_GRAPH_H
+#define __LINUX_OF_GRAPH_H
+
+#ifdef CONFIG_OF
+struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
+					struct device_node *previous);
+struct device_node *of_graph_get_remote_port_parent(
+					const struct device_node *node);
+struct device_node *of_graph_get_remote_port(const struct device_node *node);
+#else
+
+static inline struct device_node *of_graph_get_next_endpoint(
+					const struct device_node *parent,
+					struct device_node *previous)
+{
+	return NULL;
+}
+
+static inline struct device_node *of_graph_get_remote_port_parent(
+					const struct device_node *node)
+{
+	return NULL;
+}
+
+static inline struct device_node *of_graph_get_remote_port(
+					const struct device_node *node)
+{
+	return NULL;
+}
+
+#endif /* CONFIG_OF */
+
+#endif /* __LINUX_OF_GRAPH_H */
diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
index 541cea4..3a49735 100644
--- a/include/media/v4l2-of.h
+++ b/include/media/v4l2-of.h
@@ -17,6 +17,7 @@
 #include <linux/list.h>
 #include <linux/types.h>
 #include <linux/errno.h>
+#include <linux/of_graph.h>
 
 #include <media/v4l2-mediabus.h>
 
@@ -72,11 +73,6 @@ struct v4l2_of_endpoint {
 #ifdef CONFIG_OF
 int v4l2_of_parse_endpoint(const struct device_node *node,
 			   struct v4l2_of_endpoint *endpoint);
-struct device_node *v4l2_of_get_next_endpoint(const struct device_node *parent,
-					struct device_node *previous);
-struct device_node *v4l2_of_get_remote_port_parent(
-					const struct device_node *node);
-struct device_node *v4l2_of_get_remote_port(const struct device_node *node);
 #else /* CONFIG_OF */
 
 static inline int v4l2_of_parse_endpoint(const struct device_node *node,
@@ -85,25 +81,6 @@ static inline int v4l2_of_parse_endpoint(const struct device_node *node,
 	return -ENOSYS;
 }
 
-static inline struct device_node *v4l2_of_get_next_endpoint(
-					const struct device_node *parent,
-					struct device_node *previous)
-{
-	return NULL;
-}
-
-static inline struct device_node *v4l2_of_get_remote_port_parent(
-					const struct device_node *node)
-{
-	return NULL;
-}
-
-static inline struct device_node *v4l2_of_get_remote_port(
-					const struct device_node *node)
-{
-	return NULL;
-}
-
 #endif /* CONFIG_OF */
 
 #endif /* _V4L2_OF_H */
-- 
1.8.5.3

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

[Grant Likely]

On Tue, 25 Feb 2014 15:58:22 +0100, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> From: Philipp Zabel <philipp.zabel@gmail.com>
> 
> 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 to drivers/of.
> 
> This allows to reuse the same parser code from outside the V4L2
> framework, most importantly from display drivers.
> The functions v4l2_of_get_next_endpoint, v4l2_of_get_remote_port,
> and v4l2_of_get_remote_port_parent are moved. They are renamed to
> of_graph_get_next_endpoint, of_graph_get_remote_port, and
> of_graph_get_remote_port_parent, respectively.
> Since there are not that many current users yet, switch all of
> them to the new functions right away.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v3:
>  - Moved back to drivers/of
> ---
>  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/Makefile                           |   1 +
>  drivers/of/of_graph.c                         | 134 ++++++++++++++++++++++++++

Nah. Just put it into drivers/of/base.c. This isn't a separate subsystem
and the functions are pretty basic.

>  include/linux/of_graph.h                      |  46 +++++++++
>  include/media/v4l2-of.h                       |  25 +----
>  13 files changed, 199 insertions(+), 153 deletions(-)
>  create mode 100644 drivers/of/of_graph.c
>  create mode 100644 include/linux/of_graph.h
> 
[...]
> diff --git a/drivers/of/of_graph.c b/drivers/of/of_graph.c
> new file mode 100644
> index 0000000..267d8f7
> --- /dev/null
> +++ b/drivers/of/of_graph.c
> @@ -0,0 +1,134 @@
> +/*
> + * OF graph binding parsing library
> + *
> + * 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.
> + */
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/types.h>
> +
> +/**
> + * 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 you've got a "ports" node, then I would expect every single child to
be a port. Should not need the _by_name variant.

It seems that this function is merely a helper to get all grandchildren
of a node (with some very minor constraints). That could be generalized
and simplified. If the function takes the "ports" node as an argument
instead of the parent, then there is a greater likelyhood that other
code can make use of it...

Thinking further. I think the semantics of this whole feature basically
boil down to this:

#define for_each_grandchild_of_node(parent, child, grandchild) \
	for_each_child_of_node(parent, child) \
		for_each_child_of_node(child, grandchild)

Correct? Or in this specific case:

	parent = of_get_child_by_name(np, "ports")
	for_each_grandchild_of_node(parent, child, grandchild) {
		...
	}

Finally, looking at the actual patch, is any of this actually needed.
All of the users updated by this patch only ever handle a single
endpoint. Have I read it correctly? Are there any users supporting
multiple endpoints?

> +
> +		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);

If you 'return endpoint' here, then the else block can go down a level.

> +	} else {
> +		port = of_get_parent(prev);
> +		if (!port)
> +			/* Hm, has someone given us the root node ?... */
> +			return NULL;

WARN_ONCE(). That's a very definite coding failure if that happens.

> +
> +		/* 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. */

This needs a some explaining. My reading of the binding pattern is that
it will always be a fixed number of levels. Why is this test fuzzy?

> +	for (depth = 3; depth && np; depth--) {
> +		np = of_get_next_parent(np);
> +		if (depth == 2 && of_node_cmp(np->name, "ports"))
> +			break;
> +	}
> +	return np;
> +}
> +EXPORT_SYMBOL(of_graph_get_remote_port_parent);
> +
> +/**
> + * of_graph_get_remote_port() - get remote port node
> + * @node: pointer to a local endpoint device_node
> + *
> + * Return: Remote port node associated with remote endpoint node linked
> + *	   to @node. Use of_node_put() on it when done.
> + */
> +struct device_node *of_graph_get_remote_port(const struct device_node *node)
> +{
> +	struct device_node *np;
> +
> +	/* Get remote endpoint node. */
> +	np = of_parse_phandle(node, "remote-endpoint", 0);
> +	if (!np)
> +		return NULL;
> +	return of_get_next_parent(np);
> +}
> +EXPORT_SYMBOL(of_graph_get_remote_port);
> diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
> new file mode 100644
> index 0000000..3bbeb60
> --- /dev/null
> +++ b/include/linux/of_graph.h
> @@ -0,0 +1,46 @@
> +/*
> + * OF graph binding parsing helpers
> + *
> + * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd.
> + * Author: Sylwester Nawrocki <s.nawrocki@samsung.com>
> + *
> + * Copyright (C) 2012 Renesas Electronics Corp.
> + * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of version 2 of the GNU General Public License as
> + * published by the Free Software Foundation.
> + */
> +#ifndef __LINUX_OF_GRAPH_H
> +#define __LINUX_OF_GRAPH_H
> +
> +#ifdef CONFIG_OF
> +struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
> +					struct device_node *previous);
> +struct device_node *of_graph_get_remote_port_parent(
> +					const struct device_node *node);
> +struct device_node *of_graph_get_remote_port(const struct device_node *node);
> +#else
> +
> +static inline struct device_node *of_graph_get_next_endpoint(
> +					const struct device_node *parent,
> +					struct device_node *previous)
> +{
> +	return NULL;
> +}
> +
> +static inline struct device_node *of_graph_get_remote_port_parent(
> +					const struct device_node *node)
> +{
> +	return NULL;
> +}
> +
> +static inline struct device_node *of_graph_get_remote_port(
> +					const struct device_node *node)
> +{
> +	return NULL;
> +}
> +
> +#endif /* CONFIG_OF */
> +
> +#endif /* __LINUX_OF_GRAPH_H */
> diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
> index 541cea4..3a49735 100644
> --- a/include/media/v4l2-of.h
> +++ b/include/media/v4l2-of.h
> @@ -17,6 +17,7 @@
>  #include <linux/list.h>
>  #include <linux/types.h>
>  #include <linux/errno.h>
> +#include <linux/of_graph.h>
>  
>  #include <media/v4l2-mediabus.h>
>  
> @@ -72,11 +73,6 @@ struct v4l2_of_endpoint {
>  #ifdef CONFIG_OF
>  int v4l2_of_parse_endpoint(const struct device_node *node,
>  			   struct v4l2_of_endpoint *endpoint);
> -struct device_node *v4l2_of_get_next_endpoint(const struct device_node *parent,
> -					struct device_node *previous);
> -struct device_node *v4l2_of_get_remote_port_parent(
> -					const struct device_node *node);
> -struct device_node *v4l2_of_get_remote_port(const struct device_node *node);
>  #else /* CONFIG_OF */
>  
>  static inline int v4l2_of_parse_endpoint(const struct device_node *node,
> @@ -85,25 +81,6 @@ static inline int v4l2_of_parse_endpoint(const struct device_node *node,
>  	return -ENOSYS;
>  }
>  
> -static inline struct device_node *v4l2_of_get_next_endpoint(
> -					const struct device_node *parent,
> -					struct device_node *previous)
> -{
> -	return NULL;
> -}
> -
> -static inline struct device_node *v4l2_of_get_remote_port_parent(
> -					const struct device_node *node)
> -{
> -	return NULL;
> -}
> -
> -static inline struct device_node *v4l2_of_get_remote_port(
> -					const struct device_node *node)
> -{
> -	return NULL;
> -}
> -
>  #endif /* CONFIG_OF */
>  
>  #endif /* _V4L2_OF_H */
> -- 
> 1.8.5.3
> 

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

[Grant Likely]

On Tue, 25 Feb 2014 15:58:22 +0100, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> From: Philipp Zabel <philipp.zabel@gmail.com>
> 
> 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 to drivers/of.
> 
> This allows to reuse the same parser code from outside the V4L2
> framework, most importantly from display drivers.
> The functions v4l2_of_get_next_endpoint, v4l2_of_get_remote_port,
> and v4l2_of_get_remote_port_parent are moved. They are renamed to
> of_graph_get_next_endpoint, of_graph_get_remote_port, and
> of_graph_get_remote_port_parent, respectively.
> Since there are not that many current users yet, switch all of
> them to the new functions right away.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v3:
>  - Moved back to drivers/of
> ---
>  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/Makefile                           |   1 +
>  drivers/of/of_graph.c                         | 134 ++++++++++++++++++++++++++

Nah. Just put it into drivers/of/base.c. This isn't a separate subsystem
and the functions are pretty basic.

>  include/linux/of_graph.h                      |  46 +++++++++
>  include/media/v4l2-of.h                       |  25 +----
>  13 files changed, 199 insertions(+), 153 deletions(-)
>  create mode 100644 drivers/of/of_graph.c
>  create mode 100644 include/linux/of_graph.h
> 
[...]
> diff --git a/drivers/of/of_graph.c b/drivers/of/of_graph.c
> new file mode 100644
> index 0000000..267d8f7
> --- /dev/null
> +++ b/drivers/of/of_graph.c
> @@ -0,0 +1,134 @@
> +/*
> + * OF graph binding parsing library
> + *
> + * 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.
> + */
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/types.h>
> +
> +/**
> + * 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 you've got a "ports" node, then I would expect every single child to
be a port. Should not need the _by_name variant.

It seems that this function is merely a helper to get all grandchildren
of a node (with some very minor constraints). That could be generalized
and simplified. If the function takes the "ports" node as an argument
instead of the parent, then there is a greater likelyhood that other
code can make use of it...

Thinking further. I think the semantics of this whole feature basically
boil down to this:

#define for_each_grandchild_of_node(parent, child, grandchild) \
	for_each_child_of_node(parent, child) \
		for_each_child_of_node(child, grandchild)

Correct? Or in this specific case:

	parent = of_get_child_by_name(np, "ports")
	for_each_grandchild_of_node(parent, child, grandchild) {
		...
	}

Finally, looking at the actual patch, is any of this actually needed.
All of the users updated by this patch only ever handle a single
endpoint. Have I read it correctly? Are there any users supporting
multiple endpoints?

> +
> +		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);

If you 'return endpoint' here, then the else block can go down a level.

> +	} else {
> +		port = of_get_parent(prev);
> +		if (!port)
> +			/* Hm, has someone given us the root node ?... */
> +			return NULL;

WARN_ONCE(). That's a very definite coding failure if that happens.

> +
> +		/* 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. */

This needs a some explaining. My reading of the binding pattern is that
it will always be a fixed number of levels. Why is this test fuzzy?

> +	for (depth = 3; depth && np; depth--) {
> +		np = of_get_next_parent(np);
> +		if (depth == 2 && of_node_cmp(np->name, "ports"))
> +			break;
> +	}
> +	return np;
> +}
> +EXPORT_SYMBOL(of_graph_get_remote_port_parent);
> +
> +/**
> + * of_graph_get_remote_port() - get remote port node
> + * @node: pointer to a local endpoint device_node
> + *
> + * Return: Remote port node associated with remote endpoint node linked
> + *	   to @node. Use of_node_put() on it when done.
> + */
> +struct device_node *of_graph_get_remote_port(const struct device_node *node)
> +{
> +	struct device_node *np;
> +
> +	/* Get remote endpoint node. */
> +	np = of_parse_phandle(node, "remote-endpoint", 0);
> +	if (!np)
> +		return NULL;
> +	return of_get_next_parent(np);
> +}
> +EXPORT_SYMBOL(of_graph_get_remote_port);
> diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
> new file mode 100644
> index 0000000..3bbeb60
> --- /dev/null
> +++ b/include/linux/of_graph.h
> @@ -0,0 +1,46 @@
> +/*
> + * OF graph binding parsing helpers
> + *
> + * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd.
> + * Author: Sylwester Nawrocki <s.nawrocki@samsung.com>
> + *
> + * Copyright (C) 2012 Renesas Electronics Corp.
> + * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of version 2 of the GNU General Public License as
> + * published by the Free Software Foundation.
> + */
> +#ifndef __LINUX_OF_GRAPH_H
> +#define __LINUX_OF_GRAPH_H
> +
> +#ifdef CONFIG_OF
> +struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
> +					struct device_node *previous);
> +struct device_node *of_graph_get_remote_port_parent(
> +					const struct device_node *node);
> +struct device_node *of_graph_get_remote_port(const struct device_node *node);
> +#else
> +
> +static inline struct device_node *of_graph_get_next_endpoint(
> +					const struct device_node *parent,
> +					struct device_node *previous)
> +{
> +	return NULL;
> +}
> +
> +static inline struct device_node *of_graph_get_remote_port_parent(
> +					const struct device_node *node)
> +{
> +	return NULL;
> +}
> +
> +static inline struct device_node *of_graph_get_remote_port(
> +					const struct device_node *node)
> +{
> +	return NULL;
> +}
> +
> +#endif /* CONFIG_OF */
> +
> +#endif /* __LINUX_OF_GRAPH_H */
> diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
> index 541cea4..3a49735 100644
> --- a/include/media/v4l2-of.h
> +++ b/include/media/v4l2-of.h
> @@ -17,6 +17,7 @@
>  #include <linux/list.h>
>  #include <linux/types.h>
>  #include <linux/errno.h>
> +#include <linux/of_graph.h>
>  
>  #include <media/v4l2-mediabus.h>
>  
> @@ -72,11 +73,6 @@ struct v4l2_of_endpoint {
>  #ifdef CONFIG_OF
>  int v4l2_of_parse_endpoint(const struct device_node *node,
>  			   struct v4l2_of_endpoint *endpoint);
> -struct device_node *v4l2_of_get_next_endpoint(const struct device_node *parent,
> -					struct device_node *previous);
> -struct device_node *v4l2_of_get_remote_port_parent(
> -					const struct device_node *node);
> -struct device_node *v4l2_of_get_remote_port(const struct device_node *node);
>  #else /* CONFIG_OF */
>  
>  static inline int v4l2_of_parse_endpoint(const struct device_node *node,
> @@ -85,25 +81,6 @@ static inline int v4l2_of_parse_endpoint(const struct device_node *node,
>  	return -ENOSYS;
>  }
>  
> -static inline struct device_node *v4l2_of_get_next_endpoint(
> -					const struct device_node *parent,
> -					struct device_node *previous)
> -{
> -	return NULL;
> -}
> -
> -static inline struct device_node *v4l2_of_get_remote_port_parent(
> -					const struct device_node *node)
> -{
> -	return NULL;
> -}
> -
> -static inline struct device_node *v4l2_of_get_remote_port(
> -					const struct device_node *node)
> -{
> -	return NULL;
> -}
> -
>  #endif /* CONFIG_OF */
>  
>  #endif /* _V4L2_OF_H */
> -- 
> 1.8.5.3
> 

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

[Philipp Zabel]

Hi Grant,

Am Mittwoch, den 26.02.2014, 11:37 +0000 schrieb Grant Likely:
[...]
> >  drivers/media/v4l2-core/v4l2-of.c             | 117 ----------------------
> >  drivers/of/Makefile                           |   1 +
> >  drivers/of/of_graph.c                         | 134 ++++++++++++++++++++++++++
> 
> Nah. Just put it into drivers/of/base.c. This isn't a separate subsystem
> and the functions are pretty basic.

Ok.

[...]
> > +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 you've got a "ports" node, then I would expect every single child to
> be a port. Should not need the _by_name variant.

The 'ports' node is optional. It is only needed if the parent node has
its own #address-cells and #size-cells properties. If the ports are
direct children of the device node, there might be other nodes than
ports:

	device {
		#address-cells = <1>;
		#size-cells = <0>;

		port@0 {
			endpoint { ... };
		};
		port@1 {
			endpoint { ... };
		};

		some-other-child { ... };
	};

	device {
		#address-cells = <x>;
		#size-cells = <y>;

		ports {
			#address-cells = <1>;
			#size-cells = <0>;

			port@0 {
				endpoint { ... };
			};
			port@1 {
				endpoint { ... };
			};
		};

		some-other-child { ... };
	};

The helper should find the two endpoints in both cases.
Tomi suggests an even more compact form for devices with just one port:

	device {
		endpoint { ... };

		some-other-child { ... };
	};

> It seems that this function is merely a helper to get all grandchildren
> of a node (with some very minor constraints). That could be generalized
> and simplified. If the function takes the "ports" node as an argument
> instead of the parent, then there is a greater likelyhood that other
> code can make use of it...
> 
> Thinking further. I think the semantics of this whole feature basically
> boil down to this:
> 
> #define for_each_grandchild_of_node(parent, child, grandchild) \
> 	for_each_child_of_node(parent, child) \
> 		for_each_child_of_node(child, grandchild)
> 
> Correct? Or in this specific case:
> 
> 	parent = of_get_child_by_name(np, "ports")
> 	for_each_grandchild_of_node(parent, child, grandchild) {
> 		...
> 	}

Hmm, that would indeed be a bit more generic, but it doesn't handle the
optional 'ports' subnode and doesn't allow for other child nodes in the
device node.

> Finally, looking at the actual patch, is any of this actually needed.
> All of the users updated by this patch only ever handle a single
> endpoint. Have I read it correctly? Are there any users supporting
> multiple endpoints?

Yes, mainline currently only contains simple cases. I have posted i.MX6
patches that use this scheme for the output path:
  http://www.spinics.net/lists/arm-kernel/msg310817.html
  http://www.spinics.net/lists/arm-kernel/msg310821.html

> > +
> > +		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);
> 
> If you 'return endpoint' here, then the else block can go down a level.

Note that this patch is a straight move of existing code.
I can follow up with code beautification and ...

> > +	} else {
> > +		port = of_get_parent(prev);
> > +		if (!port)
> > +			/* Hm, has someone given us the root node ?... */
> > +			return NULL;
> 
> WARN_ONCE(). That's a very definite coding failure if that happens.

... with a fix for this.

> > +
> > +		/* 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. */
> 
> This needs a some explaining. My reading of the binding pattern is that
> it will always be a fixed number of levels. Why is this test fuzzy?
[...]

See above. The ports subnode level is optional. In most cases, the port
nodes will be direct children of the device node.
Walking up 3 levels from the endpoint node will return the device if
there was a ports node. If there is no ports node, we only have to walk
up two levels.

regards
Philipp

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

[Grant Likely]

On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi Grant,
> 
> Am Mittwoch, den 26.02.2014, 11:37 +0000 schrieb Grant Likely:
> [...]
> > >  drivers/media/v4l2-core/v4l2-of.c             | 117 ----------------------
> > >  drivers/of/Makefile                           |   1 +
> > >  drivers/of/of_graph.c                         | 134 ++++++++++++++++++++++++++
> > 
> > Nah. Just put it into drivers/of/base.c. This isn't a separate subsystem
> > and the functions are pretty basic.
> 
> Ok.
> 
> [...]
> > > +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 you've got a "ports" node, then I would expect every single child to
> > be a port. Should not need the _by_name variant.
> 
> The 'ports' node is optional. It is only needed if the parent node has
> its own #address-cells and #size-cells properties. If the ports are
> direct children of the device node, there might be other nodes than
> ports:
> 
> 	device {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		port@0 {
> 			endpoint { ... };
> 		};
> 		port@1 {
> 			endpoint { ... };
> 		};
> 
> 		some-other-child { ... };
> 	};
> 
> 	device {
> 		#address-cells = <x>;
> 		#size-cells = <y>;
> 
> 		ports {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			port@0 {
> 				endpoint { ... };
> 			};
> 			port@1 {
> 				endpoint { ... };
> 			};
> 		};
> 
> 		some-other-child { ... };
> 	};

>From a pattern perspective I have no problem with that.... From an
individual driver binding perspective that is just dumb! It's fine for
the ports node to be optional, but an individual driver using the
binding should be explicit about which it will accept. Please use either
a flag or a separate wrapper so that the driver can select the
behaviour.

> The helper should find the two endpoints in both cases.
> Tomi suggests an even more compact form for devices with just one port:
> 
> 	device {
> 		endpoint { ... };
> 
> 		some-other-child { ... };
> 	};

That's fine. In that case the driver would specifically require the
endpoint to be that one node.... although the above looks a little weird
to me. I would recommend that if there are other non-port child nodes
then the ports should still be encapsulated by a ports node.  The device
binding should not be ambiguous about which nodes are ports.

> > It seems that this function is merely a helper to get all grandchildren
> > of a node (with some very minor constraints). That could be generalized
> > and simplified. If the function takes the "ports" node as an argument
> > instead of the parent, then there is a greater likelyhood that other
> > code can make use of it...
> > 
> > Thinking further. I think the semantics of this whole feature basically
> > boil down to this:
> > 
> > #define for_each_grandchild_of_node(parent, child, grandchild) \
> > 	for_each_child_of_node(parent, child) \
> > 		for_each_child_of_node(child, grandchild)
> > 
> > Correct? Or in this specific case:
> > 
> > 	parent = of_get_child_by_name(np, "ports")
> > 	for_each_grandchild_of_node(parent, child, grandchild) {
> > 		...
> > 	}
> 
> Hmm, that would indeed be a bit more generic, but it doesn't handle the
> optional 'ports' subnode and doesn't allow for other child nodes in the
> device node.

See above. The no-ports-node version could be the
for_each_grandchild_of_node() block, and the yes-ports-node version
could be a wrapper around that.

> > Finally, looking at the actual patch, is any of this actually needed.
> > All of the users updated by this patch only ever handle a single
> > endpoint. Have I read it correctly? Are there any users supporting
> > multiple endpoints?
> 
> Yes, mainline currently only contains simple cases. I have posted i.MX6
> patches that use this scheme for the output path:
>   http://www.spinics.net/lists/arm-kernel/msg310817.html
>   http://www.spinics.net/lists/arm-kernel/msg310821.html

Blurg. On a plane right now. Can't go and read those links.

> > > +
> > > +		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);
> > 
> > If you 'return endpoint' here, then the else block can go down a level.
> 
> Note that this patch is a straight move of existing code.
> I can follow up with code beautification and ...

I'm fine with that.

g.

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

[Tomi Valkeinen]

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

On 07/03/14 19:18, Grant Likely wrote:

> From a pattern perspective I have no problem with that.... From an
> individual driver binding perspective that is just dumb! It's fine for
> the ports node to be optional, but an individual driver using the
> binding should be explicit about which it will accept. Please use either
> a flag or a separate wrapper so that the driver can select the
> behaviour.

Why is that? The meaning of the DT data stays the same, regardless of
the existence of the 'ports' node. The driver uses the graph helpers to
parse the port/endpoint data, so individual drivers don't even have to
care about the format used.

As I see it, the graph helpers should allow the drivers to iterate the
ports and the endpoints for a port. These should work the same way, no
matter which abbreviated format is used in the dts.

>> The helper should find the two endpoints in both cases.
>> Tomi suggests an even more compact form for devices with just one port:
>>
>> 	device {
>> 		endpoint { ... };
>>
>> 		some-other-child { ... };
>> 	};
> 
> That's fine. In that case the driver would specifically require the
> endpoint to be that one node.... although the above looks a little weird

The driver can't require that. It's up to the board designer to decide
how many endpoints are used. A driver may say that it has a single input
port. But the number of endpoints for that port is up to the use case.

> to me. I would recommend that if there are other non-port child nodes
> then the ports should still be encapsulated by a ports node.  The device
> binding should not be ambiguous about which nodes are ports.

Hmm, ambiguous in what way?

If the dts uses 'ports' node, all the ports and endpoints are inside
that 'ports' node. If there is no 'ports' node, there may be one or more
'port' nodes, which then contain endpoints. If there are no 'port'
nodes, there may be a single 'endpoint' node.

True, there are many "if"s there. But I don't think it's ambiguous. The
reason we have these abbreviations is that the full 'ports' node is not
needed that often, and it is rather verbose. In almost all the use
cases, panels and connectors can use the single endpoint format.

 Tomi



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

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

[Tomi Valkeinen]

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

On 07/03/14 19:18, Grant Likely wrote:

> From a pattern perspective I have no problem with that.... From an
> individual driver binding perspective that is just dumb! It's fine for
> the ports node to be optional, but an individual driver using the
> binding should be explicit about which it will accept. Please use either
> a flag or a separate wrapper so that the driver can select the
> behaviour.

Why is that? The meaning of the DT data stays the same, regardless of
the existence of the 'ports' node. The driver uses the graph helpers to
parse the port/endpoint data, so individual drivers don't even have to
care about the format used.

As I see it, the graph helpers should allow the drivers to iterate the
ports and the endpoints for a port. These should work the same way, no
matter which abbreviated format is used in the dts.

>> The helper should find the two endpoints in both cases.
>> Tomi suggests an even more compact form for devices with just one port:
>>
>> 	device {
>> 		endpoint { ... };
>>
>> 		some-other-child { ... };
>> 	};
> 
> That's fine. In that case the driver would specifically require the
> endpoint to be that one node.... although the above looks a little weird

The driver can't require that. It's up to the board designer to decide
how many endpoints are used. A driver may say that it has a single input
port. But the number of endpoints for that port is up to the use case.

> to me. I would recommend that if there are other non-port child nodes
> then the ports should still be encapsulated by a ports node.  The device
> binding should not be ambiguous about which nodes are ports.

Hmm, ambiguous in what way?

If the dts uses 'ports' node, all the ports and endpoints are inside
that 'ports' node. If there is no 'ports' node, there may be one or more
'port' nodes, which then contain endpoints. If there are no 'port'
nodes, there may be a single 'endpoint' node.

True, there are many "if"s there. But I don't think it's ambiguous. The
reason we have these abbreviations is that the full 'ports' node is not
needed that often, and it is rather verbose. In almost all the use
cases, panels and connectors can use the single endpoint format.

 Tomi



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

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

[Grant Likely]

On Sat, 8 Mar 2014 12:46:05 +0200, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 07/03/14 19:18, Grant Likely wrote:
> 
> > From a pattern perspective I have no problem with that.... From an
> > individual driver binding perspective that is just dumb! It's fine for
> > the ports node to be optional, but an individual driver using the
> > binding should be explicit about which it will accept. Please use either
> > a flag or a separate wrapper so that the driver can select the
> > behaviour.
> 
> Why is that? The meaning of the DT data stays the same, regardless of
> the existence of the 'ports' node. The driver uses the graph helpers to
> parse the port/endpoint data, so individual drivers don't even have to
> care about the format used.

You don't want to give options to the device tree writer. It should work
one way and one way only. Every different combination is a different
permutation to get wrong. The only time we should be allowing for more
than one way to do it is when there is an existing binding that has
proven to be insufficient and it needs to be extended, such as was done
with interrupts-extended to deal with deficiencies in the interrupts
property.

> As I see it, the graph helpers should allow the drivers to iterate the
> ports and the endpoints for a port. These should work the same way, no
> matter which abbreviated format is used in the dts.
>
> >> The helper should find the two endpoints in both cases.
> >> Tomi suggests an even more compact form for devices with just one port:
> >>
> >> 	device {
> >> 		endpoint { ... };
> >>
> >> 		some-other-child { ... };
> >> 	};
> > 
> > That's fine. In that case the driver would specifically require the
> > endpoint to be that one node.... although the above looks a little weird
> 
> The driver can't require that. It's up to the board designer to decide
> how many endpoints are used. A driver may say that it has a single input
> port. But the number of endpoints for that port is up to the use case.

Come now, when you're writing a driver you know if it will ever be
possible to have more than one port. If that is the case then the
binding should be specifically laid out for that. If there will never be
multiple ports and the binding is unambiguous, then, and only then,
should the shortcut be used, and only the shortcut should be accepted.

> > to me. I would recommend that if there are other non-port child nodes
> > then the ports should still be encapsulated by a ports node.  The device
> > binding should not be ambiguous about which nodes are ports.
> 
> Hmm, ambiguous in what way?

Parsing the binding now consists of a ladder of 'ifs' that gives three
distinct different behaviours for no benefit. You don't want that in
bindings because it makes it more difficult to get the parsing right in
the first place, and to make sure that all users parse it in the same
way (Linux, U-Boot, BSD, etc). Bindings should be as absolutely simple
as possible.

Just to be clear, I have no problem with having the option in the
pattern, but the driver needs to be specific about what layout it
expects.

g.

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

[Laurent Pinchart]

Hi Grant,

On Saturday 08 March 2014 12:23:21 Grant Likely wrote:
> On Sat, 8 Mar 2014 12:46:05 +0200, Tomi Valkeinen wrote:
> > On 07/03/14 19:18, Grant Likely wrote:
> > > From a pattern perspective I have no problem with that.... From an
> > > individual driver binding perspective that is just dumb! It's fine for
> > > the ports node to be optional, but an individual driver using the
> > > binding should be explicit about which it will accept. Please use either
> > > a flag or a separate wrapper so that the driver can select the
> > > behaviour.
> > 
> > Why is that? The meaning of the DT data stays the same, regardless of
> > the existence of the 'ports' node. The driver uses the graph helpers to
> > parse the port/endpoint data, so individual drivers don't even have to
> > care about the format used.
> 
> You don't want to give options to the device tree writer. It should work
> one way and one way only. Every different combination is a different
> permutation to get wrong. The only time we should be allowing for more
> than one way to do it is when there is an existing binding that has
> proven to be insufficient and it needs to be extended, such as was done
> with interrupts-extended to deal with deficiencies in the interrupts
> property.
> 
> > As I see it, the graph helpers should allow the drivers to iterate the
> > ports and the endpoints for a port. These should work the same way, no
> > matter which abbreviated format is used in the dts.
> > 
> > >> The helper should find the two endpoints in both cases.
> > >> 
> > >> Tomi suggests an even more compact form for devices with just one port:
> > >> 	device {
> > >> 	
> > >> 		endpoint { ... };
> > >> 		
> > >> 		some-other-child { ... };
> > >> 	
> > >> 	};
> > > 
> > > That's fine. In that case the driver would specifically require the
> > > endpoint to be that one node.... although the above looks a little weird
> > 
> > The driver can't require that. It's up to the board designer to decide
> > how many endpoints are used. A driver may say that it has a single input
> > port. But the number of endpoints for that port is up to the use case.
> 
> Come now, when you're writing a driver you know if it will ever be
> possible to have more than one port. If that is the case then the
> binding should be specifically laid out for that. If there will never be
> multiple ports and the binding is unambiguous, then, and only then,
> should the shortcut be used, and only the shortcut should be accepted.

Whether multiple nodes are possible for a device is indeed known to the driver 
author, but the number of endpoints depends on the board. In most cases 
multiple endpoints are possible. If we decide that the level of simplification 
should be set in stone in the device DT bindings (i.e. making the ports and/or 
port nodes required or forbidden, but not optional), then I believe this calls 
for always having a port node even when a single port is needed. I'm fine with 
leaving the ports node out, but having no port node might be too close to 
over-simplification.

> > > to me. I would recommend that if there are other non-port child nodes
> > > then the ports should still be encapsulated by a ports node.  The device
> > > binding should not be ambiguous about which nodes are ports.
> > 
> > Hmm, ambiguous in what way?
> 
> Parsing the binding now consists of a ladder of 'ifs' that gives three
> distinct different behaviours for no benefit. You don't want that in
> bindings because it makes it more difficult to get the parsing right in
> the first place, and to make sure that all users parse it in the same
> way (Linux, U-Boot, BSD, etc). Bindings should be as absolutely simple
> as possible.
> 
> Just to be clear, I have no problem with having the option in the
> pattern, but the driver needs to be specific about what layout it
> expects.

-- 
Regards,

Laurent Pinchart

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

[Grant Likely]

On Sat, 08 Mar 2014 16:50:33 +0100, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> Hi Grant,
> 
> On Saturday 08 March 2014 12:23:21 Grant Likely wrote:
> > On Sat, 8 Mar 2014 12:46:05 +0200, Tomi Valkeinen wrote:
> > > On 07/03/14 19:18, Grant Likely wrote:
> > > > From a pattern perspective I have no problem with that.... From an
> > > > individual driver binding perspective that is just dumb! It's fine for
> > > > the ports node to be optional, but an individual driver using the
> > > > binding should be explicit about which it will accept. Please use either
> > > > a flag or a separate wrapper so that the driver can select the
> > > > behaviour.
> > > 
> > > Why is that? The meaning of the DT data stays the same, regardless of
> > > the existence of the 'ports' node. The driver uses the graph helpers to
> > > parse the port/endpoint data, so individual drivers don't even have to
> > > care about the format used.
> > 
> > You don't want to give options to the device tree writer. It should work
> > one way and one way only. Every different combination is a different
> > permutation to get wrong. The only time we should be allowing for more
> > than one way to do it is when there is an existing binding that has
> > proven to be insufficient and it needs to be extended, such as was done
> > with interrupts-extended to deal with deficiencies in the interrupts
> > property.
> > 
> > > As I see it, the graph helpers should allow the drivers to iterate the
> > > ports and the endpoints for a port. These should work the same way, no
> > > matter which abbreviated format is used in the dts.
> > > 
> > > >> The helper should find the two endpoints in both cases.
> > > >> 
> > > >> Tomi suggests an even more compact form for devices with just one port:
> > > >> 	device {
> > > >> 	
> > > >> 		endpoint { ... };
> > > >> 		
> > > >> 		some-other-child { ... };
> > > >> 	
> > > >> 	};
> > > > 
> > > > That's fine. In that case the driver would specifically require the
> > > > endpoint to be that one node.... although the above looks a little weird
> > > 
> > > The driver can't require that. It's up to the board designer to decide
> > > how many endpoints are used. A driver may say that it has a single input
> > > port. But the number of endpoints for that port is up to the use case.
> > 
> > Come now, when you're writing a driver you know if it will ever be
> > possible to have more than one port. If that is the case then the
> > binding should be specifically laid out for that. If there will never be
> > multiple ports and the binding is unambiguous, then, and only then,
> > should the shortcut be used, and only the shortcut should be accepted.
> 
> Whether multiple nodes are possible for a device is indeed known to the driver 
> author, but the number of endpoints depends on the board. In most cases 
> multiple endpoints are possible. If we decide that the level of simplification 
> should be set in stone in the device DT bindings (i.e. making the ports and/or 
> port nodes required or forbidden, but not optional), then I believe this calls 
> for always having a port node even when a single port is needed. I'm fine with 
> leaving the ports node out, but having no port node might be too close to 
> over-simplification.

Just to make sure we're on the same page (and that I'm parsing
correctly): Are you saying the individual 'port' nodes should be
required? Or that the container 'ports' node should always be required?

If you're saying that the individual port nodes should be required, then
yes I agree. I'm still a little uncomfortable about there being a choice
between the link directly in the port node or in a child endpoint node,
but I'll compromise on this if we put sanity checking into the API (as I
replied on the other thread).

Whether or not a container 'ports' node is present I think should be
defined by the device binding. It really comes down to organization of
data. If the device is sufficiently complex that it makes sense to group
the ports together (say, because the device has other children with a
different purpose), then 'ports' makes absolute sense.

g.

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

[Laurent Pinchart]

Hi Grant,

On Thursday 20 March 2014 22:23:47 Grant Likely wrote:
> On Sat, 08 Mar 2014 16:50:33 +0100, Laurent Pinchart wrote:
> > On Saturday 08 March 2014 12:23:21 Grant Likely wrote:
> > > On Sat, 8 Mar 2014 12:46:05 +0200, Tomi Valkeinen wrote:
> > > > On 07/03/14 19:18, Grant Likely wrote:
> > > > > From a pattern perspective I have no problem with that.... From an
> > > > > individual driver binding perspective that is just dumb! It's fine
> > > > > for the ports node to be optional, but an individual driver using
> > > > > the binding should be explicit about which it will accept. Please
> > > > > use either a flag or a separate wrapper so that the driver can
> > > > > select the behaviour.
> > > > 
> > > > Why is that? The meaning of the DT data stays the same, regardless of
> > > > the existence of the 'ports' node. The driver uses the graph helpers
> > > > to parse the port/endpoint data, so individual drivers don't even have
> > > > to care about the format used.
> > > 
> > > You don't want to give options to the device tree writer. It should work
> > > one way and one way only. Every different combination is a different
> > > permutation to get wrong. The only time we should be allowing for more
> > > than one way to do it is when there is an existing binding that has
> > > proven to be insufficient and it needs to be extended, such as was done
> > > with interrupts-extended to deal with deficiencies in the interrupts
> > > property.
> > > 
> > > > As I see it, the graph helpers should allow the drivers to iterate the
> > > > ports and the endpoints for a port. These should work the same way, no
> > > > matter which abbreviated format is used in the dts.
> > > > 
> > > > >> The helper should find the two endpoints in both cases.
> > > > >> 
> > > > >> Tomi suggests an even more compact form for devices with just one
> > > > >> port:
> > > > >>
> > > > >> 	device {
> > > > >> 		endpoint { ... };
> > > > >> 		
> > > > >> 		some-other-child { ... };
> > > > >> 	};
> > > > > 
> > > > > That's fine. In that case the driver would specifically require the
> > > > > endpoint to be that one node.... although the above looks a little
> > > > > weird
> > > > 
> > > > The driver can't require that. It's up to the board designer to decide
> > > > how many endpoints are used. A driver may say that it has a single
> > > > input port. But the number of endpoints for that port is up to the use
> > > > case.
> > > 
> > > Come now, when you're writing a driver you know if it will ever be
> > > possible to have more than one port. If that is the case then the
> > > binding should be specifically laid out for that. If there will never be
> > > multiple ports and the binding is unambiguous, then, and only then,
> > > should the shortcut be used, and only the shortcut should be accepted.
> > 
> > Whether multiple nodes are possible for a device is indeed known to the
> > driver author, but the number of endpoints depends on the board. In most
> > cases multiple endpoints are possible. If we decide that the level of
> > simplification should be set in stone in the device DT bindings (i.e.
> > making the ports and/or port nodes required or forbidden, but not
> > optional), then I believe this calls for always having a port node even
> > when a single port is needed. I'm fine with leaving the ports node out,
> > but having no port node might be too close to over-simplification.
> 
> Just to make sure we're on the same page (and that I'm parsing correctly):
> Are you saying the individual 'port' nodes should be required? Or that the
> container 'ports' node should always be required?
> 
> If you're saying that the individual port nodes should be required, then
> yes I agree.

That's what I meant, yes. And I'm glad that we finally agree on something, 
even if it's a detail :-)

> I'm still a little uncomfortable about there being a choice between the link
> directly in the port node or in a child endpoint node, but I'll compromise
> on this if we put sanity checking into the API (as I replied on the other
> thread).
>
> Whether or not a container 'ports' node is present I think should be defined
> by the device binding. It really comes down to organization of data. If the
> device is sufficiently complex that it makes sense to group the ports
> together (say, because the device has other children with a different
> purpose), then 'ports' makes absolute sense.

I'm fine with that, as that's what the ports node has been originally designed 
for.

The OF graph bindings documentation could just specify the ports node as 
optional and mandate individual device bindings to specify it as mandatory or 
forbidden (possibly with a default behaviour to avoid making all device 
bindings too verbose).

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Grant,

On Thursday 20 March 2014 22:23:47 Grant Likely wrote:
> On Sat, 08 Mar 2014 16:50:33 +0100, Laurent Pinchart wrote:
> > On Saturday 08 March 2014 12:23:21 Grant Likely wrote:
> > > On Sat, 8 Mar 2014 12:46:05 +0200, Tomi Valkeinen wrote:
> > > > On 07/03/14 19:18, Grant Likely wrote:
> > > > > From a pattern perspective I have no problem with that.... From an
> > > > > individual driver binding perspective that is just dumb! It's fine
> > > > > for the ports node to be optional, but an individual driver using
> > > > > the binding should be explicit about which it will accept. Please
> > > > > use either a flag or a separate wrapper so that the driver can
> > > > > select the behaviour.
> > > > 
> > > > Why is that? The meaning of the DT data stays the same, regardless of
> > > > the existence of the 'ports' node. The driver uses the graph helpers
> > > > to parse the port/endpoint data, so individual drivers don't even have
> > > > to care about the format used.
> > > 
> > > You don't want to give options to the device tree writer. It should work
> > > one way and one way only. Every different combination is a different
> > > permutation to get wrong. The only time we should be allowing for more
> > > than one way to do it is when there is an existing binding that has
> > > proven to be insufficient and it needs to be extended, such as was done
> > > with interrupts-extended to deal with deficiencies in the interrupts
> > > property.
> > > 
> > > > As I see it, the graph helpers should allow the drivers to iterate the
> > > > ports and the endpoints for a port. These should work the same way, no
> > > > matter which abbreviated format is used in the dts.
> > > > 
> > > > >> The helper should find the two endpoints in both cases.
> > > > >> 
> > > > >> Tomi suggests an even more compact form for devices with just one
> > > > >> port:
> > > > >>
> > > > >> 	device {
> > > > >> 		endpoint { ... };
> > > > >> 		
> > > > >> 		some-other-child { ... };
> > > > >> 	};
> > > > > 
> > > > > That's fine. In that case the driver would specifically require the
> > > > > endpoint to be that one node.... although the above looks a little
> > > > > weird
> > > > 
> > > > The driver can't require that. It's up to the board designer to decide
> > > > how many endpoints are used. A driver may say that it has a single
> > > > input port. But the number of endpoints for that port is up to the use
> > > > case.
> > > 
> > > Come now, when you're writing a driver you know if it will ever be
> > > possible to have more than one port. If that is the case then the
> > > binding should be specifically laid out for that. If there will never be
> > > multiple ports and the binding is unambiguous, then, and only then,
> > > should the shortcut be used, and only the shortcut should be accepted.
> > 
> > Whether multiple nodes are possible for a device is indeed known to the
> > driver author, but the number of endpoints depends on the board. In most
> > cases multiple endpoints are possible. If we decide that the level of
> > simplification should be set in stone in the device DT bindings (i.e.
> > making the ports and/or port nodes required or forbidden, but not
> > optional), then I believe this calls for always having a port node even
> > when a single port is needed. I'm fine with leaving the ports node out,
> > but having no port node might be too close to over-simplification.
> 
> Just to make sure we're on the same page (and that I'm parsing correctly):
> Are you saying the individual 'port' nodes should be required? Or that the
> container 'ports' node should always be required?
> 
> If you're saying that the individual port nodes should be required, then
> yes I agree.

That's what I meant, yes. And I'm glad that we finally agree on something, 
even if it's a detail :-)

> I'm still a little uncomfortable about there being a choice between the link
> directly in the port node or in a child endpoint node, but I'll compromise
> on this if we put sanity checking into the API (as I replied on the other
> thread).
>
> Whether or not a container 'ports' node is present I think should be defined
> by the device binding. It really comes down to organization of data. If the
> device is sufficiently complex that it makes sense to group the ports
> together (say, because the device has other children with a different
> purpose), then 'ports' makes absolute sense.

I'm fine with that, as that's what the ports node has been originally designed 
for.

The OF graph bindings documentation could just specify the ports node as 
optional and mandate individual device bindings to specify it as mandatory or 
forbidden (possibly with a default behaviour to avoid making all device 
bindings too verbose).

-- 
Regards,

Laurent Pinchart

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

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

[Tomi Valkeinen]

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

On 21/03/14 00:32, Laurent Pinchart wrote:

> The OF graph bindings documentation could just specify the ports node as 
> optional and mandate individual device bindings to specify it as mandatory or 
> forbidden (possibly with a default behaviour to avoid making all device 
> bindings too verbose).

Isn't it so that if the device has one port, it can always do without
'ports', but if it has multiple ports, it always has to use 'ports' so
that #address-cells and #size-cells can be defined?

If so, there's nothing left for the individual device bindings to decide.

 Tomi



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

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

[Tomi Valkeinen]

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

On 21/03/14 00:32, Laurent Pinchart wrote:

> The OF graph bindings documentation could just specify the ports node as 
> optional and mandate individual device bindings to specify it as mandatory or 
> forbidden (possibly with a default behaviour to avoid making all device 
> bindings too verbose).

Isn't it so that if the device has one port, it can always do without
'ports', but if it has multiple ports, it always has to use 'ports' so
that #address-cells and #size-cells can be defined?

If so, there's nothing left for the individual device bindings to decide.

 Tomi



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

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

[Sylwester Nawrocki]

On 21/03/14 14:37, Tomi Valkeinen wrote:
> On 21/03/14 00:32, Laurent Pinchart wrote:
> 
>> > The OF graph bindings documentation could just specify the ports node as 
>> > optional and mandate individual device bindings to specify it as mandatory or 
>> > forbidden (possibly with a default behaviour to avoid making all device 
>> > bindings too verbose).
>
> Isn't it so that if the device has one port, it can always do without
> 'ports', but if it has multiple ports, it always has to use 'ports' so
> that #address-cells and #size-cells can be defined?
> 
> If so, there's nothing left for the individual device bindings to decide.

Wouldn't it make the bindings even more verbose ? Letting the individual
device bindings to decide sounds more sensible to me.

--
Thanks,
Sylwester

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

[Laurent Pinchart]

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

Hi Tomi,

On Friday 21 March 2014 15:37:17 Tomi Valkeinen wrote:
> On 21/03/14 00:32, Laurent Pinchart wrote:
> > The OF graph bindings documentation could just specify the ports node as
> > optional and mandate individual device bindings to specify it as mandatory
> > or forbidden (possibly with a default behaviour to avoid making all
> > device bindings too verbose).
> 
> Isn't it so that if the device has one port, it can always do without
> 'ports', but if it has multiple ports, it always has to use 'ports' so
> that #address-cells and #size-cells can be defined?

You can put the #address-cells and #size-cells property in the device node 
directly without requiring a ports subnode.

> If so, there's nothing left for the individual device bindings to decide.


-- 
Regards,

Laurent Pinchart

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

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

[Laurent Pinchart]

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

Hi Tomi,

On Friday 21 March 2014 15:37:17 Tomi Valkeinen wrote:
> On 21/03/14 00:32, Laurent Pinchart wrote:
> > The OF graph bindings documentation could just specify the ports node as
> > optional and mandate individual device bindings to specify it as mandatory
> > or forbidden (possibly with a default behaviour to avoid making all
> > device bindings too verbose).
> 
> Isn't it so that if the device has one port, it can always do without
> 'ports', but if it has multiple ports, it always has to use 'ports' so
> that #address-cells and #size-cells can be defined?

You can put the #address-cells and #size-cells property in the device node 
directly without requiring a ports subnode.

> If so, there's nothing left for the individual device bindings to decide.


-- 
Regards,

Laurent Pinchart

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

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

[Tomi Valkeinen]

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

On 21/03/14 16:13, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Friday 21 March 2014 15:37:17 Tomi Valkeinen wrote:
>> On 21/03/14 00:32, Laurent Pinchart wrote:
>>> The OF graph bindings documentation could just specify the ports node as
>>> optional and mandate individual device bindings to specify it as mandatory
>>> or forbidden (possibly with a default behaviour to avoid making all
>>> device bindings too verbose).
>>
>> Isn't it so that if the device has one port, it can always do without
>> 'ports', but if it has multiple ports, it always has to use 'ports' so
>> that #address-cells and #size-cells can be defined?
> 
> You can put the #address-cells and #size-cells property in the device node 
> directly without requiring a ports subnode.

Ah, right. So 'ports' is only needed when the device node has other
children nodes than the ports and those nodes need different
#address-cells and #size-cells than the ports.

In that case it sounds fine to leave it for the driver bindings to decide.

 Tomi



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

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

[Tomi Valkeinen]

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

On 21/03/14 16:13, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Friday 21 March 2014 15:37:17 Tomi Valkeinen wrote:
>> On 21/03/14 00:32, Laurent Pinchart wrote:
>>> The OF graph bindings documentation could just specify the ports node as
>>> optional and mandate individual device bindings to specify it as mandatory
>>> or forbidden (possibly with a default behaviour to avoid making all
>>> device bindings too verbose).
>>
>> Isn't it so that if the device has one port, it can always do without
>> 'ports', but if it has multiple ports, it always has to use 'ports' so
>> that #address-cells and #size-cells can be defined?
> 
> You can put the #address-cells and #size-cells property in the device node 
> directly without requiring a ports subnode.

Ah, right. So 'ports' is only needed when the device node has other
children nodes than the ports and those nodes need different
#address-cells and #size-cells than the ports.

In that case it sounds fine to leave it for the driver bindings to decide.

 Tomi



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

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

[Laurent Pinchart]

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

Hi Tomi,

On Friday 21 March 2014 16:22:52 Tomi Valkeinen wrote:
> On 21/03/14 16:13, Laurent Pinchart wrote:
> > On Friday 21 March 2014 15:37:17 Tomi Valkeinen wrote:
> >> On 21/03/14 00:32, Laurent Pinchart wrote:
> >>> The OF graph bindings documentation could just specify the ports node as
> >>> optional and mandate individual device bindings to specify it as
> >>> mandatory or forbidden (possibly with a default behaviour to avoid
> >>> making all device bindings too verbose).
> >> 
> >> Isn't it so that if the device has one port, it can always do without
> >> 'ports', but if it has multiple ports, it always has to use 'ports' so
> >> that #address-cells and #size-cells can be defined?
> > 
> > You can put the #address-cells and #size-cells property in the device node
> > directly without requiring a ports subnode.
> 
> Ah, right. So 'ports' is only needed when the device node has other children
> nodes than the ports and those nodes need different #address-cells and
> #size-cells than the ports.

I would rephrase that as the ports node being required only in that case. It 
can also be useful to cleanly group ports together when the device node has 
other unrelated children, even though no technical requirement exist (yet ?) 
in that case.

> In that case it sounds fine to leave it for the driver bindings to decide.

-- 
Regards,

Laurent Pinchart

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

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

[Laurent Pinchart]

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

Hi Tomi,

On Friday 21 March 2014 16:22:52 Tomi Valkeinen wrote:
> On 21/03/14 16:13, Laurent Pinchart wrote:
> > On Friday 21 March 2014 15:37:17 Tomi Valkeinen wrote:
> >> On 21/03/14 00:32, Laurent Pinchart wrote:
> >>> The OF graph bindings documentation could just specify the ports node as
> >>> optional and mandate individual device bindings to specify it as
> >>> mandatory or forbidden (possibly with a default behaviour to avoid
> >>> making all device bindings too verbose).
> >> 
> >> Isn't it so that if the device has one port, it can always do without
> >> 'ports', but if it has multiple ports, it always has to use 'ports' so
> >> that #address-cells and #size-cells can be defined?
> > 
> > You can put the #address-cells and #size-cells property in the device node
> > directly without requiring a ports subnode.
> 
> Ah, right. So 'ports' is only needed when the device node has other children
> nodes than the ports and those nodes need different #address-cells and
> #size-cells than the ports.

I would rephrase that as the ports node being required only in that case. It 
can also be useful to cleanly group ports together when the device node has 
other unrelated children, even though no technical requirement exist (yet ?) 
in that case.

> In that case it sounds fine to leave it for the driver bindings to decide.

-- 
Regards,

Laurent Pinchart

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

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

[Tomi Valkeinen]

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

On 08/03/14 14:23, Grant Likely wrote:

>>> That's fine. In that case the driver would specifically require the
>>> endpoint to be that one node.... although the above looks a little weird
>>
>> The driver can't require that. It's up to the board designer to decide
>> how many endpoints are used. A driver may say that it has a single input
>> port. But the number of endpoints for that port is up to the use case.
> 
> Come now, when you're writing a driver you know if it will ever be
> possible to have more than one port. If that is the case then the
> binding should be specifically laid out for that. If there will never be
> multiple ports and the binding is unambiguous, then, and only then,
> should the shortcut be used, and only the shortcut should be accepted.

I was talking about endpoints, not ports. There's no unclarity about the
number of ports, that comes directly from the hardware for that specific
component. The number of endpoints, however, come from the board
hardware. The driver writer cannot know that.

>>> to me. I would recommend that if there are other non-port child nodes
>>> then the ports should still be encapsulated by a ports node.  The device
>>> binding should not be ambiguous about which nodes are ports.
>>
>> Hmm, ambiguous in what way?
> 
> Parsing the binding now consists of a ladder of 'ifs' that gives three
> distinct different behaviours for no benefit. You don't want that in

It considerably lessens the amount of text in the DT for many use cases,
making it easier to write and maintain the dts files.

> bindings because it makes it more difficult to get the parsing right in
> the first place, and to make sure that all users parse it in the same
> way (Linux, U-Boot, BSD, etc). Bindings should be as absolutely simple
> as possible.

Well, yes, I agree there. This is not the simplest of bindings. I'd be
more than happy if we would come up with simpler version of this, which
would still allow us to have the same descriptive power.

> Just to be clear, I have no problem with having the option in the
> pattern, but the driver needs to be specific about what layout it
> expects.

If we forget the shortened endpoint format, I think it can be quite
specific.

A device has either one port, in which case it should require the
'ports' node to be omitted, or the device has more than one port, in
which case it should require 'ports' node.

Note that the original v4l2 binding doc says that 'ports' is always
optional.

 Tomi



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

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

[Tomi Valkeinen]

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

On 08/03/14 14:23, Grant Likely wrote:

>>> That's fine. In that case the driver would specifically require the
>>> endpoint to be that one node.... although the above looks a little weird
>>
>> The driver can't require that. It's up to the board designer to decide
>> how many endpoints are used. A driver may say that it has a single input
>> port. But the number of endpoints for that port is up to the use case.
> 
> Come now, when you're writing a driver you know if it will ever be
> possible to have more than one port. If that is the case then the
> binding should be specifically laid out for that. If there will never be
> multiple ports and the binding is unambiguous, then, and only then,
> should the shortcut be used, and only the shortcut should be accepted.

I was talking about endpoints, not ports. There's no unclarity about the
number of ports, that comes directly from the hardware for that specific
component. The number of endpoints, however, come from the board
hardware. The driver writer cannot know that.

>>> to me. I would recommend that if there are other non-port child nodes
>>> then the ports should still be encapsulated by a ports node.  The device
>>> binding should not be ambiguous about which nodes are ports.
>>
>> Hmm, ambiguous in what way?
> 
> Parsing the binding now consists of a ladder of 'ifs' that gives three
> distinct different behaviours for no benefit. You don't want that in

It considerably lessens the amount of text in the DT for many use cases,
making it easier to write and maintain the dts files.

> bindings because it makes it more difficult to get the parsing right in
> the first place, and to make sure that all users parse it in the same
> way (Linux, U-Boot, BSD, etc). Bindings should be as absolutely simple
> as possible.

Well, yes, I agree there. This is not the simplest of bindings. I'd be
more than happy if we would come up with simpler version of this, which
would still allow us to have the same descriptive power.

> Just to be clear, I have no problem with having the option in the
> pattern, but the driver needs to be specific about what layout it
> expects.

If we forget the shortened endpoint format, I think it can be quite
specific.

A device has either one port, in which case it should require the
'ports' node to be omitted, or the device has more than one port, in
which case it should require 'ports' node.

Note that the original v4l2 binding doc says that 'ports' is always
optional.

 Tomi



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

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

[Grant Likely]

On Mon, 10 Mar 2014 08:34:54 +0200, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 08/03/14 14:23, Grant Likely wrote:
> 
> >>> That's fine. In that case the driver would specifically require the
> >>> endpoint to be that one node.... although the above looks a little weird
> >>
> >> The driver can't require that. It's up to the board designer to decide
> >> how many endpoints are used. A driver may say that it has a single input
> >> port. But the number of endpoints for that port is up to the use case.
> > 
> > Come now, when you're writing a driver you know if it will ever be
> > possible to have more than one port. If that is the case then the
> > binding should be specifically laid out for that. If there will never be
> > multiple ports and the binding is unambiguous, then, and only then,
> > should the shortcut be used, and only the shortcut should be accepted.
> 
> I was talking about endpoints, not ports. There's no unclarity about the
> number of ports, that comes directly from the hardware for that specific
> component. The number of endpoints, however, come from the board
> hardware. The driver writer cannot know that.

Okay, I understand now.

g.

> > Just to be clear, I have no problem with having the option in the
> > pattern, but the driver needs to be specific about what layout it
> > expects.
> 
> If we forget the shortened endpoint format, I think it can be quite
> specific.
> 
> A device has either one port, in which case it should require the
> 'ports' node to be omitted, or the device has more than one port, in
> which case it should require 'ports' node.
> 
> Note that the original v4l2 binding doc says that 'ports' is always
> optional.

The original v4l2 behaviour doesn't need to change. In fact it should
not change if it will cause real-world breakage.

g.

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

[Philipp Zabel]

Hi Grant,

On Fri, Mar 7, 2014 at 6:18 PM, Grant Likely <grant.likely@linaro.org> wrote:
> On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> The 'ports' node is optional. It is only needed if the parent node has
>> its own #address-cells and #size-cells properties. If the ports are
>> direct children of the device node, there might be other nodes than
>> ports:
>>
>>       device {
>>               #address-cells = <1>;
>>               #size-cells = <0>;
>>
>>               port@0 {
>>                       endpoint { ... };
>>               };
>>               port@1 {
>>                       endpoint { ... };
>>               };
>>
>>               some-other-child { ... };
>>       };
>>
>>       device {
>>               #address-cells = <x>;
>>               #size-cells = <y>;
>>
>>               ports {
>>                       #address-cells = <1>;
>>                       #size-cells = <0>;
>>
>>                       port@0 {
>>                               endpoint { ... };
>>                       };
>>                       port@1 {
>>                               endpoint { ... };
>>                       };
>>               };
>>
>>               some-other-child { ... };
>>       };
>
> From a pattern perspective I have no problem with that.... From an
> individual driver binding perspective that is just dumb! It's fine for
> the ports node to be optional, but an individual driver using the
> binding should be explicit about which it will accept. Please use either
> a flag or a separate wrapper so that the driver can select the
> behaviour.

If the generic binding exists in both forms, most drivers should be
able to cope with both. Maybe it should be mentioned in the bindings
that the short form without ports node should be used where possible
(i.e. for devices that don't already have #address,size-cells != 1,0).

Having a separate wrapper to enforce the ports node for devices that
need it might be useful.

>> The helper should find the two endpoints in both cases.
>> Tomi suggests an even more compact form for devices with just one port:
>>
>>       device {
>>               endpoint { ... };
>>
>>               some-other-child { ... };
>>       };
>
> That's fine. In that case the driver would specifically require the
> endpoint to be that one node.... although the above looks a little weird
> to me. I would recommend that if there are other non-port child nodes
> then the ports should still be encapsulated by a ports node.  The device
> binding should not be ambiguous about which nodes are ports.

Sylwester suggested as an alternative, if I understood correctly, to
drop the endpoint node and instead keep the port:

    device-a {
        implicit_output_ep: port {
            remote-endpoint = <&explicit_input_ep>;
        };
    };

    device-b {
        port {
            explicit_input_ep: endpoint {
                remote-endpoint = <&implicit_output_ep>;
            };
        };
    };

This would have the advantage to reduce verbosity for devices with multiple
ports that are only connected via one endport each, and you'd always have
the connected ports in the device tree as 'port' nodes.

>> > It seems that this function is merely a helper to get all grandchildren
>> > of a node (with some very minor constraints). That could be generalized
>> > and simplified. If the function takes the "ports" node as an argument
>> > instead of the parent, then there is a greater likelyhood that other
>> > code can make use of it...
>> >
>> > Thinking further. I think the semantics of this whole feature basically
>> > boil down to this:
>> >
>> > #define for_each_grandchild_of_node(parent, child, grandchild) \
>> >     for_each_child_of_node(parent, child) \
>> >             for_each_child_of_node(child, grandchild)
>> >
>> > Correct? Or in this specific case:
>> >
>> >     parent = of_get_child_by_name(np, "ports")
>> >     for_each_grandchild_of_node(parent, child, grandchild) {
>> >             ...
>> >     }
>>
>> Hmm, that would indeed be a bit more generic, but it doesn't handle the
>> optional 'ports' subnode and doesn't allow for other child nodes in the
>> device node.
>
> See above. The no-ports-node version could be the
> for_each_grandchild_of_node() block, and the yes-ports-node version
> could be a wrapper around that.

For the yes-ports-node version I see no problem, but without the ports node,
for_each_grandchild_of_node would also collect the children of non-port
child nodes.
The port and endpoint nodes in this binding are identified by their name,
so maybe adding of_get_next_child_by_name() /
for_each_named_child_of_node() could be helpful here.

>> > Finally, looking at the actual patch, is any of this actually needed.
>> > All of the users updated by this patch only ever handle a single
>> > endpoint. Have I read it correctly? Are there any users supporting
>> > multiple endpoints?
>>
>> Yes, mainline currently only contains simple cases. I have posted i.MX6
>> patches that use this scheme for the output path:
>>   http://www.spinics.net/lists/arm-kernel/msg310817.html
>>   http://www.spinics.net/lists/arm-kernel/msg310821.html
>
> Blurg. On a plane right now. Can't go and read those links.

The patches are merged into the staging tree now at bfe24b9.

regards
Philipp

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

[Laurent Pinchart]

Hi Philipp,

On Saturday 08 March 2014 13:07:23 Philipp Zabel wrote:
> On Fri, Mar 7, 2014 at 6:18 PM, Grant Likely wrote:
> > On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel wrote:
> >> The 'ports' node is optional. It is only needed if the parent node has
> >> its own #address-cells and #size-cells properties. If the ports are
> >> direct children of the device node, there might be other nodes than
> >> 
> >> ports:
> >>       device {
> >>               #address-cells = <1>;
> >>               #size-cells = <0>;
> >>               
> >>               port@0 {
> >>                       endpoint { ... };
> >>               };
> >>               port@1 {
> >>                       endpoint { ... };
> >>               };
> >>               
> >>               some-other-child { ... };
> >>       };
> >>       
> >>       device {
> >>               #address-cells = <x>;
> >>               #size-cells = <y>;
> >>               
> >>               ports {
> >>                       #address-cells = <1>;
> >>                       #size-cells = <0>;
> >>                       
> >>                       port@0 {
> >>                               endpoint { ... };
> >>                       };
> >>                       port@1 {
> >>                               endpoint { ... };
> >>                       };
> >>               };
> >>               
> >>               some-other-child { ... };
> >>       };
> > 
> > From a pattern perspective I have no problem with that.... From an
> > individual driver binding perspective that is just dumb! It's fine for
> > the ports node to be optional, but an individual driver using the
> > binding should be explicit about which it will accept. Please use either
> > a flag or a separate wrapper so that the driver can select the
> > behaviour.
> 
> If the generic binding exists in both forms, most drivers should be
> able to cope with both. Maybe it should be mentioned in the bindings
> that the short form without ports node should be used where possible
> (i.e. for devices that don't already have #address,size-cells != 1,0).
> 
> Having a separate wrapper to enforce the ports node for devices that
> need it might be useful.
> 
> >> The helper should find the two endpoints in both cases.
> >> 
> >> Tomi suggests an even more compact form for devices with just one port:
> >>       device {
> >>               endpoint { ... };
> >>               
> >>               some-other-child { ... };
> >>       };
> > 
> > That's fine. In that case the driver would specifically require the
> > endpoint to be that one node.... although the above looks a little weird
> > to me. I would recommend that if there are other non-port child nodes
> > then the ports should still be encapsulated by a ports node.  The device
> > binding should not be ambiguous about which nodes are ports.
> 
> Sylwester suggested as an alternative, if I understood correctly, to
> drop the endpoint node and instead keep the port:
> 
>     device-a {
>         implicit_output_ep: port {
>             remote-endpoint = <&explicit_input_ep>;
>         };
>     };
> 
>     device-b {
>         port {
>             explicit_input_ep: endpoint {
>                 remote-endpoint = <&implicit_output_ep>;
>             };
>         };
>     };
> 
> This would have the advantage to reduce verbosity for devices with multiple
> ports that are only connected via one endport each, and you'd always have
> the connected ports in the device tree as 'port' nodes.

I like that idea. I would prefer making the 'port' nodes mandatory and the 
'ports' and 'endpoint' nodes optional. Leaving the 'port' node out slightly 
decreases readability in my opinion, but making the 'endpoint' node optional 
increases it. That's just my point of view though.

> >> > It seems that this function is merely a helper to get all grandchildren
> >> > of a node (with some very minor constraints). That could be generalized
> >> > and simplified. If the function takes the "ports" node as an argument
> >> > instead of the parent, then there is a greater likelyhood that other
> >> > code can make use of it...
> >> > 
> >> > Thinking further. I think the semantics of this whole feature basically
> >> > boil down to this:
> >> > 
> >> > #define for_each_grandchild_of_node(parent, child, grandchild) \
> >> > 
> >> >     for_each_child_of_node(parent, child) \
> >> >     
> >> >             for_each_child_of_node(child, grandchild)
> >> > 
> >> > Correct? Or in this specific case:
> >> >     parent = of_get_child_by_name(np, "ports")
> >> >     for_each_grandchild_of_node(parent, child, grandchild) {
> >> >     
> >> >             ...
> >> >     
> >> >     }
> >> 
> >> Hmm, that would indeed be a bit more generic, but it doesn't handle the
> >> optional 'ports' subnode and doesn't allow for other child nodes in the
> >> device node.
> > 
> > See above. The no-ports-node version could be the
> > for_each_grandchild_of_node() block, and the yes-ports-node version
> > could be a wrapper around that.
> 
> For the yes-ports-node version I see no problem, but without the ports node,
> for_each_grandchild_of_node would also collect the children of non-port
> child nodes.
> The port and endpoint nodes in this binding are identified by their name,
> so maybe adding of_get_next_child_by_name() /
> for_each_named_child_of_node() could be helpful here.
> 
> >> > Finally, looking at the actual patch, is any of this actually needed.
> >> > All of the users updated by this patch only ever handle a single
> >> > endpoint. Have I read it correctly? Are there any users supporting
> >> > multiple endpoints?
> >> 
> >> Yes, mainline currently only contains simple cases. I have posted i.MX6
> >> 
> >> patches that use this scheme for the output path:
> >>   http://www.spinics.net/lists/arm-kernel/msg310817.html
> >>   http://www.spinics.net/lists/arm-kernel/msg310821.html
> > 
> > Blurg. On a plane right now. Can't go and read those links.
> 
> The patches are merged into the staging tree now at bfe24b9.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Philipp,

On Saturday 08 March 2014 13:07:23 Philipp Zabel wrote:
> On Fri, Mar 7, 2014 at 6:18 PM, Grant Likely wrote:
> > On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel wrote:
> >> The 'ports' node is optional. It is only needed if the parent node has
> >> its own #address-cells and #size-cells properties. If the ports are
> >> direct children of the device node, there might be other nodes than
> >> 
> >> ports:
> >>       device {
> >>               #address-cells = <1>;
> >>               #size-cells = <0>;
> >>               
> >>               port@0 {
> >>                       endpoint { ... };
> >>               };
> >>               port@1 {
> >>                       endpoint { ... };
> >>               };
> >>               
> >>               some-other-child { ... };
> >>       };
> >>       
> >>       device {
> >>               #address-cells = <x>;
> >>               #size-cells = <y>;
> >>               
> >>               ports {
> >>                       #address-cells = <1>;
> >>                       #size-cells = <0>;
> >>                       
> >>                       port@0 {
> >>                               endpoint { ... };
> >>                       };
> >>                       port@1 {
> >>                               endpoint { ... };
> >>                       };
> >>               };
> >>               
> >>               some-other-child { ... };
> >>       };
> > 
> > From a pattern perspective I have no problem with that.... From an
> > individual driver binding perspective that is just dumb! It's fine for
> > the ports node to be optional, but an individual driver using the
> > binding should be explicit about which it will accept. Please use either
> > a flag or a separate wrapper so that the driver can select the
> > behaviour.
> 
> If the generic binding exists in both forms, most drivers should be
> able to cope with both. Maybe it should be mentioned in the bindings
> that the short form without ports node should be used where possible
> (i.e. for devices that don't already have #address,size-cells != 1,0).
> 
> Having a separate wrapper to enforce the ports node for devices that
> need it might be useful.
> 
> >> The helper should find the two endpoints in both cases.
> >> 
> >> Tomi suggests an even more compact form for devices with just one port:
> >>       device {
> >>               endpoint { ... };
> >>               
> >>               some-other-child { ... };
> >>       };
> > 
> > That's fine. In that case the driver would specifically require the
> > endpoint to be that one node.... although the above looks a little weird
> > to me. I would recommend that if there are other non-port child nodes
> > then the ports should still be encapsulated by a ports node.  The device
> > binding should not be ambiguous about which nodes are ports.
> 
> Sylwester suggested as an alternative, if I understood correctly, to
> drop the endpoint node and instead keep the port:
> 
>     device-a {
>         implicit_output_ep: port {
>             remote-endpoint = <&explicit_input_ep>;
>         };
>     };
> 
>     device-b {
>         port {
>             explicit_input_ep: endpoint {
>                 remote-endpoint = <&implicit_output_ep>;
>             };
>         };
>     };
> 
> This would have the advantage to reduce verbosity for devices with multiple
> ports that are only connected via one endport each, and you'd always have
> the connected ports in the device tree as 'port' nodes.

I like that idea. I would prefer making the 'port' nodes mandatory and the 
'ports' and 'endpoint' nodes optional. Leaving the 'port' node out slightly 
decreases readability in my opinion, but making the 'endpoint' node optional 
increases it. That's just my point of view though.

> >> > It seems that this function is merely a helper to get all grandchildren
> >> > of a node (with some very minor constraints). That could be generalized
> >> > and simplified. If the function takes the "ports" node as an argument
> >> > instead of the parent, then there is a greater likelyhood that other
> >> > code can make use of it...
> >> > 
> >> > Thinking further. I think the semantics of this whole feature basically
> >> > boil down to this:
> >> > 
> >> > #define for_each_grandchild_of_node(parent, child, grandchild) \
> >> > 
> >> >     for_each_child_of_node(parent, child) \
> >> >     
> >> >             for_each_child_of_node(child, grandchild)
> >> > 
> >> > Correct? Or in this specific case:
> >> >     parent = of_get_child_by_name(np, "ports")
> >> >     for_each_grandchild_of_node(parent, child, grandchild) {
> >> >     
> >> >             ...
> >> >     
> >> >     }
> >> 
> >> Hmm, that would indeed be a bit more generic, but it doesn't handle the
> >> optional 'ports' subnode and doesn't allow for other child nodes in the
> >> device node.
> > 
> > See above. The no-ports-node version could be the
> > for_each_grandchild_of_node() block, and the yes-ports-node version
> > could be a wrapper around that.
> 
> For the yes-ports-node version I see no problem, but without the ports node,
> for_each_grandchild_of_node would also collect the children of non-port
> child nodes.
> The port and endpoint nodes in this binding are identified by their name,
> so maybe adding of_get_next_child_by_name() /
> for_each_named_child_of_node() could be helpful here.
> 
> >> > Finally, looking at the actual patch, is any of this actually needed.
> >> > All of the users updated by this patch only ever handle a single
> >> > endpoint. Have I read it correctly? Are there any users supporting
> >> > multiple endpoints?
> >> 
> >> Yes, mainline currently only contains simple cases. I have posted i.MX6
> >> 
> >> patches that use this scheme for the output path:
> >>   http://www.spinics.net/lists/arm-kernel/msg310817.html
> >>   http://www.spinics.net/lists/arm-kernel/msg310821.html
> > 
> > Blurg. On a plane right now. Can't go and read those links.
> 
> The patches are merged into the staging tree now at bfe24b9.

-- 
Regards,

Laurent Pinchart

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

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

[Tomi Valkeinen]

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

On 08/03/14 17:54, Laurent Pinchart wrote:

>> Sylwester suggested as an alternative, if I understood correctly, to
>> drop the endpoint node and instead keep the port:
>>
>>     device-a {
>>         implicit_output_ep: port {
>>             remote-endpoint = <&explicit_input_ep>;
>>         };
>>     };
>>
>>     device-b {
>>         port {
>>             explicit_input_ep: endpoint {
>>                 remote-endpoint = <&implicit_output_ep>;
>>             };
>>         };
>>     };
>>
>> This would have the advantage to reduce verbosity for devices with multiple
>> ports that are only connected via one endport each, and you'd always have
>> the connected ports in the device tree as 'port' nodes.
> 
> I like that idea. I would prefer making the 'port' nodes mandatory and the 
> 'ports' and 'endpoint' nodes optional. Leaving the 'port' node out slightly 
> decreases readability in my opinion, but making the 'endpoint' node optional 
> increases it. That's just my point of view though.

I, on the other hand, don't like it =). With that format, the
remote-endpoint doesn't point to an EP, but a port. And you'll have
endpoint's properties in a port node, among the port's properties.

 Tomi



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

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

[Tomi Valkeinen]

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

On 08/03/14 17:54, Laurent Pinchart wrote:

>> Sylwester suggested as an alternative, if I understood correctly, to
>> drop the endpoint node and instead keep the port:
>>
>>     device-a {
>>         implicit_output_ep: port {
>>             remote-endpoint = <&explicit_input_ep>;
>>         };
>>     };
>>
>>     device-b {
>>         port {
>>             explicit_input_ep: endpoint {
>>                 remote-endpoint = <&implicit_output_ep>;
>>             };
>>         };
>>     };
>>
>> This would have the advantage to reduce verbosity for devices with multiple
>> ports that are only connected via one endport each, and you'd always have
>> the connected ports in the device tree as 'port' nodes.
> 
> I like that idea. I would prefer making the 'port' nodes mandatory and the 
> 'ports' and 'endpoint' nodes optional. Leaving the 'port' node out slightly 
> decreases readability in my opinion, but making the 'endpoint' node optional 
> increases it. That's just my point of view though.

I, on the other hand, don't like it =). With that format, the
remote-endpoint doesn't point to an EP, but a port. And you'll have
endpoint's properties in a port node, among the port's properties.

 Tomi



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

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

[Laurent Pinchart]

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

Hi Tomi,

On Monday 10 March 2014 08:00:02 Tomi Valkeinen wrote:
> On 08/03/14 17:54, Laurent Pinchart wrote:
> >> Sylwester suggested as an alternative, if I understood correctly, to
> >> 
> >> drop the endpoint node and instead keep the port:
> >>     device-a {
> >>         implicit_output_ep: port {
> >>             remote-endpoint = <&explicit_input_ep>;
> >>         };
> >>     };
> >>     
> >>     device-b {
> >>         port {
> >>             explicit_input_ep: endpoint {
> >>                 remote-endpoint = <&implicit_output_ep>;
> >>             };
> >>         };
> >>     };
> >> 
> >> This would have the advantage to reduce verbosity for devices with
> >> multiple ports that are only connected via one endport each, and you'd
> >> always have the connected ports in the device tree as 'port' nodes.
> > 
> > I like that idea. I would prefer making the 'port' nodes mandatory and the
> > 'ports' and 'endpoint' nodes optional. Leaving the 'port' node out
> > slightly
> > decreases readability in my opinion, but making the 'endpoint' node
> > optional increases it. That's just my point of view though.
> 
> I, on the other hand, don't like it =). With that format, the
> remote-endpoint doesn't point to an EP, but a port. And you'll have
> endpoint's properties in a port node, among the port's properties.

We'll need to discuss port and endpoint properties separately, but it might 
make sense to allow endpoints to override port properties instead of 
specifying the same value explicitly for each endpoint. Endpoint parsing 
functions would thus look for properties in endpoints first and then in the 
parent port node if the property can't be found. This would work with implicit 
endpoints and would be hidden to the drivers.

(Please note that this is just food for thought)

-- 
Regards,

Laurent Pinchart

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

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

[Laurent Pinchart]

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

Hi Tomi,

On Monday 10 March 2014 08:00:02 Tomi Valkeinen wrote:
> On 08/03/14 17:54, Laurent Pinchart wrote:
> >> Sylwester suggested as an alternative, if I understood correctly, to
> >> 
> >> drop the endpoint node and instead keep the port:
> >>     device-a {
> >>         implicit_output_ep: port {
> >>             remote-endpoint = <&explicit_input_ep>;
> >>         };
> >>     };
> >>     
> >>     device-b {
> >>         port {
> >>             explicit_input_ep: endpoint {
> >>                 remote-endpoint = <&implicit_output_ep>;
> >>             };
> >>         };
> >>     };
> >> 
> >> This would have the advantage to reduce verbosity for devices with
> >> multiple ports that are only connected via one endport each, and you'd
> >> always have the connected ports in the device tree as 'port' nodes.
> > 
> > I like that idea. I would prefer making the 'port' nodes mandatory and the
> > 'ports' and 'endpoint' nodes optional. Leaving the 'port' node out
> > slightly
> > decreases readability in my opinion, but making the 'endpoint' node
> > optional increases it. That's just my point of view though.
> 
> I, on the other hand, don't like it =). With that format, the
> remote-endpoint doesn't point to an EP, but a port. And you'll have
> endpoint's properties in a port node, among the port's properties.

We'll need to discuss port and endpoint properties separately, but it might 
make sense to allow endpoints to override port properties instead of 
specifying the same value explicitly for each endpoint. Endpoint parsing 
functions would thus look for properties in endpoints first and then in the 
parent port node if the property can't be found. This would work with implicit 
endpoints and would be hidden to the drivers.

(Please note that this is just food for thought)

-- 
Regards,

Laurent Pinchart

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

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

[Andrzej Hajda]

Hi,

On 03/08/2014 04:54 PM, Laurent Pinchart wrote:
> Hi Philipp,
> 
> On Saturday 08 March 2014 13:07:23 Philipp Zabel wrote:
>> On Fri, Mar 7, 2014 at 6:18 PM, Grant Likely wrote:
>>> On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel wrote:
>>>> The 'ports' node is optional. It is only needed if the parent node has
>>>> its own #address-cells and #size-cells properties. If the ports are
>>>> direct children of the device node, there might be other nodes than
>>>>
>>>> ports:
>>>>       device {
>>>>               #address-cells = <1>;
>>>>               #size-cells = <0>;
>>>>               
>>>>               port@0 {
>>>>                       endpoint { ... };
>>>>               };
>>>>               port@1 {
>>>>                       endpoint { ... };
>>>>               };
>>>>               
>>>>               some-other-child { ... };
>>>>       };
>>>>       
>>>>       device {
>>>>               #address-cells = <x>;
>>>>               #size-cells = <y>;
>>>>               
>>>>               ports {
>>>>                       #address-cells = <1>;
>>>>                       #size-cells = <0>;
>>>>                       
>>>>                       port@0 {
>>>>                               endpoint { ... };
>>>>                       };
>>>>                       port@1 {
>>>>                               endpoint { ... };
>>>>                       };
>>>>               };
>>>>               
>>>>               some-other-child { ... };
>>>>       };
>>>
>>> From a pattern perspective I have no problem with that.... From an
>>> individual driver binding perspective that is just dumb! It's fine for
>>> the ports node to be optional, but an individual driver using the
>>> binding should be explicit about which it will accept. Please use either
>>> a flag or a separate wrapper so that the driver can select the
>>> behaviour.
>>
>> If the generic binding exists in both forms, most drivers should be
>> able to cope with both. Maybe it should be mentioned in the bindings
>> that the short form without ports node should be used where possible
>> (i.e. for devices that don't already have #address,size-cells != 1,0).
>>
>> Having a separate wrapper to enforce the ports node for devices that
>> need it might be useful.
>>
>>>> The helper should find the two endpoints in both cases.
>>>>
>>>> Tomi suggests an even more compact form for devices with just one port:
>>>>       device {
>>>>               endpoint { ... };
>>>>               
>>>>               some-other-child { ... };
>>>>       };
>>>
>>> That's fine. In that case the driver would specifically require the
>>> endpoint to be that one node.... although the above looks a little weird
>>> to me. I would recommend that if there are other non-port child nodes
>>> then the ports should still be encapsulated by a ports node.  The device
>>> binding should not be ambiguous about which nodes are ports.
>>
>> Sylwester suggested as an alternative, if I understood correctly, to
>> drop the endpoint node and instead keep the port:
>>
>>     device-a {
>>         implicit_output_ep: port {
>>             remote-endpoint = <&explicit_input_ep>;
>>         };
>>     };
>>
>>     device-b {
>>         port {
>>             explicit_input_ep: endpoint {
>>                 remote-endpoint = <&implicit_output_ep>;
>>             };
>>         };
>>     };
>>
>> This would have the advantage to reduce verbosity for devices with multiple
>> ports that are only connected via one endport each, and you'd always have
>> the connected ports in the device tree as 'port' nodes.
> 
> I like that idea. I would prefer making the 'port' nodes mandatory and the 
> 'ports' and 'endpoint' nodes optional. Leaving the 'port' node out slightly 
> decreases readability in my opinion, but making the 'endpoint' node optional 
> increases it. That's just my point of view though.
> 

I want to propose another solution to simplify bindings, in fact I have
few ideas to consider:

1. Use named ports instead of address-cells/regs. Ie instead of
port@number schema, use port-function. This will allow to avoid ports
node and #address-cells, #size-cells, reg properties.
Additionally it should increase readability of the bindings.

device {
	port-dsi {
		endpoint { ... };
	};
	port-rgb {
		endpoint { ... };
	};
};

It is little bit like with gpios vs reset-gpios properties.
Another advantage I see we do not need do mappings of port numbers
to functions between dts, drivers and documentation.

2. Similar approach can be taken to endpoint nodes, in fact
as endpoints are children of port node and as I understand port node
have no other children we can use any name instead of endpoint@number,
of course some convention can be helpful.

device {
	port-dsi {
		ep-soc1 { ... };
		ep-soc2 { ... };
	};
	port-rgb {
		ep-panel { ... };
	};
};

I would like to add that those ideas would work nicely with Sylwester's
proposition of skipping endpoints nodes in case there is only one
endpoint - the most common cases are devices with one or two ports, each
port having only one remote endpoint.
The complete graph for DSI/LVDS bridge I work recently will look like:

dsim {
	dsim_ep: port-dsi {
		remote-endpoint = <&bridge_dsi_ep>;
	};
};

bridge {
	bridge_dsi_ep: port-dsi {
		remote-endpoint = <&dsim_ep>;
	};
	bridge_lvds_ep: port-lvds {
		remote-endpoint = <&panel_ep>;
	};
};

panel {
	port-lvds {
		remote-endpoint <&bridge_lvds_ep>;
	};
};

Regards
Andrzej

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

[Andrzej Hajda]

Hi,

On 03/08/2014 04:54 PM, Laurent Pinchart wrote:
> Hi Philipp,
> 
> On Saturday 08 March 2014 13:07:23 Philipp Zabel wrote:
>> On Fri, Mar 7, 2014 at 6:18 PM, Grant Likely wrote:
>>> On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel wrote:
>>>> The 'ports' node is optional. It is only needed if the parent node has
>>>> its own #address-cells and #size-cells properties. If the ports are
>>>> direct children of the device node, there might be other nodes than
>>>>
>>>> ports:
>>>>       device {
>>>>               #address-cells = <1>;
>>>>               #size-cells = <0>;
>>>>               
>>>>               port@0 {
>>>>                       endpoint { ... };
>>>>               };
>>>>               port@1 {
>>>>                       endpoint { ... };
>>>>               };
>>>>               
>>>>               some-other-child { ... };
>>>>       };
>>>>       
>>>>       device {
>>>>               #address-cells = <x>;
>>>>               #size-cells = <y>;
>>>>               
>>>>               ports {
>>>>                       #address-cells = <1>;
>>>>                       #size-cells = <0>;
>>>>                       
>>>>                       port@0 {
>>>>                               endpoint { ... };
>>>>                       };
>>>>                       port@1 {
>>>>                               endpoint { ... };
>>>>                       };
>>>>               };
>>>>               
>>>>               some-other-child { ... };
>>>>       };
>>>
>>> From a pattern perspective I have no problem with that.... From an
>>> individual driver binding perspective that is just dumb! It's fine for
>>> the ports node to be optional, but an individual driver using the
>>> binding should be explicit about which it will accept. Please use either
>>> a flag or a separate wrapper so that the driver can select the
>>> behaviour.
>>
>> If the generic binding exists in both forms, most drivers should be
>> able to cope with both. Maybe it should be mentioned in the bindings
>> that the short form without ports node should be used where possible
>> (i.e. for devices that don't already have #address,size-cells != 1,0).
>>
>> Having a separate wrapper to enforce the ports node for devices that
>> need it might be useful.
>>
>>>> The helper should find the two endpoints in both cases.
>>>>
>>>> Tomi suggests an even more compact form for devices with just one port:
>>>>       device {
>>>>               endpoint { ... };
>>>>               
>>>>               some-other-child { ... };
>>>>       };
>>>
>>> That's fine. In that case the driver would specifically require the
>>> endpoint to be that one node.... although the above looks a little weird
>>> to me. I would recommend that if there are other non-port child nodes
>>> then the ports should still be encapsulated by a ports node.  The device
>>> binding should not be ambiguous about which nodes are ports.
>>
>> Sylwester suggested as an alternative, if I understood correctly, to
>> drop the endpoint node and instead keep the port:
>>
>>     device-a {
>>         implicit_output_ep: port {
>>             remote-endpoint = <&explicit_input_ep>;
>>         };
>>     };
>>
>>     device-b {
>>         port {
>>             explicit_input_ep: endpoint {
>>                 remote-endpoint = <&implicit_output_ep>;
>>             };
>>         };
>>     };
>>
>> This would have the advantage to reduce verbosity for devices with multiple
>> ports that are only connected via one endport each, and you'd always have
>> the connected ports in the device tree as 'port' nodes.
> 
> I like that idea. I would prefer making the 'port' nodes mandatory and the 
> 'ports' and 'endpoint' nodes optional. Leaving the 'port' node out slightly 
> decreases readability in my opinion, but making the 'endpoint' node optional 
> increases it. That's just my point of view though.
> 

I want to propose another solution to simplify bindings, in fact I have
few ideas to consider:

1. Use named ports instead of address-cells/regs. Ie instead of
port@number schema, use port-function. This will allow to avoid ports
node and #address-cells, #size-cells, reg properties.
Additionally it should increase readability of the bindings.

device {
	port-dsi {
		endpoint { ... };
	};
	port-rgb {
		endpoint { ... };
	};
};

It is little bit like with gpios vs reset-gpios properties.
Another advantage I see we do not need do mappings of port numbers
to functions between dts, drivers and documentation.

2. Similar approach can be taken to endpoint nodes, in fact
as endpoints are children of port node and as I understand port node
have no other children we can use any name instead of endpoint@number,
of course some convention can be helpful.

device {
	port-dsi {
		ep-soc1 { ... };
		ep-soc2 { ... };
	};
	port-rgb {
		ep-panel { ... };
	};
};

I would like to add that those ideas would work nicely with Sylwester's
proposition of skipping endpoints nodes in case there is only one
endpoint - the most common cases are devices with one or two ports, each
port having only one remote endpoint.
The complete graph for DSI/LVDS bridge I work recently will look like:

dsim {
	dsim_ep: port-dsi {
		remote-endpoint = <&bridge_dsi_ep>;
	};
};

bridge {
	bridge_dsi_ep: port-dsi {
		remote-endpoint = <&dsim_ep>;
	};
	bridge_lvds_ep: port-lvds {
		remote-endpoint = <&panel_ep>;
	};
};

panel {
	port-lvds {
		remote-endpoint <&bridge_lvds_ep>;
	};
};

Regards
Andrzej

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

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

[Tomi Valkeinen]

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

On 10/03/14 10:58, Andrzej Hajda wrote:

> I want to propose another solution to simplify bindings, in fact I have
> few ideas to consider:
> 
> 1. Use named ports instead of address-cells/regs. Ie instead of
> port@number schema, use port-function. This will allow to avoid ports
> node and #address-cells, #size-cells, reg properties.
> Additionally it should increase readability of the bindings.
> 
> device {
> 	port-dsi {
> 		endpoint { ... };
> 	};
> 	port-rgb {
> 		endpoint { ... };
> 	};
> };
> 
> It is little bit like with gpios vs reset-gpios properties.
> Another advantage I see we do not need do mappings of port numbers
> to functions between dts, drivers and documentation.

That makes it more difficult to iterate the ports. You need to go
through all the nodes and use partial name matching. I think for things
like gpios, the driver always gives the full name, so there's no need
for any kind of partial matching or searching.

It looks nice when just looking at the DT, though.

 Tomi



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

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

[Tomi Valkeinen]

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

On 10/03/14 10:58, Andrzej Hajda wrote:

> I want to propose another solution to simplify bindings, in fact I have
> few ideas to consider:
> 
> 1. Use named ports instead of address-cells/regs. Ie instead of
> port@number schema, use port-function. This will allow to avoid ports
> node and #address-cells, #size-cells, reg properties.
> Additionally it should increase readability of the bindings.
> 
> device {
> 	port-dsi {
> 		endpoint { ... };
> 	};
> 	port-rgb {
> 		endpoint { ... };
> 	};
> };
> 
> It is little bit like with gpios vs reset-gpios properties.
> Another advantage I see we do not need do mappings of port numbers
> to functions between dts, drivers and documentation.

That makes it more difficult to iterate the ports. You need to go
through all the nodes and use partial name matching. I think for things
like gpios, the driver always gives the full name, so there's no need
for any kind of partial matching or searching.

It looks nice when just looking at the DT, though.

 Tomi



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

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

[Laurent Pinchart]

Hi Andrzej,

On Monday 10 March 2014 09:58:07 Andrzej Hajda wrote:
> On 03/08/2014 04:54 PM, Laurent Pinchart wrote:
> > On Saturday 08 March 2014 13:07:23 Philipp Zabel wrote:
> >> On Fri, Mar 7, 2014 at 6:18 PM, Grant Likely wrote:
> >>> On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel wrote:
> >>>> The 'ports' node is optional. It is only needed if the parent node has
> >>>> its own #address-cells and #size-cells properties. If the ports are
> >>>> direct children of the device node, there might be other nodes than
> >>>> 
> >>>> ports:
> >>>>       device {
> >>>>               #address-cells = <1>;
> >>>>               #size-cells = <0>;
> >>>>               
> >>>>               port@0 {
> >>>>                       endpoint { ... };
> >>>>               };
> >>>>               port@1 {
> >>>>                       endpoint { ... };
> >>>>               };
> >>>>               
> >>>>               some-other-child { ... };
> >>>>       };
> >>>>       
> >>>>       device {
> >>>>               #address-cells = <x>;
> >>>>               #size-cells = <y>;
> >>>>               
> >>>>               ports {
> >>>>                       #address-cells = <1>;
> >>>>                       #size-cells = <0>;
> >>>>                       
> >>>>                       port@0 {
> >>>>                               endpoint { ... };
> >>>>                       };
> >>>>                       port@1 {
> >>>>                               endpoint { ... };
> >>>>                       };
> >>>>               };
> >>>>               
> >>>>               some-other-child { ... };
> >>>>       };
> >>> 
> >>> From a pattern perspective I have no problem with that.... From an
> >>> individual driver binding perspective that is just dumb! It's fine for
> >>> the ports node to be optional, but an individual driver using the
> >>> binding should be explicit about which it will accept. Please use either
> >>> a flag or a separate wrapper so that the driver can select the
> >>> behaviour.
> >> 
> >> If the generic binding exists in both forms, most drivers should be
> >> able to cope with both. Maybe it should be mentioned in the bindings
> >> that the short form without ports node should be used where possible
> >> (i.e. for devices that don't already have #address,size-cells != 1,0).
> >> 
> >> Having a separate wrapper to enforce the ports node for devices that
> >> need it might be useful.
> >> 
> >>>> The helper should find the two endpoints in both cases.
> >>>> 
> >>>> Tomi suggests an even more compact form for devices with just one port:
> >>>>
> >>>>       device {
> >>>>               endpoint { ... };
> >>>>               
> >>>>               some-other-child { ... };
> >>>>       };
> >>> 
> >>> That's fine. In that case the driver would specifically require the
> >>> endpoint to be that one node.... although the above looks a little weird
> >>> to me. I would recommend that if there are other non-port child nodes
> >>> then the ports should still be encapsulated by a ports node.  The device
> >>> binding should not be ambiguous about which nodes are ports.
> >> 
> >> Sylwester suggested as an alternative, if I understood correctly, to
> >> drop the endpoint node and instead keep the port:
> >> 
> >>     device-a {
> >>         implicit_output_ep: port {
> >>             remote-endpoint = <&explicit_input_ep>;
> >>         };
> >>     };
> >>     
> >>     device-b {
> >>         port {
> >>             explicit_input_ep: endpoint {
> >>                 remote-endpoint = <&implicit_output_ep>;
> >>             };
> >>         };
> >>     };
> >> 
> >> This would have the advantage to reduce verbosity for devices with
> >> multiple ports that are only connected via one endport each, and you'd
> >> always have the connected ports in the device tree as 'port' nodes.
> > 
> > I like that idea. I would prefer making the 'port' nodes mandatory and the
> > 'ports' and 'endpoint' nodes optional. Leaving the 'port' node out
> > slightly decreases readability in my opinion, but making the 'endpoint'
> > node optional increases it. That's just my point of view though.
> 
> I want to propose another solution to simplify bindings, in fact I have
> few ideas to consider:
> 
> 1. Use named ports instead of address-cells/regs. Ie instead of
> port@number schema, use port-function. This will allow to avoid ports
> node and #address-cells, #size-cells, reg properties.
> Additionally it should increase readability of the bindings.
> 
> device {
> 	port-dsi {
> 		endpoint { ... };
> 	};
> 	port-rgb {
> 		endpoint { ... };
> 	};
> };
> 
> It is little bit like with gpios vs reset-gpios properties.
> Another advantage I see we do not need do mappings of port numbers
> to functions between dts, drivers and documentation.

The problem with this approach is that ports are identified by a number inside 
the kernel, so we would still need to define name to number mappings, or 
switch to port names internally first.

> 2. Similar approach can be taken to endpoint nodes, in fact
> as endpoints are children of port node and as I understand port node
> have no other children we can use any name instead of endpoint@number,
> of course some convention can be helpful.
> 
> device {
> 	port-dsi {
> 		ep-soc1 { ... };
> 		ep-soc2 { ... };
> 	};
> 	port-rgb {
> 		ep-panel { ... };
> 	};
> };

I see less issues here, as we don't need to number endpoints if I'm not 
mistaken.

> I would like to add that those ideas would work nicely with Sylwester's
> proposition of skipping endpoints nodes in case there is only one
> endpoint - the most common cases are devices with one or two ports, each
> port having only one remote endpoint.
> The complete graph for DSI/LVDS bridge I work recently will look like:
> 
> dsim {
> 	dsim_ep: port-dsi {
> 		remote-endpoint = <&bridge_dsi_ep>;
> 	};
> };
> 
> bridge {
> 	bridge_dsi_ep: port-dsi {
> 		remote-endpoint = <&dsim_ep>;
> 	};
> 	bridge_lvds_ep: port-lvds {
> 		remote-endpoint = <&panel_ep>;
> 	};
> };
> 
> panel {
> 	port-lvds {
> 		remote-endpoint <&bridge_lvds_ep>;
> 	};
> };

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Andrzej,

On Monday 10 March 2014 09:58:07 Andrzej Hajda wrote:
> On 03/08/2014 04:54 PM, Laurent Pinchart wrote:
> > On Saturday 08 March 2014 13:07:23 Philipp Zabel wrote:
> >> On Fri, Mar 7, 2014 at 6:18 PM, Grant Likely wrote:
> >>> On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel wrote:
> >>>> The 'ports' node is optional. It is only needed if the parent node has
> >>>> its own #address-cells and #size-cells properties. If the ports are
> >>>> direct children of the device node, there might be other nodes than
> >>>> 
> >>>> ports:
> >>>>       device {
> >>>>               #address-cells = <1>;
> >>>>               #size-cells = <0>;
> >>>>               
> >>>>               port@0 {
> >>>>                       endpoint { ... };
> >>>>               };
> >>>>               port@1 {
> >>>>                       endpoint { ... };
> >>>>               };
> >>>>               
> >>>>               some-other-child { ... };
> >>>>       };
> >>>>       
> >>>>       device {
> >>>>               #address-cells = <x>;
> >>>>               #size-cells = <y>;
> >>>>               
> >>>>               ports {
> >>>>                       #address-cells = <1>;
> >>>>                       #size-cells = <0>;
> >>>>                       
> >>>>                       port@0 {
> >>>>                               endpoint { ... };
> >>>>                       };
> >>>>                       port@1 {
> >>>>                               endpoint { ... };
> >>>>                       };
> >>>>               };
> >>>>               
> >>>>               some-other-child { ... };
> >>>>       };
> >>> 
> >>> From a pattern perspective I have no problem with that.... From an
> >>> individual driver binding perspective that is just dumb! It's fine for
> >>> the ports node to be optional, but an individual driver using the
> >>> binding should be explicit about which it will accept. Please use either
> >>> a flag or a separate wrapper so that the driver can select the
> >>> behaviour.
> >> 
> >> If the generic binding exists in both forms, most drivers should be
> >> able to cope with both. Maybe it should be mentioned in the bindings
> >> that the short form without ports node should be used where possible
> >> (i.e. for devices that don't already have #address,size-cells != 1,0).
> >> 
> >> Having a separate wrapper to enforce the ports node for devices that
> >> need it might be useful.
> >> 
> >>>> The helper should find the two endpoints in both cases.
> >>>> 
> >>>> Tomi suggests an even more compact form for devices with just one port:
> >>>>
> >>>>       device {
> >>>>               endpoint { ... };
> >>>>               
> >>>>               some-other-child { ... };
> >>>>       };
> >>> 
> >>> That's fine. In that case the driver would specifically require the
> >>> endpoint to be that one node.... although the above looks a little weird
> >>> to me. I would recommend that if there are other non-port child nodes
> >>> then the ports should still be encapsulated by a ports node.  The device
> >>> binding should not be ambiguous about which nodes are ports.
> >> 
> >> Sylwester suggested as an alternative, if I understood correctly, to
> >> drop the endpoint node and instead keep the port:
> >> 
> >>     device-a {
> >>         implicit_output_ep: port {
> >>             remote-endpoint = <&explicit_input_ep>;
> >>         };
> >>     };
> >>     
> >>     device-b {
> >>         port {
> >>             explicit_input_ep: endpoint {
> >>                 remote-endpoint = <&implicit_output_ep>;
> >>             };
> >>         };
> >>     };
> >> 
> >> This would have the advantage to reduce verbosity for devices with
> >> multiple ports that are only connected via one endport each, and you'd
> >> always have the connected ports in the device tree as 'port' nodes.
> > 
> > I like that idea. I would prefer making the 'port' nodes mandatory and the
> > 'ports' and 'endpoint' nodes optional. Leaving the 'port' node out
> > slightly decreases readability in my opinion, but making the 'endpoint'
> > node optional increases it. That's just my point of view though.
> 
> I want to propose another solution to simplify bindings, in fact I have
> few ideas to consider:
> 
> 1. Use named ports instead of address-cells/regs. Ie instead of
> port@number schema, use port-function. This will allow to avoid ports
> node and #address-cells, #size-cells, reg properties.
> Additionally it should increase readability of the bindings.
> 
> device {
> 	port-dsi {
> 		endpoint { ... };
> 	};
> 	port-rgb {
> 		endpoint { ... };
> 	};
> };
> 
> It is little bit like with gpios vs reset-gpios properties.
> Another advantage I see we do not need do mappings of port numbers
> to functions between dts, drivers and documentation.

The problem with this approach is that ports are identified by a number inside 
the kernel, so we would still need to define name to number mappings, or 
switch to port names internally first.

> 2. Similar approach can be taken to endpoint nodes, in fact
> as endpoints are children of port node and as I understand port node
> have no other children we can use any name instead of endpoint@number,
> of course some convention can be helpful.
> 
> device {
> 	port-dsi {
> 		ep-soc1 { ... };
> 		ep-soc2 { ... };
> 	};
> 	port-rgb {
> 		ep-panel { ... };
> 	};
> };

I see less issues here, as we don't need to number endpoints if I'm not 
mistaken.

> I would like to add that those ideas would work nicely with Sylwester's
> proposition of skipping endpoints nodes in case there is only one
> endpoint - the most common cases are devices with one or two ports, each
> port having only one remote endpoint.
> The complete graph for DSI/LVDS bridge I work recently will look like:
> 
> dsim {
> 	dsim_ep: port-dsi {
> 		remote-endpoint = <&bridge_dsi_ep>;
> 	};
> };
> 
> bridge {
> 	bridge_dsi_ep: port-dsi {
> 		remote-endpoint = <&dsim_ep>;
> 	};
> 	bridge_lvds_ep: port-lvds {
> 		remote-endpoint = <&panel_ep>;
> 	};
> };
> 
> panel {
> 	port-lvds {
> 		remote-endpoint <&bridge_lvds_ep>;
> 	};
> };

-- 
Regards,

Laurent Pinchart

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

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

[Andrzej Hajda]

On 03/10/2014 12:42 PM, Laurent Pinchart wrote:
> Hi Andrzej,
> 
>>> I like that idea. I would prefer making the 'port' nodes mandatory and the
>>> 'ports' and 'endpoint' nodes optional. Leaving the 'port' node out
>>> slightly decreases readability in my opinion, but making the 'endpoint'
>>> node optional increases it. That's just my point of view though.
>>
>> I want to propose another solution to simplify bindings, in fact I have
>> few ideas to consider:
>>
>> 1. Use named ports instead of address-cells/regs. Ie instead of
>> port@number schema, use port-function. This will allow to avoid ports
>> node and #address-cells, #size-cells, reg properties.
>> Additionally it should increase readability of the bindings.
>>
>> device {
>> 	port-dsi {
>> 		endpoint { ... };
>> 	};
>> 	port-rgb {
>> 		endpoint { ... };
>> 	};
>> };
>>
>> It is little bit like with gpios vs reset-gpios properties.
>> Another advantage I see we do not need do mappings of port numbers
>> to functions between dts, drivers and documentation.
> 
> The problem with this approach is that ports are identified by a number inside 
> the kernel, so we would still need to define name to number mappings, or 
> switch to port names internally first.

The mapping will be only internal in the driver.

Anyway the bindings should be kernel agnostic.

Andrzej

> 
>> 2. Similar approach can be taken to endpoint nodes, in fact
>> as endpoints are children of port node and as I understand port node
>> have no other children we can use any name instead of endpoint@number,
>> of course some convention can be helpful.
>>
>> device {
>> 	port-dsi {
>> 		ep-soc1 { ... };
>> 		ep-soc2 { ... };
>> 	};
>> 	port-rgb {
>> 		ep-panel { ... };
>> 	};
>> };
> 
> I see less issues here, as we don't need to number endpoints if I'm not 
> mistaken.
> 
>> I would like to add that those ideas would work nicely with Sylwester's
>> proposition of skipping endpoints nodes in case there is only one
>> endpoint - the most common cases are devices with one or two ports, each
>> port having only one remote endpoint.
>> The complete graph for DSI/LVDS bridge I work recently will look like:
>>
>> dsim {
>> 	dsim_ep: port-dsi {
>> 		remote-endpoint = <&bridge_dsi_ep>;
>> 	};
>> };
>>
>> bridge {
>> 	bridge_dsi_ep: port-dsi {
>> 		remote-endpoint = <&dsim_ep>;
>> 	};
>> 	bridge_lvds_ep: port-lvds {
>> 		remote-endpoint = <&panel_ep>;
>> 	};
>> };
>>
>> panel {
>> 	port-lvds {
>> 		remote-endpoint <&bridge_lvds_ep>;
>> 	};
>> };
> 

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

[Andrzej Hajda]

On 03/10/2014 12:42 PM, Laurent Pinchart wrote:
> Hi Andrzej,
> 
>>> I like that idea. I would prefer making the 'port' nodes mandatory and the
>>> 'ports' and 'endpoint' nodes optional. Leaving the 'port' node out
>>> slightly decreases readability in my opinion, but making the 'endpoint'
>>> node optional increases it. That's just my point of view though.
>>
>> I want to propose another solution to simplify bindings, in fact I have
>> few ideas to consider:
>>
>> 1. Use named ports instead of address-cells/regs. Ie instead of
>> port@number schema, use port-function. This will allow to avoid ports
>> node and #address-cells, #size-cells, reg properties.
>> Additionally it should increase readability of the bindings.
>>
>> device {
>> 	port-dsi {
>> 		endpoint { ... };
>> 	};
>> 	port-rgb {
>> 		endpoint { ... };
>> 	};
>> };
>>
>> It is little bit like with gpios vs reset-gpios properties.
>> Another advantage I see we do not need do mappings of port numbers
>> to functions between dts, drivers and documentation.
> 
> The problem with this approach is that ports are identified by a number inside 
> the kernel, so we would still need to define name to number mappings, or 
> switch to port names internally first.

The mapping will be only internal in the driver.

Anyway the bindings should be kernel agnostic.

Andrzej

> 
>> 2. Similar approach can be taken to endpoint nodes, in fact
>> as endpoints are children of port node and as I understand port node
>> have no other children we can use any name instead of endpoint@number,
>> of course some convention can be helpful.
>>
>> device {
>> 	port-dsi {
>> 		ep-soc1 { ... };
>> 		ep-soc2 { ... };
>> 	};
>> 	port-rgb {
>> 		ep-panel { ... };
>> 	};
>> };
> 
> I see less issues here, as we don't need to number endpoints if I'm not 
> mistaken.
> 
>> I would like to add that those ideas would work nicely with Sylwester's
>> proposition of skipping endpoints nodes in case there is only one
>> endpoint - the most common cases are devices with one or two ports, each
>> port having only one remote endpoint.
>> The complete graph for DSI/LVDS bridge I work recently will look like:
>>
>> dsim {
>> 	dsim_ep: port-dsi {
>> 		remote-endpoint = <&bridge_dsi_ep>;
>> 	};
>> };
>>
>> bridge {
>> 	bridge_dsi_ep: port-dsi {
>> 		remote-endpoint = <&dsim_ep>;
>> 	};
>> 	bridge_lvds_ep: port-lvds {
>> 		remote-endpoint = <&panel_ep>;
>> 	};
>> };
>>
>> panel {
>> 	port-lvds {
>> 		remote-endpoint <&bridge_lvds_ep>;
>> 	};
>> };
> 

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

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

[Grant Likely]

On Sat, 8 Mar 2014 13:07:23 +0100, Philipp Zabel <philipp.zabel@gmail.com> wrote:
> Hi Grant,
> 
> On Fri, Mar 7, 2014 at 6:18 PM, Grant Likely <grant.likely@linaro.org> wrote:
> > On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> >> The 'ports' node is optional. It is only needed if the parent node has
> >> its own #address-cells and #size-cells properties. If the ports are
> >> direct children of the device node, there might be other nodes than
> >> ports:
> >>
> >>       device {
> >>               #address-cells = <1>;
> >>               #size-cells = <0>;
> >>
> >>               port@0 {
> >>                       endpoint { ... };
> >>               };
> >>               port@1 {
> >>                       endpoint { ... };
> >>               };
> >>
> >>               some-other-child { ... };
> >>       };
> >>
> >>       device {
> >>               #address-cells = <x>;
> >>               #size-cells = <y>;
> >>
> >>               ports {
> >>                       #address-cells = <1>;
> >>                       #size-cells = <0>;
> >>
> >>                       port@0 {
> >>                               endpoint { ... };
> >>                       };
> >>                       port@1 {
> >>                               endpoint { ... };
> >>                       };
> >>               };
> >>
> >>               some-other-child { ... };
> >>       };
> >
> > From a pattern perspective I have no problem with that.... From an
> > individual driver binding perspective that is just dumb! It's fine for
> > the ports node to be optional, but an individual driver using the
> > binding should be explicit about which it will accept. Please use either
> > a flag or a separate wrapper so that the driver can select the
> > behaviour.
> 
> If the generic binding exists in both forms, most drivers should be
> able to cope with both. Maybe it should be mentioned in the bindings
> that the short form without ports node should be used where possible
> (i.e. for devices that don't already have #address,size-cells != 1,0).

I would rephrase that: (ie. for devices that have other child nodes that
aren't ports.) It isn't about the #address/size-cells values. It is
about how the driver interprets child nodes.

> 
> Having a separate wrapper to enforce the ports node for devices that
> need it might be useful.

Or the other way around. Make the core function only handle an explicit
location and use a v4l2 wrapper to preserve the current behaviour. That
will encourage stricter usage.

> >> The helper should find the two endpoints in both cases.
> >> Tomi suggests an even more compact form for devices with just one port:
> >>
> >>       device {
> >>               endpoint { ... };
> >>
> >>               some-other-child { ... };
> >>       };
> >
> > That's fine. In that case the driver would specifically require the
> > endpoint to be that one node.... although the above looks a little weird
> > to me. I would recommend that if there are other non-port child nodes
> > then the ports should still be encapsulated by a ports node.  The device
> > binding should not be ambiguous about which nodes are ports.
> 
> Sylwester suggested as an alternative, if I understood correctly, to
> drop the endpoint node and instead keep the port:
> 
>     device-a {
>         implicit_output_ep: port {
>             remote-endpoint = <&explicit_input_ep>;
>         };
>     };
> 
>     device-b {
>         port {
>             explicit_input_ep: endpoint {
>                 remote-endpoint = <&implicit_output_ep>;
>             };
>         };
>     };
> 
> This would have the advantage to reduce verbosity for devices with multiple
> ports that are only connected via one endport each, and you'd always have
> the connected ports in the device tree as 'port' nodes.

It sounds like that is a closer description of the hardware, so I agree.

> 
> >> > It seems that this function is merely a helper to get all grandchildren
> >> > of a node (with some very minor constraints). That could be generalized
> >> > and simplified. If the function takes the "ports" node as an argument
> >> > instead of the parent, then there is a greater likelyhood that other
> >> > code can make use of it...
> >> >
> >> > Thinking further. I think the semantics of this whole feature basically
> >> > boil down to this:
> >> >
> >> > #define for_each_grandchild_of_node(parent, child, grandchild) \
> >> >     for_each_child_of_node(parent, child) \
> >> >             for_each_child_of_node(child, grandchild)
> >> >
> >> > Correct? Or in this specific case:
> >> >
> >> >     parent = of_get_child_by_name(np, "ports")
> >> >     for_each_grandchild_of_node(parent, child, grandchild) {
> >> >             ...
> >> >     }
> >>
> >> Hmm, that would indeed be a bit more generic, but it doesn't handle the
> >> optional 'ports' subnode and doesn't allow for other child nodes in the
> >> device node.
> >
> > See above. The no-ports-node version could be the
> > for_each_grandchild_of_node() block, and the yes-ports-node version
> > could be a wrapper around that.
> 
> For the yes-ports-node version I see no problem, but without the ports node,
> for_each_grandchild_of_node would also collect the children of non-port
> child nodes.
> The port and endpoint nodes in this binding are identified by their name,
> so maybe adding of_get_next_child_by_name() /
> for_each_named_child_of_node() could be helpful here.

Generally I would avoid mixing child nodes of different purposes. If you
are in that situation, the recommendation should be to use a ports node.
If there are any current users for which that doesn't work, only then
would I do the child_by_name approach.

g.

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

[Grant Likely]

On Sat, 8 Mar 2014 13:07:23 +0100, Philipp Zabel <philipp.zabel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Grant,
> 
> On Fri, Mar 7, 2014 at 6:18 PM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> >> The 'ports' node is optional. It is only needed if the parent node has
> >> its own #address-cells and #size-cells properties. If the ports are
> >> direct children of the device node, there might be other nodes than
> >> ports:
> >>
> >>       device {
> >>               #address-cells = <1>;
> >>               #size-cells = <0>;
> >>
> >>               port@0 {
> >>                       endpoint { ... };
> >>               };
> >>               port@1 {
> >>                       endpoint { ... };
> >>               };
> >>
> >>               some-other-child { ... };
> >>       };
> >>
> >>       device {
> >>               #address-cells = <x>;
> >>               #size-cells = <y>;
> >>
> >>               ports {
> >>                       #address-cells = <1>;
> >>                       #size-cells = <0>;
> >>
> >>                       port@0 {
> >>                               endpoint { ... };
> >>                       };
> >>                       port@1 {
> >>                               endpoint { ... };
> >>                       };
> >>               };
> >>
> >>               some-other-child { ... };
> >>       };
> >
> > From a pattern perspective I have no problem with that.... From an
> > individual driver binding perspective that is just dumb! It's fine for
> > the ports node to be optional, but an individual driver using the
> > binding should be explicit about which it will accept. Please use either
> > a flag or a separate wrapper so that the driver can select the
> > behaviour.
> 
> If the generic binding exists in both forms, most drivers should be
> able to cope with both. Maybe it should be mentioned in the bindings
> that the short form without ports node should be used where possible
> (i.e. for devices that don't already have #address,size-cells != 1,0).

I would rephrase that: (ie. for devices that have other child nodes that
aren't ports.) It isn't about the #address/size-cells values. It is
about how the driver interprets child nodes.

> 
> Having a separate wrapper to enforce the ports node for devices that
> need it might be useful.

Or the other way around. Make the core function only handle an explicit
location and use a v4l2 wrapper to preserve the current behaviour. That
will encourage stricter usage.

> >> The helper should find the two endpoints in both cases.
> >> Tomi suggests an even more compact form for devices with just one port:
> >>
> >>       device {
> >>               endpoint { ... };
> >>
> >>               some-other-child { ... };
> >>       };
> >
> > That's fine. In that case the driver would specifically require the
> > endpoint to be that one node.... although the above looks a little weird
> > to me. I would recommend that if there are other non-port child nodes
> > then the ports should still be encapsulated by a ports node.  The device
> > binding should not be ambiguous about which nodes are ports.
> 
> Sylwester suggested as an alternative, if I understood correctly, to
> drop the endpoint node and instead keep the port:
> 
>     device-a {
>         implicit_output_ep: port {
>             remote-endpoint = <&explicit_input_ep>;
>         };
>     };
> 
>     device-b {
>         port {
>             explicit_input_ep: endpoint {
>                 remote-endpoint = <&implicit_output_ep>;
>             };
>         };
>     };
> 
> This would have the advantage to reduce verbosity for devices with multiple
> ports that are only connected via one endport each, and you'd always have
> the connected ports in the device tree as 'port' nodes.

It sounds like that is a closer description of the hardware, so I agree.

> 
> >> > It seems that this function is merely a helper to get all grandchildren
> >> > of a node (with some very minor constraints). That could be generalized
> >> > and simplified. If the function takes the "ports" node as an argument
> >> > instead of the parent, then there is a greater likelyhood that other
> >> > code can make use of it...
> >> >
> >> > Thinking further. I think the semantics of this whole feature basically
> >> > boil down to this:
> >> >
> >> > #define for_each_grandchild_of_node(parent, child, grandchild) \
> >> >     for_each_child_of_node(parent, child) \
> >> >             for_each_child_of_node(child, grandchild)
> >> >
> >> > Correct? Or in this specific case:
> >> >
> >> >     parent = of_get_child_by_name(np, "ports")
> >> >     for_each_grandchild_of_node(parent, child, grandchild) {
> >> >             ...
> >> >     }
> >>
> >> Hmm, that would indeed be a bit more generic, but it doesn't handle the
> >> optional 'ports' subnode and doesn't allow for other child nodes in the
> >> device node.
> >
> > See above. The no-ports-node version could be the
> > for_each_grandchild_of_node() block, and the yes-ports-node version
> > could be a wrapper around that.
> 
> For the yes-ports-node version I see no problem, but without the ports node,
> for_each_grandchild_of_node would also collect the children of non-port
> child nodes.
> The port and endpoint nodes in this binding are identified by their name,
> so maybe adding of_get_next_child_by_name() /
> for_each_named_child_of_node() could be helpful here.

Generally I would avoid mixing child nodes of different purposes. If you
are in that situation, the recommendation should be to use a ports node.
If there are any current users for which that doesn't work, only then
would I do the child_by_name approach.

g.

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

[PATCH v4 2/3] [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
specific, or in fact not even media specific: 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>
---
 drivers/media/v4l2-core/v4l2-of.c | 16 +++-------------
 drivers/of/of_graph.c             | 32 ++++++++++++++++++++++++++++++++
 include/linux/of_graph.h          | 20 ++++++++++++++++++++
 include/media/v4l2-of.h           |  9 +++------
 4 files changed, 58 insertions(+), 19 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
index f919db3..222ed58 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->ep);
+	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/of_graph.c b/drivers/of/of_graph.c
index 267d8f7..d0f1683 100644
--- a/drivers/of/of_graph.c
+++ b/drivers/of/of_graph.c
@@ -15,6 +15,38 @@
 #include <linux/of.h>
 #include <linux/of_graph.h>
 #include <linux/types.h>
+#include <media/of_graph.h>
+
+/**
+ * of_graph_parse_endpoint() - parse common endpoint node properties
+ * @node: pointer to endpoint device_node
+ * @endpoint: pointer to the OF endpoint data structure
+ *
+ * All properties are optional. If none are found, we don't set any flags.
+ * This means the port has a static configuration and no properties have
+ * to be specified explicitly.
+ * The caller should hold a reference to @node.
+ */
+int of_graph_parse_endpoint(const struct device_node *node,
+			    struct of_endpoint *endpoint)
+{
+	struct device_node *port_node = of_get_parent(node);
+
+	memset(endpoint, 0, sizeof(*endpoint));
+
+	endpoint->local_node = node;
+	/*
+	 * It doesn't matter whether the two calls below succeed.
+	 * If they don't then the default value 0 is used.
+	 */
+	of_property_read_u32(port_node, "reg", &endpoint->port);
+	of_property_read_u32(node, "reg", &endpoint->id);
+
+	of_node_put(port_node);
+
+	return 0;
+}
+EXPORT_SYMBOL(of_graph_parse_endpoint);
 
 /**
  * of_graph_get_next_endpoint() - get next endpoint node
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..2229a0e 100644
--- a/include/media/v4l2-of.h
+++ b/include/media/v4l2-of.h
@@ -19,6 +19,7 @@
 #include <linux/errno.h>
 #include <linux/of_graph.h>
 
+#include <media/of_graph.h>
 #include <media/v4l2-mediabus.h>
 
 struct device_node;
@@ -51,17 +52,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
+ * @ep: 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 ep;
 	enum v4l2_mbus_type bus_type;
 	union {
 		struct v4l2_of_bus_parallel parallel;
-- 
1.8.5.3

[PATCH v4 3/3] Documentation: of: Document graph bindings

[Philipp Zabel]

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

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 Documentation/devicetree/bindings/graph.txt | 98 +++++++++++++++++++++++++++++
 1 file changed, 98 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..97c877e
--- /dev/null
+++ b/Documentation/devicetree/bindings/graph.txt
@@ -0,0 +1,98 @@
+Common bindings for device graphs
+
+General concept
+---------------
+
+The hierarchical organisation of the device tree is well suited to describe
+control flow to devices, but data flow between devices that work together to
+form a logical compound device can follow arbitrarily complex graphs.
+The device tree graph bindings allow to describe data bus connections between
+individual devices, that can not be inferred from device tree parent-child
+relationships. The common bindings do not contain any information about the
+direction or type of data flow, they just map connections. Specific properties
+of the connections are described by specialized bindings depending on the type
+of connection. To see how this binding applies to video pipelines, see for
+example Documentation/device-tree/bindings/media/video-interfaces.txt.
+
+Devices can have multiple data interfaces, each of which can be connected to
+the data interfaces of one or more remote devices via a data bus.
+Data interfaces are described by the device nodes' child 'port' nodes. A port
+node contains an 'endpoint' subnode for each remote device port connected to
+this port via a bus. If a port is connected to more than one remote device on
+the same bus, an 'endpoint' child node must be provided for each of them. If
+more than one port is present in a device node or there is more than one
+endpoint at a port, or port node needs to be associated with a selected
+hardware interface, a common scheme using '#address-cells', '#size-cells'
+and 'reg' properties is used.
+
+device {
+        ...
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        port@0 {
+                ...
+                endpoint@0 { ... };
+                endpoint@1 { ... };
+        };
+
+        port@1 { ... };
+};
+
+All 'port' nodes can be grouped under optional 'ports' node, which allows to
+specify #address-cells, #size-cells properties independently for the 'port'
+and 'endpoint' nodes and any child device nodes a device might have.
+
+device {
+        ...
+        ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                        ...
+                        endpoint@0 { ... };
+                        endpoint@1 { ... };
+                };
+
+                port@1 { ... };
+        };
+};
+
+Each endpoint can contain a 'remote-endpoint' phandle property that points to
+the corresponding endpoint in the port of the remote device. Two 'endpoint'
+nodes are linked with each other through their 'remote-endpoint' phandles.
+
+device_1 {
+        port {
+                device_1_output: endpoint {
+                        remote-endpoint = <&device_2_input>;
+                };
+        };
+};
+
+device_1 {
+        port {
+                device_2_input: endpoint {
+                        remote-endpoint = <&device_1_output>;
+                };
+        };
+};
+
+
+Required properties
+-------------------
+
+If there is more than one 'port' or more than one 'endpoint' node or 'reg'
+property is present in port and/or endpoint nodes the following properties
+are required in a relevant parent node:
+
+ - #address-cells : number of cells required to define port/endpoint
+                    identifier, should be 1.
+ - #size-cells    : should be zero.
+
+Optional endpoint properties
+----------------------------
+
+- remote-endpoint: phandle to an 'endpoint' subnode of a remote device node.
+
-- 
1.8.5.3

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Tomi Valkeinen]

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

On 25/02/14 16:58, Philipp Zabel wrote:

> +Optional endpoint properties
> +----------------------------
> +
> +- remote-endpoint: phandle to an 'endpoint' subnode of a remote device node.

Why is that optional? What use is an endpoint, if it's not connected to
something?

Also, if this is being worked on, I'd like to propose the addition of
simpler single-endpoint cases which I've been using with OMAP DSS. So if
there's just a single endpoint for the device, which is very common, you
can have just:

device {
	...
	endpoint { ... };
};

However, I guess that the patch just keeps growing and growing, so maybe
it's better to add such things later =).

 Tomi



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

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Tomi Valkeinen]

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

On 25/02/14 16:58, Philipp Zabel wrote:

> +Optional endpoint properties
> +----------------------------
> +
> +- remote-endpoint: phandle to an 'endpoint' subnode of a remote device node.

Why is that optional? What use is an endpoint, if it's not connected to
something?

Also, if this is being worked on, I'd like to propose the addition of
simpler single-endpoint cases which I've been using with OMAP DSS. So if
there's just a single endpoint for the device, which is very common, you
can have just:

device {
	...
	endpoint { ... };
};

However, I guess that the patch just keeps growing and growing, so maybe
it's better to add such things later =).

 Tomi



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

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Philipp Zabel]

Hi Tomi,

Am Mittwoch, den 26.02.2014, 15:14 +0200 schrieb Tomi Valkeinen:
> On 25/02/14 16:58, Philipp Zabel wrote:
> 
> > +Optional endpoint properties
> > +----------------------------
> > +
> > +- remote-endpoint: phandle to an 'endpoint' subnode of a remote device node.
> 
> Why is that optional? What use is an endpoint, if it's not connected to
> something?

This allows to include the an empty endpoint template in a SoC dtsi for
the convenience of board dts writers. Also, the same property is
currently listed as optional in video-interfaces.txt.

  soc.dtsi:
	display-controller {
		port {
			disp0: endpoint { };
		};
	};

  board.dts:
	#include "soc.dtsi"
	&disp0 {
		remote-endpoint = <&panel_input>;
	};
	panel {
		port {
			panel_in: endpoint {
				remote-endpoint = <&disp0>;
			};
		};
	};

Any board not using that port can just leave the endpoint disconnected.

On the other hand, the same could be achieved with Heiko Stübner's
conditional nodes dtc patch:

  soc.dtsi:
	display-controller {
		port {
			/delete-unreferenced/ disp0: endpoint { };
		};
	};

> Also, if this is being worked on, I'd like to propose the addition of
> simpler single-endpoint cases which I've been using with OMAP DSS. So if
> there's just a single endpoint for the device, which is very common, you
> can have just:
> 
> device {
> 	...
> 	endpoint { ... };
> };
> 
> However, I guess that the patch just keeps growing and growing, so maybe
> it's better to add such things later =).

Yes, that looks good. I'd be happy if we could add this in a second step
as a backwards compatible simplification.

regards
Philipp

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Tomi Valkeinen]

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

On 26/02/14 16:57, Philipp Zabel wrote:
> Hi Tomi,
> 
> Am Mittwoch, den 26.02.2014, 15:14 +0200 schrieb Tomi Valkeinen:
>> On 25/02/14 16:58, Philipp Zabel wrote:
>>
>>> +Optional endpoint properties
>>> +----------------------------
>>> +
>>> +- remote-endpoint: phandle to an 'endpoint' subnode of a remote device node.
>>
>> Why is that optional? What use is an endpoint, if it's not connected to
>> something?
> 
> This allows to include the an empty endpoint template in a SoC dtsi for
> the convenience of board dts writers. Also, the same property is
> currently listed as optional in video-interfaces.txt.
> 
>   soc.dtsi:
> 	display-controller {
> 		port {
> 			disp0: endpoint { };
> 		};
> 	};
> 
>   board.dts:
> 	#include "soc.dtsi"
> 	&disp0 {
> 		remote-endpoint = <&panel_input>;
> 	};
> 	panel {
> 		port {
> 			panel_in: endpoint {
> 				remote-endpoint = <&disp0>;
> 			};
> 		};
> 	};
> 
> Any board not using that port can just leave the endpoint disconnected.

Hmm I see. I'm against that.

I think the SoC dtsi should not contain endpoint node, or even port node
(at least usually). It doesn't know how many endpoints, if any, a
particular board has. That part should be up to the board dts.

I've done this with OMAP as (much simplified):

SoC.dtsi:

dss: dss@58000000 {
	status = "disabled";
};

Nothing else (relevant here). The binding documentation states that dss
has one port, and information what data is needed for the port and endpoint.

board.dts:

&dss {
        status = "ok";

        pinctrl-names = "default";
        pinctrl-0 = <&dss_dpi_pins>;

        dpi_out: endpoint {

                remote-endpoint = <&tfp410_in>;
                data-lines = <24>;
        };
};

That's using the shortened version without port node.

Of course, it's up to the developer how his dts looks like. But to me it
makes sense to require the remote-endpoint property, as the endpoint, or
even the port, doesn't make much sense if there's nothing to connect to.

 Tomi



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

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Tomi Valkeinen]

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

On 26/02/14 16:57, Philipp Zabel wrote:
> Hi Tomi,
> 
> Am Mittwoch, den 26.02.2014, 15:14 +0200 schrieb Tomi Valkeinen:
>> On 25/02/14 16:58, Philipp Zabel wrote:
>>
>>> +Optional endpoint properties
>>> +----------------------------
>>> +
>>> +- remote-endpoint: phandle to an 'endpoint' subnode of a remote device node.
>>
>> Why is that optional? What use is an endpoint, if it's not connected to
>> something?
> 
> This allows to include the an empty endpoint template in a SoC dtsi for
> the convenience of board dts writers. Also, the same property is
> currently listed as optional in video-interfaces.txt.
> 
>   soc.dtsi:
> 	display-controller {
> 		port {
> 			disp0: endpoint { };
> 		};
> 	};
> 
>   board.dts:
> 	#include "soc.dtsi"
> 	&disp0 {
> 		remote-endpoint = <&panel_input>;
> 	};
> 	panel {
> 		port {
> 			panel_in: endpoint {
> 				remote-endpoint = <&disp0>;
> 			};
> 		};
> 	};
> 
> Any board not using that port can just leave the endpoint disconnected.

Hmm I see. I'm against that.

I think the SoC dtsi should not contain endpoint node, or even port node
(at least usually). It doesn't know how many endpoints, if any, a
particular board has. That part should be up to the board dts.

I've done this with OMAP as (much simplified):

SoC.dtsi:

dss: dss@58000000 {
	status = "disabled";
};

Nothing else (relevant here). The binding documentation states that dss
has one port, and information what data is needed for the port and endpoint.

board.dts:

&dss {
        status = "ok";

        pinctrl-names = "default";
        pinctrl-0 = <&dss_dpi_pins>;

        dpi_out: endpoint {

                remote-endpoint = <&tfp410_in>;
                data-lines = <24>;
        };
};

That's using the shortened version without port node.

Of course, it's up to the developer how his dts looks like. But to me it
makes sense to require the remote-endpoint property, as the endpoint, or
even the port, doesn't make much sense if there's nothing to connect to.

 Tomi



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

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Philipp Zabel]

Am Mittwoch, den 26.02.2014, 16:50 +0200 schrieb Tomi Valkeinen:
> On 26/02/14 16:57, Philipp Zabel wrote:
> > Hi Tomi,
> > 
> > Am Mittwoch, den 26.02.2014, 15:14 +0200 schrieb Tomi Valkeinen:
> >> On 25/02/14 16:58, Philipp Zabel wrote:
> >>
> >>> +Optional endpoint properties
> >>> +----------------------------
> >>> +
> >>> +- remote-endpoint: phandle to an 'endpoint' subnode of a remote device node.
> >>
> >> Why is that optional? What use is an endpoint, if it's not connected to
> >> something?
> > 
> > This allows to include the an empty endpoint template in a SoC dtsi for
> > the convenience of board dts writers. Also, the same property is
> > currently listed as optional in video-interfaces.txt.
> > 
> >   soc.dtsi:
> > 	display-controller {
> > 		port {
> > 			disp0: endpoint { };
> > 		};
> > 	};
> > 
> >   board.dts:
> > 	#include "soc.dtsi"
> > 	&disp0 {
> > 		remote-endpoint = <&panel_input>;
> > 	};
> > 	panel {
> > 		port {
> > 			panel_in: endpoint {
> > 				remote-endpoint = <&disp0>;
> > 			};
> > 		};
> > 	};
> > 
> > Any board not using that port can just leave the endpoint disconnected.
> 
> Hmm I see. I'm against that.
> 
> I think the SoC dtsi should not contain endpoint node, or even port node
> (at least usually).

Well, at least the port is a physical thing. I see no reason not to have
it in the dtsi.

> It doesn't know how many endpoints, if any, a
> particular board has. That part should be up to the board dts.

...

> I've done this with OMAP as (much simplified):
> 
> SoC.dtsi:
> 
> dss: dss@58000000 {
> 	status = "disabled";
> };
> 
> Nothing else (relevant here). The binding documentation states that dss
> has one port, and information what data is needed for the port and endpoint.
> 
> board.dts:
> 
> &dss {
>         status = "ok";
> 
>         pinctrl-names = "default";
>         pinctrl-0 = <&dss_dpi_pins>;
> 
>         dpi_out: endpoint {
> 
>                 remote-endpoint = <&tfp410_in>;
>                 data-lines = <24>;
>         };
> };
> 
> That's using the shortened version without port node.

Ok, that looks compact enough. I still don't see the need to change make
the remote-endpoint property required to achieve this, though. On the
other hand, I wouldn't object to making it mandatory either.

> Of course, it's up to the developer how his dts looks like. But to me it
> makes sense to require the remote-endpoint property, as the endpoint, or
> even the port, doesn't make much sense if there's nothing to connect to.

Please let's not make it mandatory for a port node to contain an
endpoint. For any device with multiple ports we can't use the simplified
form above, and only adding the (correctly numbered) port in all the
board device trees would be a pain.

regards
Philipp

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Tomi Valkeinen]

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

On 26/02/14 17:47, Philipp Zabel wrote:

> Ok, that looks compact enough. I still don't see the need to change make
> the remote-endpoint property required to achieve this, though. On the
> other hand, I wouldn't object to making it mandatory either.

Sure, having remote-endpoint as required doesn't achieve anything
particular as such. I just feel it's cleaner. If you have an endpoint,
it must point to somewhere. Maybe it makes the code a tiny bit simpler.

If we do already have users for this that do not have the
remote-endpoint, then we're stuck with having it as optional. If we
don't, I'd rather have it as mandatory.

In any case, it's not a very important thing either way.

>> Of course, it's up to the developer how his dts looks like. But to me it
>> makes sense to require the remote-endpoint property, as the endpoint, or
>> even the port, doesn't make much sense if there's nothing to connect to.
> 
> Please let's not make it mandatory for a port node to contain an
> endpoint. For any device with multiple ports we can't use the simplified
> form above, and only adding the (correctly numbered) port in all the
> board device trees would be a pain.

That's true. I went with having the ports in the board file, for example
on omap3 the dss has two ports, and N900 board uses the second one:

&dss {
	status = "ok";

	pinctrl-names = "default";
	pinctrl-0 = <&dss_sdi_pins>;

	vdds_sdi-supply = <&vaux1>;

	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		port@1 {
			reg = <1>;

			sdi_out: endpoint {
				remote-endpoint = <&lcd_in>;
				datapairs = <2>;
			};
		};
	};
};

Here I guess I could have:

&dss {
	status = "ok";

	pinctrl-names = "default";
	pinctrl-0 = <&dss_sdi_pins>;

	vdds_sdi-supply = <&vaux1>;
};

&dss_sdi_port {
	sdi_out: endpoint {
		remote-endpoint = <&lcd_in>;
		datapairs = <2>;
	};
};

But I didn't like that as it splits the pincontrol and regulator supply
from the port/endpoint, which are functionally linked together.

Actually, somewhat aside the subject, I'd like to have the pinctrl and
maybe regulator supply also per endpoint, but I didn't see how that
would be possible with the current framework. If a board would need to
endpoints for the same port, most likely it would also need to different
sets of pinctrls.

 Tomi



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

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Tomi Valkeinen]

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

On 26/02/14 17:47, Philipp Zabel wrote:

> Ok, that looks compact enough. I still don't see the need to change make
> the remote-endpoint property required to achieve this, though. On the
> other hand, I wouldn't object to making it mandatory either.

Sure, having remote-endpoint as required doesn't achieve anything
particular as such. I just feel it's cleaner. If you have an endpoint,
it must point to somewhere. Maybe it makes the code a tiny bit simpler.

If we do already have users for this that do not have the
remote-endpoint, then we're stuck with having it as optional. If we
don't, I'd rather have it as mandatory.

In any case, it's not a very important thing either way.

>> Of course, it's up to the developer how his dts looks like. But to me it
>> makes sense to require the remote-endpoint property, as the endpoint, or
>> even the port, doesn't make much sense if there's nothing to connect to.
> 
> Please let's not make it mandatory for a port node to contain an
> endpoint. For any device with multiple ports we can't use the simplified
> form above, and only adding the (correctly numbered) port in all the
> board device trees would be a pain.

That's true. I went with having the ports in the board file, for example
on omap3 the dss has two ports, and N900 board uses the second one:

&dss {
	status = "ok";

	pinctrl-names = "default";
	pinctrl-0 = <&dss_sdi_pins>;

	vdds_sdi-supply = <&vaux1>;

	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		port@1 {
			reg = <1>;

			sdi_out: endpoint {
				remote-endpoint = <&lcd_in>;
				datapairs = <2>;
			};
		};
	};
};

Here I guess I could have:

&dss {
	status = "ok";

	pinctrl-names = "default";
	pinctrl-0 = <&dss_sdi_pins>;

	vdds_sdi-supply = <&vaux1>;
};

&dss_sdi_port {
	sdi_out: endpoint {
		remote-endpoint = <&lcd_in>;
		datapairs = <2>;
	};
};

But I didn't like that as it splits the pincontrol and regulator supply
from the port/endpoint, which are functionally linked together.

Actually, somewhat aside the subject, I'd like to have the pinctrl and
maybe regulator supply also per endpoint, but I didn't see how that
would be possible with the current framework. If a board would need to
endpoints for the same port, most likely it would also need to different
sets of pinctrls.

 Tomi



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

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Philipp Zabel]

Hi Tomi,

Am Donnerstag, den 27.02.2014, 10:08 +0200 schrieb Tomi Valkeinen:
> On 26/02/14 17:47, Philipp Zabel wrote:
> > Please let's not make it mandatory for a port node to contain an
> > endpoint. For any device with multiple ports we can't use the simplified
> > form above, and only adding the (correctly numbered) port in all the
> > board device trees would be a pain.
> 
> That's true. I went with having the ports in the board file, for example
> on omap3 the dss has two ports, and N900 board uses the second one:
> 
> &dss {
> 	status = "ok";
> 
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&dss_sdi_pins>;
> 
> 	vdds_sdi-supply = <&vaux1>;
> 
> 	ports {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		port@1 {
> 			reg = <1>;
> 
> 			sdi_out: endpoint {
> 				remote-endpoint = <&lcd_in>;
> 				datapairs = <2>;
> 			};
> 		};
> 	};
> };

This is a bit verbose, and if your output port is on an encoder device
with multiple inputs, the correct port number would become a bit
unintuitive. For example, we'd have to use port@4 as the output encoder
units that have a 4-port input multiplexer and port@1 for those that
don't.

> Here I guess I could have:
> 
> &dss {
> 	status = "ok";
> 
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&dss_sdi_pins>;
> 
> 	vdds_sdi-supply = <&vaux1>;
> };

What is supplied by this regulator. Is it the PHY?

> &dss_sdi_port {
> 	sdi_out: endpoint {
> 		remote-endpoint = <&lcd_in>;
> 		datapairs = <2>;
> 	};
> };
> 
> But I didn't like that as it splits the pincontrol and regulator supply
> from the port/endpoint, which are functionally linked together.
>
> Actually, somewhat aside the subject, I'd like to have the pinctrl and
> maybe regulator supply also per endpoint, but I didn't see how that
> would be possible with the current framework. If a board would need to
> endpoints for the same port, most likely it would also need to different
> sets of pinctrls.

I have a usecase for this the other way around. The i.MX6 DISP0 parallel
display pads can be connected to two different display controllers via
multiplexers in the pin control block.

parallel-display {
	compatible = "fsl,imx-parallel-display";
	#address-cells = <1>;
	#size-cells = <0>;

	port@0 {
		endpoint {
			remote-endpoint = <&ipu1_di0>;
		};
	};

	port@1 {
		endpoint {
			remote-endpoint = <&ipu2_di0>;
		};
	};

	disp0: port@2 {
		endpoint {
			pinctrl-names = "0", "1";
			pinctrl-0 = <&pinctrl_disp0_ipu1>;
			pinctrl-1 = <&pinctrl_disp0_ipu2>;
			remote-endpoint = <&lcd_in>;
		};
	}
};

Here, depending on the active input port, the corresponding pin control
on the output port could be set. This is probably quite driver specific,
so I don't see yet how the framework should help with this. In any case,
maybe this is a bit out of scope for the generic graph bindings.

regards
Philipp

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Tomi Valkeinen]

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

On 27/02/14 12:52, Philipp Zabel wrote:

> This is a bit verbose, and if your output port is on an encoder device
> with multiple inputs, the correct port number would become a bit
> unintuitive. For example, we'd have to use port@4 as the output encoder
> units that have a 4-port input multiplexer and port@1 for those that
> don't.

Hmm, sorry, I don't follow...

The port numbers should be fixed for a particular device. If the device
has 4 input ports, the output port would always be port@4, no matter how
many of the input ports are actually used.

I don't have anything against having the ports described in the SoC
dtsi. But I do think it may make it a bit unclear that the ports are
from the same device, and share things like pinmuxing. Both approaches
should work fine, afaics.

However, if, instead, we could have the pinmuxing and other relevant
information in the port or endpoint nodes, making the ports independent
of each other and of the device behind them, I argument above would
disappear.

Also, while I'm all for making the dts files clear, I do think the one
writing the dts still needs to go carefully through the binding docs.
Say, a device may need a gpio list with a bunch of gpios. The developer
just needs to read the docs and know that gpio #3 on the list is GPIO_XYZ.

So I don't see it as a major problem that the board developer needs to
know that port@1 on OMAP3's DSS is SDI output.

>> Here I guess I could have:
>>
>> &dss {
>> 	status = "ok";
>>
>> 	pinctrl-names = "default";
>> 	pinctrl-0 = <&dss_sdi_pins>;
>>
>> 	vdds_sdi-supply = <&vaux1>;
>> };
> 
> What is supplied by this regulator. Is it the PHY?

Yes.

>> Actually, somewhat aside the subject, I'd like to have the pinctrl and
>> maybe regulator supply also per endpoint, but I didn't see how that
>> would be possible with the current framework. If a board would need to
>> endpoints for the same port, most likely it would also need to different
>> sets of pinctrls.
> 
> I have a usecase for this the other way around. The i.MX6 DISP0 parallel
> display pads can be connected to two different display controllers via
> multiplexers in the pin control block.
> 
> parallel-display {
> 	compatible = "fsl,imx-parallel-display";
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 
> 	port@0 {
> 		endpoint {
> 			remote-endpoint = <&ipu1_di0>;
> 		};
> 	};
> 
> 	port@1 {
> 		endpoint {
> 			remote-endpoint = <&ipu2_di0>;
> 		};
> 	};
> 
> 	disp0: port@2 {
> 		endpoint {
> 			pinctrl-names = "0", "1";
> 			pinctrl-0 = <&pinctrl_disp0_ipu1>;
> 			pinctrl-1 = <&pinctrl_disp0_ipu2>;
> 			remote-endpoint = <&lcd_in>;
> 		};
> 	}
> };
> 
> Here, depending on the active input port, the corresponding pin control
> on the output port could be set. This is probably quite driver specific,
> so I don't see yet how the framework should help with this. In any case,
> maybe this is a bit out of scope for the generic graph bindings.

Hmm, why wouldn't you have the pinctrl definitions in the ports 0 and 1,
then, if it's about selecting the active input pins?

I think the pinctrl framework could offer ways to have pinctrl
definitions anywhere in the DT structure. It'd be up to the driver to
point to the pinctrl in the DT, ask the framework to parse them, and
eventually enable/disable the pins.

But yes, it's a bit out of scope =).

 Tomi



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

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Tomi Valkeinen]

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

On 27/02/14 12:52, Philipp Zabel wrote:

> This is a bit verbose, and if your output port is on an encoder device
> with multiple inputs, the correct port number would become a bit
> unintuitive. For example, we'd have to use port@4 as the output encoder
> units that have a 4-port input multiplexer and port@1 for those that
> don't.

Hmm, sorry, I don't follow...

The port numbers should be fixed for a particular device. If the device
has 4 input ports, the output port would always be port@4, no matter how
many of the input ports are actually used.

I don't have anything against having the ports described in the SoC
dtsi. But I do think it may make it a bit unclear that the ports are
from the same device, and share things like pinmuxing. Both approaches
should work fine, afaics.

However, if, instead, we could have the pinmuxing and other relevant
information in the port or endpoint nodes, making the ports independent
of each other and of the device behind them, I argument above would
disappear.

Also, while I'm all for making the dts files clear, I do think the one
writing the dts still needs to go carefully through the binding docs.
Say, a device may need a gpio list with a bunch of gpios. The developer
just needs to read the docs and know that gpio #3 on the list is GPIO_XYZ.

So I don't see it as a major problem that the board developer needs to
know that port@1 on OMAP3's DSS is SDI output.

>> Here I guess I could have:
>>
>> &dss {
>> 	status = "ok";
>>
>> 	pinctrl-names = "default";
>> 	pinctrl-0 = <&dss_sdi_pins>;
>>
>> 	vdds_sdi-supply = <&vaux1>;
>> };
> 
> What is supplied by this regulator. Is it the PHY?

Yes.

>> Actually, somewhat aside the subject, I'd like to have the pinctrl and
>> maybe regulator supply also per endpoint, but I didn't see how that
>> would be possible with the current framework. If a board would need to
>> endpoints for the same port, most likely it would also need to different
>> sets of pinctrls.
> 
> I have a usecase for this the other way around. The i.MX6 DISP0 parallel
> display pads can be connected to two different display controllers via
> multiplexers in the pin control block.
> 
> parallel-display {
> 	compatible = "fsl,imx-parallel-display";
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 
> 	port@0 {
> 		endpoint {
> 			remote-endpoint = <&ipu1_di0>;
> 		};
> 	};
> 
> 	port@1 {
> 		endpoint {
> 			remote-endpoint = <&ipu2_di0>;
> 		};
> 	};
> 
> 	disp0: port@2 {
> 		endpoint {
> 			pinctrl-names = "0", "1";
> 			pinctrl-0 = <&pinctrl_disp0_ipu1>;
> 			pinctrl-1 = <&pinctrl_disp0_ipu2>;
> 			remote-endpoint = <&lcd_in>;
> 		};
> 	}
> };
> 
> Here, depending on the active input port, the corresponding pin control
> on the output port could be set. This is probably quite driver specific,
> so I don't see yet how the framework should help with this. In any case,
> maybe this is a bit out of scope for the generic graph bindings.

Hmm, why wouldn't you have the pinctrl definitions in the ports 0 and 1,
then, if it's about selecting the active input pins?

I think the pinctrl framework could offer ways to have pinctrl
definitions anywhere in the DT structure. It'd be up to the driver to
point to the pinctrl in the DT, ask the framework to parse them, and
eventually enable/disable the pins.

But yes, it's a bit out of scope =).

 Tomi



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

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Grant Likely]

On Wed, 26 Feb 2014 16:50:52 +0200, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 26/02/14 16:57, Philipp Zabel wrote:
> > Hi Tomi,
> > 
> > Am Mittwoch, den 26.02.2014, 15:14 +0200 schrieb Tomi Valkeinen:
> >> On 25/02/14 16:58, Philipp Zabel wrote:
> >>
> >>> +Optional endpoint properties
> >>> +----------------------------
> >>> +
> >>> +- remote-endpoint: phandle to an 'endpoint' subnode of a remote device node.
> >>
> >> Why is that optional? What use is an endpoint, if it's not connected to
> >> something?
> > 
> > This allows to include the an empty endpoint template in a SoC dtsi for
> > the convenience of board dts writers. Also, the same property is
> > currently listed as optional in video-interfaces.txt.
> > 
> >   soc.dtsi:
> > 	display-controller {
> > 		port {
> > 			disp0: endpoint { };
> > 		};
> > 	};
> > 
> >   board.dts:
> > 	#include "soc.dtsi"
> > 	&disp0 {
> > 		remote-endpoint = <&panel_input>;
> > 	};
> > 	panel {
> > 		port {
> > 			panel_in: endpoint {
> > 				remote-endpoint = <&disp0>;
> > 			};
> > 		};
> > 	};
> > 
> > Any board not using that port can just leave the endpoint disconnected.
> 
> Hmm I see. I'm against that.
> 
> I think the SoC dtsi should not contain endpoint node, or even port node
> (at least usually). It doesn't know how many endpoints, if any, a
> particular board has. That part should be up to the board dts.

Why? We have established precedence for unused devices still being in
the tree. I really see no issue with it.

g.

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Grant Likely]

On Wed, 26 Feb 2014 16:50:52 +0200, Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org> wrote:
> On 26/02/14 16:57, Philipp Zabel wrote:
> > Hi Tomi,
> > 
> > Am Mittwoch, den 26.02.2014, 15:14 +0200 schrieb Tomi Valkeinen:
> >> On 25/02/14 16:58, Philipp Zabel wrote:
> >>
> >>> +Optional endpoint properties
> >>> +----------------------------
> >>> +
> >>> +- remote-endpoint: phandle to an 'endpoint' subnode of a remote device node.
> >>
> >> Why is that optional? What use is an endpoint, if it's not connected to
> >> something?
> > 
> > This allows to include the an empty endpoint template in a SoC dtsi for
> > the convenience of board dts writers. Also, the same property is
> > currently listed as optional in video-interfaces.txt.
> > 
> >   soc.dtsi:
> > 	display-controller {
> > 		port {
> > 			disp0: endpoint { };
> > 		};
> > 	};
> > 
> >   board.dts:
> > 	#include "soc.dtsi"
> > 	&disp0 {
> > 		remote-endpoint = <&panel_input>;
> > 	};
> > 	panel {
> > 		port {
> > 			panel_in: endpoint {
> > 				remote-endpoint = <&disp0>;
> > 			};
> > 		};
> > 	};
> > 
> > Any board not using that port can just leave the endpoint disconnected.
> 
> Hmm I see. I'm against that.
> 
> I think the SoC dtsi should not contain endpoint node, or even port node
> (at least usually). It doesn't know how many endpoints, if any, a
> particular board has. That part should be up to the board dts.

Why? We have established precedence for unused devices still being in
the tree. I really see no issue with it.

g.

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

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Tomi Valkeinen]

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

On 07/03/14 20:11, Grant Likely wrote:

>>> Any board not using that port can just leave the endpoint disconnected.
>>
>> Hmm I see. I'm against that.
>>
>> I think the SoC dtsi should not contain endpoint node, or even port node
>> (at least usually). It doesn't know how many endpoints, if any, a
>> particular board has. That part should be up to the board dts.
> 
> Why? We have established precedence for unused devices still being in
> the tree. I really see no issue with it.

I'm fine with having ports defined in the SoC dtsi. A port is a physical
thing, a group of pins, for example.

But an endpoint is a description of the other end of a link. To me, a
single endpoint makes no sense, there has to be a pair of endpoints. The
board may need 0 to n endpoints, and the SoC dtsi cannot know how many
are needed.

If the SoC dtsi defines a single endpoint for a port, and the board
needs to use two endpoints for that port, it gets really messy: one
endpoint is defined in the SoC dtsi, and used in the board dts. The
second endpoint for the same port needs to be defined separately in the
board file. I.e. something like:

/* the first ep */
&port1_ep {
	remote-endpoint = <&..>;
};

&port1 {
	/* the second ep */
	endpoint@2 {
		remote-endpoint = <&..>;
	};
};

Versus:

&port1 {
	/* the first ep */
	endpoint@1 {
		remote-endpoint = <&..>;
	};

	/* the second ep */
	endpoint@2 {
		remote-endpoint = <&..>;
	};
};

 Tomi



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

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Tomi Valkeinen]

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

On 07/03/14 20:11, Grant Likely wrote:

>>> Any board not using that port can just leave the endpoint disconnected.
>>
>> Hmm I see. I'm against that.
>>
>> I think the SoC dtsi should not contain endpoint node, or even port node
>> (at least usually). It doesn't know how many endpoints, if any, a
>> particular board has. That part should be up to the board dts.
> 
> Why? We have established precedence for unused devices still being in
> the tree. I really see no issue with it.

I'm fine with having ports defined in the SoC dtsi. A port is a physical
thing, a group of pins, for example.

But an endpoint is a description of the other end of a link. To me, a
single endpoint makes no sense, there has to be a pair of endpoints. The
board may need 0 to n endpoints, and the SoC dtsi cannot know how many
are needed.

If the SoC dtsi defines a single endpoint for a port, and the board
needs to use two endpoints for that port, it gets really messy: one
endpoint is defined in the SoC dtsi, and used in the board dts. The
second endpoint for the same port needs to be defined separately in the
board file. I.e. something like:

/* the first ep */
&port1_ep {
	remote-endpoint = <&..>;
};

&port1 {
	/* the second ep */
	endpoint@2 {
		remote-endpoint = <&..>;
	};
};

Versus:

&port1 {
	/* the first ep */
	endpoint@1 {
		remote-endpoint = <&..>;
	};

	/* the second ep */
	endpoint@2 {
		remote-endpoint = <&..>;
	};
};

 Tomi



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

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Grant Likely]

On Sat, 8 Mar 2014 11:35:38 +0200, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 07/03/14 20:11, Grant Likely wrote:
> 
> >>> Any board not using that port can just leave the endpoint disconnected.
> >>
> >> Hmm I see. I'm against that.
> >>
> >> I think the SoC dtsi should not contain endpoint node, or even port node
> >> (at least usually). It doesn't know how many endpoints, if any, a
> >> particular board has. That part should be up to the board dts.
> > 
> > Why? We have established precedence for unused devices still being in
> > the tree. I really see no issue with it.
> 
> I'm fine with having ports defined in the SoC dtsi. A port is a physical
> thing, a group of pins, for example.
> 
> But an endpoint is a description of the other end of a link. To me, a
> single endpoint makes no sense, there has to be a pair of endpoints. The
> board may need 0 to n endpoints, and the SoC dtsi cannot know how many
> are needed.
> 
> If the SoC dtsi defines a single endpoint for a port, and the board
> needs to use two endpoints for that port, it gets really messy: one
> endpoint is defined in the SoC dtsi, and used in the board dts. The
> second endpoint for the same port needs to be defined separately in the
> board file. I.e. something like:

Sure. If endpoints are logical, then only create the ones actually
hooked up. No problem there. But nor do I see any issue with having
empty connections if the board author things it makes sense to have them
in the dtsi.

> 
> /* the first ep */
> &port1_ep {
> 	remote-endpoint = <&..>;
> };
> 
> &port1 {
> 	/* the second ep */
> 	endpoint@2 {
> 		remote-endpoint = <&..>;
> 	};
> };
> 
> Versus:
> 
> &port1 {
> 	/* the first ep */
> 	endpoint@1 {
> 		remote-endpoint = <&..>;
> 	};
> 
> 	/* the second ep */
> 	endpoint@2 {
> 		remote-endpoint = <&..>;
> 	};
> };
> 
>  Tomi
> 
> 

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Laurent Pinchart]

Hi Grant,

On Saturday 08 March 2014 12:25:32 Grant Likely wrote:
> On Sat, 8 Mar 2014 11:35:38 +0200, Tomi Valkeinen wrote:
> > On 07/03/14 20:11, Grant Likely wrote:
> > >>> Any board not using that port can just leave the endpoint
> > >>> disconnected.
> > >> 
> > >> Hmm I see. I'm against that.
> > >> 
> > >> I think the SoC dtsi should not contain endpoint node, or even port
> > >> node (at least usually). It doesn't know how many endpoints, if any, a
> > >> particular board has. That part should be up to the board dts.
> > > 
> > > Why? We have established precedence for unused devices still being in
> > > the tree. I really see no issue with it.
> > 
> > I'm fine with having ports defined in the SoC dtsi. A port is a physical
> > thing, a group of pins, for example.
> > 
> > But an endpoint is a description of the other end of a link. To me, a
> > single endpoint makes no sense, there has to be a pair of endpoints. The
> > board may need 0 to n endpoints, and the SoC dtsi cannot know how many
> > are needed.
> > 
> > If the SoC dtsi defines a single endpoint for a port, and the board
> > needs to use two endpoints for that port, it gets really messy: one
> > endpoint is defined in the SoC dtsi, and used in the board dts. The
> > second endpoint for the same port needs to be defined separately in the
> > board file. I.e. something like:
>
> Sure. If endpoints are logical, then only create the ones actually hooked
> up. No problem there. But nor do I see any issue with having empty
> connections if the board author things it makes sense to have them in the
> dtsi.

I don't mind allowing board authors to add empty connections if they want to, 
but I think it's a good practice not to include them given that endpoint are 
logical. I would at least not include them in the of-graph DT bindings 
examples.

> > /* the first ep */
> > &port1_ep {
> > 	remote-endpoint = <&..>;
> > };
> > 
> > &port1 {
> > 	/* the second ep */
> > 	endpoint@2 {
> > 		remote-endpoint = <&..>;
> > 	};
> > };
> > 
> > Versus:
> > 
> > &port1 {
> > 	/* the first ep */
> > 	endpoint@1 {
> > 		remote-endpoint = <&..>;
> > 	};
> > 	
> > 	/* the second ep */
> > 	endpoint@2 {
> > 		remote-endpoint = <&..>;
> > 	};
> > };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Tomi Valkeinen]

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

On 08/03/14 14:25, Grant Likely wrote:

> Sure. If endpoints are logical, then only create the ones actually
> hooked up. No problem there. But nor do I see any issue with having
> empty connections if the board author things it makes sense to have them
> in the dtsi.

I don't think they are usually logical, although they probably might be
in some cases.

As I see it, a "port" is a group of pins in a hardware component, and
two endpoints define a connection between two ports, which on the HW
level are the wires between the ports.

So a port with two endpoints is a group of pins, with wires that go from
the same pins to two different components.

 Tomi



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

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Tomi Valkeinen]

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

On 08/03/14 14:25, Grant Likely wrote:

> Sure. If endpoints are logical, then only create the ones actually
> hooked up. No problem there. But nor do I see any issue with having
> empty connections if the board author things it makes sense to have them
> in the dtsi.

I don't think they are usually logical, although they probably might be
in some cases.

As I see it, a "port" is a group of pins in a hardware component, and
two endpoints define a connection between two ports, which on the HW
level are the wires between the ports.

So a port with two endpoints is a group of pins, with wires that go from
the same pins to two different components.

 Tomi



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

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Sylwester Nawrocki]

On 10/03/14 07:53, Tomi Valkeinen wrote:
> On 08/03/14 14:25, Grant Likely wrote:
> 
>> Sure. If endpoints are logical, then only create the ones actually
>> hooked up. No problem there. But nor do I see any issue with having
>> empty connections if the board author things it makes sense to have them
>> in the dtsi.
> 
> I don't think they are usually logical, although they probably might be
> in some cases.

The endpoint nodes are supposed to be logical, they just group properties
describing a port's configuration.

> As I see it, a "port" is a group of pins in a hardware component, and
> two endpoints define a connection between two ports, which on the HW
> level are the wires between the ports.
> 
> So a port with two endpoints is a group of pins, with wires that go from
> the same pins to two different components.

It could be approximated like this, but I don't think it is needed.
I would rather stay with only port nodes mapped to hardware and the
endpoint nodes logical.

--
Regards,
Sylwester

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Sylwester Nawrocki]

On 10/03/14 07:53, Tomi Valkeinen wrote:
> On 08/03/14 14:25, Grant Likely wrote:
> 
>> Sure. If endpoints are logical, then only create the ones actually
>> hooked up. No problem there. But nor do I see any issue with having
>> empty connections if the board author things it makes sense to have them
>> in the dtsi.
> 
> I don't think they are usually logical, although they probably might be
> in some cases.

The endpoint nodes are supposed to be logical, they just group properties
describing a port's configuration.

> As I see it, a "port" is a group of pins in a hardware component, and
> two endpoints define a connection between two ports, which on the HW
> level are the wires between the ports.
> 
> So a port with two endpoints is a group of pins, with wires that go from
> the same pins to two different components.

It could be approximated like this, but I don't think it is needed.
I would rather stay with only port nodes mapped to hardware and the
endpoint nodes logical.

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

Re: [PATCH v4 3/3] Documentation: of: Document graph bindings

[Grant Likely]

On Wed, 26 Feb 2014 15:14:17 +0200, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 25/02/14 16:58, Philipp Zabel wrote:
> 
> > +Optional endpoint properties
> > +----------------------------
> > +
> > +- remote-endpoint: phandle to an 'endpoint' subnode of a remote device node.
> 
> Why is that optional? What use is an endpoint, if it's not connected to
> something?
> 
> Also, if this is being worked on, I'd like to propose the addition of
> simpler single-endpoint cases which I've been using with OMAP DSS. So if
> there's just a single endpoint for the device, which is very common, you
> can have just:
> 
> device {
> 	...
> 	endpoint { ... };
> };
> 
> However, I guess that the patch just keeps growing and growing, so maybe
> it's better to add such things later =).

It's good to talk about it now while boiling down the core behaviour
into a useful pattern. That said, I think if the stuff I've commented on
already is addressed then it will probably be sufficient for me to ack
or merge the change.

g.