Message-ID: <1393948056.3917.120.camel@paszta.hi.pengutronix.de>
Date:	Tue, 04 Mar 2014 16:47:36 +0100
From:	Philipp Zabel <p.zabel@...gutronix.de>
To:	Tomi Valkeinen <tomi.valkeinen@...com>
Cc:	Grant Likely <grant.likely@...aro.org>,
	Mauro Carvalho Chehab <m.chehab@...sung.com>,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	Rob Herring <robh+dt@...nel.org>,
	Sylwester Nawrocki <s.nawrocki@...sung.com>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Guennadi Liakhovetski <g.liakhovetski@....de>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [PATCH v5 5/7] [media] of: move common endpoint parsing to
 drivers/of

Am Dienstag, den 04.03.2014, 14:21 +0200 schrieb Tomi Valkeinen:
> On 04/03/14 13:36, Philipp Zabel wrote:
[...]
> >> 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?

I am not sure. We can still correctly parse the endpoint properties of a
parentless node. All current users ignore the return value anyway. So as
long as we still do the memset and and set local_node and id, returning
an error effectively won't change the current behaviour.

[...]
> > 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.

Indeed. I see no problem in adding an of_graph_get_next_port function.
But I'd like to keep the current of_graph_get_next_endpoint function
iterating over all endpoints of the device.

> >> 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.

And I fear that this series might outgrow maintainer attention spans if
I keep adding to 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 =).

Thanks. How about I follow this up with a split port/endpoint parsing
helpers after I get some acks?

> > 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".

I'd prefer this to be a generic function of_get_next_child_by_name and
possibly a macro for_each_named_child_of_node wrapping that.

[...]
> 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.

The main difference I see is that a single IPU device will have two port
nodes handled by the DRM driver and two port nodes handled by the V4L2
driver, so we can't go back up to the IPU device tree node and iterate
over all its ports in either the DRM or V4L2 drivers.
You could argue that all the device tree parsing should be done from the
IPU drivers, and the DRM and V4L2 drivers should use preparsed internal
graph structures. But then we are getting into using struct media_entity
in DRM drivers territory, and rather not go there right now.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists