[PATCH 0/5] Add of-graph helpers to loop over endpoints and find ports by id

[PATCH 0/5] Add of-graph helpers to loop over endpoints and find ports by id

[Philipp Zabel]

Hi,

this series converts all existing users of of_graph_get_next_endpoint that pass
a non-NULL prev argument to the function and decrement its refcount themselves
to stop doing that. The of_node_put is moved into of_graph_get_next_endpoint
instead.
This allows to add a for_each_endpoint_of_node helper macro to loop over all
endpoints in a device tree node.
The third of patch adds a of_graph_get_port_by_id function to retrieve a port
by its known port id from the device tree.
Finally, the last three patches convert functions in drm_of.c and imx-drm-core.c
to use the for_each_endpoint_of_node macro instead of of_graph_get_next_endpoint.

regards
Philipp

Philipp Zabel (8):
  [media] soc_camera: Do not decrement endpoint node refcount in the
    loop
  imx-drm: Do not decrement endpoint node refcount in the loop
  of: Decrement refcount of previous endpoint in
    of_graph_get_next_endpoint
  of: Add for_each_endpoint_of_node helper macro
  of: Add of_graph_get_port_by_id function
  drm: use for_each_endpoint_of_node macro in drm_of_find_possible_crtcs
  imx-drm: use for_each_endpoint_of_node macro in
    imx_drm_encoder_get_mux_id
  imx-drm: use for_each_endpoint_of_node macro in
    imx_drm_encoder_parse_of

 drivers/gpu/drm/drm_of.c                       |  8 ++----
 drivers/media/platform/soc_camera/soc_camera.c |  2 +-
 drivers/of/base.c                              | 39 ++++++++++++++++++++------
 drivers/staging/imx-drm/imx-drm-core.c         | 34 +++++++---------------
 include/linux/of_graph.h                       | 11 ++++++++
 5 files changed, 55 insertions(+), 39 deletions(-)

-- 
2.1.0.rc1

[PATCH 1/8] [media] soc_camera: Do not decrement endpoint node refcount in the loop

[Philipp Zabel]

In preparation for a following patch, stop decrementing the endpoint node
refcount in the loop. This temporarily leaks a reference to the endpoint node,
which will be fixed by having of_graph_get_next_endpoint decrement the refcount
of its prev argument instead.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/soc_camera/soc_camera.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index f4308fe..f752489 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1696,11 +1696,11 @@ static void scan_of_host(struct soc_camera_host *ici)
 		if (!i)
 			soc_of_bind(ici, epn, ren->parent);
 
-		of_node_put(epn);
 		of_node_put(ren);
 
 		if (i) {
 			dev_err(dev, "multiple subdevices aren't supported yet!\n");
+			of_node_put(epn);
 			break;
 		}
 	}
-- 
2.1.0.rc1

[PATCH 2/8] imx-drm: Do not decrement endpoint node refcount in the loop

[Philipp Zabel]

In preparation for the following patch, stop decrementing the endpoint node
refcount in the loop. This temporarily leaks a reference to the endpoint node,
which will be fixed by having of_graph_get_next_endpoint decrement the refcount
of its prev argument instead.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/staging/imx-drm/imx-drm-core.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
index 6b22106..12303b3 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -434,14 +434,6 @@ static uint32_t imx_drm_find_crtc_mask(struct imx_drm_device *imxdrm,
 	return 0;
 }
 
