Message-ID: <531595AB.4000001@ti.com>
Date:	Tue, 4 Mar 2014 10:58:19 +0200
From:	Tomi Valkeinen <tomi.valkeinen@...com>
To:	Philipp Zabel <p.zabel@...gutronix.de>
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

Hi Philipp,

On 27/02/14 19:35, Philipp Zabel wrote:
> This patch adds a new struct of_endpoint which is then embedded in struct
> v4l2_of_endpoint and contains the endpoint properties that are not V4L2
> (or even media) specific: the port number, endpoint id, local device tree
> node and remote endpoint phandle. of_graph_parse_endpoint parses those
> properties and is used by v4l2_of_parse_endpoint, which just adds the
> V4L2 MBUS information to the containing v4l2_of_endpoint structure.

<snip>

> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 8ecca7a..ba3cfca 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1985,6 +1985,37 @@ struct device_node *of_find_next_cache_node(const struct device_node *np)
>  }
>  
>  /**
> + * of_graph_parse_endpoint() - parse common endpoint node properties
> + * @node: pointer to endpoint device_node
> + * @endpoint: pointer to the OF endpoint data structure
> + *
> + * All properties are optional. If none are found, we don't set any flags.
> + * This means the port has a static configuration and no properties have
> + * to be specified explicitly.
> + * The caller should hold a reference to @node.
> + */
> +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.

> +	memset(endpoint, 0, sizeof(*endpoint));
> +
> +	endpoint->local_node = node;
> +	/*
> +	 * It doesn't matter whether the two calls below succeed.
> +	 * If they don't then the default value 0 is used.
> +	 */
> +	of_property_read_u32(port_node, "reg", &endpoint->port);
> +	of_property_read_u32(node, "reg", &endpoint->id);

If the endpoint does not have 'port' as parent (i.e. the shortened
format), the above will return the 'reg' of the device node (with
'device node' I mean the main node, with 'compatible' property).

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.

> +
> +	of_node_put(port_node);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(of_graph_parse_endpoint);
> +
> +/**
>   * 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
> diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
> index 3bbeb60..2b233db 100644
> --- a/include/linux/of_graph.h
> +++ b/include/linux/of_graph.h
> @@ -14,7 +14,21 @@
>  #ifndef __LINUX_OF_GRAPH_H
>  #define __LINUX_OF_GRAPH_H
>  
> +/**
> + * struct of_endpoint - the OF graph endpoint data structure
> + * @port: identifier (value of reg property) of a port this endpoint belongs to
> + * @id: identifier (value of reg property) of this endpoint
> + * @local_node: pointer to device_node of this endpoint
> + */
> +struct of_endpoint {
> +	unsigned int port;
> +	unsigned int id;
> +	const struct device_node *local_node;
> +};

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.

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.


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.

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.

 Tomi



Download attachment "signature.asc" of type "application/pgp-signature" (902 bytes)

Powered by blists - more mailing lists