[PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Liviu Dudau]

Hello,

This is v2 of the patchset trying to make drm_of_component_probe() cope with finding
both local crtc ports and remote encoder ones. Heiko Stübner was nice enough to test
an earlier version that was patched following Russell's suggestions on rk3288, but
I haven't seen any reports from iMX or Armada users.

Changelog:
 v2: Updated the drm_of_component_probe() comment to explain why the reference count
     is not dropped. Fixed the compare_port() function for rockchip as described by
     Russell.
 v1: Original submission. http://lists.freedesktop.org/archives/dri-devel/2015-November/094546.html

Liviu Dudau (2):
  drm: Improve drm_of_component_probe() to correctly handle ports and remote ports.
  drm/rockchip: Convert the probe function to the generic drm_of_component_probe()

 drivers/gpu/drm/armada/armada_drv.c         |  3 +-
 drivers/gpu/drm/drm_of.c                    | 19 ++++--
 drivers/gpu/drm/imx/imx-drm-core.c          |  3 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 98 ++++++-----------------------
 include/drm/drm_of.h                        |  6 +-
 5 files changed, 40 insertions(+), 89 deletions(-)

-- 
2.6.2

[PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Liviu Dudau]

Hello,

This is v2 of the patchset trying to make drm_of_component_probe() cope with finding
both local crtc ports and remote encoder ones. Heiko Stübner was nice enough to test
an earlier version that was patched following Russell's suggestions on rk3288, but
I haven't seen any reports from iMX or Armada users.

Changelog:
 v2: Updated the drm_of_component_probe() comment to explain why the reference count
     is not dropped. Fixed the compare_port() function for rockchip as described by
     Russell.
 v1: Original submission. http://lists.freedesktop.org/archives/dri-devel/2015-November/094546.html

Liviu Dudau (2):
  drm: Improve drm_of_component_probe() to correctly handle ports and remote ports.
  drm/rockchip: Convert the probe function to the generic drm_of_component_probe()

 drivers/gpu/drm/armada/armada_drv.c         |  3 +-
 drivers/gpu/drm/drm_of.c                    | 19 ++++--
 drivers/gpu/drm/imx/imx-drm-core.c          |  3 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 98 ++++++-----------------------
 include/drm/drm_of.h                        |  6 +-
 5 files changed, 40 insertions(+), 89 deletions(-)

-- 
2.6.2

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

[PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Liviu Dudau]

Hello,

This is v2 of the patchset trying to make drm_of_component_probe() cope with finding
both local crtc ports and remote encoder ones. Heiko St?bner was nice enough to test
an earlier version that was patched following Russell's suggestions on rk3288, but
I haven't seen any reports from iMX or Armada users.

Changelog:
 v2: Updated the drm_of_component_probe() comment to explain why the reference count
     is not dropped. Fixed the compare_port() function for rockchip as described by
     Russell.
 v1: Original submission. http://lists.freedesktop.org/archives/dri-devel/2015-November/094546.html

Liviu Dudau (2):
  drm: Improve drm_of_component_probe() to correctly handle ports and remote ports.
  drm/rockchip: Convert the probe function to the generic drm_of_component_probe()

 drivers/gpu/drm/armada/armada_drv.c         |  3 +-
 drivers/gpu/drm/drm_of.c                    | 19 ++++--
 drivers/gpu/drm/imx/imx-drm-core.c          |  3 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 98 ++++++-----------------------
 include/drm/drm_of.h                        |  6 +-
 5 files changed, 40 insertions(+), 89 deletions(-)

-- 
2.6.2

[PATCH v2 1/2] drm: Improve drm_of_component_probe() to correctly handle ports and remote ports.

[Liviu Dudau]

Rockchip DRM driver cannot use the same compare_of() function to
match ports and remote ports (aka encoders) as their OF sub-trees
look different. Add a second compare function to be used when encoders
are added to the component framework and patch the existing users of
the function accordingly.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/gpu/drm/armada/armada_drv.c |  3 ++-
 drivers/gpu/drm/drm_of.c            | 19 ++++++++++++++-----
 drivers/gpu/drm/imx/imx-drm-core.c  |  3 ++-
 include/drm/drm_of.h                |  6 ++++--
 4 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 77ab93d..3a2a929 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -274,7 +274,8 @@ static int armada_drm_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int ret;
 
