[PATCH v4 0/4] drm: Cleanup probe function for component based masters.

[PATCH v4 0/4] drm: Cleanup probe function for component based masters.

[Liviu Dudau]

Changelog:
v4: Fixed a bug where the wrong pointer was sent to component_match_add() and
    component_master_add_with_match() in the armada_drv.c file that was flagged
    by kbuild test robot. Dropped the RFC tag and added Acked-bys received from
    Russell King.
v3: Removed the call to dma_set_coherent_mask() from the generic
    drm_of_component_probe(). Also changes to shorten lines over 80 chars long.
v2: Rebased the patchset on top of drm-next rather than Linus' latest -rc

A few drivers in drivers/gpu/drm are component-enabled and use quite similar
code sequences to probe for their encoder slaves at the remote end of the ports.
Move the code into a "generic" function and remove it from the drivers.

The end results is that drivers get a reference count fix (imx), more thorough
error checking (imx again) plus a decrease in the overall count of LoC.

I'm looking for comments and testing of the patchset (only compile tested from my
end as I don't have access to all the devices touched by the changes). My main
interest is in finding out if -EINVAL is the correct code to return if
dev->of_node == NULL (handy now, as it is different from the other possible error
codes and used in armada to trigger old platform_data support. Also looking for
thoughts on the correctness of the patch and if it possible to co-opt more drivers
into using the function.

Best regards,
Liviu

Liviu Dudau (4):
  drm: Introduce generic probe function for component based masters.
  drm/imx: Convert the probe function to the generic drm_of_component_probe()
  drm/rockchip: Convert the probe function to the generic drm_of_component_probe()
  drm/armada: Convert the probe function to the generic drm_of_component_probe()

 drivers/gpu/drm/armada/armada_drv.c         | 68 +++++++---------------
 drivers/gpu/drm/drm_of.c                    | 88 +++++++++++++++++++++++++++++
 drivers/gpu/drm/imx/imx-drm-core.c          | 55 ++----------------
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 81 ++------------------------
 include/drm/drm_of.h                        | 13 +++++
 5 files changed, 130 insertions(+), 175 deletions(-)

-- 
2.6.0

[PATCH v4 0/4] drm: Cleanup probe function for component based masters.

[Liviu Dudau]

Changelog:
v4: Fixed a bug where the wrong pointer was sent to component_match_add() and
    component_master_add_with_match() in the armada_drv.c file that was flagged
    by kbuild test robot. Dropped the RFC tag and added Acked-bys received from
    Russell King.
v3: Removed the call to dma_set_coherent_mask() from the generic
    drm_of_component_probe(). Also changes to shorten lines over 80 chars long.
v2: Rebased the patchset on top of drm-next rather than Linus' latest -rc

A few drivers in drivers/gpu/drm are component-enabled and use quite similar
code sequences to probe for their encoder slaves at the remote end of the ports.
Move the code into a "generic" function and remove it from the drivers.

The end results is that drivers get a reference count fix (imx), more thorough
error checking (imx again) plus a decrease in the overall count of LoC.

I'm looking for comments and testing of the patchset (only compile tested from my
end as I don't have access to all the devices touched by the changes). My main
interest is in finding out if -EINVAL is the correct code to return if
dev->of_node == NULL (handy now, as it is different from the other possible error
codes and used in armada to trigger old platform_data support. Also looking for
thoughts on the correctness of the patch and if it possible to co-opt more drivers
into using the function.

Best regards,
Liviu

Liviu Dudau (4):
  drm: Introduce generic probe function for component based masters.
  drm/imx: Convert the probe function to the generic drm_of_component_probe()
  drm/rockchip: Convert the probe function to the generic drm_of_component_probe()
  drm/armada: Convert the probe function to the generic drm_of_component_probe()

 drivers/gpu/drm/armada/armada_drv.c         | 68 +++++++---------------
 drivers/gpu/drm/drm_of.c                    | 88 +++++++++++++++++++++++++++++
 drivers/gpu/drm/imx/imx-drm-core.c          | 55 ++----------------
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 81 ++------------------------
 include/drm/drm_of.h                        | 13 +++++
 5 files changed, 130 insertions(+), 175 deletions(-)

-- 
2.6.0

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

[PATCH v4 0/4] drm: Cleanup probe function for component based masters.

[Liviu Dudau]

Changelog:
v4: Fixed a bug where the wrong pointer was sent to component_match_add() and
    component_master_add_with_match() in the armada_drv.c file that was flagged
    by kbuild test robot. Dropped the RFC tag and added Acked-bys received from
    Russell King.
v3: Removed the call to dma_set_coherent_mask() from the generic
    drm_of_component_probe(). Also changes to shorten lines over 80 chars long.
v2: Rebased the patchset on top of drm-next rather than Linus' latest -rc

A few drivers in drivers/gpu/drm are component-enabled and use quite similar
code sequences to probe for their encoder slaves at the remote end of the ports.
Move the code into a "generic" function and remove it from the drivers.

The end results is that drivers get a reference count fix (imx), more thorough
error checking (imx again) plus a decrease in the overall count of LoC.

I'm looking for comments and testing of the patchset (only compile tested from my
end as I don't have access to all the devices touched by the changes). My main
interest is in finding out if -EINVAL is the correct code to return if
dev->of_node == NULL (handy now, as it is different from the other possible error
codes and used in armada to trigger old platform_data support. Also looking for
thoughts on the correctness of the patch and if it possible to co-opt more drivers
into using the function.

Best regards,
Liviu

Liviu Dudau (4):
  drm: Introduce generic probe function for component based masters.
  drm/imx: Convert the probe function to the generic drm_of_component_probe()
  drm/rockchip: Convert the probe function to the generic drm_of_component_probe()
  drm/armada: Convert the probe function to the generic drm_of_component_probe()

 drivers/gpu/drm/armada/armada_drv.c         | 68 +++++++---------------
 drivers/gpu/drm/drm_of.c                    | 88 +++++++++++++++++++++++++++++
 drivers/gpu/drm/imx/imx-drm-core.c          | 55 ++----------------
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 81 ++------------------------
 include/drm/drm_of.h                        | 13 +++++
 5 files changed, 130 insertions(+), 175 deletions(-)

-- 
2.6.0

[PATCH v4 1/4] drm: Introduce generic probe function for component based masters.

[Liviu Dudau]

A lot of component based DRM drivers use a variant of the same code
as the probe function. They bind the crtc ports in the first iteration
and then scan through the child nodes and bind the encoders attached
to the remote endpoints. Factor the common code into a separate
function called drm_of_component_probe() in order to increase code
reuse.

Cc: David Airlie <airlied@linux.ie>
Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/drm_of.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_of.h     | 13 +++++++
 2 files changed, 101 insertions(+)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index be38840..493c05c 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -1,3 +1,4 @@
+#include <linux/component.h>
 #include <linux/export.h>
 #include <linux/list.h>
 #include <linux/of_graph.h>
@@ -61,3 +62,90 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 	return possible_crtcs;
 }
 EXPORT_SYMBOL(drm_of_find_possible_crtcs);
+
+/**
+ * drm_of_component_probe - Generic probe function for a component based master
+ * @dev: master device containing the OF node
+ * @compare_of: compare function used for matching components
+ * @master_ops: component master ops to be used
+ *
+ * Parse the platform device OF node and bind all the components associated
+ * with the master. Interface ports are added before the encoders in order to
+ * satisfy their .bind requirements
+ * See Documentation/devicetree/bindings/graph.txt for the bindings.
+ *
+ * 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 *),
+			   const struct component_master_ops *m_ops)
+{
+	struct device_node *ep, *port, *remote;
+	struct component_match *match = NULL;
+	int i;
+
+	if (!dev->of_node)
+		return -EINVAL;
+
+	/*
+	 * Bind the crtc's ports first, so that drm_of_find_possible_crtcs()
+	 * called from encoder's .bind callbacks works as expected
+	 */
+	for (i = 0; ; i++) {
+		port = of_parse_phandle(dev->of_node, "ports", i);
+		if (!port)
+			break;
+
+		if (!of_device_is_available(port->parent)) {
+			of_node_put(port);
+			continue;
+		}
+
+		component_match_add(dev, &match, compare_of, port);
+		of_node_put(port);
+	}
+
+	if (i == 0) {
+		dev_err(dev, "missing 'ports' property\n");
+		return -ENODEV;
+	}
+
+	if (!match) {
+		dev_err(dev, "no available port\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * For bound crtcs, bind the encoders attached to their remote endpoint
+	 */
+	for (i = 0; ; i++) {
+		port = of_parse_phandle(dev->of_node, "ports", i);
+		if (!port)
+			break;
+
+		if (!of_device_is_available(port->parent)) {
+			of_node_put(port);
+			continue;
+		}
+
+		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);
+		}
+		of_node_put(port);
+	}
+
+	return component_master_add_with_match(dev, m_ops, match);
+}
+EXPORT_SYMBOL(drm_of_component_probe);
diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
index 2441f71..8544665 100644
--- a/include/drm/drm_of.h
+++ b/include/drm/drm_of.h
@@ -1,18 +1,31 @@
 #ifndef __DRM_OF_H__
 #define __DRM_OF_H__
 
+struct component_master_ops;
+struct device;
 struct drm_device;
 struct device_node;
 
 #ifdef CONFIG_OF
 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 *),
+				  const struct component_master_ops *m_ops);
 #else
 static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 						  struct device_node *port)
 {
 	return 0;
 }
+
+static inline int
+drm_of_component_probe(struct device *dev,
+		       int (*compare_of)(struct device *, void *),
+		       const struct component_master_ops *m_ops)
+{
+	return -EINVAL;
+}
 #endif
 
 #endif /* __DRM_OF_H__ */
-- 
2.6.0

[PATCH v4 1/4] drm: Introduce generic probe function for component based masters.

[Liviu Dudau]

A lot of component based DRM drivers use a variant of the same code
as the probe function. They bind the crtc ports in the first iteration
and then scan through the child nodes and bind the encoders attached
to the remote endpoints. Factor the common code into a separate
function called drm_of_component_probe() in order to increase code
reuse.

Cc: David Airlie <airlied@linux.ie>
Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/drm_of.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_of.h     | 13 +++++++
 2 files changed, 101 insertions(+)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index be38840..493c05c 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -1,3 +1,4 @@
+#include <linux/component.h>
 #include <linux/export.h>
 #include <linux/list.h>
 #include <linux/of_graph.h>
@@ -61,3 +62,90 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 	return possible_crtcs;
 }
 EXPORT_SYMBOL(drm_of_find_possible_crtcs);
+
+/**
+ * drm_of_component_probe - Generic probe function for a component based master
+ * @dev: master device containing the OF node
+ * @compare_of: compare function used for matching components
+ * @master_ops: component master ops to be used
+ *
+ * Parse the platform device OF node and bind all the components associated
+ * with the master. Interface ports are added before the encoders in order to
+ * satisfy their .bind requirements
+ * See Documentation/devicetree/bindings/graph.txt for the bindings.
+ *
+ * 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 *),
+			   const struct component_master_ops *m_ops)
+{
+	struct device_node *ep, *port, *remote;
+	struct component_match *match = NULL;
+	int i;
+
+	if (!dev->of_node)
+		return -EINVAL;
+
+	/*
+	 * Bind the crtc's ports first, so that drm_of_find_possible_crtcs()
+	 * called from encoder's .bind callbacks works as expected
+	 */
+	for (i = 0; ; i++) {
+		port = of_parse_phandle(dev->of_node, "ports", i);
+		if (!port)
+			break;
+
+		if (!of_device_is_available(port->parent)) {
+			of_node_put(port);
+			continue;
+		}
+
+		component_match_add(dev, &match, compare_of, port);
+		of_node_put(port);
+	}
+
+	if (i == 0) {
+		dev_err(dev, "missing 'ports' property\n");
+		return -ENODEV;
+	}
+
+	if (!match) {
+		dev_err(dev, "no available port\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * For bound crtcs, bind the encoders attached to their remote endpoint
+	 */
+	for (i = 0; ; i++) {
+		port = of_parse_phandle(dev->of_node, "ports", i);
+		if (!port)
+			break;
+
+		if (!of_device_is_available(port->parent)) {
+			of_node_put(port);
+			continue;
+		}
+
+		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);
+		}
+		of_node_put(port);
+	}
+
+	return component_master_add_with_match(dev, m_ops, match);
+}
+EXPORT_SYMBOL(drm_of_component_probe);
diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
index 2441f71..8544665 100644
--- a/include/drm/drm_of.h
+++ b/include/drm/drm_of.h
@@ -1,18 +1,31 @@
 #ifndef __DRM_OF_H__
 #define __DRM_OF_H__
 
+struct component_master_ops;
+struct device;
 struct drm_device;
 struct device_node;
 
 #ifdef CONFIG_OF
 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 *),
+				  const struct component_master_ops *m_ops);
 #else
 static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 						  struct device_node *port)
 {
 	return 0;
 }
+
+static inline int
+drm_of_component_probe(struct device *dev,
+		       int (*compare_of)(struct device *, void *),
+		       const struct component_master_ops *m_ops)
+{
+	return -EINVAL;
+}
 #endif
 
 #endif /* __DRM_OF_H__ */
-- 
2.6.0

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

[PATCH v4 1/4] drm: Introduce generic probe function for component based masters.

[Liviu Dudau]

A lot of component based DRM drivers use a variant of the same code
as the probe function. They bind the crtc ports in the first iteration
and then scan through the child nodes and bind the encoders attached
to the remote endpoints. Factor the common code into a separate
function called drm_of_component_probe() in order to increase code
reuse.

Cc: David Airlie <airlied@linux.ie>
Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/drm_of.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_of.h     | 13 +++++++
 2 files changed, 101 insertions(+)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index be38840..493c05c 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -1,3 +1,4 @@
+#include <linux/component.h>
 #include <linux/export.h>
 #include <linux/list.h>
 #include <linux/of_graph.h>
@@ -61,3 +62,90 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 	return possible_crtcs;
 }
 EXPORT_SYMBOL(drm_of_find_possible_crtcs);
+
+/**
+ * drm_of_component_probe - Generic probe function for a component based master
+ * @dev: master device containing the OF node
+ * @compare_of: compare function used for matching components
+ * @master_ops: component master ops to be used
+ *
+ * Parse the platform device OF node and bind all the components associated
+ * with the master. Interface ports are added before the encoders in order to
+ * satisfy their .bind requirements
+ * See Documentation/devicetree/bindings/graph.txt for the bindings.
+ *
+ * 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 *),
+			   const struct component_master_ops *m_ops)
+{
+	struct device_node *ep, *port, *remote;
+	struct component_match *match = NULL;
+	int i;
+
+	if (!dev->of_node)
+		return -EINVAL;
+
+	/*
+	 * Bind the crtc's ports first, so that drm_of_find_possible_crtcs()
+	 * called from encoder's .bind callbacks works as expected
+	 */
+	for (i = 0; ; i++) {
+		port = of_parse_phandle(dev->of_node, "ports", i);
+		if (!port)
+			break;
+
+		if (!of_device_is_available(port->parent)) {
+			of_node_put(port);
+			continue;
+		}
+
+		component_match_add(dev, &match, compare_of, port);
+		of_node_put(port);
+	}
+
+	if (i == 0) {
+		dev_err(dev, "missing 'ports' property\n");
+		return -ENODEV;
+	}
+
+	if (!match) {
+		dev_err(dev, "no available port\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * For bound crtcs, bind the encoders attached to their remote endpoint
+	 */
+	for (i = 0; ; i++) {
+		port = of_parse_phandle(dev->of_node, "ports", i);
+		if (!port)
+			break;
+
+		if (!of_device_is_available(port->parent)) {
+			of_node_put(port);
+			continue;
+		}
+
+		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);
+		}
+		of_node_put(port);
+	}
+
+	return component_master_add_with_match(dev, m_ops, match);
+}
+EXPORT_SYMBOL(drm_of_component_probe);
diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
index 2441f71..8544665 100644
--- a/include/drm/drm_of.h
+++ b/include/drm/drm_of.h
@@ -1,18 +1,31 @@
 #ifndef __DRM_OF_H__
 #define __DRM_OF_H__
 
+struct component_master_ops;
+struct device;
 struct drm_device;
 struct device_node;
 
 #ifdef CONFIG_OF
 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 *),
+				  const struct component_master_ops *m_ops);
 #else
 static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 						  struct device_node *port)
 {
 	return 0;
 }
+
+static inline int
+drm_of_component_probe(struct device *dev,
+		       int (*compare_of)(struct device *, void *),
+		       const struct component_master_ops *m_ops)
+{
+	return -EINVAL;
+}
 #endif
 
 #endif /* __DRM_OF_H__ */
-- 
2.6.0

[PATCH v4 2/4] drm/imx: Convert the probe function to the generic drm_of_component_probe()

[Liviu Dudau]

The generic function is functionally equivalent to the driver's
imx_drm_platform_probe(). Use the generic function and reduce the
overall code size.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/imx/imx-drm-core.c | 55 +++-----------------------------------
 1 file changed, 4 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index de00a6c..64f16ea 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -531,59 +531,12 @@ static const struct component_master_ops imx_drm_ops = {
 
 static int imx_drm_platform_probe(struct platform_device *pdev)
 {
-	struct device_node *ep, *port, *remote;
-	struct component_match *match = NULL;
-	int ret;
-	int i;
-
-	/*
-	 * Bind the IPU display interface ports first, so that
-	 * imx_drm_encoder_parse_of called from encoder .bind callbacks
-	 * works as expected.
-	 */
-	for (i = 0; ; i++) {
-		port = of_parse_phandle(pdev->dev.of_node, "ports", i);
-		if (!port)
-			break;
-
-		component_match_add(&pdev->dev, &match, compare_of, port);
-	}
+	int ret = drm_of_component_probe(&pdev->dev, compare_of, &imx_drm_ops);
 
-	if (i == 0) {
-		dev_err(&pdev->dev, "missing 'ports' property\n");
-		return -ENODEV;
-	}
+	if (!ret)
+		ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
 
-	/* Then bind all encoders */
-	for (i = 0; ; i++) {
-		port = of_parse_phandle(pdev->dev.of_node, "ports", i);
-		if (!port)
-			break;
-
-		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(&pdev->dev, "parent device of %s is not available\n",
-					 remote->full_name);
-				of_node_put(remote);
-				continue;
-			}
-
-			component_match_add(&pdev->dev, &match, compare_of,
-					    remote);
-			of_node_put(remote);
-		}
-		of_node_put(port);
-	}
-
-	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
-	if (ret)
-		return ret;
-
-	return component_master_add_with_match(&pdev->dev, &imx_drm_ops, match);
+	return ret;
 }
 
 static int imx_drm_platform_remove(struct platform_device *pdev)
-- 
2.6.0

[PATCH v4 2/4] drm/imx: Convert the probe function to the generic drm_of_component_probe()

[Liviu Dudau]

The generic function is functionally equivalent to the driver's
imx_drm_platform_probe(). Use the generic function and reduce the
overall code size.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/imx/imx-drm-core.c | 55 +++-----------------------------------
 1 file changed, 4 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index de00a6c..64f16ea 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -531,59 +531,12 @@ static const struct component_master_ops imx_drm_ops = {
 
 static int imx_drm_platform_probe(struct platform_device *pdev)
 {
-	struct device_node *ep, *port, *remote;
-	struct component_match *match = NULL;
-	int ret;
-	int i;
-
-	/*
-	 * Bind the IPU display interface ports first, so that
-	 * imx_drm_encoder_parse_of called from encoder .bind callbacks
-	 * works as expected.
-	 */
-	for (i = 0; ; i++) {
-		port = of_parse_phandle(pdev->dev.of_node, "ports", i);
-		if (!port)
-			break;
-
-		component_match_add(&pdev->dev, &match, compare_of, port);
-	}
+	int ret = drm_of_component_probe(&pdev->dev, compare_of, &imx_drm_ops);
 
-	if (i == 0) {
-		dev_err(&pdev->dev, "missing 'ports' property\n");
-		return -ENODEV;
-	}
+	if (!ret)
+		ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
 
-	/* Then bind all encoders */
-	for (i = 0; ; i++) {
-		port = of_parse_phandle(pdev->dev.of_node, "ports", i);
-		if (!port)
-			break;
-
-		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(&pdev->dev, "parent device of %s is not available\n",
-					 remote->full_name);
-				of_node_put(remote);
-				continue;
-			}
-
-			component_match_add(&pdev->dev, &match, compare_of,
-					    remote);
-			of_node_put(remote);
-		}
-		of_node_put(port);
-	}
-
-	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
-	if (ret)
-		return ret;
-
-	return component_master_add_with_match(&pdev->dev, &imx_drm_ops, match);
+	return ret;
 }
 
 static int imx_drm_platform_remove(struct platform_device *pdev)
-- 
2.6.0

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

[PATCH v4 2/4] drm/imx: Convert the probe function to the generic drm_of_component_probe()

[Liviu Dudau]

The generic function is functionally equivalent to the driver's
imx_drm_platform_probe(). Use the generic function and reduce the
overall code size.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/imx/imx-drm-core.c | 55 +++-----------------------------------
 1 file changed, 4 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index de00a6c..64f16ea 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -531,59 +531,12 @@ static const struct component_master_ops imx_drm_ops = {
 
 static int imx_drm_platform_probe(struct platform_device *pdev)
 {
-	struct device_node *ep, *port, *remote;
-	struct component_match *match = NULL;
-	int ret;
-	int i;
-
-	/*
-	 * Bind the IPU display interface ports first, so that
-	 * imx_drm_encoder_parse_of called from encoder .bind callbacks
-	 * works as expected.
-	 */
-	for (i = 0; ; i++) {
-		port = of_parse_phandle(pdev->dev.of_node, "ports", i);
-		if (!port)
-			break;
-
-		component_match_add(&pdev->dev, &match, compare_of, port);
-	}
+	int ret = drm_of_component_probe(&pdev->dev, compare_of, &imx_drm_ops);
 
-	if (i == 0) {
-		dev_err(&pdev->dev, "missing 'ports' property\n");
-		return -ENODEV;
-	}
+	if (!ret)
+		ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
 
-	/* Then bind all encoders */
-	for (i = 0; ; i++) {
-		port = of_parse_phandle(pdev->dev.of_node, "ports", i);
-		if (!port)
-			break;
-
-		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(&pdev->dev, "parent device of %s is not available\n",
-					 remote->full_name);
-				of_node_put(remote);
-				continue;
-			}
-
-			component_match_add(&pdev->dev, &match, compare_of,
-					    remote);
-			of_node_put(remote);
-		}
-		of_node_put(port);
-	}
-
-	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
-	if (ret)
-		return ret;
-
-	return component_master_add_with_match(&pdev->dev, &imx_drm_ops, match);
+	return ret;
 }
 
 static int imx_drm_platform_remove(struct platform_device *pdev)
-- 
2.6.0

[PATCH v4 3/4] drm/rockchip: Convert the probe function to the generic drm_of_component_probe()

[Liviu Dudau]

Use the generic drm_of_component_probe() function to probe for components.

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

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f22e1e1..d26e0cc 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>
@@ -418,29 +419,6 @@ static int compare_of(struct device *dev, void *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;
@@ -483,61 +461,14 @@ static const struct component_master_ops rockchip_drm_ops = {
 
 static int rockchip_drm_platform_probe(struct platform_device *pdev)
 {
-	struct device *dev = &pdev->dev;
-	struct component_match *match = NULL;
-	struct device_node *np = dev->of_node;
-	struct device_node *port;
-	int i;
-
-	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;
-		}
-
-		component_match_add(dev, &match, compare_of, port->parent);
-		of_node_put(port);
-	}
+	int ret = drm_of_component_probe(&pdev->dev, compare_of,
+					 &rockchip_drm_ops);
 
-	if (i == 0) {
-		dev_err(dev, "missing 'ports' property\n");
+	/* keep compatibility with old code that was returning -ENODEV */
+	if (ret == -EINVAL)
 		return -ENODEV;
-	}
 
-	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;
-		}
-
-		rockchip_add_endpoints(dev, &match, port);
-		of_node_put(port);
-	}
-
-	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.0

[PATCH v4 3/4] drm/rockchip: Convert the probe function to the generic drm_of_component_probe()

[Liviu Dudau]

Use the generic drm_of_component_probe() function to probe for components.

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

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f22e1e1..d26e0cc 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>
@@ -418,29 +419,6 @@ static int compare_of(struct device *dev, void *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;
@@ -483,61 +461,14 @@ static const struct component_master_ops rockchip_drm_ops = {
 
 static int rockchip_drm_platform_probe(struct platform_device *pdev)
 {
-	struct device *dev = &pdev->dev;
-	struct component_match *match = NULL;
-	struct device_node *np = dev->of_node;
-	struct device_node *port;
-	int i;
-
-	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;
-		}
-
-		component_match_add(dev, &match, compare_of, port->parent);
-		of_node_put(port);
-	}
+	int ret = drm_of_component_probe(&pdev->dev, compare_of,
+					 &rockchip_drm_ops);
 
-	if (i == 0) {
-		dev_err(dev, "missing 'ports' property\n");
+	/* keep compatibility with old code that was returning -ENODEV */
+	if (ret == -EINVAL)
 		return -ENODEV;
-	}
 
-	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;
-		}
-
-		rockchip_add_endpoints(dev, &match, port);
-		of_node_put(port);
-	}
-
-	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.0

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

[PATCH v4 3/4] drm/rockchip: Convert the probe function to the generic drm_of_component_probe()

[Liviu Dudau]

Use the generic drm_of_component_probe() function to probe for components.

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

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f22e1e1..d26e0cc 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>
@@ -418,29 +419,6 @@ static int compare_of(struct device *dev, void *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;
@@ -483,61 +461,14 @@ static const struct component_master_ops rockchip_drm_ops = {
 
 static int rockchip_drm_platform_probe(struct platform_device *pdev)
 {
-	struct device *dev = &pdev->dev;
-	struct component_match *match = NULL;
-	struct device_node *np = dev->of_node;
-	struct device_node *port;
-	int i;
-
-	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;
-		}
-
-		component_match_add(dev, &match, compare_of, port->parent);
-		of_node_put(port);
-	}
+	int ret = drm_of_component_probe(&pdev->dev, compare_of,
+					 &rockchip_drm_ops);
 
-	if (i == 0) {
-		dev_err(dev, "missing 'ports' property\n");
+	/* keep compatibility with old code that was returning -ENODEV */
+	if (ret == -EINVAL)
 		return -ENODEV;
-	}
 
-	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;
-		}
-
-		rockchip_add_endpoints(dev, &match, port);
-		of_node_put(port);
-	}
-
-	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.0

[PATCH v4 4/4] drm/armada: Convert the probe function to the generic drm_of_component_probe()

[Liviu Dudau]

The armada DRM driver keeps some old platform data compatibility in the
probe function that makes moving to the generic drm_of_component_probe()
a bit more complicated that it should. Refactor the probe function to do
the platform_data processing after the generic probe (and only if that
fails). This way future cleanup can further remove support for it.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/armada/armada_drv.c | 68 +++++++++++--------------------------
 1 file changed, 19 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 63d909e..3a7ba69 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -11,6 +11,7 @@
 #include <linux/of_graph.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_of.h>
 #include "armada_crtc.h"
 #include "armada_drm.h"
 #include "armada_gem.h"
@@ -265,43 +266,29 @@ static void armada_add_endpoints(struct device *dev,
 	}
 }
 
