Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1251508 > unrolled thread
| Started by | Liviu Dudau <Liviu.Dudau@arm.com> |
|---|---|
| First post | 2015-10-20 11:30 +0200 |
| Last post | 2015-10-20 12:20 +0200 |
| Articles | 9 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH v4 0/4] drm: Cleanup probe function for component based masters. Liviu Dudau <Liviu.Dudau@arm.com> - 2015-10-20 11:30 +0200
[PATCH v4 2/4] drm/imx: Convert the probe function to the generic drm_of_component_probe() Liviu Dudau <Liviu.Dudau@arm.com> - 2015-10-20 11:30 +0200
[PATCH v4 3/4] drm/rockchip: Convert the probe function to the generic drm_of_component_probe() Liviu Dudau <Liviu.Dudau@arm.com> - 2015-10-20 11:30 +0200
[PATCH v4 1/4] drm: Introduce generic probe function for component based masters. Liviu Dudau <Liviu.Dudau@arm.com> - 2015-10-20 11:30 +0200
Re: [PATCH v4 1/4] drm: Introduce generic probe function for component based masters. Emil Velikov <emil.l.velikov@gmail.com> - 2015-10-20 12:10 +0200
Re: [PATCH v4 1/4] drm: Introduce generic probe function for component based masters. Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-10-20 12:10 +0200
Re: [PATCH v4 1/4] drm: Introduce generic probe function for component based masters. Liviu Dudau <Liviu.Dudau@arm.com> - 2015-10-20 12:20 +0200
Re: [PATCH v4 0/4] drm: Cleanup probe function for component based masters. Daniel Vetter <daniel@ffwll.ch> - 2015-10-20 12:10 +0200
Re: [PATCH v4 0/4] drm: Cleanup probe function for component based masters. Liviu Dudau <Liviu.Dudau@arm.com> - 2015-10-20 12:20 +0200
| From | Liviu Dudau <Liviu.Dudau@arm.com> |
|---|---|
| Date | 2015-10-20 11:30 +0200 |
| Subject | [PATCH v4 0/4] drm: Cleanup probe function for component based masters. |
| Message-ID | <qlBp0-7Wo-11@gated-at.bofh.it> |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Liviu Dudau <Liviu.Dudau@arm.com> |
|---|---|
| Date | 2015-10-20 11:30 +0200 |
| Subject | [PATCH v4 2/4] drm/imx: Convert the probe function to the generic drm_of_component_probe() |
| Message-ID | <qlBp1-7Wo-31@gated-at.bofh.it> |
| In reply to | #1251508 |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Liviu Dudau <Liviu.Dudau@arm.com> |
|---|---|
| Date | 2015-10-20 11:30 +0200 |
| Subject | [PATCH v4 3/4] drm/rockchip: Convert the probe function to the generic drm_of_component_probe() |
| Message-ID | <qlBp2-7Wo-35@gated-at.bofh.it> |
| In reply to | #1251508 |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Liviu Dudau <Liviu.Dudau@arm.com> |
|---|---|
| Date | 2015-10-20 11:30 +0200 |
| Subject | [PATCH v4 1/4] drm: Introduce generic probe function for component based masters. |
| Message-ID | <qlBp2-7Wo-37@gated-at.bofh.it> |
| In reply to | #1251508 |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Emil Velikov <emil.l.velikov@gmail.com> |
|---|---|
| Date | 2015-10-20 12:10 +0200 |
| Subject | Re: [PATCH v4 1/4] drm: Introduce generic probe function for component based masters. |
| Message-ID | <qlC1I-uk-15@gated-at.bofh.it> |
| In reply to | #1251514 |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Russell King - ARM Linux <linux@arm.linux.org.uk> |
|---|---|
| Date | 2015-10-20 12:10 +0200 |
| Subject | Re: [PATCH v4 1/4] drm: Introduce generic probe function for component based masters. |
| Message-ID | <qlC1J-uk-33@gated-at.bofh.it> |
| In reply to | #1251557 |
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Liviu Dudau <Liviu.Dudau@arm.com> |
|---|---|
| Date | 2015-10-20 12:20 +0200 |
| Subject | Re: [PATCH v4 1/4] drm: Introduce generic probe function for component based masters. |
| Message-ID | <qlCbo-G0-13@gated-at.bofh.it> |
| In reply to | #1251562 |
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! /
---------------
¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2015-10-20 12:10 +0200 |
| Subject | Re: [PATCH v4 0/4] drm: Cleanup probe function for component based masters. |
| Message-ID | <qlC1J-uk-29@gated-at.bofh.it> |
| In reply to | #1251508 |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Liviu Dudau <Liviu.Dudau@arm.com> |
|---|---|
| Date | 2015-10-20 12:20 +0200 |
| Subject | Re: [PATCH v4 0/4] drm: Cleanup probe function for component based masters. |
| Message-ID | <qlCbo-G0-5@gated-at.bofh.it> |
| In reply to | #1251561 |
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! /
---------------
¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web