-	ret = drm_of_component_probe(dev, compare_dev_name, &armada_master_ops);
+	ret = drm_of_component_probe(dev, compare_dev_name, compare_dev_name,
+				     &armada_master_ops);
 	if (ret != -EINVAL)
 		return ret;
 
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 493c05c..34589d3 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -77,7 +77,8 @@ EXPORT_SYMBOL(drm_of_find_possible_crtcs);
  * Returns zero if successful, or one of the standard error codes if it fails.
  */
 int drm_of_component_probe(struct device *dev,
-			   int (*compare_of)(struct device *, void *),
+			   int (*compare_port)(struct device *, void *),
+			   int (*compare_encoder)(struct device *, void *),
 			   const struct component_master_ops *m_ops)
 {
 	struct device_node *ep, *port, *remote;
@@ -101,8 +102,12 @@ int drm_of_component_probe(struct device *dev,
 			continue;
 		}
 
-		component_match_add(dev, &match, compare_of, port);
-		of_node_put(port);
+		component_match_add(dev, &match, compare_port, port);
+		/*
+		 * component_match_add keeps a reference to the port
+		 * variable, so we need to keep the reference count
+		 * increment from of_parse_phandle()
+		 */
 	}
 
 	if (i == 0) {
@@ -140,8 +145,12 @@ int drm_of_component_probe(struct device *dev,
 				continue;
 			}
 
-			component_match_add(dev, &match, compare_of, remote);
-			of_node_put(remote);
+			component_match_add(dev, &match, compare_encoder, remote);
+			/*
+			 * component_match_add keeps a reference to the port
+			 * variable, so we need to keep the reference count
+			 * increment from of_graph_get_remote_port_parent()
+			 */
 		}
 		of_node_put(port);
 	}
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 64f16ea..0d36410 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -531,7 +531,8 @@ static const struct component_master_ops imx_drm_ops = {
 
 static int imx_drm_platform_probe(struct platform_device *pdev)
 {
-	int ret = drm_of_component_probe(&pdev->dev, compare_of, &imx_drm_ops);
+	int ret = drm_of_component_probe(&pdev->dev, compare_of, compare_of,
+					 &imx_drm_ops);
 
 	if (!ret)
 		ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
index 8544665..1c29e42 100644
--- a/include/drm/drm_of.h
+++ b/include/drm/drm_of.h
@@ -10,7 +10,8 @@ struct device_node;
 extern uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 					   struct device_node *port);
 extern int drm_of_component_probe(struct device *dev,
-				  int (*compare_of)(struct device *, void *),
+				  int (*compare_port)(struct device *, void *),
+				  int (*compare_encoder)(struct device *, void *),
 				  const struct component_master_ops *m_ops);
 #else
 static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
@@ -21,7 +22,8 @@ static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 
 static inline int
 drm_of_component_probe(struct device *dev,
-		       int (*compare_of)(struct device *, void *),
+		       int (*compare_port)(struct device *, void *),
+		       int (*compare_encoder)(struct device *, void *),
 		       const struct component_master_ops *m_ops)
 {
 	return -EINVAL;
-- 
2.6.2

[PATCH v2 1/2] drm: Improve drm_of_component_probe() to correctly handle ports and remote ports.

[Liviu Dudau]

Rockchip DRM driver cannot use the same compare_of() function to
match ports and remote ports (aka encoders) as their OF sub-trees
look different. Add a second compare function to be used when encoders
are added to the component framework and patch the existing users of
the function accordingly.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/gpu/drm/armada/armada_drv.c |  3 ++-
 drivers/gpu/drm/drm_of.c            | 19 ++++++++++++++-----
 drivers/gpu/drm/imx/imx-drm-core.c  |  3 ++-
 include/drm/drm_of.h                |  6 ++++--
 4 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 77ab93d..3a2a929 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -274,7 +274,8 @@ static int armada_drm_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int ret;
 
-	ret = drm_of_component_probe(dev, compare_dev_name, &armada_master_ops);
+	ret = drm_of_component_probe(dev, compare_dev_name, compare_dev_name,
+				     &armada_master_ops);
 	if (ret != -EINVAL)
 		return ret;
 
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 493c05c..34589d3 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -77,7 +77,8 @@ EXPORT_SYMBOL(drm_of_find_possible_crtcs);
  * Returns zero if successful, or one of the standard error codes if it fails.
  */
 int drm_of_component_probe(struct device *dev,
-			   int (*compare_of)(struct device *, void *),
+			   int (*compare_port)(struct device *, void *),
+			   int (*compare_encoder)(struct device *, void *),
 			   const struct component_master_ops *m_ops)
 {
 	struct device_node *ep, *port, *remote;
@@ -101,8 +102,12 @@ int drm_of_component_probe(struct device *dev,
 			continue;
 		}
 
-		component_match_add(dev, &match, compare_of, port);
-		of_node_put(port);
+		component_match_add(dev, &match, compare_port, port);
+		/*
+		 * component_match_add keeps a reference to the port
+		 * variable, so we need to keep the reference count
+		 * increment from of_parse_phandle()
+		 */
 	}
 
 	if (i == 0) {
@@ -140,8 +145,12 @@ int drm_of_component_probe(struct device *dev,
 				continue;
 			}
 
-			component_match_add(dev, &match, compare_of, remote);
-			of_node_put(remote);
+			component_match_add(dev, &match, compare_encoder, remote);
+			/*
+			 * component_match_add keeps a reference to the port
+			 * variable, so we need to keep the reference count
+			 * increment from of_graph_get_remote_port_parent()
+			 */
 		}
 		of_node_put(port);
 	}
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 64f16ea..0d36410 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -531,7 +531,8 @@ static const struct component_master_ops imx_drm_ops = {
 
 static int imx_drm_platform_probe(struct platform_device *pdev)
 {
-	int ret = drm_of_component_probe(&pdev->dev, compare_of, &imx_drm_ops);
+	int ret = drm_of_component_probe(&pdev->dev, compare_of, compare_of,
+					 &imx_drm_ops);
 
 	if (!ret)
 		ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
index 8544665..1c29e42 100644
--- a/include/drm/drm_of.h
+++ b/include/drm/drm_of.h
@@ -10,7 +10,8 @@ struct device_node;
 extern uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 					   struct device_node *port);
 extern int drm_of_component_probe(struct device *dev,
-				  int (*compare_of)(struct device *, void *),
+				  int (*compare_port)(struct device *, void *),
+				  int (*compare_encoder)(struct device *, void *),
 				  const struct component_master_ops *m_ops);
 #else
 static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
@@ -21,7 +22,8 @@ static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 
 static inline int
 drm_of_component_probe(struct device *dev,
-		       int (*compare_of)(struct device *, void *),
+		       int (*compare_port)(struct device *, void *),
+		       int (*compare_encoder)(struct device *, void *),
 		       const struct component_master_ops *m_ops)
 {
 	return -EINVAL;
-- 
2.6.2

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

[PATCH v2 1/2] drm: Improve drm_of_component_probe() to correctly handle ports and remote ports.

[Liviu Dudau]

Rockchip DRM driver cannot use the same compare_of() function to
match ports and remote ports (aka encoders) as their OF sub-trees
look different. Add a second compare function to be used when encoders
are added to the component framework and patch the existing users of
the function accordingly.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/gpu/drm/armada/armada_drv.c |  3 ++-
 drivers/gpu/drm/drm_of.c            | 19 ++++++++++++++-----
 drivers/gpu/drm/imx/imx-drm-core.c  |  3 ++-
 include/drm/drm_of.h                |  6 ++++--
 4 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 77ab93d..3a2a929 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -274,7 +274,8 @@ static int armada_drm_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int ret;
 
-	ret = drm_of_component_probe(dev, compare_dev_name, &armada_master_ops);
+	ret = drm_of_component_probe(dev, compare_dev_name, compare_dev_name,
+				     &armada_master_ops);
 	if (ret != -EINVAL)
 		return ret;
 
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 493c05c..34589d3 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -77,7 +77,8 @@ EXPORT_SYMBOL(drm_of_find_possible_crtcs);
  * Returns zero if successful, or one of the standard error codes if it fails.
  */
 int drm_of_component_probe(struct device *dev,
-			   int (*compare_of)(struct device *, void *),
+			   int (*compare_port)(struct device *, void *),
+			   int (*compare_encoder)(struct device *, void *),
 			   const struct component_master_ops *m_ops)
 {
 	struct device_node *ep, *port, *remote;
@@ -101,8 +102,12 @@ int drm_of_component_probe(struct device *dev,
 			continue;
 		}
 
-		component_match_add(dev, &match, compare_of, port);
-		of_node_put(port);
+		component_match_add(dev, &match, compare_port, port);
+		/*
+		 * component_match_add keeps a reference to the port
+		 * variable, so we need to keep the reference count
+		 * increment from of_parse_phandle()
+		 */
 	}
 
 	if (i == 0) {
@@ -140,8 +145,12 @@ int drm_of_component_probe(struct device *dev,
 				continue;
 			}
 
-			component_match_add(dev, &match, compare_of, remote);
-			of_node_put(remote);
+			component_match_add(dev, &match, compare_encoder, remote);
+			/*
+			 * component_match_add keeps a reference to the port
+			 * variable, so we need to keep the reference count
+			 * increment from of_graph_get_remote_port_parent()
+			 */
 		}
 		of_node_put(port);
 	}
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 64f16ea..0d36410 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -531,7 +531,8 @@ static const struct component_master_ops imx_drm_ops = {
 
 static int imx_drm_platform_probe(struct platform_device *pdev)
 {
-	int ret = drm_of_component_probe(&pdev->dev, compare_of, &imx_drm_ops);
+	int ret = drm_of_component_probe(&pdev->dev, compare_of, compare_of,
+					 &imx_drm_ops);
 
 	if (!ret)
 		ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
index 8544665..1c29e42 100644
--- a/include/drm/drm_of.h
+++ b/include/drm/drm_of.h
@@ -10,7 +10,8 @@ struct device_node;
 extern uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 					   struct device_node *port);
 extern int drm_of_component_probe(struct device *dev,
-				  int (*compare_of)(struct device *, void *),
+				  int (*compare_port)(struct device *, void *),
+				  int (*compare_encoder)(struct device *, void *),
 				  const struct component_master_ops *m_ops);
 #else
 static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
@@ -21,7 +22,8 @@ static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 
 static inline int
 drm_of_component_probe(struct device *dev,
-		       int (*compare_of)(struct device *, void *),
+		       int (*compare_port)(struct device *, void *),
+		       int (*compare_encoder)(struct device *, void *),
 		       const struct component_master_ops *m_ops)
 {
 	return -EINVAL;
-- 
2.6.2

[PATCH v2 2/2] drm/rockchip: Convert the probe function to the generic drm_of_component_probe()

[Liviu Dudau]

Initial attempt to convert rockchip to drm_of_component_probe() missed the
difference between ports and encoders when using the compare_of() function.
Now that drm_of_component_probe() has been enhanced, let's try again the
conversion.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 98 ++++++-----------------------
 1 file changed, 18 insertions(+), 80 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f22e1e1..574324e 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -19,6 +19,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_of.h>
 #include <linux/dma-mapping.h>
 #include <linux/pm_runtime.h>
 #include <linux/module.h>
@@ -411,36 +412,6 @@ int rockchip_drm_encoder_get_mux_id(struct device_node *node,
 }
 EXPORT_SYMBOL_GPL(rockchip_drm_encoder_get_mux_id);
 
-static int compare_of(struct device *dev, void *data)
-{
-	struct device_node *np = data;
-
-	return dev->of_node == np;
-}
-
-static void rockchip_add_endpoints(struct device *dev,
-				   struct component_match **match,
-				   struct device_node *port)
-{
-	struct device_node *ep, *remote;
-
-	for_each_child_of_node(port, ep) {
-		remote = of_graph_get_remote_port_parent(ep);
-		if (!remote || !of_device_is_available(remote)) {
-			of_node_put(remote);
-			continue;
-		} else if (!of_device_is_available(remote->parent)) {
-			dev_warn(dev, "parent device of %s is not available\n",
-				 remote->full_name);
-			of_node_put(remote);
-			continue;
-		}
-
-		component_match_add(dev, match, compare_of, remote);
-		of_node_put(remote);
-	}
-}
-
 static int rockchip_drm_bind(struct device *dev)
 {
 	struct drm_device *drm;
@@ -481,63 +452,30 @@ static const struct component_master_ops rockchip_drm_ops = {
 	.unbind = rockchip_drm_unbind,
 };
 
-static int rockchip_drm_platform_probe(struct platform_device *pdev)
+static int compare_port(struct device *dev, void *data)
 {
-	struct device *dev = &pdev->dev;
-	struct component_match *match = NULL;
-	struct device_node *np = dev->of_node;
-	struct device_node *port;
-	int i;
+	struct device_node *np = data;
 
-	if (!np)
-		return -ENODEV;
-	/*
-	 * Bind the crtc ports first, so that
-	 * drm_of_find_possible_crtcs called from encoder .bind callbacks
-	 * works as expected.
-	 */
-	for (i = 0;; i++) {
-		port = of_parse_phandle(np, "ports", i);
-		if (!port)
-			break;
-
-		if (!of_device_is_available(port->parent)) {
-			of_node_put(port);
-			continue;
-		}
+	return dev->of_node == np->parent;
+}
 
-		component_match_add(dev, &match, compare_of, port->parent);
-		of_node_put(port);
-	}
+static int compare_encoder(struct device *dev, void *data)
+{
+	struct device_node *np = data;
 
-	if (i == 0) {
-		dev_err(dev, "missing 'ports' property\n");
-		return -ENODEV;
-	}
+	return dev->of_node == np;
+}
 
-	if (!match) {
-		dev_err(dev, "No available vop found for display-subsystem.\n");
-		return -ENODEV;
-	}
-	/*
-	 * For each bound crtc, bind the encoders attached to its
-	 * remote endpoint.
-	 */
-	for (i = 0;; i++) {
-		port = of_parse_phandle(np, "ports", i);
-		if (!port)
-			break;
-
-		if (!of_device_is_available(port->parent)) {
-			of_node_put(port);
-			continue;
-		}
+static int rockchip_drm_platform_probe(struct platform_device *pdev)
+{
+	int ret = drm_of_component_probe(&pdev->dev, compare_port,
+					 compare_encoder, &rockchip_drm_ops);
 
-		rockchip_add_endpoints(dev, &match, port);
-		of_node_put(port);
-	}
+	/* keep compatibility with old code that was returning -ENODEV */
+	if (ret == -EINVAL)
+		return -ENODEV;
 
-	return component_master_add_with_match(dev, &rockchip_drm_ops, match);
+	return ret;
 }
 
 static int rockchip_drm_platform_remove(struct platform_device *pdev)
-- 
2.6.2

[PATCH v2 2/2] drm/rockchip: Convert the probe function to the generic drm_of_component_probe()

[Liviu Dudau]

Initial attempt to convert rockchip to drm_of_component_probe() missed the
difference between ports and encoders when using the compare_of() function.
Now that drm_of_component_probe() has been enhanced, let's try again the
conversion.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 98 ++++++-----------------------
 1 file changed, 18 insertions(+), 80 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f22e1e1..574324e 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -19,6 +19,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_of.h>
 #include <linux/dma-mapping.h>
 #include <linux/pm_runtime.h>
 #include <linux/module.h>
@@ -411,36 +412,6 @@ int rockchip_drm_encoder_get_mux_id(struct device_node *node,
 }
 EXPORT_SYMBOL_GPL(rockchip_drm_encoder_get_mux_id);
 
-static int compare_of(struct device *dev, void *data)
-{
-	struct device_node *np = data;
-
-	return dev->of_node == np;
-}
-
-static void rockchip_add_endpoints(struct device *dev,
-				   struct component_match **match,
-				   struct device_node *port)
-{
-	struct device_node *ep, *remote;
-
-	for_each_child_of_node(port, ep) {
-		remote = of_graph_get_remote_port_parent(ep);
-		if (!remote || !of_device_is_available(remote)) {
-			of_node_put(remote);
-			continue;
-		} else if (!of_device_is_available(remote->parent)) {
-			dev_warn(dev, "parent device of %s is not available\n",
-				 remote->full_name);
-			of_node_put(remote);
-			continue;
-		}
-
-		component_match_add(dev, match, compare_of, remote);
-		of_node_put(remote);
-	}
-}
-
 static int rockchip_drm_bind(struct device *dev)
 {
 	struct drm_device *drm;
@@ -481,63 +452,30 @@ static const struct component_master_ops rockchip_drm_ops = {
 	.unbind = rockchip_drm_unbind,
 };
 
-static int rockchip_drm_platform_probe(struct platform_device *pdev)
+static int compare_port(struct device *dev, void *data)
 {
-	struct device *dev = &pdev->dev;
-	struct component_match *match = NULL;
-	struct device_node *np = dev->of_node;
-	struct device_node *port;
-	int i;
+	struct device_node *np = data;
 
-	if (!np)
-		return -ENODEV;
-	/*
-	 * Bind the crtc ports first, so that
-	 * drm_of_find_possible_crtcs called from encoder .bind callbacks
-	 * works as expected.
-	 */
-	for (i = 0;; i++) {
-		port = of_parse_phandle(np, "ports", i);
-		if (!port)
-			break;
-
-		if (!of_device_is_available(port->parent)) {
-			of_node_put(port);
-			continue;
-		}
+	return dev->of_node == np->parent;
+}
 
-		component_match_add(dev, &match, compare_of, port->parent);
-		of_node_put(port);
-	}
+static int compare_encoder(struct device *dev, void *data)
+{
+	struct device_node *np = data;
 
-	if (i == 0) {
-		dev_err(dev, "missing 'ports' property\n");
-		return -ENODEV;
-	}
+	return dev->of_node == np;
+}
 
-	if (!match) {
-		dev_err(dev, "No available vop found for display-subsystem.\n");
-		return -ENODEV;
-	}
-	/*
-	 * For each bound crtc, bind the encoders attached to its
-	 * remote endpoint.
-	 */
-	for (i = 0;; i++) {
-		port = of_parse_phandle(np, "ports", i);
-		if (!port)
-			break;
-
-		if (!of_device_is_available(port->parent)) {
-			of_node_put(port);
-			continue;
-		}
+static int rockchip_drm_platform_probe(struct platform_device *pdev)
+{
+	int ret = drm_of_component_probe(&pdev->dev, compare_port,
+					 compare_encoder, &rockchip_drm_ops);
 
-		rockchip_add_endpoints(dev, &match, port);
-		of_node_put(port);
-	}
+	/* keep compatibility with old code that was returning -ENODEV */
+	if (ret == -EINVAL)
+		return -ENODEV;
 
-	return component_master_add_with_match(dev, &rockchip_drm_ops, match);
+	return ret;
 }
 
 static int rockchip_drm_platform_remove(struct platform_device *pdev)
-- 
2.6.2

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

[PATCH v2 2/2] drm/rockchip: Convert the probe function to the generic drm_of_component_probe()

[Liviu Dudau]

Initial attempt to convert rockchip to drm_of_component_probe() missed the
difference between ports and encoders when using the compare_of() function.
Now that drm_of_component_probe() has been enhanced, let's try again the
conversion.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 98 ++++++-----------------------
 1 file changed, 18 insertions(+), 80 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f22e1e1..574324e 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -19,6 +19,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_of.h>
 #include <linux/dma-mapping.h>
 #include <linux/pm_runtime.h>
 #include <linux/module.h>
@@ -411,36 +412,6 @@ int rockchip_drm_encoder_get_mux_id(struct device_node *node,
 }
 EXPORT_SYMBOL_GPL(rockchip_drm_encoder_get_mux_id);
 
-static int compare_of(struct device *dev, void *data)
-{
-	struct device_node *np = data;
-
-	return dev->of_node == np;
-}
-
-static void rockchip_add_endpoints(struct device *dev,
-				   struct component_match **match,
-				   struct device_node *port)
-{
-	struct device_node *ep, *remote;
-
-	for_each_child_of_node(port, ep) {
-		remote = of_graph_get_remote_port_parent(ep);
-		if (!remote || !of_device_is_available(remote)) {
-			of_node_put(remote);
-			continue;
-		} else if (!of_device_is_available(remote->parent)) {
-			dev_warn(dev, "parent device of %s is not available\n",
-				 remote->full_name);
-			of_node_put(remote);
-			continue;
-		}
-
-		component_match_add(dev, match, compare_of, remote);
-		of_node_put(remote);
-	}
-}
-
 static int rockchip_drm_bind(struct device *dev)
 {
 	struct drm_device *drm;
@@ -481,63 +452,30 @@ static const struct component_master_ops rockchip_drm_ops = {
 	.unbind = rockchip_drm_unbind,
 };
 
-static int rockchip_drm_platform_probe(struct platform_device *pdev)
+static int compare_port(struct device *dev, void *data)
 {
-	struct device *dev = &pdev->dev;
-	struct component_match *match = NULL;
-	struct device_node *np = dev->of_node;
-	struct device_node *port;
-	int i;
+	struct device_node *np = data;
 
-	if (!np)
-		return -ENODEV;
-	/*
-	 * Bind the crtc ports first, so that
-	 * drm_of_find_possible_crtcs called from encoder .bind callbacks
-	 * works as expected.
-	 */
-	for (i = 0;; i++) {
-		port = of_parse_phandle(np, "ports", i);
-		if (!port)
-			break;
-
-		if (!of_device_is_available(port->parent)) {
-			of_node_put(port);
-			continue;
-		}
+	return dev->of_node == np->parent;
+}
 
-		component_match_add(dev, &match, compare_of, port->parent);
-		of_node_put(port);
-	}
+static int compare_encoder(struct device *dev, void *data)
+{
+	struct device_node *np = data;
 
-	if (i == 0) {
-		dev_err(dev, "missing 'ports' property\n");
-		return -ENODEV;
-	}
+	return dev->of_node == np;
+}
 
-	if (!match) {
-		dev_err(dev, "No available vop found for display-subsystem.\n");
-		return -ENODEV;
-	}
-	/*
-	 * For each bound crtc, bind the encoders attached to its
-	 * remote endpoint.
-	 */
-	for (i = 0;; i++) {
-		port = of_parse_phandle(np, "ports", i);
-		if (!port)
-			break;
-
-		if (!of_device_is_available(port->parent)) {
-			of_node_put(port);
-			continue;
-		}
+static int rockchip_drm_platform_probe(struct platform_device *pdev)
+{
+	int ret = drm_of_component_probe(&pdev->dev, compare_port,
+					 compare_encoder, &rockchip_drm_ops);
 
-		rockchip_add_endpoints(dev, &match, port);
-		of_node_put(port);
-	}
+	/* keep compatibility with old code that was returning -ENODEV */
+	if (ret == -EINVAL)
+		return -ENODEV;
 
-	return component_master_add_with_match(dev, &rockchip_drm_ops, match);
+	return ret;
 }
 
 static int rockchip_drm_platform_remove(struct platform_device *pdev)
-- 
2.6.2

Re: [PATCH v2 2/2] drm/rockchip: Convert the probe function to the generic drm_of_component_probe()

[Mark yao]

On 2015年11月20日 22:22, Liviu Dudau wrote:
> Initial attempt to convert rockchip to drm_of_component_probe() missed the
> difference between ports and encoders when using the compare_of() function.
> Now that drm_of_component_probe() has been enhanced, let's try again the
> conversion.
>
> Signed-off-by: Liviu Dudau<Liviu.Dudau@arm.com>

Looks good for me, and it works on popmetal board, so
      Acked-by: Mark Yao <mark.yao@rock-chips.com>

Thanks.

-- 
Ｍark Yao

[PATCH v2 2/2] drm/rockchip: Convert the probe function to the generic drm_of_component_probe()

[Mark yao]

On 2015?11?20? 22:22, Liviu Dudau wrote:
> Initial attempt to convert rockchip to drm_of_component_probe() missed the
> difference between ports and encoders when using the compare_of() function.
> Now that drm_of_component_probe() has been enhanced, let's try again the
> conversion.
>
> Signed-off-by: Liviu Dudau<Liviu.Dudau@arm.com>

Looks good for me, and it works on popmetal board, so
      Acked-by: Mark Yao <mark.yao@rock-chips.com>

Thanks.

-- 
?ark Yao

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Heiko Stübner]

Am Freitag, 20. November 2015, 14:22:03 schrieb Liviu Dudau:
> Hello,
> 
> This is v2 of the patchset trying to make drm_of_component_probe() cope with
> finding both local crtc ports and remote encoder ones. Heiko Stübner was
> nice enough to test an earlier version that was patched following Russell's
> suggestions on rk3288, but I haven't seen any reports from iMX or Armada
> users.
> 
> Changelog:
>  v2: Updated the drm_of_component_probe() comment to explain why the
> reference count is not dropped. Fixed the compare_port() function for
> rockchip as described by Russell.

Now works nicely on my rk3288-veyron-jerry again.

Tested-by: Heiko Stuebner <heiko@sntech.de>


Thanks
Heiko

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Heiko Stübner]

Am Freitag, 20. November 2015, 14:22:03 schrieb Liviu Dudau:
> Hello,
> 
> This is v2 of the patchset trying to make drm_of_component_probe() cope with
> finding both local crtc ports and remote encoder ones. Heiko Stübner was
> nice enough to test an earlier version that was patched following Russell's
> suggestions on rk3288, but I haven't seen any reports from iMX or Armada
> users.
> 
> Changelog:
>  v2: Updated the drm_of_component_probe() comment to explain why the
> reference count is not dropped. Fixed the compare_port() function for
> rockchip as described by Russell.

Now works nicely on my rk3288-veyron-jerry again.

Tested-by: Heiko Stuebner <heiko@sntech.de>


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

[PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Heiko Stübner]

Am Freitag, 20. November 2015, 14:22:03 schrieb Liviu Dudau:
> Hello,
> 
> This is v2 of the patchset trying to make drm_of_component_probe() cope with
> finding both local crtc ports and remote encoder ones. Heiko St?bner was
> nice enough to test an earlier version that was patched following Russell's
> suggestions on rk3288, but I haven't seen any reports from iMX or Armada
> users.
> 
> Changelog:
>  v2: Updated the drm_of_component_probe() comment to explain why the
> reference count is not dropped. Fixed the compare_port() function for
> rockchip as described by Russell.

Now works nicely on my rk3288-veyron-jerry again.

Tested-by: Heiko Stuebner <heiko@sntech.de>


Thanks
Heiko

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Heiko Stübner]

Hi Dave,

Am Freitag, 20. November 2015, 14:22:03 schrieb Liviu Dudau:
> This is v2 of the patchset trying to make drm_of_component_probe() cope with
> finding both local crtc ports and remote encoder ones. Heiko Stübner was
> nice enough to test an earlier version that was patched following Russell's
> suggestions on rk3288, but I haven't seen any reports from iMX or Armada
> users.

these 2 patches seem to work nicely now and can probably get applied. Do you 
want to pick them up, or do you expect Mark to do this and then include them 
in a pull request?


Thanks
Heiko

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Heiko Stübner]

Hi Dave,

Am Freitag, 20. November 2015, 14:22:03 schrieb Liviu Dudau:
> This is v2 of the patchset trying to make drm_of_component_probe() cope with
> finding both local crtc ports and remote encoder ones. Heiko Stübner was
> nice enough to test an earlier version that was patched following Russell's
> suggestions on rk3288, but I haven't seen any reports from iMX or Armada
> users.

these 2 patches seem to work nicely now and can probably get applied. Do you 
want to pick them up, or do you expect Mark to do this and then include them 
in a pull request?


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

[PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Heiko Stübner]

Hi Dave,

Am Freitag, 20. November 2015, 14:22:03 schrieb Liviu Dudau:
> This is v2 of the patchset trying to make drm_of_component_probe() cope with
> finding both local crtc ports and remote encoder ones. Heiko St?bner was
> nice enough to test an earlier version that was patched following Russell's
> suggestions on rk3288, but I haven't seen any reports from iMX or Armada
> users.

these 2 patches seem to work nicely now and can probably get applied. Do you 
want to pick them up, or do you expect Mark to do this and then include them 
in a pull request?


Thanks
Heiko

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Liviu Dudau]

On Fri, Nov 20, 2015 at 02:22:03PM +0000, Liviu Dudau wrote:
> Hello,
> 
> This is v2 of the patchset trying to make drm_of_component_probe() cope with finding
> both local crtc ports and remote encoder ones. Heiko Stübner was nice enough to test
> an earlier version that was patched following Russell's suggestions on rk3288, but
> I haven't seen any reports from iMX or Armada users.
> 
> Changelog:
>  v2: Updated the drm_of_component_probe() comment to explain why the reference count
>      is not dropped. Fixed the compare_port() function for rockchip as described by
>      Russell.
>  v1: Original submission. http://lists.freedesktop.org/archives/dri-devel/2015-November/094546.html

Gentle ping, this has now been tested by Rockchip people and fixes the earlier version
that had to be reverted in mainline. Can it be included in the -next somewhere?

Many thanks,
Liviu

> 
> Liviu Dudau (2):
>   drm: Improve drm_of_component_probe() to correctly handle ports and remote ports.
>   drm/rockchip: Convert the probe function to the generic drm_of_component_probe()
> 
>  drivers/gpu/drm/armada/armada_drv.c         |  3 +-
>  drivers/gpu/drm/drm_of.c                    | 19 ++++--
>  drivers/gpu/drm/imx/imx-drm-core.c          |  3 +-
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 98 ++++++-----------------------
>  include/drm/drm_of.h                        |  6 +-
>  5 files changed, 40 insertions(+), 89 deletions(-)
> 
> -- 
> 2.6.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

[PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Liviu Dudau]

On Fri, Nov 20, 2015 at 02:22:03PM +0000, Liviu Dudau wrote:
> Hello,
> 
> This is v2 of the patchset trying to make drm_of_component_probe() cope with finding
> both local crtc ports and remote encoder ones. Heiko St?bner was nice enough to test
> an earlier version that was patched following Russell's suggestions on rk3288, but
> I haven't seen any reports from iMX or Armada users.
> 
> Changelog:
>  v2: Updated the drm_of_component_probe() comment to explain why the reference count
>      is not dropped. Fixed the compare_port() function for rockchip as described by
>      Russell.
>  v1: Original submission. http://lists.freedesktop.org/archives/dri-devel/2015-November/094546.html

Gentle ping, this has now been tested by Rockchip people and fixes the earlier version
that had to be reverted in mainline. Can it be included in the -next somewhere?

Many thanks,
Liviu

> 
> Liviu Dudau (2):
>   drm: Improve drm_of_component_probe() to correctly handle ports and remote ports.
>   drm/rockchip: Convert the probe function to the generic drm_of_component_probe()
> 
>  drivers/gpu/drm/armada/armada_drv.c         |  3 +-
>  drivers/gpu/drm/drm_of.c                    | 19 ++++--
>  drivers/gpu/drm/imx/imx-drm-core.c          |  3 +-
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 98 ++++++-----------------------
>  include/drm/drm_of.h                        |  6 +-
>  5 files changed, 40 insertions(+), 89 deletions(-)
> 
> -- 
> 2.6.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Dave Airlie]

On 23 December 2015 at 03:38, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> On Fri, Nov 20, 2015 at 02:22:03PM +0000, Liviu Dudau wrote:
>> Hello,
>>
>> This is v2 of the patchset trying to make drm_of_component_probe() cope with finding
>> both local crtc ports and remote encoder ones. Heiko Stübner was nice enough to test
>> an earlier version that was patched following Russell's suggestions on rk3288, but
>> I haven't seen any reports from iMX or Armada users.
>>
>> Changelog:
>>  v2: Updated the drm_of_component_probe() comment to explain why the reference count
>>      is not dropped. Fixed the compare_port() function for rockchip as described by
>>      Russell.
>>  v1: Original submission. http://lists.freedesktop.org/archives/dri-devel/2015-November/094546.html
>
> Gentle ping, this has now been tested by Rockchip people and fixes the earlier version
> that had to be reverted in mainline. Can it be included in the -next somewhere?

It would be good to get Russell ack on the first one, especially after
reading the previous thread.

Dave.

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Dave Airlie]

