On Tue, 11 Mar 2014 16:07:00 +0100, Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> wrote: > Hi Grant, > > Am Montag, den 10.03.2014, 14:58 +0000 schrieb Grant Likely: > > On Mon, 10 Mar 2014 14:52:53 +0100, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > > On Monday 10 March 2014 12:18:20 Tomi Valkeinen wrote: > > > > On 08/03/14 13:41, Grant Likely wrote: > > > > >> Ok. If we go for single directional link, the question is then: which > > > > >> way? And is the direction different for display and camera, which are > > > > >> kind of reflections of each other? > > > > > > > > > > In general I would recommend choosing whichever device you would > > > > > sensibly think of as a master. In the camera case I would choose the > > > > > camera controller node instead of the camera itself, and in the display > > > > > case I would choose the display controller instead of the panel. The > > > > > binding author needs to choose what she things makes the most sense, but > > > > > drivers can still use if it it turns out to be 'backwards' > > > > > > > > I would perhaps choose the same approach, but at the same time I think > > > > it's all but clear. The display controller doesn't control the panel any > > > > more than a DMA controller controls, say, the display controller. > > > > > > > > In fact, in earlier versions of OMAP DSS DT support I had a simpler port > > > > description, and in that I had the panel as the master (i.e. link from > > > > panel to dispc) because the panel driver uses the display controller's > > > > features to provide the panel device a data stream. > > > > > > > > And even with the current OMAP DSS DT version, which uses the v4l2 style > > > > ports/endpoints, the driver model is still the same, and only links > > > > towards upstream are used. > > > > > > > > So one reason I'm happy with the dual-linking is that I can easily > > > > follow the links from the downstream entities to upstream entities, and > > > > other people, who have different driver model, can easily do the opposite. > > > > > > > > But I agree that single-linking is enough and this can be handled at > > > > runtime, even if it makes the code more complex. And perhaps requires > > > > extra data in the dts, to give the start points for the graph. > > > > > > In theory unidirectional links in DT are indeed enough. However, let's not > > > forget the following. > > > > > > - There's no such thing as single start points for graphs. Sure, in some > > > simple cases the graph will have a single start point, but that's not a > > > generic rule. For instance the camera graphs > > > http://ideasonboard.org/media/omap3isp.ps and > > > http://ideasonboard.org/media/eyecam.ps have two camera sensors, and thus two > > > starting points from a data flow point of view. And if you want a better > > > understanding of how complex media graphs can become, have a look at > > > http://ideasonboard.org/media/vsp1.0.pdf (that's a real world example, albeit > > > all connections are internal to the SoC in that particular case, and don't > > > need to be described in DT). > > > > > > - There's also no such thing as a master device that can just point to slave > > > devices. Once again simple cases exist where that model could work, but real > > > world examples exist of complex pipelines with dozens of elements all > > > implemented by a separate IP core and handled by separate drivers, forming a > > > graph with long chains and branches. We thus need real graph bindings. > > > > > > - Finally, having no backlinks in DT would make the software implementation > > > very complex. We need to be able to walk the graph in a generic way without > > > having any of the IP core drivers loaded, and without any specific starting > > > point. We would thus need to parse the complete DT tree, looking at all nodes > > > and trying to find out whether they're part of the graph we're trying to walk. > > > The complexity of the operation would be at best quadratic to the number of > > > nodes in the whole DT and to the number of nodes in the graph. > > > > Not really. To being with, you cannot determine any meaning of a node > > across the tree (aside from it being an endpoint) without also > > understanding the binding that the node is a part of. That means you > > need to have something matching against the compatible string on both > > ends of the linkage. For instance: > > > > panel { > > compatible = "acme,lvds-panel"; > > lvds-port: port { > > }; > > }; > > > > display-controller { > > compatible = "encom,video"; > > port { > > remote-endpoint = <&lvds-port>; > > }; > > }; > > > > In the above example, the encom,video driver has absolutely zero > > information about what the acme,lvds-panel binding actually implements. > > There needs to be both a driver for the "acme,lvds-panel" binding and > > one for the "encom,video" binding (even if the acme,lvds-panel binding > > is very thin and defers the functionality to the video controller). > > > > What you want here is the drivers to register each side of the > > connection. That could be modeled with something like the following > > (pseudocode): > > > > struct of_endpoint { > > struct list_head list; > > struct device_node *ep_node; > > void *context; > > void (*cb)(struct of_endpoint *ep, void *data); > > } > > > > int of_register_port(struct device *node, void (*cb)(struct of_endpoint *ep, void *data), void *data) > > { > > struct of_endpoint *ep = kzalloc(sizeof(*ep), GFP_KERNEL); > > > > ep->ep_node = node; > > ep->data = data; > > ep->callback = cb; > > > > /* store the endpoint to a list */ > > /* check if the endpoint has a remote-endpoint link */ > > /* If so, then link the two together and call the > > * callbacks */ > > } > > > > That's neither expensive or complicated. > > > > Originally I suggested walking the whole tree multiple times, but as > > mentioned that doesn't scale, and as I thought about the above it isn't > > even a valid thing to do. Everything has to be driven by drivers, so > > even if the backlinks are there, nothing can be done with the link until > > the other side goes through enumeration independently. > > I have implemented your suggestion as follows. Basically, this allows > either endpoint to contain the remote-endpoint link, as long as all > drivers register their endpoints in the probe function and return > -EPROBE_DEFER from their component framework bind callback until all > their endpoints are connected. This looks reasonable. g. > > From fdda1fb2bd133200d4620adcbb28697cb360e1cb Mon Sep 17 00:00:00 2001 > From: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> > Date: Tue, 11 Mar 2014 15:56:18 +0100 > Subject: [PATCH] of: Implement of_graph_register_endpoint > > This patch adds a function that lets drivers register their endpoints in a > global list. Newly registered endpoints are compared against known endpoints > to check if a connection should be made. If so, the driver is notified via > a simple callback. > > Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> > --- > drivers/of/base.c | 69 +++++++++++++++++++++++++++++++++++++++++++++--- > include/linux/of_graph.h | 20 +++++++++++++- > 2 files changed, 84 insertions(+), 5 deletions(-) > > diff --git a/drivers/of/base.c b/drivers/of/base.c > index ebb001a..77ae54a 100644 > --- a/drivers/of/base.c > +++ b/drivers/of/base.c > @@ -29,6 +29,7 @@ > #include "of_private.h" > > LIST_HEAD(aliases_lookup); > +LIST_HEAD(endpoint_list); > > struct device_node *of_allnodes; > EXPORT_SYMBOL(of_allnodes); > @@ -2002,6 +2003,7 @@ int of_graph_parse_endpoint(const struct device_node *node, > memset(endpoint, 0, sizeof(*endpoint)); > > endpoint->local_node = node; > + endpoint->remote_node = of_parse_phandle(node, "remote-endpoint", 0); > /* > * It doesn't matter whether the two calls below succeed. > * If they don't then the default value 0 is used. > @@ -2126,6 +2128,19 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, > } > EXPORT_SYMBOL(of_graph_get_next_endpoint); > > +static struct of_endpoint *__of_graph_lookup_endpoint( > + const struct device_node *node) > +{ > + struct of_endpoint *ep; > + > + list_for_each_entry(ep, &endpoint_list, list) { > + if (ep->local_node == node) > + return ep; > + } > + > + return NULL; > +} > + > /** > * of_graph_get_remote_port_parent() - get remote port's parent node > * @node: pointer to a local endpoint device_node > @@ -2136,11 +2151,15 @@ EXPORT_SYMBOL(of_graph_get_next_endpoint); > struct device_node *of_graph_get_remote_port_parent( > const struct device_node *node) > { > + struct of_endpoint *ep; > struct device_node *np; > unsigned int depth; > > /* Get remote endpoint node. */ > - np = of_parse_phandle(node, "remote-endpoint", 0); > + ep = __of_graph_lookup_endpoint(node); > + if (!ep || !ep->remote_node) > + return NULL; > + np = ep->remote_node; > > /* Walk 3 levels up only if there is 'ports' node */ > for (depth = 3; depth && np; depth--) { > @@ -2163,13 +2182,14 @@ EXPORT_SYMBOL(of_graph_get_remote_port_parent); > */ > struct device_node *of_graph_get_remote_port(const struct device_node *node) > { > + struct of_endpoint *ep; > struct device_node *np; > > /* Get remote endpoint node. */ > - np = of_parse_phandle(node, "remote-endpoint", 0); > - if (!np) > + ep = __of_graph_lookup_endpoint(node); > + if (!ep || !ep->remote_node) > return NULL; > - np = of_get_next_parent(np); > + np = of_get_next_parent(ep->remote_node); > if (of_node_cmp(np->name, "port")) { > of_node_put(np); > return NULL; > @@ -2177,3 +2197,44 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node) > return np; > } > EXPORT_SYMBOL(of_graph_get_remote_port); > + > +int of_graph_register_endpoint(const struct device_node *node, > + void (*cb)(struct of_endpoint *ep, void *data), void *data) > +{ > + struct of_endpoint *remote_ep, *ep = kmalloc(sizeof(*ep), GFP_KERNEL); > + if (!ep) > + return -ENOMEM; > + > + of_graph_parse_endpoint(node, ep); > + ep->callback = cb; > + ep->data = data; > + > + list_add(&ep->list, &endpoint_list); > + > + list_for_each_entry(remote_ep, &endpoint_list, list) { > + struct of_endpoint *from, *to; > + if (ep->remote_node) { > + from = ep; > + to = remote_ep; > + } else { > + from = remote_ep; > + to = ep; > + } > + if (from->remote_node && > + from->remote_node == to->local_node) { > + WARN_ON(to->remote_node && > + to->remote_node != from->local_node); > + to->remote_node = from->local_node; > + to->remote_ep = from; > + from->remote_ep = to; > + if (from->callback) > + from->callback(from, from->data); > + if (to->callback) > + to->callback(to, to->data); > + return 0; > + } > + } > + > + return 0; > +} > +EXPORT_SYMBOL(of_graph_register_endpoint); > diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h > index 3a3c5a9..f00ac4e 100644 > --- a/include/linux/of_graph.h > +++ b/include/linux/of_graph.h > @@ -23,7 +23,14 @@ > struct of_endpoint { > unsigned int port; > unsigned int id; > - const struct device_node *local_node; > + struct device_node *local_node; > + struct device_node *remote_node; > + struct of_endpoint *remote_ep; > + > + /* Internal use only */ > + struct list_head list; > + void (*callback)(struct of_endpoint *ep, void *data); > + void *data; > }; > > #ifdef CONFIG_OF > @@ -35,6 +42,10 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, > 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); > + > +int of_graph_register_endpoint(const struct device_node *ep_node, > + void (*cb)(struct of_endpoint *ep, void *data), > + void *data); > #else > > static inline int of_graph_parse_endpoint(const struct device_node *node, > @@ -68,6 +79,13 @@ static inline struct device_node *of_graph_get_remote_port( > return NULL; > } > > +static inline int of_graph_register_endpoint(const struct device_node *ep_node, > + void (*cb)(struct of_endpoint *ep, void *data), > + void *data); > +{ > + return -ENOSYS; > +} > + > #endif /* CONFIG_OF */ > > #endif /* __LINUX_OF_GRAPH_H */ > -- > 1.9.0 > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