-static struct device_node *imx_drm_of_get_next_endpoint(
-		const struct device_node *parent, struct device_node *prev)
-{
-	struct device_node *node = of_graph_get_next_endpoint(parent, prev);
-	of_node_put(prev);
-	return node;
-}
-
 int imx_drm_encoder_parse_of(struct drm_device *drm,
 	struct drm_encoder *encoder, struct device_node *np)
 {
@@ -453,7 +445,7 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
 	for (i = 0; ; i++) {
 		u32 mask;
 
-		ep = imx_drm_of_get_next_endpoint(np, ep);
+		ep = of_graph_get_next_endpoint(np, ep);
 		if (!ep)
 			break;
 
@@ -502,7 +494,7 @@ int imx_drm_encoder_get_mux_id(struct device_node *node,
 		return -EINVAL;
 
 	do {
-		ep = imx_drm_of_get_next_endpoint(node, ep);
+		ep = of_graph_get_next_endpoint(node, ep);
 		if (!ep)
 			break;
 
-- 
2.1.0.rc1

[PATCH 3/8] of: Decrement refcount of previous endpoint in of_graph_get_next_endpoint

[Philipp Zabel]

Decrementing the reference count of the previous endpoint node allows to
use the of_graph_get_next_endpoint function in a for_each_... style macro.
Prior to this patch, all current users of this function that actually pass
a non-NULL prev parameter should be changed to not decrement the passed
prev argument's refcount themselves.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/of/base.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index d8574ad..a49b5628 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2058,8 +2058,7 @@ EXPORT_SYMBOL(of_graph_parse_endpoint);
  * @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.
+ * of the passed @prev node is decremented.
  */
 struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 					struct device_node *prev)
@@ -2095,12 +2094,6 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 		if (WARN_ONCE(!port, "%s(): endpoint %s has no parent node\n",
 			      __func__, prev->full_name))
 			return NULL;
-
-		/*
-		 * Avoid dropping prev node refcount to 0 when getting the next
-		 * child below.
-		 */
-		of_node_get(prev);
 	}
 
 	while (1) {
-- 
2.1.0.rc1

[PATCH 4/8] of: Add for_each_endpoint_of_node helper macro

[Philipp Zabel]

Note that while of_graph_get_next_endpoint decrements the reference count
of the child node passed to it, of_node_put(child) still has to be called
manually when breaking out of the loop.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 include/linux/of_graph.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
index befef42..2890a4c 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h
@@ -26,6 +26,10 @@ struct of_endpoint {
 	const struct device_node *local_node;
 };
 
+#define for_each_endpoint_of_node(parent, child) \
+	for (child = of_graph_get_next_endpoint(parent, NULL); child != NULL; \
+	     child = of_graph_get_next_endpoint(parent, child))
+
 #ifdef CONFIG_OF
 int of_graph_parse_endpoint(const struct device_node *node,
 				struct of_endpoint *endpoint);
-- 
2.1.0.rc1

[PATCH 5/8] of: Add of_graph_get_port_by_id function

[Philipp Zabel]

This patch adds a function to get a port device tree node by port id,
or reg property value.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/of/base.c        | 30 ++++++++++++++++++++++++++++++
 include/linux/of_graph.h |  7 +++++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index a49b5628..6044c15 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2053,6 +2053,36 @@ int of_graph_parse_endpoint(const struct device_node *node,
 EXPORT_SYMBOL(of_graph_parse_endpoint);
 
 /**
+ * of_graph_get_port_by_id() - get the port matching a given id
+ * @parent: pointer to the parent device node
+ * @id: id of the port
+ *
+ * Return: A 'port' node pointer with refcount incremented.The caller
+ * has to use of_node_put() on it when done.
+ */
+struct device_node *of_graph_get_port_by_id(struct device_node *node, int id)
+{
+	struct device_node *port = NULL;
+	int port_id;
+
+	while (true) {
+		port = of_get_next_child(node, port);
+		if (!port)
+			return NULL;
+		if (of_node_cmp(port->name, "port") != 0)
+			continue;
+		if (of_property_read_u32(port, "reg", &port_id)) {
+			if (!id)
+				return port;
+		} else {
+			if (id == port_id)
+				return port;
+		}
+	}
+}
+EXPORT_SYMBOL(of_graph_get_port_by_id);
+
+/**
  * 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 2890a4c..24ceb4b 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h
@@ -33,6 +33,7 @@ struct of_endpoint {
 #ifdef CONFIG_OF
 int of_graph_parse_endpoint(const struct device_node *node,
 				struct of_endpoint *endpoint);
+struct device_node *of_graph_get_port_by_id(struct device_node *node, int id);
 struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 					struct device_node *previous);
 struct device_node *of_graph_get_remote_port_parent(
@@ -46,6 +47,12 @@ static inline int of_graph_parse_endpoint(const struct device_node *node,
 	return -ENOSYS;
 }
 
+static inline struct device_node *of_graph_get_port_by_id(
+					struct device_node *node, int id)
+{
+	return NULL;
+}
+
 static inline struct device_node *of_graph_get_next_endpoint(
 					const struct device_node *parent,
 					struct device_node *previous)
-- 
2.1.0.rc1

[PATCH 6/8] drm: use for_each_endpoint_of_node macro in drm_of_find_possible_crtcs

[Philipp Zabel]

Using the for_each_... macro should make the code a bit shorter and
easier to read.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/drm_of.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 16150a0..024fa77 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -46,11 +46,7 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 	struct device_node *remote_port, *ep = NULL;
 	uint32_t possible_crtcs = 0;
 
-	do {
-		ep = of_graph_get_next_endpoint(port, ep);
-		if (!ep)
-			break;
-
+	for_each_endpoint_of_node(port, ep) {
 		remote_port = of_graph_get_remote_port(ep);
 		if (!remote_port) {
 			of_node_put(ep);
@@ -60,7 +56,7 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 		possible_crtcs |= drm_crtc_port_mask(dev, remote_port);
 
 		of_node_put(remote_port);
-	} while (1);
+	}
 
 	return possible_crtcs;
 }
-- 
2.1.0.rc1

[PATCH 7/8] imx-drm: use for_each_endpoint_of_node macro in imx_drm_encoder_get_mux_id

[Philipp Zabel]

Using the for_each_... macro should make the code bit shorter and
easier to read. This patch also properly decrements the endpoint node
reference count before returning out of the loop.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/staging/imx-drm/imx-drm-core.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
index 12303b3..9b5222c 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -493,18 +493,15 @@ int imx_drm_encoder_get_mux_id(struct device_node *node,
 	if (!node || !imx_crtc)
 		return -EINVAL;
 
-	do {
-		ep = of_graph_get_next_endpoint(node, ep);
-		if (!ep)
-			break;
-
+	for_each_endpoint_of_node(node, ep) {
 		port = of_graph_get_remote_port(ep);
 		of_node_put(port);
 		if (port == imx_crtc->port) {
 			ret = of_graph_parse_endpoint(ep, &endpoint);
+			of_node_put(ep);
 			return ret ? ret : endpoint.port;
 		}
-	} while (ep);
+	}
 
 	return -EINVAL;
 }
-- 
2.1.0.rc1

[PATCH 8/8] imx-drm: use for_each_endpoint_of_node macro in imx_drm_encoder_parse_of

[Philipp Zabel]

Using the for_each_... macro should make the code bit shorter and
easier to read.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/staging/imx-drm/imx-drm-core.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
index 9b5222c..460d785 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -438,17 +438,13 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
 	struct drm_encoder *encoder, struct device_node *np)
 {
 	struct imx_drm_device *imxdrm = drm->dev_private;
-	struct device_node *ep = NULL;
+	struct device_node *ep;
 	uint32_t crtc_mask = 0;
-	int i;
+	int i = 0;
 
-	for (i = 0; ; i++) {
+	for_each_endpoint_of_node(np, ep) {
 		u32 mask;
 
-		ep = of_graph_get_next_endpoint(np, ep);
-		if (!ep)
-			break;
-
 		mask = imx_drm_find_crtc_mask(imxdrm, ep);
 
 		/*
@@ -457,14 +453,15 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
 		 * not been registered yet.  Defer probing, and hope that
 		 * the required CRTC is added later.
 		 */
-		if (mask == 0)
+		if (mask == 0) {
+			of_node_put(ep);
 			return -EPROBE_DEFER;
+		}
 
 		crtc_mask |= mask;
+		i++;
 	}
 
-	if (ep)
-		of_node_put(ep);
 	if (i == 0)
 		return -ENOENT;
 
-- 
2.1.0.rc1

Re: [PATCH 4/8] of: Add for_each_endpoint_of_node helper macro

[Laurent Pinchart]

Hi Philipp,

Thank you for the patch.

On Tuesday 19 August 2014 15:02:42 Philipp Zabel wrote:
> Note that while of_graph_get_next_endpoint decrements the reference count
> of the child node passed to it, of_node_put(child) still has to be called
> manually when breaking out of the loop.

I think this is important enough to be mentioned in a comment in of_graph.h.

> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  include/linux/of_graph.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
> index befef42..2890a4c 100644
> --- a/include/linux/of_graph.h
> +++ b/include/linux/of_graph.h
> @@ -26,6 +26,10 @@ struct of_endpoint {
>  	const struct device_node *local_node;
>  };
> 
> +#define for_each_endpoint_of_node(parent, child) \
> +	for (child = of_graph_get_next_endpoint(parent, NULL); child != NULL; \
> +	     child = of_graph_get_next_endpoint(parent, child))
> +
>  #ifdef CONFIG_OF
>  int of_graph_parse_endpoint(const struct device_node *node,
>  				struct of_endpoint *endpoint);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/8] of: Add of_graph_get_port_by_id function

[Laurent Pinchart]

Hi Philipp,

Thank you for the patch.

On Tuesday 19 August 2014 15:02:43 Philipp Zabel wrote:
> This patch adds a function to get a port device tree node by port id,
> or reg property value.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/of/base.c        | 30 ++++++++++++++++++++++++++++++
>  include/linux/of_graph.h |  7 +++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index a49b5628..6044c15 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2053,6 +2053,36 @@ int of_graph_parse_endpoint(const struct device_node
> *node, EXPORT_SYMBOL(of_graph_parse_endpoint);
> 
>  /**
> + * of_graph_get_port_by_id() - get the port matching a given id
> + * @parent: pointer to the parent device node
> + * @id: id of the port
> + *
> + * Return: A 'port' node pointer with refcount incremented.The caller

Missing space before "The".

> + * has to use of_node_put() on it when done.
> + */
> +struct device_node *of_graph_get_port_by_id(struct device_node *node, int
> id)

How about making id and unsigned int, as it can't be negative ?

> +{
> +	struct device_node *port = NULL;
> +	int port_id;
> +
> +	while (true) {
> +		port = of_get_next_child(node, port);
> +		if (!port)
> +			return NULL;
> +		if (of_node_cmp(port->name, "port") != 0)
> +			continue;
> +		if (of_property_read_u32(port, "reg", &port_id)) {
> +			if (!id)
> +				return port;
> +		} else {
> +			if (id == port_id)
> +				return port;
> +		}

Nitpicking here, I would have written this

		int port_id = 0;

		port = of_get_next_child(node, port);
		if (!port)
			return NULL;
		if (of_node_cmp(port->name, "port") != 0)
			continue;
		of_property_read_u32(port, "reg", &port_id);
		if (id == port_id)
			return port;

That saves 8 bytes with my ARM cross-compiler (at the expense of using two 
extra local registers).

Please free to ignore this is you find your code layout easier to read.

> +	}
> +}
> +EXPORT_SYMBOL(of_graph_get_port_by_id);
> +
> +/**
>   * 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 2890a4c..24ceb4b 100644
> --- a/include/linux/of_graph.h
> +++ b/include/linux/of_graph.h
> @@ -33,6 +33,7 @@ struct of_endpoint {
>  #ifdef CONFIG_OF
>  int of_graph_parse_endpoint(const struct device_node *node,
>  				struct of_endpoint *endpoint);
> +struct device_node *of_graph_get_port_by_id(struct device_node *node, int
> id); struct device_node *of_graph_get_next_endpoint(const struct
> device_node *parent, struct device_node *previous);
>  struct device_node *of_graph_get_remote_port_parent(
> @@ -46,6 +47,12 @@ static inline int of_graph_parse_endpoint(const struct
> device_node *node, return -ENOSYS;
>  }
> 
> +static inline struct device_node *of_graph_get_port_by_id(
> +					struct device_node *node, int id)
> +{
> +	return NULL;
> +}
> +
>  static inline struct device_node *of_graph_get_next_endpoint(
>  					const struct device_node *parent,
>  					struct device_node *previous)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/8] drm: use for_each_endpoint_of_node macro in drm_of_find_possible_crtcs

[Laurent Pinchart]

Hi Philipp,

Thank you for the patch.

On Tuesday 19 August 2014 15:02:44 Philipp Zabel wrote:
> Using the for_each_... macro should make the code a bit shorter and
> easier to read.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/drm_of.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 16150a0..024fa77 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -46,11 +46,7 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device
> *dev, struct device_node *remote_port, *ep = NULL;
>  	uint32_t possible_crtcs = 0;
> 
> -	do {
> -		ep = of_graph_get_next_endpoint(port, ep);
> -		if (!ep)
> -			break;
> -
> +	for_each_endpoint_of_node(port, ep) {
>  		remote_port = of_graph_get_remote_port(ep);
>  		if (!remote_port) {
>  			of_node_put(ep);
> @@ -60,7 +56,7 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device
> *dev, possible_crtcs |= drm_crtc_port_mask(dev, remote_port);
> 
>  		of_node_put(remote_port);
> -	} while (1);
> +	}
> 
>  	return possible_crtcs;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/8] of: Add of_graph_get_port_by_id function

[Philipp Zabel]

Hi Laurent,

Thank you for the comments.

Am Mittwoch, den 20.08.2014, 22:13 +0200 schrieb Laurent Pinchart:
[...]
> > +	struct device_node *port = NULL;
> > +	int port_id;
> > +
> > +	while (true) {
> > +		port = of_get_next_child(node, port);
> > +		if (!port)
> > +			return NULL;
> > +		if (of_node_cmp(port->name, "port") != 0)
> > +			continue;
> > +		if (of_property_read_u32(port, "reg", &port_id)) {
> > +			if (!id)
> > +				return port;
> > +		} else {
> > +			if (id == port_id)
> > +				return port;
> > +		}
> 
> Nitpicking here, I would have written this
> 
> 		int port_id = 0;
> 
> 		port = of_get_next_child(node, port);
> 		if (!port)
> 			return NULL;
> 		if (of_node_cmp(port->name, "port") != 0)
> 			continue;
> 		of_property_read_u32(port, "reg", &port_id);
> 		if (id == port_id)
> 			return port;
> 
> That saves 8 bytes with my ARM cross-compiler (at the expense of using two 
> extra local registers).
> 
> Please free to ignore this is you find your code layout easier to read.

No, that does look sensible to me. I'll follow your suggestions and
resend the series.

regards
Philipp