On 23 December 2015 at 03:38, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> On Fri, Nov 20, 2015 at 02:22:03PM +0000, Liviu Dudau wrote:
>> Hello,
>>
>> This is v2 of the patchset trying to make drm_of_component_probe() cope with finding
>> both local crtc ports and remote encoder ones. Heiko Stübner was nice enough to test
>> an earlier version that was patched following Russell's suggestions on rk3288, but
>> I haven't seen any reports from iMX or Armada users.
>>
>> Changelog:
>>  v2: Updated the drm_of_component_probe() comment to explain why the reference count
>>      is not dropped. Fixed the compare_port() function for rockchip as described by
>>      Russell.
>>  v1: Original submission. http://lists.freedesktop.org/archives/dri-devel/2015-November/094546.html
>
> Gentle ping, this has now been tested by Rockchip people and fixes the earlier version
> that had to be reverted in mainline. Can it be included in the -next somewhere?

It would be good to get Russell ack on the first one, especially after
reading the previous thread.

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

[PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Dave Airlie]

On 23 December 2015 at 03:38, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> On Fri, Nov 20, 2015 at 02:22:03PM +0000, Liviu Dudau wrote:
>> Hello,
>>
>> This is v2 of the patchset trying to make drm_of_component_probe() cope with finding
>> both local crtc ports and remote encoder ones. Heiko St?bner was nice enough to test
>> an earlier version that was patched following Russell's suggestions on rk3288, but
>> I haven't seen any reports from iMX or Armada users.
>>
>> Changelog:
>>  v2: Updated the drm_of_component_probe() comment to explain why the reference count
>>      is not dropped. Fixed the compare_port() function for rockchip as described by
>>      Russell.
>>  v1: Original submission. http://lists.freedesktop.org/archives/dri-devel/2015-November/094546.html
>
> Gentle ping, this has now been tested by Rockchip people and fixes the earlier version
> that had to be reverted in mainline. Can it be included in the -next somewhere?

It would be good to get Russell ack on the first one, especially after
reading the previous thread.

Dave.

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Jean-Francois Moine]

On Tue, 22 Dec 2015 17:38:00 +0000
Liviu Dudau <Liviu.Dudau@arm.com> wrote:

> On Fri, Nov 20, 2015 at 02:22:03PM +0000, Liviu Dudau wrote:
> > Hello,
> > 
> > This is v2 of the patchset trying to make drm_of_component_probe() cope with finding
> > both local crtc ports and remote encoder ones. Heiko Stübner was nice enough to test
> > an earlier version that was patched following Russell's suggestions on rk3288, but
> > I haven't seen any reports from iMX or Armada users.
> > 
> > Changelog:
> >  v2: Updated the drm_of_component_probe() comment to explain why the reference count
> >      is not dropped. Fixed the compare_port() function for rockchip as described by
> >      Russell.
> >  v1: Original submission. http://lists.freedesktop.org/archives/dri-devel/2015-November/094546.html
> 
> Gentle ping, this has now been tested by Rockchip people and fixes the earlier version
> that had to be reverted in mainline. Can it be included in the -next somewhere?

Hi Liviu,

Sorry for being a bit late.

I wanted to use drm_of_component_probe() for a new DRM driver, but I
could not find any way to do it: you add the "ports" nodes as
components while, usually, the components are the device nodes
themselves.

With this simple patch:

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 493c05c..dbd2921 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -101,7 +101,7 @@ int drm_of_component_probe(struct device *dev,
 			continue;
 		}
 
-		component_match_add(dev, &match, compare_of, port);
+		component_match_add(dev, &match, compare_of, port->parent);
 		of_node_put(port);
 	}
 
everything is easy, my DT being like:

	de_controller {
		...
		ports = <&lcd0_p>;
	};

	lcd_controller {
		...
		lcd0_p: port {
			lcd0_ep: endpoint {
				remote-endpoint = <&hdmi_ep>;
			};
		};
	};

What was the reason to keep the "ports" node instead of the device?

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Jean-Francois Moine]

On Tue, 22 Dec 2015 17:38:00 +0000
Liviu Dudau <Liviu.Dudau@arm.com> wrote:

> On Fri, Nov 20, 2015 at 02:22:03PM +0000, Liviu Dudau wrote:
> > Hello,
> > 
> > This is v2 of the patchset trying to make drm_of_component_probe() cope with finding
> > both local crtc ports and remote encoder ones. Heiko Stübner was nice enough to test
> > an earlier version that was patched following Russell's suggestions on rk3288, but
> > I haven't seen any reports from iMX or Armada users.
> > 
> > Changelog:
> >  v2: Updated the drm_of_component_probe() comment to explain why the reference count
> >      is not dropped. Fixed the compare_port() function for rockchip as described by
> >      Russell.
> >  v1: Original submission. http://lists.freedesktop.org/archives/dri-devel/2015-November/094546.html
> 
> Gentle ping, this has now been tested by Rockchip people and fixes the earlier version
> that had to be reverted in mainline. Can it be included in the -next somewhere?

Hi Liviu,

Sorry for being a bit late.

I wanted to use drm_of_component_probe() for a new DRM driver, but I
could not find any way to do it: you add the "ports" nodes as
components while, usually, the components are the device nodes
themselves.

With this simple patch:

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 493c05c..dbd2921 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -101,7 +101,7 @@ int drm_of_component_probe(struct device *dev,
 			continue;
 		}
 
-		component_match_add(dev, &match, compare_of, port);
+		component_match_add(dev, &match, compare_of, port->parent);
 		of_node_put(port);
 	}
 
everything is easy, my DT being like:

	de_controller {
		...
		ports = <&lcd0_p>;
	};

	lcd_controller {
		...
		lcd0_p: port {
			lcd0_ep: endpoint {
				remote-endpoint = <&hdmi_ep>;
			};
		};
	};

What was the reason to keep the "ports" node instead of the device?

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Jean-Francois Moine]

On Tue, 22 Dec 2015 17:38:00 +0000
Liviu Dudau <Liviu.Dudau@arm.com> wrote:

> On Fri, Nov 20, 2015 at 02:22:03PM +0000, Liviu Dudau wrote:
> > Hello,
> > 
> > This is v2 of the patchset trying to make drm_of_component_probe() cope with finding
> > both local crtc ports and remote encoder ones. Heiko St?bner was nice enough to test
> > an earlier version that was patched following Russell's suggestions on rk3288, but
> > I haven't seen any reports from iMX or Armada users.
> > 
> > Changelog:
> >  v2: Updated the drm_of_component_probe() comment to explain why the reference count
> >      is not dropped. Fixed the compare_port() function for rockchip as described by
> >      Russell.
> >  v1: Original submission. http://lists.freedesktop.org/archives/dri-devel/2015-November/094546.html
> 
> Gentle ping, this has now been tested by Rockchip people and fixes the earlier version
> that had to be reverted in mainline. Can it be included in the -next somewhere?

Hi Liviu,

Sorry for being a bit late.

I wanted to use drm_of_component_probe() for a new DRM driver, but I
could not find any way to do it: you add the "ports" nodes as
components while, usually, the components are the device nodes
themselves.

With this simple patch:

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 493c05c..dbd2921 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -101,7 +101,7 @@ int drm_of_component_probe(struct device *dev,
 			continue;
 		}
 
-		component_match_add(dev, &match, compare_of, port);
+		component_match_add(dev, &match, compare_of, port->parent);
 		of_node_put(port);
 	}
 
everything is easy, my DT being like:

	de_controller {
		...
		ports = <&lcd0_p>;
	};

	lcd_controller {
		...
		lcd0_p: port {
			lcd0_ep: endpoint {
				remote-endpoint = <&hdmi_ep>;
			};
		};
	};

What was the reason to keep the "ports" node instead of the device?

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Liviu Dudau]

On Wed, Dec 23, 2015 at 10:39:06AM +0100, Jean-Francois Moine wrote:
> On Tue, 22 Dec 2015 17:38:00 +0000
> Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> 
> > On Fri, Nov 20, 2015 at 02:22:03PM +0000, Liviu Dudau wrote:
> > > Hello,
> > > 
> > > This is v2 of the patchset trying to make drm_of_component_probe() cope with finding
> > > both local crtc ports and remote encoder ones. Heiko Stübner was nice enough to test
> > > an earlier version that was patched following Russell's suggestions on rk3288, but
> > > I haven't seen any reports from iMX or Armada users.
> > > 
> > > Changelog:
> > >  v2: Updated the drm_of_component_probe() comment to explain why the reference count
> > >      is not dropped. Fixed the compare_port() function for rockchip as described by
> > >      Russell.
> > >  v1: Original submission. http://lists.freedesktop.org/archives/dri-devel/2015-November/094546.html
> > 
> > Gentle ping, this has now been tested by Rockchip people and fixes the earlier version
> > that had to be reverted in mainline. Can it be included in the -next somewhere?
> 
> Hi Liviu,
> 
> Sorry for being a bit late.
> 
> I wanted to use drm_of_component_probe() for a new DRM driver, but I
> could not find any way to do it: you add the "ports" nodes as
> components while, usually, the components are the device nodes
> themselves.
> 
> With this simple patch:
> 
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 493c05c..dbd2921 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -101,7 +101,7 @@ int drm_of_component_probe(struct device *dev,
>  			continue;
>  		}
>  
> -		component_match_add(dev, &match, compare_of, port);
> +		component_match_add(dev, &match, compare_of, port->parent);
>  		of_node_put(port);
>  	}
>  
> everything is easy, my DT being like:
> 
> 	de_controller {
> 		...
> 		ports = <&lcd0_p>;
> 	};
> 
> 	lcd_controller {
> 		...
> 		lcd0_p: port {
> 			lcd0_ep: endpoint {
> 				remote-endpoint = <&hdmi_ep>;
> 			};
> 		};
> 	};
> 
> What was the reason to keep the "ports" node instead of the device?

The function is an extract of common code sprinkled through a few DRM drivers,
they all used port rather than port->parent.

Have a look at my v2 where I've introduced two compare functions and also
modified the Rockchip compare_port() to use port->parent in the comparison. I
guess that should solve your problem.

Best regards,
Liviu

> 
> -- 
> Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
> Jef		|		http://moinejf.free.fr/
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Liviu Dudau]

On Wed, Dec 23, 2015 at 10:39:06AM +0100, Jean-Francois Moine wrote:
> On Tue, 22 Dec 2015 17:38:00 +0000
> Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> 
> > On Fri, Nov 20, 2015 at 02:22:03PM +0000, Liviu Dudau wrote:
> > > Hello,
> > > 
> > > This is v2 of the patchset trying to make drm_of_component_probe() cope with finding
> > > both local crtc ports and remote encoder ones. Heiko Stübner was nice enough to test
> > > an earlier version that was patched following Russell's suggestions on rk3288, but
> > > I haven't seen any reports from iMX or Armada users.
> > > 
> > > Changelog:
> > >  v2: Updated the drm_of_component_probe() comment to explain why the reference count
> > >      is not dropped. Fixed the compare_port() function for rockchip as described by
> > >      Russell.
> > >  v1: Original submission. http://lists.freedesktop.org/archives/dri-devel/2015-November/094546.html
> > 
> > Gentle ping, this has now been tested by Rockchip people and fixes the earlier version
> > that had to be reverted in mainline. Can it be included in the -next somewhere?
> 
> Hi Liviu,
> 
> Sorry for being a bit late.
> 
> I wanted to use drm_of_component_probe() for a new DRM driver, but I
> could not find any way to do it: you add the "ports" nodes as
> components while, usually, the components are the device nodes
> themselves.
> 
> With this simple patch:
> 
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 493c05c..dbd2921 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -101,7 +101,7 @@ int drm_of_component_probe(struct device *dev,
>  			continue;
>  		}
>  
> -		component_match_add(dev, &match, compare_of, port);
> +		component_match_add(dev, &match, compare_of, port->parent);
>  		of_node_put(port);
>  	}
>  
> everything is easy, my DT being like:
> 
> 	de_controller {
> 		...
> 		ports = <&lcd0_p>;
> 	};
> 
> 	lcd_controller {
> 		...
> 		lcd0_p: port {
> 			lcd0_ep: endpoint {
> 				remote-endpoint = <&hdmi_ep>;
> 			};
> 		};
> 	};
> 
> What was the reason to keep the "ports" node instead of the device?

The function is an extract of common code sprinkled through a few DRM drivers,
they all used port rather than port->parent.

Have a look at my v2 where I've introduced two compare functions and also
modified the Rockchip compare_port() to use port->parent in the comparison. I
guess that should solve your problem.

Best regards,
Liviu

> 
> -- 
> Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
> Jef		|		http://moinejf.free.fr/
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Liviu Dudau]

On Wed, Dec 23, 2015 at 10:39:06AM +0100, Jean-Francois Moine wrote:
> On Tue, 22 Dec 2015 17:38:00 +0000
> Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> 
> > On Fri, Nov 20, 2015 at 02:22:03PM +0000, Liviu Dudau wrote:
> > > Hello,
> > > 
> > > This is v2 of the patchset trying to make drm_of_component_probe() cope with finding
> > > both local crtc ports and remote encoder ones. Heiko St?bner was nice enough to test
> > > an earlier version that was patched following Russell's suggestions on rk3288, but
> > > I haven't seen any reports from iMX or Armada users.
> > > 
> > > Changelog:
> > >  v2: Updated the drm_of_component_probe() comment to explain why the reference count
> > >      is not dropped. Fixed the compare_port() function for rockchip as described by
> > >      Russell.
> > >  v1: Original submission. http://lists.freedesktop.org/archives/dri-devel/2015-November/094546.html
> > 
> > Gentle ping, this has now been tested by Rockchip people and fixes the earlier version
> > that had to be reverted in mainline. Can it be included in the -next somewhere?
> 
> Hi Liviu,
> 
> Sorry for being a bit late.
> 
> I wanted to use drm_of_component_probe() for a new DRM driver, but I
> could not find any way to do it: you add the "ports" nodes as
> components while, usually, the components are the device nodes
> themselves.
> 
> With this simple patch:
> 
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 493c05c..dbd2921 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -101,7 +101,7 @@ int drm_of_component_probe(struct device *dev,
>  			continue;
>  		}
>  
> -		component_match_add(dev, &match, compare_of, port);
> +		component_match_add(dev, &match, compare_of, port->parent);
>  		of_node_put(port);
>  	}
>  
> everything is easy, my DT being like:
> 
> 	de_controller {
> 		...
> 		ports = <&lcd0_p>;
> 	};
> 
> 	lcd_controller {
> 		...
> 		lcd0_p: port {
> 			lcd0_ep: endpoint {
> 				remote-endpoint = <&hdmi_ep>;
> 			};
> 		};
> 	};
> 
> What was the reason to keep the "ports" node instead of the device?

The function is an extract of common code sprinkled through a few DRM drivers,
they all used port rather than port->parent.

Have a look at my v2 where I've introduced two compare functions and also
modified the Rockchip compare_port() to use port->parent in the comparison. I
guess that should solve your problem.

Best regards,
Liviu

> 
> -- 
> Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
> Jef		|		http://moinejf.free.fr/
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Jean-Francois Moine]

On Wed, 23 Dec 2015 10:05:34 +0000
Liviu Dudau <Liviu.Dudau@arm.com> wrote:

> > What was the reason to keep the "ports" node instead of the device?
> 
> The function is an extract of common code sprinkled through a few DRM drivers,
> they all used port rather than port->parent.

Sorry for I could find such drivers. May you give me any pointer?

> Have a look at my v2 where I've introduced two compare functions and also
> modified the Rockchip compare_port() to use port->parent in the comparison. I
> guess that should solve your problem.

Keeping the port instead of the parent asks for more code, but,
especially, it also asks for changes in the component drivers because,
at bind time, in 'data', they get a port instead of the device.

You might say that this could be interesting for components with many
different masters (video and audio), but this could be solved adding
intermediate device nodes in the DT (ports).

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Jean-Francois Moine]

On Wed, 23 Dec 2015 10:05:34 +0000
Liviu Dudau <Liviu.Dudau@arm.com> wrote:

> > What was the reason to keep the "ports" node instead of the device?
> 
> The function is an extract of common code sprinkled through a few DRM drivers,
> they all used port rather than port->parent.

Sorry for I could find such drivers. May you give me any pointer?

> Have a look at my v2 where I've introduced two compare functions and also
> modified the Rockchip compare_port() to use port->parent in the comparison. I
> guess that should solve your problem.

Keeping the port instead of the parent asks for more code, but,
especially, it also asks for changes in the component drivers because,
at bind time, in 'data', they get a port instead of the device.

You might say that this could be interesting for components with many
different masters (video and audio), but this could be solved adding
intermediate device nodes in the DT (ports).

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Jean-Francois Moine]

On Wed, 23 Dec 2015 10:05:34 +0000
Liviu Dudau <Liviu.Dudau@arm.com> wrote:

> > What was the reason to keep the "ports" node instead of the device?
> 
> The function is an extract of common code sprinkled through a few DRM drivers,
> they all used port rather than port->parent.

Sorry for I could find such drivers. May you give me any pointer?

> Have a look at my v2 where I've introduced two compare functions and also
> modified the Rockchip compare_port() to use port->parent in the comparison. I
> guess that should solve your problem.

Keeping the port instead of the parent asks for more code, but,
especially, it also asks for changes in the component drivers because,
at bind time, in 'data', they get a port instead of the device.

You might say that this could be interesting for components with many
different masters (video and audio), but this could be solved adding
intermediate device nodes in the DT (ports).

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Russell King - ARM Linux]

On Wed, Dec 23, 2015 at 06:20:33PM +0100, Jean-Francois Moine wrote:
> On Wed, 23 Dec 2015 10:05:34 +0000
> Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> 
> > > What was the reason to keep the "ports" node instead of the device?
> > 
> > The function is an extract of common code sprinkled through a few DRM drivers,
> > they all used port rather than port->parent.
> 
> Sorry for I could find such drivers. May you give me any pointer?

imx-drm probably.

> > Have a look at my v2 where I've introduced two compare functions and also
> > modified the Rockchip compare_port() to use port->parent in the comparison. I
> > guess that should solve your problem.
> 
> Keeping the port instead of the parent asks for more code, but,
> especially, it also asks for changes in the component drivers because,
> at bind time, in 'data', they get a port instead of the device.

Sorry, this doesn't make sense.  You have far too many sub-clauses
which mean nothing at all.  Please rephrase.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Russell King - ARM Linux]

On Wed, Dec 23, 2015 at 06:20:33PM +0100, Jean-Francois Moine wrote:
> On Wed, 23 Dec 2015 10:05:34 +0000
> Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> 
> > > What was the reason to keep the "ports" node instead of the device?
> > 
> > The function is an extract of common code sprinkled through a few DRM drivers,
> > they all used port rather than port->parent.
> 
> Sorry for I could find such drivers. May you give me any pointer?

imx-drm probably.

> > Have a look at my v2 where I've introduced two compare functions and also
> > modified the Rockchip compare_port() to use port->parent in the comparison. I
> > guess that should solve your problem.
> 
> Keeping the port instead of the parent asks for more code, but,
> especially, it also asks for changes in the component drivers because,
> at bind time, in 'data', they get a port instead of the device.

Sorry, this doesn't make sense.  You have far too many sub-clauses
which mean nothing at all.  Please rephrase.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Russell King - ARM Linux]

On Wed, Dec 23, 2015 at 06:20:33PM +0100, Jean-Francois Moine wrote:
> On Wed, 23 Dec 2015 10:05:34 +0000
> Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> 
> > > What was the reason to keep the "ports" node instead of the device?
> > 
> > The function is an extract of common code sprinkled through a few DRM drivers,
> > they all used port rather than port->parent.
> 
> Sorry for I could find such drivers. May you give me any pointer?

imx-drm probably.

> > Have a look at my v2 where I've introduced two compare functions and also
> > modified the Rockchip compare_port() to use port->parent in the comparison. I
> > guess that should solve your problem.
> 
> Keeping the port instead of the parent asks for more code, but,
> especially, it also asks for changes in the component drivers because,
> at bind time, in 'data', they get a port instead of the device.

Sorry, this doesn't make sense.  You have far too many sub-clauses
which mean nothing at all.  Please rephrase.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Jean-Francois Moine]

On Wed, 23 Dec 2015 18:59:48 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> > > Have a look at my v2 where I've introduced two compare functions and also
> > > modified the Rockchip compare_port() to use port->parent in the comparison. I
> > > guess that should solve your problem.
> > 
> > Keeping the port instead of the parent asks for more code, but,
> > especially, it also asks for changes in the component drivers because,
> > at bind time, in 'data', they get a port instead of the device.
> 
> Sorry, this doesn't make sense.  You have far too many sub-clauses
> which mean nothing at all.  Please rephrase.

Well, two topics:

- adding a second 'of_compare' function complexifies the code
  and people may wonder why such a function is needed and what
  they have to put inside.

- usually, the component drivers just do a component_add() of the device
  at probe time.
  Now, as the bind() function of the components of the first level
  returns the port in 'data', some work has to be done for retrieving
  the device.
  This can (should?) be done in the bind() function.
  In drm/imx/ipuv3-crtc.c, this is done by a hack, changing the device
  node reference before calling component_add()!

I looked at the imx-drm and the associated DTs, and I think that,
without the v2 patch and keeping the port parent as the component
(previous mail), the code could be simplified adding an intermediate
device node in the DT.

For example, in imx6qdl.dtsi:

	ipu1: ipu@02400000 {
		...
		ports@2 {			/* di<x> device */
			ipu1_di0: port {
				...
				ipu1_di0_hdmi: endpoint@1 {
					remote-endpoint = <&hdmi_mux_0>;
				};
				...
			};
		};
	}

In the code, the ipu driver searches the 'ports' and adds them as components.
After binding, the devices are the 'ports'.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Jean-Francois Moine]

On Wed, 23 Dec 2015 18:59:48 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> > > Have a look at my v2 where I've introduced two compare functions and also
> > > modified the Rockchip compare_port() to use port->parent in the comparison. I
> > > guess that should solve your problem.
> > 
> > Keeping the port instead of the parent asks for more code, but,
> > especially, it also asks for changes in the component drivers because,
> > at bind time, in 'data', they get a port instead of the device.
> 
> Sorry, this doesn't make sense.  You have far too many sub-clauses
> which mean nothing at all.  Please rephrase.

Well, two topics:

- adding a second 'of_compare' function complexifies the code
  and people may wonder why such a function is needed and what
  they have to put inside.

- usually, the component drivers just do a component_add() of the device
  at probe time.
  Now, as the bind() function of the components of the first level
  returns the port in 'data', some work has to be done for retrieving
  the device.
  This can (should?) be done in the bind() function.
  In drm/imx/ipuv3-crtc.c, this is done by a hack, changing the device
  node reference before calling component_add()!

I looked at the imx-drm and the associated DTs, and I think that,
without the v2 patch and keeping the port parent as the component
(previous mail), the code could be simplified adding an intermediate
device node in the DT.

