| Message ID | 1393340304-19005-2-git-send-email-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 25 Feb 2014 15:58:22 +0100, Philipp Zabel <p.zabel@pengutronix.de> wrote: > From: Philipp Zabel <philipp.zabel@gmail.com> > > This patch moves the parsing helpers used to parse connected graphs > in the device tree, like the video interface bindings documented in > Documentation/devicetree/bindings/media/video-interfaces.txt, from > drivers/media/v4l2-core to drivers/of. > > This allows to reuse the same parser code from outside the V4L2 > framework, most importantly from display drivers. > The functions v4l2_of_get_next_endpoint, v4l2_of_get_remote_port, > and v4l2_of_get_remote_port_parent are moved. They are renamed to > of_graph_get_next_endpoint, of_graph_get_remote_port, and > of_graph_get_remote_port_parent, respectively. > Since there are not that many current users yet, switch all of > them to the new functions right away. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > Changes since v3: > - Moved back to drivers/of > --- > drivers/media/i2c/adv7343.c | 4 +- > drivers/media/i2c/mt9p031.c | 4 +- > drivers/media/i2c/s5k5baf.c | 3 +- > drivers/media/i2c/tvp514x.c | 3 +- > drivers/media/i2c/tvp7002.c | 3 +- > drivers/media/platform/exynos4-is/fimc-is.c | 6 +- > drivers/media/platform/exynos4-is/media-dev.c | 3 +- > drivers/media/platform/exynos4-is/mipi-csis.c | 3 +- > drivers/media/v4l2-core/v4l2-of.c | 117 ---------------------- > drivers/of/Makefile | 1 + > drivers/of/of_graph.c | 134 ++++++++++++++++++++++++++ Nah. Just put it into drivers/of/base.c. This isn't a separate subsystem and the functions are pretty basic. > include/linux/of_graph.h | 46 +++++++++ > include/media/v4l2-of.h | 25 +---- > 13 files changed, 199 insertions(+), 153 deletions(-) > create mode 100644 drivers/of/of_graph.c > create mode 100644 include/linux/of_graph.h > [...] > diff --git a/drivers/of/of_graph.c b/drivers/of/of_graph.c > new file mode 100644 > index 0000000..267d8f7 > --- /dev/null > +++ b/drivers/of/of_graph.c > @@ -0,0 +1,134 @@ > +/* > + * OF graph binding parsing library > + * > + * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd. > + * Author: Sylwester Nawrocki <s.nawrocki@samsung.com> > + * > + * Copyright (C) 2012 Renesas Electronics Corp. > + * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of version 2 of the GNU General Public License as > + * published by the Free Software Foundation. > + */ > +#include <linux/kernel.h> > +#include <linux/of.h> > +#include <linux/of_graph.h> > +#include <linux/types.h> > + > +/** > + * of_graph_get_next_endpoint() - get next endpoint node > + * @parent: pointer to the parent device node > + * @prev: previous endpoint node, or NULL to get first > + * > + * Return: An 'endpoint' node pointer with refcount incremented. Refcount > + * of the passed @prev node is not decremented, the caller have to use > + * of_node_put() on it when done. > + */ > +struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, > + struct device_node *prev) > +{ > + struct device_node *endpoint; > + struct device_node *port = NULL; > + > + if (!parent) > + return NULL; > + > + if (!prev) { > + struct device_node *node; > + /* > + * It's the first call, we have to find a port subnode > + * within this node or within an optional 'ports' node. > + */ > + node = of_get_child_by_name(parent, "ports"); > + if (node) > + parent = node; > + > + port = of_get_child_by_name(parent, "port"); If you've got a "ports" node, then I would expect every single child to be a port. Should not need the _by_name variant. It seems that this function is merely a helper to get all grandchildren of a node (with some very minor constraints). That could be generalized and simplified. If the function takes the "ports" node as an argument instead of the parent, then there is a greater likelyhood that other code can make use of it... Thinking further. I think the semantics of this whole feature basically boil down to this: #define for_each_grandchild_of_node(parent, child, grandchild) \ for_each_child_of_node(parent, child) \ for_each_child_of_node(child, grandchild) Correct? Or in this specific case: parent = of_get_child_by_name(np, "ports") for_each_grandchild_of_node(parent, child, grandchild) { ... } Finally, looking at the actual patch, is any of this actually needed. All of the users updated by this patch only ever handle a single endpoint. Have I read it correctly? Are there any users supporting multiple endpoints? > + > + if (port) { > + /* Found a port, get an endpoint. */ > + endpoint = of_get_next_child(port, NULL); > + of_node_put(port); > + } else { > + endpoint = NULL; > + } > + > + if (!endpoint) > + pr_err("%s(): no endpoint nodes specified for %s\n", > + __func__, parent->full_name); > + of_node_put(node); If you 'return endpoint' here, then the else block can go down a level. > + } else { > + port = of_get_parent(prev); > + if (!port) > + /* Hm, has someone given us the root node ?... */ > + return NULL; WARN_ONCE(). That's a very definite coding failure if that happens. > + > + /* Avoid dropping prev node refcount to 0. */ > + of_node_get(prev); > + endpoint = of_get_next_child(port, prev); > + if (endpoint) { > + of_node_put(port); > + return endpoint; > + } > + > + /* No more endpoints under this port, try the next one. */ > + do { > + port = of_get_next_child(parent, port); > + if (!port) > + return NULL; > + } while (of_node_cmp(port->name, "port")); > + > + /* Pick up the first endpoint in this port. */ > + endpoint = of_get_next_child(port, NULL); > + of_node_put(port); > + } > + > + return endpoint; > +} > +EXPORT_SYMBOL(of_graph_get_next_endpoint); > + > +/** > + * of_graph_get_remote_port_parent() - get remote port's parent node > + * @node: pointer to a local endpoint device_node > + * > + * Return: Remote device node associated with remote endpoint node linked > + * to @node. Use of_node_put() on it when done. > + */ > +struct device_node *of_graph_get_remote_port_parent( > + const struct device_node *node) > +{ > + struct device_node *np; > + unsigned int depth; > + > + /* Get remote endpoint node. */ > + np = of_parse_phandle(node, "remote-endpoint", 0); > + > + /* Walk 3 levels up only if there is 'ports' node. */ This needs a some explaining. My reading of the binding pattern is that it will always be a fixed number of levels. Why is this test fuzzy? > + for (depth = 3; depth && np; depth--) { > + np = of_get_next_parent(np); > + if (depth == 2 && of_node_cmp(np->name, "ports")) > + break; > + } > + return np; > +} > +EXPORT_SYMBOL(of_graph_get_remote_port_parent); > + > +/** > + * of_graph_get_remote_port() - get remote port node > + * @node: pointer to a local endpoint device_node > + * > + * Return: Remote port node associated with remote endpoint node linked > + * to @node. Use of_node_put() on it when done. > + */ > +struct device_node *of_graph_get_remote_port(const struct device_node *node) > +{ > + struct device_node *np; > + > + /* Get remote endpoint node. */ > + np = of_parse_phandle(node, "remote-endpoint", 0); > + if (!np) > + return NULL; > + return of_get_next_parent(np); > +} > +EXPORT_SYMBOL(of_graph_get_remote_port); > diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h > new file mode 100644 > index 0000000..3bbeb60 > --- /dev/null > +++ b/include/linux/of_graph.h > @@ -0,0 +1,46 @@ > +/* > + * OF graph binding parsing helpers > + * > + * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd. > + * Author: Sylwester Nawrocki <s.nawrocki@samsung.com> > + * > + * Copyright (C) 2012 Renesas Electronics Corp. > + * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of version 2 of the GNU General Public License as > + * published by the Free Software Foundation. > + */ > +#ifndef __LINUX_OF_GRAPH_H > +#define __LINUX_OF_GRAPH_H > + > +#ifdef CONFIG_OF > +struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, > + struct device_node *previous); > +struct device_node *of_graph_get_remote_port_parent( > + const struct device_node *node); > +struct device_node *of_graph_get_remote_port(const struct device_node *node); > +#else > + > +static inline struct device_node *of_graph_get_next_endpoint( > + const struct device_node *parent, > + struct device_node *previous) > +{ > + return NULL; > +} > + > +static inline struct device_node *of_graph_get_remote_port_parent( > + const struct device_node *node) > +{ > + return NULL; > +} > + > +static inline struct device_node *of_graph_get_remote_port( > + const struct device_node *node) > +{ > + return NULL; > +} > + > +#endif /* CONFIG_OF */ > + > +#endif /* __LINUX_OF_GRAPH_H */ > diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h > index 541cea4..3a49735 100644 > --- a/include/media/v4l2-of.h > +++ b/include/media/v4l2-of.h > @@ -17,6 +17,7 @@ > #include <linux/list.h> > #include <linux/types.h> > #include <linux/errno.h> > +#include <linux/of_graph.h> > > #include <media/v4l2-mediabus.h> > > @@ -72,11 +73,6 @@ struct v4l2_of_endpoint { > #ifdef CONFIG_OF > int v4l2_of_parse_endpoint(const struct device_node *node, > struct v4l2_of_endpoint *endpoint); > -struct device_node *v4l2_of_get_next_endpoint(const struct device_node *parent, > - struct device_node *previous); > -struct device_node *v4l2_of_get_remote_port_parent( > - const struct device_node *node); > -struct device_node *v4l2_of_get_remote_port(const struct device_node *node); > #else /* CONFIG_OF */ > > static inline int v4l2_of_parse_endpoint(const struct device_node *node, > @@ -85,25 +81,6 @@ static inline int v4l2_of_parse_endpoint(const struct device_node *node, > return -ENOSYS; > } > > -static inline struct device_node *v4l2_of_get_next_endpoint( > - const struct device_node *parent, > - struct device_node *previous) > -{ > - return NULL; > -} > - > -static inline struct device_node *v4l2_of_get_remote_port_parent( > - const struct device_node *node) > -{ > - return NULL; > -} > - > -static inline struct device_node *v4l2_of_get_remote_port( > - const struct device_node *node) > -{ > - return NULL; > -} > - > #endif /* CONFIG_OF */ > > #endif /* _V4L2_OF_H */ > -- > 1.8.5.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Hi Grant, > > Am Mittwoch, den 26.02.2014, 11:37 +0000 schrieb Grant Likely: > [...] > > > drivers/media/v4l2-core/v4l2-of.c | 117 ---------------------- > > > drivers/of/Makefile | 1 + > > > drivers/of/of_graph.c | 134 ++++++++++++++++++++++++++ > > > > Nah. Just put it into drivers/of/base.c. This isn't a separate subsystem > > and the functions are pretty basic. > > Ok. > > [...] > > > +struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, > > > + struct device_node *prev) > > > +{ > > > + struct device_node *endpoint; > > > + struct device_node *port = NULL; > > > + > > > + if (!parent) > > > + return NULL; > > > + > > > + if (!prev) { > > > + struct device_node *node; > > > + /* > > > + * It's the first call, we have to find a port subnode > > > + * within this node or within an optional 'ports' node. > > > + */ > > > + node = of_get_child_by_name(parent, "ports"); > > > + if (node) > > > + parent = node; > > > + > > > + port = of_get_child_by_name(parent, "port"); > > > > If you've got a "ports" node, then I would expect every single child to > > be a port. Should not need the _by_name variant. > > The 'ports' node is optional. It is only needed if the parent node has > its own #address-cells and #size-cells properties. If the ports are > direct children of the device node, there might be other nodes than > ports: > > device { > #address-cells = <1>; > #size-cells = <0>; > > port@0 { > endpoint { ... }; > }; > port@1 { > endpoint { ... }; > }; > > some-other-child { ... }; > }; > > device { > #address-cells = <x>; > #size-cells = <y>; > > ports { > #address-cells = <1>; > #size-cells = <0>; > > port@0 { > endpoint { ... }; > }; > port@1 { > endpoint { ... }; > }; > }; > > some-other-child { ... }; > }; From a pattern perspective I have no problem with that.... From an individual driver binding perspective that is just dumb! It's fine for the ports node to be optional, but an individual driver using the binding should be explicit about which it will accept. Please use either a flag or a separate wrapper so that the driver can select the behaviour. > The helper should find the two endpoints in both cases. > Tomi suggests an even more compact form for devices with just one port: > > device { > endpoint { ... }; > > some-other-child { ... }; > }; That's fine. In that case the driver would specifically require the endpoint to be that one node.... although the above looks a little weird to me. I would recommend that if there are other non-port child nodes then the ports should still be encapsulated by a ports node. The device binding should not be ambiguous about which nodes are ports. > > It seems that this function is merely a helper to get all grandchildren > > of a node (with some very minor constraints). That could be generalized > > and simplified. If the function takes the "ports" node as an argument > > instead of the parent, then there is a greater likelyhood that other > > code can make use of it... > > > > Thinking further. I think the semantics of this whole feature basically > > boil down to this: > > > > #define for_each_grandchild_of_node(parent, child, grandchild) \ > > for_each_child_of_node(parent, child) \ > > for_each_child_of_node(child, grandchild) > > > > Correct? Or in this specific case: > > > > parent = of_get_child_by_name(np, "ports") > > for_each_grandchild_of_node(parent, child, grandchild) { > > ... > > } > > Hmm, that would indeed be a bit more generic, but it doesn't handle the > optional 'ports' subnode and doesn't allow for other child nodes in the > device node. See above. The no-ports-node version could be the for_each_grandchild_of_node() block, and the yes-ports-node version could be a wrapper around that. > > Finally, looking at the actual patch, is any of this actually needed. > > All of the users updated by this patch only ever handle a single > > endpoint. Have I read it correctly? Are there any users supporting > > multiple endpoints? > > Yes, mainline currently only contains simple cases. I have posted i.MX6 > patches that use this scheme for the output path: > http://www.spinics.net/lists/arm-kernel/msg310817.html > http://www.spinics.net/lists/arm-kernel/msg310821.html Blurg. On a plane right now. Can't go and read those links. > > > + > > > + if (port) { > > > + /* Found a port, get an endpoint. */ > > > + endpoint = of_get_next_child(port, NULL); > > > + of_node_put(port); > > > + } else { > > > + endpoint = NULL; > > > + } > > > + > > > + if (!endpoint) > > > + pr_err("%s(): no endpoint nodes specified for %s\n", > > > + __func__, parent->full_name); > > > + of_node_put(node); > > > > If you 'return endpoint' here, then the else block can go down a level. > > Note that this patch is a straight move of existing code. > I can follow up with code beautification and ... I'm fine with that. g. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/03/14 19:18, Grant Likely wrote: > From a pattern perspective I have no problem with that.... From an > individual driver binding perspective that is just dumb! It's fine for > the ports node to be optional, but an individual driver using the > binding should be explicit about which it will accept. Please use either > a flag or a separate wrapper so that the driver can select the > behaviour. Why is that? The meaning of the DT data stays the same, regardless of the existence of the 'ports' node. The driver uses the graph helpers to parse the port/endpoint data, so individual drivers don't even have to care about the format used. As I see it, the graph helpers should allow the drivers to iterate the ports and the endpoints for a port. These should work the same way, no matter which abbreviated format is used in the dts. >> The helper should find the two endpoints in both cases. >> Tomi suggests an even more compact form for devices with just one port: >> >> device { >> endpoint { ... }; >> >> some-other-child { ... }; >> }; > > That's fine. In that case the driver would specifically require the > endpoint to be that one node.... although the above looks a little weird The driver can't require that. It's up to the board designer to decide how many endpoints are used. A driver may say that it has a single input port. But the number of endpoints for that port is up to the use case. > to me. I would recommend that if there are other non-port child nodes > then the ports should still be encapsulated by a ports node. The device > binding should not be ambiguous about which nodes are ports. Hmm, ambiguous in what way? If the dts uses 'ports' node, all the ports and endpoints are inside that 'ports' node. If there is no 'ports' node, there may be one or more 'port' nodes, which then contain endpoints. If there are no 'port' nodes, there may be a single 'endpoint' node. True, there are many "if"s there. But I don't think it's ambiguous. The reason we have these abbreviations is that the full 'ports' node is not needed that often, and it is rather verbose. In almost all the use cases, panels and connectors can use the single endpoint format. Tomi
Hi Grant, On Fri, Mar 7, 2014 at 6:18 PM, Grant Likely <grant.likely@linaro.org> wrote: > On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel <p.zabel@pengutronix.de> wrote: >> The 'ports' node is optional. It is only needed if the parent node has >> its own #address-cells and #size-cells properties. If the ports are >> direct children of the device node, there might be other nodes than >> ports: >> >> device { >> #address-cells = <1>; >> #size-cells = <0>; >> >> port@0 { >> endpoint { ... }; >> }; >> port@1 { >> endpoint { ... }; >> }; >> >> some-other-child { ... }; >> }; >> >> device { >> #address-cells = <x>; >> #size-cells = <y>; >> >> ports { >> #address-cells = <1>; >> #size-cells = <0>; >> >> port@0 { >> endpoint { ... }; >> }; >> port@1 { >> endpoint { ... }; >> }; >> }; >> >> some-other-child { ... }; >> }; > > From a pattern perspective I have no problem with that.... From an > individual driver binding perspective that is just dumb! It's fine for > the ports node to be optional, but an individual driver using the > binding should be explicit about which it will accept. Please use either > a flag or a separate wrapper so that the driver can select the > behaviour. If the generic binding exists in both forms, most drivers should be able to cope with both. Maybe it should be mentioned in the bindings that the short form without ports node should be used where possible (i.e. for devices that don't already have #address,size-cells != 1,0). Having a separate wrapper to enforce the ports node for devices that need it might be useful. >> The helper should find the two endpoints in both cases. >> Tomi suggests an even more compact form for devices with just one port: >> >> device { >> endpoint { ... }; >> >> some-other-child { ... }; >> }; > > That's fine. In that case the driver would specifically require the > endpoint to be that one node.... although the above looks a little weird > to me. I would recommend that if there are other non-port child nodes > then the ports should still be encapsulated by a ports node. The device > binding should not be ambiguous about which nodes are ports. Sylwester suggested as an alternative, if I understood correctly, to drop the endpoint node and instead keep the port: device-a { implicit_output_ep: port { remote-endpoint = <&explicit_input_ep>; }; }; device-b { port { explicit_input_ep: endpoint { remote-endpoint = <&implicit_output_ep>; }; }; }; This would have the advantage to reduce verbosity for devices with multiple ports that are only connected via one endport each, and you'd always have the connected ports in the device tree as 'port' nodes. >> > It seems that this function is merely a helper to get all grandchildren >> > of a node (with some very minor constraints). That could be generalized >> > and simplified. If the function takes the "ports" node as an argument >> > instead of the parent, then there is a greater likelyhood that other >> > code can make use of it... >> > >> > Thinking further. I think the semantics of this whole feature basically >> > boil down to this: >> > >> > #define for_each_grandchild_of_node(parent, child, grandchild) \ >> > for_each_child_of_node(parent, child) \ >> > for_each_child_of_node(child, grandchild) >> > >> > Correct? Or in this specific case: >> > >> > parent = of_get_child_by_name(np, "ports") >> > for_each_grandchild_of_node(parent, child, grandchild) { >> > ... >> > } >> >> Hmm, that would indeed be a bit more generic, but it doesn't handle the >> optional 'ports' subnode and doesn't allow for other child nodes in the >> device node. > > See above. The no-ports-node version could be the > for_each_grandchild_of_node() block, and the yes-ports-node version > could be a wrapper around that. For the yes-ports-node version I see no problem, but without the ports node, for_each_grandchild_of_node would also collect the children of non-port child nodes. The port and endpoint nodes in this binding are identified by their name, so maybe adding of_get_next_child_by_name() / for_each_named_child_of_node() could be helpful here. >> > Finally, looking at the actual patch, is any of this actually needed. >> > All of the users updated by this patch only ever handle a single >> > endpoint. Have I read it correctly? Are there any users supporting >> > multiple endpoints? >> >> Yes, mainline currently only contains simple cases. I have posted i.MX6 >> patches that use this scheme for the output path: >> http://www.spinics.net/lists/arm-kernel/msg310817.html >> http://www.spinics.net/lists/arm-kernel/msg310821.html > > Blurg. On a plane right now. Can't go and read those links. The patches are merged into the staging tree now at bfe24b9. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 8 Mar 2014 12:46:05 +0200, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > On 07/03/14 19:18, Grant Likely wrote: > > > From a pattern perspective I have no problem with that.... From an > > individual driver binding perspective that is just dumb! It's fine for > > the ports node to be optional, but an individual driver using the > > binding should be explicit about which it will accept. Please use either > > a flag or a separate wrapper so that the driver can select the > > behaviour. > > Why is that? The meaning of the DT data stays the same, regardless of > the existence of the 'ports' node. The driver uses the graph helpers to > parse the port/endpoint data, so individual drivers don't even have to > care about the format used. You don't want to give options to the device tree writer. It should work one way and one way only. Every different combination is a different permutation to get wrong. The only time we should be allowing for more than one way to do it is when there is an existing binding that has proven to be insufficient and it needs to be extended, such as was done with interrupts-extended to deal with deficiencies in the interrupts property. > As I see it, the graph helpers should allow the drivers to iterate the > ports and the endpoints for a port. These should work the same way, no > matter which abbreviated format is used in the dts. > > >> The helper should find the two endpoints in both cases. > >> Tomi suggests an even more compact form for devices with just one port: > >> > >> device { > >> endpoint { ... }; > >> > >> some-other-child { ... }; > >> }; > > > > That's fine. In that case the driver would specifically require the > > endpoint to be that one node.... although the above looks a little weird > > The driver can't require that. It's up to the board designer to decide > how many endpoints are used. A driver may say that it has a single input > port. But the number of endpoints for that port is up to the use case. Come now, when you're writing a driver you know if it will ever be possible to have more than one port. If that is the case then the binding should be specifically laid out for that. If there will never be multiple ports and the binding is unambiguous, then, and only then, should the shortcut be used, and only the shortcut should be accepted. > > to me. I would recommend that if there are other non-port child nodes > > then the ports should still be encapsulated by a ports node. The device > > binding should not be ambiguous about which nodes are ports. > > Hmm, ambiguous in what way? Parsing the binding now consists of a ladder of 'ifs' that gives three distinct different behaviours for no benefit. You don't want that in bindings because it makes it more difficult to get the parsing right in the first place, and to make sure that all users parse it in the same way (Linux, U-Boot, BSD, etc). Bindings should be as absolutely simple as possible. Just to be clear, I have no problem with having the option in the pattern, but the driver needs to be specific about what layout it expects. g. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Grant, On Saturday 08 March 2014 12:23:21 Grant Likely wrote: > On Sat, 8 Mar 2014 12:46:05 +0200, Tomi Valkeinen wrote: > > On 07/03/14 19:18, Grant Likely wrote: > > > From a pattern perspective I have no problem with that.... From an > > > individual driver binding perspective that is just dumb! It's fine for > > > the ports node to be optional, but an individual driver using the > > > binding should be explicit about which it will accept. Please use either > > > a flag or a separate wrapper so that the driver can select the > > > behaviour. > > > > Why is that? The meaning of the DT data stays the same, regardless of > > the existence of the 'ports' node. The driver uses the graph helpers to > > parse the port/endpoint data, so individual drivers don't even have to > > care about the format used. > > You don't want to give options to the device tree writer. It should work > one way and one way only. Every different combination is a different > permutation to get wrong. The only time we should be allowing for more > than one way to do it is when there is an existing binding that has > proven to be insufficient and it needs to be extended, such as was done > with interrupts-extended to deal with deficiencies in the interrupts > property. > > > As I see it, the graph helpers should allow the drivers to iterate the > > ports and the endpoints for a port. These should work the same way, no > > matter which abbreviated format is used in the dts. > > > > >> The helper should find the two endpoints in both cases. > > >> > > >> Tomi suggests an even more compact form for devices with just one port: > > >> device { > > >> > > >> endpoint { ... }; > > >> > > >> some-other-child { ... }; > > >> > > >> }; > > > > > > That's fine. In that case the driver would specifically require the > > > endpoint to be that one node.... although the above looks a little weird > > > > The driver can't require that. It's up to the board designer to decide > > how many endpoints are used. A driver may say that it has a single input > > port. But the number of endpoints for that port is up to the use case. > > Come now, when you're writing a driver you know if it will ever be > possible to have more than one port. If that is the case then the > binding should be specifically laid out for that. If there will never be > multiple ports and the binding is unambiguous, then, and only then, > should the shortcut be used, and only the shortcut should be accepted. Whether multiple nodes are possible for a device is indeed known to the driver author, but the number of endpoints depends on the board. In most cases multiple endpoints are possible. If we decide that the level of simplification should be set in stone in the device DT bindings (i.e. making the ports and/or port nodes required or forbidden, but not optional), then I believe this calls for always having a port node even when a single port is needed. I'm fine with leaving the ports node out, but having no port node might be too close to over-simplification. > > > to me. I would recommend that if there are other non-port child nodes > > > then the ports should still be encapsulated by a ports node. The device > > > binding should not be ambiguous about which nodes are ports. > > > > Hmm, ambiguous in what way? > > Parsing the binding now consists of a ladder of 'ifs' that gives three > distinct different behaviours for no benefit. You don't want that in > bindings because it makes it more difficult to get the parsing right in > the first place, and to make sure that all users parse it in the same > way (Linux, U-Boot, BSD, etc). Bindings should be as absolutely simple > as possible. > > Just to be clear, I have no problem with having the option in the > pattern, but the driver needs to be specific about what layout it > expects.
Hi Philipp, On Saturday 08 March 2014 13:07:23 Philipp Zabel wrote: > On Fri, Mar 7, 2014 at 6:18 PM, Grant Likely wrote: > > On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel wrote: > >> The 'ports' node is optional. It is only needed if the parent node has > >> its own #address-cells and #size-cells properties. If the ports are > >> direct children of the device node, there might be other nodes than > >> > >> ports: > >> device { > >> #address-cells = <1>; > >> #size-cells = <0>; > >> > >> port@0 { > >> endpoint { ... }; > >> }; > >> port@1 { > >> endpoint { ... }; > >> }; > >> > >> some-other-child { ... }; > >> }; > >> > >> device { > >> #address-cells = <x>; > >> #size-cells = <y>; > >> > >> ports { > >> #address-cells = <1>; > >> #size-cells = <0>; > >> > >> port@0 { > >> endpoint { ... }; > >> }; > >> port@1 { > >> endpoint { ... }; > >> }; > >> }; > >> > >> some-other-child { ... }; > >> }; > > > > From a pattern perspective I have no problem with that.... From an > > individual driver binding perspective that is just dumb! It's fine for > > the ports node to be optional, but an individual driver using the > > binding should be explicit about which it will accept. Please use either > > a flag or a separate wrapper so that the driver can select the > > behaviour. > > If the generic binding exists in both forms, most drivers should be > able to cope with both. Maybe it should be mentioned in the bindings > that the short form without ports node should be used where possible > (i.e. for devices that don't already have #address,size-cells != 1,0). > > Having a separate wrapper to enforce the ports node for devices that > need it might be useful. > > >> The helper should find the two endpoints in both cases. > >> > >> Tomi suggests an even more compact form for devices with just one port: > >> device { > >> endpoint { ... }; > >> > >> some-other-child { ... }; > >> }; > > > > That's fine. In that case the driver would specifically require the > > endpoint to be that one node.... although the above looks a little weird > > to me. I would recommend that if there are other non-port child nodes > > then the ports should still be encapsulated by a ports node. The device > > binding should not be ambiguous about which nodes are ports. > > Sylwester suggested as an alternative, if I understood correctly, to > drop the endpoint node and instead keep the port: > > device-a { > implicit_output_ep: port { > remote-endpoint = <&explicit_input_ep>; > }; > }; > > device-b { > port { > explicit_input_ep: endpoint { > remote-endpoint = <&implicit_output_ep>; > }; > }; > }; > > This would have the advantage to reduce verbosity for devices with multiple > ports that are only connected via one endport each, and you'd always have > the connected ports in the device tree as 'port' nodes. I like that idea. I would prefer making the 'port' nodes mandatory and the 'ports' and 'endpoint' nodes optional. Leaving the 'port' node out slightly decreases readability in my opinion, but making the 'endpoint' node optional increases it. That's just my point of view though. > >> > It seems that this function is merely a helper to get all grandchildren > >> > of a node (with some very minor constraints). That could be generalized > >> > and simplified. If the function takes the "ports" node as an argument > >> > instead of the parent, then there is a greater likelyhood that other > >> > code can make use of it... > >> > > >> > Thinking further. I think the semantics of this whole feature basically > >> > boil down to this: > >> > > >> > #define for_each_grandchild_of_node(parent, child, grandchild) \ > >> > > >> > for_each_child_of_node(parent, child) \ > >> > > >> > for_each_child_of_node(child, grandchild) > >> > > >> > Correct? Or in this specific case: > >> > parent = of_get_child_by_name(np, "ports") > >> > for_each_grandchild_of_node(parent, child, grandchild) { > >> > > >> > ... > >> > > >> > } > >> > >> Hmm, that would indeed be a bit more generic, but it doesn't handle the > >> optional 'ports' subnode and doesn't allow for other child nodes in the > >> device node. > > > > See above. The no-ports-node version could be the > > for_each_grandchild_of_node() block, and the yes-ports-node version > > could be a wrapper around that. > > For the yes-ports-node version I see no problem, but without the ports node, > for_each_grandchild_of_node would also collect the children of non-port > child nodes. > The port and endpoint nodes in this binding are identified by their name, > so maybe adding of_get_next_child_by_name() / > for_each_named_child_of_node() could be helpful here. > > >> > Finally, looking at the actual patch, is any of this actually needed. > >> > All of the users updated by this patch only ever handle a single > >> > endpoint. Have I read it correctly? Are there any users supporting > >> > multiple endpoints? > >> > >> Yes, mainline currently only contains simple cases. I have posted i.MX6 > >> > >> patches that use this scheme for the output path: > >> http://www.spinics.net/lists/arm-kernel/msg310817.html > >> http://www.spinics.net/lists/arm-kernel/msg310821.html > > > > Blurg. On a plane right now. Can't go and read those links. > > The patches are merged into the staging tree now at bfe24b9.
On 08/03/14 17:54, Laurent Pinchart wrote: >> Sylwester suggested as an alternative, if I understood correctly, to >> drop the endpoint node and instead keep the port: >> >> device-a { >> implicit_output_ep: port { >> remote-endpoint = <&explicit_input_ep>; >> }; >> }; >> >> device-b { >> port { >> explicit_input_ep: endpoint { >> remote-endpoint = <&implicit_output_ep>; >> }; >> }; >> }; >> >> This would have the advantage to reduce verbosity for devices with multiple >> ports that are only connected via one endport each, and you'd always have >> the connected ports in the device tree as 'port' nodes. > > I like that idea. I would prefer making the 'port' nodes mandatory and the > 'ports' and 'endpoint' nodes optional. Leaving the 'port' node out slightly > decreases readability in my opinion, but making the 'endpoint' node optional > increases it. That's just my point of view though. I, on the other hand, don't like it =). With that format, the remote-endpoint doesn't point to an EP, but a port. And you'll have endpoint's properties in a port node, among the port's properties. Tomi
On 08/03/14 14:23, Grant Likely wrote: >>> That's fine. In that case the driver would specifically require the >>> endpoint to be that one node.... although the above looks a little weird >> >> The driver can't require that. It's up to the board designer to decide >> how many endpoints are used. A driver may say that it has a single input >> port. But the number of endpoints for that port is up to the use case. > > Come now, when you're writing a driver you know if it will ever be > possible to have more than one port. If that is the case then the > binding should be specifically laid out for that. If there will never be > multiple ports and the binding is unambiguous, then, and only then, > should the shortcut be used, and only the shortcut should be accepted. I was talking about endpoints, not ports. There's no unclarity about the number of ports, that comes directly from the hardware for that specific component. The number of endpoints, however, come from the board hardware. The driver writer cannot know that. >>> to me. I would recommend that if there are other non-port child nodes >>> then the ports should still be encapsulated by a ports node. The device >>> binding should not be ambiguous about which nodes are ports. >> >> Hmm, ambiguous in what way? > > Parsing the binding now consists of a ladder of 'ifs' that gives three > distinct different behaviours for no benefit. You don't want that in It considerably lessens the amount of text in the DT for many use cases, making it easier to write and maintain the dts files. > bindings because it makes it more difficult to get the parsing right in > the first place, and to make sure that all users parse it in the same > way (Linux, U-Boot, BSD, etc). Bindings should be as absolutely simple > as possible. Well, yes, I agree there. This is not the simplest of bindings. I'd be more than happy if we would come up with simpler version of this, which would still allow us to have the same descriptive power. > Just to be clear, I have no problem with having the option in the > pattern, but the driver needs to be specific about what layout it > expects. If we forget the shortened endpoint format, I think it can be quite specific. A device has either one port, in which case it should require the 'ports' node to be omitted, or the device has more than one port, in which case it should require 'ports' node. Note that the original v4l2 binding doc says that 'ports' is always optional. Tomi
Hi, On 03/08/2014 04:54 PM, Laurent Pinchart wrote: > Hi Philipp, > > On Saturday 08 March 2014 13:07:23 Philipp Zabel wrote: >> On Fri, Mar 7, 2014 at 6:18 PM, Grant Likely wrote: >>> On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel wrote: >>>> The 'ports' node is optional. It is only needed if the parent node has >>>> its own #address-cells and #size-cells properties. If the ports are >>>> direct children of the device node, there might be other nodes than >>>> >>>> ports: >>>> device { >>>> #address-cells = <1>; >>>> #size-cells = <0>; >>>> >>>> port@0 { >>>> endpoint { ... }; >>>> }; >>>> port@1 { >>>> endpoint { ... }; >>>> }; >>>> >>>> some-other-child { ... }; >>>> }; >>>> >>>> device { >>>> #address-cells = <x>; >>>> #size-cells = <y>; >>>> >>>> ports { >>>> #address-cells = <1>; >>>> #size-cells = <0>; >>>> >>>> port@0 { >>>> endpoint { ... }; >>>> }; >>>> port@1 { >>>> endpoint { ... }; >>>> }; >>>> }; >>>> >>>> some-other-child { ... }; >>>> }; >>> >>> From a pattern perspective I have no problem with that.... From an >>> individual driver binding perspective that is just dumb! It's fine for >>> the ports node to be optional, but an individual driver using the >>> binding should be explicit about which it will accept. Please use either >>> a flag or a separate wrapper so that the driver can select the >>> behaviour. >> >> If the generic binding exists in both forms, most drivers should be >> able to cope with both. Maybe it should be mentioned in the bindings >> that the short form without ports node should be used where possible >> (i.e. for devices that don't already have #address,size-cells != 1,0). >> >> Having a separate wrapper to enforce the ports node for devices that >> need it might be useful. >> >>>> The helper should find the two endpoints in both cases. >>>> >>>> Tomi suggests an even more compact form for devices with just one port: >>>> device { >>>> endpoint { ... }; >>>> >>>> some-other-child { ... }; >>>> }; >>> >>> That's fine. In that case the driver would specifically require the >>> endpoint to be that one node.... although the above looks a little weird >>> to me. I would recommend that if there are other non-port child nodes >>> then the ports should still be encapsulated by a ports node. The device >>> binding should not be ambiguous about which nodes are ports. >> >> Sylwester suggested as an alternative, if I understood correctly, to >> drop the endpoint node and instead keep the port: >> >> device-a { >> implicit_output_ep: port { >> remote-endpoint = <&explicit_input_ep>; >> }; >> }; >> >> device-b { >> port { >> explicit_input_ep: endpoint { >> remote-endpoint = <&implicit_output_ep>; >> }; >> }; >> }; >> >> This would have the advantage to reduce verbosity for devices with multiple >> ports that are only connected via one endport each, and you'd always have >> the connected ports in the device tree as 'port' nodes. > > I like that idea. I would prefer making the 'port' nodes mandatory and the > 'ports' and 'endpoint' nodes optional. Leaving the 'port' node out slightly > decreases readability in my opinion, but making the 'endpoint' node optional > increases it. That's just my point of view though. > I want to propose another solution to simplify bindings, in fact I have few ideas to consider: 1. Use named ports instead of address-cells/regs. Ie instead of port@number schema, use port-function. This will allow to avoid ports node and #address-cells, #size-cells, reg properties. Additionally it should increase readability of the bindings. device { port-dsi { endpoint { ... }; }; port-rgb { endpoint { ... }; }; }; It is little bit like with gpios vs reset-gpios properties. Another advantage I see we do not need do mappings of port numbers to functions between dts, drivers and documentation. 2. Similar approach can be taken to endpoint nodes, in fact as endpoints are children of port node and as I understand port node have no other children we can use any name instead of endpoint@number, of course some convention can be helpful. device { port-dsi { ep-soc1 { ... }; ep-soc2 { ... }; }; port-rgb { ep-panel { ... }; }; }; I would like to add that those ideas would work nicely with Sylwester's proposition of skipping endpoints nodes in case there is only one endpoint - the most common cases are devices with one or two ports, each port having only one remote endpoint. The complete graph for DSI/LVDS bridge I work recently will look like: dsim { dsim_ep: port-dsi { remote-endpoint = <&bridge_dsi_ep>; }; }; bridge { bridge_dsi_ep: port-dsi { remote-endpoint = <&dsim_ep>; }; bridge_lvds_ep: port-lvds { remote-endpoint = <&panel_ep>; }; }; panel { port-lvds { remote-endpoint <&bridge_lvds_ep>; }; }; Regards Andrzej -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/03/14 10:58, Andrzej Hajda wrote: > I want to propose another solution to simplify bindings, in fact I have > few ideas to consider: > > 1. Use named ports instead of address-cells/regs. Ie instead of > port@number schema, use port-function. This will allow to avoid ports > node and #address-cells, #size-cells, reg properties. > Additionally it should increase readability of the bindings. > > device { > port-dsi { > endpoint { ... }; > }; > port-rgb { > endpoint { ... }; > }; > }; > > It is little bit like with gpios vs reset-gpios properties. > Another advantage I see we do not need do mappings of port numbers > to functions between dts, drivers and documentation. That makes it more difficult to iterate the ports. You need to go through all the nodes and use partial name matching. I think for things like gpios, the driver always gives the full name, so there's no need for any kind of partial matching or searching. It looks nice when just looking at the DT, though. Tomi
Hi Andrzej, On Monday 10 March 2014 09:58:07 Andrzej Hajda wrote: > On 03/08/2014 04:54 PM, Laurent Pinchart wrote: > > On Saturday 08 March 2014 13:07:23 Philipp Zabel wrote: > >> On Fri, Mar 7, 2014 at 6:18 PM, Grant Likely wrote: > >>> On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel wrote: > >>>> The 'ports' node is optional. It is only needed if the parent node has > >>>> its own #address-cells and #size-cells properties. If the ports are > >>>> direct children of the device node, there might be other nodes than > >>>> > >>>> ports: > >>>> device { > >>>> #address-cells = <1>; > >>>> #size-cells = <0>; > >>>> > >>>> port@0 { > >>>> endpoint { ... }; > >>>> }; > >>>> port@1 { > >>>> endpoint { ... }; > >>>> }; > >>>> > >>>> some-other-child { ... }; > >>>> }; > >>>> > >>>> device { > >>>> #address-cells = <x>; > >>>> #size-cells = <y>; > >>>> > >>>> ports { > >>>> #address-cells = <1>; > >>>> #size-cells = <0>; > >>>> > >>>> port@0 { > >>>> endpoint { ... }; > >>>> }; > >>>> port@1 { > >>>> endpoint { ... }; > >>>> }; > >>>> }; > >>>> > >>>> some-other-child { ... }; > >>>> }; > >>> > >>> From a pattern perspective I have no problem with that.... From an > >>> individual driver binding perspective that is just dumb! It's fine for > >>> the ports node to be optional, but an individual driver using the > >>> binding should be explicit about which it will accept. Please use either > >>> a flag or a separate wrapper so that the driver can select the > >>> behaviour. > >> > >> If the generic binding exists in both forms, most drivers should be > >> able to cope with both. Maybe it should be mentioned in the bindings > >> that the short form without ports node should be used where possible > >> (i.e. for devices that don't already have #address,size-cells != 1,0). > >> > >> Having a separate wrapper to enforce the ports node for devices that > >> need it might be useful. > >> > >>>> The helper should find the two endpoints in both cases. > >>>> > >>>> Tomi suggests an even more compact form for devices with just one port: > >>>> > >>>> device { > >>>> endpoint { ... }; > >>>> > >>>> some-other-child { ... }; > >>>> }; > >>> > >>> That's fine. In that case the driver would specifically require the > >>> endpoint to be that one node.... although the above looks a little weird > >>> to me. I would recommend that if there are other non-port child nodes > >>> then the ports should still be encapsulated by a ports node. The device > >>> binding should not be ambiguous about which nodes are ports. > >> > >> Sylwester suggested as an alternative, if I understood correctly, to > >> drop the endpoint node and instead keep the port: > >> > >> device-a { > >> implicit_output_ep: port { > >> remote-endpoint = <&explicit_input_ep>; > >> }; > >> }; > >> > >> device-b { > >> port { > >> explicit_input_ep: endpoint { > >> remote-endpoint = <&implicit_output_ep>; > >> }; > >> }; > >> }; > >> > >> This would have the advantage to reduce verbosity for devices with > >> multiple ports that are only connected via one endport each, and you'd > >> always have the connected ports in the device tree as 'port' nodes. > > > > I like that idea. I would prefer making the 'port' nodes mandatory and the > > 'ports' and 'endpoint' nodes optional. Leaving the 'port' node out > > slightly decreases readability in my opinion, but making the 'endpoint' > > node optional increases it. That's just my point of view though. > > I want to propose another solution to simplify bindings, in fact I have > few ideas to consider: > > 1. Use named ports instead of address-cells/regs. Ie instead of > port@number schema, use port-function. This will allow to avoid ports > node and #address-cells, #size-cells, reg properties. > Additionally it should increase readability of the bindings. > > device { > port-dsi { > endpoint { ... }; > }; > port-rgb { > endpoint { ... }; > }; > }; > > It is little bit like with gpios vs reset-gpios properties. > Another advantage I see we do not need do mappings of port numbers > to functions between dts, drivers and documentation. The problem with this approach is that ports are identified by a number inside the kernel, so we would still need to define name to number mappings, or switch to port names internally first. > 2. Similar approach can be taken to endpoint nodes, in fact > as endpoints are children of port node and as I understand port node > have no other children we can use any name instead of endpoint@number, > of course some convention can be helpful. > > device { > port-dsi { > ep-soc1 { ... }; > ep-soc2 { ... }; > }; > port-rgb { > ep-panel { ... }; > }; > }; I see less issues here, as we don't need to number endpoints if I'm not mistaken. > I would like to add that those ideas would work nicely with Sylwester's > proposition of skipping endpoints nodes in case there is only one > endpoint - the most common cases are devices with one or two ports, each > port having only one remote endpoint. > The complete graph for DSI/LVDS bridge I work recently will look like: > > dsim { > dsim_ep: port-dsi { > remote-endpoint = <&bridge_dsi_ep>; > }; > }; > > bridge { > bridge_dsi_ep: port-dsi { > remote-endpoint = <&dsim_ep>; > }; > bridge_lvds_ep: port-lvds { > remote-endpoint = <&panel_ep>; > }; > }; > > panel { > port-lvds { > remote-endpoint <&bridge_lvds_ep>; > }; > };
Hi Tomi, On Monday 10 March 2014 08:00:02 Tomi Valkeinen wrote: > On 08/03/14 17:54, Laurent Pinchart wrote: > >> Sylwester suggested as an alternative, if I understood correctly, to > >> > >> drop the endpoint node and instead keep the port: > >> device-a { > >> implicit_output_ep: port { > >> remote-endpoint = <&explicit_input_ep>; > >> }; > >> }; > >> > >> device-b { > >> port { > >> explicit_input_ep: endpoint { > >> remote-endpoint = <&implicit_output_ep>; > >> }; > >> }; > >> }; > >> > >> This would have the advantage to reduce verbosity for devices with > >> multiple ports that are only connected via one endport each, and you'd > >> always have the connected ports in the device tree as 'port' nodes. > > > > I like that idea. I would prefer making the 'port' nodes mandatory and the > > 'ports' and 'endpoint' nodes optional. Leaving the 'port' node out > > slightly > > decreases readability in my opinion, but making the 'endpoint' node > > optional increases it. That's just my point of view though. > > I, on the other hand, don't like it =). With that format, the > remote-endpoint doesn't point to an EP, but a port. And you'll have > endpoint's properties in a port node, among the port's properties. We'll need to discuss port and endpoint properties separately, but it might make sense to allow endpoints to override port properties instead of specifying the same value explicitly for each endpoint. Endpoint parsing functions would thus look for properties in endpoints first and then in the parent port node if the property can't be found. This would work with implicit endpoints and would be hidden to the drivers. (Please note that this is just food for thought)
On 03/10/2014 12:42 PM, Laurent Pinchart wrote: > Hi Andrzej, > >>> I like that idea. I would prefer making the 'port' nodes mandatory and the >>> 'ports' and 'endpoint' nodes optional. Leaving the 'port' node out >>> slightly decreases readability in my opinion, but making the 'endpoint' >>> node optional increases it. That's just my point of view though. >> >> I want to propose another solution to simplify bindings, in fact I have >> few ideas to consider: >> >> 1. Use named ports instead of address-cells/regs. Ie instead of >> port@number schema, use port-function. This will allow to avoid ports >> node and #address-cells, #size-cells, reg properties. >> Additionally it should increase readability of the bindings. >> >> device { >> port-dsi { >> endpoint { ... }; >> }; >> port-rgb { >> endpoint { ... }; >> }; >> }; >> >> It is little bit like with gpios vs reset-gpios properties. >> Another advantage I see we do not need do mappings of port numbers >> to functions between dts, drivers and documentation. > > The problem with this approach is that ports are identified by a number inside > the kernel, so we would still need to define name to number mappings, or > switch to port names internally first. The mapping will be only internal in the driver. Anyway the bindings should be kernel agnostic. Andrzej > >> 2. Similar approach can be taken to endpoint nodes, in fact >> as endpoints are children of port node and as I understand port node >> have no other children we can use any name instead of endpoint@number, >> of course some convention can be helpful. >> >> device { >> port-dsi { >> ep-soc1 { ... }; >> ep-soc2 { ... }; >> }; >> port-rgb { >> ep-panel { ... }; >> }; >> }; > > I see less issues here, as we don't need to number endpoints if I'm not > mistaken. > >> I would like to add that those ideas would work nicely with Sylwester's >> proposition of skipping endpoints nodes in case there is only one >> endpoint - the most common cases are devices with one or two ports, each >> port having only one remote endpoint. >> The complete graph for DSI/LVDS bridge I work recently will look like: >> >> dsim { >> dsim_ep: port-dsi { >> remote-endpoint = <&bridge_dsi_ep>; >> }; >> }; >> >> bridge { >> bridge_dsi_ep: port-dsi { >> remote-endpoint = <&dsim_ep>; >> }; >> bridge_lvds_ep: port-lvds { >> remote-endpoint = <&panel_ep>; >> }; >> }; >> >> panel { >> port-lvds { >> remote-endpoint <&bridge_lvds_ep>; >> }; >> }; > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 08 Mar 2014 16:50:33 +0100, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Grant, > > On Saturday 08 March 2014 12:23:21 Grant Likely wrote: > > On Sat, 8 Mar 2014 12:46:05 +0200, Tomi Valkeinen wrote: > > > On 07/03/14 19:18, Grant Likely wrote: > > > > From a pattern perspective I have no problem with that.... From an > > > > individual driver binding perspective that is just dumb! It's fine for > > > > the ports node to be optional, but an individual driver using the > > > > binding should be explicit about which it will accept. Please use either > > > > a flag or a separate wrapper so that the driver can select the > > > > behaviour. > > > > > > Why is that? The meaning of the DT data stays the same, regardless of > > > the existence of the 'ports' node. The driver uses the graph helpers to > > > parse the port/endpoint data, so individual drivers don't even have to > > > care about the format used. > > > > You don't want to give options to the device tree writer. It should work > > one way and one way only. Every different combination is a different > > permutation to get wrong. The only time we should be allowing for more > > than one way to do it is when there is an existing binding that has > > proven to be insufficient and it needs to be extended, such as was done > > with interrupts-extended to deal with deficiencies in the interrupts > > property. > > > > > As I see it, the graph helpers should allow the drivers to iterate the > > > ports and the endpoints for a port. These should work the same way, no > > > matter which abbreviated format is used in the dts. > > > > > > >> The helper should find the two endpoints in both cases. > > > >> > > > >> Tomi suggests an even more compact form for devices with just one port: > > > >> device { > > > >> > > > >> endpoint { ... }; > > > >> > > > >> some-other-child { ... }; > > > >> > > > >> }; > > > > > > > > That's fine. In that case the driver would specifically require the > > > > endpoint to be that one node.... although the above looks a little weird > > > > > > The driver can't require that. It's up to the board designer to decide > > > how many endpoints are used. A driver may say that it has a single input > > > port. But the number of endpoints for that port is up to the use case. > > > > Come now, when you're writing a driver you know if it will ever be > > possible to have more than one port. If that is the case then the > > binding should be specifically laid out for that. If there will never be > > multiple ports and the binding is unambiguous, then, and only then, > > should the shortcut be used, and only the shortcut should be accepted. > > Whether multiple nodes are possible for a device is indeed known to the driver > author, but the number of endpoints depends on the board. In most cases > multiple endpoints are possible. If we decide that the level of simplification > should be set in stone in the device DT bindings (i.e. making the ports and/or > port nodes required or forbidden, but not optional), then I believe this calls > for always having a port node even when a single port is needed. I'm fine with > leaving the ports node out, but having no port node might be too close to > over-simplification. Just to make sure we're on the same page (and that I'm parsing correctly): Are you saying the individual 'port' nodes should be required? Or that the container 'ports' node should always be required? If you're saying that the individual port nodes should be required, then yes I agree. I'm still a little uncomfortable about there being a choice between the link directly in the port node or in a child endpoint node, but I'll compromise on this if we put sanity checking into the API (as I replied on the other thread). Whether or not a container 'ports' node is present I think should be defined by the device binding. It really comes down to organization of data. If the device is sufficiently complex that it makes sense to group the ports together (say, because the device has other children with a different purpose), then 'ports' makes absolute sense. g. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 10 Mar 2014 08:34:54 +0200, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > On 08/03/14 14:23, Grant Likely wrote: > > >>> That's fine. In that case the driver would specifically require the > >>> endpoint to be that one node.... although the above looks a little weird > >> > >> The driver can't require that. It's up to the board designer to decide > >> how many endpoints are used. A driver may say that it has a single input > >> port. But the number of endpoints for that port is up to the use case. > > > > Come now, when you're writing a driver you know if it will ever be > > possible to have more than one port. If that is the case then the > > binding should be specifically laid out for that. If there will never be > > multiple ports and the binding is unambiguous, then, and only then, > > should the shortcut be used, and only the shortcut should be accepted. > > I was talking about endpoints, not ports. There's no unclarity about the > number of ports, that comes directly from the hardware for that specific > component. The number of endpoints, however, come from the board > hardware. The driver writer cannot know that. Okay, I understand now. g. > > Just to be clear, I have no problem with having the option in the > > pattern, but the driver needs to be specific about what layout it > > expects. > > If we forget the shortened endpoint format, I think it can be quite > specific. > > A device has either one port, in which case it should require the > 'ports' node to be omitted, or the device has more than one port, in > which case it should require 'ports' node. > > Note that the original v4l2 binding doc says that 'ports' is always > optional. The original v4l2 behaviour doesn't need to change. In fact it should not change if it will cause real-world breakage. g. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Grant, On Thursday 20 March 2014 22:23:47 Grant Likely wrote: > On Sat, 08 Mar 2014 16:50:33 +0100, Laurent Pinchart wrote: > > On Saturday 08 March 2014 12:23:21 Grant Likely wrote: > > > On Sat, 8 Mar 2014 12:46:05 +0200, Tomi Valkeinen wrote: > > > > On 07/03/14 19:18, Grant Likely wrote: > > > > > From a pattern perspective I have no problem with that.... From an > > > > > individual driver binding perspective that is just dumb! It's fine > > > > > for the ports node to be optional, but an individual driver using > > > > > the binding should be explicit about which it will accept. Please > > > > > use either a flag or a separate wrapper so that the driver can > > > > > select the behaviour. > > > > > > > > Why is that? The meaning of the DT data stays the same, regardless of > > > > the existence of the 'ports' node. The driver uses the graph helpers > > > > to parse the port/endpoint data, so individual drivers don't even have > > > > to care about the format used. > > > > > > You don't want to give options to the device tree writer. It should work > > > one way and one way only. Every different combination is a different > > > permutation to get wrong. The only time we should be allowing for more > > > than one way to do it is when there is an existing binding that has > > > proven to be insufficient and it needs to be extended, such as was done > > > with interrupts-extended to deal with deficiencies in the interrupts > > > property. > > > > > > > As I see it, the graph helpers should allow the drivers to iterate the > > > > ports and the endpoints for a port. These should work the same way, no > > > > matter which abbreviated format is used in the dts. > > > > > > > > >> The helper should find the two endpoints in both cases. > > > > >> > > > > >> Tomi suggests an even more compact form for devices with just one > > > > >> port: > > > > >> > > > > >> device { > > > > >> endpoint { ... }; > > > > >> > > > > >> some-other-child { ... }; > > > > >> }; > > > > > > > > > > That's fine. In that case the driver would specifically require the > > > > > endpoint to be that one node.... although the above looks a little > > > > > weird > > > > > > > > The driver can't require that. It's up to the board designer to decide > > > > how many endpoints are used. A driver may say that it has a single > > > > input port. But the number of endpoints for that port is up to the use > > > > case. > > > > > > Come now, when you're writing a driver you know if it will ever be > > > possible to have more than one port. If that is the case then the > > > binding should be specifically laid out for that. If there will never be > > > multiple ports and the binding is unambiguous, then, and only then, > > > should the shortcut be used, and only the shortcut should be accepted. > > > > Whether multiple nodes are possible for a device is indeed known to the > > driver author, but the number of endpoints depends on the board. In most > > cases multiple endpoints are possible. If we decide that the level of > > simplification should be set in stone in the device DT bindings (i.e. > > making the ports and/or port nodes required or forbidden, but not > > optional), then I believe this calls for always having a port node even > > when a single port is needed. I'm fine with leaving the ports node out, > > but having no port node might be too close to over-simplification. > > Just to make sure we're on the same page (and that I'm parsing correctly): > Are you saying the individual 'port' nodes should be required? Or that the > container 'ports' node should always be required? > > If you're saying that the individual port nodes should be required, then > yes I agree. That's what I meant, yes. And I'm glad that we finally agree on something, even if it's a detail :-) > I'm still a little uncomfortable about there being a choice between the link > directly in the port node or in a child endpoint node, but I'll compromise > on this if we put sanity checking into the API (as I replied on the other > thread). > > Whether or not a container 'ports' node is present I think should be defined > by the device binding. It really comes down to organization of data. If the > device is sufficiently complex that it makes sense to group the ports > together (say, because the device has other children with a different > purpose), then 'ports' makes absolute sense. I'm fine with that, as that's what the ports node has been originally designed for. The OF graph bindings documentation could just specify the ports node as optional and mandate individual device bindings to specify it as mandatory or forbidden (possibly with a default behaviour to avoid making all device bindings too verbose).
On Sat, 8 Mar 2014 13:07:23 +0100, Philipp Zabel <philipp.zabel@gmail.com> wrote: > Hi Grant, > > On Fri, Mar 7, 2014 at 6:18 PM, Grant Likely <grant.likely@linaro.org> wrote: > > On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel <p.zabel@pengutronix.de> wrote: > >> The 'ports' node is optional. It is only needed if the parent node has > >> its own #address-cells and #size-cells properties. If the ports are > >> direct children of the device node, there might be other nodes than > >> ports: > >> > >> device { > >> #address-cells = <1>; > >> #size-cells = <0>; > >> > >> port@0 { > >> endpoint { ... }; > >> }; > >> port@1 { > >> endpoint { ... }; > >> }; > >> > >> some-other-child { ... }; > >> }; > >> > >> device { > >> #address-cells = <x>; > >> #size-cells = <y>; > >> > >> ports { > >> #address-cells = <1>; > >> #size-cells = <0>; > >> > >> port@0 { > >> endpoint { ... }; > >> }; > >> port@1 { > >> endpoint { ... }; > >> }; > >> }; > >> > >> some-other-child { ... }; > >> }; > > > > From a pattern perspective I have no problem with that.... From an > > individual driver binding perspective that is just dumb! It's fine for > > the ports node to be optional, but an individual driver using the > > binding should be explicit about which it will accept. Please use either > > a flag or a separate wrapper so that the driver can select the > > behaviour. > > If the generic binding exists in both forms, most drivers should be > able to cope with both. Maybe it should be mentioned in the bindings > that the short form without ports node should be used where possible > (i.e. for devices that don't already have #address,size-cells != 1,0). I would rephrase that: (ie. for devices that have other child nodes that aren't ports.) It isn't about the #address/size-cells values. It is about how the driver interprets child nodes. > > Having a separate wrapper to enforce the ports node for devices that > need it might be useful. Or the other way around. Make the core function only handle an explicit location and use a v4l2 wrapper to preserve the current behaviour. That will encourage stricter usage. > >> The helper should find the two endpoints in both cases. > >> Tomi suggests an even more compact form for devices with just one port: > >> > >> device { > >> endpoint { ... }; > >> > >> some-other-child { ... }; > >> }; > > > > That's fine. In that case the driver would specifically require the > > endpoint to be that one node.... although the above looks a little weird > > to me. I would recommend that if there are other non-port child nodes > > then the ports should still be encapsulated by a ports node. The device > > binding should not be ambiguous about which nodes are ports. > > Sylwester suggested as an alternative, if I understood correctly, to > drop the endpoint node and instead keep the port: > > device-a { > implicit_output_ep: port { > remote-endpoint = <&explicit_input_ep>; > }; > }; > > device-b { > port { > explicit_input_ep: endpoint { > remote-endpoint = <&implicit_output_ep>; > }; > }; > }; > > This would have the advantage to reduce verbosity for devices with multiple > ports that are only connected via one endport each, and you'd always have > the connected ports in the device tree as 'port' nodes. It sounds like that is a closer description of the hardware, so I agree. > > >> > It seems that this function is merely a helper to get all grandchildren > >> > of a node (with some very minor constraints). That could be generalized > >> > and simplified. If the function takes the "ports" node as an argument > >> > instead of the parent, then there is a greater likelyhood that other > >> > code can make use of it... > >> > > >> > Thinking further. I think the semantics of this whole feature basically > >> > boil down to this: > >> > > >> > #define for_each_grandchild_of_node(parent, child, grandchild) \ > >> > for_each_child_of_node(parent, child) \ > >> > for_each_child_of_node(child, grandchild) > >> > > >> > Correct? Or in this specific case: > >> > > >> > parent = of_get_child_by_name(np, "ports") > >> > for_each_grandchild_of_node(parent, child, grandchild) { > >> > ... > >> > } > >> > >> Hmm, that would indeed be a bit more generic, but it doesn't handle the > >> optional 'ports' subnode and doesn't allow for other child nodes in the > >> device node. > > > > See above. The no-ports-node version could be the > > for_each_grandchild_of_node() block, and the yes-ports-node version > > could be a wrapper around that. > > For the yes-ports-node version I see no problem, but without the ports node, > for_each_grandchild_of_node would also collect the children of non-port > child nodes. > The port and endpoint nodes in this binding are identified by their name, > so maybe adding of_get_next_child_by_name() / > for_each_named_child_of_node() could be helpful here. Generally I would avoid mixing child nodes of different purposes. If you are in that situation, the recommendation should be to use a ports node. If there are any current users for which that doesn't work, only then would I do the child_by_name approach. g. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21/03/14 00:32, Laurent Pinchart wrote: > The OF graph bindings documentation could just specify the ports node as > optional and mandate individual device bindings to specify it as mandatory or > forbidden (possibly with a default behaviour to avoid making all device > bindings too verbose). Isn't it so that if the device has one port, it can always do without 'ports', but if it has multiple ports, it always has to use 'ports' so that #address-cells and #size-cells can be defined? If so, there's nothing left for the individual device bindings to decide. Tomi
On 21/03/14 14:37, Tomi Valkeinen wrote: > On 21/03/14 00:32, Laurent Pinchart wrote: > >> > The OF graph bindings documentation could just specify the ports node as >> > optional and mandate individual device bindings to specify it as mandatory or >> > forbidden (possibly with a default behaviour to avoid making all device >> > bindings too verbose). > > Isn't it so that if the device has one port, it can always do without > 'ports', but if it has multiple ports, it always has to use 'ports' so > that #address-cells and #size-cells can be defined? > > If so, there's nothing left for the individual device bindings to decide. Wouldn't it make the bindings even more verbose ? Letting the individual device bindings to decide sounds more sensible to me. -- Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tomi, On Friday 21 March 2014 15:37:17 Tomi Valkeinen wrote: > On 21/03/14 00:32, Laurent Pinchart wrote: > > The OF graph bindings documentation could just specify the ports node as > > optional and mandate individual device bindings to specify it as mandatory > > or forbidden (possibly with a default behaviour to avoid making all > > device bindings too verbose). > > Isn't it so that if the device has one port, it can always do without > 'ports', but if it has multiple ports, it always has to use 'ports' so > that #address-cells and #size-cells can be defined? You can put the #address-cells and #size-cells property in the device node directly without requiring a ports subnode. > If so, there's nothing left for the individual device bindings to decide.
On 21/03/14 16:13, Laurent Pinchart wrote: > Hi Tomi, > > On Friday 21 March 2014 15:37:17 Tomi Valkeinen wrote: >> On 21/03/14 00:32, Laurent Pinchart wrote: >>> The OF graph bindings documentation could just specify the ports node as >>> optional and mandate individual device bindings to specify it as mandatory >>> or forbidden (possibly with a default behaviour to avoid making all >>> device bindings too verbose). >> >> Isn't it so that if the device has one port, it can always do without >> 'ports', but if it has multiple ports, it always has to use 'ports' so >> that #address-cells and #size-cells can be defined? > > You can put the #address-cells and #size-cells property in the device node > directly without requiring a ports subnode. Ah, right. So 'ports' is only needed when the device node has other children nodes than the ports and those nodes need different #address-cells and #size-cells than the ports. In that case it sounds fine to leave it for the driver bindings to decide. Tomi
Hi Tomi, On Friday 21 March 2014 16:22:52 Tomi Valkeinen wrote: > On 21/03/14 16:13, Laurent Pinchart wrote: > > On Friday 21 March 2014 15:37:17 Tomi Valkeinen wrote: > >> On 21/03/14 00:32, Laurent Pinchart wrote: > >>> The OF graph bindings documentation could just specify the ports node as > >>> optional and mandate individual device bindings to specify it as > >>> mandatory or forbidden (possibly with a default behaviour to avoid > >>> making all device bindings too verbose). > >> > >> Isn't it so that if the device has one port, it can always do without > >> 'ports', but if it has multiple ports, it always has to use 'ports' so > >> that #address-cells and #size-cells can be defined? > > > > You can put the #address-cells and #size-cells property in the device node > > directly without requiring a ports subnode. > > Ah, right. So 'ports' is only needed when the device node has other children > nodes than the ports and those nodes need different #address-cells and > #size-cells than the ports. I would rephrase that as the ports node being required only in that case. It can also be useful to cleanly group ports together when the device node has other unrelated children, even though no technical requirement exist (yet ?) in that case. > In that case it sounds fine to leave it for the driver bindings to decide.
diff --git a/drivers/media/i2c/adv7343.c b/drivers/media/i2c/adv7343.c index d4e15a6..9d38f7b 100644 --- a/drivers/media/i2c/adv7343.c +++ b/drivers/media/i2c/adv7343.c @@ -26,12 +26,12 @@ #include <linux/videodev2.h> #include <linux/uaccess.h> #include <linux/of.h> +#include <linux/of_graph.h> #include <media/adv7343.h> #include <media/v4l2-async.h> #include <media/v4l2-device.h> #include <media/v4l2-ctrls.h> -#include <media/v4l2-of.h> #include "adv7343_regs.h" @@ -410,7 +410,7 @@ adv7343_get_pdata(struct i2c_client *client) if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) return client->dev.platform_data; - np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL); + np = of_graph_get_next_endpoint(client->dev.of_node, NULL); if (!np) return NULL; diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c index e5ddf47..192c4aa 100644 --- a/drivers/media/i2c/mt9p031.c +++ b/drivers/media/i2c/mt9p031.c @@ -21,6 +21,7 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/of_gpio.h> +#include <linux/of_graph.h> #include <linux/pm.h> #include <linux/regulator/consumer.h> #include <linux/slab.h> @@ -29,7 +30,6 @@ #include <media/mt9p031.h> #include <media/v4l2-ctrls.h> #include <media/v4l2-device.h> -#include <media/v4l2-of.h> #include <media/v4l2-subdev.h> #include "aptina-pll.h" @@ -943,7 +943,7 @@ mt9p031_get_pdata(struct i2c_client *client) if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) return client->dev.platform_data; - np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL); + np = of_graph_get_next_endpoint(client->dev.of_node, NULL); if (!np) return NULL; diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c index 77e10e0..2d768ef 100644 --- a/drivers/media/i2c/s5k5baf.c +++ b/drivers/media/i2c/s5k5baf.c @@ -21,6 +21,7 @@ #include <linux/media.h> #include <linux/module.h> #include <linux/of_gpio.h> +#include <linux/of_graph.h> #include <linux/regulator/consumer.h> #include <linux/slab.h> @@ -1855,7 +1856,7 @@ static int s5k5baf_parse_device_node(struct s5k5baf *state, struct device *dev) if (ret < 0) return ret; - node_ep = v4l2_of_get_next_endpoint(node, NULL); + node_ep = of_graph_get_next_endpoint(node, NULL); if (!node_ep) { dev_err(dev, "no endpoint defined at node %s\n", node->full_name); diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c index 83d85df..ca00117 100644 --- a/drivers/media/i2c/tvp514x.c +++ b/drivers/media/i2c/tvp514x.c @@ -36,6 +36,7 @@ #include <linux/module.h> #include <linux/v4l2-mediabus.h> #include <linux/of.h> +#include <linux/of_graph.h> #include <media/v4l2-async.h> #include <media/v4l2-device.h> @@ -1068,7 +1069,7 @@ tvp514x_get_pdata(struct i2c_client *client) if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) return client->dev.platform_data; - endpoint = v4l2_of_get_next_endpoint(client->dev.of_node, NULL); + endpoint = of_graph_get_next_endpoint(client->dev.of_node, NULL); if (!endpoint) return NULL; diff --git a/drivers/media/i2c/tvp7002.c b/drivers/media/i2c/tvp7002.c index 912e1cc..c4e1e2c 100644 --- a/drivers/media/i2c/tvp7002.c +++ b/drivers/media/i2c/tvp7002.c @@ -30,6 +30,7 @@ #include <linux/videodev2.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_graph.h> #include <linux/v4l2-dv-timings.h> #include <media/tvp7002.h> #include <media/v4l2-async.h> @@ -957,7 +958,7 @@ tvp7002_get_pdata(struct i2c_client *client) if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) return client->dev.platform_data; - endpoint = v4l2_of_get_next_endpoint(client->dev.of_node, NULL); + endpoint = of_graph_get_next_endpoint(client->dev.of_node, NULL); if (!endpoint) return NULL; diff --git a/drivers/media/platform/exynos4-is/fimc-is.c b/drivers/media/platform/exynos4-is/fimc-is.c index 13a4228..9bdfa45 100644 --- a/drivers/media/platform/exynos4-is/fimc-is.c +++ b/drivers/media/platform/exynos4-is/fimc-is.c @@ -24,13 +24,13 @@ #include <linux/i2c.h> #include <linux/of_irq.h> #include <linux/of_address.h> +#include <linux/of_graph.h> #include <linux/of_platform.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/slab.h> #include <linux/types.h> #include <linux/videodev2.h> -#include <media/v4l2-of.h> #include <media/videobuf2-dma-contig.h> #include "media-dev.h" @@ -167,10 +167,10 @@ static int fimc_is_parse_sensor_config(struct fimc_is_sensor *sensor, u32 tmp = 0; int ret; - np = v4l2_of_get_next_endpoint(np, NULL); + np = of_graph_get_next_endpoint(np, NULL); if (!np) return -ENXIO; - np = v4l2_of_get_remote_port(np); + np = of_graph_get_remote_port(np); if (!np) return -ENXIO; diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c index c1bce17..d0f82da 100644 --- a/drivers/media/platform/exynos4-is/media-dev.c +++ b/drivers/media/platform/exynos4-is/media-dev.c @@ -20,6 +20,7 @@ #include <linux/of.h> #include <linux/of_platform.h> #include <linux/of_device.h> +#include <linux/of_graph.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/types.h> @@ -473,7 +474,7 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd, pd->mux_id = (endpoint.port - 1) & 0x1; - rem = v4l2_of_get_remote_port_parent(ep); + rem = of_graph_get_remote_port_parent(ep); of_node_put(ep); if (rem == NULL) { v4l2_info(&fmd->v4l2_dev, "Remote device at %s not found\n", diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c index f3c3591..fd1ae65 100644 --- a/drivers/media/platform/exynos4-is/mipi-csis.c +++ b/drivers/media/platform/exynos4-is/mipi-csis.c @@ -20,6 +20,7 @@ #include <linux/memory.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_graph.h> #include <linux/phy/phy.h> #include <linux/platform_data/mipi-csis.h> #include <linux/platform_device.h> @@ -762,7 +763,7 @@ static int s5pcsis_parse_dt(struct platform_device *pdev, &state->max_num_lanes)) return -EINVAL; - node = v4l2_of_get_next_endpoint(node, NULL); + node = of_graph_get_next_endpoint(node, NULL); if (!node) { dev_err(&pdev->dev, "No port node at %s\n", pdev->dev.of_node->full_name); diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c index 42e3e8a..f919db3 100644 --- a/drivers/media/v4l2-core/v4l2-of.c +++ b/drivers/media/v4l2-core/v4l2-of.c @@ -152,120 +152,3 @@ int v4l2_of_parse_endpoint(const struct device_node *node, return 0; } EXPORT_SYMBOL(v4l2_of_parse_endpoint); - -/** - * v4l2_of_get_next_endpoint() - get next endpoint node - * @parent: pointer to the parent device node - * @prev: previous endpoint node, or NULL to get first - * - * Return: An 'endpoint' node pointer with refcount incremented. Refcount - * of the passed @prev node is not decremented, the caller have to use - * of_node_put() on it when done. - */ -struct device_node *v4l2_of_get_next_endpoint(const struct device_node *parent, - struct device_node *prev) -{ - struct device_node *endpoint; - struct device_node *port = NULL; - - if (!parent) - return NULL; - - if (!prev) { - struct device_node *node; - /* - * It's the first call, we have to find a port subnode - * within this node or within an optional 'ports' node. - */ - node = of_get_child_by_name(parent, "ports"); - if (node) - parent = node; - - port = of_get_child_by_name(parent, "port"); - - if (port) { - /* Found a port, get an endpoint. */ - endpoint = of_get_next_child(port, NULL); - of_node_put(port); - } else { - endpoint = NULL; - } - - if (!endpoint) - pr_err("%s(): no endpoint nodes specified for %s\n", - __func__, parent->full_name); - of_node_put(node); - } else { - port = of_get_parent(prev); - if (!port) - /* Hm, has someone given us the root node ?... */ - return NULL; - - /* Avoid dropping prev node refcount to 0. */ - of_node_get(prev); - endpoint = of_get_next_child(port, prev); - if (endpoint) { - of_node_put(port); - return endpoint; - } - - /* No more endpoints under this port, try the next one. */ - do { - port = of_get_next_child(parent, port); - if (!port) - return NULL; - } while (of_node_cmp(port->name, "port")); - - /* Pick up the first endpoint in this port. */ - endpoint = of_get_next_child(port, NULL); - of_node_put(port); - } - - return endpoint; -} -EXPORT_SYMBOL(v4l2_of_get_next_endpoint); - -/** - * v4l2_of_get_remote_port_parent() - get remote port's parent node - * @node: pointer to a local endpoint device_node - * - * Return: Remote device node associated with remote endpoint node linked - * to @node. Use of_node_put() on it when done. - */ -struct device_node *v4l2_of_get_remote_port_parent( - const struct device_node *node) -{ - struct device_node *np; - unsigned int depth; - - /* Get remote endpoint node. */ - np = of_parse_phandle(node, "remote-endpoint", 0); - - /* Walk 3 levels up only if there is 'ports' node. */ - for (depth = 3; depth && np; depth--) { - np = of_get_next_parent(np); - if (depth == 2 && of_node_cmp(np->name, "ports")) - break; - } - return np; -} -EXPORT_SYMBOL(v4l2_of_get_remote_port_parent); - -/** - * v4l2_of_get_remote_port() - get remote port node - * @node: pointer to a local endpoint device_node - * - * Return: Remote port node associated with remote endpoint node linked - * to @node. Use of_node_put() on it when done. - */ -struct device_node *v4l2_of_get_remote_port(const struct device_node *node) -{ - struct device_node *np; - - /* Get remote endpoint node. */ - np = of_parse_phandle(node, "remote-endpoint", 0); - if (!np) - return NULL; - return of_get_next_parent(np); -} -EXPORT_SYMBOL(v4l2_of_get_remote_port); diff --git a/drivers/of/Makefile b/drivers/of/Makefile index efd0510..b4a4bc7 100644 --- a/drivers/of/Makefile +++ b/drivers/of/Makefile @@ -9,3 +9,4 @@ obj-$(CONFIG_OF_MDIO) += of_mdio.o obj-$(CONFIG_OF_PCI) += of_pci.o obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o obj-$(CONFIG_OF_MTD) += of_mtd.o +obj-$(CONFIG_OF) += of_graph.o diff --git a/drivers/of/of_graph.c b/drivers/of/of_graph.c new file mode 100644 index 0000000..267d8f7 --- /dev/null +++ b/drivers/of/of_graph.c @@ -0,0 +1,134 @@ +/* + * OF graph binding parsing library + * + * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd. + * Author: Sylwester Nawrocki <s.nawrocki@samsung.com> + * + * Copyright (C) 2012 Renesas Electronics Corp. + * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of version 2 of the GNU General Public License as + * published by the Free Software Foundation. + */ +#include <linux/kernel.h> +#include <linux/of.h> +#include <linux/of_graph.h> +#include <linux/types.h> + +/** + * of_graph_get_next_endpoint() - get next endpoint node + * @parent: pointer to the parent device node + * @prev: previous endpoint node, or NULL to get first + * + * Return: An 'endpoint' node pointer with refcount incremented. Refcount + * of the passed @prev node is not decremented, the caller have to use + * of_node_put() on it when done. + */ +struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, + struct device_node *prev) +{ + struct device_node *endpoint; + struct device_node *port = NULL; + + if (!parent) + return NULL; + + if (!prev) { + struct device_node *node; + /* + * It's the first call, we have to find a port subnode + * within this node or within an optional 'ports' node. + */ + node = of_get_child_by_name(parent, "ports"); + if (node) + parent = node; + + port = of_get_child_by_name(parent, "port"); + + if (port) { + /* Found a port, get an endpoint. */ + endpoint = of_get_next_child(port, NULL); + of_node_put(port); + } else { + endpoint = NULL; + } + + if (!endpoint) + pr_err("%s(): no endpoint nodes specified for %s\n", + __func__, parent->full_name); + of_node_put(node); + } else { + port = of_get_parent(prev); + if (!port) + /* Hm, has someone given us the root node ?... */ + return NULL; + + /* Avoid dropping prev node refcount to 0. */ + of_node_get(prev); + endpoint = of_get_next_child(port, prev); + if (endpoint) { + of_node_put(port); + return endpoint; + } + + /* No more endpoints under this port, try the next one. */ + do { + port = of_get_next_child(parent, port); + if (!port) + return NULL; + } while (of_node_cmp(port->name, "port")); + + /* Pick up the first endpoint in this port. */ + endpoint = of_get_next_child(port, NULL); + of_node_put(port); + } + + return endpoint; +} +EXPORT_SYMBOL(of_graph_get_next_endpoint); + +/** + * of_graph_get_remote_port_parent() - get remote port's parent node + * @node: pointer to a local endpoint device_node + * + * Return: Remote device node associated with remote endpoint node linked + * to @node. Use of_node_put() on it when done. + */ +struct device_node *of_graph_get_remote_port_parent( + const struct device_node *node) +{ + struct device_node *np; + unsigned int depth; + + /* Get remote endpoint node. */ + np = of_parse_phandle(node, "remote-endpoint", 0); + + /* Walk 3 levels up only if there is 'ports' node. */ + for (depth = 3; depth && np; depth--) { + np = of_get_next_parent(np); + if (depth == 2 && of_node_cmp(np->name, "ports")) + break; + } + return np; +} +EXPORT_SYMBOL(of_graph_get_remote_port_parent); + +/** + * of_graph_get_remote_port() - get remote port node + * @node: pointer to a local endpoint device_node + * + * Return: Remote port node associated with remote endpoint node linked + * to @node. Use of_node_put() on it when done. + */ +struct device_node *of_graph_get_remote_port(const struct device_node *node) +{ + struct device_node *np; + + /* Get remote endpoint node. */ + np = of_parse_phandle(node, "remote-endpoint", 0); + if (!np) + return NULL; + return of_get_next_parent(np); +} +EXPORT_SYMBOL(of_graph_get_remote_port); diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h new file mode 100644 index 0000000..3bbeb60 --- /dev/null +++ b/include/linux/of_graph.h @@ -0,0 +1,46 @@ +/* + * OF graph binding parsing helpers + * + * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd. + * Author: Sylwester Nawrocki <s.nawrocki@samsung.com> + * + * Copyright (C) 2012 Renesas Electronics Corp. + * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of version 2 of the GNU General Public License as + * published by the Free Software Foundation. + */ +#ifndef __LINUX_OF_GRAPH_H +#define __LINUX_OF_GRAPH_H + +#ifdef CONFIG_OF +struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, + struct device_node *previous); +struct device_node *of_graph_get_remote_port_parent( + const struct device_node *node); +struct device_node *of_graph_get_remote_port(const struct device_node *node); +#else + +static inline struct device_node *of_graph_get_next_endpoint( + const struct device_node *parent, + struct device_node *previous) +{ + return NULL; +} + +static inline struct device_node *of_graph_get_remote_port_parent( + const struct device_node *node) +{ + return NULL; +} + +static inline struct device_node *of_graph_get_remote_port( + const struct device_node *node) +{ + return NULL; +} + +#endif /* CONFIG_OF */ + +#endif /* __LINUX_OF_GRAPH_H */ diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h index 541cea4..3a49735 100644 --- a/include/media/v4l2-of.h +++ b/include/media/v4l2-of.h @@ -17,6 +17,7 @@ #include <linux/list.h> #include <linux/types.h> #include <linux/errno.h> +#include <linux/of_graph.h> #include <media/v4l2-mediabus.h> @@ -72,11 +73,6 @@ struct v4l2_of_endpoint { #ifdef CONFIG_OF int v4l2_of_parse_endpoint(const struct device_node *node, struct v4l2_of_endpoint *endpoint); -struct device_node *v4l2_of_get_next_endpoint(const struct device_node *parent, - struct device_node *previous); -struct device_node *v4l2_of_get_remote_port_parent( - const struct device_node *node); -struct device_node *v4l2_of_get_remote_port(const struct device_node *node); #else /* CONFIG_OF */ static inline int v4l2_of_parse_endpoint(const struct device_node *node, @@ -85,25 +81,6 @@ static inline int v4l2_of_parse_endpoint(const struct device_node *node, return -ENOSYS; } -static inline struct device_node *v4l2_of_get_next_endpoint( - const struct device_node *parent, - struct device_node *previous) -{ - return NULL; -} - -static inline struct device_node *v4l2_of_get_remote_port_parent( - const struct device_node *node) -{ - return NULL; -} - -static inline struct device_node *v4l2_of_get_remote_port( - const struct device_node *node) -{ - return NULL; -} - #endif /* CONFIG_OF */ #endif /* _V4L2_OF_H */