Skip to contentSkip to conversation

rockchip: kevin: Enable edp display

PATCH Applied Thierry Escande 3 participants 18 messages Jan 10, 2018 – Mar 2, 2018

Subsystems (12)
DRM drivers DRM misc Arm i.MX and Layerscape Arm/Amlogic Meson SoC support +8 more subsystems
lore
Share & tools
lore

Thierry Escande

author
#0 lore raw

v8 cover rockchip: kevin: Enable edp display

Hi,

This patchset makes edp display work on Chromebook kevin.

This patchset has been originally posted by Jeffy Chen and the 2 first commits from a previous version (v6) are already merged in mainline.

v8:
Fixed compilation issue between patches #7 and #8 reported by Archit.

v7:
Rebased on top of next-20180108 and fixed conflicts Fixed a few warnings reported by checkpatch

Jeffy Chen (8):
drm/bridge: analogix: Do not use device's drvdata drm/bridge: analogix_dp: Fix connector and encoder cleanup drm/rockchip: analogix_dp: Add a sanity check for
rockchip_drm_psr_register()
drm/rockchip: dw-mipi-dsi: Fix error handling path drm/rockchip: inno_hdmi: Fix error handling path drm/bridge/synopsys: dw-hdmi: Add missing bridge detach drm/bridge/synopsys: dw-hdmi: Do not use device's drvdata drm/rockchip: dw_hdmi: Fix error handling path

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 52 +++++++----------
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          | 53 ++++++++---------
 drivers/gpu/drm/exynos/exynos_dp.c                 | 29 ++++++----
 drivers/gpu/drm/imx/dw_hdmi-imx.c                  | 22 ++++---
 drivers/gpu/drm/meson/meson_dw_hdmi.c              | 20 +++++--
 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c             | 14 ++++-
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 67 +++++++++++++---------
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c             | 21 ++++---
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        | 39 +++++++------
 drivers/gpu/drm/rockchip/inno_hdmi.c               | 22 +++++--
 include/drm/bridge/analogix_dp.h                   | 19 +++---
 include/drm/bridge/dw_hdmi.h                       | 17 +++---
 12 files changed, 214 insertions(+), 161 deletions(-)
Signature · 1 line
2.14.1
author
#1 lore raw

PATCH v8 1/8 drm/bridge: analogix: Do not use device's drvdata

From: Jeffy Chen <[email protected]>

The driver that instantiates the bridge should own the drvdata, as all driver model callbacks (probe, remove, shutdown, PM ops, etc.) are also owned by its driver struct. Moreover, storing two different pointer types in driver data depending on driver initialization status is barely a good practice and in fact has led to many bugs in this driver.

Let's clean up this mess and change Analogix entry points to simply accept some opaque struct pointer, adjusting their users at the same time to avoid breaking the compilation.

Signed-off-by Tomasz Figa <[email protected]>
Signed-off-by Jeffy Chen <[email protected]>
Signed-off-by Thierry Escande <thierry.escande-ZGY8ohtN/[email protected]>
Reviewed-by Andrzej Hajda <[email protected]>
Reviewed-by Sean Paul <[email protected]>
Acked-by Jingoo Han <[email protected]>
Acked-by Archit Taneja <[email protected]>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 50 +++++++++-------------
 drivers/gpu/drm/exynos/exynos_dp.c                 | 26 ++++++-----
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 48 ++++++++++++---------
 include/drm/bridge/analogix_dp.h                   | 19 ++++----
 4 files changed, 74 insertions(+), 69 deletions(-)
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +20 -30
index a8905049b9da..878f61b65cb2 100644
@@ -98,17 +98,15 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
 	return 0;
 }
 
-int analogix_dp_psr_supported(struct device *dev)
+int analogix_dp_psr_supported(struct analogix_dp_device *dp)
 {
-	struct analogix_dp_device *dp = dev_get_drvdata(dev);
 
 	return dp->psr_support;
 }
 EXPORT_SYMBOL_GPL(analogix_dp_psr_supported);
 
-int analogix_dp_enable_psr(struct device *dev)
+int analogix_dp_enable_psr(struct analogix_dp_device *dp)
 {
-	struct analogix_dp_device *dp = dev_get_drvdata(dev);
 	struct edp_vsc_psr psr_vsc;
 
 	if (!dp->psr_support)

@@ -129,9 +127,8 @@ int analogix_dp_enable_psr(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(analogix_dp_enable_psr);
 
-int analogix_dp_disable_psr(struct device *dev)
+int analogix_dp_disable_psr(struct analogix_dp_device *dp)
 {
-	struct analogix_dp_device *dp = dev_get_drvdata(dev);
 	struct edp_vsc_psr psr_vsc;
 	int ret;
 

@@ -1283,8 +1280,9 @@ static ssize_t analogix_dpaux_transfer(struct drm_dp_aux *aux,
 	return analogix_dp_transfer(dp, msg);
 }
 
-int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
-		     struct analogix_dp_plat_data *plat_data)
+struct analogix_dp_device *
+analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
+		 struct analogix_dp_plat_data *plat_data)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct analogix_dp_device *dp;

@@ -1294,14 +1292,12 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 
 	if (!plat_data) {
 		dev_err(dev, "Invalided input plat_data\n");
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	dp = devm_kzalloc(dev, sizeof(struct analogix_dp_device), GFP_KERNEL);
 	if (!dp)
-		return -ENOMEM;
-
-	dev_set_drvdata(dev, dp);
+		return ERR_PTR(-ENOMEM);
 
 	dp->dev = &pdev->dev;
 	dp->dpms_mode = DRM_MODE_DPMS_OFF;

@@ -1318,7 +1314,7 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 
 	ret = analogix_dp_dt_parse_pdata(dp);
 	if (ret)
-		return ret;
+		return ERR_PTR(ret);
 
 	dp->phy = devm_phy_get(dp->dev, "dp");
 	if (IS_ERR(dp->phy)) {

@@ -1332,14 +1328,14 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 			if (ret == -ENOSYS || ret == -ENODEV)
 				dp->phy = NULL;
 			else
-				return ret;
+				return ERR_PTR(ret);
 		}
 	}
 
 	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);
+		return ERR_CAST(dp->clock);
 	}
 
 	clk_prepare_enable(dp->clock);

@@ -1348,7 +1344,7 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 
 	dp->reg_base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(dp->reg_base))
-		return PTR_ERR(dp->reg_base);
+		return ERR_CAST(dp->reg_base);
 
 	dp->force_hpd = of_property_read_bool(dev->of_node, "force-hpd");
 

@@ -1369,7 +1365,7 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 					    "hpd_gpio");
 		if (ret) {
 			dev_err(&pdev->dev, "failed to get hpd gpio\n");
-			return ret;
+			return ERR_PTR(ret);
 		}
 		dp->irq = gpio_to_irq(dp->hpd_gpio);
 		irq_flags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;

@@ -1381,7 +1377,7 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 
 	if (dp->irq == -ENXIO) {
 		dev_err(&pdev->dev, "failed to get irq\n");
-		return -ENODEV;
+		return ERR_PTR(-ENODEV);
 	}
 
 	pm_runtime_enable(dev);

@@ -1422,7 +1418,7 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 	phy_power_off(dp->phy);
 	pm_runtime_put(dev);
 
-	return 0;
+	return dp;
 
 err_disable_pm_runtime:
 

@@ -1430,15 +1426,12 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 	pm_runtime_put(dev);
 	pm_runtime_disable(dev);
 
-	return ret;
+	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(analogix_dp_bind);
 
-void analogix_dp_unbind(struct device *dev, struct device *master,
-			void *data)
+void analogix_dp_unbind(struct analogix_dp_device *dp)
 {
-	struct analogix_dp_device *dp = dev_get_drvdata(dev);
-
 	analogix_dp_bridge_disable(dp->bridge);
 	dp->connector.funcs->destroy(&dp->connector);
 	dp->encoder->funcs->destroy(dp->encoder);

@@ -1451,16 +1444,14 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
 	}
 
 	drm_dp_aux_unregister(&dp->aux);
-	pm_runtime_disable(dev);
+	pm_runtime_disable(dp->dev);
 	clk_disable_unprepare(dp->clock);
 }
 EXPORT_SYMBOL_GPL(analogix_dp_unbind);
 
 #ifdef CONFIG_PM
-int analogix_dp_suspend(struct device *dev)
+int analogix_dp_suspend(struct analogix_dp_device *dp)
 {
-	struct analogix_dp_device *dp = dev_get_drvdata(dev);
-
 	clk_disable_unprepare(dp->clock);
 
 	if (dp->plat_data->panel) {

@@ -1472,9 +1463,8 @@ int analogix_dp_suspend(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(analogix_dp_suspend);
 
-int analogix_dp_resume(struct device *dev)
+int analogix_dp_resume(struct analogix_dp_device *dp)
 {
-	struct analogix_dp_device *dp = dev_get_drvdata(dev);
 	int ret;
 
 	ret = clk_prepare_enable(dp->clock);
drivers/gpu/drm/exynos/exynos_dp.c +15 -11
index 39629e7a80b9..f7e5b2c405ed 100644
@@ -41,6 +41,7 @@ struct exynos_dp_device {
 	struct device              *dev;
 
 	struct videomode           vm;
+	struct analogix_dp_device *adp;
 	struct analogix_dp_plat_data plat_data;
 };
 

@@ -157,13 +158,6 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
 	struct drm_device *drm_dev = data;
 	int ret;
 
-	/*
-	 * Just like the probe function said, we don't need the
-	 * device drvrate anymore, we should leave the charge to
-	 * analogix dp driver, set the device drvdata to NULL.
-	 */
-	dev_set_drvdata(dev, NULL);
-
 	dp->dev = dev;
 	dp->drm_dev = drm_dev;
 

@@ -190,13 +184,19 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
 
 	dp->plat_data.encoder = encoder;
 
-	return analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
+	dp->adp = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
+	if (IS_ERR(dp->adp))
+		return PTR_ERR(dp->adp);
+
+	return 0;
 }
 
 static void exynos_dp_unbind(struct device *dev, struct device *master,
 			     void *data)
 {
-	return analogix_dp_unbind(dev, master, data);
+	struct exynos_dp_device *dp = dev_get_drvdata(dev);
+
+	return analogix_dp_unbind(dp->adp);
 }
 
 static const struct component_ops exynos_dp_ops = {

@@ -257,12 +257,16 @@ static int exynos_dp_remove(struct platform_device *pdev)
 #ifdef CONFIG_PM
 static int exynos_dp_suspend(struct device *dev)
 {
-	return analogix_dp_suspend(dev);
+	struct exynos_dp_device *dp = dev_get_drvdata(dev);
+
+	return analogix_dp_suspend(dp->adp);
 }
 
 static int exynos_dp_resume(struct device *dev)
 {
-	return analogix_dp_resume(dev);
+	struct exynos_dp_device *dp = dev_get_drvdata(dev);
+
+	return analogix_dp_resume(dp->adp);
 }
 #endif
drivers/gpu/drm/rockchip/analogix_dp-rockchip.c +28 -20

99 lines not on this page · raw message

include/drm/bridge/analogix_dp.h +11 -8
index c99d6eaef1ac..5518fc75dd6e 100644
@@ -13,6 +13,8 @@
 
 #include <drm/drm_crtc.h>
 
+struct analogix_dp_device;
+
 enum analogix_dp_devtype {
 	EXYNOS_DP,
 	RK3288_DP,

@@ -38,16 +40,17 @@ struct analogix_dp_plat_data {
 			 struct drm_connector *);
 };
 
-int analogix_dp_psr_supported(struct device *dev);
-int analogix_dp_enable_psr(struct device *dev);
-int analogix_dp_disable_psr(struct device *dev);
+int analogix_dp_psr_supported(struct analogix_dp_device *dp);
+int analogix_dp_enable_psr(struct analogix_dp_device *dp);
+int analogix_dp_disable_psr(struct analogix_dp_device *dp);
 
-int analogix_dp_resume(struct device *dev);
-int analogix_dp_suspend(struct device *dev);
+int analogix_dp_resume(struct analogix_dp_device *dp);
+int analogix_dp_suspend(struct analogix_dp_device *dp);
 
-int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
-		     struct analogix_dp_plat_data *plat_data);
-void analogix_dp_unbind(struct device *dev, struct device *master, void *data);
+struct analogix_dp_device *
+analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
+		 struct analogix_dp_plat_data *plat_data);
+void analogix_dp_unbind(struct analogix_dp_device *dp);
 
 int analogix_dp_start_crc(struct drm_connector *connector);
 int analogix_dp_stop_crc(struct drm_connector *connector);
Signature · 1 line
2.14.1
maintainer Applied
#2 lore raw

Re: [PATCH v8 1/8] drm/bridge: analogix: Do not use device's drvdata

Am Mittwoch, 10. Januar 2018, 17:23:41 CET schrieb Thierry Escande:

Quoted 12 lines

From: Jeffy Chen <[email protected]>

The driver that instantiates the bridge should own the drvdata, as all driver model callbacks (probe, remove, shutdown, PM ops, etc.) are also owned by its driver struct. Moreover, storing two different pointer types in driver data depending on driver initialization status is barely a good practice and in fact has led to many bugs in this driver.

Let's clean up this mess and change Analogix entry points to simply accept some opaque struct pointer, adjusting their users at the same time to avoid breaking the compilation.

Quoted 7 lines
Signed-off-by Tomasz Figa <[email protected]>
Signed-off-by Jeffy Chen <[email protected]>
Signed-off-by Thierry Escande <[email protected]>
Reviewed-by Andrzej Hajda <[email protected]>
Reviewed-by Sean Paul <[email protected]>
Acked-by Jingoo Han <[email protected]>
Acked-by Archit Taneja <[email protected]>

applied to drm-misc-next as it had Reviews/Acks from drm-bridge people

Thanks
Heiko
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

author
#3 lore raw

PATCH v8 2/8 drm/bridge: analogix_dp: Fix connector and encoder cleanup

In reply to Thierry Escande

From: Jeffy Chen <[email protected]>

Since we are initing connector in the core driver and encoder in the plat driver, let's clean them up in the right places.

Signed-off-by Jeffy Chen <[email protected]>
Signed-off-by Thierry Escande <thierry.escande-ZGY8ohtN/[email protected]>
Reviewed-by Andrzej Hajda <[email protected]>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  2 --
 drivers/gpu/drm/exynos/exynos_dp.c                 |  7 +++++--
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 12 +++++-------
 3 files changed, 10 insertions(+), 11 deletions(-)
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +0 -2
index 878f61b65cb2..cb5e18d6ba04 100644
@@ -1411,7 +1411,6 @@ analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 	ret = analogix_dp_create_bridge(drm_dev, dp);
 	if (ret) {
 		DRM_ERROR("failed to create bridge (%d)\n", ret);
-		drm_encoder_cleanup(dp->encoder);
 		goto err_disable_pm_runtime;
 	}
 

@@ -1434,7 +1433,6 @@ void analogix_dp_unbind(struct analogix_dp_device *dp)
 {
 	analogix_dp_bridge_disable(dp->bridge);
 	dp->connector.funcs->destroy(&dp->connector);
-	dp->encoder->funcs->destroy(dp->encoder);
 
 	if (dp->plat_data->panel) {
 		if (drm_panel_unprepare(dp->plat_data->panel))
drivers/gpu/drm/exynos/exynos_dp.c +5 -2
index f7e5b2c405ed..33319a858f3a 100644
@@ -185,8 +185,10 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
 	dp->plat_data.encoder = encoder;
 
 	dp->adp = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
-	if (IS_ERR(dp->adp))
+	if (IS_ERR(dp->adp)) {
+		dp->encoder.funcs->destroy(&dp->encoder);
 		return PTR_ERR(dp->adp);
+	}
 
 	return 0;
 }

@@ -196,7 +198,8 @@ static void exynos_dp_unbind(struct device *dev, struct device *master,
 {
 	struct exynos_dp_device *dp = dev_get_drvdata(dev);
 
-	return analogix_dp_unbind(dp->adp);
+	analogix_dp_unbind(dp->adp);
+	dp->encoder.funcs->destroy(&dp->encoder);
 }
 
 static const struct component_ops exynos_dp_ops = {
drivers/gpu/drm/rockchip/analogix_dp-rockchip.c +5 -7
index 8a58ad80f509..37250ab63bd7 100644
@@ -259,13 +259,8 @@ static struct drm_encoder_helper_funcs rockchip_dp_encoder_helper_funcs = {
 	.atomic_check = rockchip_dp_drm_encoder_atomic_check,
 };
 
-static void rockchip_dp_drm_encoder_destroy(struct drm_encoder *encoder)
-{
-	drm_encoder_cleanup(encoder);
-}
-
 static struct drm_encoder_funcs rockchip_dp_encoder_funcs = {
-	.destroy = rockchip_dp_drm_encoder_destroy,
+	.destroy = drm_encoder_cleanup,
 };
 
 static int rockchip_dp_of_probe(struct rockchip_dp_device *dp)

@@ -362,8 +357,10 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 	rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);
 
 	dp->adp = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
-	if (IS_ERR(dp->adp))
+	if (IS_ERR(dp->adp)) {
+		dp->encoder.funcs->destroy(&dp->encoder);
 		return PTR_ERR(dp->adp);
+	}
 
 	return 0;
 }

@@ -375,6 +372,7 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
 
 	rockchip_drm_psr_unregister(&dp->encoder);
 	analogix_dp_unbind(dp->adp);
+	dp->encoder.funcs->destroy(&dp->encoder);
 }
 
 static const struct component_ops rockchip_dp_component_ops = {
Signature · 1 line
2.14.1
maintainer Applied
#4 lore raw

Re: [PATCH v8 2/8] drm/bridge: analogix_dp: Fix connector and encoder cleanup

Am Mittwoch, 10. Januar 2018, 17:23:42 CET schrieb Thierry Escande:

Quoted 8 lines

From: Jeffy Chen <[email protected]>

Since we are initing connector in the core driver and encoder in the plat driver, let's clean them up in the right places.

Signed-off-by Jeffy Chen <[email protected]>
Signed-off-by Thierry Escande <[email protected]>
Reviewed-by Andrzej Hajda <[email protected]>

applied to drm-misc-next as it had also a Review from drm-bridge people

Thanks
Heiko
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

author
#5 lore raw

PATCH v8 3/8 drm/rockchip: analogix_dp: Add a sanity check for rockchip_drm_psr_register()

In reply to Thierry Escande

From: Jeffy Chen <[email protected]>

The rockchip_drm_psr_register() can fail, so add a sanity check for that.

Also reorder the calls in unbind() to match bind().

Signed-off-by Jeffy Chen <[email protected]>
Signed-off-by Thierry Escande <thierry.escande-ZGY8ohtN/[email protected]>
---
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/gpu/drm/rockchip/analogix_dp-rockchip.c +11 -4
index 37250ab63bd7..eb88c52336a7 100644
@@ -354,15 +354,22 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 	dp->psr_state = ~EDP_VSC_PSR_STATE_ACTIVE;
 	INIT_WORK(&dp->psr_work, analogix_dp_psr_work);
 
-	rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);
+	ret = rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);
+	if (ret < 0)
+		goto err_cleanup_encoder;
 
 	dp->adp = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
 	if (IS_ERR(dp->adp)) {
-		dp->encoder.funcs->destroy(&dp->encoder);
-		return PTR_ERR(dp->adp);
+		ret = PTR_ERR(dp->adp);
+		goto err_unreg_psr;
 	}
 
 	return 0;
+err_unreg_psr:
+	rockchip_drm_psr_unregister(&dp->encoder);
+err_cleanup_encoder:
+	dp->encoder.funcs->destroy(&dp->encoder);
+	return ret;
 }
 
 static void rockchip_dp_unbind(struct device *dev, struct device *master,

@@ -370,8 +377,8 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
 {
 	struct rockchip_dp_device *dp = dev_get_drvdata(dev);
 
-	rockchip_drm_psr_unregister(&dp->encoder);
 	analogix_dp_unbind(dp->adp);
+	rockchip_drm_psr_unregister(&dp->encoder);
 	dp->encoder.funcs->destroy(&dp->encoder);
 }
Signature · 1 line
2.14.1
maintainer
#6 lore raw

PATCH 1/1 drm/rockchip: analogix_dp: reorder psr_unregister call in unbind

In bind the psr handler gets registered first before the core analogix_dp_bind() gets called. So it should be the other way around in unbind, first unbind the analogix_dp and then unregister the psr.

Signed-off-by Jeffy Chen <[email protected]>
Signed-off-by Thierry Escande <[email protected]>
Signed-off-by Heiko Stuebner <[email protected]>
---
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/gpu/drm/rockchip/analogix_dp-rockchip.c +1 -1
index 37250ab63bd7..eb88c52336a7 100644
@@ -370,8 +377,8 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
 {
 	struct rockchip_dp_device *dp = dev_get_drvdata(dev);
 
-	rockchip_drm_psr_unregister(&dp->encoder);
 	analogix_dp_unbind(dp->adp);
+	rockchip_drm_psr_unregister(&dp->encoder);
 	dp->encoder.funcs->destroy(&dp->encoder);
 }
Signature · 7 lines
2.15.1


_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
maintainer Applied
#7 lore raw

Re: [PATCH v8 3/8] drm/rockchip: analogix_dp: Add a sanity check for rockchip_drm_psr_register()

In reply to Thierry Escande

Am Mittwoch, 10. Januar 2018, 17:23:43 CET schrieb Thierry Escande:

Quoted 8 lines

From: Jeffy Chen <[email protected]>

The rockchip_drm_psr_register() can fail, so add a sanity check for that.

Also reorder the calls in unbind() to match bind().

Signed-off-by Jeffy Chen <[email protected]>
Signed-off-by Thierry Escande <[email protected]>

applied to drm-misc, after splitting out the reordering into a separate patch. Having these surprise changes hidden behind an "also" feels not ideal.

Heiko
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

author
#8 lore raw

PATCH v8 4/8 drm/rockchip: dw-mipi-dsi: Fix error handling path

In reply to Thierry Escande

From: Jeffy Chen <[email protected]>

Add missing pm_runtime_disable() in bind()'s error handling path.

Also cleanup encoder & connector in unbind().

Fixes 80a9a059d4e4 ("drm/rockchip/dsi: add dw-mipi power domain support")
Signed-off-by Jeffy Chen <[email protected]>
Signed-off-by Thierry Escande <thierry.escande-ZGY8ohtN/[email protected]>
---
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)
drivers/gpu/drm/rockchip/dw-mipi-dsi.c +13 -8
index b1fe0639227e..78e6b7919bf7 100644
@@ -1282,7 +1282,7 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master,
 	ret = dw_mipi_dsi_register(drm, dsi);
 	if (ret) {
 		DRM_DEV_ERROR(dev, "Failed to register mipi_dsi: %d\n", ret);
-		goto err_pllref;
+		goto err_disable_pllref;
 	}
 
 	dsi->dsi_host.ops = &dw_mipi_dsi_host_ops;

