[PATCH 0/4] drm: use for_each_endpoint_of_node macro

[PATCH 0/4] drm: use for_each_endpoint_of_node macro

[Philipp Zabel]

Hi,

once the "Add of-graph helpers to loop over endpoints and find ports by id"
series [1] gets merged, these patches simplify the common case of looping
over all of graph endpoints in a device node.

[1] https://lkml.org/lkml/2015/2/23/115

regards
Philipp

Philipp Zabel (4):
  drm: use for_each_endpoint_of_node macro in drm_of_find_possible_crtcs
  drm/imx: use for_each_endpoint_of_node macro in
    imx_drm_encoder_get_mux_id
  drm/rcar-du: use for_each_endpoint_of_node macro
  drm/rockchip: use for_each_endpoint_of_node macro, drop endpoint
    reference on break

 drivers/gpu/drm/drm_of.c                    | 10 +++-------
 drivers/gpu/drm/imx/imx-drm-core.c          |  9 +++------
 drivers/gpu/drm/rcar-du/rcar_du_kms.c       | 16 +++-------------
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 11 ++++-------
 4 files changed, 13 insertions(+), 33 deletions(-)

-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/4] 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>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/drm_of.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 16150a0..aaa1307 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -43,14 +43,10 @@ static uint32_t drm_crtc_port_mask(struct drm_device *dev,
 uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 				    struct device_node *port)
 {
-	struct device_node *remote_port, *ep = NULL;
+	struct device_node *remote_port, *ep;
 	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.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/imx: 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/gpu/drm/imx/imx-drm-core.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 84cf99f..3da9cc5 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -447,18 +447,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->crtc->port) {
 			ret = of_graph_parse_endpoint(ep, &endpoint);
+			of_node_put(ep);
 			return ret ? ret : endpoint.port;
 		}
-	} while (ep);
+	}
 
 	return -EINVAL;
 }
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm/rcar-du: use for_each_endpoint_of_node macro

