[PATCH 0/9] tilcdc driver fixes

[PATCH 0/9] tilcdc driver fixes

[Guido Martínez]

The tilcdc driver could be compiled as a module, but was severely broken
and could not be used as such. This patchset attempts to fix the issues
preventing a proper load/unload of the module.

Issues included dangling sysfs nodes, dangling devices, memory leaks and
a double kfree.

It now seems to be working ok. We have tested this by loading and
unloading the driver repeteadly, with both panel and slave connectors
and found no flaws.

There is still one warning left on tilcdc_crtc_destroy, caused by
destroying the connector while still in an ON status. We don't know why
this happens or why it's an issue, so we did not fix it.

The first 7 patches are bug fixes which and should probably be applied
in the stable trees. The last two are clean-ups.

The series applies cleanly over -next and 3.15-rc8, and will be resent
for v3.16-rc1, once it's out.

Guido Martínez (9):
  drm/i2c: tda998x: move drm_i2c_encoder_destroy call
  drm/tilcdc: panel: fix dangling sysfs connector node
  drm/tilcdc: slave: fix dangling sysfs connector node
  drm/tilcdc: tfp410: fix dangling sysfs connector node
  drm/tilcdc: panel: fix leak when unloading the module
  drm/tilcdc: fix release order on exit
  drm/tilcdc: fix double kfree
  drm/tilcdc: remove submodule destroy calls
  drm/tilcdc: replace late_initcall with module_init

 drivers/gpu/drm/i2c/tda998x_drv.c      |  2 +-
 drivers/gpu/drm/tilcdc/Module.symvers  |  0
 drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 15 +++++--------
 drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
 drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 39 +++++++++++++++++-----------------
 drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 27 +++++++++++++----------
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35 +++++++++++++++---------------
 7 files changed, 59 insertions(+), 60 deletions(-)
 create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers

-- 
2.0.0.rc2

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

[PATCH 1/9] drm/i2c: tda998x: move drm_i2c_encoder_destroy call

[Guido Martínez]

Currently tda998x_encoder_destroy() calls cec_write() and reg_clear(),
as part of the release procedure. Such calls need to access the I2C bus
and therefore, we need to call them before drm_i2c_encoder_destroy()
which unregisters the I2C device.

This commit moves the latter so it's done afterwards.

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
Signed-off-by: Ezequiel García <ezequiel@vanguardiasur.com.ar>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 219c7e8..d06eff6 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1183,7 +1183,6 @@ static void
 tda998x_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct tda998x_priv *priv = to_tda998x_priv(encoder);
-	drm_i2c_encoder_destroy(encoder);
 
 	/* disable all IRQs and free the IRQ handler */
 	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
@@ -1193,6 +1192,7 @@ tda998x_encoder_destroy(struct drm_encoder *encoder)
 
 	if (priv->cec)
 		i2c_unregister_device(priv->cec);
+	drm_i2c_encoder_destroy(encoder);
 	kfree(priv);
 }
 
-- 
2.0.0.rc2

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/9] drm/tilcdc: panel: fix dangling sysfs connector node

[Guido Martínez]

Add a drm_sysfs_connector_remove call when we destroy the panel to make
sure the connector node in sysfs gets deleted.

