Re: [PATCH v5 5/7] [media] of: move common endpoint parsing to drivers/of

[Tomi Valkeinen]

On 04/03/14 13:36, Philipp Zabel wrote:
> Hi Tomi,
> 
> Am Dienstag, den 04.03.2014, 10:58 +0200 schrieb Tomi Valkeinen:
> [...]
>>> +int of_graph_parse_endpoint(const struct device_node *node,
>>> +			    struct of_endpoint *endpoint)
>>> +{
>>> +	struct device_node *port_node = of_get_parent(node);
>>
>> Can port_node be NULL? Probably only if something is quite wrong, but
>> maybe it's safer to return error in that case.
> 
> both of_property_read_u32 and of_node_put can handle port_node == NULL.
> I'll add a WARN_ONCE here as for of_graph_get_next_endpoint and continue
> on.

Isn't it better to return an error?

>> And generally speaking, if struct of_endpoint is needed, maybe it would
>> be better to return the struct of_endpoint when iterating the ports and
>> endpoints. That way there's no need to do parsing "afterwards", trying
>> to figure out if there's a parent port node, but the information is
>> already at hand.
> 
> I'd like to keep the iteration separate from parsing so we can
> eventually introduce a for_each_endpoint_of_node helper macro around
> of_graph_get_next_endpoint.
> 
> [...]
>> A few thoughts about the iteration, and the API in general.
>>
>> In the omapdss version I separated iterating ports and endpoints, for
>> the two reasons:
>>
>> 1) I think there are cases where you may want to have properties in the
>> port node, for things that are common for all the port's endpoints.
>>
>> 2) if there are multiple ports, I think the driver code is cleaner if
>> you first take the port, decide what port that is and maybe call a
>> sub-function, and then iterate the endpoints for that port only.
> 
> It depends a bit on whether you are actually iterating over individual
> ports, or if you are just walking the whole endpoint graph to find
> remote devices that have to be added to the component master's waiting
> list, for example.

True, but the latter is easily implemented using the separate
port/endpoint iteration. So I see it as a more powerful API.

>> Both of those are possible with the API in the series, but not very cleanly.
>>
>> Also, if you just want to iterate the endpoints, it's easy to implement
>> a helper using the separate port and endpoint iterations.
> 
> I started out to move an existing (albeit lightly used) API to a common
> place so others can use it and improve upon it, too. I'm happy to pile
> on fixes directly in this series, but could we separate the improvement
> step from the move, for the bigger modifications?

Yes, I understand that. What I wonder is that which is easier: make it a
public API now, more or less as it was in v4l2, or make it a public API
only when all the improvements we can think of have been made.

So my fear is that the API is now made public, and you and others start
to use it. But I can't use it, as I need things like separate
port/endpoint iteration. I need to add those, which also means that I
need to change all the users of the API, making the task more difficult
than I'd like.

However, this is more of "thinking out loud" than "I don't like the
series". It's a good series =).

> I had no immediate use for the port iteration, so I have taken no steps
> to add a function for this. I see no problem to add this later when
> somebody needs it, or even rewrite of_graph_get_next_endpoint to use it
> if it is feasible. Iterating over endpoints on a given port needs no
> helper, as you can just use for_each_child_of_node.

I would have a helper, which should do some sanity checks, like that the
node names are "endpoint".

>> Then, about the get_remote functions: I think there should be only one
>> function for that purpose, one that returns the device node that
>> contains the remote endpoint.
>>
>> My reasoning is that the ports and endpoints, and their contents, should
>> be private to the device. So the only use to get the remote is to get
>> the actual device, to see if it's been probed, or maybe get some video
>> API for that device.
> 
> of_graph_get_remote_port currently is used in the exynos4-is/fimc-is.c
> v4l2 driver to get the mipi-csi channel id from the remote port, and
> I've started using it in imx-drm-core.c for two cases:
> - given an endpoint on the encoder, find the remote port connected to
>   it, get the associated drm_crtc, to obtain its the drm_crtc_mask
>   for encoder->possible_crtcs.
> - given an encoder and a connected drm_crtc, walk all endpoints to find
>   the remote port associated with the drm_crtc, and then use the local
>   endpoint parent port to determine multiplexer settings.

Ok.

In omapdss each driver handles only the ports and endpoints defined for
its device, and they can be considered private to that device. The only
reason to look for the remote endpoint is to find the remote device. To
me the omapdss model makes sense, and feels logical and sane =). So I
have to say I'm not really familiar with the model you're using.

Of course, the different get_remove_* funcs do no harm, even if we have
a bunch of them. My point was only about enforcing the correct use of
the model, where "correct" is of course subjective =).

>> If the driver model used has some kind of master-driver, which goes
>> through all the display entities, I think the above is still valid. When
>> the master-driver follows the remote-link, it still needs to first get
>> the main device node, as the ports and endpoints make no sense without
>> the context of the main device node.
> 
> I'm not sure about this. I might just need the remote port node
> associated with a remote drm_crtc or drm_encoder structure to find out
> which local endpoint I should look at to retrieve configuration.

Ok. I guess if you have a fixed model for the video pipeline elements it
works out. For omapdss we don't know what's there on the remote side, as
we can have arbitrary amount of video pipeline elements.

 Tomi

Attachment: signature.asc
Description: OpenPGP digital signature