Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
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 signature.asc Description: OpenPGP digital signature
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
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. -- Regards, Laurent Pinchart signature.asc Description: This is a digitally signed message part.
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
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 signature.asc Description: OpenPGP digital signature
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
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. -- Regards, Laurent Pinchart signature.asc Description: This is a digitally signed message part.
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
On Mon, 10 Mar 2014 08:34:54 +0200, Tomi Valkeinen tomi.valkei...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
On Sat, 08 Mar 2014 16:50:33 +0100, Laurent Pinchart
laurent.pinch...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
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).
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
On Sat, 8 Mar 2014 13:07:23 +0100, Philipp Zabel philipp.za...@gmail.com
wrote:
Hi Grant,
On Fri, Mar 7, 2014 at 6:18 PM, Grant Likely grant.lik...@linaro.org wrote:
On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel p.za...@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
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
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 signature.asc Description: OpenPGP digital signature
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
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 {
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
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
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
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 { ...
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
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)
--
Regards,
Laurent Pinchart
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
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 ifs 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
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
Hi Grant,
On Fri, Mar 7, 2014 at 6:18 PM, Grant Likely grant.lik...@linaro.org wrote:
On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel p.za...@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
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
On Sat, 8 Mar 2014 12:46:05 +0200, Tomi Valkeinen tomi.valkei...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
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.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
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
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
On Wed, 26 Feb 2014 16:24:57 +0100, Philipp Zabel p.za...@pengutronix.de
wrote:
Hi Grant,
Am Mittwoch, den 26.02.2014, 11:37 + 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. */
+
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
On Tue, 25 Feb 2014 15:58:22 +0100, Philipp Zabel p.za...@pengutronix.de
wrote:
From: Philipp Zabel philipp.za...@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.za...@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 000..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.nawro...@samsung.com
+ *
+ * Copyright (C) 2012 Renesas Electronics Corp.
+ * Author: Guennadi Liakhovetski g.liakhovet...@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
Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
Hi Grant,
Am Mittwoch, den 26.02.2014, 11:37 + 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
[PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
From: Philipp Zabel philipp.za...@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.za...@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 ++
include/linux/of_graph.h | 46 +
include/media/v4l2-of.h | 25 +
13 files changed, 199 insertions(+), 153 deletions(-)
create mode 100644 drivers/of/of_graph.c
create mode 100644 include/linux/of_graph.h
diff --git a/drivers/media/i2c/adv7343.c b/drivers/media/i2c/adv7343.c
index d4e15a6..9d38f7b 100644
--- a/drivers/media/i2c/adv7343.c
+++ b/drivers/media/i2c/adv7343.c
@@ -26,12 +26,12 @@
#include linux/videodev2.h
#include linux/uaccess.h
#include linux/of.h
+#include linux/of_graph.h
#include media/adv7343.h
#include media/v4l2-async.h
#include media/v4l2-device.h
#include media/v4l2-ctrls.h
-#include media/v4l2-of.h
#include adv7343_regs.h
@@ -410,7 +410,7 @@ adv7343_get_pdata(struct i2c_client *client)
if (!IS_ENABLED(CONFIG_OF) || !client-dev.of_node)
return client-dev.platform_data;
- np = v4l2_of_get_next_endpoint(client-dev.of_node, NULL);
+ np = of_graph_get_next_endpoint(client-dev.of_node, NULL);
if (!np)
return NULL;
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index e5ddf47..192c4aa 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -21,6 +21,7 @@
#include linux/module.h
#include linux/of.h
#include linux/of_gpio.h
+#include linux/of_graph.h
#include linux/pm.h
#include linux/regulator/consumer.h
#include linux/slab.h
@@ -29,7 +30,6 @@
#include media/mt9p031.h
#include media/v4l2-ctrls.h
#include media/v4l2-device.h
-#include media/v4l2-of.h
#include media/v4l2-subdev.h
#include aptina-pll.h
@@ -943,7 +943,7 @@ mt9p031_get_pdata(struct i2c_client *client)
if (!IS_ENABLED(CONFIG_OF) || !client-dev.of_node)
return client-dev.platform_data;
- np = v4l2_of_get_next_endpoint(client-dev.of_node, NULL);
+ np = of_graph_get_next_endpoint(client-dev.of_node, NULL);
if (!np)
return NULL;
diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
index 77e10e0..2d768ef 100644
--- a/drivers/media/i2c/s5k5baf.c
+++ b/drivers/media/i2c/s5k5baf.c
@@ -21,6 +21,7 @@
#include linux/media.h
#include linux/module.h
#include linux/of_gpio.h
+#include linux/of_graph.h
#include linux/regulator/consumer.h
#include linux/slab.h
@@ -1855,7 +1856,7 @@ static int s5k5baf_parse_device_node(struct s5k5baf
*state, struct device *dev)
if (ret 0)
return ret;
- node_ep = v4l2_of_get_next_endpoint(node, NULL);
+ node_ep = of_graph_get_next_endpoint(node, NULL);
if (!node_ep) {
dev_err(dev, no endpoint defined at node %s\n,
node-full_name);
diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
index 83d85df..ca00117 100644
--- a/drivers/media/i2c/tvp514x.c
+++ b/drivers/media/i2c/tvp514x.c
@@ -36,6 +36,7 @@
#include linux/module.h
#include linux/v4l2-mediabus.h
#include linux/of.h
+#include linux/of_graph.h
#include media/v4l2-async.h
#include media/v4l2-device.h
@@ -1068,7 +1069,7 @@ tvp514x_get_pdata(struct i2c_client *client)
if (!IS_ENABLED(CONFIG_OF) || !client-dev.of_node)
return client-dev.platform_data;
- endpoint = v4l2_of_get_next_endpoint(client-dev.of_node, NULL);
+ endpoint = of_graph_get_next_endpoint(client-dev.of_node, NULL);
if (!endpoint)
return NULL;
diff --git
