[PATCH RFC 0/3] drm/exynos: refactoring drm initialization/cleanup code

[PATCH RFC 0/3] drm/exynos: refactoring drm initialization/cleanup code

[Andrzej Hajda]

Hi,

The patchset presents alternative approach to superdevice DT node
and components framework. It also refactors Exynos DRM device initialization.

The first patch uses linker sections to get rid of ifdef macros, it is not
essential for the rest of patchset but it makes code more readable.
Similar approach is used by irqchip, clks and clk_sources.
Any comments are welcome, especially regarding linker script usage in drivers.

The second patch proposes pending_components framework - lightweight alternative
for componentized devices.
Details are described in the patch description. But as it is an alternative for
component framework I would like to show differences:
- it is simpler and more straightforward,
- it requires only minimal changes to component drivers,
- only one callback to implement,
- no unwinding, component framework in case of error removes all already added
  components and unbinds all already bound components,
- it is instantiated per superdevice, component framework uses global list,
  the disadvantage is that we create dependency between components and
  the superdevice, but in case of Exynos DRM the dependency is present already,
- no two stage component initialization: probe, bind,
- it is more lightweight,
- it clearly separates device probing order issue from registering interfaces
  provided by the device.

The third patch implements pending_components framework in Exynos DRM.
Details are in patch description.

The patchset is based on exynos-drm-next branch.

Regards
Andrzej


Andrzej Hajda (3):
  drm/exynos: refactor drm drivers registration code
  drivers/base: provide lightweight framework for componentized devices
  drm/exynos: use pending_components for components tracking

 drivers/base/Kconfig                        |   3 +
 drivers/base/Makefile                       |   1 +
 drivers/base/pending_components.c           |  93 +++++++++++
 drivers/gpu/drm/exynos/Kconfig              |   1 +
 drivers/gpu/drm/exynos/Makefile             |   2 +
 drivers/gpu/drm/exynos/exynos_dp_core.c     |  38 +++--
 drivers/gpu/drm/exynos/exynos_drm.lds.S     |   9 +
 drivers/gpu/drm/exynos/exynos_drm_drv.c     | 250 ++++++++++------------------
 drivers/gpu/drm/exynos/exynos_drm_drv.h     |  33 ++--
 drivers/gpu/drm/exynos/exynos_drm_dsi.c     |  43 +++--
 drivers/gpu/drm/exynos/exynos_drm_fimc.c    |  36 ++--
 drivers/gpu/drm/exynos/exynos_drm_fimd.c    |  39 +++--
 drivers/gpu/drm/exynos/exynos_drm_g2d.c     |  19 ++-
 drivers/gpu/drm/exynos/exynos_drm_gsc.c     |  32 ++--
 drivers/gpu/drm/exynos/exynos_drm_ipp.c     |  20 ++-
 drivers/gpu/drm/exynos/exynos_drm_rotator.c |  29 +++-
 drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  18 +-
 drivers/gpu/drm/exynos/exynos_hdmi.c        |  55 ++++--
 drivers/gpu/drm/exynos/exynos_mixer.c       |  15 +-
 include/linux/pending_components.h          |  30 ++++
 20 files changed, 466 insertions(+), 300 deletions(-)
 create mode 100644 drivers/base/pending_components.c
 create mode 100644 drivers/gpu/drm/exynos/exynos_drm.lds.S
 create mode 100644 include/linux/pending_components.h

-- 
1.8.3.2

[PATCH RFC 1/3] drm/exynos: refactor drm drivers registration code

[Andrzej Hajda]

The patch removes driver registration code based on preprocessor conditionals.
Instead it uses private linker section to create array of drm drivers.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
v2:
- minor fixes of compilation issues
---
 drivers/gpu/drm/exynos/Makefile             |   2 +
 drivers/gpu/drm/exynos/exynos_dp_core.c     |   2 +-
 drivers/gpu/drm/exynos/exynos_drm.lds.S     |   9 ++
 drivers/gpu/drm/exynos/exynos_drm_drv.c     | 203 +++++-----------------------
 drivers/gpu/drm/exynos/exynos_drm_drv.h     |  30 ++--
 drivers/gpu/drm/exynos/exynos_drm_dsi.c     |   2 +-
 drivers/gpu/drm/exynos/exynos_drm_fimc.c    |   2 +-
 drivers/gpu/drm/exynos/exynos_drm_fimd.c    |   2 +-
 drivers/gpu/drm/exynos/exynos_drm_g2d.c     |   2 +-
 drivers/gpu/drm/exynos/exynos_drm_gsc.c     |   2 +-
 drivers/gpu/drm/exynos/exynos_drm_ipp.c     |   2 +-
 drivers/gpu/drm/exynos/exynos_drm_rotator.c |   2 +-
 drivers/gpu/drm/exynos/exynos_drm_vidi.c    |   2 +-
 drivers/gpu/drm/exynos/exynos_hdmi.c        |   2 +-
 drivers/gpu/drm/exynos/exynos_mixer.c       |   2 +-
 15 files changed, 68 insertions(+), 198 deletions(-)
 create mode 100644 drivers/gpu/drm/exynos/exynos_drm.lds.S

diff --git a/drivers/gpu/drm/exynos/Makefile b/drivers/gpu/drm/exynos/Makefile
index 33ae365..c8190c1 100644
--- a/drivers/gpu/drm/exynos/Makefile
+++ b/drivers/gpu/drm/exynos/Makefile
@@ -22,4 +22,6 @@ exynosdrm-$(CONFIG_DRM_EXYNOS_FIMC)	+= exynos_drm_fimc.o
 exynosdrm-$(CONFIG_DRM_EXYNOS_ROTATOR)	+= exynos_drm_rotator.o
 exynosdrm-$(CONFIG_DRM_EXYNOS_GSC)	+= exynos_drm_gsc.o
 
+exynosdrm-y				+= exynos_drm.lds
+
 obj-$(CONFIG_DRM_EXYNOS)		+= exynosdrm.o
diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
index aed533b..9385e96 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -1340,7 +1340,7 @@ static const struct of_device_id exynos_dp_match[] = {
 	{},
 };
 
-struct platform_driver dp_driver = {
+EXYNOS_DRM_DRV(dp_driver) = {
 	.probe		= exynos_dp_probe,
 	.remove		= exynos_dp_remove,
 	.driver		= {
diff --git a/drivers/gpu/drm/exynos/exynos_drm.lds.S b/drivers/gpu/drm/exynos/exynos_drm.lds.S
new file mode 100644
index 0000000..fd37dc1
--- /dev/null
+++ b/drivers/gpu/drm/exynos/exynos_drm.lds.S
@@ -0,0 +1,9 @@
+SECTIONS
+{
+	.data : {
+		. = ALIGN(8);
+		exynos_drm_drivers = .;
+		*(exynos_drm_drivers)
+		*(exynos_drm_drivers_last)
+	}
+}
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 2d27ba2..5067b32 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -430,7 +430,7 @@ static const struct dev_pm_ops exynos_drm_pm_ops = {
 			exynos_drm_runtime_resume, NULL)
 };
 
-static struct platform_driver exynos_drm_platform_driver = {
+EXYNOS_DRM_DRV_LAST(exynos_drm_drv) = {
 	.probe		= exynos_drm_platform_probe,
 	.remove		= exynos_drm_platform_remove,
 	.driver		= {
@@ -440,197 +440,64 @@ static struct platform_driver exynos_drm_platform_driver = {
 	},
 };
 
-static int __init exynos_drm_init(void)
+static int exynos_platform_device_drm_register(void)
 {
-	int ret;
-
-#ifdef CONFIG_DRM_EXYNOS_DP
-	ret = platform_driver_register(&dp_driver);
-	if (ret < 0)
-		goto out_dp;
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_DSI
-	ret = platform_driver_register(&dsi_driver);
-	if (ret < 0)
-		goto out_dsi;
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_FIMD
-	ret = platform_driver_register(&fimd_driver);
-	if (ret < 0)
-		goto out_fimd;
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_HDMI
-	ret = platform_driver_register(&hdmi_driver);
-	if (ret < 0)
-		goto out_hdmi;
-	ret = platform_driver_register(&mixer_driver);
-	if (ret < 0)
-		goto out_mixer;
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_VIDI
-	ret = platform_driver_register(&vidi_driver);
-	if (ret < 0)
-		goto out_vidi;
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_G2D
-	ret = platform_driver_register(&g2d_driver);
-	if (ret < 0)
-		goto out_g2d;
-#endif
+	exynos_drm_pdev = platform_device_register_simple("exynos-drm", -1,
+							  NULL, 0);
+	if (IS_ERR(exynos_drm_pdev))
+		return PTR_ERR(exynos_drm_pdev);
 
-#ifdef CONFIG_DRM_EXYNOS_FIMC
-	ret = platform_driver_register(&fimc_driver);
-	if (ret < 0)
-		goto out_fimc;
-#endif
+	return 0;
+}
 
-#ifdef CONFIG_DRM_EXYNOS_ROTATOR
-	ret = platform_driver_register(&rotator_driver);
-	if (ret < 0)
-		goto out_rotator;
-#endif
+static void exynos_platform_device_drm_unregister(void)
+{
+	platform_device_unregister(exynos_drm_pdev);
+}
 
-#ifdef CONFIG_DRM_EXYNOS_GSC
-	ret = platform_driver_register(&gsc_driver);
-	if (ret < 0)
-		goto out_gsc;
-#endif
+extern struct platform_driver exynos_drm_drivers;
 
-#ifdef CONFIG_DRM_EXYNOS_IPP
-	ret = platform_driver_register(&ipp_driver);
-	if (ret < 0)
-		goto out_ipp;
+static int __init exynos_drm_init(void)
+{
+	struct platform_driver *pd = &exynos_drm_drivers;
+	int ret;
 
 	ret = exynos_platform_device_ipp_register();
 	if (ret < 0)
-		goto out_ipp_dev;
-#endif
+		return ret;
 
-	ret = platform_driver_register(&exynos_drm_platform_driver);
+	ret = exynos_platform_device_drm_register();
 	if (ret < 0)
-		goto out_drm;
-
-	exynos_drm_pdev = platform_device_register_simple("exynos-drm", -1,
-				NULL, 0);
-	if (IS_ERR(exynos_drm_pdev)) {
-		ret = PTR_ERR(exynos_drm_pdev);
-		goto out;
+		goto err_dev;
+
+	while (pd <= &exynos_drm_drv) {
+		pr_debug("%s: registering %s\n", __func__, pd->driver.name);
+		ret = platform_driver_register(pd);
+		if (ret < 0)
+			goto err;
+		++pd;
 	}
 
 	return 0;
-
-out:
-	platform_driver_unregister(&exynos_drm_platform_driver);
-
-out_drm:
-#ifdef CONFIG_DRM_EXYNOS_IPP
+err:
+	while (pd-- > &exynos_drm_drivers)
+		platform_driver_unregister(pd);
+err_dev:
 	exynos_platform_device_ipp_unregister();
-out_ipp_dev:
-	platform_driver_unregister(&ipp_driver);
-out_ipp:
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_GSC
-	platform_driver_unregister(&gsc_driver);
-out_gsc:
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_ROTATOR
-	platform_driver_unregister(&rotator_driver);
-out_rotator:
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_FIMC
-	platform_driver_unregister(&fimc_driver);
-out_fimc:
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_G2D
-	platform_driver_unregister(&g2d_driver);
-out_g2d:
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_VIDI
-	platform_driver_unregister(&vidi_driver);
-out_vidi:
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_HDMI
-	platform_driver_unregister(&mixer_driver);
-out_mixer:
-	platform_driver_unregister(&hdmi_driver);
-out_hdmi:
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_FIMD
-	platform_driver_unregister(&fimd_driver);
-out_fimd:
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_DSI
-	platform_driver_unregister(&dsi_driver);
-out_dsi:
-#endif
 
-#ifdef CONFIG_DRM_EXYNOS_DP
-	platform_driver_unregister(&dp_driver);
-out_dp:
-#endif
 	return ret;
 }
 
 static void __exit exynos_drm_exit(void)
 {
-	platform_device_unregister(exynos_drm_pdev);
+	struct platform_driver *pd = &exynos_drm_drv + 1;
 
-	platform_driver_unregister(&exynos_drm_platform_driver);
+	while (--pd >= &exynos_drm_drivers)
+		platform_driver_unregister(pd);
 
-#ifdef CONFIG_DRM_EXYNOS_IPP
+	exynos_platform_device_drm_unregister();
 	exynos_platform_device_ipp_unregister();
-	platform_driver_unregister(&ipp_driver);
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_GSC
-	platform_driver_unregister(&gsc_driver);
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_ROTATOR
-	platform_driver_unregister(&rotator_driver);
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_FIMC
-	platform_driver_unregister(&fimc_driver);
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_G2D
-	platform_driver_unregister(&g2d_driver);
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_HDMI
-	platform_driver_unregister(&mixer_driver);
-	platform_driver_unregister(&hdmi_driver);
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_VIDI
-	platform_driver_unregister(&vidi_driver);
-#endif
 
-#ifdef CONFIG_DRM_EXYNOS_FIMD
-	platform_driver_unregister(&fimd_driver);
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_DSI
-	platform_driver_unregister(&dsi_driver);
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_DP
-	platform_driver_unregister(&dp_driver);
-#endif
 }
 
 module_init(exynos_drm_init);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 4c5cf68..419b9de 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -351,15 +351,13 @@ int exynos_platform_device_hdmi_register(void);
  */
 void exynos_platform_device_hdmi_unregister(void);
 
-/*
- * this function registers exynos drm ipp platform device.
- */
+#ifdef CONFIG_DRM_EXYNOS_IPP
 int exynos_platform_device_ipp_register(void);
-
-/*
- * this function unregisters exynos drm ipp platform device if it exists.
- */
 void exynos_platform_device_ipp_unregister(void);
+#else
+static inline int exynos_platform_device_ipp_register(void) { return 0; }
+static inline void exynos_platform_device_ipp_unregister(void) {}
+#endif
 
 #ifdef CONFIG_DRM_EXYNOS_DPI
 int exynos_dpi_probe(struct device *dev);
@@ -369,16 +367,10 @@ static inline int exynos_dpi_probe(struct device *dev) { return 0; }
 static inline int exynos_dpi_remove(struct device *dev) { return 0; }
 #endif
 
-extern struct platform_driver dp_driver;
-extern struct platform_driver dsi_driver;
-extern struct platform_driver fimd_driver;
-extern struct platform_driver hdmi_driver;
-extern struct platform_driver mixer_driver;
-extern struct platform_driver exynos_drm_common_hdmi_driver;
-extern struct platform_driver vidi_driver;
-extern struct platform_driver g2d_driver;
-extern struct platform_driver fimc_driver;
-extern struct platform_driver rotator_driver;
-extern struct platform_driver gsc_driver;
-extern struct platform_driver ipp_driver;
+#define EXYNOS_DRM_DRV(name) \
+	static struct platform_driver name __section(exynos_drm_drivers) __used
+
+#define EXYNOS_DRM_DRV_LAST(name) \
+	static struct platform_driver name __section(exynos_drm_drivers_last) __used
+
 #endif
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index eb73e3b..56230e6 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1507,7 +1507,7 @@ static struct of_device_id exynos_dsi_of_match[] = {
 	{ }
 };
 
-struct platform_driver dsi_driver = {
+EXYNOS_DRM_DRV(dsi_driver) = {
 	.probe = exynos_dsi_probe,
 	.remove = exynos_dsi_remove,
 	.driver = {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
index 30d76b2..0865d5d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
@@ -1945,7 +1945,7 @@ static const struct of_device_id fimc_of_match[] = {
 	{ },
 };
 
-struct platform_driver fimc_driver = {
+EXYNOS_DRM_DRV(fimc_driver) = {
 	.probe		= fimc_probe,
 	.remove		= fimc_remove,
 	.driver		= {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 40fd6cc..a0f5037 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -937,7 +937,7 @@ static int fimd_remove(struct platform_device *pdev)
 	return 0;
 }
 
-struct platform_driver fimd_driver = {
+EXYNOS_DRM_DRV(fimd_driver) = {
 	.probe		= fimd_probe,
 	.remove		= fimd_remove,
 	.driver		= {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
index 6c1885e..2eb4676 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
@@ -1541,7 +1541,7 @@ static const struct of_device_id exynos_g2d_match[] = {
 	{},
 };
 
-struct platform_driver g2d_driver = {
+EXYNOS_DRM_DRV(g2d_driver) = {
 	.probe		= g2d_probe,
 	.remove		= g2d_remove,
 	.driver		= {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gsc.c b/drivers/gpu/drm/exynos/exynos_drm_gsc.c
index fa75059..76b15fd 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gsc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gsc.c
@@ -1796,7 +1796,7 @@ static const struct dev_pm_ops gsc_pm_ops = {
 	SET_RUNTIME_PM_OPS(gsc_runtime_suspend, gsc_runtime_resume, NULL)
 };
 
-struct platform_driver gsc_driver = {
+EXYNOS_DRM_DRV(gsc_driver) = {
 	.probe		= gsc_probe,
 	.remove		= gsc_remove,
 	.driver		= {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
index 09312b8..1393486 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
@@ -1973,7 +1973,7 @@ static const struct dev_pm_ops ipp_pm_ops = {
 	SET_RUNTIME_PM_OPS(ipp_runtime_suspend, ipp_runtime_resume, NULL)
 };
 
-struct platform_driver ipp_driver = {
+EXYNOS_DRM_DRV(ipp_driver) = {
 	.probe		= ipp_probe,
 	.remove		= ipp_remove,
 	.driver		= {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
index 7b90168..625fd9b 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
@@ -849,7 +849,7 @@ static const struct dev_pm_ops rotator_pm_ops = {
 									NULL)
 };
 
-struct platform_driver rotator_driver = {
+EXYNOS_DRM_DRV(rotator_driver) = {
 	.probe		= rotator_probe,
 	.remove		= rotator_remove,
 	.driver		= {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index 7afead9..976a8fe 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -632,7 +632,7 @@ static int vidi_remove(struct platform_device *pdev)
 	return 0;
 }
 
-struct platform_driver vidi_driver = {
+EXYNOS_DRM_DRV(vidi_driver) = {
 	.probe		= vidi_probe,
 	.remove		= vidi_remove,
 	.driver		= {
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 9a6d652..3e659e9 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -2175,7 +2175,7 @@ static int hdmi_remove(struct platform_device *pdev)
 	return 0;
 }
 
-struct platform_driver hdmi_driver = {
+EXYNOS_DRM_DRV(hdmi_driver) = {
 	.probe		= hdmi_probe,
 	.remove		= hdmi_remove,
 	.driver		= {
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index ce28881..b978bde 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -1250,7 +1250,7 @@ static int mixer_remove(struct platform_device *pdev)
 	return 0;
 }
 
-struct platform_driver mixer_driver = {
+EXYNOS_DRM_DRV(mixer_driver) = {
 	.driver = {
 		.name = "exynos-mixer",
 		.owner = THIS_MODULE,
-- 
1.8.3.2

[PATCH RFC 2/3] drivers/base: provide lightweight framework for componentized devices

[Andrzej Hajda]

Many subsystems (eg. DRM, ALSA) requires that multiple devices should
be composed into one superdevice. The superdevice cannot start until
all components are ready and it should stop before any of its components
becomes not-ready.
This framework provides a way to track readiness of all components with
minimal impact on the code of the drivers.
The superdevice provides pending_components structure and adds all components
to it, device drivers removes themselves from the list if they becomes ready.
If the list becomes empty callback is fired which causes superdevice to start.
Later if any components wants to become not-ready it adds again itself to the
list and callback is fired to stop superdevice.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/base/Kconfig               |  3 ++
 drivers/base/Makefile              |  1 +
 drivers/base/pending_components.c  | 93 ++++++++++++++++++++++++++++++++++++++
 include/linux/pending_components.h | 30 ++++++++++++
 4 files changed, 127 insertions(+)
 create mode 100644 drivers/base/pending_components.c
 create mode 100644 include/linux/pending_components.h

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index ec36e77..71ce050 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -278,4 +278,7 @@ config CMA_AREAS
 
 endif
 
+config PENDING_COMPONENTS
+	boolean
+
 endmenu
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 04b314e..3a51654 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
 obj-$(CONFIG_REGMAP)	+= regmap/
 obj-$(CONFIG_SOC_BUS) += soc.o
 obj-$(CONFIG_PINCTRL) += pinctrl.o
+obj-$(CONFIG_PENDING_COMPONENTS) += pending_components.o
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
 
diff --git a/drivers/base/pending_components.c b/drivers/base/pending_components.c
new file mode 100644
index 0000000..f15104e
--- /dev/null
+++ b/drivers/base/pending_components.c
@@ -0,0 +1,93 @@
+/*
+ * Lightweight framework for handling componentized devices.
+ *
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd
+ * Andrzej Hajda <a.hajda@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * pending_components structure contains list of components which are not yet
+ * ready. Components can remove or add themselves from/to the list. If the list
+ * becomes empty/non-empty optional callback is fired.
+*/
+
+#include <linux/pending_components.h>
+#include <linux/slab.h>
+
+struct pending_components_node {
+	struct list_head list;
+	void *data;
+};
+
+int pending_components_insert(struct pending_components *set, void *item)
+{
+	struct pending_components_node *n;
+	int ret = 0;
+
+	mutex_lock(&set->lock);
+
+	list_for_each_entry(n, &set->list, list) {
+		if (n->data == item)
+			goto out;
+	}
+
+	n = kmalloc(sizeof(*n), GFP_KERNEL);
+	if (!n) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	n->data = item;
+	list_add_tail(&n->list, &set->list);
+	if (set->callback && set->list.next == set->list.prev)
+		ret = set->callback(set, false);
+
+out:
+	mutex_unlock(&set->lock);
+
+	return ret;
+}
+
+int pending_components_delete(struct pending_components *set, void *item)
+{
+	struct pending_components_node *n;
+	int ret = 0;
+
+	mutex_lock(&set->lock);
+
+	list_for_each_entry(n, &set->list, list) {
+		if (n->data == item) {
+			list_del(&n->list);
+			kfree(n);
+			if (set->callback && list_empty(&set->list))
+				ret = set->callback(set, true);
+			break;
+		}
+	}
+
+	mutex_unlock(&set->lock);
+
+	return ret;
+}
+
+void pending_components_set_callback(struct pending_components *set,
+				     pending_components_callback cb)
+{
+	mutex_lock(&set->lock);
+
+	set->callback = cb;
+
+	mutex_unlock(&set->lock);
+}
+
+void pending_components_cleanup(struct pending_components *set)
+{
+	struct pending_components_node *n, *tmp;
+
+	list_for_each_entry_safe(n, tmp, &set->list, list) {
+		list_del(&n->list);
+		kfree(n);
+	}
+}
diff --git a/include/linux/pending_components.h b/include/linux/pending_components.h
new file mode 100644
index 0000000..dd29616
--- /dev/null
+++ b/include/linux/pending_components.h
@@ -0,0 +1,30 @@
+#ifndef PENDING_COMPONENTS_H
+#define PENDING_COMPONENTS_H
+
+#include <linux/list.h>
+#include <linux/mutex.h>
+
+struct pending_components;
+
+typedef int (*pending_components_callback)(struct pending_components *set,
+					   bool empty);
+
+struct pending_components {
+	struct mutex lock;
+	struct list_head list;
+	pending_components_callback callback;
+};
+
+#define DEFINE_PENDING_COMPONENTS(set) \
+	struct pending_components set = { \
+		.lock = __MUTEX_INITIALIZER(set.lock), \
+		.list = LIST_HEAD_INIT(set.list) \
+	}
+
+int pending_components_insert(struct pending_components *set, void *item);
+int pending_components_delete(struct pending_components *set, void *item);
+void pending_components_set_callback(struct pending_components *set,
+				     pending_components_callback cb);
+void pending_components_cleanup(struct pending_components *set);
+
+#endif /* PENDING_COMPONENTS_H */
-- 
1.8.3.2

[PATCH RFC 3/3] drm/exynos: use pending_components for components tracking

[Andrzej Hajda]

exynos_drm is composed from multiple devices which provides different
interfaces. To properly start/stop drm master device it should track
readiness of all its components. This patch uses pending_components
framework to perform this task.
On module initialization before component driver registration all devices
matching drivers are added to pending_components. Drivers during probe
are removed from the list unless deferred probe happens. When list becomes
empty callback is fired to start drm master. Later if any device adds itself
to the list callback is fired to stop drm master.

The core of the changes is in exynos_drm_drv.c. Driver modifications are
limited only to signal its readiness in probe and remove driver callbacks.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/Kconfig              |  1 +
 drivers/gpu/drm/exynos/exynos_dp_core.c     | 36 +++++++++++------
 drivers/gpu/drm/exynos/exynos_drm_drv.c     | 61 +++++++++++++++++++++++++++--
 drivers/gpu/drm/exynos/exynos_drm_drv.h     |  3 ++
 drivers/gpu/drm/exynos/exynos_drm_dsi.c     | 41 +++++++++++--------
 drivers/gpu/drm/exynos/exynos_drm_fimc.c    | 34 ++++++++++------
 drivers/gpu/drm/exynos/exynos_drm_fimd.c    | 37 +++++++++++------
 drivers/gpu/drm/exynos/exynos_drm_g2d.c     | 17 ++++++--
 drivers/gpu/drm/exynos/exynos_drm_gsc.c     | 30 +++++++++-----
 drivers/gpu/drm/exynos/exynos_drm_ipp.c     | 18 ++++++---
 drivers/gpu/drm/exynos/exynos_drm_rotator.c | 27 +++++++++----
 drivers/gpu/drm/exynos/exynos_drm_vidi.c    | 16 +++++---
 drivers/gpu/drm/exynos/exynos_hdmi.c        | 53 +++++++++++++++++--------
 drivers/gpu/drm/exynos/exynos_mixer.c       | 13 ++++--
 14 files changed, 278 insertions(+), 109 deletions(-)

diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig
index 5bf5bca..4ed8eb2 100644
--- a/drivers/gpu/drm/exynos/Kconfig
+++ b/drivers/gpu/drm/exynos/Kconfig
@@ -8,6 +8,7 @@ config DRM_EXYNOS
 	select FB_CFB_IMAGEBLIT
 	select VT_HW_CONSOLE_BINDING if FRAMEBUFFER_CONSOLE
 	select VIDEOMODE_HELPERS
+	select PENDING_COMPONENTS
 	help
 	  Choose this option if you have a Samsung SoC EXYNOS chipset.
 	  If M is selected the module will be called exynosdrm.
diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
index 9385e96..24f5c98 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -1240,29 +1240,32 @@ static int exynos_dp_probe(struct platform_device *pdev)
 	dp = devm_kzalloc(&pdev->dev, sizeof(struct exynos_dp_device),
 				GFP_KERNEL);
 	if (!dp) {
-		dev_err(&pdev->dev, "no memory for device data\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out;
 	}
 
 	dp->dev = &pdev->dev;
 	dp->dpms_mode = DRM_MODE_DPMS_OFF;
 
 	dp->video_info = exynos_dp_dt_parse_pdata(&pdev->dev);
-	if (IS_ERR(dp->video_info))
-		return PTR_ERR(dp->video_info);
+	if (IS_ERR(dp->video_info)) {
+		ret = PTR_ERR(dp->video_info);
+		goto out;
+	}
 
 	ret = exynos_dp_dt_parse_phydata(dp);
 	if (ret)
-		return ret;
+		goto out;
 
 	ret = exynos_dp_dt_parse_panel(dp);
 	if (ret)
-		return ret;
+		goto out;
 
 	dp->clock = devm_clk_get(&pdev->dev, "dp");
 	if (IS_ERR(dp->clock)) {
 		dev_err(&pdev->dev, "failed to get clock\n");
-		return PTR_ERR(dp->clock);
+		ret = PTR_ERR(dp->clock);
+		goto out;
 	}
 
 	clk_prepare_enable(dp->clock);
@@ -1270,13 +1273,16 @@ static int exynos_dp_probe(struct platform_device *pdev)
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
 	dp->reg_base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(dp->reg_base))
-		return PTR_ERR(dp->reg_base);
+	if (IS_ERR(dp->reg_base)) {
+		ret = PTR_ERR(dp->reg_base);
+		goto out;
+	}
 
 	dp->irq = platform_get_irq(pdev, 0);
 	if (dp->irq == -ENXIO) {
 		dev_err(&pdev->dev, "failed to get irq\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto out;
 	}
 
 	INIT_WORK(&dp->hotplug_work, exynos_dp_hotplug);
@@ -1289,7 +1295,7 @@ static int exynos_dp_probe(struct platform_device *pdev)
 				"exynos-dp", dp);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to request irq\n");
-		return ret;
+		goto out;
 	}
 	disable_irq(dp->irq);
 
@@ -1298,13 +1304,19 @@ static int exynos_dp_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, &exynos_dp_display);
 	exynos_drm_display_register(&exynos_dp_display);
 
-	return 0;
+out:
+	if (ret != -EPROBE_DEFER)
+		exynos_drm_dev_ready(&pdev->dev);
+
+	return ret;
 }
 
 static int exynos_dp_remove(struct platform_device *pdev)
 {
 	struct exynos_drm_display *display = platform_get_drvdata(pdev);
 
+	exynos_drm_dev_busy(&pdev->dev);
+
 	exynos_dp_dpms(display, DRM_MODE_DPMS_OFF);
 	exynos_drm_display_unregister(&exynos_dp_display);
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 5067b32..6e73c7f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -14,10 +14,10 @@
 #include <linux/pm_runtime.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/exynos_drm.h>
 
 #include <linux/anon_inodes.h>
-
-#include <drm/exynos_drm.h>
+#include <linux/pending_components.h>
 
 #include "exynos_drm_drv.h"
 #include "exynos_drm_crtc.h"
@@ -355,6 +355,18 @@ static struct drm_driver exynos_drm_driver = {
 	.minor	= DRIVER_MINOR,
 };
 
+static DEFINE_PENDING_COMPONENTS(exynos_drm_blockers);
+
+int exynos_drm_blocker_callback(struct pending_components *set, bool empty)
+{
+	if (empty)
+		return drm_platform_init(&exynos_drm_driver, exynos_drm_pdev);
+
+	drm_put_dev(platform_get_drvdata(exynos_drm_pdev));
+
+	return 0;
+}
+
 static int exynos_drm_platform_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -366,12 +378,19 @@ static int exynos_drm_platform_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 
-	return drm_platform_init(&exynos_drm_driver, pdev);
+	pending_components_set_callback(&exynos_drm_blockers,
+					exynos_drm_blocker_callback);
+	ret = exynos_drm_dev_ready(&pdev->dev);
+	if (ret < 0)
+		pending_components_set_callback(&exynos_drm_blockers, NULL);
+
+	return ret;
 }
 
 static int exynos_drm_platform_remove(struct platform_device *pdev)
 {
-	drm_put_dev(platform_get_drvdata(pdev));
+	exynos_drm_dev_busy(&pdev->dev);
+	pending_components_set_callback(&exynos_drm_blockers, NULL);
 
 	return 0;
 }
@@ -455,6 +474,36 @@ static void exynos_platform_device_drm_unregister(void)
 	platform_device_unregister(exynos_drm_pdev);
 }
 
+int exynos_drm_dev_ready(struct device *dev)
+{
+	return pending_components_delete(&exynos_drm_blockers, dev);
+}
+
+void exynos_drm_dev_busy(struct device *dev)
+{
+	pending_components_insert(&exynos_drm_blockers, dev);
+}
+
+static int exynos_drm_add_blocker(struct device *dev, void *data)
+{
+	struct device_driver *drv = data;
+
+	if (!platform_bus_type.match(dev, drv))
+		return 0;
+
+	device_lock(dev);
+	if (!dev->driver)
+		exynos_drm_dev_busy(dev);
+	device_unlock(dev);
+
+	return 0;
+}
+
+static void exynos_drm_init_blockers(struct device_driver *drv)
+{
+	bus_for_each_dev(&platform_bus_type, NULL, drv, exynos_drm_add_blocker);
+}
+
 extern struct platform_driver exynos_drm_drivers;
 
 static int __init exynos_drm_init(void)
@@ -472,6 +521,7 @@ static int __init exynos_drm_init(void)
 
 	while (pd <= &exynos_drm_drv) {
 		pr_debug("%s: registering %s\n", __func__, pd->driver.name);
+		exynos_drm_init_blockers(&pd->driver);
 		ret = platform_driver_register(pd);
 		if (ret < 0)
 			goto err;
@@ -485,6 +535,8 @@ err:
 err_dev:
 	exynos_platform_device_ipp_unregister();
 
+	pending_components_cleanup(&exynos_drm_blockers);
+
 	return ret;
 }
 
@@ -498,6 +550,7 @@ static void __exit exynos_drm_exit(void)
 	exynos_platform_device_drm_unregister();
 	exynos_platform_device_ipp_unregister();
 
+	pending_components_cleanup(&exynos_drm_blockers);
 }
 
 module_init(exynos_drm_init);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 419b9de..03de62b 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -359,6 +359,9 @@ static inline int exynos_platform_device_ipp_register(void) { return 0; }
 static inline void exynos_platform_device_ipp_unregister(void) {}
 #endif
 
+int exynos_drm_dev_ready(struct device *dev);
+void exynos_drm_dev_busy(struct device *dev);
+
 #ifdef CONFIG_DRM_EXYNOS_DPI
 int exynos_dpi_probe(struct device *dev);
 int exynos_dpi_remove(struct device *dev);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 56230e6..f606ec2 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1386,8 +1386,8 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 
 	dsi = devm_kzalloc(&pdev->dev, sizeof(*dsi), GFP_KERNEL);
 	if (!dsi) {
-		dev_err(&pdev->dev, "failed to allocate dsi object.\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out;
 	}
 
 	init_completion(&dsi->completed);
@@ -1401,46 +1401,48 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 
 	ret = exynos_dsi_parse_dt(dsi);
 	if (ret)
-		return ret;
+		goto out;
 
 	dsi->supplies[0].supply = "vddcore";
 	dsi->supplies[1].supply = "vddio";
 	ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(dsi->supplies),
 				      dsi->supplies);
-	if (ret) {
-		dev_info(&pdev->dev, "failed to get regulators: %d\n", ret);
-		return -EPROBE_DEFER;
-	}
+	if (ret)
+		goto out;
 
 	dsi->pll_clk = devm_clk_get(&pdev->dev, "pll_clk");
 	if (IS_ERR(dsi->pll_clk)) {
-		dev_info(&pdev->dev, "failed to get dsi pll input clock\n");
-		return -EPROBE_DEFER;
+		ret = PTR_ERR(dsi->pll_clk);
+		dev_info(&pdev->dev, "failed to get dsi pll clock\n");
+		goto out;
 	}
 
 	dsi->bus_clk = devm_clk_get(&pdev->dev, "bus_clk");
 	if (IS_ERR(dsi->bus_clk)) {
+		ret = PTR_ERR(dsi->bus_clk);
 		dev_info(&pdev->dev, "failed to get dsi bus clock\n");
-		return -EPROBE_DEFER;
+		goto out;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	dsi->reg_base = devm_ioremap_resource(&pdev->dev, res);
 	if (!dsi->reg_base) {
 		dev_err(&pdev->dev, "failed to remap io region\n");
-		return -EADDRNOTAVAIL;
+		ret = -EADDRNOTAVAIL;
+		goto out;
 	}
 
 	dsi->phy = devm_phy_get(&pdev->dev, "dsim");
 	if (IS_ERR(dsi->phy)) {
-		dev_info(&pdev->dev, "failed to get dsim phy\n");
-		return -EPROBE_DEFER;
+		ret = PTR_ERR(dsi->phy);
+		goto out;
 	}
 
 	dsi->irq = platform_get_irq(pdev, 0);
 	if (dsi->irq < 0) {
+		ret = dsi->irq;
 		dev_err(&pdev->dev, "failed to request dsi irq resource\n");
-		return dsi->irq;
+		goto out;
 	}
 
 	irq_set_status_flags(dsi->irq, IRQ_NOAUTOEN);
@@ -1449,7 +1451,7 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 					dev_name(&pdev->dev), dsi);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to request dsi irq\n");
-		return ret;
+		goto out;
 	}
 
 	exynos_dsi_display.ctx = dsi;
@@ -1457,7 +1459,12 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, &exynos_dsi_display);
 	exynos_drm_display_register(&exynos_dsi_display);
 
-	return mipi_dsi_host_register(&dsi->dsi_host);
+	ret = mipi_dsi_host_register(&dsi->dsi_host);
+out:
+	if (ret != -EPROBE_DEFER)
+		exynos_drm_dev_ready(&pdev->dev);
+
+	return ret;
 }
 
 static int exynos_dsi_remove(struct platform_device *pdev)
@@ -1465,7 +1472,7 @@ static int exynos_dsi_remove(struct platform_device *pdev)
 	struct exynos_dsi *dsi = exynos_dsi_display.ctx;
 
 	exynos_dsi_dpms(&exynos_dsi_display, DRM_MODE_DPMS_OFF);
-
+	exynos_drm_dev_busy(&pdev->dev);
 	exynos_drm_display_unregister(&exynos_dsi_display);
 	mipi_dsi_host_unregister(&dsi->dsi_host);
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
index 0865d5d..ce4d77b 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
@@ -1788,37 +1788,44 @@ static int fimc_probe(struct platform_device *pdev)
 
 	if (!dev->of_node) {
 		dev_err(dev, "device tree node not found.\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto out;
 	}
 
 	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
-	if (!ctx)
-		return -ENOMEM;
+	if (!ctx) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	ctx->ippdrv.dev = dev;
 
 	ret = fimc_parse_dt(ctx);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	ctx->sysreg = syscon_regmap_lookup_by_phandle(dev->of_node,
 						"samsung,sysreg");
 	if (IS_ERR(ctx->sysreg)) {
 		dev_err(dev, "syscon regmap lookup failed.\n");
-		return PTR_ERR(ctx->sysreg);
+		ret = PTR_ERR(ctx->sysreg);
+		goto out;
 	}
 
 	/* resource memory */
 	ctx->regs_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	ctx->regs = devm_ioremap_resource(dev, ctx->regs_res);
-	if (IS_ERR(ctx->regs))
-		return PTR_ERR(ctx->regs);
+	if (IS_ERR(ctx->regs)) {
+		ret = PTR_ERR(ctx->regs);
+		goto out;
+	}
 
 	/* resource irq */
 	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (!res) {
 		dev_err(dev, "failed to request irq resource.\n");
-		return -ENOENT;
+		ret = -ENOENT;
+		goto out;
 	}
 
 	ctx->irq = res->start;
@@ -1826,12 +1833,12 @@ static int fimc_probe(struct platform_device *pdev)
 		IRQF_ONESHOT, "drm_fimc", ctx);
 	if (ret < 0) {
 		dev_err(dev, "failed to request irq.\n");
-		return ret;
+		goto out;
 	}
 
 	ret = fimc_setup_clocks(ctx);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	ippdrv = &ctx->ippdrv;
 	ippdrv->ops[EXYNOS_DRM_OPS_SRC] = &fimc_src_ops;
@@ -1862,12 +1869,15 @@ static int fimc_probe(struct platform_device *pdev)
 
 	dev_info(dev, "drm fimc registered successfully.\n");
 
-	return 0;
+	goto out;
 
 err_pm_dis:
 	pm_runtime_disable(dev);
 err_put_clk:
 	fimc_put_clocks(ctx);
+out:
+	if (ret != -EPROBE_DEFER)
+		exynos_drm_dev_ready(dev);
 
 	return ret;
 }
@@ -1878,6 +1888,7 @@ static int fimc_remove(struct platform_device *pdev)
 	struct fimc_context *ctx = get_fimc_context(dev);
 	struct exynos_drm_ippdrv *ippdrv = &ctx->ippdrv;
 
+	exynos_drm_dev_busy(dev);
 	exynos_drm_ippdrv_unregister(ippdrv);
 	mutex_destroy(&ctx->lock);
 
@@ -1955,4 +1966,3 @@ EXYNOS_DRM_DRV(fimc_driver) = {
 		.pm	= &fimc_pm_ops,
 	},
 };
-
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index a0f5037..8bc37c4 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -857,12 +857,16 @@ static int fimd_probe(struct platform_device *pdev)
 	int win;
 	int ret = -EINVAL;
 
-	if (!dev->of_node)
-		return -ENODEV;
+	if (!dev->of_node) {
+		ret = -ENODEV;
+		goto out;
+	}
 
 	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
-	if (!ctx)
-		return -ENOMEM;
+	if (!ctx) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	ctx->dev = dev;
 	ctx->suspended = true;
@@ -875,32 +879,37 @@ static int fimd_probe(struct platform_device *pdev)
 	ctx->bus_clk = devm_clk_get(dev, "fimd");
 	if (IS_ERR(ctx->bus_clk)) {
 		dev_err(dev, "failed to get bus clock\n");
-		return PTR_ERR(ctx->bus_clk);
+		ret = PTR_ERR(ctx->bus_clk);
+		goto out;
 	}
 
 	ctx->lcd_clk = devm_clk_get(dev, "sclk_fimd");
 	if (IS_ERR(ctx->lcd_clk)) {
 		dev_err(dev, "failed to get lcd clock\n");
-		return PTR_ERR(ctx->lcd_clk);
+		ret = PTR_ERR(ctx->lcd_clk);
+		goto out;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
 	ctx->regs = devm_ioremap_resource(dev, res);
-	if (IS_ERR(ctx->regs))
-		return PTR_ERR(ctx->regs);
+	if (IS_ERR(ctx->regs)) {
+		ret = PTR_ERR(ctx->regs);
+		goto out;
+	}
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "vsync");
 	if (!res) {
 		dev_err(dev, "irq request failed.\n");
-		return -ENXIO;
+		ret = -ENXIO;
+		goto out;
 	}
 
 	ret = devm_request_irq(dev, res->start, fimd_irq_handler,
 							0, "drm_fimd", ctx);
 	if (ret) {
 		dev_err(dev, "irq request failed.\n");
-		return ret;
+		goto out;
 	}
 
 	ctx->driver_data = drm_fimd_get_driver_data(pdev);
@@ -919,13 +928,19 @@ static int fimd_probe(struct platform_device *pdev)
 	for (win = 0; win < WINDOWS_NR; win++)
 		fimd_clear_win(ctx, win);
 
-	return 0;
+out:
+	if (ret != -EPROBE_DEFER)
+		exynos_drm_dev_ready(dev);
+
+	return ret;
 }
 
 static int fimd_remove(struct platform_device *pdev)
 {
 	struct exynos_drm_manager *mgr = platform_get_drvdata(pdev);
 
+	exynos_drm_dev_busy(&pdev->dev);
+
 	exynos_dpi_remove(&pdev->dev);
 
 	exynos_drm_manager_unregister(&fimd_manager);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
index 2eb4676..0dfd23c 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
@@ -1372,13 +1372,17 @@ static int g2d_probe(struct platform_device *pdev)
 	int ret;
 
 	g2d = devm_kzalloc(dev, sizeof(*g2d), GFP_KERNEL);
-	if (!g2d)
-		return -ENOMEM;
+	if (!g2d) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	g2d->runqueue_slab = kmem_cache_create("g2d_runqueue_slab",
 			sizeof(struct g2d_runqueue_node), 0, 0, NULL);
-	if (!g2d->runqueue_slab)
-		return -ENOMEM;
+	if (!g2d->runqueue_slab) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	g2d->dev = dev;
 
@@ -1455,6 +1459,10 @@ err_destroy_workqueue:
 	destroy_workqueue(g2d->g2d_workq);
 err_destroy_slab:
 	kmem_cache_destroy(g2d->runqueue_slab);
+out:
+	if (ret != -EPROBE_DEFER)
+		exynos_drm_dev_ready(dev);
+
 	return ret;
 }
 
@@ -1463,6 +1471,7 @@ static int g2d_remove(struct platform_device *pdev)
 	struct g2d_data *g2d = platform_get_drvdata(pdev);
 
 	cancel_work_sync(&g2d->runqueue_work);
+	exynos_drm_dev_busy(&pdev->dev);
 	exynos_drm_subdrv_unregister(&g2d->subdrv);
 
 	while (g2d->runqueue_node) {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gsc.c b/drivers/gpu/drm/exynos/exynos_drm_gsc.c
index 76b15fd..3ab602e 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gsc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gsc.c
@@ -1659,27 +1659,33 @@ static int gsc_probe(struct platform_device *pdev)
 	int ret;
 
 	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
-	if (!ctx)
-		return -ENOMEM;
+	if (!ctx) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	/* clock control */
 	ctx->gsc_clk = devm_clk_get(dev, "gscl");
 	if (IS_ERR(ctx->gsc_clk)) {
 		dev_err(dev, "failed to get gsc clock.\n");
-		return PTR_ERR(ctx->gsc_clk);
+		ret = PTR_ERR(ctx->gsc_clk);
+		goto out;
 	}
 
 	/* resource memory */
 	ctx->regs_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	ctx->regs = devm_ioremap_resource(dev, ctx->regs_res);
-	if (IS_ERR(ctx->regs))
-		return PTR_ERR(ctx->regs);
+	if (IS_ERR(ctx->regs)) {
+		ret = PTR_ERR(ctx->regs);
+		goto out;
+	}
 
 	/* resource irq */
 	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (!res) {
 		dev_err(dev, "failed to request irq resource.\n");
-		return -ENOENT;
+		ret = -ENOENT;
+		goto out;
 	}
 
 	ctx->irq = res->start;
@@ -1687,7 +1693,7 @@ static int gsc_probe(struct platform_device *pdev)
 		IRQF_ONESHOT, "drm_gsc", ctx);
 	if (ret < 0) {
 		dev_err(dev, "failed to request irq.\n");
-		return ret;
+		goto out;
 	}
 
 	/* context initailization */
@@ -1704,7 +1710,7 @@ static int gsc_probe(struct platform_device *pdev)
 	ret = gsc_init_prop_list(ippdrv);
 	if (ret < 0) {
 		dev_err(dev, "failed to init property list.\n");
-		return ret;
+		goto out;
 	}
 
 	DRM_DEBUG_KMS("id[%d]ippdrv[0x%x]\n", ctx->id, (int)ippdrv);
@@ -1723,10 +1729,14 @@ static int gsc_probe(struct platform_device *pdev)
 
 	dev_info(dev, "drm gsc registered successfully.\n");
 
-	return 0;
+	goto out;
 
 err_ippdrv_register:
 	pm_runtime_disable(dev);
+out:
+	if (ret != -EPROBE_DEFER)
+		exynos_drm_dev_ready(dev);
+
 	return ret;
 }
 
@@ -1736,6 +1746,7 @@ static int gsc_remove(struct platform_device *pdev)
 	struct gsc_context *ctx = get_gsc_context(dev);
 	struct exynos_drm_ippdrv *ippdrv = &ctx->ippdrv;
 
+	exynos_drm_dev_busy(dev);
 	exynos_drm_ippdrv_unregister(ippdrv);
 	mutex_destroy(&ctx->lock);
 
@@ -1805,4 +1816,3 @@ EXYNOS_DRM_DRV(gsc_driver) = {
 		.pm	= &gsc_pm_ops,
 	},
 };
-
diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
index 1393486..bc4338f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
@@ -1840,8 +1840,10 @@ static int ipp_probe(struct platform_device *pdev)
 	int ret;
 
 	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
-	if (!ctx)
-		return -ENOMEM;
+	if (!ctx) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	mutex_init(&ctx->ipp_lock);
 	mutex_init(&ctx->prop_lock);
@@ -1858,7 +1860,8 @@ static int ipp_probe(struct platform_device *pdev)
 	ctx->event_workq = create_singlethread_workqueue("ipp_event");
 	if (!ctx->event_workq) {
 		dev_err(dev, "failed to create event workqueue\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
 
 	/*
@@ -1893,12 +1896,15 @@ static int ipp_probe(struct platform_device *pdev)
 
 	dev_info(dev, "drm ipp registered successfully.\n");
 
-	return 0;
-
+	goto out;
 err_cmd_workq:
 	destroy_workqueue(ctx->cmd_workq);
 err_event_workq:
 	destroy_workqueue(ctx->event_workq);
+out:
+	if (ret != -EPROBE_DEFER)
+		exynos_drm_dev_ready(dev);
+
 	return ret;
 }
 
@@ -1907,6 +1913,7 @@ static int ipp_remove(struct platform_device *pdev)
 	struct ipp_context *ctx = platform_get_drvdata(pdev);
 
 	/* unregister sub driver */
+	exynos_drm_dev_busy(&pdev->dev);
 	exynos_drm_subdrv_unregister(&ctx->subdrv);
 
 	/* remove,destroy ipp idr */
@@ -1982,4 +1989,3 @@ EXYNOS_DRM_DRV(ipp_driver) = {
 		.pm	= &ipp_pm_ops,
 	},
 };
-
diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
index 625fd9b..8d6e6cd 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
@@ -707,17 +707,21 @@ static int rotator_probe(struct platform_device *pdev)
 
 	if (!dev->of_node) {
 		dev_err(dev, "cannot find of_node.\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto out;
 	}
 
 	rot = devm_kzalloc(dev, sizeof(*rot), GFP_KERNEL);
-	if (!rot)
-		return -ENOMEM;
+	if (!rot) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	match = of_match_node(exynos_rotator_match, dev->of_node);
 	if (!match) {
 		dev_err(dev, "failed to match node\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto out;
 	}
 	rot->limit_tbl = (struct rot_limit_table *)match->data;
 
@@ -729,20 +733,22 @@ static int rotator_probe(struct platform_device *pdev)
 	rot->irq = platform_get_irq(pdev, 0);
 	if (rot->irq < 0) {
 		dev_err(dev, "failed to get irq\n");
-		return rot->irq;
+		ret = rot->irq;
+		goto out;
 	}
 
 	ret = devm_request_threaded_irq(dev, rot->irq, NULL,
 			rotator_irq_handler, IRQF_ONESHOT, "drm_rotator", rot);
 	if (ret < 0) {
 		dev_err(dev, "failed to request irq\n");
-		return ret;
+		goto out;
 	}
 
 	rot->clock = devm_clk_get(dev, "rotator");
 	if (IS_ERR(rot->clock)) {
 		dev_err(dev, "failed to get clock\n");
-		return PTR_ERR(rot->clock);
+		ret = PTR_ERR(rot->clock);
+		goto out;
 	}
 
 	pm_runtime_enable(dev);
@@ -771,10 +777,14 @@ static int rotator_probe(struct platform_device *pdev)
 
 	dev_info(dev, "The exynos rotator is probed successfully\n");
 
-	return 0;
+	goto out;
 
 err_ippdrv_register:
 	pm_runtime_disable(dev);
+out:
+	if (ret != -EPROBE_DEFER)
+		exynos_drm_dev_ready(dev);
+
 	return ret;
 }
 
@@ -784,6 +794,7 @@ static int rotator_remove(struct platform_device *pdev)
 	struct rot_context *rot = dev_get_drvdata(dev);
 	struct exynos_drm_ippdrv *ippdrv = &rot->ippdrv;
 
+	exynos_drm_dev_busy(dev);
 	exynos_drm_ippdrv_unregister(ippdrv);
 
 	pm_runtime_disable(dev);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index 976a8fe..bfb01a4 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -593,8 +593,10 @@ static int vidi_probe(struct platform_device *pdev)
 	int ret;
 
 	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
-	if (!ctx)
-		return -ENOMEM;
+	if (!ctx) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	ctx->default_win = 0;
 
@@ -607,20 +609,24 @@ static int vidi_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, &vidi_manager);
 
-	ret = device_create_file(dev, &dev_attr_connection);
-	if (ret < 0)
+	if (device_create_file(dev, &dev_attr_connection) < 0)
 		DRM_INFO("failed to create connection sysfs.\n");
 
 	exynos_drm_manager_register(&vidi_manager);
 	exynos_drm_display_register(&vidi_display);
+	ret = 0;
+out:
+	if (ret != -EPROBE_DEFER)
+		exynos_drm_dev_ready(dev);
 
-	return 0;
+	return ret;
 }
 
 static int vidi_remove(struct platform_device *pdev)
 {
 	struct vidi_context *ctx = platform_get_drvdata(pdev);
 
+	exynos_drm_dev_busy(&pdev->dev);
 	exynos_drm_display_unregister(&vidi_display);
 	exynos_drm_manager_unregister(&vidi_manager);
 
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 3e659e9..7947f37 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -2058,24 +2058,32 @@ static int hdmi_probe(struct platform_device *pdev)
 	struct hdmi_driver_data *drv_data;
 	int ret;
 
-	 if (!dev->of_node)
-		return -ENODEV;
+	 if (!dev->of_node) {
+		ret = -ENODEV;
+		goto out;
+	 }
 
 	pdata = drm_hdmi_dt_parse_pdata(dev);
-	if (!pdata)
-		return -EINVAL;
+	if (!pdata) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	hdata = devm_kzalloc(dev, sizeof(struct hdmi_context), GFP_KERNEL);
-	if (!hdata)
-		return -ENOMEM;
+	if (!hdata) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	mutex_init(&hdata->hdmi_mutex);
 
 	platform_set_drvdata(pdev, &hdmi_display);
 
 	match = of_match_node(hdmi_match_types, dev->of_node);
-	if (!match)
-		return -ENODEV;
+	if (!match) {
+		ret = -ENODEV;
+		goto out;
+	}
 
 	drv_data = (struct hdmi_driver_data *)match->data;
 	hdata->type = drv_data->type;
@@ -2086,35 +2094,41 @@ static int hdmi_probe(struct platform_device *pdev)
 	ret = hdmi_resources_init(hdata);
 	if (ret) {
 		DRM_ERROR("hdmi_resources_init failed\n");
-		return -EINVAL;
+		goto out;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	hdata->regs = devm_ioremap_resource(dev, res);
-	if (IS_ERR(hdata->regs))
-		return PTR_ERR(hdata->regs);
+	if (IS_ERR(hdata->regs)) {
+		ret = PTR_ERR(hdata->regs);
+		goto out;
+	}
 
 	ret = devm_gpio_request(dev, hdata->hpd_gpio, "HPD");
 	if (ret) {
 		DRM_ERROR("failed to request HPD gpio\n");
-		return ret;
+		goto out;
 	}
 
 	/* DDC i2c driver */
 	ddc_node = of_parse_phandle(dev->of_node, "ddc", 0);
 	if (!ddc_node) {
 		DRM_ERROR("Failed to find ddc node in device tree\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto out;
 	}
 	hdata->ddc_adpt = of_find_i2c_adapter_by_node(ddc_node);
 	if (!hdata->ddc_adpt) {
 		DRM_ERROR("Failed to get ddc i2c adapter by node\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto out;
 	}
 
 	/* Not support APB PHY yet. */
-	if (drv_data->is_apb_phy)
-		return -EPERM;
+	if (drv_data->is_apb_phy) {
+		ret = -EPERM;
+		goto out;
+	}
 
 	/* hdmiphy i2c driver */
 	phy_node = of_parse_phandle(dev->of_node, "phy", 0);
@@ -2153,12 +2167,16 @@ static int hdmi_probe(struct platform_device *pdev)
 	hdmi_display.ctx = hdata;
 	exynos_drm_display_register(&hdmi_display);
 
-	return 0;
+	goto out;
 
 err_hdmiphy:
 	put_device(&hdata->hdmiphy_port->dev);
 err_ddc:
 	put_device(&hdata->ddc_adpt->dev);
+out:
+	if (ret != -EPROBE_DEFER)
+		exynos_drm_dev_ready(dev);
+
 	return ret;
 }
 
@@ -2168,6 +2186,7 @@ static int hdmi_remove(struct platform_device *pdev)
 	struct exynos_drm_display *display = get_hdmi_display(dev);
 	struct hdmi_context *hdata = display->ctx;
 
+	exynos_drm_dev_busy(dev);
 	put_device(&hdata->hdmiphy_port->dev);
 	put_device(&hdata->ddc_adpt->dev);
 	pm_runtime_disable(&pdev->dev);
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index b978bde..cca1cbb 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -1205,13 +1205,14 @@ static int mixer_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct mixer_context *ctx;
 	struct mixer_drv_data *drv;
+	int ret;
 
 	dev_info(dev, "probe start\n");
 
 	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx) {
-		DRM_ERROR("failed to alloc mixer context.\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out;
 	}
 
 	mutex_init(&ctx->mixer_mutex);
@@ -1237,12 +1238,18 @@ static int mixer_probe(struct platform_device *pdev)
 	exynos_drm_manager_register(&mixer_manager);
 
 	pm_runtime_enable(dev);
+	ret = 0;
+out:
+	if (ret != -EPROBE_DEFER)
+		exynos_drm_dev_ready(dev);
 
-	return 0;
+	return ret;
 }
 
 static int mixer_remove(struct platform_device *pdev)
 {
+	exynos_drm_dev_busy(&pdev->dev);
+
 	dev_info(&pdev->dev, "remove successful\n");
 
 	pm_runtime_disable(&pdev->dev);
-- 
1.8.3.2

[PATCH RFC 3/3] drm/exynos: use pending_components for components tracking

[Russell King - ARM Linux]

On Thu, Apr 17, 2014 at 01:28:50PM +0200, Andrzej Hajda wrote:
> +out:
> +	if (ret != -EPROBE_DEFER)
> +		exynos_drm_dev_ready(&pdev->dev);

So we end up with everyone needing a "ready" call in each sub-driver
back into the main driver... this makes it impossible to write a
generic subcomponent driver which is not tied in any way to the
main driver.

That is quite some restriction, and would prevent, for example, the
TDA998x driver being used both with Armada DRM, tilcdc and any other
driver.

So, while your solution may work for exynos, it's not suitable for
general use.

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

[PATCH RFC 3/3] drm/exynos: use pending_components for components tracking

[Andrzej Hajda]

Hi Russel,

Thanks for comments.

On 04/17/2014 11:47 PM, Russell King - ARM Linux wrote:
> On Thu, Apr 17, 2014 at 01:28:50PM +0200, Andrzej Hajda wrote:
>> +out:
>> +	if (ret != -EPROBE_DEFER)
>> +		exynos_drm_dev_ready(&pdev->dev);
> So we end up with everyone needing a "ready" call in each sub-driver
> back into the main driver... this makes it impossible to write a
> generic subcomponent driver which is not tied in any way to the
> main driver.
>
> That is quite some restriction, and would prevent, for example, the
> TDA998x driver being used both with Armada DRM, tilcdc and any other
> driver.

As I see in armada driver drm is deferred in case tda998x is not yet
available. The same solution can be still used with pending_devices
approach - the main driver will not report its readiness until tda998x
is present.

Anyway it still seems to be better than componentize every driver which can
probably become a part of some superdevice.

If you want to get rid of deferred probe one can make global list of
'ready' devices with notifications systems for master devices.

Maybe it would be good to consider notification system for devices probe
result,
it will require that driver register all its interfaces in probe, ie its
readiness cannot
be reported later but will not require to add new framework. I hope just
extending current
notification system should be enough.

Regards
Andrzej

[PATCH RFC 3/3] drm/exynos: use pending_components for components tracking

[Russell King - ARM Linux]

On Fri, Apr 18, 2014 at 01:27:53PM +0200, Andrzej Hajda wrote:
> Hi Russel,
> 
> Thanks for comments.
> 
> On 04/17/2014 11:47 PM, Russell King - ARM Linux wrote:
> > On Thu, Apr 17, 2014 at 01:28:50PM +0200, Andrzej Hajda wrote:
> >> +out:
> >> +	if (ret != -EPROBE_DEFER)
> >> +		exynos_drm_dev_ready(&pdev->dev);
> > So we end up with everyone needing a "ready" call in each sub-driver
> > back into the main driver... this makes it impossible to write a
> > generic subcomponent driver which is not tied in any way to the
> > main driver.
> >
> > That is quite some restriction, and would prevent, for example, the
> > TDA998x driver being used both with Armada DRM, tilcdc and any other
> > driver.
> 
> As I see in armada driver drm is deferred in case tda998x is not yet
> available. The same solution can be still used with pending_devices
> approach - the main driver will not report its readiness until tda998x
> is present.
> 
> Anyway it still seems to be better than componentize every driver which can
> probably become a part of some superdevice.
> 
> If you want to get rid of deferred probe one can make global list of
> 'ready' devices with notifications systems for master devices.
> 
> Maybe it would be good to consider notification system for devices probe
> result,
> it will require that driver register all its interfaces in probe, ie its
> readiness cannot
> be reported later but will not require to add new framework. I hope just
> extending current
> notification system should be enough.

You aren't addressing my point.  If I were to convert tda998x to use
your infrastructure, then I would have to add in ifdefs to tie it into
armada DRM _and_ a different set of ifdefs to tie it into tilcdc.  Then
when someone else wanted to use it in their driver, they'd have to add
yet more ifdefs into it to tie it into their driver.

This does not scale.

So, please address my point: in your system, how can a single component
be shared between multiple different master drivers?

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

[PATCH RFC 3/3] drm/exynos: use pending_components for components tracking

[Andrzej Hajda]

Hi Russel,

My answer little bit later due to Easter.

On 04/18/2014 02:42 PM, Russell King - ARM Linux wrote:
> On Fri, Apr 18, 2014 at 01:27:53PM +0200, Andrzej Hajda wrote:
>> Hi Russel,
>>
>> Thanks for comments.
>>
>> On 04/17/2014 11:47 PM, Russell King - ARM Linux wrote:
>>> On Thu, Apr 17, 2014 at 01:28:50PM +0200, Andrzej Hajda wrote:
>>>> +out:
>>>> +	if (ret != -EPROBE_DEFER)
>>>> +		exynos_drm_dev_ready(&pdev->dev);
>>> So we end up with everyone needing a "ready" call in each sub-driver
>>> back into the main driver... this makes it impossible to write a
>>> generic subcomponent driver which is not tied in any way to the
>>> main driver.
>>>
>>> That is quite some restriction, and would prevent, for example, the
>>> TDA998x driver being used both with Armada DRM, tilcdc and any other
>>> driver.
>> As I see in armada driver drm is deferred in case tda998x is not yet
>> available. The same solution can be still used with pending_devices
>> approach - the main driver will not report its readiness until tda998x
>> is present.
>>
>> Anyway it still seems to be better than componentize every driver which can
>> probably become a part of some superdevice.
>>
>> If you want to get rid of deferred probe one can make global list of
>> 'ready' devices with notifications systems for master devices.
>>
>> Maybe it would be good to consider notification system for devices probe
>> result,
>> it will require that driver register all its interfaces in probe, ie its
>> readiness cannot
>> be reported later but will not require to add new framework. I hope just
>> extending current
>> notification system should be enough.
> You aren't addressing my point.  If I were to convert tda998x to use
> your infrastructure, then I would have to add in ifdefs to tie it into
> armada DRM _and_ a different set of ifdefs to tie it into tilcdc.  Then
> when someone else wanted to use it in their driver, they'd have to add
> yet more ifdefs into it to tie it into their driver.
>
> This does not scale.

As I already answered, you should not use 'my' framework for tda998x
driver, you can still use current approach with deferred probe. I am not
sure
why have you used ifdefs in armada, tilcdc also uses tda998x and without
ifdefs.

'My' framework (I think helper library is a better name) was created to
use with
devices which are closely tied together by another framework - case
of some SoC devices.


>
> So, please address my point: in your system, how can a single component
> be shared between multiple different master drivers?
>

I have answered this question above, again. But your question suggests
you want to componentize
also drivers which are shared by different DRMs. How do you want to do it?
- componentize all DRM drivers sharing given driver?
- componentize shared device in a way that it can used by
non-componentized devices? how? I guess it
will be possible but will have some price.

Regards
Andrzej

[PATCH RFC 3/3] drm/exynos: use pending_components for components tracking

[Russell King - ARM Linux]

On Thu, Apr 17, 2014 at 01:28:50PM +0200, Andrzej Hajda wrote:
> +static int exynos_drm_add_blocker(struct device *dev, void *data)
> +{
> +	struct device_driver *drv = data;
> +
> +	if (!platform_bus_type.match(dev, drv))
> +		return 0;
> +
> +	device_lock(dev);
> +	if (!dev->driver)
> +		exynos_drm_dev_busy(dev);
> +	device_unlock(dev);
> +
> +	return 0;
> +}
> +
> +static void exynos_drm_init_blockers(struct device_driver *drv)
> +{
> +	bus_for_each_dev(&platform_bus_type, NULL, drv, exynos_drm_add_blocker);
> +}

This feels very wrong to be dumping the above code into every driver which
wants to make use of some kind of componentised support.

You also appear to need to know the struct device_driver's for every
component.  While that may work for exynos, it doesn't work elsewhere
where the various components of the system are very real separate
kernel modules - for example, a separate I2C driver such as the TDA998x
case I mentioned in my first reply.

I can't see how your solution would be usable in that circumstance.

The third issue I have is that you're still needing to have internal
exynos sub-device management - you're having to add the individual
devices to some kind of list, array or static data, and during DRM
probe you're having to then walk these lists/arrays/static data to
locate these sub-devices and finish each of their individual
initialisations.  So you're ending up with a two-tier initialisation.

That's not particularly good because it means you're exposed to
problems where the state is different between two initialisations -
when the device is recreated, your component attempts to re-finalise
the initialisation a second time.  It wouldn't take much for a field
to be assumed to be zero at init time somewhere for a bug to creep
in.

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

[PATCH RFC 3/3] drm/exynos: use pending_components for components tracking

[Andrzej Hajda]

On 04/18/2014 12:04 AM, Russell King - ARM Linux wrote:
> On Thu, Apr 17, 2014 at 01:28:50PM +0200, Andrzej Hajda wrote:
>> +static int exynos_drm_add_blocker(struct device *dev, void *data)
>> +{
>> +	struct device_driver *drv = data;
>> +
>> +	if (!platform_bus_type.match(dev, drv))
>> +		return 0;
>> +
>> +	device_lock(dev);
>> +	if (!dev->driver)
>> +		exynos_drm_dev_busy(dev);
>> +	device_unlock(dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static void exynos_drm_init_blockers(struct device_driver *drv)
>> +{
>> +	bus_for_each_dev(&platform_bus_type, NULL, drv, exynos_drm_add_blocker);
>> +}
> This feels very wrong to be dumping the above code into every driver which
> wants to make use of some kind of componentised support.
>
> You also appear to need to know the struct device_driver's for every
> component.  While that may work for exynos, it doesn't work elsewhere
> where the various components of the system are very real separate
> kernel modules - for example, a separate I2C driver such as the TDA998x
> case I mentioned in my first reply.
>
> I can't see how your solution would be usable in that circumstance.

It is up to the master driver how it want to create list of required
devices,
this is why I put it into exynos_drm and not into the framework.
You can use superdevice DT node for it, fixed list whatever you want.
It is not a part of the framework, it is just part of exynos_drm specific
implementation.
Component framework also does not provide mechanism for it.

Regarding TDA998x I have replied in the previous e-mail.

>
> The third issue I have is that you're still needing to have internal
> exynos sub-device management - you're having to add the individual
> devices to some kind of list, array or static data, and during DRM
> probe you're having to then walk these lists/arrays/static data to
> locate these sub-devices and finish each of their individual
> initialisations.  So you're ending up with a two-tier initialisation.
>
> That's not particularly good because it means you're exposed to
> problems where the state is different between two initialisations -
> when the device is recreated, your component attempts to re-finalise
> the initialisation a second time.  It wouldn't take much for a field
> to be assumed to be zero at init time somewhere for a bug to creep
> in.
>

Separation of the interfaces exposed by the device from the device itself
seems to me a good thing. I would even consider it as a biggest
advantage of this solution :)

The problem of re-initialization does not seems to be relevant here, it
is classic
problem of coding correctness nothing more, it can appear here and in
many different
places.

Anyway it seems we have different point of view on the problem, your say
about
devices with two stage initialization. I see it more as devices
registering interfaces and superdevice
using it.

Regards
Andrzej

[PATCH RFC 3/3] drm/exynos: use pending_components for components tracking

[Russell King - ARM Linux]

On Fri, Apr 18, 2014 at 02:02:37PM +0200, Andrzej Hajda wrote:
> Separation of the interfaces exposed by the device from the device itself
> seems to me a good thing. I would even consider it as a biggest
> advantage of this solution :)
> 
> The problem of re-initialization does not seems to be relevant here, it
> is classic
> problem of coding correctness nothing more, it can appear here and in
> many different
> places.

It may be a problem of coding correctless, but it's also a maintainability
problem too - it makes it _much_ more difficult to ensure that everything
is correct.

> Anyway it seems we have different point of view on the problem, your say
> about devices with two stage initialization. I see it more as devices
> registering interfaces and superdevice using it.

Right, so please make this exynos-specific, because from what I can see it
has no reason to pretend to be generic.  As I've already pointed out, it
can't be used in the general case because it ties sub-components directly
to their main driver, which is absolutely absurd.  Please keep this
absurdness in exynos and don't spread it around.  Thanks.

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

[PATCH RFC 3/3] drm/exynos: use pending_components for components tracking

[Andrzej Hajda]

On 04/18/2014 02:46 PM, Russell King - ARM Linux wrote:
> On Fri, Apr 18, 2014 at 02:02:37PM +0200, Andrzej Hajda wrote:
>> Separation of the interfaces exposed by the device from the device itself
>> seems to me a good thing. I would even consider it as a biggest
>> advantage of this solution :)
>>
>> The problem of re-initialization does not seems to be relevant here, it
>> is classic
>> problem of coding correctness nothing more, it can appear here and in
>> many different
>> places.
> It may be a problem of coding correctless, but it's also a maintainability
> problem too - it makes it _much_ more difficult to ensure that everything
> is correct.

But forcibly re-initializing all component devices instead of fixing bugs
in specific drivers seems to be 'absolutely absurd' as classic says.
>> Anyway it seems we have different point of view on the problem, your say
>> about devices with two stage initialization. I see it more as devices
>> registering interfaces and superdevice using it.
> Right, so please make this exynos-specific, because from what I can see it
> has no reason to pretend to be generic.  As I've already pointed out, it
> can't be used in the general case because it ties sub-components directly
> to their main driver, which is absolutely absurd.  Please keep this
> absurdness in exynos and don't spread it around.  Thanks.

As I wrote already, this framework was proposed for drivers which
are tied together anyway, and this is case of many drivers, not only exynos.
Standalone drivers were not at my sight but I have also described in
other mail
how the framework can be 'improved' to support standalone drivers also.

Regards
Andrzej

>

[PATCH RFC 3/3] drm/exynos: use pending_components for components tracking

[Russell King - ARM Linux]

On Tue, Apr 22, 2014 at 01:29:54PM +0200, Andrzej Hajda wrote:
> On 04/18/2014 02:46 PM, Russell King - ARM Linux wrote:
> > On Fri, Apr 18, 2014 at 02:02:37PM +0200, Andrzej Hajda wrote:
> >> Separation of the interfaces exposed by the device from the device itself
> >> seems to me a good thing. I would even consider it as a biggest
> >> advantage of this solution :)
> >>
> >> The problem of re-initialization does not seems to be relevant here, it
> >> is classic
> >> problem of coding correctness nothing more, it can appear here and in
> >> many different
> >> places.
> > It may be a problem of coding correctless, but it's also a maintainability
> > problem too - it makes it _much_ more difficult to ensure that everything
> > is correct.
> 
> But forcibly re-initializing all component devices instead of fixing bugs
> in specific drivers seems to be 'absolutely absurd' as classic says.

They're *unnecessary* bugs that wouldn't even exist if it weren't for
the forced-splitup of the initialisation into two separate parts that
your approach mandates.

Yes, I know that you're desperate to play that down, but you can't get
away from this fact: your approach _forces_ a split up of the
initialisation into dependent two stages and that fact _alone_ adds
additional complexity, and along with that additional complexity comes
more opportunity for bugs.  Also with that additional complexity comes
the need to perform more tests to find those bugs, and given that most
people just say "okay, it boots and seems to work, that's good enough
for me" there is a high possibility that these kinds of bugs will take
a long time to find.

> As I wrote already, this framework was proposed for drivers which
> are tied together anyway, and this is case of many drivers, not
> only exynos.

Please name them.

> Standalone drivers were not at my sight but I have also described in
> other mail how the framework can be 'improved' to support standalone
> drivers also.

At the moment, I don't see a justification for your "simplified"
and restrictive solution, which if used will lock drivers into that
simplisitic method, and which can't ever be extended without lots of
ifdeffery to having other components (such as TDA998x) attached.

My objections are entirely based on where imx-drm and armada DRM are
going, neither of which could ever use your implementation.

Before you say that it isn't meant to deal with stuff like the TDA998x,
take a moment to think about this - the Dove video subsystem was
designed to support OLPC.  It was primerly designed to drive a LCD
screen plus an on-board VGA DAC.  Everything for that is on-SoC.  With
that, the hardware is well known, and your solution could be used.

However, then SolidRun came along and dropped a TDA998x on the LCD output
pins.  Suddenly, things aren't that simple, and your solution falls
apart, because it can't cope with a generic component that has no knowledge
of the rest of its "master".

This kind of scenario can happen /any/ time, and any time it does happen,
your simple solution falls apart.

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

[PATCH RFC 3/3] drm/exynos: use pending_components for components tracking

[Andrzej Hajda]

On 04/22/2014 01:51 PM, Russell King - ARM Linux wrote:
> On Tue, Apr 22, 2014 at 01:29:54PM +0200, Andrzej Hajda wrote:
>> On 04/18/2014 02:46 PM, Russell King - ARM Linux wrote:
>>> On Fri, Apr 18, 2014 at 02:02:37PM +0200, Andrzej Hajda wrote:
>>>> Separation of the interfaces exposed by the device from the device itself
>>>> seems to me a good thing. I would even consider it as a biggest
>>>> advantage of this solution :)
>>>>
>>>> The problem of re-initialization does not seems to be relevant here, it
>>>> is classic
>>>> problem of coding correctness nothing more, it can appear here and in
>>>> many different
>>>> places.
>>> It may be a problem of coding correctless, but it's also a maintainability
>>> problem too - it makes it _much_ more difficult to ensure that everything
>>> is correct.
>> But forcibly re-initializing all component devices instead of fixing bugs
>> in specific drivers seems to be 'absolutely absurd' as classic says.
> They're *unnecessary* bugs that wouldn't even exist if it weren't for
> the forced-splitup of the initialisation into two separate parts that
> your approach mandates.
>
> Yes, I know that you're desperate to play that down, but you can't get

Not true. I only try to find the best solution and the approach with
multiple re-probing just to avoid potential bugs in drivers does not
look to me correct.

> away from this fact: your approach _forces_ a split up of the
> initialisation into dependent two stages and that fact _alone_ adds
> additional complexity, and along with that additional complexity comes
> more opportunity for bugs. 

This sound so funny, just look at componentize patches - every patch
adds two stage initialization for every component and the master,
with forced unwinding and two levels of devres management.

'My approach' adds only one call to probe and one call to remove of
components,
and very simple and straightforward interface to the master.

'My approach' is very standard - during probe driver probes hardware,
and registers interfaces which can be used by other drivers, for example
by drm master. The only addition is reporting its readiness. Comparing to
'your approach' it is bloody simple.


>  Also with that additional complexity comes
> the need to perform more tests to find those bugs, and given that most
> people just say "okay, it boots and seems to work, that's good enough
> for me" there is a high possibility that these kinds of bugs will take
> a long time to find.

Volume of changes for each component and drm device management
dispersed on all components makes your argument very valid for
component subsystem.

Btw have you observed component framework when one of the components
during bind returns -EPROBE_DEFER ? In my tests it resulted in
deferred probing of master and unbind/bind of other components.
So lets say you have N components and the last component will be deferred
K times, it results in:
- K times deferring of the last component and the master,
- (N - 1) * K - unbinds and binds of other components.


>
>> As I wrote already, this framework was proposed for drivers which
>> are tied together anyway, and this is case of many drivers, not
>> only exynos.
> Please name them.
>
>> Standalone drivers were not at my sight but I have also described in
>> other mail how the framework can be 'improved' to support standalone
>> drivers also.
> At the moment, I don't see a justification for your "simplified"
> and restrictive solution, which if used will lock drivers into that
> simplisitic method, and which can't ever be extended without lots of
> ifdeffery to having other components (such as TDA998x) attached.
>
> My objections are entirely based on where imx-drm and armada DRM are
> going, neither of which could ever use your implementation.
>
> Before you say that it isn't meant to deal with stuff like the TDA998x,
> take a moment to think about this - the Dove video subsystem was
> designed to support OLPC.  It was primerly designed to drive a LCD
> screen plus an on-board VGA DAC.  Everything for that is on-SoC.  With
> that, the hardware is well known, and your solution could be used.
>
> However, then SolidRun came along and dropped a TDA998x on the LCD output
> pins.  Suddenly, things aren't that simple, and your solution falls
> apart, because it can't cope with a generic component that has no knowledge
> of the rest of its "master".
>
> This kind of scenario can happen /any/ time, and any time it does happen,
> your simple solution falls apart.

I think I have answered you two or three times that it is not a problem
to remove
'glued drivers' restriction. I desperately try to avoid accusing you for
'desperately
playing down' on this subject, so I will not comment this anymore.

On the other hand you have not answered quite important question - how
do you plan to componentize drivers shared by different drms when
one of drms is not componentized???


Regards
Andrzej

[PATCH RFC 3/3] drm/exynos: use pending_components for components tracking

[Russell King - ARM Linux]

On Wed, Apr 23, 2014 at 05:04:46PM +0200, Andrzej Hajda wrote:
> On 04/22/2014 01:51 PM, Russell King - ARM Linux wrote:
> > Yes, I know that you're desperate to play that down, but you can't get
> 
> Not true. I only try to find the best solution and the approach with
> multiple re-probing just to avoid potential bugs in drivers does not
> look to me correct.
> 
> > away from this fact: your approach _forces_ a split up of the
> > initialisation into dependent two stages and that fact _alone_ adds
> > additional complexity, and along with that additional complexity comes
> > more opportunity for bugs. 
> 
> This sound so funny, just look at componentize patches - every patch
> adds two stage initialization for every component and the master,
> with forced unwinding and two levels of devres management.

*Sigh*.  Why am I bothering discussing this with you.

*NO* it does not, for the VERY SIMPLE reason that NOTHING is done before
the BIND.  NO structures are allocated.  NOTHING is setup.  The *only*
thing that is done is the driver registers with the component helper.

That's not two stage initialisation.  That's *single* stage.

> 'My approach' adds only one call to probe and one call to remove of
> components, and very simple and straightforward interface to the master.

You're talking utter garbage there.

> 'My approach' is very standard - during probe driver probes hardware,
> and registers interfaces which can be used by other drivers, for example
> by drm master. The only addition is reporting its readiness. Comparing to
> 'your approach' it is bloody simple.

More unbelievable crap.

> >  Also with that additional complexity comes
> > the need to perform more tests to find those bugs, and given that most
> > people just say "okay, it boots and seems to work, that's good enough
> > for me" there is a high possibility that these kinds of bugs will take
> > a long time to find.
> 
> Volume of changes for each component and drm device management
> dispersed on all components makes your argument very valid for
> component subsystem.
> 
> Btw have you observed component framework when one of the components
> during bind returns -EPROBE_DEFER ? In my tests it resulted in
> deferred probing of master and unbind/bind of other components.
> So lets say you have N components and the last component will be deferred
> K times, it results in:
> - K times deferring of the last component and the master,
> - (N - 1) * K - unbinds and binds of other components.

True, and you can't get away from that with proper behaviour.

> >> As I wrote already, this framework was proposed for drivers which
> >> are tied together anyway, and this is case of many drivers, not
> >> only exynos.
> > Please name them.

You ignored this.  Therefore, I assume that you *can't* name them because
there *aren't* any.  I called your bluff, I win.

> > At the moment, I don't see a justification for your "simplified"
> > and restrictive solution, which if used will lock drivers into that
> > simplisitic method, and which can't ever be extended without lots of
> > ifdeffery to having other components (such as TDA998x) attached.
> >
> > My objections are entirely based on where imx-drm and armada DRM are
> > going, neither of which could ever use your implementation.
> >
> > Before you say that it isn't meant to deal with stuff like the TDA998x,
> > take a moment to think about this - the Dove video subsystem was
> > designed to support OLPC.  It was primerly designed to drive a LCD
> > screen plus an on-board VGA DAC.  Everything for that is on-SoC.  With
> > that, the hardware is well known, and your solution could be used.
> >
> > However, then SolidRun came along and dropped a TDA998x on the LCD output
> > pins.  Suddenly, things aren't that simple, and your solution falls
> > apart, because it can't cope with a generic component that has no knowledge
> > of the rest of its "master".
> >
> > This kind of scenario can happen /any/ time, and any time it does happen,
> > your simple solution falls apart.
> 
> I think I have answered you two or three times that it is not a problem
> to remove
> 'glued drivers' restriction. I desperately try to avoid accusing you for
> 'desperately
> playing down' on this subject, so I will not comment this anymore.

Right, so what I draw from this is that *you* again refuse to answer this
point because despite your assertions that your solution can do it, you
have no clue as to *how* it can be done.  I've looked at your solution
with respect to this, and I *can't* see how it can be done either.  That's
why I've been asking *you* the question, so that *you* can provide some
technical input to it.

> On the other hand you have not answered quite important question - how
> do you plan to componentize drivers shared by different drms when
> one of drms is not componentized???

Read this, from a message I sent at the beginning of February:
| Here's my changes to the TDA998x driver to add support for the component
| helper.  The TDA998x driver retains support for the old way so that
| drivers can be transitioned.  For any one DRM "card" the transition to
| using the component layer must be all-in or all-out - partial transitions
| are not permitted with the simple locking implementation currently in
| the component helper due to the possibility of deadlock.  (Master
| binds, holding the component lock, master declares i2c device, i2c
| device is bound to tda998x, which tries to register with the component
| layer, trying to take the held lock.)
| 
| http://ftp.arm.linux.org.uk/cgit/linux-cubox.git/log/?h=unstable/tda998x-devel

It would appear that I've already converted a driver there into a
structure where it can exist as either a componentised device, _or_
it can exist as a DRM slave device.

Ergo, your claim that I haven't answered this question is... interesting
because I seem to have an implementation dated over two months ago.

So, maybe you would like to finally address *my* point about TDA998x
and your solution in a way that provides a satisfactory answer.  *Show*
how it can be done, or *outline* how it can be done.

If you can't or won't do either of those, I shall continue to regard your
solution as highly sub-optimal and only suitable for exynos, and continue
to request that it should live in drivers/gpu/drm/exynos and not
drivers/base.

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

[PATCH RFC 3/3] drm/exynos: use pending_components for components tracking

[Russell King - ARM Linux]

On Wed, Apr 23, 2014 at 05:43:28PM +0100, Russell King - ARM Linux wrote:
> So, maybe you would like to finally address *my* point about TDA998x
> and your solution in a way that provides a satisfactory answer.  *Show*
> how it can be done, or *outline* how it can be done.

Let me be absolutely clear *why* I'm very interested in this - and that
is because I'm presently converting TDA998x and Armada DRM to use the
component helpers.  If your solution is better, then I'd want to convert
to that instead, and let's retire the component helpers.

At the moment, my belief is that your solution is *very* substandard and
suboptimal precisely for the reasons I've outlined, especially when it
comes to sharing a component between several drivers.

So, if you *really* care, you'll stop fobbing me off on this point and
provide some real technical input how you'd see your solution being used
in exactly the scenario that I've been outlining several times in this
thread.

For example, you could show what kind of modifications you expect would
be required to the _existing_ TDA998x driver to allow it to participate
as a device declared in DT as an entirely separate entity, probed via the
standard I2C probing methods, and then hook itself into the appropriate
DRM driver.  Remembering, of course, that the TDA998x device is used by
more than _just_ Armada DRM.

I don't care if you show it via pseudo-code or by real patch.  I just
want to know _how_ your solution could be used.  And I won't want some
silly remark like "trivially" or "I've already answered that."  I want
_you_ to _show_ _how_ it can be done.

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

[PATCH RFC 3/3] drm/exynos: use pending_components for components tracking

[Andrzej Hajda]

On 04/23/2014 07:13 PM, Russell King - ARM Linux wrote:
> On Wed, Apr 23, 2014 at 05:43:28PM +0100, Russell King - ARM Linux wrote:
>> So, maybe you would like to finally address *my* point about TDA998x
>> and your solution in a way that provides a satisfactory answer.  *Show*
>> how it can be done, or *outline* how it can be done.
> 
> Let me be absolutely clear *why* I'm very interested in this - and that
> is because I'm presently converting TDA998x and Armada DRM to use the
> component helpers.  If your solution is better, then I'd want to convert
> to that instead, and let's retire the component helpers.
> 
> At the moment, my belief is that your solution is *very* substandard and
> suboptimal precisely for the reasons I've outlined, especially when it
> comes to sharing a component between several drivers.
> 
> So, if you *really* care, you'll stop fobbing me off on this point and
> provide some real technical input how you'd see your solution being used
> in exactly the scenario that I've been outlining several times in this
> thread.
> 
> For example, you could show what kind of modifications you expect would
> be required to the _existing_ TDA998x driver to allow it to participate
> as a device declared in DT as an entirely separate entity, probed via the
> standard I2C probing methods, and then hook itself into the appropriate
> DRM driver.  Remembering, of course, that the TDA998x device is used by
> more than _just_ Armada DRM.
> 
> I don't care if you show it via pseudo-code or by real patch.  I just
> want to know _how_ your solution could be used.  And I won't want some
> silly remark like "trivially" or "I've already answered that."  I want
> _you_ to _show_ _how_ it can be done.
> 

Thats good, constructive discussion is better.

I have no experience with tda998x, armada and drm_encoder_slave, so
please be kind if I make some mistakes regarding them.
I will try to show the complete solution starting from the current state
of tda998x and armada in linux-next. I will present solution for DT as
it is more standardized than board files. I hope it should not be a
problem to port it back to board files if necessary.

First I will try to show how to get rid of ifdefs in armada_drv.c, it is
not a part of my proposition but you have emphasized it.

1. tda998x have already minimal DT support, so we can create proper i2c
client device node with "nxp,tda998x" compatible.

2. There are tda998x_encoder_params defined in armada_drv.c they should
be removed from armada and added to tda998x node, its parsing to tda
driver should be added as well.

3. In armada_drv.c there is armada_drm_slave_config specific for tda. I
am not sure of all its content but it seems it could be mapped to DT
video interface.
So if in armada_drm node there will be port node it means there
is something connected to the output and it uses drm_encoder_slave
interface, if there is no port node there is no encoder.
Sample bindings:

armada_drm {
	...
	port {
		drm_ep: endpoint {
			remote-endpoint = <&tda_ep>;
			crtcs = <1>;
			poll_connect;
			poll_disconnect;
			interlace_allowed;
		};
	};
};

...

i2c_x {
	tda at 70 {
		reg = <0x70>;
		compatible = "nxp,tda998x";
		swap_a = ...;
		swap_b = ...;
		...
		port {
			endpoint {
				remote-endpoint = <&drm_ep>;
			};
		};
	};
};

4. In armada_drm call of drm_i2c_encoder_init should be replaced
by sth like:
	client = of_find_i2c_device_by_node(dev node containing drm_ep phandle);
	device_lock(&client->dev);
	if (!client->dev.driver) {
		ret = -EPROBE_DEFER;
		goto unlock;
	}
	module_get(client->dev.driver->owner);
	encoder_drv = to_drm_i2c_encoder_driver(to_i2c_driver(client->dev.driver));
	encoder_drv->encoder_init(client, dev, encoder);
unlock:
	device_unlock(&client->dev);

Similar change should be done to destroy code.
Of course please consider this code as a draft.
	
All the above is just for removing ifdefs from armada_drv.c, it
is not really connected with my proposition.

It is not really safe, and I am not sure where exactly the locking
should be performed. For sure it can crash when unbind will be called
via sysfs property, but it seems at least as safe as
drm_i2c_encoder_init or it should be possible to make it such.
And it allows to attach to armada theoretically any hardware compatible
encoder having drm_i2c_encoder interface.

What do you think about above steps? Is it OK for you?

And now about my proposition for device probe order issue. My first
answer to your objections about 'glue problem' was to make global list
of 'ready' devices instead of the one glued to the main drm driver.
Over time my idea evolved.
My proposition is still in the form of adding one call to component
probe for reporting device readiness and one symmetrical call to remove.

It seems I can simply reuse notification framework which I am working
on. I will send RFC next week and I will accompany it with its usage for
solving drmdev problem.


Regards
Andrzej

[PATCH RFC 3/3] drm/exynos: use pending_components for components tracking

[Russell King - ARM Linux]

On Fri, Apr 25, 2014 at 04:36:01PM +0200, Andrzej Hajda wrote:
> On 04/23/2014 07:13 PM, Russell King - ARM Linux wrote:
> > Let me be absolutely clear *why* I'm very interested in this - and that
> > is because I'm presently converting TDA998x and Armada DRM to use the
> > component helpers.  If your solution is better, then I'd want to convert
> > to that instead, and let's retire the component helpers.
> > 
> > At the moment, my belief is that your solution is *very* substandard and
> > suboptimal precisely for the reasons I've outlined, especially when it
> > comes to sharing a component between several drivers.
> > 
> > So, if you *really* care, you'll stop fobbing me off on this point and
> > provide some real technical input how you'd see your solution being used
> > in exactly the scenario that I've been outlining several times in this
> > thread.
> > 
> > For example, you could show what kind of modifications you expect would
> > be required to the _existing_ TDA998x driver to allow it to participate
> > as a device declared in DT as an entirely separate entity, probed via the
> > standard I2C probing methods, and then hook itself into the appropriate
> > DRM driver.  Remembering, of course, that the TDA998x device is used by
> > more than _just_ Armada DRM.
> > 
> > I don't care if you show it via pseudo-code or by real patch.  I just
> > want to know _how_ your solution could be used.  And I won't want some
> > silly remark like "trivially" or "I've already answered that."  I want
> > _you_ to _show_ _how_ it can be done.
> > 
> 
> Thats good, constructive discussion is better.

Well, the above is the _same_ fscking question I've been asking you
all along and that you have failed to answer properly until now.
Unfortunately, your reply still doesn't answer my concerns.

> I have no experience with tda998x, armada and drm_encoder_slave, so
> please be kind if I make some mistakes regarding them.
> I will try to show the complete solution starting from the current state
> of tda998x and armada in linux-next. I will present solution for DT as
> it is more standardized than board files. I hope it should not be a
> problem to port it back to board files if necessary.
> 
> First I will try to show how to get rid of ifdefs in armada_drv.c, it is
> not a part of my proposition but you have emphasized it.
> 
> 1. tda998x have already minimal DT support, so we can create proper i2c
> client device node with "nxp,tda998x" compatible.
> 
> 2. There are tda998x_encoder_params defined in armada_drv.c they should
> be removed from armada and added to tda998x node, its parsing to tda
> driver should be added as well.
> 
> 3. In armada_drv.c there is armada_drm_slave_config specific for tda. I
> am not sure of all its content but it seems it could be mapped to DT
> video interface.
> So if in armada_drm node there will be port node it means there
> is something connected to the output and it uses drm_encoder_slave
> interface, if there is no port node there is no encoder.
> Sample bindings:
> 
> armada_drm {
> 	...
> 	port {
> 		drm_ep: endpoint {
> 			remote-endpoint = <&tda_ep>;
> 			crtcs = <1>;
> 			poll_connect;
> 			poll_disconnect;
> 			interlace_allowed;
> 		};
> 	};
> };
> 
> ...
> 
> i2c_x {
> 	tda at 70 {
> 		reg = <0x70>;
> 		compatible = "nxp,tda998x";
> 		swap_a = ...;
> 		swap_b = ...;
> 		...
> 		port {
> 			endpoint {
> 				remote-endpoint = <&drm_ep>;
> 			};
> 		};
> 	};
> };
> 
> 4. In armada_drm call of drm_i2c_encoder_init should be replaced
> by sth like:
> 	client = of_find_i2c_device_by_node(dev node containing drm_ep phandle);
> 	device_lock(&client->dev);
> 	if (!client->dev.driver) {
> 		ret = -EPROBE_DEFER;
> 		goto unlock;
> 	}
> 	module_get(client->dev.driver->owner);
> 	encoder_drv = to_drm_i2c_encoder_driver(to_i2c_driver(client->dev.driver));
> 	encoder_drv->encoder_init(client, dev, encoder);
> unlock:
> 	device_unlock(&client->dev);
> 
> Similar change should be done to destroy code.
> Of course please consider this code as a draft.

This looks reasonable - and I'm sure we can easily work around the
situation when there's no i2c encoder.  There is one thing which concerns
me about the above - that module_get() call.  module_get() only saves you
from the module being unloaded, which really isn't an issue if the
problem with the device going away is solved.

If you think about this for a moment, you'll understand why - in order for
the module to be unloaded, the device driver must be first unbound from
any devices.  So:

(a) you need to prevent the device driver being unbound while the DRM device
is being brought up, and
(b) you need to bring the DRM device down if the device driver is unbound.

Those are the two difficult problems which need solving.

The other thing which I notice from what you've illustrated above is that
you're not using your framework for this.

> All the above is just for removing ifdefs from armada_drv.c, it
> is not really connected with my proposition.

In any case, these aren't the ifdefs I was talking about.  Let me try
yet again.

In the exynos component drivers, you have this in each of their probe
functions:

	exynos_drm_dev_ready(&pdev->dev);

This is implemented by the core exynos driver, and this ties each
component to the core exynos driver.  The reason you do this is because
you need to get the handle to the "blockers" specific to the "master"
device:

+int exynos_drm_dev_ready(struct device *dev)
+{
+       return pending_components_delete(&exynos_drm_blockers, dev);
+}

Now, let's translate this to the TDA998x/Armada DRM setup.  We first
need a list of blockers for Armada DRM, let's call it armada_drm_blockers.
We then need to provide a similar function to the above:

int armada_drm_dev_ready(struct device *dev)
{
       return pending_components_delete(&armada_drm_blockers, dev);
}

And now we need TDA998x to call that to indicate that it's ready.  So
in the TDA998x probe function, we would end up with a call to:

	armada_drm_dev_ready(&client->dev);

And this is where my concerns about this approach start - by doing so,
the driver is now tied directly to Armada DRM, where it wasn't hard-tied
before.  This is a problem, because it's used by more drivers than just
Armada DRM - tilcdc as an example.

So, the above then needs to become:

#ifdef CONFIG_DRM_ARMADA
	armada_drm_dev_ready(&client->dev);
#endif

Now, if/when tilcdc starts to take on a similar structure, it too grows
one of these "dev_ready" calls, let's call it tilcdc_dev_ready().  Now
we need tda998x to do this:

#ifdef CONFIG_DRM_ARMADA
	armada_drm_dev_ready(&client->dev);
#endif
#ifdef CONFIG_DRM_TILCDC
	tilcdc_dev_ready(&client->dev);
#endif

And these are the ifdef's I've been talking about all along.  This presents
two problems:

1) these ifdefs should not be necessary,
2) each ifdef, when enabled, adds a hard reference between tda998x and
   it's host driver.

Now consider the effect that this has if we build a multi-platform kernel
which includes both tilcdc and Armada DRM as modules.  The first issue
here is that if _either_ of these are built as modules, tda998x must
also be built as modules.  So we need to come up with some kind of
Kconfig language to express this dependency.

The second problem is that in order to load the tda998x module, we must
have both Armada DRM and tilcdc both loaded in this situation (because of
the direct references to those modules.)  Many people would find that
unacceptable (I find it unacceptable that drivers for hardware that do
not exist would need to be loaded.)

> Over time my idea evolved.
> My proposition is still in the form of adding one call to component
> probe for reporting device readiness and one symmetrical call to remove.
> 
> It seems I can simply reuse notification framework which I am working
> on. I will send RFC next week and I will accompany it with its usage for
> solving drmdev problem.

So... how do you know which "master" a component belongs to in order to
report its readiness?  How do you get the "master" to be re-probed when
that component becomes ready?

Bear in mind that while the component device's probe function is running,
the device is locked, which means the device will be locked when your
call reporting device readiness will be made.  If you trigger a call
at that point to try and bind the master, you'll deadlock when the driver
code (such as you've shown above) also tries to take the device lock.

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