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
From: Philipp Zabel <[email protected]>
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 <[email protected]>
---
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 <[email protected]>
+ *
+ * Copyright (C) 2012 Renesas Electronics Corp.
+ * Author: Guennadi Liakhovetski <[email protected]>
+ *
+ * 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 <[email protected]>
+ *
+ * Copyright (C) 2012 Renesas Electronics Corp.
+ * Author: Guennadi Liakhovetski <[email protected]>
+ *
+ * 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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Tue, 25 Feb 2014 15:58:22 +0100, Philipp Zabel <[email protected]> wrote:
> From: Philipp Zabel <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> + *
> + * Copyright (C) 2012 Renesas Electronics Corp.
> + * Author: Guennadi Liakhovetski <[email protected]>
> + *
> + * 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 <[email protected]>
> + *
> + * Copyright (C) 2012 Renesas Electronics Corp.
> + * Author: Guennadi Liakhovetski <[email protected]>
> + *
> + * 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
>
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
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
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
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
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
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
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
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