For example, in imx6qdl.dtsi:

	ipu1: ipu@02400000 {
		...
		ports@2 {			/* di<x> device */
			ipu1_di0: port {
				...
				ipu1_di0_hdmi: endpoint@1 {
					remote-endpoint = <&hdmi_mux_0>;
				};
				...
			};
		};
	}

In the code, the ipu driver searches the 'ports' and adds them as components.
After binding, the devices are the 'ports'.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Jean-Francois Moine]

On Wed, 23 Dec 2015 18:59:48 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> > > Have a look at my v2 where I've introduced two compare functions and also
> > > modified the Rockchip compare_port() to use port->parent in the comparison. I
> > > guess that should solve your problem.
> > 
> > Keeping the port instead of the parent asks for more code, but,
> > especially, it also asks for changes in the component drivers because,
> > at bind time, in 'data', they get a port instead of the device.
> 
> Sorry, this doesn't make sense.  You have far too many sub-clauses
> which mean nothing at all.  Please rephrase.

Well, two topics:

- adding a second 'of_compare' function complexifies the code
  and people may wonder why such a function is needed and what
  they have to put inside.

- usually, the component drivers just do a component_add() of the device
  at probe time.
  Now, as the bind() function of the components of the first level
  returns the port in 'data', some work has to be done for retrieving
  the device.
  This can (should?) be done in the bind() function.
  In drm/imx/ipuv3-crtc.c, this is done by a hack, changing the device
  node reference before calling component_add()!

I looked at the imx-drm and the associated DTs, and I think that,
without the v2 patch and keeping the port parent as the component
(previous mail), the code could be simplified adding an intermediate
device node in the DT.

For example, in imx6qdl.dtsi:

	ipu1: ipu at 02400000 {
		...
		ports at 2 {			/* di<x> device */
			ipu1_di0: port {
				...
				ipu1_di0_hdmi: endpoint at 1 {
					remote-endpoint = <&hdmi_mux_0>;
				};
				...
			};
		};
	}

In the code, the ipu driver searches the 'ports' and adds them as components.
After binding, the devices are the 'ports'.

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Russell King - ARM Linux]

On Thu, Dec 24, 2015 at 09:15:28AM +0100, Jean-Francois Moine wrote:
> On Wed, 23 Dec 2015 18:59:48 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > > > Have a look at my v2 where I've introduced two compare functions and also
> > > > modified the Rockchip compare_port() to use port->parent in the comparison. I
> > > > guess that should solve your problem.
> > > 
> > > Keeping the port instead of the parent asks for more code, but,
> > > especially, it also asks for changes in the component drivers because,
> > > at bind time, in 'data', they get a port instead of the device.
> > 
> > Sorry, this doesn't make sense.  You have far too many sub-clauses
> > which mean nothing at all.  Please rephrase.
> 
> Well, two topics:
> 
> - adding a second 'of_compare' function complexifies the code
>   and people may wonder why such a function is needed and what
>   they have to put inside.
> 
> - usually, the component drivers just do a component_add() of the device
>   at probe time.

... which is exactly what does happen throughout imx-drm.

>   Now, as the bind() function of the components of the first level
>   returns the port in 'data', some work has to be done for retrieving
>   the device.
>   This can (should?) be done in the bind() function.

Sorry, this still makes zero sense to me.  "retrieving the device"
is all done by the core component code and has nothing to do with
the drivers themselves.

>   In drm/imx/ipuv3-crtc.c, this is done by a hack, changing the device
>   node reference before calling component_add()!

What hack?

static int ipu_drm_probe(struct platform_device *pdev)
{
        struct device *dev = &pdev->dev;
        int ret;

        if (!dev->platform_data)
                return -EINVAL;

        ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
        if (ret)
                return ret;

        return component_add(dev, &ipu_crtc_ops);
}

There's no hack there.  I see nothing changing dev->of_node there.

> I looked at the imx-drm and the associated DTs, and I think that,
> without the v2 patch and keeping the port parent as the component
> (previous mail), the code could be simplified adding an intermediate
> device node in the DT.

Not going to happen, because that's going to break compatibility with
existing DTs.

Let me explain instead what's going on, and why imx-drm is different.

The iMX DT files describe the hardware, which is a very complex block.
The IPU as a whole in DT, with its external interfaces.  The IPU driver
lives in drivers/gpu/ipu-v3/.

The hardware is a single controller (aka IPU - image processing unit)
which consists of many sub-blocks of hardware.  Two of these blocks
are display controllers with associated display interfaces.  These
_can_ be programmed to behave as a CRTC, but they're essentially just
waveform generators.  There's other blocks, including camera interfaces.

We _choose_ in Linux to have the IPU driver create several different
platform devices, one for each of its ports, whether it's a camera
interface or a display interface.  These platform devices are bound
to the IPU's port DT nodes.

Some iMX chips have two IPUs.  This means there can be a total of four
display outputs.

On the display bridge side, display bridges can be configured via
muxes to be connected to any of these display outputs.  Several
display bridges can even be connected to a single display output,
though this is not done in practise.

DT fully describes these links between the display outputs and
display bridges using the OF graph support.  From the DT point of
view, this is all very elegant and correct to the hardware structure.

However, when we come to the Linux implementation, things get sticky
because we need to select the correct platform device corresponding
with the IPU's port.  This can only be done using the 'port' node
and not port->parent.

port->parent would be the IPU device node itself.  If we were to
introduce the additional ports {} node, that doesn't help, because
now port->parent points at the ports {} node instead, not the actual
port - and we need the port itself to identify which of the IPU's
own created platform devices to select.

So, modifying DT doesn't help in any way, even if you ignore the fact
that we need to maintain backwards compatibility.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Russell King - ARM Linux]

On Thu, Dec 24, 2015 at 09:15:28AM +0100, Jean-Francois Moine wrote:
> On Wed, 23 Dec 2015 18:59:48 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > > > Have a look at my v2 where I've introduced two compare functions and also
> > > > modified the Rockchip compare_port() to use port->parent in the comparison. I
> > > > guess that should solve your problem.
> > > 
> > > Keeping the port instead of the parent asks for more code, but,
> > > especially, it also asks for changes in the component drivers because,
> > > at bind time, in 'data', they get a port instead of the device.
> > 
> > Sorry, this doesn't make sense.  You have far too many sub-clauses
> > which mean nothing at all.  Please rephrase.
> 
> Well, two topics:
> 
> - adding a second 'of_compare' function complexifies the code
>   and people may wonder why such a function is needed and what
>   they have to put inside.
> 
> - usually, the component drivers just do a component_add() of the device
>   at probe time.

... which is exactly what does happen throughout imx-drm.

>   Now, as the bind() function of the components of the first level
>   returns the port in 'data', some work has to be done for retrieving
>   the device.
>   This can (should?) be done in the bind() function.

Sorry, this still makes zero sense to me.  "retrieving the device"
is all done by the core component code and has nothing to do with
the drivers themselves.

>   In drm/imx/ipuv3-crtc.c, this is done by a hack, changing the device
>   node reference before calling component_add()!

What hack?

static int ipu_drm_probe(struct platform_device *pdev)
{
        struct device *dev = &pdev->dev;
        int ret;

        if (!dev->platform_data)
                return -EINVAL;

        ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
        if (ret)
                return ret;

        return component_add(dev, &ipu_crtc_ops);
}

There's no hack there.  I see nothing changing dev->of_node there.

> I looked at the imx-drm and the associated DTs, and I think that,
> without the v2 patch and keeping the port parent as the component
> (previous mail), the code could be simplified adding an intermediate
> device node in the DT.

Not going to happen, because that's going to break compatibility with
existing DTs.

Let me explain instead what's going on, and why imx-drm is different.

The iMX DT files describe the hardware, which is a very complex block.
The IPU as a whole in DT, with its external interfaces.  The IPU driver
lives in drivers/gpu/ipu-v3/.

The hardware is a single controller (aka IPU - image processing unit)
which consists of many sub-blocks of hardware.  Two of these blocks
are display controllers with associated display interfaces.  These
_can_ be programmed to behave as a CRTC, but they're essentially just
waveform generators.  There's other blocks, including camera interfaces.

We _choose_ in Linux to have the IPU driver create several different
platform devices, one for each of its ports, whether it's a camera
interface or a display interface.  These platform devices are bound
to the IPU's port DT nodes.

Some iMX chips have two IPUs.  This means there can be a total of four
display outputs.

On the display bridge side, display bridges can be configured via
muxes to be connected to any of these display outputs.  Several
display bridges can even be connected to a single display output,
though this is not done in practise.

DT fully describes these links between the display outputs and
display bridges using the OF graph support.  From the DT point of
view, this is all very elegant and correct to the hardware structure.

However, when we come to the Linux implementation, things get sticky
because we need to select the correct platform device corresponding
with the IPU's port.  This can only be done using the 'port' node
and not port->parent.

port->parent would be the IPU device node itself.  If we were to
introduce the additional ports {} node, that doesn't help, because
now port->parent points at the ports {} node instead, not the actual
port - and we need the port itself to identify which of the IPU's
own created platform devices to select.

So, modifying DT doesn't help in any way, even if you ignore the fact
that we need to maintain backwards compatibility.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Russell King - ARM Linux]

On Thu, Dec 24, 2015 at 09:15:28AM +0100, Jean-Francois Moine wrote:
> On Wed, 23 Dec 2015 18:59:48 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > > > Have a look at my v2 where I've introduced two compare functions and also
> > > > modified the Rockchip compare_port() to use port->parent in the comparison. I
> > > > guess that should solve your problem.
> > > 
> > > Keeping the port instead of the parent asks for more code, but,
> > > especially, it also asks for changes in the component drivers because,
> > > at bind time, in 'data', they get a port instead of the device.
> > 
> > Sorry, this doesn't make sense.  You have far too many sub-clauses
> > which mean nothing at all.  Please rephrase.
> 
> Well, two topics:
> 
> - adding a second 'of_compare' function complexifies the code
>   and people may wonder why such a function is needed and what
>   they have to put inside.
> 
> - usually, the component drivers just do a component_add() of the device
>   at probe time.

... which is exactly what does happen throughout imx-drm.

>   Now, as the bind() function of the components of the first level
>   returns the port in 'data', some work has to be done for retrieving
>   the device.
>   This can (should?) be done in the bind() function.

Sorry, this still makes zero sense to me.  "retrieving the device"
is all done by the core component code and has nothing to do with
the drivers themselves.

>   In drm/imx/ipuv3-crtc.c, this is done by a hack, changing the device
>   node reference before calling component_add()!

What hack?

static int ipu_drm_probe(struct platform_device *pdev)
{
        struct device *dev = &pdev->dev;
        int ret;

        if (!dev->platform_data)
                return -EINVAL;

        ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
        if (ret)
                return ret;

        return component_add(dev, &ipu_crtc_ops);
}

There's no hack there.  I see nothing changing dev->of_node there.

> I looked at the imx-drm and the associated DTs, and I think that,
> without the v2 patch and keeping the port parent as the component
> (previous mail), the code could be simplified adding an intermediate
> device node in the DT.

Not going to happen, because that's going to break compatibility with
existing DTs.

Let me explain instead what's going on, and why imx-drm is different.

The iMX DT files describe the hardware, which is a very complex block.
The IPU as a whole in DT, with its external interfaces.  The IPU driver
lives in drivers/gpu/ipu-v3/.

The hardware is a single controller (aka IPU - image processing unit)
which consists of many sub-blocks of hardware.  Two of these blocks
are display controllers with associated display interfaces.  These
_can_ be programmed to behave as a CRTC, but they're essentially just
waveform generators.  There's other blocks, including camera interfaces.

We _choose_ in Linux to have the IPU driver create several different
platform devices, one for each of its ports, whether it's a camera
interface or a display interface.  These platform devices are bound
to the IPU's port DT nodes.

Some iMX chips have two IPUs.  This means there can be a total of four
display outputs.

On the display bridge side, display bridges can be configured via
muxes to be connected to any of these display outputs.  Several
display bridges can even be connected to a single display output,
though this is not done in practise.

DT fully describes these links between the display outputs and
display bridges using the OF graph support.  From the DT point of
view, this is all very elegant and correct to the hardware structure.

However, when we come to the Linux implementation, things get sticky
because we need to select the correct platform device corresponding
with the IPU's port.  This can only be done using the 'port' node
and not port->parent.

port->parent would be the IPU device node itself.  If we were to
introduce the additional ports {} node, that doesn't help, because
now port->parent points at the ports {} node instead, not the actual
port - and we need the port itself to identify which of the IPU's
own created platform devices to select.

So, modifying DT doesn't help in any way, even if you ignore the fact
that we need to maintain backwards compatibility.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Jean-Francois Moine]

On Thu, 24 Dec 2015 10:52:07 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Thu, Dec 24, 2015 at 09:15:28AM +0100, Jean-Francois Moine wrote:
> > Well, two topics:
> > 
> > - adding a second 'of_compare' function complexifies the code
> >   and people may wonder why such a function is needed and what
> >   they have to put inside.
> > 
> > - usually, the component drivers just do a component_add() of the device
> >   at probe time.
> 
> ... which is exactly what does happen throughout imx-drm.
> 
> >   Now, as the bind() function of the components of the first level
> >   returns the port in 'data', some work has to be done for retrieving
> >   the device.
> >   This can (should?) be done in the bind() function.
> 
> Sorry, this still makes zero sense to me.  "retrieving the device"
> is all done by the core component code and has nothing to do with
> the drivers themselves.

Right, sorry, I wrote 'data' while thinking 'dev'.

> >   In drm/imx/ipuv3-crtc.c, this is done by a hack, changing the device
> >   node reference before calling component_add()!
> 
> What hack?
	[snip]
> There's no hack there.  I see nothing changing dev->of_node there.

Right again, I was looking in 4.4-rc1.

> > I looked at the imx-drm and the associated DTs, and I think that,
> > without the v2 patch and keeping the port parent as the component
> > (previous mail), the code could be simplified adding an intermediate
> > device node in the DT.
> 
> Not going to happen, because that's going to break compatibility with
> existing DTs.

OK, I cannot discuss against that!

> Let me explain instead what's going on, and why imx-drm is different.

Already understood.

	[snip]
> However, when we come to the Linux implementation, things get sticky
> because we need to select the correct platform device corresponding
> with the IPU's port.  This can only be done using the 'port' node
> and not port->parent.
> 
> port->parent would be the IPU device node itself.  If we were to
> introduce the additional ports {} node, that doesn't help, because
> now port->parent points at the ports {} node instead, not the actual
> port - and we need the port itself to identify which of the IPU's
> own created platform devices to select.
> 
> So, modifying DT doesn't help in any way, even if you ignore the fact
> that we need to maintain backwards compatibility.