-static int armada_drm_find_components(struct device *dev,
-	struct component_match **match)
-{
-	struct device_node *port;
-	int i;
-
-	if (dev->of_node) {
-		struct device_node *np = dev->of_node;
-
-		for (i = 0; ; i++) {
-			port = of_parse_phandle(np, "ports", i);
-			if (!port)
-				break;
-
-			component_match_add(dev, match, compare_of, port);
-			of_node_put(port);
-		}
+static const struct component_master_ops armada_master_ops = {
+	.bind = armada_drm_bind,
+	.unbind = armada_drm_unbind,
+};
 
-		if (i == 0) {
-			dev_err(dev, "missing 'ports' property\n");
-			return -ENODEV;
-		}
+static int armada_drm_probe(struct platform_device *pdev)
+{
+	struct component_match *match = NULL;
+	struct device *dev = &pdev->dev;
+	int ret;
 
-		for (i = 0; ; i++) {
-			port = of_parse_phandle(np, "ports", i);
-			if (!port)
-				break;
+	ret = drm_of_component_probe(dev, compare_dev_name, &armada_master_ops);
+	if (ret != -EINVAL)
+		return ret;
 
-			armada_add_endpoints(dev, match, port);
-			of_node_put(port);
-		}
-	} else if (dev->platform_data) {
+	if (dev->platform_data) {
 		char **devices = dev->platform_data;
+		struct device_node *port;
 		struct device *d;
+		int i;
 
 		for (i = 0; devices[i]; i++)
-			component_match_add(dev, match, compare_dev_name,
+			component_match_add(dev, &match, compare_dev_name,
 					    devices[i]);
 
 		if (i == 0) {
@@ -311,32 +298,15 @@ static int armada_drm_find_components(struct device *dev,
 
 		for (i = 0; devices[i]; i++) {
 			d = bus_find_device_by_name(&platform_bus_type, NULL,
-					devices[i]);
+						    devices[i]);
 			if (d && d->of_node) {
 				for_each_child_of_node(d->of_node, port)
-					armada_add_endpoints(dev, match, port);
+					armada_add_endpoints(dev, &match, port);
 			}
 			put_device(d);
 		}
 	}
 
-	return 0;
-}
-
-static const struct component_master_ops armada_master_ops = {
-	.bind = armada_drm_bind,
-	.unbind = armada_drm_unbind,
-};
-
-static int armada_drm_probe(struct platform_device *pdev)
-{
-	struct component_match *match = NULL;
-	int ret;
-
-	ret = armada_drm_find_components(&pdev->dev, &match);
-	if (ret < 0)
-		return ret;
-
 	return component_master_add_with_match(&pdev->dev, &armada_master_ops,
 					       match);
 }
-- 
2.6.0

[PATCH v4 4/4] drm/armada: Convert the probe function to the generic drm_of_component_probe()

[Liviu Dudau]

The armada DRM driver keeps some old platform data compatibility in the
probe function that makes moving to the generic drm_of_component_probe()
a bit more complicated that it should. Refactor the probe function to do
the platform_data processing after the generic probe (and only if that
fails). This way future cleanup can further remove support for it.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/armada/armada_drv.c | 68 +++++++++++--------------------------
 1 file changed, 19 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 63d909e..3a7ba69 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -11,6 +11,7 @@
 #include <linux/of_graph.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_of.h>
 #include "armada_crtc.h"
 #include "armada_drm.h"
 #include "armada_gem.h"
@@ -265,43 +266,29 @@ static void armada_add_endpoints(struct device *dev,
 	}
 }
 
-static int armada_drm_find_components(struct device *dev,
-	struct component_match **match)
-{
-	struct device_node *port;
-	int i;
-
-	if (dev->of_node) {
-		struct device_node *np = dev->of_node;
-
-		for (i = 0; ; i++) {
-			port = of_parse_phandle(np, "ports", i);
-			if (!port)
-				break;
-
-			component_match_add(dev, match, compare_of, port);
-			of_node_put(port);
-		}
+static const struct component_master_ops armada_master_ops = {
+	.bind = armada_drm_bind,
+	.unbind = armada_drm_unbind,
+};
 
-		if (i == 0) {
-			dev_err(dev, "missing 'ports' property\n");
-			return -ENODEV;
-		}
+static int armada_drm_probe(struct platform_device *pdev)
+{
+	struct component_match *match = NULL;
+	struct device *dev = &pdev->dev;
+	int ret;
 
-		for (i = 0; ; i++) {
-			port = of_parse_phandle(np, "ports", i);
-			if (!port)
-				break;
+	ret = drm_of_component_probe(dev, compare_dev_name, &armada_master_ops);
+	if (ret != -EINVAL)
+		return ret;
 
-			armada_add_endpoints(dev, match, port);
-			of_node_put(port);
-		}
-	} else if (dev->platform_data) {
+	if (dev->platform_data) {
 		char **devices = dev->platform_data;
+		struct device_node *port;
 		struct device *d;
+		int i;
 
 		for (i = 0; devices[i]; i++)
-			component_match_add(dev, match, compare_dev_name,
+			component_match_add(dev, &match, compare_dev_name,
 					    devices[i]);
 
 		if (i == 0) {
@@ -311,32 +298,15 @@ static int armada_drm_find_components(struct device *dev,
 
 		for (i = 0; devices[i]; i++) {
 			d = bus_find_device_by_name(&platform_bus_type, NULL,
-					devices[i]);
+						    devices[i]);
 			if (d && d->of_node) {
 				for_each_child_of_node(d->of_node, port)
-					armada_add_endpoints(dev, match, port);
+					armada_add_endpoints(dev, &match, port);
 			}
 			put_device(d);
 		}
 	}
 
-	return 0;
-}
-
-static const struct component_master_ops armada_master_ops = {
-	.bind = armada_drm_bind,
-	.unbind = armada_drm_unbind,
-};
-
-static int armada_drm_probe(struct platform_device *pdev)
-{
-	struct component_match *match = NULL;
-	int ret;
-
-	ret = armada_drm_find_components(&pdev->dev, &match);
-	if (ret < 0)
-		return ret;
-
 	return component_master_add_with_match(&pdev->dev, &armada_master_ops,
 					       match);
 }
-- 
2.6.0

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

[PATCH v4 4/4] drm/armada: Convert the probe function to the generic drm_of_component_probe()

[Liviu Dudau]

The armada DRM driver keeps some old platform data compatibility in the
probe function that makes moving to the generic drm_of_component_probe()
a bit more complicated that it should. Refactor the probe function to do
the platform_data processing after the generic probe (and only if that
fails). This way future cleanup can further remove support for it.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/armada/armada_drv.c | 68 +++++++++++--------------------------
 1 file changed, 19 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 63d909e..3a7ba69 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -11,6 +11,7 @@
 #include <linux/of_graph.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_of.h>
 #include "armada_crtc.h"
 #include "armada_drm.h"
 #include "armada_gem.h"
@@ -265,43 +266,29 @@ static void armada_add_endpoints(struct device *dev,
 	}
 }
 
-static int armada_drm_find_components(struct device *dev,
-	struct component_match **match)
-{
-	struct device_node *port;
-	int i;
-
-	if (dev->of_node) {
-		struct device_node *np = dev->of_node;
-
-		for (i = 0; ; i++) {
-			port = of_parse_phandle(np, "ports", i);
-			if (!port)
-				break;
-
-			component_match_add(dev, match, compare_of, port);
-			of_node_put(port);
-		}
+static const struct component_master_ops armada_master_ops = {
+	.bind = armada_drm_bind,
+	.unbind = armada_drm_unbind,
+};
 
-		if (i == 0) {
-			dev_err(dev, "missing 'ports' property\n");
-			return -ENODEV;
-		}
+static int armada_drm_probe(struct platform_device *pdev)
+{
+	struct component_match *match = NULL;
+	struct device *dev = &pdev->dev;
+	int ret;
 
-		for (i = 0; ; i++) {
-			port = of_parse_phandle(np, "ports", i);
-			if (!port)
-				break;
+	ret = drm_of_component_probe(dev, compare_dev_name, &armada_master_ops);
+	if (ret != -EINVAL)
+		return ret;
 
-			armada_add_endpoints(dev, match, port);
-			of_node_put(port);
-		}
-	} else if (dev->platform_data) {
+	if (dev->platform_data) {
 		char **devices = dev->platform_data;
+		struct device_node *port;
 		struct device *d;
+		int i;
 
 		for (i = 0; devices[i]; i++)
-			component_match_add(dev, match, compare_dev_name,
+			component_match_add(dev, &match, compare_dev_name,
 					    devices[i]);
 
 		if (i == 0) {
@@ -311,32 +298,15 @@ static int armada_drm_find_components(struct device *dev,
 
 		for (i = 0; devices[i]; i++) {
 			d = bus_find_device_by_name(&platform_bus_type, NULL,
-					devices[i]);
+						    devices[i]);
 			if (d && d->of_node) {
 				for_each_child_of_node(d->of_node, port)
-					armada_add_endpoints(dev, match, port);
+					armada_add_endpoints(dev, &match, port);
 			}
 			put_device(d);
 		}
 	}
 
-	return 0;
-}
-
-static const struct component_master_ops armada_master_ops = {
-	.bind = armada_drm_bind,
-	.unbind = armada_drm_unbind,
-};
-
-static int armada_drm_probe(struct platform_device *pdev)
-{
-	struct component_match *match = NULL;
-	int ret;
-
-	ret = armada_drm_find_components(&pdev->dev, &match);
-	if (ret < 0)
-		return ret;
-
 	return component_master_add_with_match(&pdev->dev, &armada_master_ops,
 					       match);
 }
-- 
2.6.0

Re: [PATCH v4 1/4] drm: Introduce generic probe function for component based masters.

[Emil Velikov]

Hi Liviu,

On 20 October 2015 at 10:23, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> A lot of component based DRM drivers use a variant of the same code
> as the probe function. They bind the crtc ports in the first iteration
> and then scan through the child nodes and bind the encoders attached
> to the remote endpoints. Factor the common code into a separate
> function called drm_of_component_probe() in order to increase code
> reuse.
>
> Cc: David Airlie <airlied@linux.ie>
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/gpu/drm/drm_of.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_of.h     | 13 +++++++
>  2 files changed, 101 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index be38840..493c05c 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -1,3 +1,4 @@
> +#include <linux/component.h>
>  #include <linux/export.h>
>  #include <linux/list.h>
>  #include <linux/of_graph.h>
> @@ -61,3 +62,90 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
>         return possible_crtcs;
>  }
>  EXPORT_SYMBOL(drm_of_find_possible_crtcs);
> +
> +/**
> + * drm_of_component_probe - Generic probe function for a component based master
> + * @dev: master device containing the OF node
> + * @compare_of: compare function used for matching components
> + * @master_ops: component master ops to be used
> + *
> + * Parse the platform device OF node and bind all the components associated
> + * with the master. Interface ports are added before the encoders in order to
> + * satisfy their .bind requirements
> + * See Documentation/devicetree/bindings/graph.txt for the bindings.
> + *
> + * 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 *),
> +                          const struct component_master_ops *m_ops)
> +{
> +       struct device_node *ep, *port, *remote;
> +       struct component_match *match = NULL;
> +       int i;
> +
> +       if (!dev->of_node)
> +               return -EINVAL;
> +
> +       /*
> +        * Bind the crtc's ports first, so that drm_of_find_possible_crtcs()
> +        * called from encoder's .bind callbacks works as expected
> +        */
> +       for (i = 0; ; i++) {
> +               port = of_parse_phandle(dev->of_node, "ports", i);
> +               if (!port)
> +                       break;
> +
> +               if (!of_device_is_available(port->parent)) {
> +                       of_node_put(port);
> +                       continue;
> +               }
> +
> +               component_match_add(dev, &match, compare_of, port);
> +               of_node_put(port);
> +       }
> +
> +       if (i == 0) {
> +               dev_err(dev, "missing 'ports' property\n");
> +               return -ENODEV;
> +       }
> +
> +       if (!match) {
> +               dev_err(dev, "no available port\n");
> +               return -ENODEV;
> +       }
> +
> +       /*
> +        * For bound crtcs, bind the encoders attached to their remote endpoint
> +        */
> +       for (i = 0; ; i++) {
> +               port = of_parse_phandle(dev->of_node, "ports", i);
> +               if (!port)
> +                       break;
> +
> +               if (!of_device_is_available(port->parent)) {
> +                       of_node_put(port);
> +                       continue;
> +               }
> +
Of the three drivers converted only the rockchip one has the above
of_device_is_available() hunk. Based on the handling in previous loop
I'm not entirely sure if it's needed, but if so shouldn't one mention
the difference when converting the respective drivers ?

I'm not working on/familiar with either of these drivers so this is
just a fly-by comment.

Cheers,
Emil

Re: [PATCH v4 1/4] drm: Introduce generic probe function for component based masters.

[Emil Velikov]

Hi Liviu,

On 20 October 2015 at 10:23, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> A lot of component based DRM drivers use a variant of the same code
> as the probe function. They bind the crtc ports in the first iteration
> and then scan through the child nodes and bind the encoders attached
> to the remote endpoints. Factor the common code into a separate
> function called drm_of_component_probe() in order to increase code
> reuse.
>
> Cc: David Airlie <airlied@linux.ie>
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/gpu/drm/drm_of.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_of.h     | 13 +++++++
>  2 files changed, 101 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index be38840..493c05c 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -1,3 +1,4 @@
> +#include <linux/component.h>
>  #include <linux/export.h>
>  #include <linux/list.h>
>  #include <linux/of_graph.h>
> @@ -61,3 +62,90 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
>         return possible_crtcs;
>  }
>  EXPORT_SYMBOL(drm_of_find_possible_crtcs);
> +
> +/**
> + * drm_of_component_probe - Generic probe function for a component based master
> + * @dev: master device containing the OF node
> + * @compare_of: compare function used for matching components
> + * @master_ops: component master ops to be used
> + *
> + * Parse the platform device OF node and bind all the components associated
> + * with the master. Interface ports are added before the encoders in order to
> + * satisfy their .bind requirements
> + * See Documentation/devicetree/bindings/graph.txt for the bindings.
> + *
> + * 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 *),
> +                          const struct component_master_ops *m_ops)
> +{
> +       struct device_node *ep, *port, *remote;
> +       struct component_match *match = NULL;
> +       int i;
> +
> +       if (!dev->of_node)
> +               return -EINVAL;
> +
> +       /*
> +        * Bind the crtc's ports first, so that drm_of_find_possible_crtcs()
> +        * called from encoder's .bind callbacks works as expected
> +        */
> +       for (i = 0; ; i++) {
> +               port = of_parse_phandle(dev->of_node, "ports", i);
> +               if (!port)
> +                       break;
> +
> +               if (!of_device_is_available(port->parent)) {
> +                       of_node_put(port);
> +                       continue;
> +               }
> +
> +               component_match_add(dev, &match, compare_of, port);
> +               of_node_put(port);
> +       }
> +
> +       if (i == 0) {
> +               dev_err(dev, "missing 'ports' property\n");
> +               return -ENODEV;
> +       }
> +
> +       if (!match) {
> +               dev_err(dev, "no available port\n");
> +               return -ENODEV;
> +       }
> +
> +       /*
> +        * For bound crtcs, bind the encoders attached to their remote endpoint
> +        */
> +       for (i = 0; ; i++) {
> +               port = of_parse_phandle(dev->of_node, "ports", i);
> +               if (!port)
> +                       break;
> +
> +               if (!of_device_is_available(port->parent)) {
> +                       of_node_put(port);
> +                       continue;
> +               }
> +
Of the three drivers converted only the rockchip one has the above
of_device_is_available() hunk. Based on the handling in previous loop
I'm not entirely sure if it's needed, but if so shouldn't one mention
the difference when converting the respective drivers ?

I'm not working on/familiar with either of these drivers so this is
just a fly-by comment.

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

[PATCH v4 1/4] drm: Introduce generic probe function for component based masters.

[Emil Velikov]

Hi Liviu,

On 20 October 2015 at 10:23, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> A lot of component based DRM drivers use a variant of the same code
> as the probe function. They bind the crtc ports in the first iteration
> and then scan through the child nodes and bind the encoders attached
> to the remote endpoints. Factor the common code into a separate
> function called drm_of_component_probe() in order to increase code
> reuse.
>
> Cc: David Airlie <airlied@linux.ie>
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/gpu/drm/drm_of.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_of.h     | 13 +++++++
>  2 files changed, 101 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index be38840..493c05c 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -1,3 +1,4 @@
> +#include <linux/component.h>
>  #include <linux/export.h>
>  #include <linux/list.h>
>  #include <linux/of_graph.h>
> @@ -61,3 +62,90 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
>         return possible_crtcs;
>  }
>  EXPORT_SYMBOL(drm_of_find_possible_crtcs);
> +
> +/**
> + * drm_of_component_probe - Generic probe function for a component based master
> + * @dev: master device containing the OF node
> + * @compare_of: compare function used for matching components
> + * @master_ops: component master ops to be used
> + *
> + * Parse the platform device OF node and bind all the components associated
> + * with the master. Interface ports are added before the encoders in order to
> + * satisfy their .bind requirements
> + * See Documentation/devicetree/bindings/graph.txt for the bindings.
> + *
> + * 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 *),
> +                          const struct component_master_ops *m_ops)
> +{
> +       struct device_node *ep, *port, *remote;
> +       struct component_match *match = NULL;
> +       int i;
> +
> +       if (!dev->of_node)
> +               return -EINVAL;
> +
> +       /*
> +        * Bind the crtc's ports first, so that drm_of_find_possible_crtcs()
> +        * called from encoder's .bind callbacks works as expected
> +        */
> +       for (i = 0; ; i++) {
> +               port = of_parse_phandle(dev->of_node, "ports", i);
> +               if (!port)
> +                       break;
> +
> +               if (!of_device_is_available(port->parent)) {
> +                       of_node_put(port);
> +                       continue;
> +               }
> +
> +               component_match_add(dev, &match, compare_of, port);
> +               of_node_put(port);
> +       }
> +
> +       if (i == 0) {
> +               dev_err(dev, "missing 'ports' property\n");
> +               return -ENODEV;
> +       }
> +
> +       if (!match) {
> +               dev_err(dev, "no available port\n");
> +               return -ENODEV;
> +       }
> +
> +       /*
> +        * For bound crtcs, bind the encoders attached to their remote endpoint
> +        */
> +       for (i = 0; ; i++) {
> +               port = of_parse_phandle(dev->of_node, "ports", i);
> +               if (!port)
> +                       break;
> +
> +               if (!of_device_is_available(port->parent)) {
> +                       of_node_put(port);
> +                       continue;
> +               }
> +
Of the three drivers converted only the rockchip one has the above
of_device_is_available() hunk. Based on the handling in previous loop
I'm not entirely sure if it's needed, but if so shouldn't one mention
the difference when converting the respective drivers ?

I'm not working on/familiar with either of these drivers so this is
just a fly-by comment.

Cheers,
Emil

Re: [PATCH v4 0/4] drm: Cleanup probe function for component based masters.

[Daniel Vetter]

On Tue, Oct 20, 2015 at 10:23:11AM +0100, Liviu Dudau wrote:
> Changelog:
> v4: Fixed a bug where the wrong pointer was sent to component_match_add() and
>     component_master_add_with_match() in the armada_drv.c file that was flagged
>     by kbuild test robot. Dropped the RFC tag and added Acked-bys received from
>     Russell King.
> v3: Removed the call to dma_set_coherent_mask() from the generic
>     drm_of_component_probe(). Also changes to shorten lines over 80 chars long.
> v2: Rebased the patchset on top of drm-next rather than Linus' latest -rc
> 
> A few drivers in drivers/gpu/drm are component-enabled and use quite similar
> code sequences to probe for their encoder slaves at the remote end of the ports.
> Move the code into a "generic" function and remove it from the drivers.
> 
> The end results is that drivers get a reference count fix (imx), more thorough
> error checking (imx again) plus a decrease in the overall count of LoC.
> 
> I'm looking for comments and testing of the patchset (only compile tested from my
> end as I don't have access to all the devices touched by the changes). My main
> interest is in finding out if -EINVAL is the correct code to return if
> dev->of_node == NULL (handy now, as it is different from the other possible error
> codes and used in armada to trigger old platform_data support. Also looking for
> thoughts on the correctness of the patch and if it possible to co-opt more drivers
> into using the function.

Merged all four to drm-misc, thanks.
-Daniel

> 
> Best regards,
> Liviu
> 
> Liviu Dudau (4):
>   drm: Introduce generic probe function for component based masters.
>   drm/imx: Convert the probe function to the generic drm_of_component_probe()
>   drm/rockchip: Convert the probe function to the generic drm_of_component_probe()
>   drm/armada: Convert the probe function to the generic drm_of_component_probe()
> 
>  drivers/gpu/drm/armada/armada_drv.c         | 68 +++++++---------------
>  drivers/gpu/drm/drm_of.c                    | 88 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/imx/imx-drm-core.c          | 55 ++----------------
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 81 ++------------------------
>  include/drm/drm_of.h                        | 13 +++++
>  5 files changed, 130 insertions(+), 175 deletions(-)
> 
> -- 
> 2.6.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v4 0/4] drm: Cleanup probe function for component based masters.

[Daniel Vetter]

On Tue, Oct 20, 2015 at 10:23:11AM +0100, Liviu Dudau wrote:
> Changelog:
> v4: Fixed a bug where the wrong pointer was sent to component_match_add() and
>     component_master_add_with_match() in the armada_drv.c file that was flagged
>     by kbuild test robot. Dropped the RFC tag and added Acked-bys received from
>     Russell King.
> v3: Removed the call to dma_set_coherent_mask() from the generic
>     drm_of_component_probe(). Also changes to shorten lines over 80 chars long.
> v2: Rebased the patchset on top of drm-next rather than Linus' latest -rc
> 
> A few drivers in drivers/gpu/drm are component-enabled and use quite similar
> code sequences to probe for their encoder slaves at the remote end of the ports.
> Move the code into a "generic" function and remove it from the drivers.
> 
> The end results is that drivers get a reference count fix (imx), more thorough
> error checking (imx again) plus a decrease in the overall count of LoC.
> 
> I'm looking for comments and testing of the patchset (only compile tested from my
> end as I don't have access to all the devices touched by the changes). My main
> interest is in finding out if -EINVAL is the correct code to return if
> dev->of_node == NULL (handy now, as it is different from the other possible error
> codes and used in armada to trigger old platform_data support. Also looking for
> thoughts on the correctness of the patch and if it possible to co-opt more drivers
> into using the function.

Merged all four to drm-misc, thanks.
-Daniel

> 
> Best regards,
> Liviu
> 
> Liviu Dudau (4):
>   drm: Introduce generic probe function for component based masters.
>   drm/imx: Convert the probe function to the generic drm_of_component_probe()
>   drm/rockchip: Convert the probe function to the generic drm_of_component_probe()
>   drm/armada: Convert the probe function to the generic drm_of_component_probe()
> 
>  drivers/gpu/drm/armada/armada_drv.c         | 68 +++++++---------------
>  drivers/gpu/drm/drm_of.c                    | 88 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/imx/imx-drm-core.c          | 55 ++----------------
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 81 ++------------------------
>  include/drm/drm_of.h                        | 13 +++++
>  5 files changed, 130 insertions(+), 175 deletions(-)
> 
> -- 
> 2.6.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 0/4] drm: Cleanup probe function for component based masters.

[Daniel Vetter]

On Tue, Oct 20, 2015 at 10:23:11AM +0100, Liviu Dudau wrote:
> Changelog:
> v4: Fixed a bug where the wrong pointer was sent to component_match_add() and
>     component_master_add_with_match() in the armada_drv.c file that was flagged
>     by kbuild test robot. Dropped the RFC tag and added Acked-bys received from
>     Russell King.
> v3: Removed the call to dma_set_coherent_mask() from the generic
>     drm_of_component_probe(). Also changes to shorten lines over 80 chars long.
> v2: Rebased the patchset on top of drm-next rather than Linus' latest -rc
> 
> A few drivers in drivers/gpu/drm are component-enabled and use quite similar
> code sequences to probe for their encoder slaves at the remote end of the ports.
> Move the code into a "generic" function and remove it from the drivers.
> 
> The end results is that drivers get a reference count fix (imx), more thorough
> error checking (imx again) plus a decrease in the overall count of LoC.
> 
> I'm looking for comments and testing of the patchset (only compile tested from my
> end as I don't have access to all the devices touched by the changes). My main
> interest is in finding out if -EINVAL is the correct code to return if
> dev->of_node == NULL (handy now, as it is different from the other possible error
> codes and used in armada to trigger old platform_data support. Also looking for
> thoughts on the correctness of the patch and if it possible to co-opt more drivers
> into using the function.

Merged all four to drm-misc, thanks.
-Daniel

> 
> Best regards,
> Liviu
> 
> Liviu Dudau (4):
>   drm: Introduce generic probe function for component based masters.
>   drm/imx: Convert the probe function to the generic drm_of_component_probe()
>   drm/rockchip: Convert the probe function to the generic drm_of_component_probe()
>   drm/armada: Convert the probe function to the generic drm_of_component_probe()
> 
>  drivers/gpu/drm/armada/armada_drv.c         | 68 +++++++---------------
>  drivers/gpu/drm/drm_of.c                    | 88 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/imx/imx-drm-core.c          | 55 ++----------------
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 81 ++------------------------
>  include/drm/drm_of.h                        | 13 +++++
>  5 files changed, 130 insertions(+), 175 deletions(-)
> 
> -- 
> 2.6.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v4 1/4] drm: Introduce generic probe function for component based masters.

[Russell King - ARM Linux]

On Tue, Oct 20, 2015 at 11:00:55AM +0100, Emil Velikov wrote:
> Hi Liviu,
> 
> On 20 October 2015 at 10:23, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > A lot of component based DRM drivers use a variant of the same code
> > as the probe function. They bind the crtc ports in the first iteration
> > and then scan through the child nodes and bind the encoders attached
> > to the remote endpoints. Factor the common code into a separate
> > function called drm_of_component_probe() in order to increase code
> > reuse.
> >
> > Cc: David Airlie <airlied@linux.ie>
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  drivers/gpu/drm/drm_of.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_of.h     | 13 +++++++
> >  2 files changed, 101 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > index be38840..493c05c 100644
> > --- a/drivers/gpu/drm/drm_of.c
> > +++ b/drivers/gpu/drm/drm_of.c
> > @@ -1,3 +1,4 @@
> > +#include <linux/component.h>
> >  #include <linux/export.h>
> >  #include <linux/list.h>
> >  #include <linux/of_graph.h>
> > @@ -61,3 +62,90 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> >         return possible_crtcs;
> >  }
> >  EXPORT_SYMBOL(drm_of_find_possible_crtcs);
> > +
> > +/**
> > + * drm_of_component_probe - Generic probe function for a component based master
> > + * @dev: master device containing the OF node
> > + * @compare_of: compare function used for matching components
> > + * @master_ops: component master ops to be used
> > + *
> > + * Parse the platform device OF node and bind all the components associated
> > + * with the master. Interface ports are added before the encoders in order to
> > + * satisfy their .bind requirements
> > + * See Documentation/devicetree/bindings/graph.txt for the bindings.
> > + *
> > + * 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 *),
> > +                          const struct component_master_ops *m_ops)
> > +{
> > +       struct device_node *ep, *port, *remote;
> > +       struct component_match *match = NULL;
> > +       int i;
> > +
> > +       if (!dev->of_node)
> > +               return -EINVAL;
> > +
> > +       /*
> > +        * Bind the crtc's ports first, so that drm_of_find_possible_crtcs()
> > +        * called from encoder's .bind callbacks works as expected
> > +        */
> > +       for (i = 0; ; i++) {
> > +               port = of_parse_phandle(dev->of_node, "ports", i);
> > +               if (!port)
> > +                       break;
> > +
> > +               if (!of_device_is_available(port->parent)) {
> > +                       of_node_put(port);
> > +                       continue;
> > +               }
> > +
> > +               component_match_add(dev, &match, compare_of, port);
> > +               of_node_put(port);
> > +       }
> > +
> > +       if (i == 0) {
> > +               dev_err(dev, "missing 'ports' property\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       if (!match) {
> > +               dev_err(dev, "no available port\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       /*
> > +        * For bound crtcs, bind the encoders attached to their remote endpoint
> > +        */
> > +       for (i = 0; ; i++) {
> > +               port = of_parse_phandle(dev->of_node, "ports", i);
> > +               if (!port)
> > +                       break;
> > +
> > +               if (!of_device_is_available(port->parent)) {
> > +                       of_node_put(port);
> > +                       continue;
> > +               }
> > +
> Of the three drivers converted only the rockchip one has the above
> of_device_is_available() hunk. Based on the handling in previous loop
> I'm not entirely sure if it's needed, but if so shouldn't one mention
> the difference when converting the respective drivers ?

Yes, it should've been there, otherwise we'll end up binding encoders
which are connected to a CRTC which has been disabled - and that means
the DRM driver won't come up.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v4 1/4] drm: Introduce generic probe function for component based masters.

[Russell King - ARM Linux]

On Tue, Oct 20, 2015 at 11:00:55AM +0100, Emil Velikov wrote:
> Hi Liviu,
> 
> On 20 October 2015 at 10:23, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > A lot of component based DRM drivers use a variant of the same code
> > as the probe function. They bind the crtc ports in the first iteration
> > and then scan through the child nodes and bind the encoders attached
> > to the remote endpoints. Factor the common code into a separate
> > function called drm_of_component_probe() in order to increase code
> > reuse.
> >
> > Cc: David Airlie <airlied@linux.ie>
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  drivers/gpu/drm/drm_of.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_of.h     | 13 +++++++
> >  2 files changed, 101 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > index be38840..493c05c 100644
> > --- a/drivers/gpu/drm/drm_of.c
> > +++ b/drivers/gpu/drm/drm_of.c
> > @@ -1,3 +1,4 @@
> > +#include <linux/component.h>
> >  #include <linux/export.h>
> >  #include <linux/list.h>
> >  #include <linux/of_graph.h>
> > @@ -61,3 +62,90 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> >         return possible_crtcs;
> >  }
> >  EXPORT_SYMBOL(drm_of_find_possible_crtcs);
> > +
> > +/**
> > + * drm_of_component_probe - Generic probe function for a component based master
> > + * @dev: master device containing the OF node
> > + * @compare_of: compare function used for matching components
> > + * @master_ops: component master ops to be used
> > + *
> > + * Parse the platform device OF node and bind all the components associated
> > + * with the master. Interface ports are added before the encoders in order to
> > + * satisfy their .bind requirements
> > + * See Documentation/devicetree/bindings/graph.txt for the bindings.
> > + *
> > + * 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 *),
> > +                          const struct component_master_ops *m_ops)
> > +{
> > +       struct device_node *ep, *port, *remote;
> > +       struct component_match *match = NULL;
> > +       int i;
> > +
> > +       if (!dev->of_node)
> > +               return -EINVAL;
> > +
> > +       /*
> > +        * Bind the crtc's ports first, so that drm_of_find_possible_crtcs()
> > +        * called from encoder's .bind callbacks works as expected
> > +        */
> > +       for (i = 0; ; i++) {
> > +               port = of_parse_phandle(dev->of_node, "ports", i);
> > +               if (!port)
> > +                       break;
> > +
> > +               if (!of_device_is_available(port->parent)) {
> > +                       of_node_put(port);
> > +                       continue;
> > +               }
> > +
> > +               component_match_add(dev, &match, compare_of, port);
> > +               of_node_put(port);
> > +       }
> > +
> > +       if (i == 0) {
> > +               dev_err(dev, "missing 'ports' property\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       if (!match) {
> > +               dev_err(dev, "no available port\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       /*
> > +        * For bound crtcs, bind the encoders attached to their remote endpoint
> > +        */
> > +       for (i = 0; ; i++) {
> > +               port = of_parse_phandle(dev->of_node, "ports", i);
> > +               if (!port)
> > +                       break;
> > +
> > +               if (!of_device_is_available(port->parent)) {
> > +                       of_node_put(port);
> > +                       continue;
> > +               }
> > +
> Of the three drivers converted only the rockchip one has the above
> of_device_is_available() hunk. Based on the handling in previous loop
> I'm not entirely sure if it's needed, but if so shouldn't one mention
> the difference when converting the respective drivers ?

Yes, it should've been there, otherwise we'll end up binding encoders
which are connected to a CRTC which has been disabled - and that means
the DRM driver won't come up.

-- 
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 v4 1/4] drm: Introduce generic probe function for component based masters.

[Russell King - ARM Linux]

On Tue, Oct 20, 2015 at 11:00:55AM +0100, Emil Velikov wrote:
> Hi Liviu,
> 
> On 20 October 2015 at 10:23, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > A lot of component based DRM drivers use a variant of the same code
> > as the probe function. They bind the crtc ports in the first iteration
> > and then scan through the child nodes and bind the encoders attached
> > to the remote endpoints. Factor the common code into a separate
> > function called drm_of_component_probe() in order to increase code
> > reuse.
> >
> > Cc: David Airlie <airlied@linux.ie>
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  drivers/gpu/drm/drm_of.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_of.h     | 13 +++++++
> >  2 files changed, 101 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > index be38840..493c05c 100644
> > --- a/drivers/gpu/drm/drm_of.c
> > +++ b/drivers/gpu/drm/drm_of.c
> > @@ -1,3 +1,4 @@
> > +#include <linux/component.h>
> >  #include <linux/export.h>
> >  #include <linux/list.h>
> >  #include <linux/of_graph.h>
> > @@ -61,3 +62,90 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> >         return possible_crtcs;
> >  }
> >  EXPORT_SYMBOL(drm_of_find_possible_crtcs);
> > +
> > +/**
> > + * drm_of_component_probe - Generic probe function for a component based master
> > + * @dev: master device containing the OF node
> > + * @compare_of: compare function used for matching components
> > + * @master_ops: component master ops to be used
> > + *
> > + * Parse the platform device OF node and bind all the components associated
> > + * with the master. Interface ports are added before the encoders in order to
> > + * satisfy their .bind requirements
> > + * See Documentation/devicetree/bindings/graph.txt for the bindings.
> > + *
> > + * 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 *),
> > +                          const struct component_master_ops *m_ops)
> > +{
> > +       struct device_node *ep, *port, *remote;
> > +       struct component_match *match = NULL;
> > +       int i;
> > +
> > +       if (!dev->of_node)
> > +               return -EINVAL;
> > +
> > +       /*
> > +        * Bind the crtc's ports first, so that drm_of_find_possible_crtcs()
> > +        * called from encoder's .bind callbacks works as expected
> > +        */
> > +       for (i = 0; ; i++) {
> > +               port = of_parse_phandle(dev->of_node, "ports", i);
> > +               if (!port)
> > +                       break;
> > +
> > +               if (!of_device_is_available(port->parent)) {
> > +                       of_node_put(port);
> > +                       continue;
> > +               }
> > +
> > +               component_match_add(dev, &match, compare_of, port);
> > +               of_node_put(port);
> > +       }
> > +
> > +       if (i == 0) {
> > +               dev_err(dev, "missing 'ports' property\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       if (!match) {
> > +               dev_err(dev, "no available port\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       /*
> > +        * For bound crtcs, bind the encoders attached to their remote endpoint
> > +        */
> > +       for (i = 0; ; i++) {
> > +               port = of_parse_phandle(dev->of_node, "ports", i);
> > +               if (!port)
> > +                       break;
> > +
> > +               if (!of_device_is_available(port->parent)) {
> > +                       of_node_put(port);
> > +                       continue;
> > +               }
> > +
> Of the three drivers converted only the rockchip one has the above
> of_device_is_available() hunk. Based on the handling in previous loop
> I'm not entirely sure if it's needed, but if so shouldn't one mention
> the difference when converting the respective drivers ?

Yes, it should've been there, otherwise we'll end up binding encoders
which are connected to a CRTC which has been disabled - and that means
the DRM driver won't come up.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v4 1/4] drm: Introduce generic probe function for component based masters.

[Liviu Dudau]

On Tue, Oct 20, 2015 at 11:09:09AM +0100, Russell King - ARM Linux wrote:
> On Tue, Oct 20, 2015 at 11:00:55AM +0100, Emil Velikov wrote:
> > Hi Liviu,
> > 
> > On 20 October 2015 at 10:23, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > > A lot of component based DRM drivers use a variant of the same code
> > > as the probe function. They bind the crtc ports in the first iteration
> > > and then scan through the child nodes and bind the encoders attached
> > > to the remote endpoints. Factor the common code into a separate
> > > function called drm_of_component_probe() in order to increase code
> > > reuse.
> > >
> > > Cc: David Airlie <airlied@linux.ie>
> > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > > ---
> > >  drivers/gpu/drm/drm_of.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_of.h     | 13 +++++++
> > >  2 files changed, 101 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > > index be38840..493c05c 100644
> > > --- a/drivers/gpu/drm/drm_of.c
> > > +++ b/drivers/gpu/drm/drm_of.c
> > > @@ -1,3 +1,4 @@
> > > +#include <linux/component.h>
> > >  #include <linux/export.h>
> > >  #include <linux/list.h>
> > >  #include <linux/of_graph.h>
> > > @@ -61,3 +62,90 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> > >         return possible_crtcs;
> > >  }
> > >  EXPORT_SYMBOL(drm_of_find_possible_crtcs);
> > > +
> > > +/**
> > > + * drm_of_component_probe - Generic probe function for a component based master
> > > + * @dev: master device containing the OF node
> > > + * @compare_of: compare function used for matching components
> > > + * @master_ops: component master ops to be used
> > > + *
> > > + * Parse the platform device OF node and bind all the components associated
> > > + * with the master. Interface ports are added before the encoders in order to
> > > + * satisfy their .bind requirements
> > > + * See Documentation/devicetree/bindings/graph.txt for the bindings.
> > > + *
> > > + * 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 *),
> > > +                          const struct component_master_ops *m_ops)
> > > +{
> > > +       struct device_node *ep, *port, *remote;
> > > +       struct component_match *match = NULL;
> > > +       int i;
> > > +
> > > +       if (!dev->of_node)
> > > +               return -EINVAL;
> > > +
> > > +       /*
> > > +        * Bind the crtc's ports first, so that drm_of_find_possible_crtcs()
> > > +        * called from encoder's .bind callbacks works as expected
> > > +        */
> > > +       for (i = 0; ; i++) {
> > > +               port = of_parse_phandle(dev->of_node, "ports", i);
> > > +               if (!port)
> > > +                       break;
> > > +
> > > +               if (!of_device_is_available(port->parent)) {
> > > +                       of_node_put(port);
> > > +                       continue;
> > > +               }
> > > +
> > > +               component_match_add(dev, &match, compare_of, port);
> > > +               of_node_put(port);
> > > +       }
> > > +
> > > +       if (i == 0) {
> > > +               dev_err(dev, "missing 'ports' property\n");
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       if (!match) {
> > > +               dev_err(dev, "no available port\n");
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       /*
> > > +        * For bound crtcs, bind the encoders attached to their remote endpoint
> > > +        */
> > > +       for (i = 0; ; i++) {
> > > +               port = of_parse_phandle(dev->of_node, "ports", i);
> > > +               if (!port)
> > > +                       break;
> > > +
> > > +               if (!of_device_is_available(port->parent)) {
> > > +                       of_node_put(port);
> > > +                       continue;
> > > +               }
> > > +
> > Of the three drivers converted only the rockchip one has the above
> > of_device_is_available() hunk. Based on the handling in previous loop
> > I'm not entirely sure if it's needed, but if so shouldn't one mention
> > the difference when converting the respective drivers ?
> 
> Yes, it should've been there, otherwise we'll end up binding encoders
> which are connected to a CRTC which has been disabled - and that means
> the DRM driver won't come up.

That's my understanding as well. As mentioned in the cover letter, the generic
function tries to gather together the best practice and be more complete than
the individual versions found in the drivers. So you get additional checks
that should've been in there in the first place if it were not for the code
being spread out.

Best regards,
Liviu

> 
> -- 
> 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 v4 1/4] drm: Introduce generic probe function for component based masters.

[Liviu Dudau]

On Tue, Oct 20, 2015 at 11:09:09AM +0100, Russell King - ARM Linux wrote:
> On Tue, Oct 20, 2015 at 11:00:55AM +0100, Emil Velikov wrote:
> > Hi Liviu,
> > 
> > On 20 October 2015 at 10:23, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > > A lot of component based DRM drivers use a variant of the same code
> > > as the probe function. They bind the crtc ports in the first iteration
> > > and then scan through the child nodes and bind the encoders attached
> > > to the remote endpoints. Factor the common code into a separate
> > > function called drm_of_component_probe() in order to increase code
> > > reuse.
> > >
> > > Cc: David Airlie <airlied@linux.ie>
> > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > > ---
> > >  drivers/gpu/drm/drm_of.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_of.h     | 13 +++++++
> > >  2 files changed, 101 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > > index be38840..493c05c 100644
> > > --- a/drivers/gpu/drm/drm_of.c
> > > +++ b/drivers/gpu/drm/drm_of.c
> > > @@ -1,3 +1,4 @@
> > > +#include <linux/component.h>
> > >  #include <linux/export.h>
> > >  #include <linux/list.h>
> > >  #include <linux/of_graph.h>
> > > @@ -61,3 +62,90 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> > >         return possible_crtcs;
> > >  }
> > >  EXPORT_SYMBOL(drm_of_find_possible_crtcs);
> > > +
> > > +/**
> > > + * drm_of_component_probe - Generic probe function for a component based master
> > > + * @dev: master device containing the OF node
> > > + * @compare_of: compare function used for matching components
> > > + * @master_ops: component master ops to be used
> > > + *
> > > + * Parse the platform device OF node and bind all the components associated
> > > + * with the master. Interface ports are added before the encoders in order to
> > > + * satisfy their .bind requirements
> > > + * See Documentation/devicetree/bindings/graph.txt for the bindings.
> > > + *
> > > + * 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 *),
> > > +                          const struct component_master_ops *m_ops)
> > > +{
> > > +       struct device_node *ep, *port, *remote;
> > > +       struct component_match *match = NULL;
> > > +       int i;
> > > +
> > > +       if (!dev->of_node)
> > > +               return -EINVAL;
> > > +
> > > +       /*
> > > +        * Bind the crtc's ports first, so that drm_of_find_possible_crtcs()
> > > +        * called from encoder's .bind callbacks works as expected
> > > +        */
> > > +       for (i = 0; ; i++) {
> > > +               port = of_parse_phandle(dev->of_node, "ports", i);
> > > +               if (!port)
> > > +                       break;
> > > +
> > > +               if (!of_device_is_available(port->parent)) {
> > > +                       of_node_put(port);
> > > +                       continue;
> > > +               }
> > > +
> > > +               component_match_add(dev, &match, compare_of, port);
> > > +               of_node_put(port);
> > > +       }
> > > +
> > > +       if (i == 0) {
> > > +               dev_err(dev, "missing 'ports' property\n");
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       if (!match) {
> > > +               dev_err(dev, "no available port\n");
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       /*
> > > +        * For bound crtcs, bind the encoders attached to their remote endpoint
> > > +        */
> > > +       for (i = 0; ; i++) {
> > > +               port = of_parse_phandle(dev->of_node, "ports", i);
> > > +               if (!port)
> > > +                       break;
> > > +
> > > +               if (!of_device_is_available(port->parent)) {
> > > +                       of_node_put(port);
> > > +                       continue;
> > > +               }
> > > +
> > Of the three drivers converted only the rockchip one has the above
> > of_device_is_available() hunk. Based on the handling in previous loop
> > I'm not entirely sure if it's needed, but if so shouldn't one mention
> > the difference when converting the respective drivers ?
> 
> Yes, it should've been there, otherwise we'll end up binding encoders
> which are connected to a CRTC which has been disabled - and that means
> the DRM driver won't come up.

That's my understanding as well. As mentioned in the cover letter, the generic
function tries to gather together the best practice and be more complete than
the individual versions found in the drivers. So you get additional checks
that should've been in there in the first place if it were not for the code
being spread out.

Best regards,
Liviu

> 
> -- 
> 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
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 1/4] drm: Introduce generic probe function for component based masters.

[Liviu Dudau]

On Tue, Oct 20, 2015 at 11:09:09AM +0100, Russell King - ARM Linux wrote:
> On Tue, Oct 20, 2015 at 11:00:55AM +0100, Emil Velikov wrote:
> > Hi Liviu,
> > 
> > On 20 October 2015 at 10:23, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > > A lot of component based DRM drivers use a variant of the same code
> > > as the probe function. They bind the crtc ports in the first iteration
> > > and then scan through the child nodes and bind the encoders attached
> > > to the remote endpoints. Factor the common code into a separate
> > > function called drm_of_component_probe() in order to increase code
> > > reuse.
> > >
> > > Cc: David Airlie <airlied@linux.ie>
> > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > > ---
> > >  drivers/gpu/drm/drm_of.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_of.h     | 13 +++++++
> > >  2 files changed, 101 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > > index be38840..493c05c 100644
> > > --- a/drivers/gpu/drm/drm_of.c
> > > +++ b/drivers/gpu/drm/drm_of.c
> > > @@ -1,3 +1,4 @@
> > > +#include <linux/component.h>
> > >  #include <linux/export.h>
> > >  #include <linux/list.h>
> > >  #include <linux/of_graph.h>
> > > @@ -61,3 +62,90 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> > >         return possible_crtcs;
> > >  }
> > >  EXPORT_SYMBOL(drm_of_find_possible_crtcs);
> > > +
> > > +/**
> > > + * drm_of_component_probe - Generic probe function for a component based master
> > > + * @dev: master device containing the OF node
> > > + * @compare_of: compare function used for matching components
> > > + * @master_ops: component master ops to be used
> > > + *
> > > + * Parse the platform device OF node and bind all the components associated
> > > + * with the master. Interface ports are added before the encoders in order to
> > > + * satisfy their .bind requirements
> > > + * See Documentation/devicetree/bindings/graph.txt for the bindings.
> > > + *
> > > + * 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 *),
> > > +                          const struct component_master_ops *m_ops)
> > > +{
> > > +       struct device_node *ep, *port, *remote;
> > > +       struct component_match *match = NULL;
> > > +       int i;
> > > +
> > > +       if (!dev->of_node)
> > > +               return -EINVAL;
> > > +
> > > +       /*
> > > +        * Bind the crtc's ports first, so that drm_of_find_possible_crtcs()
> > > +        * called from encoder's .bind callbacks works as expected
> > > +        */
> > > +       for (i = 0; ; i++) {
> > > +               port = of_parse_phandle(dev->of_node, "ports", i);
> > > +               if (!port)
> > > +                       break;
> > > +
> > > +               if (!of_device_is_available(port->parent)) {
> > > +                       of_node_put(port);
> > > +                       continue;
> > > +               }
> > > +
> > > +               component_match_add(dev, &match, compare_of, port);
> > > +               of_node_put(port);
> > > +       }
> > > +
> > > +       if (i == 0) {
> > > +               dev_err(dev, "missing 'ports' property\n");
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       if (!match) {
> > > +               dev_err(dev, "no available port\n");
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       /*
> > > +        * For bound crtcs, bind the encoders attached to their remote endpoint
> > > +        */
> > > +       for (i = 0; ; i++) {
> > > +               port = of_parse_phandle(dev->of_node, "ports", i);
> > > +               if (!port)
> > > +                       break;
> > > +
> > > +               if (!of_device_is_available(port->parent)) {
> > > +                       of_node_put(port);
> > > +                       continue;
> > > +               }
> > > +
> > Of the three drivers converted only the rockchip one has the above
> > of_device_is_available() hunk. Based on the handling in previous loop
> > I'm not entirely sure if it's needed, but if so shouldn't one mention
> > the difference when converting the respective drivers ?
> 
> Yes, it should've been there, otherwise we'll end up binding encoders
> which are connected to a CRTC which has been disabled - and that means
> the DRM driver won't come up.

That's my understanding as well. As mentioned in the cover letter, the generic
function tries to gather together the best practice and be more complete than
the individual versions found in the drivers. So you get additional checks
that should've been in there in the first place if it were not for the code
being spread out.

Best regards,
Liviu

> 
> -- 
> 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 v4 0/4] drm: Cleanup probe function for component based masters.

[Liviu Dudau]

On Tue, Oct 20, 2015 at 12:02:33PM +0200, Daniel Vetter wrote:
> On Tue, Oct 20, 2015 at 10:23:11AM +0100, Liviu Dudau wrote:
> > Changelog:
> > v4: Fixed a bug where the wrong pointer was sent to component_match_add() and
> >     component_master_add_with_match() in the armada_drv.c file that was flagged
> >     by kbuild test robot. Dropped the RFC tag and added Acked-bys received from
> >     Russell King.
> > v3: Removed the call to dma_set_coherent_mask() from the generic
> >     drm_of_component_probe(). Also changes to shorten lines over 80 chars long.
> > v2: Rebased the patchset on top of drm-next rather than Linus' latest -rc
> > 
> > A few drivers in drivers/gpu/drm are component-enabled and use quite similar
> > code sequences to probe for their encoder slaves at the remote end of the ports.
> > Move the code into a "generic" function and remove it from the drivers.
> > 
> > The end results is that drivers get a reference count fix (imx), more thorough
> > error checking (imx again) plus a decrease in the overall count of LoC.
> > 
> > I'm looking for comments and testing of the patchset (only compile tested from my
> > end as I don't have access to all the devices touched by the changes). My main
> > interest is in finding out if -EINVAL is the correct code to return if
> > dev->of_node == NULL (handy now, as it is different from the other possible error
> > codes and used in armada to trigger old platform_data support. Also looking for
> > thoughts on the correctness of the patch and if it possible to co-opt more drivers
> > into using the function.
> 
> Merged all four to drm-misc, thanks.
> -Daniel

Thanks!

Liviu

> 
> > 
> > Best regards,
> > Liviu
> > 
> > Liviu Dudau (4):
> >   drm: Introduce generic probe function for component based masters.
> >   drm/imx: Convert the probe function to the generic drm_of_component_probe()
> >   drm/rockchip: Convert the probe function to the generic drm_of_component_probe()
> >   drm/armada: Convert the probe function to the generic drm_of_component_probe()
> > 
> >  drivers/gpu/drm/armada/armada_drv.c         | 68 +++++++---------------
> >  drivers/gpu/drm/drm_of.c                    | 88 +++++++++++++++++++++++++++++
> >  drivers/gpu/drm/imx/imx-drm-core.c          | 55 ++----------------
> >  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 81 ++------------------------
> >  include/drm/drm_of.h                        | 13 +++++
> >  5 files changed, 130 insertions(+), 175 deletions(-)
> > 
> > -- 
> > 2.6.0
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> 

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

Re: [PATCH v4 0/4] drm: Cleanup probe function for component based masters.

[Liviu Dudau]

On Tue, Oct 20, 2015 at 12:02:33PM +0200, Daniel Vetter wrote:
> On Tue, Oct 20, 2015 at 10:23:11AM +0100, Liviu Dudau wrote:
> > Changelog:
> > v4: Fixed a bug where the wrong pointer was sent to component_match_add() and
> >     component_master_add_with_match() in the armada_drv.c file that was flagged
> >     by kbuild test robot. Dropped the RFC tag and added Acked-bys received from
> >     Russell King.
> > v3: Removed the call to dma_set_coherent_mask() from the generic
> >     drm_of_component_probe(). Also changes to shorten lines over 80 chars long.
> > v2: Rebased the patchset on top of drm-next rather than Linus' latest -rc
> > 
> > A few drivers in drivers/gpu/drm are component-enabled and use quite similar
> > code sequences to probe for their encoder slaves at the remote end of the ports.
> > Move the code into a "generic" function and remove it from the drivers.
> > 
> > The end results is that drivers get a reference count fix (imx), more thorough
> > error checking (imx again) plus a decrease in the overall count of LoC.
> > 
> > I'm looking for comments and testing of the patchset (only compile tested from my
> > end as I don't have access to all the devices touched by the changes). My main
> > interest is in finding out if -EINVAL is the correct code to return if
> > dev->of_node == NULL (handy now, as it is different from the other possible error
> > codes and used in armada to trigger old platform_data support. Also looking for
> > thoughts on the correctness of the patch and if it possible to co-opt more drivers
> > into using the function.
> 
> Merged all four to drm-misc, thanks.
> -Daniel

Thanks!

Liviu

> 
> > 
> > Best regards,
> > Liviu
> > 
> > Liviu Dudau (4):
> >   drm: Introduce generic probe function for component based masters.
> >   drm/imx: Convert the probe function to the generic drm_of_component_probe()
> >   drm/rockchip: Convert the probe function to the generic drm_of_component_probe()
> >   drm/armada: Convert the probe function to the generic drm_of_component_probe()
> > 
> >  drivers/gpu/drm/armada/armada_drv.c         | 68 +++++++---------------
> >  drivers/gpu/drm/drm_of.c                    | 88 +++++++++++++++++++++++++++++
> >  drivers/gpu/drm/imx/imx-drm-core.c          | 55 ++----------------
> >  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 81 ++------------------------
> >  include/drm/drm_of.h                        | 13 +++++
> >  5 files changed, 130 insertions(+), 175 deletions(-)
> > 
> > -- 
> > 2.6.0
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> 

-- 
====================
| 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 v4 0/4] drm: Cleanup probe function for component based masters.

[Liviu Dudau]

On Tue, Oct 20, 2015 at 12:02:33PM +0200, Daniel Vetter wrote:
> On Tue, Oct 20, 2015 at 10:23:11AM +0100, Liviu Dudau wrote:
> > Changelog:
> > v4: Fixed a bug where the wrong pointer was sent to component_match_add() and
> >     component_master_add_with_match() in the armada_drv.c file that was flagged
> >     by kbuild test robot. Dropped the RFC tag and added Acked-bys received from
> >     Russell King.
> > v3: Removed the call to dma_set_coherent_mask() from the generic
> >     drm_of_component_probe(). Also changes to shorten lines over 80 chars long.
> > v2: Rebased the patchset on top of drm-next rather than Linus' latest -rc
> > 
> > A few drivers in drivers/gpu/drm are component-enabled and use quite similar
> > code sequences to probe for their encoder slaves at the remote end of the ports.
> > Move the code into a "generic" function and remove it from the drivers.
> > 
> > The end results is that drivers get a reference count fix (imx), more thorough
> > error checking (imx again) plus a decrease in the overall count of LoC.
> > 
> > I'm looking for comments and testing of the patchset (only compile tested from my
> > end as I don't have access to all the devices touched by the changes). My main
> > interest is in finding out if -EINVAL is the correct code to return if
> > dev->of_node == NULL (handy now, as it is different from the other possible error
> > codes and used in armada to trigger old platform_data support. Also looking for
> > thoughts on the correctness of the patch and if it possible to co-opt more drivers
> > into using the function.
> 
> Merged all four to drm-misc, thanks.
> -Daniel

Thanks!

Liviu

> 
> > 
> > Best regards,
> > Liviu
> > 
> > Liviu Dudau (4):
> >   drm: Introduce generic probe function for component based masters.
> >   drm/imx: Convert the probe function to the generic drm_of_component_probe()
> >   drm/rockchip: Convert the probe function to the generic drm_of_component_probe()
> >   drm/armada: Convert the probe function to the generic drm_of_component_probe()
> > 
> >  drivers/gpu/drm/armada/armada_drv.c         | 68 +++++++---------------
> >  drivers/gpu/drm/drm_of.c                    | 88 +++++++++++++++++++++++++++++
> >  drivers/gpu/drm/imx/imx-drm-core.c          | 55 ++----------------
> >  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 81 ++------------------------
> >  include/drm/drm_of.h                        | 13 +++++
> >  5 files changed, 130 insertions(+), 175 deletions(-)
> > 
> > -- 
> > 2.6.0
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> 

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