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

[Grant Likely <grant.likely@linaro.org>]

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.