This is required for proper unload and re-load of this driver as a
module. Without this, we would get a warning at re-load time like so:

   ------------[ cut here ]------------
   WARNING: CPU: 0 PID: 824 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x54/0x74()
   sysfs: cannot create duplicate filename '/class/drm/card0-LVDS-1'
   Modules linked in: [...]
   CPU: 0 PID: 824 Comm: modprobe Not tainted 3.15.0-rc4-00027-g6484f96-dirty #81
   [<c0013bb8>] (unwind_backtrace) from [<c0011824>] (show_stack+0x10/0x14)
   [<c0011824>] (show_stack) from [<c0034e8c>] (warn_slowpath_common+0x68/0x88)
   [<c0034e8c>] (warn_slowpath_common) from [<c0034edc>] (warn_slowpath_fmt+0x30/0x40)
   [<c0034edc>] (warn_slowpath_fmt) from [<c01243f4>] (sysfs_warn_dup+0x54/0x74)
   [<c01243f4>] (sysfs_warn_dup) from [<c0124708>] (sysfs_do_create_link_sd.isra.2+0xb0/0xb8)
   [<c0124708>] (sysfs_do_create_link_sd.isra.2) from [<c02ae37c>] (device_add+0x338/0x520)
   [<c02ae37c>] (device_add) from [<c02ae6e8>] (device_create_groups_vargs+0xa0/0xc4)
   [<c02ae6e8>] (device_create_groups_vargs) from [<c02ae758>] (device_create+0x24/0x2c)
   [<c02ae758>] (device_create) from [<c029b4ec>] (drm_sysfs_connector_add+0x64/0x204)
   [<c029b4ec>] (drm_sysfs_connector_add) from [<bf0b1fec>] (panel_modeset_init+0xb8/0x134 [tilcdc])
   [<bf0b1fec>] (panel_modeset_init [tilcdc]) from [<bf0b2bf0>] (tilcdc_load+0x214/0x4c0 [tilcdc])
   [<bf0b2bf0>] (tilcdc_load [tilcdc]) from [<c029955c>] (drm_dev_register+0xa4/0x104)
      [ .. snip .. ]
   ---[ end trace b2d09cd9578b0497 ]---
   [drm:drm_sysfs_connector_add] *ERROR* failed to register connector device: -17

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/tilcdc/tilcdc_panel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
index 86c6732..1943b2f 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
@@ -151,6 +151,7 @@ struct panel_connector {
 static void panel_connector_destroy(struct drm_connector *connector)
 {
 	struct panel_connector *panel_connector = to_panel_connector(connector);
+	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(panel_connector);
 }
-- 
2.0.0.rc2

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

[PATCH 3/9] drm/tilcdc: slave: fix dangling sysfs connector node

[Guido Martínez]

Add a drm_sysfs_connector_remove call when we destroy the panel to make
sure the connector node in sysfs gets deleted.

This is required for proper unload and re-load of this driver as a
module. Without this, we would get a warning at re-load time like so:

   tda998x 0-0070: found TDA19988
   ------------[ cut here ]------------
   WARNING: CPU: 0 PID: 825 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x54/0x74()
   sysfs: cannot create duplicate filename '/class/drm/card0-HDMI-A-1'
   Modules linked in: [..]
   CPU: 0 PID: 825 Comm: modprobe Not tainted 3.15.0-rc4-00027-g9dcdef4 #82
   [<c0013bb8>] (unwind_backtrace) from [<c0011824>] (show_stack+0x10/0x14)
   [<c0011824>] (show_stack) from [<c0034e8c>] (warn_slowpath_common+0x68/0x88)
   [<c0034e8c>] (warn_slowpath_common) from [<c0034edc>] (warn_slowpath_fmt+0x30/0x40)
   [<c0034edc>] (warn_slowpath_fmt) from [<c01243f4>] (sysfs_warn_dup+0x54/0x74)
   [<c01243f4>] (sysfs_warn_dup) from [<c0124708>] (sysfs_do_create_link_sd.isra.2+0xb0/0xb8)
   [<c0124708>] (sysfs_do_create_link_sd.isra.2) from [<c02ae37c>] (device_add+0x338/0x520)
   [<c02ae37c>] (device_add) from [<c02ae6e8>] (device_create_groups_vargs+0xa0/0xc4)
   [<c02ae6e8>] (device_create_groups_vargs) from [<c02ae758>] (device_create+0x24/0x2c)
   [<c02ae758>] (device_create) from [<c029b4ec>] (drm_sysfs_connector_add+0x64/0x204)
   [<c029b4ec>] (drm_sysfs_connector_add) from [<bf0b1b40>] (slave_modeset_init+0x120/0x1bc [tilcdc])
   [<bf0b1b40>] (slave_modeset_init [tilcdc]) from [<bf0b2be8>] (tilcdc_load+0x214/0x4c0 [tilcdc])
   [<bf0b2be8>] (tilcdc_load [tilcdc]) from [<c029955c>] (drm_dev_register+0xa4/0x104)
      [..snip..]
   ---[ end trace 4df8d614936ebdee ]---
   [drm:drm_sysfs_connector_add] *ERROR* failed to register connector device: -17

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/tilcdc/tilcdc_slave.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
index a37924e..28c8478 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
@@ -176,6 +176,7 @@ struct slave_connector {
 static void slave_connector_destroy(struct drm_connector *connector)
 {
 	struct slave_connector *slave_connector = to_slave_connector(connector);
+	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(slave_connector);
 }
-- 
2.0.0.rc2

[PATCH 4/9] drm/tilcdc: tfp410: fix dangling sysfs connector node

[Guido Martínez]

Add a drm_sysfs_connector_remove call when we destroy the panel to make
sure the connector node in sysfs gets deleted.

This is required for proper unload and re-load of this driver, otherwise
we will get a warning about a duplicate filename in sysfs.

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
index c38b56b..ce75ac8 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
@@ -167,6 +167,7 @@ struct tfp410_connector {
 static void tfp410_connector_destroy(struct drm_connector *connector)
 {
 	struct tfp410_connector *tfp410_connector = to_tfp410_connector(connector);
+	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(tfp410_connector);
 }
-- 
2.0.0.rc2

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

[PATCH 5/9] drm/tilcdc: panel: fix leak when unloading the module

[Guido Martínez]

The driver did not unregister the allocated framebuffer, which caused
memory leaks (and memory manager WARNs) when unloading. Also, the
framebuffer device under /dev still existed after unloading.

Add a call to drm_fbdev_cma_fini when unloading the module to prevent
both issues.

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 171a820..de34657 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -122,6 +122,7 @@ static int tilcdc_unload(struct drm_device *dev)
 	struct tilcdc_drm_private *priv = dev->dev_private;
 	struct tilcdc_module *mod, *cur;
 
+	drm_fbdev_cma_fini(priv->fbdev);
 	drm_kms_helper_poll_fini(dev);
 	drm_mode_config_cleanup(dev);
 	drm_vblank_cleanup(dev);
-- 
2.0.0.rc2

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

[PATCH 6/9] drm/tilcdc: fix release order on exit

[Guido Martínez]

Unregister resources in the correct order on tilcdc_drm_fini, which is
the reverse order they were registered during tilcdc_drm_init.

This also means unregistering the driver before releasing its resources.

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index de34657..0644429 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -629,10 +629,10 @@ static int __init tilcdc_drm_init(void)
 static void __exit tilcdc_drm_fini(void)
 {
 	DBG("fini");
-	tilcdc_tfp410_fini();
-	tilcdc_slave_fini();
-	tilcdc_panel_fini();
 	platform_driver_unregister(&tilcdc_platform_driver);
+	tilcdc_panel_fini();
+	tilcdc_slave_fini();
+	tilcdc_tfp410_fini();
 }
 
 late_initcall(tilcdc_drm_init);
-- 
2.0.0.rc2

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 7/9] drm/tilcdc: fix double kfree

[Guido Martínez]

display_timings_release calls kfree on the display_timings object passed
to it. Calling kfree after it is wrong. SLUB debug showed the following
warning:

    =============================================================================
    BUG kmalloc-64 (Tainted: G        W    ): Object already free
    -----------------------------------------------------------------------------

    Disabling lock debugging due to kernel taint
    INFO: Allocated in of_get_display_timings+0x2c/0x214 age=601 cpu=0
    pid=884
     __slab_alloc.constprop.79+0x2e0/0x33c
     kmem_cache_alloc+0xac/0xdc
     of_get_display_timings+0x2c/0x214
     panel_probe+0x7c/0x314 [tilcdc]
     platform_drv_probe+0x18/0x48
     [..snip..]
    INFO: Freed in panel_destroy+0x18/0x3c [tilcdc] age=0 cpu=0 pid=907
     __slab_free+0x34/0x330
     panel_destroy+0x18/0x3c [tilcdc]
     tilcdc_unload+0xd0/0x118 [tilcdc]
     drm_dev_unregister+0x24/0x98
     [..snip..]

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/tilcdc/tilcdc_panel.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
index 1943b2f..b085dcc 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
@@ -286,10 +286,8 @@ static void panel_destroy(struct tilcdc_module *mod)
 {
 	struct panel_module *panel_mod = to_panel_module(mod);
 
-	if (panel_mod->timings) {
+	if (panel_mod->timings)
 		display_timings_release(panel_mod->timings);
-		kfree(panel_mod->timings);
-	}
 
 	tilcdc_module_cleanup(mod);
 	kfree(panel_mod->info);
-- 
2.0.0.rc2

[PATCH 8/9] drm/tilcdc: remove submodule destroy calls

[Guido Martínez]

The TI tilcdc driver is designed with a notion of submodules. Currently,
at unload time, these submodules are iterated and destroyed.

Now that the tilcdc remove order is fixed, this can be handled perfectly
by the kernel using the device infrastructure, since each submodule
is a kernel driver itself, and they are only destroy()'ed at unload
time. Therefore we move the destroy() functionality to each submodule's
remove().

Also, remove some checks in the unloading process since the new code
guarantees the resources are allocated and need a release.

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
---
 drivers/gpu/drm/tilcdc/Module.symvers  |  0
 drivers/gpu/drm/tilcdc/tilcdc_drv.c    |  6 ------
 drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
 drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 36 +++++++++++++++++-----------------
 drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 26 +++++++++++++-----------
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 34 ++++++++++++++++----------------
 6 files changed, 50 insertions(+), 53 deletions(-)
 create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers

diff --git a/drivers/gpu/drm/tilcdc/Module.symvers b/drivers/gpu/drm/tilcdc/Module.symvers
new file mode 100644
index 0000000..e69de29
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 0644429..702315f 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -120,7 +120,6 @@ static int cpufreq_transition(struct notifier_block *nb,
 static int tilcdc_unload(struct drm_device *dev)
 {
 	struct tilcdc_drm_private *priv = dev->dev_private;
-	struct tilcdc_module *mod, *cur;
 
 	drm_fbdev_cma_fini(priv->fbdev);
 	drm_kms_helper_poll_fini(dev);
@@ -149,11 +148,6 @@ static int tilcdc_unload(struct drm_device *dev)
 
 	pm_runtime_disable(dev->dev);
 
-	list_for_each_entry_safe(mod, cur, &module_list, list) {
-		DBG("destroying module: %s", mod->name);
-		mod->funcs->destroy(mod);
-	}
-
 	kfree(priv);
 
 	return 0;
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index 0938036..7596c14 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -98,7 +98,6 @@ struct tilcdc_module;
 struct tilcdc_module_ops {
 	/* create appropriate encoders/connectors: */
 	int (*modeset_init)(struct tilcdc_module *mod, struct drm_device *dev);
-	void (*destroy)(struct tilcdc_module *mod);
 #ifdef CONFIG_DEBUG_FS
 	/* create debugfs nodes (can be NULL): */
 	int (*debugfs_init)(struct tilcdc_module *mod, struct drm_minor *minor);
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
index b085dcc..2f6efae 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
@@ -282,21 +282,8 @@ static int panel_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
 	return 0;
 }
 
-static void panel_destroy(struct tilcdc_module *mod)
-{
-	struct panel_module *panel_mod = to_panel_module(mod);
-
-	if (panel_mod->timings)
-		display_timings_release(panel_mod->timings);
-
-	tilcdc_module_cleanup(mod);
-	kfree(panel_mod->info);
-	kfree(panel_mod);
-}
-
 static const struct tilcdc_module_ops panel_module_ops = {
 		.modeset_init = panel_modeset_init,
-		.destroy = panel_destroy,
 };
 
 /*
@@ -372,6 +359,7 @@ static int panel_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	mod = &panel_mod->base;
+	pdev->dev.platform_data = mod;
 
 	tilcdc_module_init(mod, "panel", &panel_module_ops);
 
@@ -379,17 +367,16 @@ static int panel_probe(struct platform_device *pdev)
 	if (IS_ERR(pinctrl))
 		dev_warn(&pdev->dev, "pins are not configured\n");
 
-
 	panel_mod->timings = of_get_display_timings(node);
 	if (!panel_mod->timings) {
 		dev_err(&pdev->dev, "could not get panel timings\n");
-		goto fail;
+		goto fail_free;
 	}
 
 	panel_mod->info = of_get_panel_info(node);
 	if (!panel_mod->info) {
 		dev_err(&pdev->dev, "could not get panel info\n");
-		goto fail;
+		goto fail_timings;
 	}
 
 	mod->preferred_bpp = panel_mod->info->bpp;
@@ -400,13 +387,26 @@ static int panel_probe(struct platform_device *pdev)
 
 	return 0;
 
-fail:
-	panel_destroy(mod);
+fail_timings:
+	display_timings_release(panel_mod->timings);
+
+fail_free:
+	kfree(panel_mod);
+	tilcdc_module_cleanup(mod);
 	return ret;
 }
 
 static int panel_remove(struct platform_device *pdev)
 {
+	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
+	struct panel_module *panel_mod = to_panel_module(mod);
+
+	display_timings_release(panel_mod->timings);
+
+	tilcdc_module_cleanup(mod);
+	kfree(panel_mod->info);
+	kfree(panel_mod);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
index 28c8478..9e4b0b2 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
@@ -306,17 +306,8 @@ static int slave_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
 	return 0;
 }
 
-static void slave_destroy(struct tilcdc_module *mod)
-{
-	struct slave_module *slave_mod = to_slave_module(mod);
-
-	tilcdc_module_cleanup(mod);
-	kfree(slave_mod);
-}
-
 static const struct tilcdc_module_ops slave_module_ops = {
 		.modeset_init = slave_modeset_init,
-		.destroy = slave_destroy,
 };
 
 /*
@@ -366,10 +357,13 @@ static int slave_probe(struct platform_device *pdev)
 	}
 
 	slave_mod = kzalloc(sizeof(*slave_mod), GFP_KERNEL);
-	if (!slave_mod)
-		return -ENOMEM;
+	if (!slave_mod) {
+		ret = -ENOMEM;
+		goto fail_adapter;
+	}
 
 	mod = &slave_mod->base;
+	pdev->dev.platform_data = mod;
 
 	mod->preferred_bpp = slave_info.bpp;
 
@@ -384,10 +378,20 @@ static int slave_probe(struct platform_device *pdev)
 	tilcdc_slave_probedefer(false);
 
 	return 0;
+
+fail_adapter:
+	i2c_put_adapter(slavei2c);
+	return ret;
 }
 
 static int slave_remove(struct platform_device *pdev)
 {
+	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
+	struct slave_module *slave_mod = to_slave_module(mod);
+
+	tilcdc_module_cleanup(mod);
+	kfree(slave_mod);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
index ce75ac8..32a0d2d 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
@@ -296,23 +296,8 @@ static int tfp410_modeset_init(struct tilcdc_module *mod, struct drm_device *dev
 	return 0;
 }
 
-static void tfp410_destroy(struct tilcdc_module *mod)
-{
-	struct tfp410_module *tfp410_mod = to_tfp410_module(mod);
-
-	if (tfp410_mod->i2c)
-		i2c_put_adapter(tfp410_mod->i2c);
-
-	if (!IS_ERR_VALUE(tfp410_mod->gpio))
-		gpio_free(tfp410_mod->gpio);
-
-	tilcdc_module_cleanup(mod);
-	kfree(tfp410_mod);
-}
-
 static const struct tilcdc_module_ops tfp410_module_ops = {
 		.modeset_init = tfp410_modeset_init,
-		.destroy = tfp410_destroy,
 };
 
 /*
@@ -342,6 +327,7 @@ static int tfp410_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	mod = &tfp410_mod->base;
+	pdev->dev.platform_data = mod;
 
 	tilcdc_module_init(mod, "tfp410", &tfp410_module_ops);
 
@@ -365,6 +351,7 @@ static int tfp410_probe(struct platform_device *pdev)
 	tfp410_mod->i2c = of_find_i2c_adapter_by_node(i2c_node);
 	if (!tfp410_mod->i2c) {
 		dev_err(&pdev->dev, "could not get i2c\n");
+		of_node_put(i2c_node);
 		goto fail;
 	}
 
@@ -378,19 +365,32 @@ static int tfp410_probe(struct platform_device *pdev)
 		ret = gpio_request(tfp410_mod->gpio, "DVI_PDn");
 		if (ret) {
 			dev_err(&pdev->dev, "could not get DVI_PDn gpio\n");
-			goto fail;
+			goto fail_adapter;
 		}
 	}
 
 	return 0;
 
+fail_adapter:
+	i2c_put_adapter(tfp410_mod->i2c);
+
 fail:
-	tfp410_destroy(mod);
+	kfree(tfp410_mod);
+	tilcdc_module_cleanup(mod);
 	return ret;
 }
 
 static int tfp410_remove(struct platform_device *pdev)
 {
+	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
+	struct tfp410_module *tfp410_mod = to_tfp410_module(mod);
+
+	i2c_put_adapter(tfp410_mod->i2c);
+	gpio_free(tfp410_mod->gpio);
+
+	tilcdc_module_cleanup(mod);
+	kfree(tfp410_mod);
+
 	return 0;
 }
 
-- 
2.0.0.rc2

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 9/9] drm/tilcdc: replace late_initcall with module_init

[Guido Martínez]

Use module_init instead of late_initcall, as is the norm for modular
drivers.

module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b
("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall,
but it does not explain why. Tests show it's working properly with
module_init.

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 702315f..80e6697 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -629,7 +629,7 @@ static void __exit tilcdc_drm_fini(void)
 	tilcdc_tfp410_fini();
 }
 
-late_initcall(tilcdc_drm_init);
+module_init(tilcdc_drm_init);
 module_exit(tilcdc_drm_fini);
 
 MODULE_AUTHOR("Rob Clark <robdclark@gmail.com");
-- 
2.0.0.rc2

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

[PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Guido Martínez]

The tilcdc driver could be compiled as a module, but was severely broken
and could not be used as such. This patchset attempts to fix the issues
preventing a proper load/unload of the module.

Issues included dangling sysfs nodes, dangling devices, memory leaks and
a double kfree.

It now seems to be working ok. We have tested this by loading and
unloading the driver repeteadly, with both panel and slave connectors
and found no flaws.

There is still one warning left on tilcdc_crtc_destroy, caused by
destroying the connector while still in an ON status. We don't know why
this happens or why it's an issue, so we did not fix it.

The first 7 patches are bug fixes which and should probably be applied
in the stable trees. The last two are clean-ups.


Resending this since I got no replies.


Guido Martínez (9):
  drm/i2c: tda998x: move drm_i2c_encoder_destroy call
  drm/tilcdc: panel: fix dangling sysfs connector node
  drm/tilcdc: slave: fix dangling sysfs connector node
  drm/tilcdc: tfp410: fix dangling sysfs connector node
  drm/tilcdc: panel: fix leak when unloading the module
  drm/tilcdc: fix release order on exit
  drm/tilcdc: fix double kfree
  drm/tilcdc: remove submodule destroy calls
  drm/tilcdc: replace late_initcall with module_init

 drivers/gpu/drm/i2c/tda998x_drv.c      |  2 +-
 drivers/gpu/drm/tilcdc/Module.symvers  |  0
 drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 15 +++++--------
 drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
 drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 39 +++++++++++++++++-----------------
 drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 27 +++++++++++++----------
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35 +++++++++++++++---------------
 7 files changed, 59 insertions(+), 60 deletions(-)
 create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers

-- 
2.0.0

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

[PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Guido Martínez]

The tilcdc driver could be compiled as a module, but was severely broken
and could not be used as such. This patchset attempts to fix the issues
preventing a proper load/unload of the module.

Issues included dangling sysfs nodes, dangling devices, memory leaks and
a double kfree.

It now seems to be working ok. We have tested this by loading and
unloading the driver repeteadly, with both panel and slave connectors
and found no flaws.

There is still one warning left on tilcdc_crtc_destroy, caused by
destroying the connector while still in an ON status. We don't know why
this happens or why it's an issue, so we did not fix it.

The first 7 patches are bug fixes which and should probably be applied
in the stable trees. The last two are clean-ups.


Resending this since I got no replies.


Guido Mart?nez (9):
  drm/i2c: tda998x: move drm_i2c_encoder_destroy call
  drm/tilcdc: panel: fix dangling sysfs connector node
  drm/tilcdc: slave: fix dangling sysfs connector node
  drm/tilcdc: tfp410: fix dangling sysfs connector node
  drm/tilcdc: panel: fix leak when unloading the module
  drm/tilcdc: fix release order on exit
  drm/tilcdc: fix double kfree
  drm/tilcdc: remove submodule destroy calls
  drm/tilcdc: replace late_initcall with module_init

 drivers/gpu/drm/i2c/tda998x_drv.c      |  2 +-
 drivers/gpu/drm/tilcdc/Module.symvers  |  0
 drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 15 +++++--------
 drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
 drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 39 +++++++++++++++++-----------------
 drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 27 +++++++++++++----------
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35 +++++++++++++++---------------
 7 files changed, 59 insertions(+), 60 deletions(-)
 create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers

-- 
2.0.0

[PATCH/RESEND 1/9] drm/i2c: tda998x: move drm_i2c_encoder_destroy call

[Guido Martínez]

Currently tda998x_encoder_destroy() calls cec_write() and reg_clear(),
as part of the release procedure. Such calls need to access the I2C bus
and therefore, we need to call them before drm_i2c_encoder_destroy()
which unregisters the I2C device.

This commit moves the latter so it's done afterwards.

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
Signed-off-by: Ezequiel García <ezequiel@vanguardiasur.com.ar>
Cc: <stable@vger.kernel.org> #v3.9+
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 240c331..db9515f 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1183,7 +1183,6 @@ static void
 tda998x_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct tda998x_priv *priv = to_tda998x_priv(encoder);
-	drm_i2c_encoder_destroy(encoder);
 
 	/* disable all IRQs and free the IRQ handler */
 	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
@@ -1193,6 +1192,7 @@ tda998x_encoder_destroy(struct drm_encoder *encoder)
 
 	if (priv->cec)
 		i2c_unregister_device(priv->cec);
+	drm_i2c_encoder_destroy(encoder);
 	kfree(priv);
 }
 
-- 
2.0.0

[PATCH/RESEND 1/9] drm/i2c: tda998x: move drm_i2c_encoder_destroy call

[Guido Martínez]

Currently tda998x_encoder_destroy() calls cec_write() and reg_clear(),
as part of the release procedure. Such calls need to access the I2C bus
and therefore, we need to call them before drm_i2c_encoder_destroy()
which unregisters the I2C device.

This commit moves the latter so it's done afterwards.

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
Signed-off-by: Ezequiel Garc?a <ezequiel@vanguardiasur.com.ar>
Cc: <stable@vger.kernel.org> #v3.9+
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 240c331..db9515f 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1183,7 +1183,6 @@ static void
 tda998x_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct tda998x_priv *priv = to_tda998x_priv(encoder);
-	drm_i2c_encoder_destroy(encoder);
 
 	/* disable all IRQs and free the IRQ handler */
 	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
@@ -1193,6 +1192,7 @@ tda998x_encoder_destroy(struct drm_encoder *encoder)
 
 	if (priv->cec)
 		i2c_unregister_device(priv->cec);
+	drm_i2c_encoder_destroy(encoder);
 	kfree(priv);
 }
 
-- 
2.0.0

[PATCH/RESEND 2/9] drm/tilcdc: panel: fix dangling sysfs connector node

[Guido Martínez]

Add a drm_sysfs_connector_remove call when we destroy the panel to make
sure the connector node in sysfs gets deleted.

This is required for proper unload and re-load of this driver as a
module. Without this, we would get a warning at re-load time like so:

   ------------[ cut here ]------------
   WARNING: CPU: 0 PID: 824 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x54/0x74()
   sysfs: cannot create duplicate filename '/class/drm/card0-LVDS-1'
   Modules linked in: [...]
   CPU: 0 PID: 824 Comm: modprobe Not tainted 3.15.0-rc4-00027-g6484f96-dirty #81
   [<c0013bb8>] (unwind_backtrace) from [<c0011824>] (show_stack+0x10/0x14)
   [<c0011824>] (show_stack) from [<c0034e8c>] (warn_slowpath_common+0x68/0x88)
   [<c0034e8c>] (warn_slowpath_common) from [<c0034edc>] (warn_slowpath_fmt+0x30/0x40)
   [<c0034edc>] (warn_slowpath_fmt) from [<c01243f4>] (sysfs_warn_dup+0x54/0x74)
   [<c01243f4>] (sysfs_warn_dup) from [<c0124708>] (sysfs_do_create_link_sd.isra.2+0xb0/0xb8)
   [<c0124708>] (sysfs_do_create_link_sd.isra.2) from [<c02ae37c>] (device_add+0x338/0x520)
   [<c02ae37c>] (device_add) from [<c02ae6e8>] (device_create_groups_vargs+0xa0/0xc4)
   [<c02ae6e8>] (device_create_groups_vargs) from [<c02ae758>] (device_create+0x24/0x2c)
   [<c02ae758>] (device_create) from [<c029b4ec>] (drm_sysfs_connector_add+0x64/0x204)
   [<c029b4ec>] (drm_sysfs_connector_add) from [<bf0b1fec>] (panel_modeset_init+0xb8/0x134 [tilcdc])
   [<bf0b1fec>] (panel_modeset_init [tilcdc]) from [<bf0b2bf0>] (tilcdc_load+0x214/0x4c0 [tilcdc])
   [<bf0b2bf0>] (tilcdc_load [tilcdc]) from [<c029955c>] (drm_dev_register+0xa4/0x104)
      [ .. snip .. ]
   ---[ end trace b2d09cd9578b0497 ]---
   [drm:drm_sysfs_connector_add] *ERROR* failed to register connector device: -17

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
Cc: <stable@vger.kernel.org> #v3.9+
---
 drivers/gpu/drm/tilcdc/tilcdc_panel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
index 86c6732..1943b2f 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
@@ -151,6 +151,7 @@ struct panel_connector {
 static void panel_connector_destroy(struct drm_connector *connector)
 {
 	struct panel_connector *panel_connector = to_panel_connector(connector);
+	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(panel_connector);
 }
-- 
2.0.0

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

[PATCH/RESEND 2/9] drm/tilcdc: panel: fix dangling sysfs connector node

[Guido Martínez]

Add a drm_sysfs_connector_remove call when we destroy the panel to make
sure the connector node in sysfs gets deleted.

This is required for proper unload and re-load of this driver as a
module. Without this, we would get a warning at re-load time like so:

   ------------[ cut here ]------------
   WARNING: CPU: 0 PID: 824 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x54/0x74()
   sysfs: cannot create duplicate filename '/class/drm/card0-LVDS-1'
   Modules linked in: [...]
   CPU: 0 PID: 824 Comm: modprobe Not tainted 3.15.0-rc4-00027-g6484f96-dirty #81
   [<c0013bb8>] (unwind_backtrace) from [<c0011824>] (show_stack+0x10/0x14)
   [<c0011824>] (show_stack) from [<c0034e8c>] (warn_slowpath_common+0x68/0x88)
   [<c0034e8c>] (warn_slowpath_common) from [<c0034edc>] (warn_slowpath_fmt+0x30/0x40)
   [<c0034edc>] (warn_slowpath_fmt) from [<c01243f4>] (sysfs_warn_dup+0x54/0x74)
   [<c01243f4>] (sysfs_warn_dup) from [<c0124708>] (sysfs_do_create_link_sd.isra.2+0xb0/0xb8)
   [<c0124708>] (sysfs_do_create_link_sd.isra.2) from [<c02ae37c>] (device_add+0x338/0x520)
   [<c02ae37c>] (device_add) from [<c02ae6e8>] (device_create_groups_vargs+0xa0/0xc4)
   [<c02ae6e8>] (device_create_groups_vargs) from [<c02ae758>] (device_create+0x24/0x2c)
   [<c02ae758>] (device_create) from [<c029b4ec>] (drm_sysfs_connector_add+0x64/0x204)
   [<c029b4ec>] (drm_sysfs_connector_add) from [<bf0b1fec>] (panel_modeset_init+0xb8/0x134 [tilcdc])
   [<bf0b1fec>] (panel_modeset_init [tilcdc]) from [<bf0b2bf0>] (tilcdc_load+0x214/0x4c0 [tilcdc])
   [<bf0b2bf0>] (tilcdc_load [tilcdc]) from [<c029955c>] (drm_dev_register+0xa4/0x104)
      [ .. snip .. ]
   ---[ end trace b2d09cd9578b0497 ]---
   [drm:drm_sysfs_connector_add] *ERROR* failed to register connector device: -17

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
Cc: <stable@vger.kernel.org> #v3.9+
---
 drivers/gpu/drm/tilcdc/tilcdc_panel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
index 86c6732..1943b2f 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
@@ -151,6 +151,7 @@ struct panel_connector {
 static void panel_connector_destroy(struct drm_connector *connector)
 {
 	struct panel_connector *panel_connector = to_panel_connector(connector);
+	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(panel_connector);
 }
-- 
2.0.0

[PATCH/RESEND 3/9] drm/tilcdc: slave: fix dangling sysfs connector node

[Guido Martínez]

Add a drm_sysfs_connector_remove call when we destroy the panel to make
sure the connector node in sysfs gets deleted.

This is required for proper unload and re-load of this driver as a
module. Without this, we would get a warning at re-load time like so:

   tda998x 0-0070: found TDA19988
   ------------[ cut here ]------------
   WARNING: CPU: 0 PID: 825 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x54/0x74()
   sysfs: cannot create duplicate filename '/class/drm/card0-HDMI-A-1'
   Modules linked in: [..]
   CPU: 0 PID: 825 Comm: modprobe Not tainted 3.15.0-rc4-00027-g9dcdef4 #82
   [<c0013bb8>] (unwind_backtrace) from [<c0011824>] (show_stack+0x10/0x14)
   [<c0011824>] (show_stack) from [<c0034e8c>] (warn_slowpath_common+0x68/0x88)
   [<c0034e8c>] (warn_slowpath_common) from [<c0034edc>] (warn_slowpath_fmt+0x30/0x40)
   [<c0034edc>] (warn_slowpath_fmt) from [<c01243f4>] (sysfs_warn_dup+0x54/0x74)
   [<c01243f4>] (sysfs_warn_dup) from [<c0124708>] (sysfs_do_create_link_sd.isra.2+0xb0/0xb8)
   [<c0124708>] (sysfs_do_create_link_sd.isra.2) from [<c02ae37c>] (device_add+0x338/0x520)
   [<c02ae37c>] (device_add) from [<c02ae6e8>] (device_create_groups_vargs+0xa0/0xc4)
   [<c02ae6e8>] (device_create_groups_vargs) from [<c02ae758>] (device_create+0x24/0x2c)
   [<c02ae758>] (device_create) from [<c029b4ec>] (drm_sysfs_connector_add+0x64/0x204)
   [<c029b4ec>] (drm_sysfs_connector_add) from [<bf0b1b40>] (slave_modeset_init+0x120/0x1bc [tilcdc])
   [<bf0b1b40>] (slave_modeset_init [tilcdc]) from [<bf0b2be8>] (tilcdc_load+0x214/0x4c0 [tilcdc])
   [<bf0b2be8>] (tilcdc_load [tilcdc]) from [<c029955c>] (drm_dev_register+0xa4/0x104)
      [..snip..]
   ---[ end trace 4df8d614936ebdee ]---
   [drm:drm_sysfs_connector_add] *ERROR* failed to register connector device: -17

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
Cc: <stable@vger.kernel.org> #v3.9+
---
 drivers/gpu/drm/tilcdc/tilcdc_slave.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
index 595068b..2f83ffb 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
@@ -166,6 +166,7 @@ struct slave_connector {
 static void slave_connector_destroy(struct drm_connector *connector)
 {
 	struct slave_connector *slave_connector = to_slave_connector(connector);
+	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(slave_connector);
 }
-- 
2.0.0

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

[PATCH/RESEND 3/9] drm/tilcdc: slave: fix dangling sysfs connector node

[Guido Martínez]

Add a drm_sysfs_connector_remove call when we destroy the panel to make
sure the connector node in sysfs gets deleted.

This is required for proper unload and re-load of this driver as a
module. Without this, we would get a warning at re-load time like so:

   tda998x 0-0070: found TDA19988
   ------------[ cut here ]------------
   WARNING: CPU: 0 PID: 825 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x54/0x74()
   sysfs: cannot create duplicate filename '/class/drm/card0-HDMI-A-1'
   Modules linked in: [..]
   CPU: 0 PID: 825 Comm: modprobe Not tainted 3.15.0-rc4-00027-g9dcdef4 #82
   [<c0013bb8>] (unwind_backtrace) from [<c0011824>] (show_stack+0x10/0x14)
   [<c0011824>] (show_stack) from [<c0034e8c>] (warn_slowpath_common+0x68/0x88)
   [<c0034e8c>] (warn_slowpath_common) from [<c0034edc>] (warn_slowpath_fmt+0x30/0x40)
   [<c0034edc>] (warn_slowpath_fmt) from [<c01243f4>] (sysfs_warn_dup+0x54/0x74)
   [<c01243f4>] (sysfs_warn_dup) from [<c0124708>] (sysfs_do_create_link_sd.isra.2+0xb0/0xb8)
   [<c0124708>] (sysfs_do_create_link_sd.isra.2) from [<c02ae37c>] (device_add+0x338/0x520)
   [<c02ae37c>] (device_add) from [<c02ae6e8>] (device_create_groups_vargs+0xa0/0xc4)
   [<c02ae6e8>] (device_create_groups_vargs) from [<c02ae758>] (device_create+0x24/0x2c)
   [<c02ae758>] (device_create) from [<c029b4ec>] (drm_sysfs_connector_add+0x64/0x204)
   [<c029b4ec>] (drm_sysfs_connector_add) from [<bf0b1b40>] (slave_modeset_init+0x120/0x1bc [tilcdc])
   [<bf0b1b40>] (slave_modeset_init [tilcdc]) from [<bf0b2be8>] (tilcdc_load+0x214/0x4c0 [tilcdc])
   [<bf0b2be8>] (tilcdc_load [tilcdc]) from [<c029955c>] (drm_dev_register+0xa4/0x104)
      [..snip..]
   ---[ end trace 4df8d614936ebdee ]---
   [drm:drm_sysfs_connector_add] *ERROR* failed to register connector device: -17

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
Cc: <stable@vger.kernel.org> #v3.9+
---
 drivers/gpu/drm/tilcdc/tilcdc_slave.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
index 595068b..2f83ffb 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
@@ -166,6 +166,7 @@ struct slave_connector {
 static void slave_connector_destroy(struct drm_connector *connector)
 {
 	struct slave_connector *slave_connector = to_slave_connector(connector);
+	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(slave_connector);
 }
-- 
2.0.0

[PATCH/RESEND 4/9] drm/tilcdc: tfp410: fix dangling sysfs connector node

[Guido Martínez]

Add a drm_sysfs_connector_remove call when we destroy the panel to make
sure the connector node in sysfs gets deleted.

This is required for proper unload and re-load of this driver, otherwise
we will get a warning about a duplicate filename in sysfs.

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
Cc: <stable@vger.kernel.org> #v3.9+
---
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
index c38b56b..ce75ac8 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
@@ -167,6 +167,7 @@ struct tfp410_connector {
 static void tfp410_connector_destroy(struct drm_connector *connector)
 {
 	struct tfp410_connector *tfp410_connector = to_tfp410_connector(connector);
+	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(tfp410_connector);
 }
-- 
2.0.0

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

[PATCH/RESEND 4/9] drm/tilcdc: tfp410: fix dangling sysfs connector node

[Guido Martínez]

Add a drm_sysfs_connector_remove call when we destroy the panel to make
sure the connector node in sysfs gets deleted.

This is required for proper unload and re-load of this driver, otherwise
we will get a warning about a duplicate filename in sysfs.

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
Cc: <stable@vger.kernel.org> #v3.9+
---
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
index c38b56b..ce75ac8 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
@@ -167,6 +167,7 @@ struct tfp410_connector {
 static void tfp410_connector_destroy(struct drm_connector *connector)
 {
 	struct tfp410_connector *tfp410_connector = to_tfp410_connector(connector);
+	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(tfp410_connector);
 }
-- 
2.0.0

[PATCH/RESEND 5/9] drm/tilcdc: panel: fix leak when unloading the module

[Guido Martínez]

The driver did not unregister the allocated framebuffer, which caused
memory leaks (and memory manager WARNs) when unloading. Also, the
framebuffer device under /dev still existed after unloading.

Add a call to drm_fbdev_cma_fini when unloading the module to prevent
both issues.

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
Cc: <stable@vger.kernel.org> #v3.9+
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index b20b694..490aee7 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -122,6 +122,7 @@ static int tilcdc_unload(struct drm_device *dev)
 	struct tilcdc_drm_private *priv = dev->dev_private;
 	struct tilcdc_module *mod, *cur;
 
+	drm_fbdev_cma_fini(priv->fbdev);
 	drm_kms_helper_poll_fini(dev);
 	drm_mode_config_cleanup(dev);
 	drm_vblank_cleanup(dev);
-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RESEND 5/9] drm/tilcdc: panel: fix leak when unloading the module

[Guido Martínez]

The driver did not unregister the allocated framebuffer, which caused
memory leaks (and memory manager WARNs) when unloading. Also, the
framebuffer device under /dev still existed after unloading.

Add a call to drm_fbdev_cma_fini when unloading the module to prevent
both issues.

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
Cc: <stable@vger.kernel.org> #v3.9+
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index b20b694..490aee7 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -122,6 +122,7 @@ static int tilcdc_unload(struct drm_device *dev)
 	struct tilcdc_drm_private *priv = dev->dev_private;
 	struct tilcdc_module *mod, *cur;
 
+	drm_fbdev_cma_fini(priv->fbdev);
 	drm_kms_helper_poll_fini(dev);
 	drm_mode_config_cleanup(dev);
 	drm_vblank_cleanup(dev);
-- 
2.0.0

[PATCH/RESEND 6/9] drm/tilcdc: fix release order on exit

[Guido Martínez]

Unregister resources in the correct order on tilcdc_drm_fini, which is
the reverse order they were registered during tilcdc_drm_init.

This also means unregistering the driver before releasing its resources.

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
Cc: <stable@vger.kernel.org> #v3.9+
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 490aee7..006a30e 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -629,10 +629,10 @@ static int __init tilcdc_drm_init(void)
 static void __exit tilcdc_drm_fini(void)
 {
 	DBG("fini");
-	tilcdc_tfp410_fini();
-	tilcdc_slave_fini();
-	tilcdc_panel_fini();
 	platform_driver_unregister(&tilcdc_platform_driver);
+	tilcdc_panel_fini();
+	tilcdc_slave_fini();
+	tilcdc_tfp410_fini();
 }
 
 late_initcall(tilcdc_drm_init);
-- 
2.0.0

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

[PATCH/RESEND 6/9] drm/tilcdc: fix release order on exit

[Guido Martínez]

Unregister resources in the correct order on tilcdc_drm_fini, which is
the reverse order they were registered during tilcdc_drm_init.

This also means unregistering the driver before releasing its resources.

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
Cc: <stable@vger.kernel.org> #v3.9+
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 490aee7..006a30e 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -629,10 +629,10 @@ static int __init tilcdc_drm_init(void)
 static void __exit tilcdc_drm_fini(void)
 {
 	DBG("fini");
-	tilcdc_tfp410_fini();
-	tilcdc_slave_fini();
-	tilcdc_panel_fini();
 	platform_driver_unregister(&tilcdc_platform_driver);
+	tilcdc_panel_fini();
+	tilcdc_slave_fini();
+	tilcdc_tfp410_fini();
 }
 
 late_initcall(tilcdc_drm_init);
-- 
2.0.0

[PATCH/RESEND 7/9] drm/tilcdc: fix double kfree

[Guido Martínez]

display_timings_release calls kfree on the display_timings object passed
to it. Calling kfree after it is wrong. SLUB debug showed the following
warning:

    =============================================================================
    BUG kmalloc-64 (Tainted: G        W    ): Object already free
    -----------------------------------------------------------------------------

    Disabling lock debugging due to kernel taint
    INFO: Allocated in of_get_display_timings+0x2c/0x214 age=601 cpu=0
    pid=884
     __slab_alloc.constprop.79+0x2e0/0x33c
     kmem_cache_alloc+0xac/0xdc
     of_get_display_timings+0x2c/0x214
     panel_probe+0x7c/0x314 [tilcdc]
     platform_drv_probe+0x18/0x48
     [..snip..]
    INFO: Freed in panel_destroy+0x18/0x3c [tilcdc] age=0 cpu=0 pid=907
     __slab_free+0x34/0x330
     panel_destroy+0x18/0x3c [tilcdc]
     tilcdc_unload+0xd0/0x118 [tilcdc]
     drm_dev_unregister+0x24/0x98
     [..snip..]

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
Cc: <stable@vger.kernel.org> #v3.9+
---
 drivers/gpu/drm/tilcdc/tilcdc_panel.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
index 1943b2f..b085dcc 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
@@ -286,10 +286,8 @@ static void panel_destroy(struct tilcdc_module *mod)
 {
 	struct panel_module *panel_mod = to_panel_module(mod);
 
-	if (panel_mod->timings) {
+	if (panel_mod->timings)
 		display_timings_release(panel_mod->timings);
-		kfree(panel_mod->timings);
-	}
 
 	tilcdc_module_cleanup(mod);
 	kfree(panel_mod->info);
-- 
2.0.0

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

[PATCH/RESEND 7/9] drm/tilcdc: fix double kfree

[Guido Martínez]

display_timings_release calls kfree on the display_timings object passed
to it. Calling kfree after it is wrong. SLUB debug showed the following
warning:

    =============================================================================
    BUG kmalloc-64 (Tainted: G        W    ): Object already free
    -----------------------------------------------------------------------------

    Disabling lock debugging due to kernel taint
    INFO: Allocated in of_get_display_timings+0x2c/0x214 age=601 cpu=0
    pid=884
     __slab_alloc.constprop.79+0x2e0/0x33c
     kmem_cache_alloc+0xac/0xdc
     of_get_display_timings+0x2c/0x214
     panel_probe+0x7c/0x314 [tilcdc]
     platform_drv_probe+0x18/0x48
     [..snip..]
    INFO: Freed in panel_destroy+0x18/0x3c [tilcdc] age=0 cpu=0 pid=907
     __slab_free+0x34/0x330
     panel_destroy+0x18/0x3c [tilcdc]
     tilcdc_unload+0xd0/0x118 [tilcdc]
     drm_dev_unregister+0x24/0x98
     [..snip..]

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
Cc: <stable@vger.kernel.org> #v3.9+
---
 drivers/gpu/drm/tilcdc/tilcdc_panel.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
index 1943b2f..b085dcc 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
@@ -286,10 +286,8 @@ static void panel_destroy(struct tilcdc_module *mod)
 {
 	struct panel_module *panel_mod = to_panel_module(mod);
 
-	if (panel_mod->timings) {
+	if (panel_mod->timings)
 		display_timings_release(panel_mod->timings);
-		kfree(panel_mod->timings);
-	}
 
 	tilcdc_module_cleanup(mod);
 	kfree(panel_mod->info);
-- 
2.0.0

[PATCH/RESEND 8/9] drm/tilcdc: remove submodule destroy calls

[Guido Martínez]

The TI tilcdc driver is designed with a notion of submodules. Currently,
at unload time, these submodules are iterated and destroyed.

Now that the tilcdc remove order is fixed, this can be handled perfectly
by the kernel using the device infrastructure, since each submodule
is a kernel driver itself, and they are only destroy()'ed at unload
time. Therefore we move the destroy() functionality to each submodule's
remove().

Also, remove some checks in the unloading process since the new code
guarantees the resources are allocated and need a release.

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
---
 drivers/gpu/drm/tilcdc/Module.symvers  |  0
 drivers/gpu/drm/tilcdc/tilcdc_drv.c    |  6 ------
 drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
 drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 36 +++++++++++++++++-----------------
 drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 26 +++++++++++++-----------
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 34 ++++++++++++++++----------------
 6 files changed, 50 insertions(+), 53 deletions(-)
 create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers

diff --git a/drivers/gpu/drm/tilcdc/Module.symvers b/drivers/gpu/drm/tilcdc/Module.symvers
new file mode 100644
index 0000000..e69de29
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 006a30e..2c860c4 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -120,7 +120,6 @@ static int cpufreq_transition(struct notifier_block *nb,
 static int tilcdc_unload(struct drm_device *dev)
 {
 	struct tilcdc_drm_private *priv = dev->dev_private;
-	struct tilcdc_module *mod, *cur;
 
 	drm_fbdev_cma_fini(priv->fbdev);
 	drm_kms_helper_poll_fini(dev);
@@ -149,11 +148,6 @@ static int tilcdc_unload(struct drm_device *dev)
 
 	pm_runtime_disable(dev->dev);
 
-	list_for_each_entry_safe(mod, cur, &module_list, list) {
-		DBG("destroying module: %s", mod->name);
-		mod->funcs->destroy(mod);
-	}
-
 	kfree(priv);
 
 	return 0;
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index 0938036..7596c14 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -98,7 +98,6 @@ struct tilcdc_module;
 struct tilcdc_module_ops {
 	/* create appropriate encoders/connectors: */
 	int (*modeset_init)(struct tilcdc_module *mod, struct drm_device *dev);
-	void (*destroy)(struct tilcdc_module *mod);
 #ifdef CONFIG_DEBUG_FS
 	/* create debugfs nodes (can be NULL): */
 	int (*debugfs_init)(struct tilcdc_module *mod, struct drm_minor *minor);
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
index b085dcc..2f6efae 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
@@ -282,21 +282,8 @@ static int panel_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
 	return 0;
 }
 
-static void panel_destroy(struct tilcdc_module *mod)
-{
-	struct panel_module *panel_mod = to_panel_module(mod);
-
-	if (panel_mod->timings)
-		display_timings_release(panel_mod->timings);
-
-	tilcdc_module_cleanup(mod);
-	kfree(panel_mod->info);
-	kfree(panel_mod);
-}
-
 static const struct tilcdc_module_ops panel_module_ops = {
 		.modeset_init = panel_modeset_init,
-		.destroy = panel_destroy,
 };
 
 /*
@@ -372,6 +359,7 @@ static int panel_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	mod = &panel_mod->base;
+	pdev->dev.platform_data = mod;
 
 	tilcdc_module_init(mod, "panel", &panel_module_ops);
 
@@ -379,17 +367,16 @@ static int panel_probe(struct platform_device *pdev)
 	if (IS_ERR(pinctrl))
 		dev_warn(&pdev->dev, "pins are not configured\n");
 
-
 	panel_mod->timings = of_get_display_timings(node);
 	if (!panel_mod->timings) {
 		dev_err(&pdev->dev, "could not get panel timings\n");
-		goto fail;
+		goto fail_free;
 	}
 
 	panel_mod->info = of_get_panel_info(node);
 	if (!panel_mod->info) {
 		dev_err(&pdev->dev, "could not get panel info\n");
-		goto fail;
+		goto fail_timings;
 	}
 
 	mod->preferred_bpp = panel_mod->info->bpp;
@@ -400,13 +387,26 @@ static int panel_probe(struct platform_device *pdev)
 
 	return 0;
 
-fail:
-	panel_destroy(mod);
+fail_timings:
+	display_timings_release(panel_mod->timings);
+
+fail_free:
+	kfree(panel_mod);
+	tilcdc_module_cleanup(mod);
 	return ret;
 }
 
 static int panel_remove(struct platform_device *pdev)
 {
+	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
+	struct panel_module *panel_mod = to_panel_module(mod);
+
+	display_timings_release(panel_mod->timings);
+
+	tilcdc_module_cleanup(mod);
+	kfree(panel_mod->info);
+	kfree(panel_mod);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
index 2f83ffb..1e568ca 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
@@ -296,17 +296,8 @@ static int slave_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
 	return 0;
 }
 
-static void slave_destroy(struct tilcdc_module *mod)
-{
-	struct slave_module *slave_mod = to_slave_module(mod);
-
-	tilcdc_module_cleanup(mod);
-	kfree(slave_mod);
-}
-
 static const struct tilcdc_module_ops slave_module_ops = {
 		.modeset_init = slave_modeset_init,
-		.destroy = slave_destroy,
 };
 
 /*
@@ -356,10 +347,13 @@ static int slave_probe(struct platform_device *pdev)
 	}
 
 	slave_mod = kzalloc(sizeof(*slave_mod), GFP_KERNEL);
-	if (!slave_mod)
-		return -ENOMEM;
+	if (!slave_mod) {
+		ret = -ENOMEM;
+		goto fail_adapter;
+	}
 
 	mod = &slave_mod->base;
+	pdev->dev.platform_data = mod;
 
 	mod->preferred_bpp = slave_info.bpp;
 
@@ -374,10 +368,20 @@ static int slave_probe(struct platform_device *pdev)
 	tilcdc_slave_probedefer(false);
 
 	return 0;
+
+fail_adapter:
+	i2c_put_adapter(slavei2c);
+	return ret;
 }
 
 static int slave_remove(struct platform_device *pdev)
 {
+	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
+	struct slave_module *slave_mod = to_slave_module(mod);
+
+	tilcdc_module_cleanup(mod);
+	kfree(slave_mod);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
index ce75ac8..32a0d2d 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
@@ -296,23 +296,8 @@ static int tfp410_modeset_init(struct tilcdc_module *mod, struct drm_device *dev
 	return 0;
 }
 
-static void tfp410_destroy(struct tilcdc_module *mod)
-{
-	struct tfp410_module *tfp410_mod = to_tfp410_module(mod);
-
-	if (tfp410_mod->i2c)
-		i2c_put_adapter(tfp410_mod->i2c);
-
-	if (!IS_ERR_VALUE(tfp410_mod->gpio))
-		gpio_free(tfp410_mod->gpio);
-
-	tilcdc_module_cleanup(mod);
-	kfree(tfp410_mod);
-}
-
 static const struct tilcdc_module_ops tfp410_module_ops = {
 		.modeset_init = tfp410_modeset_init,
-		.destroy = tfp410_destroy,
 };
 
 /*
@@ -342,6 +327,7 @@ static int tfp410_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	mod = &tfp410_mod->base;
+	pdev->dev.platform_data = mod;
 
 	tilcdc_module_init(mod, "tfp410", &tfp410_module_ops);
 
@@ -365,6 +351,7 @@ static int tfp410_probe(struct platform_device *pdev)
 	tfp410_mod->i2c = of_find_i2c_adapter_by_node(i2c_node);
 	if (!tfp410_mod->i2c) {
 		dev_err(&pdev->dev, "could not get i2c\n");
+		of_node_put(i2c_node);
 		goto fail;
 	}
 
@@ -378,19 +365,32 @@ static int tfp410_probe(struct platform_device *pdev)
 		ret = gpio_request(tfp410_mod->gpio, "DVI_PDn");
 		if (ret) {
 			dev_err(&pdev->dev, "could not get DVI_PDn gpio\n");
-			goto fail;
+			goto fail_adapter;
 		}
 	}
 
 	return 0;
 
+fail_adapter:
+	i2c_put_adapter(tfp410_mod->i2c);
+
 fail:
-	tfp410_destroy(mod);
+	kfree(tfp410_mod);
+	tilcdc_module_cleanup(mod);
 	return ret;
 }
 
 static int tfp410_remove(struct platform_device *pdev)
 {
+	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
+	struct tfp410_module *tfp410_mod = to_tfp410_module(mod);
+
+	i2c_put_adapter(tfp410_mod->i2c);
+	gpio_free(tfp410_mod->gpio);
+
+	tilcdc_module_cleanup(mod);
+	kfree(tfp410_mod);
+
 	return 0;
 }
 
-- 
2.0.0

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

[PATCH/RESEND 8/9] drm/tilcdc: remove submodule destroy calls

[Guido Martínez]

The TI tilcdc driver is designed with a notion of submodules. Currently,
at unload time, these submodules are iterated and destroyed.

Now that the tilcdc remove order is fixed, this can be handled perfectly
by the kernel using the device infrastructure, since each submodule
is a kernel driver itself, and they are only destroy()'ed at unload
time. Therefore we move the destroy() functionality to each submodule's
remove().

Also, remove some checks in the unloading process since the new code
guarantees the resources are allocated and need a release.

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
---
 drivers/gpu/drm/tilcdc/Module.symvers  |  0
 drivers/gpu/drm/tilcdc/tilcdc_drv.c    |  6 ------
 drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
 drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 36 +++++++++++++++++-----------------
 drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 26 +++++++++++++-----------
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 34 ++++++++++++++++----------------
 6 files changed, 50 insertions(+), 53 deletions(-)
 create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers

diff --git a/drivers/gpu/drm/tilcdc/Module.symvers b/drivers/gpu/drm/tilcdc/Module.symvers
new file mode 100644
index 0000000..e69de29
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 006a30e..2c860c4 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -120,7 +120,6 @@ static int cpufreq_transition(struct notifier_block *nb,
 static int tilcdc_unload(struct drm_device *dev)
 {
 	struct tilcdc_drm_private *priv = dev->dev_private;
-	struct tilcdc_module *mod, *cur;
 
 	drm_fbdev_cma_fini(priv->fbdev);
 	drm_kms_helper_poll_fini(dev);
@@ -149,11 +148,6 @@ static int tilcdc_unload(struct drm_device *dev)
 
 	pm_runtime_disable(dev->dev);
 
-	list_for_each_entry_safe(mod, cur, &module_list, list) {
-		DBG("destroying module: %s", mod->name);
-		mod->funcs->destroy(mod);
-	}
-
 	kfree(priv);
 
 	return 0;
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index 0938036..7596c14 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -98,7 +98,6 @@ struct tilcdc_module;
 struct tilcdc_module_ops {
 	/* create appropriate encoders/connectors: */
 	int (*modeset_init)(struct tilcdc_module *mod, struct drm_device *dev);
-	void (*destroy)(struct tilcdc_module *mod);
 #ifdef CONFIG_DEBUG_FS
 	/* create debugfs nodes (can be NULL): */
 	int (*debugfs_init)(struct tilcdc_module *mod, struct drm_minor *minor);
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
index b085dcc..2f6efae 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
@@ -282,21 +282,8 @@ static int panel_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
 	return 0;
 }
 
-static void panel_destroy(struct tilcdc_module *mod)
-{
-	struct panel_module *panel_mod = to_panel_module(mod);
-
-	if (panel_mod->timings)
-		display_timings_release(panel_mod->timings);
-
-	tilcdc_module_cleanup(mod);
-	kfree(panel_mod->info);
-	kfree(panel_mod);
-}
-
 static const struct tilcdc_module_ops panel_module_ops = {
 		.modeset_init = panel_modeset_init,
-		.destroy = panel_destroy,
 };
 
 /*
@@ -372,6 +359,7 @@ static int panel_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	mod = &panel_mod->base;
+	pdev->dev.platform_data = mod;
 
 	tilcdc_module_init(mod, "panel", &panel_module_ops);
 
@@ -379,17 +367,16 @@ static int panel_probe(struct platform_device *pdev)
 	if (IS_ERR(pinctrl))
 		dev_warn(&pdev->dev, "pins are not configured\n");
 
-
 	panel_mod->timings = of_get_display_timings(node);
 	if (!panel_mod->timings) {
 		dev_err(&pdev->dev, "could not get panel timings\n");
-		goto fail;
+		goto fail_free;
 	}
 
 	panel_mod->info = of_get_panel_info(node);
 	if (!panel_mod->info) {
 		dev_err(&pdev->dev, "could not get panel info\n");
-		goto fail;
+		goto fail_timings;
 	}
 
 	mod->preferred_bpp = panel_mod->info->bpp;
@@ -400,13 +387,26 @@ static int panel_probe(struct platform_device *pdev)
 
 	return 0;
 
-fail:
-	panel_destroy(mod);
+fail_timings:
+	display_timings_release(panel_mod->timings);
+
+fail_free:
+	kfree(panel_mod);
+	tilcdc_module_cleanup(mod);
 	return ret;
 }
 
 static int panel_remove(struct platform_device *pdev)
 {
+	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
+	struct panel_module *panel_mod = to_panel_module(mod);
+
+	display_timings_release(panel_mod->timings);
+
+	tilcdc_module_cleanup(mod);
+	kfree(panel_mod->info);
+	kfree(panel_mod);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
index 2f83ffb..1e568ca 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
@@ -296,17 +296,8 @@ static int slave_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
 	return 0;
 }
 
-static void slave_destroy(struct tilcdc_module *mod)
-{
-	struct slave_module *slave_mod = to_slave_module(mod);
-
-	tilcdc_module_cleanup(mod);
-	kfree(slave_mod);
-}
-
 static const struct tilcdc_module_ops slave_module_ops = {
 		.modeset_init = slave_modeset_init,
-		.destroy = slave_destroy,
 };
 
 /*
@@ -356,10 +347,13 @@ static int slave_probe(struct platform_device *pdev)
 	}
 
 	slave_mod = kzalloc(sizeof(*slave_mod), GFP_KERNEL);
-	if (!slave_mod)
-		return -ENOMEM;
+	if (!slave_mod) {
+		ret = -ENOMEM;
+		goto fail_adapter;
+	}
 
 	mod = &slave_mod->base;
+	pdev->dev.platform_data = mod;
 
 	mod->preferred_bpp = slave_info.bpp;
 
@@ -374,10 +368,20 @@ static int slave_probe(struct platform_device *pdev)
 	tilcdc_slave_probedefer(false);
 
 	return 0;
+
+fail_adapter:
+	i2c_put_adapter(slavei2c);
+	return ret;
 }
 
 static int slave_remove(struct platform_device *pdev)
 {
+	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
+	struct slave_module *slave_mod = to_slave_module(mod);
+
+	tilcdc_module_cleanup(mod);
+	kfree(slave_mod);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
index ce75ac8..32a0d2d 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
@@ -296,23 +296,8 @@ static int tfp410_modeset_init(struct tilcdc_module *mod, struct drm_device *dev
 	return 0;
 }
 
-static void tfp410_destroy(struct tilcdc_module *mod)
-{
-	struct tfp410_module *tfp410_mod = to_tfp410_module(mod);
-
-	if (tfp410_mod->i2c)
-		i2c_put_adapter(tfp410_mod->i2c);
-
-	if (!IS_ERR_VALUE(tfp410_mod->gpio))
-		gpio_free(tfp410_mod->gpio);
-
-	tilcdc_module_cleanup(mod);
-	kfree(tfp410_mod);
-}
-
 static const struct tilcdc_module_ops tfp410_module_ops = {
 		.modeset_init = tfp410_modeset_init,
-		.destroy = tfp410_destroy,
 };
 
 /*
@@ -342,6 +327,7 @@ static int tfp410_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	mod = &tfp410_mod->base;
+	pdev->dev.platform_data = mod;
 
 	tilcdc_module_init(mod, "tfp410", &tfp410_module_ops);
 
@@ -365,6 +351,7 @@ static int tfp410_probe(struct platform_device *pdev)
 	tfp410_mod->i2c = of_find_i2c_adapter_by_node(i2c_node);
 	if (!tfp410_mod->i2c) {
 		dev_err(&pdev->dev, "could not get i2c\n");
+		of_node_put(i2c_node);
 		goto fail;
 	}
 
@@ -378,19 +365,32 @@ static int tfp410_probe(struct platform_device *pdev)
 		ret = gpio_request(tfp410_mod->gpio, "DVI_PDn");
 		if (ret) {
 			dev_err(&pdev->dev, "could not get DVI_PDn gpio\n");
-			goto fail;
+			goto fail_adapter;
 		}
 	}
 
 	return 0;
 
+fail_adapter:
+	i2c_put_adapter(tfp410_mod->i2c);
+
 fail:
-	tfp410_destroy(mod);
+	kfree(tfp410_mod);
+	tilcdc_module_cleanup(mod);
 	return ret;
 }
 
 static int tfp410_remove(struct platform_device *pdev)
 {
+	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
+	struct tfp410_module *tfp410_mod = to_tfp410_module(mod);
+
+	i2c_put_adapter(tfp410_mod->i2c);
+	gpio_free(tfp410_mod->gpio);
+
+	tilcdc_module_cleanup(mod);
+	kfree(tfp410_mod);
+
 	return 0;
 }
 
-- 
2.0.0

[PATCH/RESEND 9/9] drm/tilcdc: replace late_initcall with module_init

[Guido Martínez]

Use module_init instead of late_initcall, as is the norm for modular
drivers.

module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b
("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall,
but it does not explain why. Tests show it's working properly with
module_init.

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 2c860c4..6be623b 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -629,7 +629,7 @@ static void __exit tilcdc_drm_fini(void)
 	tilcdc_tfp410_fini();
 }
 
-late_initcall(tilcdc_drm_init);
+module_init(tilcdc_drm_init);
 module_exit(tilcdc_drm_fini);
 
 MODULE_AUTHOR("Rob Clark <robdclark@gmail.com");
-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RESEND 9/9] drm/tilcdc: replace late_initcall with module_init

[Guido Martínez]

Use module_init instead of late_initcall, as is the norm for modular
drivers.

module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b
("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall,
but it does not explain why. Tests show it's working properly with
module_init.

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 2c860c4..6be623b 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -629,7 +629,7 @@ static void __exit tilcdc_drm_fini(void)
 	tilcdc_tfp410_fini();
 }
 
-late_initcall(tilcdc_drm_init);
+module_init(tilcdc_drm_init);
 module_exit(tilcdc_drm_fini);
 
 MODULE_AUTHOR("Rob Clark <robdclark@gmail.com");
-- 
2.0.0

Re: [PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Darren Etheridge]

Guido,

Thanks and sorry I missed this the first time around.  I'll give it a 
try on a few of my AM335x based boards when I have access to them again 
on Monday.

Darren

On 06/17/2014 09:17 AM, Guido Martínez wrote:
> The tilcdc driver could be compiled as a module, but was severely broken
> and could not be used as such. This patchset attempts to fix the issues
> preventing a proper load/unload of the module.
>
> Issues included dangling sysfs nodes, dangling devices, memory leaks and
> a double kfree.
>
> It now seems to be working ok. We have tested this by loading and
> unloading the driver repeteadly, with both panel and slave connectors
> and found no flaws.
>
> There is still one warning left on tilcdc_crtc_destroy, caused by
> destroying the connector while still in an ON status. We don't know why
> this happens or why it's an issue, so we did not fix it.
>
> The first 7 patches are bug fixes which and should probably be applied
> in the stable trees. The last two are clean-ups.
>
>
> Resending this since I got no replies.
>
>
> Guido Martínez (9):
>    drm/i2c: tda998x: move drm_i2c_encoder_destroy call
>    drm/tilcdc: panel: fix dangling sysfs connector node
>    drm/tilcdc: slave: fix dangling sysfs connector node
>    drm/tilcdc: tfp410: fix dangling sysfs connector node
>    drm/tilcdc: panel: fix leak when unloading the module
>    drm/tilcdc: fix release order on exit
>    drm/tilcdc: fix double kfree
>    drm/tilcdc: remove submodule destroy calls
>    drm/tilcdc: replace late_initcall with module_init
>
>   drivers/gpu/drm/i2c/tda998x_drv.c      |  2 +-
>   drivers/gpu/drm/tilcdc/Module.symvers  |  0
>   drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 15 +++++--------
>   drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
>   drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 39 +++++++++++++++++-----------------
>   drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 27 +++++++++++++----------
>   drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35 +++++++++++++++---------------
>   7 files changed, 59 insertions(+), 60 deletions(-)
>   create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Darren Etheridge]

Guido,

Thanks and sorry I missed this the first time around.  I'll give it a 
try on a few of my AM335x based boards when I have access to them again 
on Monday.

Darren

On 06/17/2014 09:17 AM, Guido Mart?nez wrote:
> The tilcdc driver could be compiled as a module, but was severely broken
> and could not be used as such. This patchset attempts to fix the issues
> preventing a proper load/unload of the module.
>
> Issues included dangling sysfs nodes, dangling devices, memory leaks and
> a double kfree.
>
> It now seems to be working ok. We have tested this by loading and
> unloading the driver repeteadly, with both panel and slave connectors
> and found no flaws.
>
> There is still one warning left on tilcdc_crtc_destroy, caused by
> destroying the connector while still in an ON status. We don't know why
> this happens or why it's an issue, so we did not fix it.
>
> The first 7 patches are bug fixes which and should probably be applied
> in the stable trees. The last two are clean-ups.
>
>
> Resending this since I got no replies.
>
>
> Guido Mart?nez (9):
>    drm/i2c: tda998x: move drm_i2c_encoder_destroy call
>    drm/tilcdc: panel: fix dangling sysfs connector node
>    drm/tilcdc: slave: fix dangling sysfs connector node
>    drm/tilcdc: tfp410: fix dangling sysfs connector node
>    drm/tilcdc: panel: fix leak when unloading the module
>    drm/tilcdc: fix release order on exit
>    drm/tilcdc: fix double kfree
>    drm/tilcdc: remove submodule destroy calls
>    drm/tilcdc: replace late_initcall with module_init
>
>   drivers/gpu/drm/i2c/tda998x_drv.c      |  2 +-
>   drivers/gpu/drm/tilcdc/Module.symvers  |  0
>   drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 15 +++++--------
>   drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
>   drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 39 +++++++++++++++++-----------------
>   drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 27 +++++++++++++----------
>   drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35 +++++++++++++++---------------
>   7 files changed, 59 insertions(+), 60 deletions(-)
>   create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
>

Re: [PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Guido Martínez]

Hi Darren,

On Thu, Jun 19, 2014 at 08:41:43AM -0500, Darren Etheridge wrote:
> Guido,
> 
> Thanks and sorry I missed this the first time around.  I'll give it a try on
> a few of my AM335x based boards when I have access to them again on Monday.
Thanks! And no worries :)

> 
> Darren
> 
-- 
Guido Martínez, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Guido Martínez]

Hi Darren,

On Thu, Jun 19, 2014 at 08:41:43AM -0500, Darren Etheridge wrote:
> Guido,
> 
> Thanks and sorry I missed this the first time around.  I'll give it a try on
> a few of my AM335x based boards when I have access to them again on Monday.
Thanks! And no worries :)

> 
> Darren
> 
-- 
Guido Mart?nez, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Guido Martínez]

Hi Rob,

On Tue, Jun 17, 2014 at 11:17:02AM -0300, Guido Martínez wrote:
> The tilcdc driver could be compiled as a module, but was severely broken
> and could not be used as such. This patchset attempts to fix the issues
> preventing a proper load/unload of the module.
> 
> Issues included dangling sysfs nodes, dangling devices, memory leaks and
> a double kfree.
> 
> It now seems to be working ok. We have tested this by loading and
> unloading the driver repeteadly, with both panel and slave connectors
> and found no flaws.
> 
> There is still one warning left on tilcdc_crtc_destroy, caused by
> destroying the connector while still in an ON status. We don't know why
> this happens or why it's an issue, so we did not fix it.
> 
> The first 7 patches are bug fixes which and should probably be applied
> in the stable trees. The last two are clean-ups.
>

Any comment on this patchset?

Thanks!

> 
> Resending this since I got no replies.
> 
> 
> Guido Martínez (9):
>   drm/i2c: tda998x: move drm_i2c_encoder_destroy call
>   drm/tilcdc: panel: fix dangling sysfs connector node
>   drm/tilcdc: slave: fix dangling sysfs connector node
>   drm/tilcdc: tfp410: fix dangling sysfs connector node
>   drm/tilcdc: panel: fix leak when unloading the module
>   drm/tilcdc: fix release order on exit
>   drm/tilcdc: fix double kfree
>   drm/tilcdc: remove submodule destroy calls
>   drm/tilcdc: replace late_initcall with module_init
> 
>  drivers/gpu/drm/i2c/tda998x_drv.c      |  2 +-
>  drivers/gpu/drm/tilcdc/Module.symvers  |  0
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 15 +++++--------
>  drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
>  drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 39 +++++++++++++++++-----------------
>  drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 27 +++++++++++++----------
>  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35 +++++++++++++++---------------
>  7 files changed, 59 insertions(+), 60 deletions(-)
>  create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
> 
> -- 
> 2.0.0
> 

-- 
Guido Martínez, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Guido Martínez]

Hi Rob,

On Tue, Jun 17, 2014 at 11:17:02AM -0300, Guido Mart?nez wrote:
> The tilcdc driver could be compiled as a module, but was severely broken
> and could not be used as such. This patchset attempts to fix the issues
> preventing a proper load/unload of the module.
> 
> Issues included dangling sysfs nodes, dangling devices, memory leaks and
> a double kfree.
> 
> It now seems to be working ok. We have tested this by loading and
> unloading the driver repeteadly, with both panel and slave connectors
> and found no flaws.
> 
> There is still one warning left on tilcdc_crtc_destroy, caused by
> destroying the connector while still in an ON status. We don't know why
> this happens or why it's an issue, so we did not fix it.
> 
> The first 7 patches are bug fixes which and should probably be applied
> in the stable trees. The last two are clean-ups.
>

Any comment on this patchset?

Thanks!

> 
> Resending this since I got no replies.
> 
> 
> Guido Mart?nez (9):
>   drm/i2c: tda998x: move drm_i2c_encoder_destroy call
>   drm/tilcdc: panel: fix dangling sysfs connector node
>   drm/tilcdc: slave: fix dangling sysfs connector node
>   drm/tilcdc: tfp410: fix dangling sysfs connector node
>   drm/tilcdc: panel: fix leak when unloading the module
>   drm/tilcdc: fix release order on exit
>   drm/tilcdc: fix double kfree
>   drm/tilcdc: remove submodule destroy calls
>   drm/tilcdc: replace late_initcall with module_init
> 
>  drivers/gpu/drm/i2c/tda998x_drv.c      |  2 +-
>  drivers/gpu/drm/tilcdc/Module.symvers  |  0
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 15 +++++--------
>  drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
>  drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 39 +++++++++++++++++-----------------
>  drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 27 +++++++++++++----------
>  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35 +++++++++++++++---------------
>  7 files changed, 59 insertions(+), 60 deletions(-)
>  create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
> 
> -- 
> 2.0.0
> 

-- 
Guido Mart?nez, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH/RESEND 1/9] drm/i2c: tda998x: move drm_i2c_encoder_destroy call

[Russell King - ARM Linux]

On Tue, Jun 17, 2014 at 11:17:03AM -0300, Guido Martínez wrote:
> Currently tda998x_encoder_destroy() calls cec_write() and reg_clear(),
> as part of the release procedure. Such calls need to access the I2C bus
> and therefore, we need to call them before drm_i2c_encoder_destroy()
> which unregisters the I2C device.
> 
> This commit moves the latter so it's done afterwards.
> 
> Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
> Signed-off-by: Ezequiel García <ezequiel@vanguardiasur.com.ar>
> Cc: <stable@vger.kernel.org> #v3.9+

You really should have sent this with me in the To: header as I'm now the
maintainer of this driver.  Yes, this is a valid fix, and I'll apply it
shortly.  Thanks.

> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 240c331..db9515f 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1183,7 +1183,6 @@ static void
>  tda998x_encoder_destroy(struct drm_encoder *encoder)
>  {
>  	struct tda998x_priv *priv = to_tda998x_priv(encoder);
> -	drm_i2c_encoder_destroy(encoder);
>  
>  	/* disable all IRQs and free the IRQ handler */
>  	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
> @@ -1193,6 +1192,7 @@ tda998x_encoder_destroy(struct drm_encoder *encoder)
>  
>  	if (priv->cec)
>  		i2c_unregister_device(priv->cec);
> +	drm_i2c_encoder_destroy(encoder);
>  	kfree(priv);
>  }
>  
> -- 
> 2.0.0
> 

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RESEND 1/9] drm/i2c: tda998x: move drm_i2c_encoder_destroy call

[Russell King - ARM Linux]

On Tue, Jun 17, 2014 at 11:17:03AM -0300, Guido Mart?nez wrote:
> Currently tda998x_encoder_destroy() calls cec_write() and reg_clear(),
> as part of the release procedure. Such calls need to access the I2C bus
> and therefore, we need to call them before drm_i2c_encoder_destroy()
> which unregisters the I2C device.
> 
> This commit moves the latter so it's done afterwards.
> 
> Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
> Signed-off-by: Ezequiel Garc?a <ezequiel@vanguardiasur.com.ar>
> Cc: <stable@vger.kernel.org> #v3.9+

You really should have sent this with me in the To: header as I'm now the
maintainer of this driver.  Yes, this is a valid fix, and I'll apply it
shortly.  Thanks.

> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 240c331..db9515f 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1183,7 +1183,6 @@ static void
>  tda998x_encoder_destroy(struct drm_encoder *encoder)
>  {
>  	struct tda998x_priv *priv = to_tda998x_priv(encoder);
> -	drm_i2c_encoder_destroy(encoder);
>  
>  	/* disable all IRQs and free the IRQ handler */
>  	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
> @@ -1193,6 +1192,7 @@ tda998x_encoder_destroy(struct drm_encoder *encoder)
>  
>  	if (priv->cec)
>  		i2c_unregister_device(priv->cec);
> +	drm_i2c_encoder_destroy(encoder);
>  	kfree(priv);
>  }
>  
> -- 
> 2.0.0
> 

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH/RESEND 9/9] drm/tilcdc: replace late_initcall with module_init

[Darren Etheridge]

On 06/17/2014 09:17 AM, Guido Martínez wrote:
> Use module_init instead of late_initcall, as is the norm for modular
> drivers.
>
> module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b
> ("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall,
> but it does not explain why. Tests show it's working properly with
> module_init.
>

If I recall, the late_initcall stuff was done to try and make sure the 
tda998x/i2c subsystem came up before tilcdc. However it didn't always 
work so we added commit: 39de6194131c155901f96686a063212656d80c2e to try 
and ensure the ordering.  This might be why changing back to module_init 
is fine (and I agree with your assessment from my testing).

> Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
> ---
>   drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index 2c860c4..6be623b 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -629,7 +629,7 @@ static void __exit tilcdc_drm_fini(void)
>   	tilcdc_tfp410_fini();
>   }
>
> -late_initcall(tilcdc_drm_init);
> +module_init(tilcdc_drm_init);
>   module_exit(tilcdc_drm_fini);
>
>   MODULE_AUTHOR("Rob Clark <robdclark@gmail.com");
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH/RESEND 9/9] drm/tilcdc: replace late_initcall with module_init

[Darren Etheridge]

On 06/17/2014 09:17 AM, Guido Mart?nez wrote:
> Use module_init instead of late_initcall, as is the norm for modular
> drivers.
>
> module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b
> ("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall,
> but it does not explain why. Tests show it's working properly with
> module_init.
>

If I recall, the late_initcall stuff was done to try and make sure the 
tda998x/i2c subsystem came up before tilcdc. However it didn't always 
work so we added commit: 39de6194131c155901f96686a063212656d80c2e to try 
and ensure the ordering.  This might be why changing back to module_init 
is fine (and I agree with your assessment from my testing).

> Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
> ---
>   drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index 2c860c4..6be623b 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -629,7 +629,7 @@ static void __exit tilcdc_drm_fini(void)
>   	tilcdc_tfp410_fini();
>   }
>
> -late_initcall(tilcdc_drm_init);
> +module_init(tilcdc_drm_init);
>   module_exit(tilcdc_drm_fini);
>
>   MODULE_AUTHOR("Rob Clark <robdclark@gmail.com");
>

Re: [PATCH/RESEND 8/9] drm/tilcdc: remove submodule destroy calls

[Darren Etheridge]

Guido,

On 06/17/2014 09:17 AM, Guido Martínez wrote:
> The TI tilcdc driver is designed with a notion of submodules. Currently,
> at unload time, these submodules are iterated and destroyed.
>
> Now that the tilcdc remove order is fixed, this can be handled perfectly

I am not sure I understand the first part of the above sentence - did 
something change with tilcdc ordering?  I think you a referring to 
previous patches in your series which really mean tilcdc can actually 
unload now.  So really the method this patch uses could always have been 
used, it just wasn't for some reason?

I have tested all of the other patches in your series and all looks good 
on BeagleBone Black and AM335xEVM, I tested as both built-ins and 
modules and can load/unload on BeagleBone Black with HDMI enabled correctly.

I want to play around a bit more with this particular patch, to try and 
understand how it differs from Rob's original intent with his module 
registration/deregistration scheme.  I prefer your method, but do we 
loose anything that Rob's originally had in mind?

Darren

> by the kernel using the device infrastructure, since each submodule
> is a kernel driver itself, and they are only destroy()'ed at unload
> time. Therefore we move the destroy() functionality to each submodule's
> remove().
>
> Also, remove some checks in the unloading process since the new code
> guarantees the resources are allocated and need a release.
>
> Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
> ---
>   drivers/gpu/drm/tilcdc/Module.symvers  |  0
>   drivers/gpu/drm/tilcdc/tilcdc_drv.c    |  6 ------
>   drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
>   drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 36 +++++++++++++++++-----------------
>   drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 26 +++++++++++++-----------
>   drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 34 ++++++++++++++++----------------
>   6 files changed, 50 insertions(+), 53 deletions(-)
>   create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
>
> diff --git a/drivers/gpu/drm/tilcdc/Module.symvers b/drivers/gpu/drm/tilcdc/Module.symvers
> new file mode 100644
> index 0000000..e69de29
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index 006a30e..2c860c4 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -120,7 +120,6 @@ static int cpufreq_transition(struct notifier_block *nb,
>   static int tilcdc_unload(struct drm_device *dev)
>   {
>   	struct tilcdc_drm_private *priv = dev->dev_private;
> -	struct tilcdc_module *mod, *cur;
>
>   	drm_fbdev_cma_fini(priv->fbdev);
>   	drm_kms_helper_poll_fini(dev);
> @@ -149,11 +148,6 @@ static int tilcdc_unload(struct drm_device *dev)
>
>   	pm_runtime_disable(dev->dev);
>
> -	list_for_each_entry_safe(mod, cur, &module_list, list) {
> -		DBG("destroying module: %s", mod->name);
> -		mod->funcs->destroy(mod);
> -	}
> -
>   	kfree(priv);
>
>   	return 0;
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> index 0938036..7596c14 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> @@ -98,7 +98,6 @@ struct tilcdc_module;
>   struct tilcdc_module_ops {
>   	/* create appropriate encoders/connectors: */
>   	int (*modeset_init)(struct tilcdc_module *mod, struct drm_device *dev);
> -	void (*destroy)(struct tilcdc_module *mod);
>   #ifdef CONFIG_DEBUG_FS
>   	/* create debugfs nodes (can be NULL): */
>   	int (*debugfs_init)(struct tilcdc_module *mod, struct drm_minor *minor);
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> index b085dcc..2f6efae 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> @@ -282,21 +282,8 @@ static int panel_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
>   	return 0;
>   }
>
> -static void panel_destroy(struct tilcdc_module *mod)
> -{
> -	struct panel_module *panel_mod = to_panel_module(mod);
> -
> -	if (panel_mod->timings)
> -		display_timings_release(panel_mod->timings);
> -
> -	tilcdc_module_cleanup(mod);
> -	kfree(panel_mod->info);
> -	kfree(panel_mod);
> -}
> -
>   static const struct tilcdc_module_ops panel_module_ops = {
>   		.modeset_init = panel_modeset_init,
> -		.destroy = panel_destroy,
>   };
>
>   /*
> @@ -372,6 +359,7 @@ static int panel_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>
>   	mod = &panel_mod->base;
> +	pdev->dev.platform_data = mod;
>
>   	tilcdc_module_init(mod, "panel", &panel_module_ops);
>
> @@ -379,17 +367,16 @@ static int panel_probe(struct platform_device *pdev)
>   	if (IS_ERR(pinctrl))
>   		dev_warn(&pdev->dev, "pins are not configured\n");
>
> -
>   	panel_mod->timings = of_get_display_timings(node);
>   	if (!panel_mod->timings) {
>   		dev_err(&pdev->dev, "could not get panel timings\n");
> -		goto fail;
> +		goto fail_free;
>   	}
>
>   	panel_mod->info = of_get_panel_info(node);
>   	if (!panel_mod->info) {
>   		dev_err(&pdev->dev, "could not get panel info\n");
> -		goto fail;
> +		goto fail_timings;
>   	}
>
>   	mod->preferred_bpp = panel_mod->info->bpp;
> @@ -400,13 +387,26 @@ static int panel_probe(struct platform_device *pdev)
>
>   	return 0;
>
> -fail:
> -	panel_destroy(mod);
> +fail_timings:
> +	display_timings_release(panel_mod->timings);
> +
> +fail_free:
> +	kfree(panel_mod);
> +	tilcdc_module_cleanup(mod);
>   	return ret;
>   }
>
>   static int panel_remove(struct platform_device *pdev)
>   {
> +	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
> +	struct panel_module *panel_mod = to_panel_module(mod);
> +
> +	display_timings_release(panel_mod->timings);
> +
> +	tilcdc_module_cleanup(mod);
> +	kfree(panel_mod->info);
> +	kfree(panel_mod);
> +
>   	return 0;
>   }
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
> index 2f83ffb..1e568ca 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
> @@ -296,17 +296,8 @@ static int slave_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
>   	return 0;
>   }
>
> -static void slave_destroy(struct tilcdc_module *mod)
> -{
> -	struct slave_module *slave_mod = to_slave_module(mod);
> -
> -	tilcdc_module_cleanup(mod);
> -	kfree(slave_mod);
> -}
> -
>   static const struct tilcdc_module_ops slave_module_ops = {
>   		.modeset_init = slave_modeset_init,
> -		.destroy = slave_destroy,
>   };
>
>   /*
> @@ -356,10 +347,13 @@ static int slave_probe(struct platform_device *pdev)
>   	}
>
>   	slave_mod = kzalloc(sizeof(*slave_mod), GFP_KERNEL);
> -	if (!slave_mod)
> -		return -ENOMEM;
> +	if (!slave_mod) {
> +		ret = -ENOMEM;
> +		goto fail_adapter;
> +	}
>
>   	mod = &slave_mod->base;
> +	pdev->dev.platform_data = mod;
>
>   	mod->preferred_bpp = slave_info.bpp;
>
> @@ -374,10 +368,20 @@ static int slave_probe(struct platform_device *pdev)
>   	tilcdc_slave_probedefer(false);
>
>   	return 0;
> +
> +fail_adapter:
> +	i2c_put_adapter(slavei2c);
> +	return ret;
>   }
>
>   static int slave_remove(struct platform_device *pdev)
>   {
> +	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
> +	struct slave_module *slave_mod = to_slave_module(mod);
> +
> +	tilcdc_module_cleanup(mod);
> +	kfree(slave_mod);
> +
>   	return 0;
>   }
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> index ce75ac8..32a0d2d 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> @@ -296,23 +296,8 @@ static int tfp410_modeset_init(struct tilcdc_module *mod, struct drm_device *dev
>   	return 0;
>   }
>
> -static void tfp410_destroy(struct tilcdc_module *mod)
> -{
> -	struct tfp410_module *tfp410_mod = to_tfp410_module(mod);
> -
> -	if (tfp410_mod->i2c)
> -		i2c_put_adapter(tfp410_mod->i2c);
> -
> -	if (!IS_ERR_VALUE(tfp410_mod->gpio))
> -		gpio_free(tfp410_mod->gpio);
> -
> -	tilcdc_module_cleanup(mod);
> -	kfree(tfp410_mod);
> -}
> -
>   static const struct tilcdc_module_ops tfp410_module_ops = {
>   		.modeset_init = tfp410_modeset_init,
> -		.destroy = tfp410_destroy,
>   };
>
>   /*
> @@ -342,6 +327,7 @@ static int tfp410_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>
>   	mod = &tfp410_mod->base;
> +	pdev->dev.platform_data = mod;
>
>   	tilcdc_module_init(mod, "tfp410", &tfp410_module_ops);
>
> @@ -365,6 +351,7 @@ static int tfp410_probe(struct platform_device *pdev)
>   	tfp410_mod->i2c = of_find_i2c_adapter_by_node(i2c_node);
>   	if (!tfp410_mod->i2c) {
>   		dev_err(&pdev->dev, "could not get i2c\n");
> +		of_node_put(i2c_node);
>   		goto fail;
>   	}
>
> @@ -378,19 +365,32 @@ static int tfp410_probe(struct platform_device *pdev)
>   		ret = gpio_request(tfp410_mod->gpio, "DVI_PDn");
>   		if (ret) {
>   			dev_err(&pdev->dev, "could not get DVI_PDn gpio\n");
> -			goto fail;
> +			goto fail_adapter;
>   		}
>   	}
>
>   	return 0;
>
> +fail_adapter:
> +	i2c_put_adapter(tfp410_mod->i2c);
> +
>   fail:
> -	tfp410_destroy(mod);
> +	kfree(tfp410_mod);
> +	tilcdc_module_cleanup(mod);
>   	return ret;
>   }
>
>   static int tfp410_remove(struct platform_device *pdev)
>   {
> +	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
> +	struct tfp410_module *tfp410_mod = to_tfp410_module(mod);
> +
> +	i2c_put_adapter(tfp410_mod->i2c);
> +	gpio_free(tfp410_mod->gpio);
> +
> +	tilcdc_module_cleanup(mod);
> +	kfree(tfp410_mod);
> +
>   	return 0;
>   }
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH/RESEND 8/9] drm/tilcdc: remove submodule destroy calls

[Darren Etheridge]

Guido,

On 06/17/2014 09:17 AM, Guido Mart?nez wrote:
> The TI tilcdc driver is designed with a notion of submodules. Currently,
> at unload time, these submodules are iterated and destroyed.
>
> Now that the tilcdc remove order is fixed, this can be handled perfectly

I am not sure I understand the first part of the above sentence - did 
something change with tilcdc ordering?  I think you a referring to 
previous patches in your series which really mean tilcdc can actually 
unload now.  So really the method this patch uses could always have been 
used, it just wasn't for some reason?

I have tested all of the other patches in your series and all looks good 
on BeagleBone Black and AM335xEVM, I tested as both built-ins and 
modules and can load/unload on BeagleBone Black with HDMI enabled correctly.

I want to play around a bit more with this particular patch, to try and 
understand how it differs from Rob's original intent with his module 
registration/deregistration scheme.  I prefer your method, but do we 
loose anything that Rob's originally had in mind?

Darren

> by the kernel using the device infrastructure, since each submodule
> is a kernel driver itself, and they are only destroy()'ed at unload
> time. Therefore we move the destroy() functionality to each submodule's
> remove().
>
> Also, remove some checks in the unloading process since the new code
> guarantees the resources are allocated and need a release.
>
> Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
> ---
>   drivers/gpu/drm/tilcdc/Module.symvers  |  0
>   drivers/gpu/drm/tilcdc/tilcdc_drv.c    |  6 ------
>   drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
>   drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 36 +++++++++++++++++-----------------
>   drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 26 +++++++++++++-----------
>   drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 34 ++++++++++++++++----------------
>   6 files changed, 50 insertions(+), 53 deletions(-)
>   create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
>
> diff --git a/drivers/gpu/drm/tilcdc/Module.symvers b/drivers/gpu/drm/tilcdc/Module.symvers
> new file mode 100644
> index 0000000..e69de29
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index 006a30e..2c860c4 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -120,7 +120,6 @@ static int cpufreq_transition(struct notifier_block *nb,
>   static int tilcdc_unload(struct drm_device *dev)
>   {
>   	struct tilcdc_drm_private *priv = dev->dev_private;
> -	struct tilcdc_module *mod, *cur;
>
>   	drm_fbdev_cma_fini(priv->fbdev);
>   	drm_kms_helper_poll_fini(dev);
> @@ -149,11 +148,6 @@ static int tilcdc_unload(struct drm_device *dev)
>
>   	pm_runtime_disable(dev->dev);
>
> -	list_for_each_entry_safe(mod, cur, &module_list, list) {
> -		DBG("destroying module: %s", mod->name);
> -		mod->funcs->destroy(mod);
> -	}
> -
>   	kfree(priv);
>
>   	return 0;
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> index 0938036..7596c14 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> @@ -98,7 +98,6 @@ struct tilcdc_module;
>   struct tilcdc_module_ops {
>   	/* create appropriate encoders/connectors: */
>   	int (*modeset_init)(struct tilcdc_module *mod, struct drm_device *dev);
> -	void (*destroy)(struct tilcdc_module *mod);
>   #ifdef CONFIG_DEBUG_FS
>   	/* create debugfs nodes (can be NULL): */
>   	int (*debugfs_init)(struct tilcdc_module *mod, struct drm_minor *minor);
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> index b085dcc..2f6efae 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> @@ -282,21 +282,8 @@ static int panel_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
>   	return 0;
>   }
>
> -static void panel_destroy(struct tilcdc_module *mod)
> -{
> -	struct panel_module *panel_mod = to_panel_module(mod);
> -
> -	if (panel_mod->timings)
> -		display_timings_release(panel_mod->timings);
> -
> -	tilcdc_module_cleanup(mod);
> -	kfree(panel_mod->info);
> -	kfree(panel_mod);
> -}
> -
>   static const struct tilcdc_module_ops panel_module_ops = {
>   		.modeset_init = panel_modeset_init,
> -		.destroy = panel_destroy,
>   };
>
>   /*
> @@ -372,6 +359,7 @@ static int panel_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>
>   	mod = &panel_mod->base;
> +	pdev->dev.platform_data = mod;
>
>   	tilcdc_module_init(mod, "panel", &panel_module_ops);
>
> @@ -379,17 +367,16 @@ static int panel_probe(struct platform_device *pdev)
>   	if (IS_ERR(pinctrl))
>   		dev_warn(&pdev->dev, "pins are not configured\n");
>
> -
>   	panel_mod->timings = of_get_display_timings(node);
>   	if (!panel_mod->timings) {
>   		dev_err(&pdev->dev, "could not get panel timings\n");
> -		goto fail;
> +		goto fail_free;
>   	}
>
>   	panel_mod->info = of_get_panel_info(node);
>   	if (!panel_mod->info) {
>   		dev_err(&pdev->dev, "could not get panel info\n");
> -		goto fail;
> +		goto fail_timings;
>   	}
>
>   	mod->preferred_bpp = panel_mod->info->bpp;
> @@ -400,13 +387,26 @@ static int panel_probe(struct platform_device *pdev)
>
>   	return 0;
>
> -fail:
> -	panel_destroy(mod);
> +fail_timings:
> +	display_timings_release(panel_mod->timings);
> +
> +fail_free:
> +	kfree(panel_mod);
> +	tilcdc_module_cleanup(mod);
>   	return ret;
>   }
>
>   static int panel_remove(struct platform_device *pdev)
>   {
> +	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
> +	struct panel_module *panel_mod = to_panel_module(mod);
> +
> +	display_timings_release(panel_mod->timings);
> +
> +	tilcdc_module_cleanup(mod);
> +	kfree(panel_mod->info);
> +	kfree(panel_mod);
> +
>   	return 0;
>   }
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
> index 2f83ffb..1e568ca 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
> @@ -296,17 +296,8 @@ static int slave_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
>   	return 0;
>   }
>
> -static void slave_destroy(struct tilcdc_module *mod)
> -{
> -	struct slave_module *slave_mod = to_slave_module(mod);
> -
> -	tilcdc_module_cleanup(mod);
> -	kfree(slave_mod);
> -}
> -
>   static const struct tilcdc_module_ops slave_module_ops = {
>   		.modeset_init = slave_modeset_init,
> -		.destroy = slave_destroy,
>   };
>
>   /*
> @@ -356,10 +347,13 @@ static int slave_probe(struct platform_device *pdev)
>   	}
>
>   	slave_mod = kzalloc(sizeof(*slave_mod), GFP_KERNEL);
> -	if (!slave_mod)
> -		return -ENOMEM;
> +	if (!slave_mod) {
> +		ret = -ENOMEM;
> +		goto fail_adapter;
> +	}
>
>   	mod = &slave_mod->base;
> +	pdev->dev.platform_data = mod;
>
>   	mod->preferred_bpp = slave_info.bpp;
>
> @@ -374,10 +368,20 @@ static int slave_probe(struct platform_device *pdev)
>   	tilcdc_slave_probedefer(false);
>
>   	return 0;
> +
> +fail_adapter:
> +	i2c_put_adapter(slavei2c);
> +	return ret;
>   }
>
>   static int slave_remove(struct platform_device *pdev)
>   {
> +	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
> +	struct slave_module *slave_mod = to_slave_module(mod);
> +
> +	tilcdc_module_cleanup(mod);
> +	kfree(slave_mod);
> +
>   	return 0;
>   }
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> index ce75ac8..32a0d2d 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> @@ -296,23 +296,8 @@ static int tfp410_modeset_init(struct tilcdc_module *mod, struct drm_device *dev
>   	return 0;
>   }
>
> -static void tfp410_destroy(struct tilcdc_module *mod)
> -{
> -	struct tfp410_module *tfp410_mod = to_tfp410_module(mod);
> -
> -	if (tfp410_mod->i2c)
> -		i2c_put_adapter(tfp410_mod->i2c);
> -
> -	if (!IS_ERR_VALUE(tfp410_mod->gpio))
> -		gpio_free(tfp410_mod->gpio);
> -
> -	tilcdc_module_cleanup(mod);
> -	kfree(tfp410_mod);
> -}
> -
>   static const struct tilcdc_module_ops tfp410_module_ops = {
>   		.modeset_init = tfp410_modeset_init,
> -		.destroy = tfp410_destroy,
>   };
>
>   /*
> @@ -342,6 +327,7 @@ static int tfp410_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>
>   	mod = &tfp410_mod->base;
> +	pdev->dev.platform_data = mod;
>
>   	tilcdc_module_init(mod, "tfp410", &tfp410_module_ops);
>
> @@ -365,6 +351,7 @@ static int tfp410_probe(struct platform_device *pdev)
>   	tfp410_mod->i2c = of_find_i2c_adapter_by_node(i2c_node);
>   	if (!tfp410_mod->i2c) {
>   		dev_err(&pdev->dev, "could not get i2c\n");
> +		of_node_put(i2c_node);
>   		goto fail;
>   	}
>
> @@ -378,19 +365,32 @@ static int tfp410_probe(struct platform_device *pdev)
>   		ret = gpio_request(tfp410_mod->gpio, "DVI_PDn");
>   		if (ret) {
>   			dev_err(&pdev->dev, "could not get DVI_PDn gpio\n");
> -			goto fail;
> +			goto fail_adapter;
>   		}
>   	}
>
>   	return 0;
>
> +fail_adapter:
> +	i2c_put_adapter(tfp410_mod->i2c);
> +
>   fail:
> -	tfp410_destroy(mod);
> +	kfree(tfp410_mod);
> +	tilcdc_module_cleanup(mod);
>   	return ret;
>   }
>
>   static int tfp410_remove(struct platform_device *pdev)
>   {
> +	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
> +	struct tfp410_module *tfp410_mod = to_tfp410_module(mod);
> +
> +	i2c_put_adapter(tfp410_mod->i2c);
> +	gpio_free(tfp410_mod->gpio);
> +
> +	tilcdc_module_cleanup(mod);
> +	kfree(tfp410_mod);
> +
>   	return 0;
>   }
>
>

Re: [PATCH/RESEND 8/9] drm/tilcdc: remove submodule destroy calls

[Guido Martínez]

On Tue, Jun 24, 2014 at 05:06:24PM -0500, Darren Etheridge wrote:
> Guido,
> 
> On 06/17/2014 09:17 AM, Guido Martínez wrote:
> >The TI tilcdc driver is designed with a notion of submodules. Currently,
> >at unload time, these submodules are iterated and destroyed.
> >
> >Now that the tilcdc remove order is fixed, this can be handled perfectly
> 
> I am not sure I understand the first part of the above sentence - did
> something change with tilcdc ordering?  I think you a referring to previous
> patches in your series which really mean tilcdc can actually unload now.
Right, I guess I got a bit dizzy while writing that commit log :). If we
find the patch reasonable I'll send a better explanation.

> So really the method this patch uses could always have been used, it
> just wasn't for some reason?
Yes, I think so.

> I have tested all of the other patches in your series and all looks good on
> BeagleBone Black and AM335xEVM, I tested as both built-ins and modules and
> can load/unload on BeagleBone Black with HDMI enabled correctly.
Nice to hear that :)

> I want to play around a bit more with this particular patch, to try and
> understand how it differs from Rob's original intent with his module
> registration/deregistration scheme.  I prefer your method, but do we loose
> anything that Rob's originally had in mind?
Nothing really comes to mind, but I may be wrong on this...

Thanks a lot for your review!

> Darren
> 
> >by the kernel using the device infrastructure, since each submodule
> >is a kernel driver itself, and they are only destroy()'ed at unload
> >time. Therefore we move the destroy() functionality to each submodule's
> >remove().
> >
> >Also, remove some checks in the unloading process since the new code
> >guarantees the resources are allocated and need a release.
> >
> >Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
> >---
> >  drivers/gpu/drm/tilcdc/Module.symvers  |  0
> >  drivers/gpu/drm/tilcdc/tilcdc_drv.c    |  6 ------
> >  drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
> >  drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 36 +++++++++++++++++-----------------
> >  drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 26 +++++++++++++-----------
> >  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 34 ++++++++++++++++----------------
> >  6 files changed, 50 insertions(+), 53 deletions(-)
> >  create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
> >
> >diff --git a/drivers/gpu/drm/tilcdc/Module.symvers b/drivers/gpu/drm/tilcdc/Module.symvers
> >new file mode 100644
> >index 0000000..e69de29
> >diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> >index 006a30e..2c860c4 100644
> >--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> >+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> >@@ -120,7 +120,6 @@ static int cpufreq_transition(struct notifier_block *nb,
> >  static int tilcdc_unload(struct drm_device *dev)
> >  {
> >  	struct tilcdc_drm_private *priv = dev->dev_private;
> >-	struct tilcdc_module *mod, *cur;
> >
> >  	drm_fbdev_cma_fini(priv->fbdev);
> >  	drm_kms_helper_poll_fini(dev);
> >@@ -149,11 +148,6 @@ static int tilcdc_unload(struct drm_device *dev)
> >
> >  	pm_runtime_disable(dev->dev);
> >
> >-	list_for_each_entry_safe(mod, cur, &module_list, list) {
> >-		DBG("destroying module: %s", mod->name);
> >-		mod->funcs->destroy(mod);
> >-	}
> >-
> >  	kfree(priv);
> >
> >  	return 0;
> >diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> >index 0938036..7596c14 100644
> >--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> >+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> >@@ -98,7 +98,6 @@ struct tilcdc_module;
> >  struct tilcdc_module_ops {
> >  	/* create appropriate encoders/connectors: */
> >  	int (*modeset_init)(struct tilcdc_module *mod, struct drm_device *dev);
> >-	void (*destroy)(struct tilcdc_module *mod);
> >  #ifdef CONFIG_DEBUG_FS
> >  	/* create debugfs nodes (can be NULL): */
> >  	int (*debugfs_init)(struct tilcdc_module *mod, struct drm_minor *minor);
> >diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> >index b085dcc..2f6efae 100644
> >--- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> >+++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> >@@ -282,21 +282,8 @@ static int panel_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
> >  	return 0;
> >  }
> >
> >-static void panel_destroy(struct tilcdc_module *mod)
> >-{
> >-	struct panel_module *panel_mod = to_panel_module(mod);
> >-
> >-	if (panel_mod->timings)
> >-		display_timings_release(panel_mod->timings);
> >-
> >-	tilcdc_module_cleanup(mod);
> >-	kfree(panel_mod->info);
> >-	kfree(panel_mod);
> >-}
> >-
> >  static const struct tilcdc_module_ops panel_module_ops = {
> >  		.modeset_init = panel_modeset_init,
> >-		.destroy = panel_destroy,
> >  };
> >
> >  /*
> >@@ -372,6 +359,7 @@ static int panel_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >
> >  	mod = &panel_mod->base;
> >+	pdev->dev.platform_data = mod;
> >
> >  	tilcdc_module_init(mod, "panel", &panel_module_ops);
> >
> >@@ -379,17 +367,16 @@ static int panel_probe(struct platform_device *pdev)
> >  	if (IS_ERR(pinctrl))
> >  		dev_warn(&pdev->dev, "pins are not configured\n");
> >
> >-
> >  	panel_mod->timings = of_get_display_timings(node);
> >  	if (!panel_mod->timings) {
> >  		dev_err(&pdev->dev, "could not get panel timings\n");
> >-		goto fail;
> >+		goto fail_free;
> >  	}
> >
> >  	panel_mod->info = of_get_panel_info(node);
> >  	if (!panel_mod->info) {
> >  		dev_err(&pdev->dev, "could not get panel info\n");
> >-		goto fail;
> >+		goto fail_timings;
> >  	}
> >
> >  	mod->preferred_bpp = panel_mod->info->bpp;
> >@@ -400,13 +387,26 @@ static int panel_probe(struct platform_device *pdev)
> >
> >  	return 0;
> >
> >-fail:
> >-	panel_destroy(mod);
> >+fail_timings:
> >+	display_timings_release(panel_mod->timings);
> >+
> >+fail_free:
> >+	kfree(panel_mod);
> >+	tilcdc_module_cleanup(mod);
> >  	return ret;
> >  }
> >
> >  static int panel_remove(struct platform_device *pdev)
> >  {
> >+	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
> >+	struct panel_module *panel_mod = to_panel_module(mod);
> >+
> >+	display_timings_release(panel_mod->timings);
> >+
> >+	tilcdc_module_cleanup(mod);
> >+	kfree(panel_mod->info);
> >+	kfree(panel_mod);
> >+
> >  	return 0;
> >  }
> >
> >diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
> >index 2f83ffb..1e568ca 100644
> >--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
> >+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
> >@@ -296,17 +296,8 @@ static int slave_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
> >  	return 0;
> >  }
> >
> >-static void slave_destroy(struct tilcdc_module *mod)
> >-{
> >-	struct slave_module *slave_mod = to_slave_module(mod);
> >-
> >-	tilcdc_module_cleanup(mod);
> >-	kfree(slave_mod);
> >-}
> >-
> >  static const struct tilcdc_module_ops slave_module_ops = {
> >  		.modeset_init = slave_modeset_init,
> >-		.destroy = slave_destroy,
> >  };
> >
> >  /*
> >@@ -356,10 +347,13 @@ static int slave_probe(struct platform_device *pdev)
> >  	}
> >
> >  	slave_mod = kzalloc(sizeof(*slave_mod), GFP_KERNEL);
> >-	if (!slave_mod)
> >-		return -ENOMEM;
> >+	if (!slave_mod) {
> >+		ret = -ENOMEM;
> >+		goto fail_adapter;
> >+	}
> >
> >  	mod = &slave_mod->base;
> >+	pdev->dev.platform_data = mod;
> >
> >  	mod->preferred_bpp = slave_info.bpp;
> >
> >@@ -374,10 +368,20 @@ static int slave_probe(struct platform_device *pdev)
> >  	tilcdc_slave_probedefer(false);
> >
> >  	return 0;
> >+
> >+fail_adapter:
> >+	i2c_put_adapter(slavei2c);
> >+	return ret;
> >  }
> >
> >  static int slave_remove(struct platform_device *pdev)
> >  {
> >+	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
> >+	struct slave_module *slave_mod = to_slave_module(mod);
> >+
> >+	tilcdc_module_cleanup(mod);
> >+	kfree(slave_mod);
> >+
> >  	return 0;
> >  }
> >
> >diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> >index ce75ac8..32a0d2d 100644
> >--- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> >+++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> >@@ -296,23 +296,8 @@ static int tfp410_modeset_init(struct tilcdc_module *mod, struct drm_device *dev
> >  	return 0;
> >  }
> >
> >-static void tfp410_destroy(struct tilcdc_module *mod)
> >-{
> >-	struct tfp410_module *tfp410_mod = to_tfp410_module(mod);
> >-
> >-	if (tfp410_mod->i2c)
> >-		i2c_put_adapter(tfp410_mod->i2c);
> >-
> >-	if (!IS_ERR_VALUE(tfp410_mod->gpio))
> >-		gpio_free(tfp410_mod->gpio);
> >-
> >-	tilcdc_module_cleanup(mod);
> >-	kfree(tfp410_mod);
> >-}
> >-
> >  static const struct tilcdc_module_ops tfp410_module_ops = {
> >  		.modeset_init = tfp410_modeset_init,
> >-		.destroy = tfp410_destroy,
> >  };
> >
> >  /*
> >@@ -342,6 +327,7 @@ static int tfp410_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >
> >  	mod = &tfp410_mod->base;
> >+	pdev->dev.platform_data = mod;
> >
> >  	tilcdc_module_init(mod, "tfp410", &tfp410_module_ops);
> >
> >@@ -365,6 +351,7 @@ static int tfp410_probe(struct platform_device *pdev)
> >  	tfp410_mod->i2c = of_find_i2c_adapter_by_node(i2c_node);
> >  	if (!tfp410_mod->i2c) {
> >  		dev_err(&pdev->dev, "could not get i2c\n");
> >+		of_node_put(i2c_node);
> >  		goto fail;
> >  	}
> >
> >@@ -378,19 +365,32 @@ static int tfp410_probe(struct platform_device *pdev)
> >  		ret = gpio_request(tfp410_mod->gpio, "DVI_PDn");
> >  		if (ret) {
> >  			dev_err(&pdev->dev, "could not get DVI_PDn gpio\n");
> >-			goto fail;
> >+			goto fail_adapter;
> >  		}
> >  	}
> >
> >  	return 0;
> >
> >+fail_adapter:
> >+	i2c_put_adapter(tfp410_mod->i2c);
> >+
> >  fail:
> >-	tfp410_destroy(mod);
> >+	kfree(tfp410_mod);
> >+	tilcdc_module_cleanup(mod);
> >  	return ret;
> >  }
> >
> >  static int tfp410_remove(struct platform_device *pdev)
> >  {
> >+	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
> >+	struct tfp410_module *tfp410_mod = to_tfp410_module(mod);
> >+
> >+	i2c_put_adapter(tfp410_mod->i2c);
> >+	gpio_free(tfp410_mod->gpio);
> >+
> >+	tilcdc_module_cleanup(mod);
> >+	kfree(tfp410_mod);
> >+
> >  	return 0;
> >  }
> >
> >

-- 
Guido Martínez, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RESEND 8/9] drm/tilcdc: remove submodule destroy calls

[Guido Martínez]

On Tue, Jun 24, 2014 at 05:06:24PM -0500, Darren Etheridge wrote:
> Guido,
> 
> On 06/17/2014 09:17 AM, Guido Mart?nez wrote:
> >The TI tilcdc driver is designed with a notion of submodules. Currently,
> >at unload time, these submodules are iterated and destroyed.
> >
> >Now that the tilcdc remove order is fixed, this can be handled perfectly
> 
> I am not sure I understand the first part of the above sentence - did
> something change with tilcdc ordering?  I think you a referring to previous
> patches in your series which really mean tilcdc can actually unload now.
Right, I guess I got a bit dizzy while writing that commit log :). If we
find the patch reasonable I'll send a better explanation.

> So really the method this patch uses could always have been used, it
> just wasn't for some reason?
Yes, I think so.

> I have tested all of the other patches in your series and all looks good on
> BeagleBone Black and AM335xEVM, I tested as both built-ins and modules and
> can load/unload on BeagleBone Black with HDMI enabled correctly.
Nice to hear that :)

> I want to play around a bit more with this particular patch, to try and
> understand how it differs from Rob's original intent with his module
> registration/deregistration scheme.  I prefer your method, but do we loose
> anything that Rob's originally had in mind?
Nothing really comes to mind, but I may be wrong on this...

Thanks a lot for your review!

> Darren
> 
> >by the kernel using the device infrastructure, since each submodule
> >is a kernel driver itself, and they are only destroy()'ed at unload
> >time. Therefore we move the destroy() functionality to each submodule's
> >remove().
> >
> >Also, remove some checks in the unloading process since the new code
> >guarantees the resources are allocated and need a release.
> >
> >Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
> >---
> >  drivers/gpu/drm/tilcdc/Module.symvers  |  0
> >  drivers/gpu/drm/tilcdc/tilcdc_drv.c    |  6 ------
> >  drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
> >  drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 36 +++++++++++++++++-----------------
> >  drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 26 +++++++++++++-----------
> >  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 34 ++++++++++++++++----------------
> >  6 files changed, 50 insertions(+), 53 deletions(-)
> >  create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
> >
> >diff --git a/drivers/gpu/drm/tilcdc/Module.symvers b/drivers/gpu/drm/tilcdc/Module.symvers
> >new file mode 100644
> >index 0000000..e69de29
> >diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> >index 006a30e..2c860c4 100644
> >--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> >+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> >@@ -120,7 +120,6 @@ static int cpufreq_transition(struct notifier_block *nb,
> >  static int tilcdc_unload(struct drm_device *dev)
> >  {
> >  	struct tilcdc_drm_private *priv = dev->dev_private;
> >-	struct tilcdc_module *mod, *cur;
> >
> >  	drm_fbdev_cma_fini(priv->fbdev);
> >  	drm_kms_helper_poll_fini(dev);
> >@@ -149,11 +148,6 @@ static int tilcdc_unload(struct drm_device *dev)
> >
> >  	pm_runtime_disable(dev->dev);
> >
> >-	list_for_each_entry_safe(mod, cur, &module_list, list) {
> >-		DBG("destroying module: %s", mod->name);
> >-		mod->funcs->destroy(mod);
> >-	}
> >-
> >  	kfree(priv);
> >
> >  	return 0;
> >diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> >index 0938036..7596c14 100644
> >--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> >+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> >@@ -98,7 +98,6 @@ struct tilcdc_module;
> >  struct tilcdc_module_ops {
> >  	/* create appropriate encoders/connectors: */
> >  	int (*modeset_init)(struct tilcdc_module *mod, struct drm_device *dev);
> >-	void (*destroy)(struct tilcdc_module *mod);
> >  #ifdef CONFIG_DEBUG_FS
> >  	/* create debugfs nodes (can be NULL): */
> >  	int (*debugfs_init)(struct tilcdc_module *mod, struct drm_minor *minor);
> >diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> >index b085dcc..2f6efae 100644
> >--- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> >+++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> >@@ -282,21 +282,8 @@ static int panel_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
> >  	return 0;
> >  }
> >
> >-static void panel_destroy(struct tilcdc_module *mod)
> >-{
> >-	struct panel_module *panel_mod = to_panel_module(mod);
> >-
> >-	if (panel_mod->timings)
> >-		display_timings_release(panel_mod->timings);
> >-
> >-	tilcdc_module_cleanup(mod);
> >-	kfree(panel_mod->info);
> >-	kfree(panel_mod);
> >-}
> >-
> >  static const struct tilcdc_module_ops panel_module_ops = {
> >  		.modeset_init = panel_modeset_init,
> >-		.destroy = panel_destroy,
> >  };
> >
> >  /*
> >@@ -372,6 +359,7 @@ static int panel_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >
> >  	mod = &panel_mod->base;
> >+	pdev->dev.platform_data = mod;
> >
> >  	tilcdc_module_init(mod, "panel", &panel_module_ops);
> >
> >@@ -379,17 +367,16 @@ static int panel_probe(struct platform_device *pdev)
> >  	if (IS_ERR(pinctrl))
> >  		dev_warn(&pdev->dev, "pins are not configured\n");
> >
> >-
> >  	panel_mod->timings = of_get_display_timings(node);
> >  	if (!panel_mod->timings) {
> >  		dev_err(&pdev->dev, "could not get panel timings\n");
> >-		goto fail;
> >+		goto fail_free;
> >  	}
> >
> >  	panel_mod->info = of_get_panel_info(node);
> >  	if (!panel_mod->info) {
> >  		dev_err(&pdev->dev, "could not get panel info\n");
> >-		goto fail;
> >+		goto fail_timings;
> >  	}
> >
> >  	mod->preferred_bpp = panel_mod->info->bpp;
> >@@ -400,13 +387,26 @@ static int panel_probe(struct platform_device *pdev)
> >
> >  	return 0;
> >
> >-fail:
> >-	panel_destroy(mod);
> >+fail_timings:
> >+	display_timings_release(panel_mod->timings);
> >+
> >+fail_free:
> >+	kfree(panel_mod);
> >+	tilcdc_module_cleanup(mod);
> >  	return ret;
> >  }
> >
> >  static int panel_remove(struct platform_device *pdev)
> >  {
> >+	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
> >+	struct panel_module *panel_mod = to_panel_module(mod);
> >+
> >+	display_timings_release(panel_mod->timings);
> >+
> >+	tilcdc_module_cleanup(mod);
> >+	kfree(panel_mod->info);
> >+	kfree(panel_mod);
> >+
> >  	return 0;
> >  }
> >
> >diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
> >index 2f83ffb..1e568ca 100644
> >--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
> >+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
> >@@ -296,17 +296,8 @@ static int slave_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
> >  	return 0;
> >  }
> >
> >-static void slave_destroy(struct tilcdc_module *mod)
> >-{
> >-	struct slave_module *slave_mod = to_slave_module(mod);
> >-
> >-	tilcdc_module_cleanup(mod);
> >-	kfree(slave_mod);
> >-}
> >-
> >  static const struct tilcdc_module_ops slave_module_ops = {
> >  		.modeset_init = slave_modeset_init,
> >-		.destroy = slave_destroy,
> >  };
> >
> >  /*
> >@@ -356,10 +347,13 @@ static int slave_probe(struct platform_device *pdev)
> >  	}
> >
> >  	slave_mod = kzalloc(sizeof(*slave_mod), GFP_KERNEL);
> >-	if (!slave_mod)
> >-		return -ENOMEM;
> >+	if (!slave_mod) {
> >+		ret = -ENOMEM;
> >+		goto fail_adapter;
> >+	}
> >
> >  	mod = &slave_mod->base;
> >+	pdev->dev.platform_data = mod;
> >
> >  	mod->preferred_bpp = slave_info.bpp;
> >
> >@@ -374,10 +368,20 @@ static int slave_probe(struct platform_device *pdev)
> >  	tilcdc_slave_probedefer(false);
> >
> >  	return 0;
> >+
> >+fail_adapter:
> >+	i2c_put_adapter(slavei2c);
> >+	return ret;
> >  }
> >
> >  static int slave_remove(struct platform_device *pdev)
> >  {
> >+	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
> >+	struct slave_module *slave_mod = to_slave_module(mod);
> >+
> >+	tilcdc_module_cleanup(mod);
> >+	kfree(slave_mod);
> >+
> >  	return 0;
> >  }
> >
> >diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> >index ce75ac8..32a0d2d 100644
> >--- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> >+++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> >@@ -296,23 +296,8 @@ static int tfp410_modeset_init(struct tilcdc_module *mod, struct drm_device *dev
> >  	return 0;
> >  }
> >
> >-static void tfp410_destroy(struct tilcdc_module *mod)
> >-{
> >-	struct tfp410_module *tfp410_mod = to_tfp410_module(mod);
> >-
> >-	if (tfp410_mod->i2c)
> >-		i2c_put_adapter(tfp410_mod->i2c);
> >-
> >-	if (!IS_ERR_VALUE(tfp410_mod->gpio))
> >-		gpio_free(tfp410_mod->gpio);
> >-
> >-	tilcdc_module_cleanup(mod);
> >-	kfree(tfp410_mod);
> >-}
> >-
> >  static const struct tilcdc_module_ops tfp410_module_ops = {
> >  		.modeset_init = tfp410_modeset_init,
> >-		.destroy = tfp410_destroy,
> >  };
> >
> >  /*
> >@@ -342,6 +327,7 @@ static int tfp410_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >
> >  	mod = &tfp410_mod->base;
> >+	pdev->dev.platform_data = mod;
> >
> >  	tilcdc_module_init(mod, "tfp410", &tfp410_module_ops);
> >
> >@@ -365,6 +351,7 @@ static int tfp410_probe(struct platform_device *pdev)
> >  	tfp410_mod->i2c = of_find_i2c_adapter_by_node(i2c_node);
> >  	if (!tfp410_mod->i2c) {
> >  		dev_err(&pdev->dev, "could not get i2c\n");
> >+		of_node_put(i2c_node);
> >  		goto fail;
> >  	}
> >
> >@@ -378,19 +365,32 @@ static int tfp410_probe(struct platform_device *pdev)
> >  		ret = gpio_request(tfp410_mod->gpio, "DVI_PDn");
> >  		if (ret) {
> >  			dev_err(&pdev->dev, "could not get DVI_PDn gpio\n");
> >-			goto fail;
> >+			goto fail_adapter;
> >  		}
> >  	}
> >
> >  	return 0;
> >
> >+fail_adapter:
> >+	i2c_put_adapter(tfp410_mod->i2c);
> >+
> >  fail:
> >-	tfp410_destroy(mod);
> >+	kfree(tfp410_mod);
> >+	tilcdc_module_cleanup(mod);
> >  	return ret;
> >  }
> >
> >  static int tfp410_remove(struct platform_device *pdev)
> >  {
> >+	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
> >+	struct tfp410_module *tfp410_mod = to_tfp410_module(mod);
> >+
> >+	i2c_put_adapter(tfp410_mod->i2c);
> >+	gpio_free(tfp410_mod->gpio);
> >+
> >+	tilcdc_module_cleanup(mod);
> >+	kfree(tfp410_mod);
> >+
> >  	return 0;
> >  }
> >
> >

-- 
Guido Mart?nez, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH/RESEND 1/9] drm/i2c: tda998x: move drm_i2c_encoder_destroy call

[Guido Martínez]

Hi Russell,

On Tue, Jun 24, 2014 at 05:38:13PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 17, 2014 at 11:17:03AM -0300, Guido Martínez wrote:
> > Currently tda998x_encoder_destroy() calls cec_write() and reg_clear(),
> > as part of the release procedure. Such calls need to access the I2C bus
> > and therefore, we need to call them before drm_i2c_encoder_destroy()
> > which unregisters the I2C device.
> > 
> > This commit moves the latter so it's done afterwards.
> > 
> > Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
> > Signed-off-by: Ezequiel García <ezequiel@vanguardiasur.com.ar>
> > Cc: <stable@vger.kernel.org> #v3.9+
> 
> You really should have sent this with me in the To: header as I'm now the
> maintainer of this driver.  Yes, this is a valid fix, and I'll apply it
> shortly.  Thanks.
Sorry about that, I'm still kind of new to this whole deal. I'll keep it
mind for future patches.

Thanks,
Guido


> > ---
> >  drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> > index 240c331..db9515f 100644
> > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > @@ -1183,7 +1183,6 @@ static void
> >  tda998x_encoder_destroy(struct drm_encoder *encoder)
> >  {
> >  	struct tda998x_priv *priv = to_tda998x_priv(encoder);
> > -	drm_i2c_encoder_destroy(encoder);
> >  
> >  	/* disable all IRQs and free the IRQ handler */
> >  	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
> > @@ -1193,6 +1192,7 @@ tda998x_encoder_destroy(struct drm_encoder *encoder)
> >  
> >  	if (priv->cec)
> >  		i2c_unregister_device(priv->cec);
> > +	drm_i2c_encoder_destroy(encoder);
> >  	kfree(priv);
> >  }
> >  
> > -- 
> > 2.0.0
> > 
> 
> -- 
> FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
> improving, and getting towards what was expected from it.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Guido Martínez, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH/RESEND 1/9] drm/i2c: tda998x: move drm_i2c_encoder_destroy call

[Guido Martínez]

Hi Russell,

On Tue, Jun 24, 2014 at 05:38:13PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 17, 2014 at 11:17:03AM -0300, Guido Mart?nez wrote:
> > Currently tda998x_encoder_destroy() calls cec_write() and reg_clear(),
> > as part of the release procedure. Such calls need to access the I2C bus
> > and therefore, we need to call them before drm_i2c_encoder_destroy()
> > which unregisters the I2C device.
> > 
> > This commit moves the latter so it's done afterwards.
> > 
> > Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
> > Signed-off-by: Ezequiel Garc?a <ezequiel@vanguardiasur.com.ar>
> > Cc: <stable@vger.kernel.org> #v3.9+
> 
> You really should have sent this with me in the To: header as I'm now the
> maintainer of this driver.  Yes, this is a valid fix, and I'll apply it
> shortly.  Thanks.
Sorry about that, I'm still kind of new to this whole deal. I'll keep it
mind for future patches.

Thanks,
Guido


> > ---
> >  drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> > index 240c331..db9515f 100644
> > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > @@ -1183,7 +1183,6 @@ static void
> >  tda998x_encoder_destroy(struct drm_encoder *encoder)
> >  {
> >  	struct tda998x_priv *priv = to_tda998x_priv(encoder);
> > -	drm_i2c_encoder_destroy(encoder);
> >  
> >  	/* disable all IRQs and free the IRQ handler */
> >  	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
> > @@ -1193,6 +1192,7 @@ tda998x_encoder_destroy(struct drm_encoder *encoder)
> >  
> >  	if (priv->cec)
> >  		i2c_unregister_device(priv->cec);
> > +	drm_i2c_encoder_destroy(encoder);
> >  	kfree(priv);
> >  }
> >  
> > -- 
> > 2.0.0
> > 
> 
> -- 
> FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
> improving, and getting towards what was expected from it.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Guido Mart?nez, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH/RESEND 9/9] drm/tilcdc: replace late_initcall with module_init

[Russell King - ARM Linux]

On Tue, Jun 24, 2014 at 05:04:36PM -0500, Darren Etheridge wrote:
> On 06/17/2014 09:17 AM, Guido Martínez wrote:
>> Use module_init instead of late_initcall, as is the norm for modular
>> drivers.
>>
>> module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b
>> ("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall,
>> but it does not explain why. Tests show it's working properly with
>> module_init.
>>
>
> If I recall, the late_initcall stuff was done to try and make sure the  
> tda998x/i2c subsystem came up before tilcdc. However it didn't always  
> work so we added commit: 39de6194131c155901f96686a063212656d80c2e to try  
> and ensure the ordering.  This might be why changing back to module_init  
> is fine (and I agree with your assessment from my testing).

There's a solution to that... I have patches which convert the tda998x
driver to also register into the component helpers as well as remaining
as a drm slave device.  This makes it possible to transition tilcdc to
use the component helper to solve the initialisation ordering.

I'll (re-)post them later today.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH/RESEND 9/9] drm/tilcdc: replace late_initcall with module_init

[Russell King - ARM Linux]

On Tue, Jun 24, 2014 at 05:04:36PM -0500, Darren Etheridge wrote:
> On 06/17/2014 09:17 AM, Guido Mart?nez wrote:
>> Use module_init instead of late_initcall, as is the norm for modular
>> drivers.
>>
>> module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b
>> ("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall,
>> but it does not explain why. Tests show it's working properly with
>> module_init.
>>
>
> If I recall, the late_initcall stuff was done to try and make sure the  
> tda998x/i2c subsystem came up before tilcdc. However it didn't always  
> work so we added commit: 39de6194131c155901f96686a063212656d80c2e to try  
> and ensure the ordering.  This might be why changing back to module_init  
> is fine (and I agree with your assessment from my testing).

There's a solution to that... I have patches which convert the tda998x
driver to also register into the component helpers as well as remaining
as a drm slave device.  This makes it possible to transition tilcdc to
use the component helper to solve the initialisation ordering.

I'll (re-)post them later today.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH/RESEND 9/9] drm/tilcdc: replace late_initcall with module_init

[Russell King - ARM Linux]

On Wed, Jun 25, 2014 at 02:00:42PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 24, 2014 at 05:04:36PM -0500, Darren Etheridge wrote:
> > On 06/17/2014 09:17 AM, Guido Martínez wrote:
> >> Use module_init instead of late_initcall, as is the norm for modular
> >> drivers.
> >>
> >> module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b
> >> ("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall,
> >> but it does not explain why. Tests show it's working properly with
> >> module_init.
> >>
> >
> > If I recall, the late_initcall stuff was done to try and make sure the  
> > tda998x/i2c subsystem came up before tilcdc. However it didn't always  
> > work so we added commit: 39de6194131c155901f96686a063212656d80c2e to try  
> > and ensure the ordering.  This might be why changing back to module_init  
> > is fine (and I agree with your assessment from my testing).
> 
> There's a solution to that... I have patches which convert the tda998x
> driver to also register into the component helpers as well as remaining
> as a drm slave device.  This makes it possible to transition tilcdc to
> use the component helper to solve the initialisation ordering.
> 
> I'll (re-)post them later today.

Except Daniel Mack will not be receiving any copies of them:

  zonque@gmail.com
    SMTP error from remote mail server after end of data:
    host gmail-smtp-in.l.google.com [2a00:1450:400c:c03::1a]:
    550-5.7.1 [2001:4d48:ad52:3201:214:fdff:fe10:1be6      12] Our system has
    550-5.7.1 detected that this message is likely unsolicited mail. To reduce the
    550-5.7.1 amount of spam sent to Gmail, this message has been blocked. Please
    550-5.7.1 visit
    550-5.7.1 http://support.google.com/mail/bin/answer.py?hl=en&answer=188131 for
    550 5.7.1 more information. fs8si6627678wib.99 - gsmtp

Google's anti-ham filter seems to be working perfectly, allowing spam
through and blocking real messages... as usual.  And as usual, google
provides no support for this crap.  Gmail has a history of increasingly
blocking legitimate email in their alleged anti-spam fight.  Don't use
gmail.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH/RESEND 9/9] drm/tilcdc: replace late_initcall with module_init

[Russell King - ARM Linux]

On Wed, Jun 25, 2014 at 02:00:42PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 24, 2014 at 05:04:36PM -0500, Darren Etheridge wrote:
> > On 06/17/2014 09:17 AM, Guido Mart?nez wrote:
> >> Use module_init instead of late_initcall, as is the norm for modular
> >> drivers.
> >>
> >> module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b
> >> ("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall,
> >> but it does not explain why. Tests show it's working properly with
> >> module_init.
> >>
> >
> > If I recall, the late_initcall stuff was done to try and make sure the  
> > tda998x/i2c subsystem came up before tilcdc. However it didn't always  
> > work so we added commit: 39de6194131c155901f96686a063212656d80c2e to try  
> > and ensure the ordering.  This might be why changing back to module_init  
> > is fine (and I agree with your assessment from my testing).
> 
> There's a solution to that... I have patches which convert the tda998x
> driver to also register into the component helpers as well as remaining
> as a drm slave device.  This makes it possible to transition tilcdc to
> use the component helper to solve the initialisation ordering.
> 
> I'll (re-)post them later today.

Except Daniel Mack will not be receiving any copies of them:

  zonque at gmail.com
    SMTP error from remote mail server after end of data:
    host gmail-smtp-in.l.google.com [2a00:1450:400c:c03::1a]:
    550-5.7.1 [2001:4d48:ad52:3201:214:fdff:fe10:1be6      12] Our system has
    550-5.7.1 detected that this message is likely unsolicited mail. To reduce the
    550-5.7.1 amount of spam sent to Gmail, this message has been blocked. Please
    550-5.7.1 visit
    550-5.7.1 http://support.google.com/mail/bin/answer.py?hl=en&answer=188131 for
    550 5.7.1 more information. fs8si6627678wib.99 - gsmtp

Google's anti-ham filter seems to be working perfectly, allowing spam
through and blocking real messages... as usual.  And as usual, google
provides no support for this crap.  Gmail has a history of increasingly
blocking legitimate email in their alleged anti-spam fight.  Don't use
gmail.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH/RESEND 9/9] drm/tilcdc: replace late_initcall with module_init

[Ezequiel García]

(Ccing Guido back)

Hello Russell, Darren,

On 25 Jun 02:00 PM, Russell King - ARM Linux wrote:
> On Tue, Jun 24, 2014 at 05:04:36PM -0500, Darren Etheridge wrote:
> > On 06/17/2014 09:17 AM, Guido Martínez wrote:
> >> Use module_init instead of late_initcall, as is the norm for modular
> >> drivers.
> >>
> >> module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b
> >> ("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall,
> >> but it does not explain why. Tests show it's working properly with
> >> module_init.
> >>
> >
> > If I recall, the late_initcall stuff was done to try and make sure the  
> > tda998x/i2c subsystem came up before tilcdc.

That doesn't make any sense. Using late_initcall for the tilcdc DRM
driver would make the tilcdc DRM get probed before any other regular
module_init driver, including the tda998x encoder.

> > However it didn't always  
> > work so we added commit: 39de6194131c155901f96686a063212656d80c2e to try  
> > and ensure the ordering.  This might be why changing back to module_init  
> > is fine (and I agree with your assessment from my testing).

That commit is adds a proper probe deferal mechanism in case the
i2c is not available. OMAP is special because it has its own nasty initcall
to probe the i2c busses. However, that should be fixed and then i2c would be
always probed *after* the DRM, as per the ordering in drivers/Makefile.

> 
> There's a solution to that...

A solution to *what* ?

> I have patches which convert the tda998x
> driver to also register into the component helpers as well as remaining
> as a drm slave device.  This makes it possible to transition tilcdc to
> use the component helper to solve the initialisation ordering.
> 

AFAIK, there's no issue with the initialisation ordering, except that the
DRM driver is currently probed before the TDA998x encoder and so the probe
is defered. Unless I'm missing something that's very easy to fix, you just
need to change the link order to guarantee that i2c encoders are probed
*before* the DRM drivers that require them.

I have just posted a very simple patch to fix it, but it seems Thierry wasn't
happy with it, not sure why yet [1].

[1] http://www.spinics.net/lists/dri-devel/msg61987.html
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RESEND 9/9] drm/tilcdc: replace late_initcall with module_init

[Ezequiel García]

(Ccing Guido back)

Hello Russell, Darren,

On 25 Jun 02:00 PM, Russell King - ARM Linux wrote:
> On Tue, Jun 24, 2014 at 05:04:36PM -0500, Darren Etheridge wrote:
> > On 06/17/2014 09:17 AM, Guido Mart?nez wrote:
> >> Use module_init instead of late_initcall, as is the norm for modular
> >> drivers.
> >>
> >> module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b
> >> ("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall,
> >> but it does not explain why. Tests show it's working properly with
> >> module_init.
> >>
> >
> > If I recall, the late_initcall stuff was done to try and make sure the  
> > tda998x/i2c subsystem came up before tilcdc.

That doesn't make any sense. Using late_initcall for the tilcdc DRM
driver would make the tilcdc DRM get probed before any other regular
module_init driver, including the tda998x encoder.

> > However it didn't always  
> > work so we added commit: 39de6194131c155901f96686a063212656d80c2e to try  
> > and ensure the ordering.  This might be why changing back to module_init  
> > is fine (and I agree with your assessment from my testing).

That commit is adds a proper probe deferal mechanism in case the
i2c is not available. OMAP is special because it has its own nasty initcall
to probe the i2c busses. However, that should be fixed and then i2c would be
always probed *after* the DRM, as per the ordering in drivers/Makefile.

> 
> There's a solution to that...

A solution to *what* ?

> I have patches which convert the tda998x
> driver to also register into the component helpers as well as remaining
> as a drm slave device.  This makes it possible to transition tilcdc to
> use the component helper to solve the initialisation ordering.
> 

AFAIK, there's no issue with the initialisation ordering, except that the
DRM driver is currently probed before the TDA998x encoder and so the probe
is defered. Unless I'm missing something that's very easy to fix, you just
need to change the link order to guarantee that i2c encoders are probed
*before* the DRM drivers that require them.

I have just posted a very simple patch to fix it, but it seems Thierry wasn't
happy with it, not sure why yet [1].

[1] http://www.spinics.net/lists/dri-devel/msg61987.html
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH/RESEND 9/9] drm/tilcdc: replace late_initcall with module_init

[Russell King - ARM Linux]

On Wed, Jun 25, 2014 at 11:32:46AM -0300, Ezequiel García wrote:
> (Ccing Guido back)
> 
> Hello Russell, Darren,
> 
> On 25 Jun 02:00 PM, Russell King - ARM Linux wrote:
> > On Tue, Jun 24, 2014 at 05:04:36PM -0500, Darren Etheridge wrote:
> > > If I recall, the late_initcall stuff was done to try and make sure the  
> > > tda998x/i2c subsystem came up before tilcdc.
> 
> That doesn't make any sense. Using late_initcall for the tilcdc DRM
> driver would make the tilcdc DRM get probed before any other regular
> module_init driver, including the tda998x encoder.

A module_init() is a device_initcall(), which is at level 6.  A
late_initcall() is at level 7.  Level 6 initcalls are run before level
7 initcalls.  The tda998x is a module_init(), so the tda998x gets
initialised *before* tilcdc's late_initcall().

Now, if you build everything as a module, then you have no initialisation
ordering, and you can't rely on any kind of order.  Initialisation
functions can even run in parallel on different CPUs due to modules being
loaded from userspace in a multi-threaded way.  So anything which relies
on a certain initcall ordering is fundamentally broken if it can be
modular.

> > There's a solution to that...
> 
> A solution to *what* ?

Maybe if you left the context above that line in place, you might
understand that my "that" refers to what was discussed in that
context.  That being the initialisation ordering.

Convention and proper Internet etiquette is to trim the quoted text
and place relies below the paragraph to which they refer, the quoted
paragraph giving the context to the reply.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH/RESEND 9/9] drm/tilcdc: replace late_initcall with module_init

[Russell King - ARM Linux]

On Wed, Jun 25, 2014 at 11:32:46AM -0300, Ezequiel Garc?a wrote:
> (Ccing Guido back)
> 
> Hello Russell, Darren,
> 
> On 25 Jun 02:00 PM, Russell King - ARM Linux wrote:
> > On Tue, Jun 24, 2014 at 05:04:36PM -0500, Darren Etheridge wrote:
> > > If I recall, the late_initcall stuff was done to try and make sure the  
> > > tda998x/i2c subsystem came up before tilcdc.
> 
> That doesn't make any sense. Using late_initcall for the tilcdc DRM
> driver would make the tilcdc DRM get probed before any other regular
> module_init driver, including the tda998x encoder.

A module_init() is a device_initcall(), which is at level 6.  A
late_initcall() is at level 7.  Level 6 initcalls are run before level
7 initcalls.  The tda998x is a module_init(), so the tda998x gets
initialised *before* tilcdc's late_initcall().

Now, if you build everything as a module, then you have no initialisation
ordering, and you can't rely on any kind of order.  Initialisation
functions can even run in parallel on different CPUs due to modules being
loaded from userspace in a multi-threaded way.  So anything which relies
on a certain initcall ordering is fundamentally broken if it can be
modular.

> > There's a solution to that...
> 
> A solution to *what* ?

Maybe if you left the context above that line in place, you might
understand that my "that" refers to what was discussed in that
context.  That being the initialisation ordering.

Convention and proper Internet etiquette is to trim the quoted text
and place relies below the paragraph to which they refer, the quoted
paragraph giving the context to the reply.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH/RESEND 8/9] drm/tilcdc: remove submodule destroy calls

[Ezequiel García]

On 24 Jun 05:06 PM, Darren Etheridge wrote:
> 
> On 06/17/2014 09:17 AM, Guido Martínez wrote:
> >The TI tilcdc driver is designed with a notion of submodules. Currently,
> >at unload time, these submodules are iterated and destroyed.
> >
> >Now that the tilcdc remove order is fixed, this can be handled perfectly
> 
> I am not sure I understand the first part of the above sentence - did
> something change with tilcdc ordering?

Yes, patch [PATCH/RESEND 6/9] drm/tilcdc: fix release order on exit changes
the tilcdc remove ordering. 

Currently, the tilcdc DRM is removed with this:

        tilcdc_tfp410_fini();
        tilcdc_slave_fini();
        tilcdc_panel_fini();
        platform_driver_unregister(&tilcdc_platform_driver);

Which is wrong as you shouldn't remove the tilcdc "modules" (panel, slave,
tfp410) before the DRM driver itself. So the above patch fixed it to be:

        platform_driver_unregister(&tilcdc_platform_driver);
        tilcdc_panel_fini();
        tilcdc_slave_fini();
        tilcdc_tfp410_fini();

> I think you a referring to previous
> patches in your series which really mean tilcdc can actually unload now.  So
> really the method this patch uses could always have been used, it just
> wasn't for some reason?
> 

No, I believe this patch which removes the tilcdc sub-module destroy
infrastructure can only be applied *after* the above remove order is fixed
(iow, the 6/9 patch mentioned above).

In other words, only once you have a proper remove() that releases things
in the right order you can rely in the kernel and avoid any custom
implementation.
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RESEND 8/9] drm/tilcdc: remove submodule destroy calls

[Ezequiel García]

On 24 Jun 05:06 PM, Darren Etheridge wrote:
> 
> On 06/17/2014 09:17 AM, Guido Mart?nez wrote:
> >The TI tilcdc driver is designed with a notion of submodules. Currently,
> >at unload time, these submodules are iterated and destroyed.
> >
> >Now that the tilcdc remove order is fixed, this can be handled perfectly
> 
> I am not sure I understand the first part of the above sentence - did
> something change with tilcdc ordering?

Yes, patch [PATCH/RESEND 6/9] drm/tilcdc: fix release order on exit changes
the tilcdc remove ordering. 

Currently, the tilcdc DRM is removed with this:

        tilcdc_tfp410_fini();
        tilcdc_slave_fini();
        tilcdc_panel_fini();
        platform_driver_unregister(&tilcdc_platform_driver);

Which is wrong as you shouldn't remove the tilcdc "modules" (panel, slave,
tfp410) before the DRM driver itself. So the above patch fixed it to be:

        platform_driver_unregister(&tilcdc_platform_driver);
        tilcdc_panel_fini();
        tilcdc_slave_fini();
        tilcdc_tfp410_fini();