[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/rcar-du/rcar_du_kms.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 68dab26..c235074 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -224,12 +224,7 @@ static int rcar_du_encoders_init_one(struct rcar_du_device *rcdu,
 
 	entity_ep_node = of_parse_phandle(ep->local_node, "remote-endpoint", 0);
 
-	while (1) {
-		ep_node = of_graph_get_next_endpoint(entity, ep_node);
-
-		if (!ep_node)
-			break;
-
+	for_each_endpoint_of_node(entity, ep_node) {
 		if (ep_node == entity_ep_node)
 			continue;
 
@@ -296,24 +291,19 @@ static int rcar_du_encoders_init_one(struct rcar_du_device *rcdu,
 static int rcar_du_encoders_init(struct rcar_du_device *rcdu)
 {
 	struct device_node *np = rcdu->dev->of_node;
-	struct device_node *ep_node = NULL;
+	struct device_node *ep_node;
 	unsigned int num_encoders = 0;
 
 	/*
 	 * Iterate over the endpoints and create one encoder for each output
 	 * pipeline.
 	 */
-	while (1) {
+	for_each_endpoint_of_node(np, ep_node) {
 		enum rcar_du_output output;
 		struct of_endpoint ep;
 		unsigned int i;
 		int ret;
 
-		ep_node = of_graph_get_next_endpoint(np, ep_node);
-
-		if (ep_node == NULL)
-			break;
-
 		ret = of_graph_parse_endpoint(ep_node, &ep);
 		if (ret < 0) {
 			of_node_put(ep_node);
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/rcar-du: use for_each_endpoint_of_node macro

[Laurent Pinchart]

Hi Philipp,

Thank you for the patch.

On Monday 23 February 2015 17:34:28 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/rcar-du/rcar_du_kms.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 68dab26..c235074 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -224,12 +224,7 @@ static int rcar_du_encoders_init_one(struct
> rcar_du_device *rcdu,
> 
>  	entity_ep_node = of_parse_phandle(ep->local_node, "remote-endpoint", 0);
> 
> -	while (1) {
> -		ep_node = of_graph_get_next_endpoint(entity, ep_node);
> -
> -		if (!ep_node)
> -			break;
> -
> +	for_each_endpoint_of_node(entity, ep_node) {
>  		if (ep_node == entity_ep_node)
>  			continue;
> 
> @@ -296,24 +291,19 @@ static int rcar_du_encoders_init_one(struct
> rcar_du_device *rcdu, static int rcar_du_encoders_init(struct
> rcar_du_device *rcdu)
>  {
>  	struct device_node *np = rcdu->dev->of_node;
> -	struct device_node *ep_node = NULL;
> +	struct device_node *ep_node;
>  	unsigned int num_encoders = 0;
> 
>  	/*
>  	 * Iterate over the endpoints and create one encoder for each output
>  	 * pipeline.
>  	 */
> -	while (1) {
> +	for_each_endpoint_of_node(np, ep_node) {
>  		enum rcar_du_output output;
>  		struct of_endpoint ep;
>  		unsigned int i;
>  		int ret;
> 
> -		ep_node = of_graph_get_next_endpoint(np, ep_node);
> -
> -		if (ep_node == NULL)
> -			break;
> -
>  		ret = of_graph_parse_endpoint(ep_node, &ep);
>  		if (ret < 0) {
>  			of_node_put(ep_node);

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/rockchip: use for_each_endpoint_of_node macro, drop endpoint reference on break

[Philipp Zabel]

Using the for_each_... macro should make the code a bit shorter and
easier to read. Also, when breaking out of the loop, the endpoint node
reference count needs to be decremented.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 21a481b..9bb4fd2 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -366,7 +366,7 @@ static const struct dev_pm_ops rockchip_drm_pm_ops = {
 int rockchip_drm_encoder_get_mux_id(struct device_node *node,
 				    struct drm_encoder *encoder)
 {
-	struct device_node *ep = NULL;
+	struct device_node *ep;
 	struct drm_crtc *crtc = encoder->crtc;
 	struct of_endpoint endpoint;
 	struct device_node *port;
@@ -375,18 +375,15 @@ int rockchip_drm_encoder_get_mux_id(struct device_node *node,
 	if (!node || !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 == crtc->port) {
 			ret = of_graph_parse_endpoint(ep, &endpoint);
+			of_node_put(ep);
 			return ret ?: endpoint.id;
 		}
-	} while (ep);
+	}
 
 	return -EINVAL;
 }
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/rockchip: use for_each_endpoint_of_node macro, drop endpoint reference on break

[Daniel Kurtz]

Hi Philipp,

Thanks for this cleanup!

On Tue, Feb 24, 2015 at 12:34 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> Using the for_each_... macro should make the code a bit shorter and
> easier to read. Also, when breaking out of the loop, the endpoint node
> reference count needs to be decremented.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index 21a481b..9bb4fd2 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -366,7 +366,7 @@ static const struct dev_pm_ops rockchip_drm_pm_ops = {
>  int rockchip_drm_encoder_get_mux_id(struct device_node *node,
>                                     struct drm_encoder *encoder)
>  {
> -       struct device_node *ep = NULL;
> +       struct device_node *ep;
>         struct drm_crtc *crtc = encoder->crtc;
>         struct of_endpoint endpoint;
>         struct device_node *port;
> @@ -375,18 +375,15 @@ int rockchip_drm_encoder_get_mux_id(struct device_node *node,
>         if (!node || !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);

Shouldn't we put port after comparing it to crtc->port?

This looks like an existing issue, though so this patch is (assuming
the series [0] is merged):

Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>

[0] https://lkml.org/lkml/2015/2/23/115

>
>                 if (port == crtc->port) {
>                         ret = of_graph_parse_endpoint(ep, &endpoint);
> +                       of_node_put(ep);
>                         return ret ?: endpoint.id;

This function looks really similar to imx_drm_encoder_get_mux_id().
The only difference is that it looks up endpoint.id rather than endpoint.port.

Is there any way we can resolve this slight difference so that both
can use a single common helper?

Or, at least move both of them to somewhere more generic?

>                 }
> -       } while (ep);
> +       }
>
>         return -EINVAL;
>  }
> --
> 2.1.4
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/rockchip: use for_each_endpoint_of_node macro, drop endpoint reference on break

[Philipp Zabel]

Hi Daniel,

thank you for the review!

Am Dienstag, den 24.02.2015, 11:13 +0800 schrieb Daniel Kurtz:
> > @@ -375,18 +375,15 @@ int rockchip_drm_encoder_get_mux_id(struct device_node *node,
> >         if (!node || !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);
> 
> Shouldn't we put port after comparing it to crtc->port?

We don't dereference the pointer. It is only compared to crtc->port in
the line below, so this should be fine.

> This looks like an existing issue, though so this patch is (assuming
> the series [0] is merged):
> 
> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
>
> [0] https://lkml.org/lkml/2015/2/23/115
> 
> >
> >                 if (port == crtc->port) {
> >                         ret = of_graph_parse_endpoint(ep, &endpoint);
> > +                       of_node_put(ep);
> >                         return ret ?: endpoint.id;
> 
> This function looks really similar to imx_drm_encoder_get_mux_id().

Good point.

> The only difference is that it looks up endpoint.id rather than endpoint.port.
>
> Is there any way we can resolve this slight difference so that both
> can use a single common helper?

The way I see it, the ports describe the physical inputs to the mux, so
the port number should be used to determine the mux setting. In theory
there could be multiple endpoints (multiple sources on the same bus
connected to a single mux input), so I wouldn't like to change imx-drm
to determine the mux setting from endpoint ids.
Could we change the rockchip driver to use the port id instead?

> Or, at least move both of them to somewhere more generic?

There's drm_of_find_possible_crtcs already, we could add
drm_of_encoder_get_port_id and possibly drm_of_encoder_get_endpoint_id
next to it.

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel