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. There have been patches that duplicate
the code (and I am going to send one of my own), such as
http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html
and others that parse the same binding in a different way:
https://www.mail-archive.com/[email protected]/msg100761.html
I think that all common video interface parsing helpers should be moved to a
single place, outside of the specific subsystems, so that it can be reused
by all drivers.
I moved v4l2_of_get_next_endpoint, v4l2_of_get_remote_port,
and v4l2_of_get_remote_port_parent. They are renamed to
of_graph_get_next_endpoint, of_graph_get_remote_port, and
of_graph_get_remote_port_parent, respectively.
Signed-off-by: Philipp Zabel <[email protected]>
---
drivers/media/Kconfig | 1 +
drivers/media/v4l2-core/v4l2-of.c | 117 ---------------------------------
drivers/of/Kconfig | 3 +
drivers/of/Makefile | 1 +
drivers/of/of_graph.c | 133 ++++++++++++++++++++++++++++++++++++++
include/linux/of_graph.h | 23 +++++++
include/media/v4l2-of.h | 16 ++---
7 files changed, 167 insertions(+), 127 deletions(-)
create mode 100644 drivers/of/of_graph.c
create mode 100644 include/linux/of_graph.h
diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
index 1d0758a..882faeb 100644
--- a/drivers/media/Kconfig
+++ b/drivers/media/Kconfig
@@ -96,6 +96,7 @@ config VIDEO_DEV
tristate
depends on MEDIA_SUPPORT
depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_RADIO_SUPPORT
+ select OF_GRAPH if OF
default y
config VIDEO_V4L2_SUBDEV_API
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/Kconfig b/drivers/of/Kconfig
index c6973f1..1bfbb0e 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -75,4 +75,7 @@ config OF_MTD
depends on MTD
def_bool y
+config OF_GRAPH
+ bool
+
endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index efd0510..7ee8ab3 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_GRAPH) += of_graph.o
diff --git a/drivers/of/of_graph.c b/drivers/of/of_graph.c
new file mode 100644
index 0000000..aa526d7
--- /dev/null
+++ b/drivers/of/of_graph.c
@@ -0,0 +1,133 @@
+/*
+ * 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/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..352306a
--- /dev/null
+++ b/include/linux/of_graph.h
@@ -0,0 +1,23 @@
+/*
+ * 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
+
+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);
+
+#endif /* __LINUX_OF_GRAPH_H */
diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
index 541cea4..404a493 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,25 @@ static inline int v4l2_of_parse_endpoint(const struct device_node *node,
return -ENOSYS;
}
+#endif /* CONFIG_OF */
+
static inline struct device_node *v4l2_of_get_next_endpoint(
const struct device_node *parent,
struct device_node *previous)
{
- return NULL;
+ return of_graph_get_next_endpoint(parent, previous);
}
static inline struct device_node *v4l2_of_get_remote_port_parent(
const struct device_node *node)
{
- return NULL;
+ return of_graph_get_remote_port_parent(node);
}
static inline struct device_node *v4l2_of_get_remote_port(
const struct device_node *node)
{
- return NULL;
+ return of_graph_get_remote_port(node);
}
-#endif /* CONFIG_OF */
-
#endif /* _V4L2_OF_H */
--
1.8.5.3
On Tue, Feb 11, 2014 at 5:45 AM, 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 is the opposite direction things have been moving...
> This allows to reuse the same parser code from outside the V4L2 framework,
> most importantly from display drivers. There have been patches that duplicate
> the code (and I am going to send one of my own), such as
> http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html
> and others that parse the same binding in a different way:
> https://www.mail-archive.com/[email protected]/msg100761.html
>
> I think that all common video interface parsing helpers should be moved to a
> single place, outside of the specific subsystems, so that it can be reused
> by all drivers.
Perhaps that should be done rather than moving to drivers/of now and
then again to somewhere else.
> I moved v4l2_of_get_next_endpoint, v4l2_of_get_remote_port,
> and v4l2_of_get_remote_port_parent. They are renamed to
> of_graph_get_next_endpoint, of_graph_get_remote_port, and
> of_graph_get_remote_port_parent, respectively.
>
> Signed-off-by: Philipp Zabel <[email protected]>
> ---
> drivers/media/Kconfig | 1 +
> drivers/media/v4l2-core/v4l2-of.c | 117 ---------------------------------
> drivers/of/Kconfig | 3 +
> drivers/of/Makefile | 1 +
> drivers/of/of_graph.c | 133 ++++++++++++++++++++++++++++++++++++++
> include/linux/of_graph.h | 23 +++++++
> include/media/v4l2-of.h | 16 ++---
> 7 files changed, 167 insertions(+), 127 deletions(-)
> create mode 100644 drivers/of/of_graph.c
> create mode 100644 include/linux/of_graph.h
[snip]
> diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
> index 541cea4..404a493 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,25 @@ static inline int v4l2_of_parse_endpoint(const struct device_node *node,
> return -ENOSYS;
> }
>
> +#endif /* CONFIG_OF */
> +
> static inline struct device_node *v4l2_of_get_next_endpoint(
> const struct device_node *parent,
> struct device_node *previous)
> {
> - return NULL;
> + return of_graph_get_next_endpoint(parent, previous);
Won't this break for !OF?
Rob
On Tue, Feb 11, 2014 at 07:56:33AM -0600, Rob Herring wrote:
> On Tue, Feb 11, 2014 at 5:45 AM, Philipp Zabel <[email protected]> wrote:
> > This allows to reuse the same parser code from outside the V4L2 framework,
> > most importantly from display drivers. There have been patches that duplicate
> > the code (and I am going to send one of my own), such as
> > http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html
> > and others that parse the same binding in a different way:
> > https://www.mail-archive.com/[email protected]/msg100761.html
> >
> > I think that all common video interface parsing helpers should be moved to a
> > single place, outside of the specific subsystems, so that it can be reused
> > by all drivers.
>
> Perhaps that should be done rather than moving to drivers/of now and
> then again to somewhere else.
Do you have a better suggestion where it should move to?
drivers/gpu/drm - no, because v4l2 wants to use it
drivers/media/video - no, because DRM drivers want to use it
drivers/video - no, because v4l2 and drm drivers want to use it
Maybe drivers/of-graph/ ? Or maybe it's just as good a place to move it
into drivers/of ?
--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
Hi Russell,
On Tuesday 11 February 2014 14:52:48 Russell King - ARM Linux wrote:
> On Tue, Feb 11, 2014 at 07:56:33AM -0600, Rob Herring wrote:
> > On Tue, Feb 11, 2014 at 5:45 AM, Philipp Zabel wrote:
> > > This allows to reuse the same parser code from outside the V4L2
> > > framework, most importantly from display drivers. There have been
> > > patches that duplicate the code (and I am going to send one of my own),
> > > such as
> > > http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html
> > > and others that parse the same binding in a different way:
> > > https://www.mail-archive.com/[email protected]/msg100761.html
> > >
> > > I think that all common video interface parsing helpers should be moved
> > > to a single place, outside of the specific subsystems, so that it can
> > > be reused by all drivers.
> >
> > Perhaps that should be done rather than moving to drivers/of now and
> > then again to somewhere else.
>
> Do you have a better suggestion where it should move to?
>
> drivers/gpu/drm - no, because v4l2 wants to use it
> drivers/media/video - no, because DRM drivers want to use it
> drivers/video - no, because v4l2 and drm drivers want to use it
Just pointing out a missing location (which might be rejected due to similar
concerns), there's also drivers/media, which isn't V4L-specific.
> Maybe drivers/of-graph/ ? Or maybe it's just as good a place to move it
> into drivers/of ?
--
Regards,
Laurent Pinchart
Hi Rob,
Am Dienstag, den 11.02.2014, 07:56 -0600 schrieb Rob Herring:
> On Tue, Feb 11, 2014 at 5:45 AM, 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 is the opposite direction things have been moving...
I understand subsystem specific functionality is moving from drivers/of
into the subsystems. In this case three subsystems all could benefit
from the same set of parsing functions, so it is not clear to me where
if not drivers/of would be the correct place for this code.
> > This allows to reuse the same parser code from outside the V4L2 framework,
> > most importantly from display drivers. There have been patches that duplicate
> > the code (and I am going to send one of my own), such as
> > http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html
> > and others that parse the same binding in a different way:
> > https://www.mail-archive.com/[email protected]/msg100761.html
> >
> > I think that all common video interface parsing helpers should be moved to a
> > single place, outside of the specific subsystems, so that it can be reused
> > by all drivers.
>
> Perhaps that should be done rather than moving to drivers/of now and
> then again to somewhere else.
>
> > I moved v4l2_of_get_next_endpoint, v4l2_of_get_remote_port,
> > and v4l2_of_get_remote_port_parent. They are renamed to
> > of_graph_get_next_endpoint, of_graph_get_remote_port, and
> > of_graph_get_remote_port_parent, respectively.
> >
> > Signed-off-by: Philipp Zabel <[email protected]>
> > ---
> > drivers/media/Kconfig | 1 +
> > drivers/media/v4l2-core/v4l2-of.c | 117 ---------------------------------
> > drivers/of/Kconfig | 3 +
> > drivers/of/Makefile | 1 +
> > drivers/of/of_graph.c | 133 ++++++++++++++++++++++++++++++++++++++
> > include/linux/of_graph.h | 23 +++++++
> > include/media/v4l2-of.h | 16 ++---
> > 7 files changed, 167 insertions(+), 127 deletions(-)
> > create mode 100644 drivers/of/of_graph.c
> > create mode 100644 include/linux/of_graph.h
>
> [snip]
>
> > diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
> > index 541cea4..404a493 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,25 @@ static inline int v4l2_of_parse_endpoint(const struct device_node *node,
> > return -ENOSYS;
> > }
> >
> > +#endif /* CONFIG_OF */
> > +
> > static inline struct device_node *v4l2_of_get_next_endpoint(
> > const struct device_node *parent,
> > struct device_node *previous)
> > {
> > - return NULL;
> > + return of_graph_get_next_endpoint(parent, previous);
>
> Won't this break for !OF?
It will. The of_graph_* functions should get their own stubs for that
case.
regards
Philipp
On Tue, Feb 11, 2014 at 8:52 AM, Russell King - ARM Linux
<[email protected]> wrote:
> On Tue, Feb 11, 2014 at 07:56:33AM -0600, Rob Herring wrote:
>> On Tue, Feb 11, 2014 at 5:45 AM, Philipp Zabel <[email protected]> wrote:
>> > This allows to reuse the same parser code from outside the V4L2 framework,
>> > most importantly from display drivers. There have been patches that duplicate
>> > the code (and I am going to send one of my own), such as
>> > http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html
>> > and others that parse the same binding in a different way:
>> > https://www.mail-archive.com/[email protected]/msg100761.html
>> >
>> > I think that all common video interface parsing helpers should be moved to a
>> > single place, outside of the specific subsystems, so that it can be reused
>> > by all drivers.
>>
>> Perhaps that should be done rather than moving to drivers/of now and
>> then again to somewhere else.
>
> Do you have a better suggestion where it should move to?
No.
> drivers/gpu/drm - no, because v4l2 wants to use it
> drivers/media/video - no, because DRM drivers want to use it
> drivers/video - no, because v4l2 and drm drivers want to use it
I don't believe it exists currently, so it would need to be created.
Perhaps adding a layer of directory to combine these. This patch alone
is not enough to really justify that, but if there's a lot more shared
code possible then it would be the right direction.
> Maybe drivers/of-graph/ ? Or maybe it's just as good a place to move it
> into drivers/of ?
I assume you weren't serious, but no for /of-graph. If a better place
can't be found/made, I'll take it.
Rob
Am Dienstag, den 11.02.2014, 16:23 +0100 schrieb Laurent Pinchart:
> Hi Russell,
>
> On Tuesday 11 February 2014 14:52:48 Russell King - ARM Linux wrote:
> > On Tue, Feb 11, 2014 at 07:56:33AM -0600, Rob Herring wrote:
> > > On Tue, Feb 11, 2014 at 5:45 AM, Philipp Zabel wrote:
> > > > This allows to reuse the same parser code from outside the V4L2
> > > > framework, most importantly from display drivers. There have been
> > > > patches that duplicate the code (and I am going to send one of my own),
> > > > such as
> > > > http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html
> > > > and others that parse the same binding in a different way:
> > > > https://www.mail-archive.com/[email protected]/msg100761.html
> > > >
> > > > I think that all common video interface parsing helpers should be moved
> > > > to a single place, outside of the specific subsystems, so that it can
> > > > be reused by all drivers.
> > >
> > > Perhaps that should be done rather than moving to drivers/of now and
> > > then again to somewhere else.
> >
> > Do you have a better suggestion where it should move to?
> >
> > drivers/gpu/drm - no, because v4l2 wants to use it
> > drivers/media/video - no, because DRM drivers want to use it
> > drivers/video - no, because v4l2 and drm drivers want to use it
>
> Just pointing out a missing location (which might be rejected due to similar
> concerns), there's also drivers/media, which isn't V4L-specific.
Since drivers/Makefile has media/ in obj-y, moving the graph helpers to
drivers/media should technically work.
> > Maybe drivers/of-graph/ ? Or maybe it's just as good a place to move it
> > into drivers/of ?
include/media/of_graph.h,
drivers/media/of_graph.c?
regards
Philipp
(adding Guennadi to Cc)
On 11/02/14 17:36, Philipp Zabel wrote:
> Am Dienstag, den 11.02.2014, 16:23 +0100 schrieb Laurent Pinchart:
>> Hi Russell,
>>
>> On Tuesday 11 February 2014 14:52:48 Russell King - ARM Linux wrote:
>>> On Tue, Feb 11, 2014 at 07:56:33AM -0600, Rob Herring wrote:
>>>> On Tue, Feb 11, 2014 at 5:45 AM, Philipp Zabel wrote:
>>>>> This allows to reuse the same parser code from outside the V4L2
>>>>> framework, most importantly from display drivers. There have been
>>>>> patches that duplicate the code (and I am going to send one of my own),
>>>>> such as
>>>>> http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html
>>>>> and others that parse the same binding in a different way:
>>>>> https://www.mail-archive.com/[email protected]/msg100761.html
>>>>>
>>>>> I think that all common video interface parsing helpers should be moved
>>>>> to a single place, outside of the specific subsystems, so that it can
>>>>> be reused by all drivers.
>>>>
>>>> Perhaps that should be done rather than moving to drivers/of now and
>>>> then again to somewhere else.
>>>
>>> Do you have a better suggestion where it should move to?
>>>
>>> drivers/gpu/drm - no, because v4l2 wants to use it
>>> drivers/media/video - no, because DRM drivers want to use it
>>> drivers/video - no, because v4l2 and drm drivers want to use it
>>
>> Just pointing out a missing location (which might be rejected due to similar
>> concerns), there's also drivers/media, which isn't V4L-specific.
>
> Since drivers/Makefile has media/ in obj-y, moving the graph helpers to
> drivers/media should technically work.
>
>>> Maybe drivers/of-graph/ ? Or maybe it's just as good a place to move it
>>> into drivers/of ?
>
> include/media/of_graph.h,
> drivers/media/of_graph.c?
drivers/media sounds like a good alternative to me.
I would just remove also v4l2_of_{parse/get}* and update the users
to call of_graph_* directly, there should not be many of them.
--
Thanks,
Sylwester
Hi Philipp,
On Tuesday 11 February 2014 17:36:57 Philipp Zabel wrote:
> Am Dienstag, den 11.02.2014, 16:23 +0100 schrieb Laurent Pinchart:
> > On Tuesday 11 February 2014 14:52:48 Russell King - ARM Linux wrote:
> > > On Tue, Feb 11, 2014 at 07:56:33AM -0600, Rob Herring wrote:
> > > > On Tue, Feb 11, 2014 at 5:45 AM, Philipp Zabel wrote:
> > > > > This allows to reuse the same parser code from outside the V4L2
> > > > > framework, most importantly from display drivers. There have been
> > > > > patches that duplicate the code (and I am going to send one of my
> > > > > own),
> > > > > such as
> > > > > http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.h
> > > > > tml
> > > > > and others that parse the same binding in a different way:
> > > > > https://www.mail-archive.com/[email protected]/msg100761.ht
> > > > > ml
> > > > >
> > > > > I think that all common video interface parsing helpers should be
> > > > > moved to a single place, outside of the specific subsystems, so that
> > > > > it can be reused by all drivers.
> > > >
> > > > Perhaps that should be done rather than moving to drivers/of now and
> > > > then again to somewhere else.
> > >
> > > Do you have a better suggestion where it should move to?
> > >
> > > drivers/gpu/drm - no, because v4l2 wants to use it
> > > drivers/media/video - no, because DRM drivers want to use it
> > > drivers/video - no, because v4l2 and drm drivers want to use it
> >
> > Just pointing out a missing location (which might be rejected due to
> > similar concerns), there's also drivers/media, which isn't V4L-specific.
>
> Since drivers/Makefile has media/ in obj-y, moving the graph helpers to
> drivers/media should technically work.
>
> > > Maybe drivers/of-graph/ ? Or maybe it's just as good a place to move it
> > > into drivers/of ?
>
> include/media/of_graph.h,
> drivers/media/of_graph.c?
I'm personally fine with that.
--
Regards,
Laurent Pinchart
Em Tue, 11 Feb 2014 18:22:34 +0100
Sylwester Nawrocki <[email protected]> escreveu:
> (adding Guennadi to Cc)
>
> On 11/02/14 17:36, Philipp Zabel wrote:
> > Am Dienstag, den 11.02.2014, 16:23 +0100 schrieb Laurent Pinchart:
> >> Hi Russell,
> >>
> >> On Tuesday 11 February 2014 14:52:48 Russell King - ARM Linux wrote:
> >>> On Tue, Feb 11, 2014 at 07:56:33AM -0600, Rob Herring wrote:
> >>>> On Tue, Feb 11, 2014 at 5:45 AM, Philipp Zabel wrote:
> >>>>> This allows to reuse the same parser code from outside the V4L2
> >>>>> framework, most importantly from display drivers. There have been
> >>>>> patches that duplicate the code (and I am going to send one of my own),
> >>>>> such as
> >>>>> http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html
> >>>>> and others that parse the same binding in a different way:
> >>>>> https://www.mail-archive.com/[email protected]/msg100761.html
> >>>>>
> >>>>> I think that all common video interface parsing helpers should be moved
> >>>>> to a single place, outside of the specific subsystems, so that it can
> >>>>> be reused by all drivers.
> >>>>
> >>>> Perhaps that should be done rather than moving to drivers/of now and
> >>>> then again to somewhere else.
> >>>
> >>> Do you have a better suggestion where it should move to?
> >>>
> >>> drivers/gpu/drm - no, because v4l2 wants to use it
> >>> drivers/media/video - no, because DRM drivers want to use it
> >>> drivers/video - no, because v4l2 and drm drivers want to use it
> >>
> >> Just pointing out a missing location (which might be rejected due to similar
> >> concerns), there's also drivers/media, which isn't V4L-specific.
> >
> > Since drivers/Makefile has media/ in obj-y, moving the graph helpers to
> > drivers/media should technically work.
> >
> >>> Maybe drivers/of-graph/ ? Or maybe it's just as good a place to move it
> >>> into drivers/of ?
> >
> > include/media/of_graph.h,
> > drivers/media/of_graph.c?
>
> drivers/media sounds like a good alternative to me.
>From my side, I'm ok with putting them at drivers/media. You may add my acked-by
for such change:
Acked-by: Mauro Carvalho Chehab <[email protected]>
> I would just remove also v4l2_of_{parse/get}* and update the users
> to call of_graph_* directly, there should not be many of them.
>
> --
> Thanks,
> Sylwester
>
--
Cheers,
Mauro
On Wed, 12 Feb 2014, Mauro Carvalho Chehab wrote:
> Em Tue, 11 Feb 2014 18:22:34 +0100
> Sylwester Nawrocki <[email protected]> escreveu:
>
> > (adding Guennadi to Cc)
> >
> > On 11/02/14 17:36, Philipp Zabel wrote:
> > > Am Dienstag, den 11.02.2014, 16:23 +0100 schrieb Laurent Pinchart:
> > >> Hi Russell,
> > >>
> > >> On Tuesday 11 February 2014 14:52:48 Russell King - ARM Linux wrote:
> > >>> On Tue, Feb 11, 2014 at 07:56:33AM -0600, Rob Herring wrote:
> > >>>> On Tue, Feb 11, 2014 at 5:45 AM, Philipp Zabel wrote:
> > >>>>> This allows to reuse the same parser code from outside the V4L2
> > >>>>> framework, most importantly from display drivers. There have been
> > >>>>> patches that duplicate the code (and I am going to send one of my own),
> > >>>>> such as
> > >>>>> http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html
> > >>>>> and others that parse the same binding in a different way:
> > >>>>> https://www.mail-archive.com/[email protected]/msg100761.html
> > >>>>>
> > >>>>> I think that all common video interface parsing helpers should be moved
> > >>>>> to a single place, outside of the specific subsystems, so that it can
> > >>>>> be reused by all drivers.
> > >>>>
> > >>>> Perhaps that should be done rather than moving to drivers/of now and
> > >>>> then again to somewhere else.
> > >>>
> > >>> Do you have a better suggestion where it should move to?
> > >>>
> > >>> drivers/gpu/drm - no, because v4l2 wants to use it
> > >>> drivers/media/video - no, because DRM drivers want to use it
> > >>> drivers/video - no, because v4l2 and drm drivers want to use it
> > >>
> > >> Just pointing out a missing location (which might be rejected due to similar
> > >> concerns), there's also drivers/media, which isn't V4L-specific.
> > >
> > > Since drivers/Makefile has media/ in obj-y, moving the graph helpers to
> > > drivers/media should technically work.
> > >
> > >>> Maybe drivers/of-graph/ ? Or maybe it's just as good a place to move it
> > >>> into drivers/of ?
> > >
> > > include/media/of_graph.h,
> > > drivers/media/of_graph.c?
> >
> > drivers/media sounds like a good alternative to me.
>
> From my side, I'm ok with putting them at drivers/media. You may add my acked-by
> for such change:
>
> Acked-by: Mauro Carvalho Chehab <[email protected]>
Cannot see any problems with this
Acked-by: Guennadi Liakhovetski <[email protected]>
Thanks
Guennadi
> > I would just remove also v4l2_of_{parse/get}* and update the users
> > to call of_graph_* directly, there should not be many of them.
> >
> > --
> > Thanks,
> > Sylwester
>
> --
>
> Cheers,
> Mauro
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Hi Sylwester,
On Tue, Feb 11, 2014 at 06:22:34PM +0100, Sylwester Nawrocki wrote:
> drivers/media sounds like a good alternative to me.
>
> I would just remove also v4l2_of_{parse/get}* and update the users
> to call of_graph_* directly, there should not be many of them.
For now I'd like to skip v4l2_of_parse_endpoint. The others can just be
copied verbatim, but this one also depends on struct 4l2_of_endpoint,
struct v4l2_of_bus_*, and <media/v4l2-mediabus.h>.
regards
Philipp
On Tue, 11 Feb 2014 07:56:33 -0600, Rob Herring <[email protected]> wrote:
> On Tue, Feb 11, 2014 at 5:45 AM, 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 is the opposite direction things have been moving...
>
> > This allows to reuse the same parser code from outside the V4L2 framework,
> > most importantly from display drivers. There have been patches that duplicate
> > the code (and I am going to send one of my own), such as
> > http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html
> > and others that parse the same binding in a different way:
> > https://www.mail-archive.com/[email protected]/msg100761.html
> >
> > I think that all common video interface parsing helpers should be moved to a
> > single place, outside of the specific subsystems, so that it can be reused
> > by all drivers.
>
> Perhaps that should be done rather than moving to drivers/of now and
> then again to somewhere else.
This is just parsing helpers though, isn't it? I have no problem pulling
helper functions into drivers/of if they are usable by multiple
subsystems. I don't really understand the model being used though. I
would appreciate a description of the usage model for these functions
for poor folks like me who can't keep track of what is going on in
subsystems.
g.
>
> > I moved v4l2_of_get_next_endpoint, v4l2_of_get_remote_port,
> > and v4l2_of_get_remote_port_parent. They are renamed to
> > of_graph_get_next_endpoint, of_graph_get_remote_port, and
> > of_graph_get_remote_port_parent, respectively.
> >
> > Signed-off-by: Philipp Zabel <[email protected]>
> > ---
> > drivers/media/Kconfig | 1 +
> > drivers/media/v4l2-core/v4l2-of.c | 117 ---------------------------------
> > drivers/of/Kconfig | 3 +
> > drivers/of/Makefile | 1 +
> > drivers/of/of_graph.c | 133 ++++++++++++++++++++++++++++++++++++++
> > include/linux/of_graph.h | 23 +++++++
> > include/media/v4l2-of.h | 16 ++---
> > 7 files changed, 167 insertions(+), 127 deletions(-)
> > create mode 100644 drivers/of/of_graph.c
> > create mode 100644 include/linux/of_graph.h
>
> [snip]
>
> > diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
> > index 541cea4..404a493 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,25 @@ static inline int v4l2_of_parse_endpoint(const struct device_node *node,
> > return -ENOSYS;
> > }
> >
> > +#endif /* CONFIG_OF */
> > +
> > static inline struct device_node *v4l2_of_get_next_endpoint(
> > const struct device_node *parent,
> > struct device_node *previous)
> > {
> > - return NULL;
> > + return of_graph_get_next_endpoint(parent, previous);
>
> Won't this break for !OF?
>
> Rob
Hi Grant,
On Mon, Feb 17, 2014 at 06:14:51PM +0000, Grant Likely wrote:
> On Tue, 11 Feb 2014 07:56:33 -0600, Rob Herring <[email protected]> wrote:
> > On Tue, Feb 11, 2014 at 5:45 AM, 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 is the opposite direction things have been moving...
> >
> > > This allows to reuse the same parser code from outside the V4L2 framework,
> > > most importantly from display drivers. There have been patches that duplicate
> > > the code (and I am going to send one of my own), such as
> > > http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html
> > > and others that parse the same binding in a different way:
> > > https://www.mail-archive.com/[email protected]/msg100761.html
> > >
> > > I think that all common video interface parsing helpers should be moved to a
> > > single place, outside of the specific subsystems, so that it can be reused
> > > by all drivers.
> >
> > Perhaps that should be done rather than moving to drivers/of now and
> > then again to somewhere else.
>
> This is just parsing helpers though, isn't it? I have no problem pulling
> helper functions into drivers/of if they are usable by multiple
> subsystems. I don't really understand the model being used though. I
> would appreciate a description of the usage model for these functions
> for poor folks like me who can't keep track of what is going on in
> subsystems.
You can find it under Documentation/devicetree/bindings/media/video-interfaces.txt
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 17/02/14 19:14, Grant Likely wrote:
> On Tue, 11 Feb 2014 07:56:33 -0600, Rob Herring <[email protected]> wrote:
>> > On Tue, Feb 11, 2014 at 5:45 AM, 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 is the opposite direction things have been moving...
>> >
>>> > > This allows to reuse the same parser code from outside the V4L2 framework,
>>> > > most importantly from display drivers. There have been patches that duplicate
>>> > > the code (and I am going to send one of my own), such as
>>> > > http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html
>>> > > and others that parse the same binding in a different way:
>>> > > https://www.mail-archive.com/[email protected]/msg100761.html
>>> > >
>>> > > I think that all common video interface parsing helpers should be moved to a
>>> > > single place, outside of the specific subsystems, so that it can be reused
>>> > > by all drivers.
>> >
>> > Perhaps that should be done rather than moving to drivers/of now and
>> > then again to somewhere else.
>
> This is just parsing helpers though, isn't it? I have no problem pulling
> helper functions into drivers/of if they are usable by multiple
> subsystems. I don't really understand the model being used though. I
> would appreciate a description of the usage model for these functions
> for poor folks like me who can't keep track of what is going on in
> subsystems.
Yes, this is (mostly) just parsing helpers to walk graph of connected (sub-)
devices. Originally I though about adding this code to drivers/of/of_video.c,
nevertheless it ended up in drivers/media/vl2-core/v4l2-of.c. However those
helpers, likely only after some improvements/enhancements, could be used
in other subsystems like drivers/video or drivers/gpu/drm, wherever a whole
device consists of multiple connected sub-devices.
These helpers are supposed to be used (generically) to walk a graph of
sub-devices, e.g. to find a remote sub-device (e.g. a data transmitter)
to some sub-device (e.g. a data receiver) in order to configure it for
data transmission.
This parsing helpers code could be somehow related to rmk's componentized
device handling [1], in a sense it is supposed to be similarly generic.
I think having those helpers in a common location and shared by subsystems
could be useful in terms of consistent DT bindings for same devices (e.g.
displays) handled currently by multiple kernel subsystems, e.g. DRM, FB,
V4L2.
Of course there might be still some work needed so these helpers are usable
for all (e.g. simplifying corresponding DT binding to have less bloated
description for very simple devices - this has been on my todo list), but
it would be really nice to first agree to the common location.
[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/base/component.c?id=2a41e6070dd7ef539d0f3b1652b4839d04378e11
--
Thanks,
Sylwester
Hi Grant,
Am Montag, den 17.02.2014, 18:14 +0000 schrieb Grant Likely:
> On Tue, 11 Feb 2014 07:56:33 -0600, Rob Herring <[email protected]> wrote:
> > On Tue, Feb 11, 2014 at 5:45 AM, 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 is the opposite direction things have been moving...
> >
> > > This allows to reuse the same parser code from outside the V4L2 framework,
> > > most importantly from display drivers. There have been patches that duplicate
> > > the code (and I am going to send one of my own), such as
> > > http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html
> > > and others that parse the same binding in a different way:
> > > https://www.mail-archive.com/[email protected]/msg100761.html
> > >
> > > I think that all common video interface parsing helpers should be moved to a
> > > single place, outside of the specific subsystems, so that it can be reused
> > > by all drivers.
> >
> > Perhaps that should be done rather than moving to drivers/of now and
> > then again to somewhere else.
>
> This is just parsing helpers though, isn't it? I have no problem pulling
> helper functions into drivers/of if they are usable by multiple
> subsystems. I don't really understand the model being used though. I
> would appreciate a description of the usage model for these functions
> for poor folks like me who can't keep track of what is going on in
> subsystems.
I have taken the liberty to put you on Cc: for the i.MX drm series that
I'd like to use these helpers in. The patch in question is
"[RFC PATCH v3 3/9] staging: imx-drm-core: Use OF graph to find
components and connections between encoder and crtcs"
(http://www.spinics.net/lists/arm-kernel/msg308542.html).
It currently uses local copies (s/of_graph/imx_drm_of/) of the
get_next_endpoint, get_remote_port, and get_remote_port_parent
functions to obtain all necessary components for the componentized
imx-drm device, and to map the connections between crtcs (display
interface ports) and encoders.
regards
Philipp
On Tue, 18 Feb 2014 08:06:24 +0100, Sascha Hauer <[email protected]> wrote:
> Hi Grant,
>
> On Mon, Feb 17, 2014 at 06:14:51PM +0000, Grant Likely wrote:
> > On Tue, 11 Feb 2014 07:56:33 -0600, Rob Herring <[email protected]> wrote:
> > > On Tue, Feb 11, 2014 at 5:45 AM, 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 is the opposite direction things have been moving...
> > >
> > > > This allows to reuse the same parser code from outside the V4L2 framework,
> > > > most importantly from display drivers. There have been patches that duplicate
> > > > the code (and I am going to send one of my own), such as
> > > > http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html
> > > > and others that parse the same binding in a different way:
> > > > https://www.mail-archive.com/[email protected]/msg100761.html
> > > >
> > > > I think that all common video interface parsing helpers should be moved to a
> > > > single place, outside of the specific subsystems, so that it can be reused
> > > > by all drivers.
> > >
> > > Perhaps that should be done rather than moving to drivers/of now and
> > > then again to somewhere else.
> >
> > This is just parsing helpers though, isn't it? I have no problem pulling
> > helper functions into drivers/of if they are usable by multiple
> > subsystems. I don't really understand the model being used though. I
> > would appreciate a description of the usage model for these functions
> > for poor folks like me who can't keep track of what is going on in
> > subsystems.
>
> You can find it under Documentation/devicetree/bindings/media/video-interfaces.txt
Okay, I think I'm okay with moving the helpers, but I will make one
requirement. I would like to have a short binding document describing
the pattern being used. The document above talks a lot about video
specific issues, but the helpers appear to be specifically about the
tree walking properties of the API.
g.
Am Dienstag, den 18.02.2014, 16:26 +0000 schrieb Grant Likely:
> On Tue, 18 Feb 2014 08:06:24 +0100, Sascha Hauer <[email protected]> wrote:
> > Hi Grant,
> >
> > On Mon, Feb 17, 2014 at 06:14:51PM +0000, Grant Likely wrote:
> > > On Tue, 11 Feb 2014 07:56:33 -0600, Rob Herring <[email protected]> wrote:
> > > > On Tue, Feb 11, 2014 at 5:45 AM, 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 is the opposite direction things have been moving...
> > > >
> > > > > This allows to reuse the same parser code from outside the V4L2 framework,
> > > > > most importantly from display drivers. There have been patches that duplicate
> > > > > the code (and I am going to send one of my own), such as
> > > > > http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html
> > > > > and others that parse the same binding in a different way:
> > > > > https://www.mail-archive.com/[email protected]/msg100761.html
> > > > >
> > > > > I think that all common video interface parsing helpers should be moved to a
> > > > > single place, outside of the specific subsystems, so that it can be reused
> > > > > by all drivers.
> > > >
> > > > Perhaps that should be done rather than moving to drivers/of now and
> > > > then again to somewhere else.
> > >
> > > This is just parsing helpers though, isn't it? I have no problem pulling
> > > helper functions into drivers/of if they are usable by multiple
> > > subsystems. I don't really understand the model being used though. I
> > > would appreciate a description of the usage model for these functions
> > > for poor folks like me who can't keep track of what is going on in
> > > subsystems.
> >
> > You can find it under Documentation/devicetree/bindings/media/video-interfaces.txt
>
> Okay, I think I'm okay with moving the helpers, but I will make one
> requirement. I would like to have a short binding document describing
> the pattern being used. The document above talks a lot about video
> specific issues, but the helpers appear to be specifically about the
> tree walking properties of the API.
Reusing the non-video-secific parts of video-interfaces.txt, how about
the following:
"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't 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 can be set 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.
Data interfaces on devices are described by their child 'port' nodes. The port
node contains an 'endpoint' subnode for each remote device 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.
On Mon, 24 Feb 2014 18:36:29 +0100, Philipp Zabel <[email protected]> wrote:
> Am Dienstag, den 18.02.2014, 16:26 +0000 schrieb Grant Likely:
> > >
> > > You can find it under Documentation/devicetree/bindings/media/video-interfaces.txt
> >
> > Okay, I think I'm okay with moving the helpers, but I will make one
> > requirement. I would like to have a short binding document describing
> > the pattern being used. The document above talks a lot about video
> > specific issues, but the helpers appear to be specifically about the
> > tree walking properties of the API.
>
> Reusing the non-video-secific parts of video-interfaces.txt, how about
> the following:
This is good, but I have some comments. This document describes itself
as the common way for doing a device graph within the device tree, but
there is already a well established pattern for device graphs that is
used by the interrupts-extended, clocks and other bindings. Those are
all domain-specific bindings, but the core concept is one device uses
a resource provided by another device. The resource references construct
a graph independent from the natural FDT node graph. (ie. the interrupts
binding forms the interrupt graph. Same for the clock binding).
So, while this binding does describe a pattern for separate device
graphs, it is by no means the only common way of doing so.
I would like the document to acknowledge the difference from the
phandle+args pattern used elsewhere and a description of when it would
be appropriate to use this instead of a simpler binding.
>
> "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.
I would argue that this pattern isn't necessarily restricted to data
flow descriptions. It wants to describe linkage between devices that are
sufficiently complex that the simple binding doesn't do the job.
> The device tree graph bindings allow to describe data bus connections between
> individual devices, that can't 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 can be set 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.
Even if you don't want to declare the direction of data flow, there does
need to be some guidance as to how the binding is constructed. Does
device A point to device B? Or the other way around? Why would someone
choose one over the other? I don't want to see a situation where A & B
point to each other. Things get complex if the graph is allowed to be
cyclical.
> Data interfaces on devices are described by their child 'port' nodes. The port
> node contains an 'endpoint' subnode for each remote device 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.
I really don't like this aspect. It is far too easy to get wrong. Graphs
should be one direction only.
>
> 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.
>
>
Hi Grant,
thank you for the comments.
Am Mittwoch, den 26.02.2014, 11:01 +0000 schrieb Grant Likely:
> On Mon, 24 Feb 2014 18:36:29 +0100, Philipp Zabel <[email protected]> wrote:
> > Am Dienstag, den 18.02.2014, 16:26 +0000 schrieb Grant Likely:
> > > >
> > > > You can find it under Documentation/devicetree/bindings/media/video-interfaces.txt
> > >
> > > Okay, I think I'm okay with moving the helpers, but I will make one
> > > requirement. I would like to have a short binding document describing
> > > the pattern being used. The document above talks a lot about video
> > > specific issues, but the helpers appear to be specifically about the
> > > tree walking properties of the API.
> >
> > Reusing the non-video-secific parts of video-interfaces.txt, how about
> > the following:
>
> This is good, but I have some comments. This document describes itself
> as the common way for doing a device graph within the device tree, but
> there is already a well established pattern for device graphs that is
> used by the interrupts-extended, clocks and other bindings. Those are
> all domain-specific bindings, but the core concept is one device uses
> a resource provided by another device. The resource references construct
> a graph independent from the natural FDT node graph. (ie. the interrupts
> binding forms the interrupt graph. Same for the clock binding).
>
> So, while this binding does describe a pattern for separate device
> graphs, it is by no means the only common way of doing so.
>
> I would like the document to acknowledge the difference from the
> phandle+args pattern used elsewhere and a description of when it would
> be appropriate to use this instead of a simpler binding.
Alright. The main point of this binding is that the devices may have
multiple distinct ports that each can be connected to other devices.
So, contrary to the other graph bindings, devices are not simple nodes
in a directed graph. For example a single capture device with two
separate inputs connected to camera sensors:
,--------.
| cam [port]-. ,-----------------.
`--------´ `-[port@0] capture |
,---------. ,-[port@1] interface |
| cam [port]-´ `-----------------´
`---------´
Or two separate display controllers with two parallel outputs each, a
4-port multiplexer, and an encoder (e.g. lvds):
,------------------.
| display [port@0]--. ,-------------.
| controller [port@1]-. `-[port@0] |
`------------------´ `--[port@1] mux |
,------------------. ,--[port@2] | ,-------------.
| display [port@0]-´ ,-[port@3] [port@4]--[port] encoder |
| controller [port@1]--´ `-------------´ `-------------´
`------------------´
Optionally, the same with the multiplexer integrated into the
encoder device:
,------------------.
| display [port@0]--. ,----------------.
| controller [port@1]-. `-[port@0] |
`------------------´ `--[port@1] encoder |
,------------------. ,--[port@2] with mux |
| display [port@0]-´ ,-[port@3] |
| controller [port@1]--´ `----------------´
`------------------´
Or display controller, encoder, and panel:
,--------. ,---------.
| dc [port]--[port@0] |
`--------´ | encoder | ,-----------.
| [port@1]--[port] panel |
`---------´ `-----------´
> > "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.
>
> I would argue that this pattern isn't necessarily restricted to data
> flow descriptions. It wants to describe linkage between devices that are
> sufficiently complex that the simple binding doesn't do the job.
Ok. In principle it would be possible to use the same scheme for other
connections than data links.
> > The device tree graph bindings allow to describe data bus connections between
> > individual devices, that can't 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 can be set 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.
>
> Even if you don't want to declare the direction of data flow, there does
> need to be some guidance as to how the binding is constructed. Does
> device A point to device B? Or the other way around? Why would someone
> choose one over the other? I don't want to see a situation where A & B
> point to each other. Things get complex if the graph is allowed to be
> cyclical.
According to video-interfaces.txt, it is expected that endpoints contain
phandles pointing to the remote endpoint on both sides. I'd like to
leave this up to the more specialized bindings, but I can see that this
makes enumerating the connections starting from each device tree node
easier, for example at probe time.
If the back links are not provided in the device tree, a device at the
receiving end of a remote-endpoint phandle can only know about connected
remote ports after the kernel parsed the whole graph and created the
back links itself.
> > 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.
>
> I really don't like this aspect. It is far too easy to get wrong.
On the other hand it is really easy to test for and warn about missing
or misdirected backlinks.
> Graphs should be one direction only.
But this is not what the current binding in video-interfaces.txt
describes. I don't think it is a good idea to explicitly forbid
backlinks in this binding.
regards
Philipp
On 26/02/14 16:48, Philipp Zabel wrote:
>> I would like the document to acknowledge the difference from the
>> phandle+args pattern used elsewhere and a description of when it would
>> be appropriate to use this instead of a simpler binding.
>
> Alright. The main point of this binding is that the devices may have
> multiple distinct ports that each can be connected to other devices.
The other main point with this binding are multiple endpoints per port.
So you can have, say, a display controller, with single port, which has
two endpoints going to two separate LCD panels.
In physical level that would usually mean that the same pins from the
display controller are connected to two panels. Most likely this would
mean that only one panel can be used at a time, possibly with different
settings (say, 16 RGB pins for one panel, 24 RGB pins for the other).
Tomi