> I think you a referring to previous
> patches in your series which really mean tilcdc can actually unload now.  So
> really the method this patch uses could always have been used, it just
> wasn't for some reason?
> 

No, I believe this patch which removes the tilcdc sub-module destroy
infrastructure can only be applied *after* the above remove order is fixed
(iow, the 6/9 patch mentioned above).

In other words, only once you have a proper remove() that releases things
in the right order you can rely in the kernel and avoid any custom
implementation.
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH/RESEND 9/9] drm/tilcdc: replace late_initcall with module_init

[Ezequiel García]

Hi Russell,

On 25 Jun 03:46 PM, Russell King - ARM Linux wrote:
> > 
> > That doesn't make any sense. Using late_initcall for the tilcdc DRM
> > driver would make the tilcdc DRM get probed before any other regular
> > module_init driver, including the tda998x encoder.
> 
> A module_init() is a device_initcall(), which is at level 6.  A
> late_initcall() is at level 7.  Level 6 initcalls are run before level
> 7 initcalls.  The tda998x is a module_init(), so the tda998x gets
> initialised *before* tilcdc's late_initcall().
> 

Oh, thanks a lot for correcting me.

> Now, if you build everything as a module, then you have no initialisation
> ordering, and you can't rely on any kind of order.  Initialisation
> functions can even run in parallel on different CPUs due to modules being
> loaded from userspace in a multi-threaded way.  So anything which relies
> on a certain initcall ordering is fundamentally broken if it can be
> modular.
> 

OK, but is there any outstanding issue with the tilcdc initialization, either
when compiled as built-in or as modules, *after* this patchset?

Of course, this driver only worked as built-in (see the horrible bugs we are
fixing here, that prevented unloading and re-loading it). This series fixes
all such bugs, and also adds the required changes to allow to build everything
as a module.

Again, I can't see any issues that remain to be fixed after this patchset.
When compiled as built-in, the tilcdc DRM will be defered and once the tda998x
is ready, the tilcdc DRM will load properly.

When compiled as module you can load any of them in any order. If the tda998x
module is not loaded by the time you load the tilcdc module, the former will
get loaded.

Hm... now that I think about this. We still get into trouble if the tda998x
is built as module, but the tilcdc is built-in. Shouldn't we just forbid that?
Either both built-ins or both as modules?

> > > There's a solution to that...
> > 
> > A solution to *what* ?
> 
> Maybe if you left the context above that line in place, you might
> understand that my "that" refers to what was discussed in that
> context.  That being the initialisation ordering.
> 
> Convention and proper Internet etiquette is to trim the quoted text
> and place relies below the paragraph to which they refer, the quoted
> paragraph giving the context to the reply.
> 

Yeah, sorry about that.
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH/RESEND 9/9] drm/tilcdc: replace late_initcall with module_init

[Ezequiel García]

Hi Russell,

On 25 Jun 03:46 PM, Russell King - ARM Linux wrote:
> > 
> > That doesn't make any sense. Using late_initcall for the tilcdc DRM
> > driver would make the tilcdc DRM get probed before any other regular
> > module_init driver, including the tda998x encoder.
> 
> A module_init() is a device_initcall(), which is at level 6.  A
> late_initcall() is at level 7.  Level 6 initcalls are run before level
> 7 initcalls.  The tda998x is a module_init(), so the tda998x gets
> initialised *before* tilcdc's late_initcall().
> 

Oh, thanks a lot for correcting me.

> Now, if you build everything as a module, then you have no initialisation
> ordering, and you can't rely on any kind of order.  Initialisation
> functions can even run in parallel on different CPUs due to modules being
> loaded from userspace in a multi-threaded way.  So anything which relies
> on a certain initcall ordering is fundamentally broken if it can be
> modular.
> 

OK, but is there any outstanding issue with the tilcdc initialization, either
when compiled as built-in or as modules, *after* this patchset?

Of course, this driver only worked as built-in (see the horrible bugs we are
fixing here, that prevented unloading and re-loading it). This series fixes
all such bugs, and also adds the required changes to allow to build everything
as a module.

Again, I can't see any issues that remain to be fixed after this patchset.
When compiled as built-in, the tilcdc DRM will be defered and once the tda998x
is ready, the tilcdc DRM will load properly.

When compiled as module you can load any of them in any order. If the tda998x
module is not loaded by the time you load the tilcdc module, the former will
get loaded.

Hm... now that I think about this. We still get into trouble if the tda998x
is built as module, but the tilcdc is built-in. Shouldn't we just forbid that?
Either both built-ins or both as modules?

> > > There's a solution to that...
> > 
> > A solution to *what* ?
> 
> Maybe if you left the context above that line in place, you might
> understand that my "that" refers to what was discussed in that
> context.  That being the initialisation ordering.
> 
> Convention and proper Internet etiquette is to trim the quoted text
> and place relies below the paragraph to which they refer, the quoted
> paragraph giving the context to the reply.
> 

Yeah, sorry about that.
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Darren Etheridge]

Guido,

On 06/17/2014 09:17 AM, Guido Martínez wrote:
> The tilcdc driver could be compiled as a module, but was severely broken
> and could not be used as such. This patchset attempts to fix the issues
> preventing a proper load/unload of the module.
>
> Issues included dangling sysfs nodes, dangling devices, memory leaks and
> a double kfree.
>
> It now seems to be working ok. We have tested this by loading and
> unloading the driver repeteadly, with both panel and slave connectors
> and found no flaws.
>
> There is still one warning left on tilcdc_crtc_destroy, caused by
> destroying the connector while still in an ON status. We don't know why
> this happens or why it's an issue, so we did not fix it.
>