The ports {} node is just a container, and so is the (unique) port {}
node which is inside:

	ipu1: ipu@02400000 {
		...
		ports@2 {			/* di0 device */
			ipu1_di0: port {
				...
				ipu1_di0_hdmi: endpoint@1 {
					remote-endpoint = <&hdmi_mux_0>;
				};
				ipu1_di0_mipi: endpoint@2 {
					remote-endpoint = <&mipi_mux_0>;
				};
				...
			};
		};
		ports@3 {			/* di1 device */
			ipu1_di1: port {
				...
				ipu1_di1_hdmi: endpoint@1 {  
					remote-endpoint = <&hdmi_mux_1>;
				};
				ipu1_di1_mipi: endpoint@2 {
					remote-endpoint = <&mipi_mux_1>;
				};
				...
			};
		};
	};

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Jean-Francois Moine]

On Thu, 24 Dec 2015 10:52:07 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Thu, Dec 24, 2015 at 09:15:28AM +0100, Jean-Francois Moine wrote:
> > Well, two topics:
> > 
> > - adding a second 'of_compare' function complexifies the code
> >   and people may wonder why such a function is needed and what
> >   they have to put inside.
> > 
> > - usually, the component drivers just do a component_add() of the device
> >   at probe time.
> 
> ... which is exactly what does happen throughout imx-drm.
> 
> >   Now, as the bind() function of the components of the first level
> >   returns the port in 'data', some work has to be done for retrieving
> >   the device.
> >   This can (should?) be done in the bind() function.
> 
> Sorry, this still makes zero sense to me.  "retrieving the device"
> is all done by the core component code and has nothing to do with
> the drivers themselves.

Right, sorry, I wrote 'data' while thinking 'dev'.

> >   In drm/imx/ipuv3-crtc.c, this is done by a hack, changing the device
> >   node reference before calling component_add()!
> 
> What hack?
	[snip]
> There's no hack there.  I see nothing changing dev->of_node there.

Right again, I was looking in 4.4-rc1.

> > I looked at the imx-drm and the associated DTs, and I think that,
> > without the v2 patch and keeping the port parent as the component
> > (previous mail), the code could be simplified adding an intermediate
> > device node in the DT.
> 
> Not going to happen, because that's going to break compatibility with
> existing DTs.

OK, I cannot discuss against that!

> Let me explain instead what's going on, and why imx-drm is different.

Already understood.

	[snip]
> However, when we come to the Linux implementation, things get sticky
> because we need to select the correct platform device corresponding
> with the IPU's port.  This can only be done using the 'port' node
> and not port->parent.
> 
> port->parent would be the IPU device node itself.  If we were to
> introduce the additional ports {} node, that doesn't help, because
> now port->parent points at the ports {} node instead, not the actual
> port - and we need the port itself to identify which of the IPU's
> own created platform devices to select.
> 
> So, modifying DT doesn't help in any way, even if you ignore the fact
> that we need to maintain backwards compatibility.

The ports {} node is just a container, and so is the (unique) port {}
node which is inside:

	ipu1: ipu@02400000 {
		...
		ports@2 {			/* di0 device */
			ipu1_di0: port {
				...
				ipu1_di0_hdmi: endpoint@1 {
					remote-endpoint = <&hdmi_mux_0>;
				};
				ipu1_di0_mipi: endpoint@2 {
					remote-endpoint = <&mipi_mux_0>;
				};
				...
			};
		};
		ports@3 {			/* di1 device */
			ipu1_di1: port {
				...
				ipu1_di1_hdmi: endpoint@1 {  
					remote-endpoint = <&hdmi_mux_1>;
				};
				ipu1_di1_mipi: endpoint@2 {
					remote-endpoint = <&mipi_mux_1>;
				};
				...
			};
		};
	};

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Jean-Francois Moine]

On Thu, 24 Dec 2015 10:52:07 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Thu, Dec 24, 2015 at 09:15:28AM +0100, Jean-Francois Moine wrote:
> > Well, two topics:
> > 
> > - adding a second 'of_compare' function complexifies the code
> >   and people may wonder why such a function is needed and what
> >   they have to put inside.
> > 
> > - usually, the component drivers just do a component_add() of the device
> >   at probe time.
> 
> ... which is exactly what does happen throughout imx-drm.
> 
> >   Now, as the bind() function of the components of the first level
> >   returns the port in 'data', some work has to be done for retrieving
> >   the device.
> >   This can (should?) be done in the bind() function.
> 
> Sorry, this still makes zero sense to me.  "retrieving the device"
> is all done by the core component code and has nothing to do with
> the drivers themselves.

Right, sorry, I wrote 'data' while thinking 'dev'.

> >   In drm/imx/ipuv3-crtc.c, this is done by a hack, changing the device
> >   node reference before calling component_add()!
> 
> What hack?
	[snip]
> There's no hack there.  I see nothing changing dev->of_node there.

Right again, I was looking in 4.4-rc1.

> > I looked at the imx-drm and the associated DTs, and I think that,
> > without the v2 patch and keeping the port parent as the component
> > (previous mail), the code could be simplified adding an intermediate
> > device node in the DT.
> 
> Not going to happen, because that's going to break compatibility with
> existing DTs.

OK, I cannot discuss against that!

> Let me explain instead what's going on, and why imx-drm is different.

Already understood.

	[snip]
> However, when we come to the Linux implementation, things get sticky
> because we need to select the correct platform device corresponding
> with the IPU's port.  This can only be done using the 'port' node
> and not port->parent.
> 
> port->parent would be the IPU device node itself.  If we were to
> introduce the additional ports {} node, that doesn't help, because
> now port->parent points at the ports {} node instead, not the actual
> port - and we need the port itself to identify which of the IPU's
> own created platform devices to select.
> 
> So, modifying DT doesn't help in any way, even if you ignore the fact
> that we need to maintain backwards compatibility.

The ports {} node is just a container, and so is the (unique) port {}
node which is inside:

	ipu1: ipu at 02400000 {
		...
		ports at 2 {			/* di0 device */
			ipu1_di0: port {
				...
				ipu1_di0_hdmi: endpoint at 1 {
					remote-endpoint = <&hdmi_mux_0>;
				};
				ipu1_di0_mipi: endpoint at 2 {
					remote-endpoint = <&mipi_mux_0>;
				};
				...
			};
		};
		ports at 3 {			/* di1 device */
			ipu1_di1: port {
				...
				ipu1_di1_hdmi: endpoint at 1 {  
					remote-endpoint = <&hdmi_mux_1>;
				};
				ipu1_di1_mipi: endpoint at 2 {
					remote-endpoint = <&mipi_mux_1>;
				};
				...
			};
		};
	};

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Russell King - ARM Linux]

On Thu, Dec 24, 2015 at 01:27:08PM +0100, Jean-Francois Moine wrote:
> On Thu, 24 Dec 2015 10:52:07 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > However, when we come to the Linux implementation, things get sticky
> > because we need to select the correct platform device corresponding
> > with the IPU's port.  This can only be done using the 'port' node
> > and not port->parent.
> > 
> > port->parent would be the IPU device node itself.  If we were to
> > introduce the additional ports {} node, that doesn't help, because
> > now port->parent points at the ports {} node instead, not the actual
> > port - and we need the port itself to identify which of the IPU's
> > own created platform devices to select.
> > 
> > So, modifying DT doesn't help in any way, even if you ignore the fact
> > that we need to maintain backwards compatibility.
> 
> The ports {} node is just a container, and so is the (unique) port {}
> node which is inside:
> 
> 	ipu1: ipu@02400000 {
> 		...
> 		ports@2 {			/* di0 device */
> 			ipu1_di0: port {
> 				...
> 				ipu1_di0_hdmi: endpoint@1 {
> 					remote-endpoint = <&hdmi_mux_0>;
> 				};
> 				ipu1_di0_mipi: endpoint@2 {
> 					remote-endpoint = <&mipi_mux_0>;
> 				};
> 				...
> 			};
> 		};
> 		ports@3 {			/* di1 device */
> 			ipu1_di1: port {
> 				...
> 				ipu1_di1_hdmi: endpoint@1 {  
> 					remote-endpoint = <&hdmi_mux_1>;
> 				};
> 				ipu1_di1_mipi: endpoint@2 {
> 					remote-endpoint = <&mipi_mux_1>;
> 				};
> 				...
> 			};
> 		};
> 	};

That's against the binding documentation for graphs:

All 'port' nodes can be grouped under an optional 'ports' node, which
allows to specify #address-cells, #size-cells properties for the 'port'
nodes independently from any other child device nodes a device might
have.

It says "All 'port' nodes" not "Some" or similar.  The DT code requires
this.  To change this would mean changing the DT binding and the code
parsing that binding, adding much more complexity there.

You earlier argued against adding (what would be less) complexity to
the DRM OF helper, now you seem to be wanting more complexity elsewhere
to save what would be trivial complexity elsewhere - all the functions
which iterate over the port nodes would need to be updated to find
all the "ports" nodes, and end up needing an additional level of looping
and complexity to jump from one port node in a ports { } block to the
first port node in the next ports { } block.

Also it makes the API more difficult because we end up with the ports@n
nodes needing a reg= property (as per ePAPR requirements) and it becomes
unclear what that would represent at the hardware level.

It seems that you're trying to work around a limitation in Linux by
modifying the hardware representation...

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Russell King - ARM Linux]

On Thu, Dec 24, 2015 at 01:27:08PM +0100, Jean-Francois Moine wrote:
> On Thu, 24 Dec 2015 10:52:07 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > However, when we come to the Linux implementation, things get sticky
> > because we need to select the correct platform device corresponding
> > with the IPU's port.  This can only be done using the 'port' node
> > and not port->parent.
> > 
> > port->parent would be the IPU device node itself.  If we were to
> > introduce the additional ports {} node, that doesn't help, because
> > now port->parent points at the ports {} node instead, not the actual
> > port - and we need the port itself to identify which of the IPU's
> > own created platform devices to select.
> > 
> > So, modifying DT doesn't help in any way, even if you ignore the fact
> > that we need to maintain backwards compatibility.
> 
> The ports {} node is just a container, and so is the (unique) port {}
> node which is inside:
> 
> 	ipu1: ipu@02400000 {
> 		...
> 		ports@2 {			/* di0 device */
> 			ipu1_di0: port {
> 				...
> 				ipu1_di0_hdmi: endpoint@1 {
> 					remote-endpoint = <&hdmi_mux_0>;
> 				};
> 				ipu1_di0_mipi: endpoint@2 {
> 					remote-endpoint = <&mipi_mux_0>;
> 				};
> 				...
> 			};
> 		};
> 		ports@3 {			/* di1 device */
> 			ipu1_di1: port {
> 				...
> 				ipu1_di1_hdmi: endpoint@1 {  
> 					remote-endpoint = <&hdmi_mux_1>;
> 				};
> 				ipu1_di1_mipi: endpoint@2 {
> 					remote-endpoint = <&mipi_mux_1>;
> 				};
> 				...
> 			};
> 		};
> 	};

That's against the binding documentation for graphs:

All 'port' nodes can be grouped under an optional 'ports' node, which
allows to specify #address-cells, #size-cells properties for the 'port'
nodes independently from any other child device nodes a device might
have.

It says "All 'port' nodes" not "Some" or similar.  The DT code requires
this.  To change this would mean changing the DT binding and the code
parsing that binding, adding much more complexity there.

You earlier argued against adding (what would be less) complexity to
the DRM OF helper, now you seem to be wanting more complexity elsewhere
to save what would be trivial complexity elsewhere - all the functions
which iterate over the port nodes would need to be updated to find
all the "ports" nodes, and end up needing an additional level of looping
and complexity to jump from one port node in a ports { } block to the
first port node in the next ports { } block.

Also it makes the API more difficult because we end up with the ports@n
nodes needing a reg= property (as per ePAPR requirements) and it becomes
unclear what that would represent at the hardware level.

It seems that you're trying to work around a limitation in Linux by
modifying the hardware representation...

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Russell King - ARM Linux]

On Thu, Dec 24, 2015 at 01:27:08PM +0100, Jean-Francois Moine wrote:
> On Thu, 24 Dec 2015 10:52:07 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > However, when we come to the Linux implementation, things get sticky
> > because we need to select the correct platform device corresponding
> > with the IPU's port.  This can only be done using the 'port' node
> > and not port->parent.
> > 
> > port->parent would be the IPU device node itself.  If we were to
> > introduce the additional ports {} node, that doesn't help, because
> > now port->parent points at the ports {} node instead, not the actual
> > port - and we need the port itself to identify which of the IPU's
> > own created platform devices to select.
> > 
> > So, modifying DT doesn't help in any way, even if you ignore the fact
> > that we need to maintain backwards compatibility.
> 
> The ports {} node is just a container, and so is the (unique) port {}
> node which is inside:
> 
> 	ipu1: ipu at 02400000 {
> 		...
> 		ports at 2 {			/* di0 device */
> 			ipu1_di0: port {
> 				...
> 				ipu1_di0_hdmi: endpoint at 1 {
> 					remote-endpoint = <&hdmi_mux_0>;
> 				};
> 				ipu1_di0_mipi: endpoint at 2 {
> 					remote-endpoint = <&mipi_mux_0>;
> 				};
> 				...
> 			};
> 		};
> 		ports at 3 {			/* di1 device */
> 			ipu1_di1: port {
> 				...
> 				ipu1_di1_hdmi: endpoint at 1 {  
> 					remote-endpoint = <&hdmi_mux_1>;
> 				};
> 				ipu1_di1_mipi: endpoint at 2 {
> 					remote-endpoint = <&mipi_mux_1>;
> 				};
> 				...
> 			};
> 		};
> 	};

That's against the binding documentation for graphs:

All 'port' nodes can be grouped under an optional 'ports' node, which
allows to specify #address-cells, #size-cells properties for the 'port'
nodes independently from any other child device nodes a device might
have.

It says "All 'port' nodes" not "Some" or similar.  The DT code requires
this.  To change this would mean changing the DT binding and the code
parsing that binding, adding much more complexity there.

You earlier argued against adding (what would be less) complexity to
the DRM OF helper, now you seem to be wanting more complexity elsewhere
to save what would be trivial complexity elsewhere - all the functions
which iterate over the port nodes would need to be updated to find
all the "ports" nodes, and end up needing an additional level of looping
and complexity to jump from one port node in a ports { } block to the
first port node in the next ports { } block.

Also it makes the API more difficult because we end up with the ports at n
nodes needing a reg= property (as per ePAPR requirements) and it becomes
unclear what that would represent at the hardware level.

It seems that you're trying to work around a limitation in Linux by
modifying the hardware representation...

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Jean-Francois Moine]

On Thu, 24 Dec 2015 12:36:10 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> It seems that you're trying to work around a limitation in Linux by
> modifying the hardware representation...

Sorry to come back to this topic, but I think you are wrong.

Looking at the imx6 DTs, the problem comes from the display-subsystem
node which is a pure Linux specific software entity.

If you want to describe only the hardware in the DT, everything is
simple.

A IPU is a image controller with its sub-devices. Seen from the system, it
is like a 'board' with its devices (LCDs, camera...).

When 2 IPUs, there are 2 independant boards.

Here is what could be a pure hardware DT:

/* no display-subsystem */

	ipu1: ipu@02400000 {		/* image controller / board 1 */
		compatible = "fsl,imx6q-ipu";
		...
		ports = <&ipu1_di0>, <&ipu1_di1>;
	};
	ipu1_di0: di@0 {		/* display interface / crtc 1 */
		compatible = "fsl,imx6q-di";
		...
		ipu1_di0_hdmi: endpoint@1 {
			remote-endpoint = <&hdmi_mux_0>;
		};
		ipu1_di0_mipi: endpoint@2 {
			remote-endpoint = <&mipi_mux_0>;
		}
		...
	};
	ipu1_di1: di@1 {		/* display interface / crtc 2 */
		compatible = "fsl,imx6q-di";
		...
		ipu1_di1_hdmi: endpoint@1 {
			remote-endpoint = <&hdmi_mux_1>;
		};
		ipu1_di1_mipi: endpoint@2 {
			remote-endpoint = <&mipi_mux_1>;
		}
		...
	};

	ipu2: ipu@02800000 {		/* image controller / board 2 */
		compatible = "fsl,imx6q-ipu";
		...
		ports = <&ipu2_di0>, <&ipu2_di1>;
	};
	ipu2_di0: di@0 {		/* display interface / crtc 1 */
		compatible = "fsl,imx6q-di";
		...
		ipu2_di0_hdmi: endpoint@1 {
			remote-endpoint = <&hdmi_mux_2>;
		};
		ipu2_di0_mipi: endpoint@2 {
			remote-endpoint = <&mipi_mux_2>;
		}
		...
	};
	ipu2_di1: di@1 {		/* display interface / crtc 2 */
		compatible = "fsl,imx6q-di";
		...
		ipu2_di1_hdmi: endpoint@1 {
			remote-endpoint = <&hdmi_mux_3>;
		};
		ipu2_di1_mipi: endpoint@2 {
			remote-endpoint = <&mipi_mux_3>;
		}
		...
	};

Then, a standard component binding (port->parent) works fine...

(you may note that the same problem exists with audio: the
'simple-card' is also a pure Linux specific software entity)

-- 
Merry Christmas	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Jean-Francois Moine]

On Thu, 24 Dec 2015 12:36:10 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> It seems that you're trying to work around a limitation in Linux by
> modifying the hardware representation...

Sorry to come back to this topic, but I think you are wrong.

Looking at the imx6 DTs, the problem comes from the display-subsystem
node which is a pure Linux specific software entity.

If you want to describe only the hardware in the DT, everything is
simple.

A IPU is a image controller with its sub-devices. Seen from the system, it
is like a 'board' with its devices (LCDs, camera...).

When 2 IPUs, there are 2 independant boards.

Here is what could be a pure hardware DT:

/* no display-subsystem */

	ipu1: ipu@02400000 {		/* image controller / board 1 */
		compatible = "fsl,imx6q-ipu";
		...
		ports = <&ipu1_di0>, <&ipu1_di1>;
	};
	ipu1_di0: di@0 {		/* display interface / crtc 1 */
		compatible = "fsl,imx6q-di";
		...
		ipu1_di0_hdmi: endpoint@1 {
			remote-endpoint = <&hdmi_mux_0>;
		};
		ipu1_di0_mipi: endpoint@2 {
			remote-endpoint = <&mipi_mux_0>;
		}
		...
	};
	ipu1_di1: di@1 {		/* display interface / crtc 2 */
		compatible = "fsl,imx6q-di";
		...
		ipu1_di1_hdmi: endpoint@1 {
			remote-endpoint = <&hdmi_mux_1>;
		};
		ipu1_di1_mipi: endpoint@2 {
			remote-endpoint = <&mipi_mux_1>;
		}
		...
	};

	ipu2: ipu@02800000 {		/* image controller / board 2 */
		compatible = "fsl,imx6q-ipu";
		...
		ports = <&ipu2_di0>, <&ipu2_di1>;
	};
	ipu2_di0: di@0 {		/* display interface / crtc 1 */
		compatible = "fsl,imx6q-di";
		...
		ipu2_di0_hdmi: endpoint@1 {
			remote-endpoint = <&hdmi_mux_2>;
		};
		ipu2_di0_mipi: endpoint@2 {
			remote-endpoint = <&mipi_mux_2>;
		}
		...
	};
	ipu2_di1: di@1 {		/* display interface / crtc 2 */
		compatible = "fsl,imx6q-di";
		...
		ipu2_di1_hdmi: endpoint@1 {
			remote-endpoint = <&hdmi_mux_3>;
		};
		ipu2_di1_mipi: endpoint@2 {
			remote-endpoint = <&mipi_mux_3>;
		}
		...
	};

Then, a standard component binding (port->parent) works fine...

(you may note that the same problem exists with audio: the
'simple-card' is also a pure Linux specific software entity)

-- 
Merry Christmas	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 0/2] Improve drm_of_component_probe() and move rockchip to use it

[Jean-Francois Moine]

On Thu, 24 Dec 2015 12:36:10 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> It seems that you're trying to work around a limitation in Linux by
> modifying the hardware representation...

Sorry to come back to this topic, but I think you are wrong.

Looking at the imx6 DTs, the problem comes from the display-subsystem
node which is a pure Linux specific software entity.

If you want to describe only the hardware in the DT, everything is
simple.

A IPU is a image controller with its sub-devices. Seen from the system, it
is like a 'board' with its devices (LCDs, camera...).

When 2 IPUs, there are 2 independant boards.

Here is what could be a pure hardware DT:

/* no display-subsystem */

	ipu1: ipu at 02400000 {		/* image controller / board 1 */
		compatible = "fsl,imx6q-ipu";
		...
		ports = <&ipu1_di0>, <&ipu1_di1>;
	};
	ipu1_di0: di at 0 {		/* display interface / crtc 1 */
		compatible = "fsl,imx6q-di";
		...
		ipu1_di0_hdmi: endpoint at 1 {
			remote-endpoint = <&hdmi_mux_0>;
		};
		ipu1_di0_mipi: endpoint at 2 {
			remote-endpoint = <&mipi_mux_0>;
		}
		...
	};
	ipu1_di1: di at 1 {		/* display interface / crtc 2 */
		compatible = "fsl,imx6q-di";
		...
		ipu1_di1_hdmi: endpoint at 1 {
			remote-endpoint = <&hdmi_mux_1>;
		};
		ipu1_di1_mipi: endpoint at 2 {
			remote-endpoint = <&mipi_mux_1>;
		}
		...
	};

	ipu2: ipu at 02800000 {		/* image controller / board 2 */
		compatible = "fsl,imx6q-ipu";
		...
		ports = <&ipu2_di0>, <&ipu2_di1>;
	};
	ipu2_di0: di at 0 {		/* display interface / crtc 1 */
		compatible = "fsl,imx6q-di";
		...
		ipu2_di0_hdmi: endpoint at 1 {
			remote-endpoint = <&hdmi_mux_2>;
		};
		ipu2_di0_mipi: endpoint at 2 {
			remote-endpoint = <&mipi_mux_2>;
		}
		...
	};
	ipu2_di1: di at 1 {		/* display interface / crtc 2 */
		compatible = "fsl,imx6q-di";
		...
		ipu2_di1_hdmi: endpoint at 1 {
			remote-endpoint = <&hdmi_mux_3>;
		};
		ipu2_di1_mipi: endpoint at 2 {
			remote-endpoint = <&mipi_mux_3>;
		}
		...
	};

Then, a standard component binding (port->parent) works fine...

(you may note that the same problem exists with audio: the
'simple-card' is also a pure Linux specific software entity)

-- 
Merry Christmas	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH v2 1/2] drm: Improve drm_of_component_probe() to correctly handle ports and remote ports.

[Liviu Dudau]

On Fri, Nov 20, 2015 at 02:22:04PM +0000, Liviu Dudau wrote:
> Rockchip DRM driver cannot use the same compare_of() function to
> match ports and remote ports (aka encoders) as their OF sub-trees
> look different. Add a second compare function to be used when encoders
> are added to the component framework and patch the existing users of
> the function accordingly.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>

Russell,

Resurecting this old patch from around Christmas time (bad time for patch review).

Are you happy enough with this version to re-issue the Ack or do you think I still
need to work on it?

Best regards,
Liviu

