Am Mittwoch, den 05.03.2014, 10:05 +0000 schrieb Russell King - ARM
Linux:
> On Wed, Mar 05, 2014 at 10:20:52AM +0100, Philipp Zabel wrote:
> > +struct imx_drm_component {
> > + struct device_node *of_node;
> > + struct list_head list;
> > +};
> > +
>
> The only thing this structure appears to be doing is ensuring that a
> single component doesn't get added twice - is that correct?
I also think of it as an optimization. Now we scan the whole device
graph once in the probe function into a list of needed components that
can be walked quickly every time master_ops' .add_components callback is
run, instead of having to walk the device tree graph over and over.
Functionally, it only protects against duplicate addition.
> If so, (and the troublesome problem with the IPU crtcs is now gone)
> we can modify the core component code such that it does this:
>
> if (c->master && c->master != master)
> continue;
>
> if (compare(c->dev, compare_data)) {
> if (!c->master)
> component_attach_master(master, c);
> ret = 0;
> break;
> }
>
> which will mean that you don't need to build this list anymore to track
> what will be added - though I'd like to think a little more about that
> before making that change. Please confirm whether this will eliminate
> your list generation.
Yes, I can confirm that with this change, I can remove the list, like
this (tested on i.MX6S with a single LVDS panel):
diff --git a/drivers/staging/imx-drm/imx-drm-core.c
b/drivers/staging/imx-drm/imx-drm-core.c
index 014e546..f6135b9 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -32,11 +32,6 @@
struct imx_drm_crtc;
-struct imx_drm_component {
- struct device_node *of_node;
- struct list_head list;
-};
-
struct imx_drm_device {
struct drm_device *drm;
struct imx_drm_crtc *crtc[MAX_CRTC];
@@ -587,72 +582,10 @@ static int compare_of(struct device *dev, void
*data)
return dev->of_node == np;
}
-static LIST_HEAD(imx_drm_components);
-
static int imx_drm_add_components(struct device *master, struct master
*m)
{
- struct imx_drm_component *component;
- int ret;
-
- list_for_each_entry(component, &imx_drm_components, list) {
- ret = component_master_add_child(m, compare_of,
- component->of_node);
- if (ret)
- return ret;
- }
- return 0;
-}
-
-static int imx_drm_bind(struct device *dev)
-{
- return drm_platform_init(&imx_drm_driver, to_platform_device(dev));
-}
-
-static void imx_drm_unbind(struct device *dev)
-{
- drm_put_dev(dev_get_drvdata(dev));
-}
-
-static const struct component_master_ops imx_drm_ops = {
- .add_components = imx_drm_add_components,
- .bind = imx_drm_bind,
- .unbind = imx_drm_unbind,
-};
-
-static struct imx_drm_component *imx_drm_find_component(struct device
*dev,
- struct device_node *node)
-{
- struct imx_drm_component *component;
-
- list_for_each_entry(component, &imx_drm_components, list)
- if (component->of_node == node)
- return component;
-
- return NULL;
-}
-
-static int imx_drm_add_component(struct device *dev, struct device_node
*node)
-{
- struct imx_drm_component *component;
-
- if (imx_drm_find_component(dev, node))
- return 0;
-
- component = devm_kzalloc(dev, sizeof(*component), GFP_KERNEL);
- if (!component)
- return -ENOMEM;
-
- component->of_node = node;
- list_add_tail(&component->list, &imx_drm_components);
-
- return 0;
-}
-
-static int imx_drm_platform_probe(struct platform_device *pdev)
-{
struct device_node *ep, *port, *remote;
- int ret;
- int i;
+ int i, ret;
/*
* Bind the IPU display interface ports first, so that
@@ -660,23 +593,18 @@ static int imx_drm_platform_probe(struct
platform_device *pdev)
* works as expected.
*/
for (i = 0; ; i++) {
- port = of_parse_phandle(pdev->dev.of_node, "ports", i);
+ port = of_parse_phandle(master->of_node, "ports", i);
if (!port)
break;
- ret = imx_drm_add_component(&pdev->dev, port);
- if (ret < 0)
+ ret = component_master_add_child(m, compare_of, port);
+ if (ret)
return ret;
}
- if (i == 0) {
- dev_err(&pdev->dev, "missing 'ports' property\n");
- return -ENODEV;
- }
-
/* Then bind all encoders */
for (i = 0; ; i++) {
- port = of_parse_phandle(pdev->dev.of_node, "ports", i);
+ port = of_parse_phandle(master->of_node, "ports", i);
if (!port)
break;
@@ -687,14 +615,44 @@ static int imx_drm_platform_probe(struct
platform_device *pdev)
continue;
}
- ret = imx_drm_add_component(&pdev->dev, remote);
+ ret = component_master_add_child(m, compare_of, remote);
of_node_put(remote);
- if (ret < 0)
+ if (ret)
return ret;
}
of_node_put(port);
}
+ return 0;
+}
+
+static int imx_drm_bind(struct device *dev)
+{
+ return drm_platform_init(&imx_drm_driver, to_platform_device(dev));
+}
+
+static void imx_drm_unbind(struct device *dev)
+{
+ drm_put_dev(dev_get_drvdata(dev));
+}
+
+static const struct component_master_ops imx_drm_ops = {
+ .add_components = imx_drm_add_components,
+ .bind = imx_drm_bind,
+ .unbind = imx_drm_unbind,
+};
+
+static int imx_drm_platform_probe(struct platform_device *pdev)
+{
+ struct device_node *port;
+ int ret;
+
+ port = of_parse_phandle(pdev->dev.of_node, "ports", 0);
+ if (!port) {
+ dev_err(&pdev->dev, "missing 'ports' property\n");
+ return -ENODEV;
+ }
+
ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
if (ret)
return ret;
--
1.9.0
regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