Yes I see what you mean, it triggers the WARN_ON in tilcdc_crtc_destroy 
because DRM_MODE_DPMS_ON is still set.  This WARN_ON does make some 
sense because DPMS_OFF would have the effect of turning off clocks and 
putting the monitor to sleep which seems logical considering we have 
torn down the display.  Adding a tilcdc_crtc_dpms(DPMS_OFF) right before 
the WARN_ON confirms this, but it seems strange that this hasn't 
happened automatically (+ Russell doesn't need to do it in his Armada 
driver) - so I suspect there is a better way.

Otherwise I think this is a good and useful patch series.

Darren

> The first 7 patches are bug fixes which and should probably be applied
> in the stable trees. The last two are clean-ups.
>
>
> Resending this since I got no replies.
>
>
> Guido Martínez (9):
>    drm/i2c: tda998x: move drm_i2c_encoder_destroy call
>    drm/tilcdc: panel: fix dangling sysfs connector node
>    drm/tilcdc: slave: fix dangling sysfs connector node
>    drm/tilcdc: tfp410: fix dangling sysfs connector node
>    drm/tilcdc: panel: fix leak when unloading the module
>    drm/tilcdc: fix release order on exit
>    drm/tilcdc: fix double kfree
>    drm/tilcdc: remove submodule destroy calls
>    drm/tilcdc: replace late_initcall with module_init
>
>   drivers/gpu/drm/i2c/tda998x_drv.c      |  2 +-
>   drivers/gpu/drm/tilcdc/Module.symvers  |  0
>   drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 15 +++++--------
>   drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
>   drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 39 +++++++++++++++++-----------------
>   drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 27 +++++++++++++----------
>   drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35 +++++++++++++++---------------
>   7 files changed, 59 insertions(+), 60 deletions(-)
>   create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Darren Etheridge]

Guido,

On 06/17/2014 09:17 AM, Guido Mart?nez wrote:
> The tilcdc driver could be compiled as a module, but was severely broken
> and could not be used as such. This patchset attempts to fix the issues
> preventing a proper load/unload of the module.
>
> Issues included dangling sysfs nodes, dangling devices, memory leaks and
> a double kfree.
>
> It now seems to be working ok. We have tested this by loading and
> unloading the driver repeteadly, with both panel and slave connectors
> and found no flaws.
>
> There is still one warning left on tilcdc_crtc_destroy, caused by
> destroying the connector while still in an ON status. We don't know why
> this happens or why it's an issue, so we did not fix it.
>

Yes I see what you mean, it triggers the WARN_ON in tilcdc_crtc_destroy 
because DRM_MODE_DPMS_ON is still set.  This WARN_ON does make some 
sense because DPMS_OFF would have the effect of turning off clocks and 
putting the monitor to sleep which seems logical considering we have 
torn down the display.  Adding a tilcdc_crtc_dpms(DPMS_OFF) right before 
the WARN_ON confirms this, but it seems strange that this hasn't 
happened automatically (+ Russell doesn't need to do it in his Armada 
driver) - so I suspect there is a better way.

Otherwise I think this is a good and useful patch series.

Darren

> The first 7 patches are bug fixes which and should probably be applied
> in the stable trees. The last two are clean-ups.
>
>
> Resending this since I got no replies.
>
>
> Guido Mart?nez (9):
>    drm/i2c: tda998x: move drm_i2c_encoder_destroy call
>    drm/tilcdc: panel: fix dangling sysfs connector node
>    drm/tilcdc: slave: fix dangling sysfs connector node
>    drm/tilcdc: tfp410: fix dangling sysfs connector node
>    drm/tilcdc: panel: fix leak when unloading the module
>    drm/tilcdc: fix release order on exit
>    drm/tilcdc: fix double kfree
>    drm/tilcdc: remove submodule destroy calls
>    drm/tilcdc: replace late_initcall with module_init
>
>   drivers/gpu/drm/i2c/tda998x_drv.c      |  2 +-
>   drivers/gpu/drm/tilcdc/Module.symvers  |  0
>   drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 15 +++++--------
>   drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
>   drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 39 +++++++++++++++++-----------------
>   drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 27 +++++++++++++----------
>   drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35 +++++++++++++++---------------
>   7 files changed, 59 insertions(+), 60 deletions(-)
>   create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
>

Re: [PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Rob Clark]

On Fri, Jun 27, 2014 at 6:08 PM, Darren Etheridge <detheridge@ti.com> wrote:
> Guido,
>
>
> On 06/17/2014 09:17 AM, Guido Martínez wrote:
>>
>> The tilcdc driver could be compiled as a module, but was severely broken
>> and could not be used as such. This patchset attempts to fix the issues
>> preventing a proper load/unload of the module.
>>
>> Issues included dangling sysfs nodes, dangling devices, memory leaks and
>> a double kfree.
>>
>> It now seems to be working ok. We have tested this by loading and
>> unloading the driver repeteadly, with both panel and slave connectors
>> and found no flaws.
>>
>> There is still one warning left on tilcdc_crtc_destroy, caused by
>> destroying the connector while still in an ON status. We don't know why
>> this happens or why it's an issue, so we did not fix it.
>>
>
> Yes I see what you mean, it triggers the WARN_ON in tilcdc_crtc_destroy
> because DRM_MODE_DPMS_ON is still set.  This WARN_ON does make some sense
> because DPMS_OFF would have the effect of turning off clocks and putting the
> monitor to sleep which seems logical considering we have torn down the
> display.  Adding a tilcdc_crtc_dpms(DPMS_OFF) right before the WARN_ON
> confirms this, but it seems strange that this hasn't happened automatically
> (+ Russell doesn't need to do it in his Armada driver) - so I suspect there
> is a better way.

tbh, I'm not entirely sure offhand why drm_mode_config_cleanup()
doesn't remove the fb's first (which should have the effect of
shutting down any lit crtc/encoder/connector)..  that would seem like
the sensible way to shut down..

BR,
-R

> Otherwise I think this is a good and useful patch series.
>
> Darren
>
>
>> The first 7 patches are bug fixes which and should probably be applied
>> in the stable trees. The last two are clean-ups.
>>
>>
>> Resending this since I got no replies.
>>
>>
>> Guido Martínez (9):
>>    drm/i2c: tda998x: move drm_i2c_encoder_destroy call
>>    drm/tilcdc: panel: fix dangling sysfs connector node
>>    drm/tilcdc: slave: fix dangling sysfs connector node
>>    drm/tilcdc: tfp410: fix dangling sysfs connector node
>>    drm/tilcdc: panel: fix leak when unloading the module
>>    drm/tilcdc: fix release order on exit
>>    drm/tilcdc: fix double kfree
>>    drm/tilcdc: remove submodule destroy calls
>>    drm/tilcdc: replace late_initcall with module_init
>>
>>   drivers/gpu/drm/i2c/tda998x_drv.c      |  2 +-
>>   drivers/gpu/drm/tilcdc/Module.symvers  |  0
>>   drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 15 +++++--------
>>   drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
>>   drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 39
>> +++++++++++++++++-----------------
>>   drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 27 +++++++++++++----------
>>   drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35
>> +++++++++++++++---------------
>>   7 files changed, 59 insertions(+), 60 deletions(-)
>>   create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Rob Clark]

On Fri, Jun 27, 2014 at 6:08 PM, Darren Etheridge <detheridge@ti.com> wrote:
> Guido,
>
>
> On 06/17/2014 09:17 AM, Guido Mart?nez wrote:
>>
>> The tilcdc driver could be compiled as a module, but was severely broken
>> and could not be used as such. This patchset attempts to fix the issues
>> preventing a proper load/unload of the module.
>>
>> Issues included dangling sysfs nodes, dangling devices, memory leaks and
>> a double kfree.
>>
>> It now seems to be working ok. We have tested this by loading and
>> unloading the driver repeteadly, with both panel and slave connectors
>> and found no flaws.
>>
>> There is still one warning left on tilcdc_crtc_destroy, caused by
>> destroying the connector while still in an ON status. We don't know why
>> this happens or why it's an issue, so we did not fix it.
>>
>
> Yes I see what you mean, it triggers the WARN_ON in tilcdc_crtc_destroy
> because DRM_MODE_DPMS_ON is still set.  This WARN_ON does make some sense
> because DPMS_OFF would have the effect of turning off clocks and putting the
> monitor to sleep which seems logical considering we have torn down the
> display.  Adding a tilcdc_crtc_dpms(DPMS_OFF) right before the WARN_ON
> confirms this, but it seems strange that this hasn't happened automatically
> (+ Russell doesn't need to do it in his Armada driver) - so I suspect there
> is a better way.

tbh, I'm not entirely sure offhand why drm_mode_config_cleanup()
doesn't remove the fb's first (which should have the effect of
shutting down any lit crtc/encoder/connector)..  that would seem like
the sensible way to shut down..

BR,
-R

> Otherwise I think this is a good and useful patch series.
>
> Darren
>
>
>> The first 7 patches are bug fixes which and should probably be applied
>> in the stable trees. The last two are clean-ups.
>>
>>
>> Resending this since I got no replies.
>>
>>
>> Guido Mart?nez (9):
>>    drm/i2c: tda998x: move drm_i2c_encoder_destroy call
>>    drm/tilcdc: panel: fix dangling sysfs connector node
>>    drm/tilcdc: slave: fix dangling sysfs connector node
>>    drm/tilcdc: tfp410: fix dangling sysfs connector node
>>    drm/tilcdc: panel: fix leak when unloading the module
>>    drm/tilcdc: fix release order on exit
>>    drm/tilcdc: fix double kfree
>>    drm/tilcdc: remove submodule destroy calls
>>    drm/tilcdc: replace late_initcall with module_init
>>
>>   drivers/gpu/drm/i2c/tda998x_drv.c      |  2 +-
>>   drivers/gpu/drm/tilcdc/Module.symvers  |  0
>>   drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 15 +++++--------
>>   drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
>>   drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 39
>> +++++++++++++++++-----------------
>>   drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 27 +++++++++++++----------
>>   drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35
>> +++++++++++++++---------------
>>   7 files changed, 59 insertions(+), 60 deletions(-)
>>   create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Guido Martínez]