@@ -1290,24 +1290,25 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master,
 	ret = mipi_dsi_host_register(&dsi->dsi_host);
 	if (ret) {
 		DRM_DEV_ERROR(dev, "Failed to register MIPI host: %d\n", ret);
-		goto err_cleanup;
+		goto err_disable_pm_runtime;
 	}
 
 	if (!dsi->panel) {
 		ret = -EPROBE_DEFER;
-		goto err_mipi_dsi_host;
+		goto err_unreg_mipi_dsi_host;
 	}
 
 	dev_set_drvdata(dev, dsi);
 	pm_runtime_enable(dev);
 	return 0;
 
-err_mipi_dsi_host:
+err_unreg_mipi_dsi_host:
 	mipi_dsi_host_unregister(&dsi->dsi_host);
-err_cleanup:
-	drm_encoder_cleanup(&dsi->encoder);
-	drm_connector_cleanup(&dsi->connector);
-err_pllref:
+err_disable_pm_runtime:
+	pm_runtime_disable(dev);
+	dsi->connector.funcs->destroy(&dsi->connector);
+	dsi->encoder.funcs->destroy(&dsi->encoder);
+err_disable_pllref:
 	clk_disable_unprepare(dsi->pllref_clk);
 	return ret;
 }

@@ -1319,6 +1320,10 @@ static void dw_mipi_dsi_unbind(struct device *dev, struct device *master,
 
 	mipi_dsi_host_unregister(&dsi->dsi_host);
 	pm_runtime_disable(dev);
+
+	dsi->connector.funcs->destroy(&dsi->connector);
+	dsi->encoder.funcs->destroy(&dsi->encoder);
+
 	clk_disable_unprepare(dsi->pllref_clk);
 }
Signature · 1 line
2.14.1
maintainer
#9 lore raw

Re: [PATCH v8 4/8] drm/rockchip: dw-mipi-dsi: Fix error handling path

Hi Jeffy, Thierry, Enric,

Am Mittwoch, 10. Januar 2018, 17:23:44 CET schrieb Thierry Escande:

Quoted 5 lines

From: Jeffy Chen <[email protected]>

Add missing pm_runtime_disable() in bind()'s error handling path.

Also cleanup encoder & connector in unbind().

Can you please split all these surprise "Also" sections into separate patches?

It looks like this is true for all following patch to some degree and the inno-hdmi patch even has a unbind reordering-change without mentioning it in the commit message.

Thanks
Heiko

Quoted 55 lines
Fixes 80a9a059d4e4 ("drm/rockchip/dsi: add dw-mipi power domain support")
Signed-off-by Jeffy Chen <[email protected]>
Signed-off-by Thierry Escande <[email protected]>
---
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index b1fe0639227e..78e6b7919bf7 100644

drivers/gpu/drm/rockchip/dw-mipi-dsi.c +9 -8
@@ -1282,7 +1282,7 @@ static int dw_mipi_dsi_bind(struct device *dev, struct
 device *master, ret = dw_mipi_dsi_register(drm, dsi);
 	if (ret) {
 		DRM_DEV_ERROR(dev, "Failed to register mipi_dsi: %d\n", ret);
-		goto err_pllref;
+		goto err_disable_pllref;
 	}
 
 	dsi->dsi_host.ops = &dw_mipi_dsi_host_ops;

@@ -1290,24 +1290,25 @@ static int dw_mipi_dsi_bind(struct device *dev,
 struct device *master, ret = mipi_dsi_host_register(&dsi->dsi_host);
 	if (ret) {
 		DRM_DEV_ERROR(dev, "Failed to register MIPI host: %d\n", ret);
-		goto err_cleanup;
+		goto err_disable_pm_runtime;
 	}
 
 	if (!dsi->panel) {
 		ret = -EPROBE_DEFER;
-		goto err_mipi_dsi_host;
+		goto err_unreg_mipi_dsi_host;
 	}
 
 	dev_set_drvdata(dev, dsi);
 	pm_runtime_enable(dev);
 	return 0;
 
-err_mipi_dsi_host:
+err_unreg_mipi_dsi_host:
 	mipi_dsi_host_unregister(&dsi->dsi_host);
-err_cleanup:
-	drm_encoder_cleanup(&dsi->encoder);
-	drm_connector_cleanup(&dsi->connector);
-err_pllref:
+err_disable_pm_runtime:
+	pm_runtime_disable(dev);
+	dsi->connector.funcs->destroy(&dsi->connector);
+	dsi->encoder.funcs->destroy(&dsi->encoder);
+err_disable_pllref:
 	clk_disable_unprepare(dsi->pllref_clk);
 	return ret;
 }

drivers/gpu/drm/rockchip/dw-mipi-dsi.c static void dw_mipi_dsi_unbind(struct device *dev, · line 1329

Quoted 10 lines
drivers/gpu/drm/rockchip/dw-mipi-dsi.c +4 -0
@@ -1319,6 +1320,10 @@ static void dw_mipi_dsi_unbind(struct device *dev,
 struct device *master,
 
 	mipi_dsi_host_unregister(&dsi->dsi_host);
 	pm_runtime_disable(dev);
+
+	dsi->connector.funcs->destroy(&dsi->connector);
+	dsi->encoder.funcs->destroy(&dsi->encoder);
+
 	clk_disable_unprepare(dsi->pllref_clk);
 }

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

#10 lore raw

Re: [PATCH v8 4/8] drm/rockchip: dw-mipi-dsi: Fix error handling path

Hi Heiko,

Quoted 12 lines · Heiko Stübner

Hi Jeffy, Thierry, Enric,

Am Mittwoch, 10. Januar 2018, 17:23:44 CET schrieb Thierry Escande:

Quoted 5 lines

From: Jeffy Chen <[email protected]>

Add missing pm_runtime_disable() in bind()'s error handling path.

Also cleanup encoder & connector in unbind().

Can you please split all these surprise "Also" sections into separate patches?

I'll take a look.

Quoted 4 lines

It looks like this is true for all following patch to some degree and the inno-hdmi patch even has a unbind reordering-change without mentioning it in the commit message.

Actually, I think this patch is not correct against current mainline, see below.

Quoted 43 lines

Thanks
Heiko

Quoted 39 lines
Fixes 80a9a059d4e4 ("drm/rockchip/dsi: add dw-mipi power domain support")
Signed-off-by Jeffy Chen <[email protected]>
Signed-off-by Thierry Escande <[email protected]>
---
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index b1fe0639227e..78e6b7919bf7 100644

drivers/gpu/drm/rockchip/dw-mipi-dsi.c +3 -3
@@ -1282,7 +1282,7 @@ static int dw_mipi_dsi_bind(struct device *dev, struct
 device *master, ret = dw_mipi_dsi_register(drm, dsi);
 	if (ret) {
 		DRM_DEV_ERROR(dev, "Failed to register mipi_dsi: %d\n", ret);
-		goto err_pllref;
+		goto err_disable_pllref;
 	}
 
 	dsi->dsi_host.ops = &dw_mipi_dsi_host_ops;

@@ -1290,24 +1290,25 @@ static int dw_mipi_dsi_bind(struct device *dev,
 struct device *master, ret = mipi_dsi_host_register(&dsi->dsi_host);
 	if (ret) {
 		DRM_DEV_ERROR(dev, "Failed to register MIPI host: %d\n", ret);
-		goto err_cleanup;
+		goto err_disable_pm_runtime;
 	}
 
 	if (!dsi->panel) {
 		ret = -EPROBE_DEFER;
-		goto err_mipi_dsi_host;
+		goto err_unreg_mipi_dsi_host;
 	}
 
 	dev_set_drvdata(dev, dsi);
 	pm_runtime_enable(dev);
 	return 0;
Quoted 9 lines

drivers/gpu/drm/rockchip/dw-mipi-dsi.c static int dw_mipi_dsi_bind(struct device *dev, struct device *master, · line 1308

Quoted 9 lines
drivers/gpu/drm/rockchip/dw-mipi-dsi.c +3 -5
-err_mipi_dsi_host:
+err_unreg_mipi_dsi_host:
 	mipi_dsi_host_unregister(&dsi->dsi_host);
-err_cleanup:
-	drm_encoder_cleanup(&dsi->encoder);
-	drm_connector_cleanup(&dsi->connector);
-err_pllref:
+err_disable_pm_runtime:
+	pm_runtime_disable(dev);

I think that this is not required, 'pm_runtime_enable' is called just before the 'return 0' (see above). Commit 517f56839f58 'drm/rockchip: dw-mipi-dsi: fix possible un-balanced runtime PM enable' moved the call to that place. So, now the call to pm_runtime_disable is not needed.

Quoted 2 lines

drivers/gpu/drm/rockchip/dw-mipi-dsi.c static int dw_mipi_dsi_bind(struct device *dev, · line 1310

Quoted 2 lines
drivers/gpu/drm/rockchip/dw-mipi-dsi.c +2 -0
+	dsi->connector.funcs->destroy(&dsi->connector);
+	dsi->encoder.funcs->destroy(&dsi->encoder);

Here, there is a reordering and also a function replacement. The reorder makes sense to me, first cleanup the connector and then the encoder, but I'm not sure about the function change and if is needed, anyway, in the case is needed, shouldn't be

+ drm_connector_cleanup(&dsi->connector);
+ drm_encoder_cleanup(&dsi->encoder);

instead?

Quoted 7 lines
Quoted 7 lines
drivers/gpu/drm/rockchip/dw-mipi-dsi.c +1 -0
+err_disable_pllref:
 	clk_disable_unprepare(dsi->pllref_clk);
 	return ret;
 }

@@ -1319,6 +1320,10 @@ static void dw_mipi_dsi_unbind(struct device *dev,
 struct device *master,
Quoted 6 lines

drivers/gpu/drm/rockchip/dw-mipi-dsi.c static void dw_mipi_dsi_unbind(struct device *dev, struct device *master, · line 1326

Quoted 6 lines
drivers/gpu/drm/rockchip/dw-mipi-dsi.c +4 -0
 	mipi_dsi_host_unregister(&dsi->dsi_host);
 	pm_runtime_disable(dev);
+
+	dsi->connector.funcs->destroy(&dsi->connector);
+	dsi->encoder.funcs->destroy(&dsi->encoder);
+

Same here.

Quoted 4 lines

drivers/gpu/drm/rockchip/dw-mipi-dsi.c static void dw_mipi_dsi_unbind(struct device *dev, · line 1329

Quoted 2 lines
drivers/gpu/drm/rockchip/dw-mipi-dsi.c +0 -0
 	clk_disable_unprepare(dsi->pllref_clk);
 }

Best regards,
Enric

maintainer
#11 lore raw

Re: [PATCH v8 4/8] drm/rockchip: dw-mipi-dsi: Fix error handling path

Hi Enric,

Am Freitag, 2. März 2018, 13:09:02 CET schrieb Enric Balletbo i Serra:

Quoted 94 lines

Hi Heiko,

Quoted 12 lines · Heiko Stübner

Hi Jeffy, Thierry, Enric,

Am Mittwoch, 10. Januar 2018, 17:23:44 CET schrieb Thierry Escande:

Quoted 5 lines

From: Jeffy Chen <[email protected]>

Add missing pm_runtime_disable() in bind()'s error handling path.

Also cleanup encoder & connector in unbind().

Can you please split all these surprise "Also" sections into separate patches?

I'll take a look.

Quoted 4 lines

It looks like this is true for all following patch to some degree and the inno-hdmi patch even has a unbind reordering-change without mentioning it in the commit message.

Actually, I think this patch is not correct against current mainline, see below.

Quoted 43 lines

Thanks
Heiko

Quoted 39 lines
Fixes 80a9a059d4e4 ("drm/rockchip/dsi: add dw-mipi power domain support")
Signed-off-by Jeffy Chen <[email protected]>
Signed-off-by Thierry Escande <[email protected]>
---
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index b1fe0639227e..78e6b7919bf7 100644

drivers/gpu/drm/rockchip/dw-mipi-dsi.c +3 -3
@@ -1282,7 +1282,7 @@ static int dw_mipi_dsi_bind(struct device *dev, struct
 device *master, ret = dw_mipi_dsi_register(drm, dsi);
 	if (ret) {
 		DRM_DEV_ERROR(dev, "Failed to register mipi_dsi: %d\n", ret);
-		goto err_pllref;
+		goto err_disable_pllref;
 	}
 
 	dsi->dsi_host.ops = &dw_mipi_dsi_host_ops;

@@ -1290,24 +1290,25 @@ static int dw_mipi_dsi_bind(struct device *dev,
 struct device *master, ret = mipi_dsi_host_register(&dsi->dsi_host);
 	if (ret) {
 		DRM_DEV_ERROR(dev, "Failed to register MIPI host: %d\n", ret);
-		goto err_cleanup;
+		goto err_disable_pm_runtime;
 	}
 
 	if (!dsi->panel) {
 		ret = -EPROBE_DEFER;
-		goto err_mipi_dsi_host;
+		goto err_unreg_mipi_dsi_host;
 	}
 
 	dev_set_drvdata(dev, dsi);
 	pm_runtime_enable(dev);
 	return 0;
Quoted 9 lines

drivers/gpu/drm/rockchip/dw-mipi-dsi.c static int dw_mipi_dsi_bind(struct device *dev, struct device *master, · line 1308

Quoted 9 lines
drivers/gpu/drm/rockchip/dw-mipi-dsi.c +3 -5
-err_mipi_dsi_host:
+err_unreg_mipi_dsi_host:
 	mipi_dsi_host_unregister(&dsi->dsi_host);
-err_cleanup:
-	drm_encoder_cleanup(&dsi->encoder);
-	drm_connector_cleanup(&dsi->connector);
-err_pllref:
+err_disable_pm_runtime:
+	pm_runtime_disable(dev);

I think that this is not required, 'pm_runtime_enable' is called just before the 'return 0' (see above). Commit 517f56839f58 'drm/rockchip: dw-mipi-dsi: fix possible un-balanced runtime PM enable' moved the call to that place. So, now the call to pm_runtime_disable is not needed.

Quoted 2 lines

drivers/gpu/drm/rockchip/dw-mipi-dsi.c static int dw_mipi_dsi_bind(struct device *dev, · line 1310

Quoted 2 lines
drivers/gpu/drm/rockchip/dw-mipi-dsi.c +2 -0
+	dsi->connector.funcs->destroy(&dsi->connector);
+	dsi->encoder.funcs->destroy(&dsi->encoder);

Here, there is a reordering and also a function replacement. The reorder makes sense to me, first cleanup the connector and then the encoder, but I'm not sure about the function change and if is needed, anyway, in the case is needed, shouldn't be

(quoted diff) +2 -0
+	drm_connector_cleanup(&dsi->connector);
+	drm_encoder_cleanup(&dsi->encoder);
Quoted 1 line

instead?

If you look at drm/rockchip/dw-mipi-dsi.c you'll see that dw_mipi_dsi_drm_connector_destroy() does both connector_unregister() and drm_connector_cleanup().

And looking at other drivers it really seems that calling that destroy callback is really the way to go.

Heiko
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

#12 lore raw

Re: [PATCH v8 4/8] drm/rockchip: dw-mipi-dsi: Fix error handling path

Hi Heiko,

Quoted 97 lines · Heiko Stuebner

Hi Enric,

Am Freitag, 2. März 2018, 13:09:02 CET schrieb Enric Balletbo i Serra:

Quoted 94 lines

Hi Heiko,

Quoted 12 lines · Heiko Stübner

Hi Jeffy, Thierry, Enric,

Am Mittwoch, 10. Januar 2018, 17:23:44 CET schrieb Thierry Escande:

Quoted 5 lines

From: Jeffy Chen <[email protected]>

Add missing pm_runtime_disable() in bind()'s error handling path.

Also cleanup encoder & connector in unbind().

Can you please split all these surprise "Also" sections into separate patches?

I'll take a look.

Quoted 4 lines

It looks like this is true for all following patch to some degree and the inno-hdmi patch even has a unbind reordering-change without mentioning it in the commit message.

Actually, I think this patch is not correct against current mainline, see below.

Quoted 43 lines

Thanks
Heiko

Quoted 39 lines
Fixes 80a9a059d4e4 ("drm/rockchip/dsi: add dw-mipi power domain support")
Signed-off-by Jeffy Chen <[email protected]>
Signed-off-by Thierry Escande <[email protected]>
---
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index b1fe0639227e..78e6b7919bf7 100644

drivers/gpu/drm/rockchip/dw-mipi-dsi.c +3 -3
@@ -1282,7 +1282,7 @@ static int dw_mipi_dsi_bind(struct device *dev, struct
 device *master, ret = dw_mipi_dsi_register(drm, dsi);
 	if (ret) {
 		DRM_DEV_ERROR(dev, "Failed to register mipi_dsi: %d\n", ret);
-		goto err_pllref;
+		goto err_disable_pllref;
 	}
 
 	dsi->dsi_host.ops = &dw_mipi_dsi_host_ops;

@@ -1290,24 +1290,25 @@ static int dw_mipi_dsi_bind(struct device *dev,
 struct device *master, ret = mipi_dsi_host_register(&dsi->dsi_host);
 	if (ret) {
 		DRM_DEV_ERROR(dev, "Failed to register MIPI host: %d\n", ret);
-		goto err_cleanup;
+		goto err_disable_pm_runtime;
 	}
 
 	if (!dsi->panel) {
 		ret = -EPROBE_DEFER;
-		goto err_mipi_dsi_host;
+		goto err_unreg_mipi_dsi_host;
 	}
 
 	dev_set_drvdata(dev, dsi);
 	pm_runtime_enable(dev);
 	return 0;
Quoted 9 lines

drivers/gpu/drm/rockchip/dw-mipi-dsi.c static int dw_mipi_dsi_bind(struct device *dev, struct device *master, · line 1308

Quoted 9 lines
drivers/gpu/drm/rockchip/dw-mipi-dsi.c +3 -5
-err_mipi_dsi_host:
+err_unreg_mipi_dsi_host:
 	mipi_dsi_host_unregister(&dsi->dsi_host);
-err_cleanup:
-	drm_encoder_cleanup(&dsi->encoder);
-	drm_connector_cleanup(&dsi->connector);
-err_pllref:
+err_disable_pm_runtime:
+	pm_runtime_disable(dev);

I think that this is not required, 'pm_runtime_enable' is called just before the 'return 0' (see above). Commit 517f56839f58 'drm/rockchip: dw-mipi-dsi: fix possible un-balanced runtime PM enable' moved the call to that place. So, now the call to pm_runtime_disable is not needed.

Quoted 2 lines

drivers/gpu/drm/rockchip/dw-mipi-dsi.c static int dw_mipi_dsi_bind(struct device *dev, · line 1310

Quoted 2 lines
drivers/gpu/drm/rockchip/dw-mipi-dsi.c +2 -0
+	dsi->connector.funcs->destroy(&dsi->connector);
+	dsi->encoder.funcs->destroy(&dsi->encoder);

Here, there is a reordering and also a function replacement. The reorder makes sense to me, first cleanup the connector and then the encoder, but I'm not sure about the function change and if is needed, anyway, in the case is needed, shouldn't be

(quoted diff) +2 -0
+	drm_connector_cleanup(&dsi->connector);
+	drm_encoder_cleanup(&dsi->encoder);
Quoted 9 lines
Quoted 1 line

instead?

If you look at drm/rockchip/dw-mipi-dsi.c you'll see that dw_mipi_dsi_drm_connector_destroy() does both connector_unregister() and drm_connector_cleanup().

And looking at other drivers it really seems that calling that destroy callback is really the way to go.

Right, thanks for pointing me in the right direction. Learning about this graphics stuff ;)

Regards,
Enric

Quoted 3 lines

Heiko

author
#13 lore raw

PATCH v8 5/8 drm/rockchip: inno_hdmi: Fix error handling path

In reply to Thierry Escande

From: Jeffy Chen <[email protected]>

Add missing error handling in bind().

Fixes 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
Signed-off-by Jeffy Chen <[email protected]>
Signed-off-by Thierry Escande <thierry.escande-ZGY8ohtN/[email protected]>
---
 drivers/gpu/drm/rockchip/inno_hdmi.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)
drivers/gpu/drm/rockchip/inno_hdmi.c +17 -5
index fab30927a889..b775283d7363 100644
@@ -852,8 +852,10 @@ static int inno_hdmi_bind(struct device *dev, struct device *master,
 	}
 
 	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
+	if (irq < 0) {
+		ret = irq;
+		goto err_disable_clk;
+	}
 
 	inno_hdmi_reset(hdmi);
 

@@ -861,7 +863,7 @@ static int inno_hdmi_bind(struct device *dev, struct device *master,
 	if (IS_ERR(hdmi->ddc)) {
 		ret = PTR_ERR(hdmi->ddc);
 		hdmi->ddc = NULL;
-		return ret;
+		goto err_disable_clk;
 	}
 
 	/*

@@ -875,7 +877,7 @@ static int inno_hdmi_bind(struct device *dev, struct device *master,
 
 	ret = inno_hdmi_register(drm, hdmi);
 	if (ret)
-		return ret;
+		goto err_put_adapter;
 
 	dev_set_drvdata(dev, hdmi);
 

@@ -885,7 +887,17 @@ static int inno_hdmi_bind(struct device *dev, struct device *master,
 	ret = devm_request_threaded_irq(dev, irq, inno_hdmi_hardirq,
 					inno_hdmi_irq, IRQF_SHARED,
 					dev_name(dev), hdmi);
+	if (ret < 0)
+		goto err_cleanup_hdmi;
 
+	return 0;
+err_cleanup_hdmi:
+	hdmi->connector.funcs->destroy(&hdmi->connector);
+	hdmi->encoder.funcs->destroy(&hdmi->encoder);
+err_put_adapter:
+	i2c_put_adapter(hdmi->ddc);
+err_disable_clk:
+	clk_disable_unprepare(hdmi->pclk);
 	return ret;
 }
 

@@ -897,8 +909,8 @@ static void inno_hdmi_unbind(struct device *dev, struct device *master,
 	hdmi->connector.funcs->destroy(&hdmi->connector);
 	hdmi->encoder.funcs->destroy(&hdmi->encoder);
 
-	clk_disable_unprepare(hdmi->pclk);
 	i2c_put_adapter(hdmi->ddc);
+	clk_disable_unprepare(hdmi->pclk);
 }
 
 static const struct component_ops inno_hdmi_ops = {
Signature · 1 line
2.14.1
author
#14 lore raw

PATCH v8 6/8 drm/bridge/synopsys: dw-hdmi: Add missing bridge detach

In reply to Thierry Escande

From: Jeffy Chen <[email protected]>

We inited connector in attach(), so need a detach() to cleanup.

Also fix wrong use of dw_hdmi_remove() in bind().

Signed-off-by Jeffy Chen <[email protected]>
Signed-off-by Thierry Escande <thierry.escande-ZGY8ohtN/[email protected]>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +9 -1
index a38db40ce990..1cc63a18b7d5 100644
@@ -1967,6 +1967,13 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge)
 	return 0;
 }
 
+static void dw_hdmi_bridge_detach(struct drm_bridge *bridge)
+{
+	struct dw_hdmi *hdmi = bridge->driver_private;
+
+	drm_connector_cleanup(&hdmi->connector);
+}
+
 static enum drm_mode_status
 dw_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
 			  const struct drm_display_mode *mode)

@@ -2023,6 +2030,7 @@ static void dw_hdmi_bridge_enable(struct drm_bridge *bridge)
 
 static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = {
 	.attach = dw_hdmi_bridge_attach,
+	.detach = dw_hdmi_bridge_detach,
 	.enable = dw_hdmi_bridge_enable,
 	.disable = dw_hdmi_bridge_disable,
 	.mode_set = dw_hdmi_bridge_mode_set,

@@ -2616,7 +2624,7 @@ int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
 
 	ret = drm_bridge_attach(encoder, &hdmi->bridge, NULL);
 	if (ret) {
-		dw_hdmi_remove(pdev);
+		__dw_hdmi_remove(hdmi);
 		DRM_ERROR("Failed to initialize bridge with drm\n");
 		return ret;
 	}
Signature · 1 line
2.14.1
author
#15 lore raw

PATCH v8 7/8 drm/bridge/synopsys: dw-hdmi: Do not use device's drvdata

In reply to Thierry Escande

From: Jeffy Chen <[email protected]>

Let plat drivers own the drvdata, so that they could cleanup resources in their unbind().

Signed-off-by Jeffy Chen <[email protected]>
Signed-off-by Thierry Escande <[email protected]>
Reviewed-by Neil Armstrong <[email protected]>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c   | 43 ++++++++++-------------------
 drivers/gpu/drm/imx/dw_hdmi-imx.c           | 22 +++++++++------
 drivers/gpu/drm/meson/meson_dw_hdmi.c       | 20 ++++++++++----
 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c      | 14 ++++++++--
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 22 +++++++++------
 include/drm/bridge/dw_hdmi.h                | 17 ++++++------
 6 files changed, 77 insertions(+), 61 deletions(-)
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +15 -28
index 1cc63a18b7d5..f0380bcae513 100644
@@ -2073,7 +2073,7 @@ static irqreturn_t dw_hdmi_hardirq(int irq, void *dev_id)
 	return ret;
 }
 
-void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense)
+void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense)
 {
 	mutex_lock(&hdmi->mutex);
 

@@ -2099,13 +2099,6 @@ void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense)
 	}
 	mutex_unlock(&hdmi->mutex);
 }
-
-void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense)
-{
-	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
-
-	__dw_hdmi_setup_rx_sense(hdmi, hpd, rx_sense);
-}
 EXPORT_SYMBOL_GPL(dw_hdmi_setup_rx_sense);
 
 static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)

@@ -2141,9 +2134,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
 	 */
 	if (intr_stat &
 	    (HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_IH_PHY_STAT0_HPD)) {
-		__dw_hdmi_setup_rx_sense(hdmi,
-					 phy_stat & HDMI_PHY_HPD,
-					 phy_stat & HDMI_PHY_RX_SENSE);
+		dw_hdmi_setup_rx_sense(hdmi, phy_stat & HDMI_PHY_HPD,
+				       phy_stat & HDMI_PHY_RX_SENSE);
 
 		if ((phy_stat & (HDMI_PHY_RX_SENSE | HDMI_PHY_HPD)) == 0)
 			cec_notifier_set_phys_addr(hdmi->cec_notifier,

@@ -2533,8 +2525,6 @@ __dw_hdmi_probe(struct platform_device *pdev,
 	if (hdmi->i2c)
 		dw_hdmi_i2c_init(hdmi);
 
-	platform_set_drvdata(pdev, hdmi);
-
 	return hdmi;
 
 err_iahb:

@@ -2584,25 +2574,23 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi)
 /* -----------------------------------------------------------------------------
  * Probe/remove API, used from platforms based on the DRM bridge API.
  */
-int dw_hdmi_probe(struct platform_device *pdev,
-		  const struct dw_hdmi_plat_data *plat_data)
+struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
+			      const struct dw_hdmi_plat_data *plat_data)
 {
 	struct dw_hdmi *hdmi;
 
 	hdmi = __dw_hdmi_probe(pdev, plat_data);
 	if (IS_ERR(hdmi))
-		return PTR_ERR(hdmi);
+		return hdmi;
 
 	drm_bridge_add(&hdmi->bridge);
 
-	return 0;
+	return hdmi;
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_probe);
 
-void dw_hdmi_remove(struct platform_device *pdev)
+void dw_hdmi_remove(struct dw_hdmi *hdmi)
 {
-	struct dw_hdmi *hdmi = platform_get_drvdata(pdev);
-
 	drm_bridge_remove(&hdmi->bridge);
 
 	__dw_hdmi_remove(hdmi);

@@ -2612,31 +2600,30 @@ EXPORT_SYMBOL_GPL(dw_hdmi_remove);
 /* -----------------------------------------------------------------------------
  * Bind/unbind API, used from platforms based on the component framework.
  */
-int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
-		 const struct dw_hdmi_plat_data *plat_data)
+struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev,
+			     struct drm_encoder *encoder,
+			     const struct dw_hdmi_plat_data *plat_data)
 {
 	struct dw_hdmi *hdmi;
 	int ret;
 
 	hdmi = __dw_hdmi_probe(pdev, plat_data);
 	if (IS_ERR(hdmi))
-		return PTR_ERR(hdmi);
+		return hdmi;
 
 	ret = drm_bridge_attach(encoder, &hdmi->bridge, NULL);
 	if (ret) {
 		__dw_hdmi_remove(hdmi);
 		DRM_ERROR("Failed to initialize bridge with drm\n");
-		return ret;
+		return ERR_PTR(ret);
 	}
 
-	return 0;
+	return hdmi;
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_bind);
 
-void dw_hdmi_unbind(struct device *dev)
+void dw_hdmi_unbind(struct dw_hdmi *hdmi)
 {
-	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
-
 	__dw_hdmi_remove(hdmi);
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
drivers/gpu/drm/imx/dw_hdmi-imx.c +13 -9
index b62763aa8706..b01d03e02ce0 100644
@@ -26,6 +26,8 @@ struct imx_hdmi {
 	struct device *dev;
 	struct drm_encoder encoder;
 	struct regmap *regmap;
+
+	struct dw_hdmi *hdmi;
 };
 
 static inline struct imx_hdmi *enc_to_imx_hdmi(struct drm_encoder *e)

@@ -239,22 +241,24 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 	drm_encoder_init(drm, encoder, &dw_hdmi_imx_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS, NULL);
 
-	ret = dw_hdmi_bind(pdev, encoder, plat_data);
+	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
+	if (IS_ERR(hdmi->hdmi)) {
+		encoder->funcs->destroy(encoder);
+		return PTR_ERR(hdmi->hdmi);
+	}
 
-	/*
-	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
-	 * which would have called the encoder cleanup.  Do it manually.
-	 */
-	if (ret)
-		drm_encoder_cleanup(encoder);
+	dev_set_drvdata(dev, hdmi);
 
-	return ret;
+	return 0;
 }
 
 static void dw_hdmi_imx_unbind(struct device *dev, struct device *master,
 			       void *data)
 {
-	return dw_hdmi_unbind(dev);
+	struct imx_hdmi *hdmi = dev_get_drvdata(dev);
+
+	dw_hdmi_unbind(hdmi->hdmi);
+	hdmi->encoder.funcs->destroy(&hdmi->encoder);
 }
 
 static const struct component_ops dw_hdmi_imx_ops = {
drivers/gpu/drm/meson/meson_dw_hdmi.c +15 -5
index 17de3afd98f6..3f1ec9a5cbfb 100644
@@ -140,6 +140,8 @@ struct meson_dw_hdmi {
 	struct clk *venci_clk;
 	struct regulator *hdmi_supply;
 	u32 irq_stat;
+
+	struct dw_hdmi *hdmi;
 };
 #define encoder_to_meson_dw_hdmi(x) \
 	container_of(x, struct meson_dw_hdmi, encoder)

@@ -528,7 +530,7 @@ static irqreturn_t dw_hdmi_top_thread_irq(int irq, void *dev_id)
 		if (stat & HDMITX_TOP_INTR_HPD_RISE)
 			hpd_connected = true;
 
-		dw_hdmi_setup_rx_sense(dw_hdmi->dev, hpd_connected,
+		dw_hdmi_setup_rx_sense(dw_hdmi->hdmi, hpd_connected,
 				       hpd_connected);
 
 		drm_helper_hpd_irq_event(dw_hdmi->encoder.dev);

@@ -878,9 +880,14 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
 	dw_plat_data->input_bus_format = MEDIA_BUS_FMT_YUV8_1X24;
 	dw_plat_data->input_bus_encoding = V4L2_YCBCR_ENC_709;
 
-	ret = dw_hdmi_bind(pdev, encoder, &meson_dw_hdmi->dw_plat_data);
-	if (ret)
-		return ret;
+	meson_dw_hdmi->hdmi = dw_hdmi_bind(pdev, encoder,
+					   &meson_dw_hdmi->dw_plat_data);
+	if (IS_ERR(meson_dw_hdmi->hdmi)) {
+		encoder->funcs->destroy(encoder);
+		return PTR_ERR(meson_dw_hdmi->hdmi);
+	}
+
+	dev_set_drvdata(dev, meson_dw_hdmi);
 
 	DRM_DEBUG_DRIVER("HDMI controller initialized\n");
 

@@ -890,7 +897,10 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
 static void meson_dw_hdmi_unbind(struct device *dev, struct device *master,
 				   void *data)
 {
-	dw_hdmi_unbind(dev);
+	struct meson_dw_hdmi *meson_dw_hdmi = dev_get_drvdata(dev);
+
+	dw_hdmi_unbind(meson_dw_hdmi->hdmi);
+	meson_dw_hdmi->encoder.funcs->destroy(&meson_dw_hdmi->encoder);
 }
 
 static const struct component_ops meson_dw_hdmi_ops = {
drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c +12 -2
index dc85b53d58ef..76210ae25094 100644
@@ -68,12 +68,22 @@ static const struct dw_hdmi_plat_data rcar_dw_hdmi_plat_data = {
 
 static int rcar_dw_hdmi_probe(struct platform_device *pdev)
 {
-	return dw_hdmi_probe(pdev, &rcar_dw_hdmi_plat_data);
+	struct dw_hdmi *hdmi;
+
+	hdmi = dw_hdmi_probe(pdev, &rcar_dw_hdmi_plat_data);
+	if (IS_ERR(hdmi))
+		return PTR_ERR(hdmi);
+
+	platform_set_drvdata(pdev, hdmi);
+
+	return 0;
 }
 
 static int rcar_dw_hdmi_remove(struct platform_device *pdev)
 {
-	dw_hdmi_remove(pdev);
+	struct dw_hdmi *hdmi = platform_get_drvdata(pdev);
+
+	dw_hdmi_remove(hdmi);
 
 	return 0;
 }
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +13 -9
index 1eb02a82fd91..d406d7779ed9 100644
@@ -48,6 +48,8 @@ struct rockchip_hdmi {
 	const struct rockchip_hdmi_chip_data *chip_data;
 	struct clk *vpll_clk;
 	struct clk *grf_clk;
+
+	struct dw_hdmi *hdmi;
 };
 
 #define to_rockchip_hdmi(x)	container_of(x, struct rockchip_hdmi, x)

@@ -377,22 +379,24 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	drm_encoder_init(drm, encoder, &dw_hdmi_rockchip_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS, NULL);
 
-	ret = dw_hdmi_bind(pdev, encoder, plat_data);
+	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
+	if (IS_ERR(hdmi->hdmi)) {
+		encoder->funcs->destroy(encoder);
+		return PTR_ERR(hdmi->hdmi);
+	}
 
-	/*
-	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
-	 * which would have called the encoder cleanup.  Do it manually.
-	 */
-	if (ret)
-		drm_encoder_cleanup(encoder);
+	dev_set_drvdata(dev, hdmi);
 
-	return ret;
+	return 0;
 }
 
 static void dw_hdmi_rockchip_unbind(struct device *dev, struct device *master,
 				    void *data)
 {
-	return dw_hdmi_unbind(dev);
+	struct rockchip_hdmi *hdmi = dev_get_drvdata(dev);
+
+	dw_hdmi_unbind(hdmi->hdmi);
+	hdmi->encoder.funcs->destroy(&hdmi->encoder);
 }
 
 static const struct component_ops dw_hdmi_rockchip_ops = {
include/drm/bridge/dw_hdmi.h +9 -8
index 182f83283e24..29b8900caaf7 100644
@@ -143,14 +143,15 @@ struct dw_hdmi_plat_data {
 			     unsigned long mpixelclock);
 };
 
-int dw_hdmi_probe(struct platform_device *pdev,
-		  const struct dw_hdmi_plat_data *plat_data);
-void dw_hdmi_remove(struct platform_device *pdev);
-void dw_hdmi_unbind(struct device *dev);
-int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
-		 const struct dw_hdmi_plat_data *plat_data);
-
-void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense);
+struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
+			      const struct dw_hdmi_plat_data *plat_data);
+void dw_hdmi_remove(struct dw_hdmi *hdmi);
+void dw_hdmi_unbind(struct dw_hdmi *hdmi);
+struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev,
+			     struct drm_encoder *encoder,
+			     const struct dw_hdmi_plat_data *plat_data);
+
+void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense);
 
 void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
 void dw_hdmi_audio_enable(struct dw_hdmi *hdmi);
Signature · 1 line
2.14.1
maintainer
#16 lore raw

Re: [PATCH v8 7/8] drm/bridge/synopsys: dw-hdmi: Do not use device's drvdata

Am Mittwoch, 10. Januar 2018, 17:23:47 CET schrieb Thierry Escande:

Quoted 8 lines

From: Jeffy Chen <[email protected]>

Let plat drivers own the drvdata, so that they could cleanup resources in their unbind().

Signed-off-by Jeffy Chen <[email protected]>
Signed-off-by Thierry Escande <[email protected]>
Reviewed-by Neil Armstrong <[email protected]>

This looks like it is probably superseeded by "drm/bridge/synopsys: dw-hdmi: don't clobber drvdata" [0] ?

If there are still necessary things in it, it will need to be adapted for the recent addition of the sunxi dw-hdmi variant.

Heiko

[0] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=eea034af90c64802fd747a9dc534c26a7ebe7754

Quoted 328 lines
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c   | 43
++++++++++------------------- drivers/gpu/drm/imx/dw_hdmi-imx.c           |
22 +++++++++------
 drivers/gpu/drm/meson/meson_dw_hdmi.c       | 20 ++++++++++----
 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c      | 14 ++++++++--
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 22 +++++++++------
 include/drm/bridge/dw_hdmi.h                | 17 ++++++------
 6 files changed, 77 insertions(+), 61 deletions(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index
1cc63a18b7d5..f0380bcae513 100644

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +0 -1
@@ -2073,7 +2073,7 @@ static irqreturn_t dw_hdmi_hardirq(int irq, void
 *dev_id) return ret;
 }
 
-void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool

rx_sense) +void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense) {

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +14 -31
 	mutex_lock(&hdmi->mutex);

@@ -2099,13 +2099,6 @@ void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi,
 bool hpd, bool rx_sense) }
 	mutex_unlock(&hdmi->mutex);
 }
-
-void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense)
-{
-	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
-
-	__dw_hdmi_setup_rx_sense(hdmi, hpd, rx_sense);
-}
 EXPORT_SYMBOL_GPL(dw_hdmi_setup_rx_sense);
 
 static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)

@@ -2141,9 +2134,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
 	 */
 	if (intr_stat &
 	    (HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_IH_PHY_STAT0_HPD)) {
-		__dw_hdmi_setup_rx_sense(hdmi,
-					 phy_stat & HDMI_PHY_HPD,
-					 phy_stat & HDMI_PHY_RX_SENSE);
+		dw_hdmi_setup_rx_sense(hdmi, phy_stat & HDMI_PHY_HPD,
+				       phy_stat & HDMI_PHY_RX_SENSE);
 
 		if ((phy_stat & (HDMI_PHY_RX_SENSE | HDMI_PHY_HPD)) == 0)
 			cec_notifier_set_phys_addr(hdmi->cec_notifier,

@@ -2533,8 +2525,6 @@ __dw_hdmi_probe(struct platform_device *pdev,
 	if (hdmi->i2c)
 		dw_hdmi_i2c_init(hdmi);
 
-	platform_set_drvdata(pdev, hdmi);
-
 	return hdmi;
 
 err_iahb:

@@ -2584,25 +2574,23 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi)
 /*
---------------------------------------------------------------------------
-- * Probe/remove API, used from platforms based on the DRM bridge API. */
-int dw_hdmi_probe(struct platform_device *pdev,
-		  const struct dw_hdmi_plat_data *plat_data)
+struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
+			      const struct dw_hdmi_plat_data *plat_data)
 {
 	struct dw_hdmi *hdmi;
 
 	hdmi = __dw_hdmi_probe(pdev, plat_data);
 	if (IS_ERR(hdmi))
-		return PTR_ERR(hdmi);
+		return hdmi;
 
 	drm_bridge_add(&hdmi->bridge);
 
-	return 0;
+	return hdmi;
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_probe);
 
-void dw_hdmi_remove(struct platform_device *pdev)
+void dw_hdmi_remove(struct dw_hdmi *hdmi)
 {
-	struct dw_hdmi *hdmi = platform_get_drvdata(pdev);
-
 	drm_bridge_remove(&hdmi->bridge);
 
 	__dw_hdmi_remove(hdmi);

@@ -2612,31 +2600,30 @@ EXPORT_SYMBOL_GPL(dw_hdmi_remove);
 /*
---------------------------------------------------------------------------
-- * Bind/unbind API, used from platforms based on the component framework.
 */
-int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
-		 const struct dw_hdmi_plat_data *plat_data)
+struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev,
+			     struct drm_encoder *encoder,
+			     const struct dw_hdmi_plat_data *plat_data)
 {
 	struct dw_hdmi *hdmi;
 	int ret;
 
 	hdmi = __dw_hdmi_probe(pdev, plat_data);
 	if (IS_ERR(hdmi))
-		return PTR_ERR(hdmi);
+		return hdmi;
 
 	ret = drm_bridge_attach(encoder, &hdmi->bridge, NULL);
 	if (ret) {
 		__dw_hdmi_remove(hdmi);
 		DRM_ERROR("Failed to initialize bridge with drm\n");
-		return ret;
+		return ERR_PTR(ret);
 	}
 
-	return 0;
+	return hdmi;
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_bind);
 
-void dw_hdmi_unbind(struct device *dev)
+void dw_hdmi_unbind(struct dw_hdmi *hdmi)
 {
-	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
-
 	__dw_hdmi_remove(hdmi);
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
(quoted diff) +0 -0

b/drivers/gpu/drm/imx/dw_hdmi-imx.c index b62763aa8706..b01d03e02ce0 100644

drivers/gpu/drm/imx/dw_hdmi-imx.c +13 -9
@@ -26,6 +26,8 @@ struct imx_hdmi {
 	struct device *dev;
 	struct drm_encoder encoder;
 	struct regmap *regmap;
+
+	struct dw_hdmi *hdmi;
 };
 
 static inline struct imx_hdmi *enc_to_imx_hdmi(struct drm_encoder *e)

@@ -239,22 +241,24 @@ static int dw_hdmi_imx_bind(struct device *dev, struct
 device *master, drm_encoder_init(drm, encoder, &dw_hdmi_imx_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS, NULL);
 
-	ret = dw_hdmi_bind(pdev, encoder, plat_data);
+	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
+	if (IS_ERR(hdmi->hdmi)) {
+		encoder->funcs->destroy(encoder);
+		return PTR_ERR(hdmi->hdmi);
+	}
 
-	/*
-	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
-	 * which would have called the encoder cleanup.  Do it manually.
-	 */
-	if (ret)
-		drm_encoder_cleanup(encoder);
+	dev_set_drvdata(dev, hdmi);
 
-	return ret;
+	return 0;
 }
 
 static void dw_hdmi_imx_unbind(struct device *dev, struct device *master,
 			       void *data)
 {
-	return dw_hdmi_unbind(dev);
+	struct imx_hdmi *hdmi = dev_get_drvdata(dev);
+
+	dw_hdmi_unbind(hdmi->hdmi);
+	hdmi->encoder.funcs->destroy(&hdmi->encoder);
 }
 
 static const struct component_ops dw_hdmi_imx_ops = {
(quoted diff) +0 -0

b/drivers/gpu/drm/meson/meson_dw_hdmi.c index 17de3afd98f6..3f1ec9a5cbfb 100644

drivers/gpu/drm/meson/meson_dw_hdmi.c +3 -1
@@ -140,6 +140,8 @@ struct meson_dw_hdmi {
 	struct clk *venci_clk;
 	struct regulator *hdmi_supply;
 	u32 irq_stat;
+
+	struct dw_hdmi *hdmi;
 };
 #define encoder_to_meson_dw_hdmi(x) \
 	container_of(x, struct meson_dw_hdmi, encoder)

@@ -528,7 +530,7 @@ static irqreturn_t dw_hdmi_top_thread_irq(int irq, void
 *dev_id) if (stat & HDMITX_TOP_INTR_HPD_RISE)
 			hpd_connected = true;
 
-		dw_hdmi_setup_rx_sense(dw_hdmi->dev, hpd_connected,
+		dw_hdmi_setup_rx_sense(dw_hdmi->hdmi, hpd_connected,
 				       hpd_connected);
 
 		drm_helper_hpd_irq_event(dw_hdmi->encoder.dev);

@@ -878,9 +880,14 @@ static int meson_dw_hdmi_bind(struct device *dev,

struct device *master, dw_plat_data->input_bus_format = MEDIA_BUS_FMT_YUV8_1X24;

drivers/gpu/drm/meson/meson_dw_hdmi.c +12 -4
 	dw_plat_data->input_bus_encoding = V4L2_YCBCR_ENC_709;
 
-	ret = dw_hdmi_bind(pdev, encoder, &meson_dw_hdmi->dw_plat_data);
-	if (ret)
-		return ret;
+	meson_dw_hdmi->hdmi = dw_hdmi_bind(pdev, encoder,
+					   &meson_dw_hdmi->dw_plat_data);
+	if (IS_ERR(meson_dw_hdmi->hdmi)) {
+		encoder->funcs->destroy(encoder);
+		return PTR_ERR(meson_dw_hdmi->hdmi);
+	}
+
+	dev_set_drvdata(dev, meson_dw_hdmi);
 
 	DRM_DEBUG_DRIVER("HDMI controller initialized\n");

@@ -890,7 +897,10 @@ static int meson_dw_hdmi_bind(struct device *dev,
 struct device *master, static void meson_dw_hdmi_unbind(struct device *dev,
 struct device *master, void *data)
 {
-	dw_hdmi_unbind(dev);
+	struct meson_dw_hdmi *meson_dw_hdmi = dev_get_drvdata(dev);
+
+	dw_hdmi_unbind(meson_dw_hdmi->hdmi);
+	meson_dw_hdmi->encoder.funcs->destroy(&meson_dw_hdmi->encoder);
 }
 
 static const struct component_ops meson_dw_hdmi_ops = {
(quoted diff) +0 -0

b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c index dc85b53d58ef..76210ae25094 100644

drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c +12 -2
@@ -68,12 +68,22 @@ static const struct dw_hdmi_plat_data
 rcar_dw_hdmi_plat_data = {
 
 static int rcar_dw_hdmi_probe(struct platform_device *pdev)
 {
-	return dw_hdmi_probe(pdev, &rcar_dw_hdmi_plat_data);
+	struct dw_hdmi *hdmi;
+
+	hdmi = dw_hdmi_probe(pdev, &rcar_dw_hdmi_plat_data);
+	if (IS_ERR(hdmi))
+		return PTR_ERR(hdmi);
+
+	platform_set_drvdata(pdev, hdmi);
+
+	return 0;
 }
 
 static int rcar_dw_hdmi_remove(struct platform_device *pdev)
 {
-	dw_hdmi_remove(pdev);
+	struct dw_hdmi *hdmi = platform_get_drvdata(pdev);
+
+	dw_hdmi_remove(hdmi);
 
 	return 0;

}

(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c index 1eb02a82fd91..d406d7779ed9 100644

drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +13 -9
@@ -48,6 +48,8 @@ struct rockchip_hdmi {
 	const struct rockchip_hdmi_chip_data *chip_data;
 	struct clk *vpll_clk;
 	struct clk *grf_clk;
+
+	struct dw_hdmi *hdmi;
 };
 
 #define to_rockchip_hdmi(x)	container_of(x, struct rockchip_hdmi, x)

@@ -377,22 +379,24 @@ static int dw_hdmi_rockchip_bind(struct device *dev,
 struct device *master, drm_encoder_init(drm, encoder,
 &dw_hdmi_rockchip_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS, NULL);
 
-	ret = dw_hdmi_bind(pdev, encoder, plat_data);
+	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
+	if (IS_ERR(hdmi->hdmi)) {
+		encoder->funcs->destroy(encoder);
+		return PTR_ERR(hdmi->hdmi);
+	}
 
-	/*
-	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
-	 * which would have called the encoder cleanup.  Do it manually.
-	 */
-	if (ret)
-		drm_encoder_cleanup(encoder);
+	dev_set_drvdata(dev, hdmi);
 
-	return ret;
+	return 0;
 }
 
 static void dw_hdmi_rockchip_unbind(struct device *dev, struct device
 *master, void *data)
 {
-	return dw_hdmi_unbind(dev);
+	struct rockchip_hdmi *hdmi = dev_get_drvdata(dev);
+
+	dw_hdmi_unbind(hdmi->hdmi);
+	hdmi->encoder.funcs->destroy(&hdmi->encoder);
 }

static const struct component_ops dw_hdmi_rockchip_ops = {

include/drm/bridge/dw_hdmi.h +2 -8
index 182f83283e24..29b8900caaf7 100644
@@ -143,14 +143,15 @@ struct dw_hdmi_plat_data {
 			     unsigned long mpixelclock);
 };
 
-int dw_hdmi_probe(struct platform_device *pdev,
-		  const struct dw_hdmi_plat_data *plat_data);
-void dw_hdmi_remove(struct platform_device *pdev);
-void dw_hdmi_unbind(struct device *dev);
-int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
-		 const struct dw_hdmi_plat_data *plat_data);
-
-void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense);
+struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
+			      const struct dw_hdmi_plat_data *plat_data);

include/drm/bridge/dw_hdmi.h struct dw_hdmi_plat_data { · line 157

Quoted 10 lines
include/drm/bridge/dw_hdmi.h +7 -0
+void dw_hdmi_remove(struct dw_hdmi *hdmi);
+void dw_hdmi_unbind(struct dw_hdmi *hdmi);
+struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev,
+			     struct drm_encoder *encoder,
+			     const struct dw_hdmi_plat_data *plat_data);
+
+void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense);
 
 void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
 void dw_hdmi_audio_enable(struct dw_hdmi *hdmi);

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

author
#17 lore raw

PATCH v8 8/8 drm/rockchip: dw_hdmi: Fix error handling path

In reply to Thierry Escande

From: Jeffy Chen <[email protected]>

Add missing clk_disable_unprepare() in bind()'s error handling path and unbind().

Also inline clk_prepare_enable() with bind().

Fixes 12b9f204e804 ("drm: bridge/dw_hdmi: add rockchip rk3288 support")
Signed-off-by Jeffy Chen <[email protected]>
Signed-off-by Thierry Escande <[email protected]>
---
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +9 -8
index d406d7779ed9..0b2c1a5dd1e6 100644
@@ -166,7 +166,6 @@ static const struct dw_hdmi_phy_config rockchip_phy_config[] = {
 static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 {
 	struct device_node *np = hdmi->dev->of_node;
-	int ret;
 
 	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
 	if (IS_ERR(hdmi->regmap)) {

@@ -194,13 +193,6 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 		return PTR_ERR(hdmi->grf_clk);
 	}
 
-	ret = clk_prepare_enable(hdmi->vpll_clk);
-	if (ret) {
-		DRM_DEV_ERROR(hdmi->dev,
-			      "Failed to enable HDMI vpll: %d\n", ret);
-		return ret;
-	}
-
 	return 0;
 }
 

@@ -375,6 +367,13 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 		return ret;
 	}
 
+	ret = clk_prepare_enable(hdmi->vpll_clk);
+	if (ret) {
+		DRM_DEV_ERROR(hdmi->dev, "Failed to enable HDMI vpll: %d\n",
+			      ret);
+		return ret;
+	}
+
 	drm_encoder_helper_add(encoder, &dw_hdmi_rockchip_encoder_helper_funcs);
 	drm_encoder_init(drm, encoder, &dw_hdmi_rockchip_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS, NULL);

@@ -382,6 +381,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
 	if (IS_ERR(hdmi->hdmi)) {
 		encoder->funcs->destroy(encoder);
+		clk_disable_unprepare(hdmi->vpll_clk);
 		return PTR_ERR(hdmi->hdmi);
 	}
 

@@ -397,6 +397,7 @@ static void dw_hdmi_rockchip_unbind(struct device *dev, struct device *master,
 
 	dw_hdmi_unbind(hdmi->hdmi);
 	hdmi->encoder.funcs->destroy(&hdmi->encoder);
+	clk_disable_unprepare(hdmi->vpll_clk);
 }
 
 static const struct component_ops dw_hdmi_rockchip_ops = {
Signature · 1 line
2.14.1