> ---
>  drivers/gpu/drm/armada/armada_drv.c |  3 ++-
>  drivers/gpu/drm/drm_of.c            | 19 ++++++++++++++-----
>  drivers/gpu/drm/imx/imx-drm-core.c  |  3 ++-
>  include/drm/drm_of.h                |  6 ++++--
>  4 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
> index 77ab93d..3a2a929 100644
> --- a/drivers/gpu/drm/armada/armada_drv.c
> +++ b/drivers/gpu/drm/armada/armada_drv.c
> @@ -274,7 +274,8 @@ static int armada_drm_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	int ret;
>  
> -	ret = drm_of_component_probe(dev, compare_dev_name, &armada_master_ops);
> +	ret = drm_of_component_probe(dev, compare_dev_name, compare_dev_name,
> +				     &armada_master_ops);
>  	if (ret != -EINVAL)
>  		return ret;
>  
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 493c05c..34589d3 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -77,7 +77,8 @@ EXPORT_SYMBOL(drm_of_find_possible_crtcs);
>   * Returns zero if successful, or one of the standard error codes if it fails.
>   */
>  int drm_of_component_probe(struct device *dev,
> -			   int (*compare_of)(struct device *, void *),
> +			   int (*compare_port)(struct device *, void *),
> +			   int (*compare_encoder)(struct device *, void *),
>  			   const struct component_master_ops *m_ops)
>  {
>  	struct device_node *ep, *port, *remote;
> @@ -101,8 +102,12 @@ int drm_of_component_probe(struct device *dev,
>  			continue;
>  		}
>  
> -		component_match_add(dev, &match, compare_of, port);
> -		of_node_put(port);
> +		component_match_add(dev, &match, compare_port, port);
> +		/*
> +		 * component_match_add keeps a reference to the port
> +		 * variable, so we need to keep the reference count
> +		 * increment from of_parse_phandle()
> +		 */
>  	}
>  
>  	if (i == 0) {
> @@ -140,8 +145,12 @@ int drm_of_component_probe(struct device *dev,
>  				continue;
>  			}
>  
> -			component_match_add(dev, &match, compare_of, remote);
> -			of_node_put(remote);
> +			component_match_add(dev, &match, compare_encoder, remote);
> +			/*
> +			 * component_match_add keeps a reference to the port
> +			 * variable, so we need to keep the reference count
> +			 * increment from of_graph_get_remote_port_parent()
> +			 */
>  		}
>  		of_node_put(port);
>  	}
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 64f16ea..0d36410 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -531,7 +531,8 @@ static const struct component_master_ops imx_drm_ops = {
>  
>  static int imx_drm_platform_probe(struct platform_device *pdev)
>  {
> -	int ret = drm_of_component_probe(&pdev->dev, compare_of, &imx_drm_ops);
> +	int ret = drm_of_component_probe(&pdev->dev, compare_of, compare_of,
> +					 &imx_drm_ops);
>  
>  	if (!ret)
>  		ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> index 8544665..1c29e42 100644
> --- a/include/drm/drm_of.h
> +++ b/include/drm/drm_of.h
> @@ -10,7 +10,8 @@ struct device_node;
>  extern uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
>  					   struct device_node *port);
>  extern int drm_of_component_probe(struct device *dev,
> -				  int (*compare_of)(struct device *, void *),
> +				  int (*compare_port)(struct device *, void *),
> +				  int (*compare_encoder)(struct device *, void *),
>  				  const struct component_master_ops *m_ops);
>  #else
>  static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> @@ -21,7 +22,8 @@ static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
>  
>  static inline int
>  drm_of_component_probe(struct device *dev,
> -		       int (*compare_of)(struct device *, void *),
> +		       int (*compare_port)(struct device *, void *),
> +		       int (*compare_encoder)(struct device *, void *),
>  		       const struct component_master_ops *m_ops)
>  {
>  	return -EINVAL;
> -- 
> 2.6.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v2 1/2] drm: Improve drm_of_component_probe() to correctly handle ports and remote ports.

[Liviu Dudau]

On Fri, Nov 20, 2015 at 02:22:04PM +0000, Liviu Dudau wrote:
> Rockchip DRM driver cannot use the same compare_of() function to
> match ports and remote ports (aka encoders) as their OF sub-trees
> look different. Add a second compare function to be used when encoders
> are added to the component framework and patch the existing users of
> the function accordingly.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>

Russell,

Resurecting this old patch from around Christmas time (bad time for patch review).

Are you happy enough with this version to re-issue the Ack or do you think I still
need to work on it?

Best regards,
Liviu

> ---
>  drivers/gpu/drm/armada/armada_drv.c |  3 ++-
>  drivers/gpu/drm/drm_of.c            | 19 ++++++++++++++-----
>  drivers/gpu/drm/imx/imx-drm-core.c  |  3 ++-
>  include/drm/drm_of.h                |  6 ++++--
>  4 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
> index 77ab93d..3a2a929 100644
> --- a/drivers/gpu/drm/armada/armada_drv.c
> +++ b/drivers/gpu/drm/armada/armada_drv.c
> @@ -274,7 +274,8 @@ static int armada_drm_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	int ret;
>  
> -	ret = drm_of_component_probe(dev, compare_dev_name, &armada_master_ops);
> +	ret = drm_of_component_probe(dev, compare_dev_name, compare_dev_name,
> +				     &armada_master_ops);
>  	if (ret != -EINVAL)
>  		return ret;
>  
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 493c05c..34589d3 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -77,7 +77,8 @@ EXPORT_SYMBOL(drm_of_find_possible_crtcs);
>   * Returns zero if successful, or one of the standard error codes if it fails.
>   */
>  int drm_of_component_probe(struct device *dev,
> -			   int (*compare_of)(struct device *, void *),
> +			   int (*compare_port)(struct device *, void *),
> +			   int (*compare_encoder)(struct device *, void *),
>  			   const struct component_master_ops *m_ops)
>  {
>  	struct device_node *ep, *port, *remote;
> @@ -101,8 +102,12 @@ int drm_of_component_probe(struct device *dev,
>  			continue;
>  		}
>  
> -		component_match_add(dev, &match, compare_of, port);
> -		of_node_put(port);
> +		component_match_add(dev, &match, compare_port, port);
> +		/*
> +		 * component_match_add keeps a reference to the port
> +		 * variable, so we need to keep the reference count
> +		 * increment from of_parse_phandle()
> +		 */
>  	}
>  
>  	if (i == 0) {
> @@ -140,8 +145,12 @@ int drm_of_component_probe(struct device *dev,
>  				continue;
>  			}
>  
> -			component_match_add(dev, &match, compare_of, remote);
> -			of_node_put(remote);
> +			component_match_add(dev, &match, compare_encoder, remote);
> +			/*
> +			 * component_match_add keeps a reference to the port
> +			 * variable, so we need to keep the reference count
> +			 * increment from of_graph_get_remote_port_parent()
> +			 */
>  		}
>  		of_node_put(port);
>  	}
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 64f16ea..0d36410 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -531,7 +531,8 @@ static const struct component_master_ops imx_drm_ops = {
>  
>  static int imx_drm_platform_probe(struct platform_device *pdev)
>  {
> -	int ret = drm_of_component_probe(&pdev->dev, compare_of, &imx_drm_ops);
> +	int ret = drm_of_component_probe(&pdev->dev, compare_of, compare_of,
> +					 &imx_drm_ops);
>  
>  	if (!ret)
>  		ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> index 8544665..1c29e42 100644
> --- a/include/drm/drm_of.h
> +++ b/include/drm/drm_of.h
> @@ -10,7 +10,8 @@ struct device_node;
>  extern uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
>  					   struct device_node *port);
>  extern int drm_of_component_probe(struct device *dev,
> -				  int (*compare_of)(struct device *, void *),
> +				  int (*compare_port)(struct device *, void *),
> +				  int (*compare_encoder)(struct device *, void *),
>  				  const struct component_master_ops *m_ops);
>  #else
>  static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> @@ -21,7 +22,8 @@ static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
>  
>  static inline int
>  drm_of_component_probe(struct device *dev,
> -		       int (*compare_of)(struct device *, void *),
> +		       int (*compare_port)(struct device *, void *),
> +		       int (*compare_encoder)(struct device *, void *),
>  		       const struct component_master_ops *m_ops)
>  {
>  	return -EINVAL;
> -- 
> 2.6.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/2] drm: Improve drm_of_component_probe() to correctly handle ports and remote ports.

[Liviu Dudau]

On Fri, Nov 20, 2015 at 02:22:04PM +0000, Liviu Dudau wrote:
> Rockchip DRM driver cannot use the same compare_of() function to
> match ports and remote ports (aka encoders) as their OF sub-trees
> look different. Add a second compare function to be used when encoders
> are added to the component framework and patch the existing users of
> the function accordingly.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>

Russell,

Resurecting this old patch from around Christmas time (bad time for patch review).

Are you happy enough with this version to re-issue the Ack or do you think I still
need to work on it?

Best regards,
Liviu

> ---
>  drivers/gpu/drm/armada/armada_drv.c |  3 ++-
>  drivers/gpu/drm/drm_of.c            | 19 ++++++++++++++-----
>  drivers/gpu/drm/imx/imx-drm-core.c  |  3 ++-
>  include/drm/drm_of.h                |  6 ++++--
>  4 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
> index 77ab93d..3a2a929 100644
> --- a/drivers/gpu/drm/armada/armada_drv.c
> +++ b/drivers/gpu/drm/armada/armada_drv.c
> @@ -274,7 +274,8 @@ static int armada_drm_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	int ret;
>  
> -	ret = drm_of_component_probe(dev, compare_dev_name, &armada_master_ops);
> +	ret = drm_of_component_probe(dev, compare_dev_name, compare_dev_name,
> +				     &armada_master_ops);
>  	if (ret != -EINVAL)
>  		return ret;
>  
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 493c05c..34589d3 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -77,7 +77,8 @@ EXPORT_SYMBOL(drm_of_find_possible_crtcs);
>   * Returns zero if successful, or one of the standard error codes if it fails.
>   */
>  int drm_of_component_probe(struct device *dev,
> -			   int (*compare_of)(struct device *, void *),
> +			   int (*compare_port)(struct device *, void *),
> +			   int (*compare_encoder)(struct device *, void *),
>  			   const struct component_master_ops *m_ops)
>  {
>  	struct device_node *ep, *port, *remote;
> @@ -101,8 +102,12 @@ int drm_of_component_probe(struct device *dev,
>  			continue;
>  		}
>  
> -		component_match_add(dev, &match, compare_of, port);
> -		of_node_put(port);
> +		component_match_add(dev, &match, compare_port, port);
> +		/*
> +		 * component_match_add keeps a reference to the port
> +		 * variable, so we need to keep the reference count
> +		 * increment from of_parse_phandle()
> +		 */
>  	}
>  
>  	if (i == 0) {
> @@ -140,8 +145,12 @@ int drm_of_component_probe(struct device *dev,
>  				continue;
>  			}
>  
> -			component_match_add(dev, &match, compare_of, remote);
> -			of_node_put(remote);
> +			component_match_add(dev, &match, compare_encoder, remote);
> +			/*
> +			 * component_match_add keeps a reference to the port
> +			 * variable, so we need to keep the reference count
> +			 * increment from of_graph_get_remote_port_parent()
> +			 */
>  		}
>  		of_node_put(port);
>  	}
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 64f16ea..0d36410 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -531,7 +531,8 @@ static const struct component_master_ops imx_drm_ops = {
>  
>  static int imx_drm_platform_probe(struct platform_device *pdev)
>  {
> -	int ret = drm_of_component_probe(&pdev->dev, compare_of, &imx_drm_ops);
> +	int ret = drm_of_component_probe(&pdev->dev, compare_of, compare_of,
> +					 &imx_drm_ops);
>  
>  	if (!ret)
>  		ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> index 8544665..1c29e42 100644
> --- a/include/drm/drm_of.h
> +++ b/include/drm/drm_of.h
> @@ -10,7 +10,8 @@ struct device_node;
>  extern uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
>  					   struct device_node *port);
>  extern int drm_of_component_probe(struct device *dev,
> -				  int (*compare_of)(struct device *, void *),
> +				  int (*compare_port)(struct device *, void *),
> +				  int (*compare_encoder)(struct device *, void *),
>  				  const struct component_master_ops *m_ops);
>  #else
>  static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> @@ -21,7 +22,8 @@ static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
>  
>  static inline int
>  drm_of_component_probe(struct device *dev,
> -		       int (*compare_of)(struct device *, void *),
> +		       int (*compare_port)(struct device *, void *),
> +		       int (*compare_encoder)(struct device *, void *),
>  		       const struct component_master_ops *m_ops)
>  {
>  	return -EINVAL;
> -- 
> 2.6.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?

Re: [PATCH v2 1/2] drm: Improve drm_of_component_probe() to correctly handle ports and remote ports.

[Russell King - ARM Linux]

On Mon, Feb 22, 2016 at 11:51:47AM +0000, Liviu Dudau wrote:
> On Fri, Nov 20, 2015 at 02:22:04PM +0000, Liviu Dudau wrote:
> > Rockchip DRM driver cannot use the same compare_of() function to
> > match ports and remote ports (aka encoders) as their OF sub-trees
> > look different. Add a second compare function to be used when encoders
> > are added to the component framework and patch the existing users of
> > the function accordingly.
> > 
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> 
> Russell,
> 
> Resurecting this old patch from around Christmas time (bad time for patch
> review).
> 
> Are you happy enough with this version to re-issue the Ack or do you think
> I still need to work on it?

What I'd like to see is the patch reworked, because:

> > +		component_match_add(dev, &match, compare_port, port);
> > +		/*
> > +		 * component_match_add keeps a reference to the port
> > +		 * variable, so we need to keep the reference count
> > +		 * increment from of_parse_phandle()
> > +		 */
...
> > +			component_match_add(dev, &match, compare_encoder, remote);
> > +			/*
> > +			 * component_match_add keeps a reference to the port
> > +			 * variable, so we need to keep the reference count
> > +			 * increment from of_graph_get_remote_port_parent()
> > +			 */

This problem no longer exists.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 1/2] drm: Improve drm_of_component_probe() to correctly handle ports and remote ports.

[Russell King - ARM Linux]

On Mon, Feb 22, 2016 at 11:51:47AM +0000, Liviu Dudau wrote:
> On Fri, Nov 20, 2015 at 02:22:04PM +0000, Liviu Dudau wrote:
> > Rockchip DRM driver cannot use the same compare_of() function to
> > match ports and remote ports (aka encoders) as their OF sub-trees
> > look different. Add a second compare function to be used when encoders
> > are added to the component framework and patch the existing users of
> > the function accordingly.
> > 
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> 
> Russell,
> 
> Resurecting this old patch from around Christmas time (bad time for patch
> review).
> 
> Are you happy enough with this version to re-issue the Ack or do you think
> I still need to work on it?

What I'd like to see is the patch reworked, because:

> > +		component_match_add(dev, &match, compare_port, port);
> > +		/*
> > +		 * component_match_add keeps a reference to the port
> > +		 * variable, so we need to keep the reference count
> > +		 * increment from of_parse_phandle()
> > +		 */
...
> > +			component_match_add(dev, &match, compare_encoder, remote);
> > +			/*
> > +			 * component_match_add keeps a reference to the port
> > +			 * variable, so we need to keep the reference count
> > +			 * increment from of_graph_get_remote_port_parent()
> > +			 */

This problem no longer exists.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/2] drm: Improve drm_of_component_probe() to correctly handle ports and remote ports.

[Russell King - ARM Linux]

On Mon, Feb 22, 2016 at 11:51:47AM +0000, Liviu Dudau wrote:
> On Fri, Nov 20, 2015 at 02:22:04PM +0000, Liviu Dudau wrote:
> > Rockchip DRM driver cannot use the same compare_of() function to
> > match ports and remote ports (aka encoders) as their OF sub-trees
> > look different. Add a second compare function to be used when encoders
> > are added to the component framework and patch the existing users of
> > the function accordingly.
> > 
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> 
> Russell,
> 
> Resurecting this old patch from around Christmas time (bad time for patch
> review).
> 
> Are you happy enough with this version to re-issue the Ack or do you think
> I still need to work on it?

What I'd like to see is the patch reworked, because:

> > +		component_match_add(dev, &match, compare_port, port);
> > +		/*
> > +		 * component_match_add keeps a reference to the port
> > +		 * variable, so we need to keep the reference count
> > +		 * increment from of_parse_phandle()
> > +		 */
...
> > +			component_match_add(dev, &match, compare_encoder, remote);
> > +			/*
> > +			 * component_match_add keeps a reference to the port
> > +			 * variable, so we need to keep the reference count
> > +			 * increment from of_graph_get_remote_port_parent()
> > +			 */

This problem no longer exists.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 1/2] drm: Improve drm_of_component_probe() to correctly handle ports and remote ports.

[Liviu Dudau]

On Mon, Feb 22, 2016 at 03:51:29PM +0000, Russell King - ARM Linux wrote:
> On Mon, Feb 22, 2016 at 11:51:47AM +0000, Liviu Dudau wrote:
> > On Fri, Nov 20, 2015 at 02:22:04PM +0000, Liviu Dudau wrote:
> > > Rockchip DRM driver cannot use the same compare_of() function to
> > > match ports and remote ports (aka encoders) as their OF sub-trees
> > > look different. Add a second compare function to be used when encoders
> > > are added to the component framework and patch the existing users of
> > > the function accordingly.
> > > 
> > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > 
> > Russell,
> > 
> > Resurecting this old patch from around Christmas time (bad time for patch
> > review).
> > 
> > Are you happy enough with this version to re-issue the Ack or do you think
> > I still need to work on it?
> 
> What I'd like to see is the patch reworked, because:
> 
> > > +		component_match_add(dev, &match, compare_port, port);
> > > +		/*
> > > +		 * component_match_add keeps a reference to the port
> > > +		 * variable, so we need to keep the reference count
> > > +		 * increment from of_parse_phandle()
> > > +		 */
> ...
> > > +			component_match_add(dev, &match, compare_encoder, remote);
> > > +			/*
> > > +			 * component_match_add keeps a reference to the port
> > > +			 * variable, so we need to keep the reference count
> > > +			 * increment from of_graph_get_remote_port_parent()
> > > +			 */
> 
> This problem no longer exists.

Fair enough, I will send a v3 version.

Thanks,
Liviu

> 
> -- 
> RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v2 1/2] drm: Improve drm_of_component_probe() to correctly handle ports and remote ports.

[Liviu Dudau]

On Mon, Feb 22, 2016 at 03:51:29PM +0000, Russell King - ARM Linux wrote:
> On Mon, Feb 22, 2016 at 11:51:47AM +0000, Liviu Dudau wrote:
> > On Fri, Nov 20, 2015 at 02:22:04PM +0000, Liviu Dudau wrote:
> > > Rockchip DRM driver cannot use the same compare_of() function to
> > > match ports and remote ports (aka encoders) as their OF sub-trees
> > > look different. Add a second compare function to be used when encoders
> > > are added to the component framework and patch the existing users of
> > > the function accordingly.
> > > 
> > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > 
> > Russell,
> > 
> > Resurecting this old patch from around Christmas time (bad time for patch
> > review).
> > 
> > Are you happy enough with this version to re-issue the Ack or do you think
> > I still need to work on it?
> 
> What I'd like to see is the patch reworked, because:
> 
> > > +		component_match_add(dev, &match, compare_port, port);
> > > +		/*
> > > +		 * component_match_add keeps a reference to the port
> > > +		 * variable, so we need to keep the reference count
> > > +		 * increment from of_parse_phandle()
> > > +		 */
> ...
> > > +			component_match_add(dev, &match, compare_encoder, remote);
> > > +			/*
> > > +			 * component_match_add keeps a reference to the port
> > > +			 * variable, so we need to keep the reference count
> > > +			 * increment from of_graph_get_remote_port_parent()
> > > +			 */
> 
> This problem no longer exists.

Fair enough, I will send a v3 version.

Thanks,
Liviu

> 
> -- 
> RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/2] drm: Improve drm_of_component_probe() to correctly handle ports and remote ports.

[Liviu Dudau]

On Mon, Feb 22, 2016 at 03:51:29PM +0000, Russell King - ARM Linux wrote:
> On Mon, Feb 22, 2016 at 11:51:47AM +0000, Liviu Dudau wrote:
> > On Fri, Nov 20, 2015 at 02:22:04PM +0000, Liviu Dudau wrote:
> > > Rockchip DRM driver cannot use the same compare_of() function to
> > > match ports and remote ports (aka encoders) as their OF sub-trees
> > > look different. Add a second compare function to be used when encoders
> > > are added to the component framework and patch the existing users of
> > > the function accordingly.
> > > 
> > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > 
> > Russell,
> > 
> > Resurecting this old patch from around Christmas time (bad time for patch
> > review).
> > 
> > Are you happy enough with this version to re-issue the Ack or do you think
> > I still need to work on it?
> 
> What I'd like to see is the patch reworked, because:
> 
> > > +		component_match_add(dev, &match, compare_port, port);
> > > +		/*
> > > +		 * component_match_add keeps a reference to the port
> > > +		 * variable, so we need to keep the reference count
> > > +		 * increment from of_parse_phandle()
> > > +		 */
> ...
> > > +			component_match_add(dev, &match, compare_encoder, remote);
> > > +			/*
> > > +			 * component_match_add keeps a reference to the port
> > > +			 * variable, so we need to keep the reference count
> > > +			 * increment from of_graph_get_remote_port_parent()
> > > +			 */
> 
> This problem no longer exists.

Fair enough, I will send a v3 version.

Thanks,
Liviu

> 
> -- 
> RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?