On Fri, Jun 27, 2014 at 05:08:51PM -0500, Darren Etheridge wrote:
> [snip]
> 
> Otherwise I think this is a good and useful patch series.
It that a Tested-by tag? :)

Thanks!
Guido

> 
> Darren
> 
> >The first 7 patches are bug fixes which and should probably be applied
> >in the stable trees. The last two are clean-ups.
> >
> >
> >Resending this since I got no replies.
> >
> >
> >Guido Martínez (9):
> >   drm/i2c: tda998x: move drm_i2c_encoder_destroy call
> >   drm/tilcdc: panel: fix dangling sysfs connector node
> >   drm/tilcdc: slave: fix dangling sysfs connector node
> >   drm/tilcdc: tfp410: fix dangling sysfs connector node
> >   drm/tilcdc: panel: fix leak when unloading the module
> >   drm/tilcdc: fix release order on exit
> >   drm/tilcdc: fix double kfree
> >   drm/tilcdc: remove submodule destroy calls
> >   drm/tilcdc: replace late_initcall with module_init
> >
> >  drivers/gpu/drm/i2c/tda998x_drv.c      |  2 +-
> >  drivers/gpu/drm/tilcdc/Module.symvers  |  0
> >  drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 15 +++++--------
> >  drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
> >  drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 39 +++++++++++++++++-----------------
> >  drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 27 +++++++++++++----------
> >  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35 +++++++++++++++---------------
> >  7 files changed, 59 insertions(+), 60 deletions(-)
> >  create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
> >

-- 
Guido Martínez, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Guido Martínez]

On Fri, Jun 27, 2014 at 05:08:51PM -0500, Darren Etheridge wrote:
> [snip]
> 
> Otherwise I think this is a good and useful patch series.
It that a Tested-by tag? :)

Thanks!
Guido

> 
> Darren
> 
> >The first 7 patches are bug fixes which and should probably be applied
> >in the stable trees. The last two are clean-ups.
> >
> >
> >Resending this since I got no replies.
> >
> >
> >Guido Mart?nez (9):
> >   drm/i2c: tda998x: move drm_i2c_encoder_destroy call
> >   drm/tilcdc: panel: fix dangling sysfs connector node
> >   drm/tilcdc: slave: fix dangling sysfs connector node
> >   drm/tilcdc: tfp410: fix dangling sysfs connector node
> >   drm/tilcdc: panel: fix leak when unloading the module
> >   drm/tilcdc: fix release order on exit
> >   drm/tilcdc: fix double kfree
> >   drm/tilcdc: remove submodule destroy calls
> >   drm/tilcdc: replace late_initcall with module_init
> >
> >  drivers/gpu/drm/i2c/tda998x_drv.c      |  2 +-
> >  drivers/gpu/drm/tilcdc/Module.symvers  |  0
> >  drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 15 +++++--------
> >  drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
> >  drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 39 +++++++++++++++++-----------------
> >  drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 27 +++++++++++++----------
> >  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35 +++++++++++++++---------------
> >  7 files changed, 59 insertions(+), 60 deletions(-)
> >  create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
> >

-- 
Guido Mart?nez, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Guido Martínez]

Hi Dave,

did you take a look at this patchset? I foolishly missed adding you on
the To: header, so apologies for that in advance.

Thanks,

On Tue, Jun 17, 2014 at 11:17:02AM -0300, Guido Martínez wrote:
> The tilcdc driver could be compiled as a module, but was severely broken
> and could not be used as such. This patchset attempts to fix the issues
> preventing a proper load/unload of the module.
> 
> Issues included dangling sysfs nodes, dangling devices, memory leaks and
> a double kfree.
> 
> It now seems to be working ok. We have tested this by loading and
> unloading the driver repeteadly, with both panel and slave connectors
> and found no flaws.
> 
> There is still one warning left on tilcdc_crtc_destroy, caused by
> destroying the connector while still in an ON status. We don't know why
> this happens or why it's an issue, so we did not fix it.
> 
> The first 7 patches are bug fixes which and should probably be applied
> in the stable trees. The last two are clean-ups.
> 
> 
> Resending this since I got no replies.
> 
> 
> Guido Martínez (9):
>   drm/i2c: tda998x: move drm_i2c_encoder_destroy call
>   drm/tilcdc: panel: fix dangling sysfs connector node
>   drm/tilcdc: slave: fix dangling sysfs connector node
>   drm/tilcdc: tfp410: fix dangling sysfs connector node
>   drm/tilcdc: panel: fix leak when unloading the module
>   drm/tilcdc: fix release order on exit
>   drm/tilcdc: fix double kfree
>   drm/tilcdc: remove submodule destroy calls
>   drm/tilcdc: replace late_initcall with module_init
> 
>  drivers/gpu/drm/i2c/tda998x_drv.c      |  2 +-
>  drivers/gpu/drm/tilcdc/Module.symvers  |  0
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 15 +++++--------
>  drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
>  drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 39 +++++++++++++++++-----------------
>  drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 27 +++++++++++++----------
>  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35 +++++++++++++++---------------
>  7 files changed, 59 insertions(+), 60 deletions(-)
>  create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
> 
> -- 
> 2.0.0
> 

-- 
Guido Martínez, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Guido Martínez]

Hi Dave,

did you take a look at this patchset? I foolishly missed adding you on
the To: header, so apologies for that in advance.

Thanks,

On Tue, Jun 17, 2014 at 11:17:02AM -0300, Guido Mart?nez wrote:
> The tilcdc driver could be compiled as a module, but was severely broken
> and could not be used as such. This patchset attempts to fix the issues
> preventing a proper load/unload of the module.
> 
> Issues included dangling sysfs nodes, dangling devices, memory leaks and
> a double kfree.
> 
> It now seems to be working ok. We have tested this by loading and
> unloading the driver repeteadly, with both panel and slave connectors
> and found no flaws.
> 
> There is still one warning left on tilcdc_crtc_destroy, caused by
> destroying the connector while still in an ON status. We don't know why
> this happens or why it's an issue, so we did not fix it.
> 
> The first 7 patches are bug fixes which and should probably be applied
> in the stable trees. The last two are clean-ups.
> 
> 
> Resending this since I got no replies.
> 
> 
> Guido Mart?nez (9):
>   drm/i2c: tda998x: move drm_i2c_encoder_destroy call
>   drm/tilcdc: panel: fix dangling sysfs connector node
>   drm/tilcdc: slave: fix dangling sysfs connector node
>   drm/tilcdc: tfp410: fix dangling sysfs connector node
>   drm/tilcdc: panel: fix leak when unloading the module
>   drm/tilcdc: fix release order on exit
>   drm/tilcdc: fix double kfree
>   drm/tilcdc: remove submodule destroy calls
>   drm/tilcdc: replace late_initcall with module_init
> 
>  drivers/gpu/drm/i2c/tda998x_drv.c      |  2 +-
>  drivers/gpu/drm/tilcdc/Module.symvers  |  0
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 15 +++++--------
>  drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
>  drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 39 +++++++++++++++++-----------------
>  drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 27 +++++++++++++----------
>  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35 +++++++++++++++---------------
>  7 files changed, 59 insertions(+), 60 deletions(-)
>  create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
> 
> -- 
> 2.0.0
> 

-- 
Guido Mart?nez, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Darren Etheridge]

On 07/01/2014 06:39 PM, Guido Martínez wrote:
> On Fri, Jun 27, 2014 at 05:08:51PM -0500, Darren Etheridge wrote:
>> [snip]
>>
>> Otherwise I think this is a good and useful patch series.
> It that a Tested-by tag? :)

Sure - I would prefer that the WARN_ON was silenced when the tilcdc 
module is removed, but the series adds improvements over what is there 
now.  I have tested it across a few variants of AM335x boards and 
attached display hardware in both module form and built-in to the 
kernel, therefore:

Tested-by: Darren Etheridge <detheridge@ti.com>

>
> Thanks!
> Guido
>
>>
>> Darren
>>
>>> The first 7 patches are bug fixes which and should probably be applied
>>> in the stable trees. The last two are clean-ups.
>>>
>>>
>>> Resending this since I got no replies.
>>>
>>>
>>> Guido Martínez (9):
>>>    drm/i2c: tda998x: move drm_i2c_encoder_destroy call
>>>    drm/tilcdc: panel: fix dangling sysfs connector node
>>>    drm/tilcdc: slave: fix dangling sysfs connector node
>>>    drm/tilcdc: tfp410: fix dangling sysfs connector node
>>>    drm/tilcdc: panel: fix leak when unloading the module
>>>    drm/tilcdc: fix release order on exit
>>>    drm/tilcdc: fix double kfree
>>>    drm/tilcdc: remove submodule destroy calls
>>>    drm/tilcdc: replace late_initcall with module_init
>>>
>>>   drivers/gpu/drm/i2c/tda998x_drv.c      |  2 +-
>>>   drivers/gpu/drm/tilcdc/Module.symvers  |  0
>>>   drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 15 +++++--------
>>>   drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
>>>   drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 39 +++++++++++++++++-----------------
>>>   drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 27 +++++++++++++----------
>>>   drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35 +++++++++++++++---------------
>>>   7 files changed, 59 insertions(+), 60 deletions(-)
>>>   create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
>>>
>

[PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Darren Etheridge]

On 07/01/2014 06:39 PM, Guido Mart?nez wrote:
> On Fri, Jun 27, 2014 at 05:08:51PM -0500, Darren Etheridge wrote:
>> [snip]
>>
>> Otherwise I think this is a good and useful patch series.
> It that a Tested-by tag? :)

Sure - I would prefer that the WARN_ON was silenced when the tilcdc 
module is removed, but the series adds improvements over what is there 
now.  I have tested it across a few variants of AM335x boards and 
attached display hardware in both module form and built-in to the 
kernel, therefore:

Tested-by: Darren Etheridge <detheridge@ti.com>

>
> Thanks!
> Guido
>
>>
>> Darren
>>
>>> The first 7 patches are bug fixes which and should probably be applied
>>> in the stable trees. The last two are clean-ups.
>>>
>>>
>>> Resending this since I got no replies.
>>>
>>>
>>> Guido Mart?nez (9):
>>>    drm/i2c: tda998x: move drm_i2c_encoder_destroy call
>>>    drm/tilcdc: panel: fix dangling sysfs connector node
>>>    drm/tilcdc: slave: fix dangling sysfs connector node
>>>    drm/tilcdc: tfp410: fix dangling sysfs connector node
>>>    drm/tilcdc: panel: fix leak when unloading the module
>>>    drm/tilcdc: fix release order on exit
>>>    drm/tilcdc: fix double kfree
>>>    drm/tilcdc: remove submodule destroy calls
>>>    drm/tilcdc: replace late_initcall with module_init
>>>
>>>   drivers/gpu/drm/i2c/tda998x_drv.c      |  2 +-
>>>   drivers/gpu/drm/tilcdc/Module.symvers  |  0
>>>   drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 15 +++++--------
>>>   drivers/gpu/drm/tilcdc/tilcdc_drv.h    |  1 -
>>>   drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 39 +++++++++++++++++-----------------
>>>   drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 27 +++++++++++++----------
>>>   drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35 +++++++++++++++---------------
>>>   7 files changed, 59 insertions(+), 60 deletions(-)
>>>   create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
>>>
>

Re: [PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Dave Airlie]

On 2 July 2014 12:31, Darren Etheridge <detheridge@ti.com> wrote:
> On 07/01/2014 06:39 PM, Guido Martínez wrote:
>>
>> On Fri, Jun 27, 2014 at 05:08:51PM -0500, Darren Etheridge wrote:
>>>
>>> [snip]
>>>
>>> Otherwise I think this is a good and useful patch series.
>>
>> It that a Tested-by tag? :)
>
>
> Sure - I would prefer that the WARN_ON was silenced when the tilcdc module
> is removed, but the series adds improvements over what is there now.  I have
> tested it across a few variants of AM335x boards and attached display
> hardware in both module form and built-in to the kernel, therefore:
>
> Tested-by: Darren Etheridge <detheridge@ti.com>

Has someone gathered that tags and put these in a git tree by any chance?

Dave.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Dave Airlie]

On 2 July 2014 12:31, Darren Etheridge <detheridge@ti.com> wrote:
> On 07/01/2014 06:39 PM, Guido Mart?nez wrote:
>>
>> On Fri, Jun 27, 2014 at 05:08:51PM -0500, Darren Etheridge wrote:
>>>
>>> [snip]
>>>
>>> Otherwise I think this is a good and useful patch series.
>>
>> It that a Tested-by tag? :)
>
>
> Sure - I would prefer that the WARN_ON was silenced when the tilcdc module
> is removed, but the series adds improvements over what is there now.  I have
> tested it across a few variants of AM335x boards and attached display
> hardware in both module form and built-in to the kernel, therefore:
>
> Tested-by: Darren Etheridge <detheridge@ti.com>

Has someone gathered that tags and put these in a git tree by any chance?

Dave.

Re: [PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Ezequiel García]

On 02 Jul 02:08 PM, Dave Airlie wrote:
> On 2 July 2014 12:31, Darren Etheridge <detheridge@ti.com> wrote:
> > On 07/01/2014 06:39 PM, Guido Martínez wrote:
> >>
> >> On Fri, Jun 27, 2014 at 05:08:51PM -0500, Darren Etheridge wrote:
> >>>
> >>> [snip]
> >>>
> >>> Otherwise I think this is a good and useful patch series.
> >>
> >> It that a Tested-by tag? :)
> >
> >
> > Sure - I would prefer that the WARN_ON was silenced when the tilcdc module
> > is removed, but the series adds improvements over what is there now.  I have
> > tested it across a few variants of AM335x boards and attached display
> > hardware in both module form and built-in to the kernel, therefore:
> >
> > Tested-by: Darren Etheridge <detheridge@ti.com>
> 
> Has someone gathered that tags and put these in a git tree by any chance?
> 

I don't think anyone has. Is that a problem?

If you need a pull request with these, I can prepare something in bitbucket.
Would that work you?
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Ezequiel García]

On 02 Jul 02:08 PM, Dave Airlie wrote:
> On 2 July 2014 12:31, Darren Etheridge <detheridge@ti.com> wrote:
> > On 07/01/2014 06:39 PM, Guido Mart?nez wrote:
> >>
> >> On Fri, Jun 27, 2014 at 05:08:51PM -0500, Darren Etheridge wrote:
> >>>
> >>> [snip]
> >>>
> >>> Otherwise I think this is a good and useful patch series.
> >>
> >> It that a Tested-by tag? :)
> >
> >
> > Sure - I would prefer that the WARN_ON was silenced when the tilcdc module
> > is removed, but the series adds improvements over what is there now.  I have
> > tested it across a few variants of AM335x boards and attached display
> > hardware in both module form and built-in to the kernel, therefore:
> >
> > Tested-by: Darren Etheridge <detheridge@ti.com>
> 
> Has someone gathered that tags and put these in a git tree by any chance?
> 

I don't think anyone has. Is that a problem?

If you need a pull request with these, I can prepare something in bitbucket.
Would that work you?
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Dave Airlie]

On 3 July 2014 06:38, Ezequiel García <ezequiel@vanguardiasur.com.ar> wrote:
> On 02 Jul 02:08 PM, Dave Airlie wrote:
>> On 2 July 2014 12:31, Darren Etheridge <detheridge@ti.com> wrote:
>> > On 07/01/2014 06:39 PM, Guido Martínez wrote:
>> >>
>> >> On Fri, Jun 27, 2014 at 05:08:51PM -0500, Darren Etheridge wrote:
>> >>>
>> >>> [snip]
>> >>>
>> >>> Otherwise I think this is a good and useful patch series.
>> >>
>> >> It that a Tested-by tag? :)
>> >
>> >
>> > Sure - I would prefer that the WARN_ON was silenced when the tilcdc module
>> > is removed, but the series adds improvements over what is there now.  I have
>> > tested it across a few variants of AM335x boards and attached display
>> > hardware in both module form and built-in to the kernel, therefore:
>> >
>> > Tested-by: Darren Etheridge <detheridge@ti.com>
>>
>> Has someone gathered that tags and put these in a git tree by any chance?
>>
>
> I don't think anyone has. Is that a problem?
>
> If you need a pull request with these, I can prepare something in bitbucket.
> Would that work you?

I've picked them up manually now, so should be all fine.

Thanks,
Dave.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Dave Airlie]

On 3 July 2014 06:38, Ezequiel Garc?a <ezequiel@vanguardiasur.com.ar> wrote:
> On 02 Jul 02:08 PM, Dave Airlie wrote:
>> On 2 July 2014 12:31, Darren Etheridge <detheridge@ti.com> wrote:
>> > On 07/01/2014 06:39 PM, Guido Mart?nez wrote:
>> >>
>> >> On Fri, Jun 27, 2014 at 05:08:51PM -0500, Darren Etheridge wrote:
>> >>>
>> >>> [snip]
>> >>>
>> >>> Otherwise I think this is a good and useful patch series.
>> >>
>> >> It that a Tested-by tag? :)
>> >
>> >
>> > Sure - I would prefer that the WARN_ON was silenced when the tilcdc module
>> > is removed, but the series adds improvements over what is there now.  I have
>> > tested it across a few variants of AM335x boards and attached display
>> > hardware in both module form and built-in to the kernel, therefore:
>> >
>> > Tested-by: Darren Etheridge <detheridge@ti.com>
>>
>> Has someone gathered that tags and put these in a git tree by any chance?
>>
>
> I don't think anyone has. Is that a problem?
>
> If you need a pull request with these, I can prepare something in bitbucket.
> Would that work you?

I've picked them up manually now, so should be all fine.

Thanks,
Dave.

Re: [PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Daniel Vetter]

On Sat, Jun 28, 2014 at 06:51:15AM -0400, Rob Clark wrote:
> On Fri, Jun 27, 2014 at 6:08 PM, Darren Etheridge <detheridge@ti.com> wrote:
> > Guido,
> >
> >
> > On 06/17/2014 09:17 AM, Guido Martínez wrote:
> >>
> >> The tilcdc driver could be compiled as a module, but was severely broken
> >> and could not be used as such. This patchset attempts to fix the issues
> >> preventing a proper load/unload of the module.
> >>
> >> Issues included dangling sysfs nodes, dangling devices, memory leaks and
> >> a double kfree.
> >>
> >> It now seems to be working ok. We have tested this by loading and
> >> unloading the driver repeteadly, with both panel and slave connectors
> >> and found no flaws.
> >>
> >> There is still one warning left on tilcdc_crtc_destroy, caused by
> >> destroying the connector while still in an ON status. We don't know why
> >> this happens or why it's an issue, so we did not fix it.
> >>
> >
> > Yes I see what you mean, it triggers the WARN_ON in tilcdc_crtc_destroy
> > because DRM_MODE_DPMS_ON is still set.  This WARN_ON does make some sense
> > because DPMS_OFF would have the effect of turning off clocks and putting the
> > monitor to sleep which seems logical considering we have torn down the
> > display.  Adding a tilcdc_crtc_dpms(DPMS_OFF) right before the WARN_ON
> > confirms this, but it seems strange that this hasn't happened automatically
> > (+ Russell doesn't need to do it in his Armada driver) - so I suspect there
> > is a better way.
> 
> tbh, I'm not entirely sure offhand why drm_mode_config_cleanup()
> doesn't remove the fb's first (which should have the effect of
> shutting down any lit crtc/encoder/connector)..  that would seem like
> the sensible way to shut down..

All userspace fbs should be cleared already before going into the driver
unload. Which only leaves you with driver internal fbs (usually just one
for fbdev emulation). It's the driver's job to clean that up explicitly.
Then you can call mode_config_cleanup and the WARN_ON in there is a really
nice space leak check.

If we'd unconditionally clean up all fbs we'd have trouble with
driver-private embedded fbs and their refcounting and would loose the
space leak check.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Daniel Vetter]

On Sat, Jun 28, 2014 at 06:51:15AM -0400, Rob Clark wrote:
> On Fri, Jun 27, 2014 at 6:08 PM, Darren Etheridge <detheridge@ti.com> wrote:
> > Guido,
> >
> >
> > On 06/17/2014 09:17 AM, Guido Mart?nez wrote:
> >>
> >> The tilcdc driver could be compiled as a module, but was severely broken
> >> and could not be used as such. This patchset attempts to fix the issues
> >> preventing a proper load/unload of the module.
> >>
> >> Issues included dangling sysfs nodes, dangling devices, memory leaks and
> >> a double kfree.
> >>
> >> It now seems to be working ok. We have tested this by loading and
> >> unloading the driver repeteadly, with both panel and slave connectors
> >> and found no flaws.
> >>
> >> There is still one warning left on tilcdc_crtc_destroy, caused by
> >> destroying the connector while still in an ON status. We don't know why
> >> this happens or why it's an issue, so we did not fix it.
> >>
> >
> > Yes I see what you mean, it triggers the WARN_ON in tilcdc_crtc_destroy
> > because DRM_MODE_DPMS_ON is still set.  This WARN_ON does make some sense
> > because DPMS_OFF would have the effect of turning off clocks and putting the
> > monitor to sleep which seems logical considering we have torn down the
> > display.  Adding a tilcdc_crtc_dpms(DPMS_OFF) right before the WARN_ON
> > confirms this, but it seems strange that this hasn't happened automatically
> > (+ Russell doesn't need to do it in his Armada driver) - so I suspect there
> > is a better way.
> 
> tbh, I'm not entirely sure offhand why drm_mode_config_cleanup()
> doesn't remove the fb's first (which should have the effect of
> shutting down any lit crtc/encoder/connector)..  that would seem like
> the sensible way to shut down..

All userspace fbs should be cleared already before going into the driver
unload. Which only leaves you with driver internal fbs (usually just one
for fbdev emulation). It's the driver's job to clean that up explicitly.
Then you can call mode_config_cleanup and the WARN_ON in there is a really
nice space leak check.

If we'd unconditionally clean up all fbs we'd have trouble with
driver-private embedded fbs and their refcounting and would loose the
space leak check.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch