[RFC PATCH v4 0/8] rockchip: kevin: Enable edp display

[RFC PATCH v4 0/8] rockchip: kevin: Enable edp display

[Jeffy Chen]

Make edp display works on chromebook kevin(at least for boot animation).

Also solve some issues i meet during the bringup.

Changes in v4:
Fix compile warning.

Changes in v3:
Assign orphan pwms to dummy pwmchip instead of adding device link in the
customer driver.

Changes in v2:
Use device link to correct the suspend/resume and shutdown ordering,
instead of converting rockchip spi's suspend/resume PM callbacks to
late suspend/resume PM callbacks.

Jeffy Chen (7):
  arm64: dts: rockchip: Enable edp disaplay on kevin
  drm/rockchip: analogix_dp: Fix error handling path
  drm/rockchip: dw-mipi-dsi: Fix error handling path
  drm/rockchip: dw_hdmi: Fix error handling path
  drm/rockchip: inno_hdmi: Fix error handling path
  pwm: Add dummy pwmchip for orphan pwms
  drm/rockchip: Add device links for master and components

Tomasz Figa (1):
  drm/bridge/analogix: Do not use device's drvdata

 arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts  | 29 ++++++++
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi       | 16 +++++
 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    | 57 +++++++++------
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c             | 17 +++--
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        |  4 +-
 drivers/gpu/drm/rockchip/inno_hdmi.c               | 20 ++++--
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c        | 24 ++++++-
 drivers/pwm/core.c                                 | 81 ++++++++++++++++++++--
 include/drm/bridge/analogix_dp.h                   | 19 ++---
 11 files changed, 254 insertions(+), 89 deletions(-)

-- 
2.11.0

[RFC PATCH v4 1/8] arm64: dts: rockchip: Enable edp disaplay on kevin

[Jeffy Chen]

Add edp panel and enable related nodes on kevin.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
Reviewed-by: Mark Yao <mark.yao@rock-chips.com>

---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 29 +++++++++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi      | 16 +++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
index a3d3cea7dc4f..bc67b19f0af5 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
@@ -93,6 +93,18 @@
 		pwm-delay-us = <10000>;
 	};
 
+	edp_panel: edp-panel {
+		compatible = "sharp,lq123p1jx31", "simple-panel";
+		backlight = <&backlight>;
+		power-supply = <&pp3300_disp>;
+
+		ports {
+			panel_in_edp: endpoint {
+				remote-endpoint = <&edp_out_panel>;
+			};
+		};
+	};
+
 	thermistor_ppvar_bigcpu: thermistor-ppvar-bigcpu {
 		compatible = "murata,ncp15wb473";
 		pullup-uv = <1800000>;
@@ -264,6 +276,23 @@ ap_i2c_dig: &i2c2 {
 	};
 };
 
+&edp {
+	status = "okay";
+
+	ports {
+		edp_out: port@1 {
+			reg = <1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			edp_out_panel: endpoint@0 {
+				reg = <0>;
+				remote-endpoint = <&panel_in_edp>;
+			};
+		};
+	};
+};
+
 &ppvar_bigcpu_pwm {
 	regulator-min-microvolt = <798674>;
 	regulator-max-microvolt = <1302172>;
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
index 5772c52fbfd3..470105d651c2 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
@@ -927,6 +927,22 @@ ap_i2c_audio: &i2c8 {
 	dr_mode = "host";
 };
 
+&vopb {
+	status = "okay";
+};
+
+&vopb_mmu {
+	status = "okay";
+};
+
+&vopl {
+	status = "okay";
+};
+
+&vopl_mmu {
+	status = "okay";
+};
+
 #include <arm/cros-ec-keyboard.dtsi>
 #include <arm/cros-ec-sbs.dtsi>
 
-- 
2.11.0

[RFC PATCH v4 2/8] drm/rockchip: analogix_dp: Fix error handling path

[Jeffy Chen]

Add missing error handling in rockchip_dp_bind().

Fixes: 9e32e16e9e98 ("drm: rockchip: dp: add rockchip platform dp driver")
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 4d3f6ad0abdd..4b689c0f3fc1 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -371,7 +371,7 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 	ret = rockchip_dp_drm_create_encoder(dp);
 	if (ret) {
 		DRM_ERROR("failed to create drm encoder\n");
-		return ret;
+		goto err_disable_pclk;
 	}
 
 	dp->plat_data.encoder = &dp->encoder;
@@ -387,7 +387,17 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 
 	rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);
 
-	return analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
+	ret = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
+	if (ret < 0)
+		goto err_unreg_psr;
+	return 0;
+
+err_unreg_psr:
+	rockchip_drm_psr_unregister(&dp->encoder);
+	rockchip_dp_drm_encoder_destroy(&dp->encoder);
+err_disable_pclk:
+	clk_disable_unprepare(dp->pclk);
+	return ret;
 }
 
 static void rockchip_dp_unbind(struct device *dev, struct device *master,
-- 
2.11.0

[RFC PATCH v4 3/8] drm/rockchip: dw-mipi-dsi: Fix error handling path

[Jeffy Chen]

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 <jeffy.chen@rock-chips.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
index b15755b6129c..a17ff0f6f489 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
@@ -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;
 	}
 
 	pm_runtime_enable(dev);
@@ -1292,23 +1292,24 @@ 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);
 	return 0;
 
-err_mipi_dsi_host:
+err_unreg_mipi_dsi_host:
 	mipi_dsi_host_unregister(&dsi->dsi_host);
-err_cleanup:
+err_disable_pm_runtime:
+	pm_runtime_disable(dev);
 	drm_encoder_cleanup(&dsi->encoder);
 	drm_connector_cleanup(&dsi->connector);
-err_pllref:
+err_disable_pllref:
 	clk_disable_unprepare(dsi->pllref_clk);
 	return ret;
 }
@@ -1320,6 +1321,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);
 }
 
-- 
2.11.0

[RFC PATCH v4 4/8] drm/rockchip: dw_hdmi: Fix error handling path

[Jeffy Chen]

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

Fixes: 12b9f204e804 ("drm: bridge/dw_hdmi: add rockchip rk3288 support")
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 1eb02a82fd91..582283da7861 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -383,8 +383,10 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
 	 * which would have called the encoder cleanup.  Do it manually.
 	 */
-	if (ret)
+	if (ret) {
 		drm_encoder_cleanup(encoder);
+		clk_disable_unprepare(hdmi->vpll_clk);
+	}
 
 	return ret;
 }
-- 
2.11.0

[RFC PATCH v4 5/8] drm/rockchip: inno_hdmi: Fix error handling path

[Jeffy Chen]

Add missing error handling in bind().

Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/rockchip/inno_hdmi.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index ee584d87111f..9c258b05dfa5 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -851,8 +851,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);
 
@@ -860,7 +862,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;
 	}
 
 	/*
@@ -874,7 +876,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);
 
@@ -884,7 +886,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:
+	drm_connector_cleanup(&hdmi->connector);
+	drm_encoder_cleanup(&hdmi->encoder);
+err_put_adapter:
+	i2c_put_adapter(hdmi->ddc);
+err_disable_clk:
+	clk_disable_unprepare(hdmi->pclk);
 	return ret;
 }
 
-- 
2.11.0

[RFC PATCH v4 6/8] drm/bridge/analogix: Do not use device's drvdata

[Jeffy Chen]

From: Tomasz Figa <tfiga@chromium.org>

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 <tfiga@chromium.org>
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 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    | 47 +++++++++++---------
 include/drm/bridge/analogix_dp.h                   | 19 ++++----
 4 files changed, 73 insertions(+), 69 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 5dd3f1cd074a..74d274b6d31d 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -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;
 
@@ -1281,8 +1278,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;
@@ -1292,14 +1290,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;
@@ -1316,7 +1312,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)) {
@@ -1330,14 +1326,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);
@@ -1346,7 +1342,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");
 
@@ -1367,7 +1363,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;
@@ -1379,7 +1375,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);
@@ -1420,7 +1416,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:
 
@@ -1428,15 +1424,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);
@@ -1449,16 +1442,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) {
@@ -1470,9 +1461,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);
diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
index 39629e7a80b9..f7e5b2c405ed 100644
--- a/drivers/gpu/drm/exynos/exynos_dp.c
+++ b/drivers/gpu/drm/exynos/exynos_dp.c
@@ -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
 
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 4b689c0f3fc1..b5f39bf59234 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -77,6 +77,7 @@ struct rockchip_dp_device {
 
 	const struct rockchip_dp_chip_data *data;
 
+	struct analogix_dp_device *adp;
 	struct analogix_dp_plat_data plat_data;
 };
 
@@ -85,7 +86,7 @@ static void analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
 	struct rockchip_dp_device *dp = to_dp(encoder);
 	unsigned long flags;
 
-	if (!analogix_dp_psr_supported(dp->dev))
+	if (!analogix_dp_psr_supported(dp->adp))
 		return;
 
 	DRM_DEV_DEBUG(dp->dev, "%s PSR...\n", enabled ? "Entry" : "Exit");
@@ -116,9 +117,9 @@ static void analogix_dp_psr_work(struct work_struct *work)
 
 	spin_lock_irqsave(&dp->psr_lock, flags);
 	if (dp->psr_state == EDP_VSC_PSR_STATE_ACTIVE)
-		analogix_dp_enable_psr(dp->dev);
+		analogix_dp_enable_psr(dp->adp);
 	else
-		analogix_dp_disable_psr(dp->dev);
+		analogix_dp_disable_psr(dp->adp);
 	spin_unlock_irqrestore(&dp->psr_lock, flags);
 }
 
@@ -350,13 +351,6 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 	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_data = of_device_get_match_data(dev);
 	if (!dp_data)
 		return -ENODEV;
@@ -387,9 +381,11 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 
 	rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);
 
-	ret = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
-	if (ret < 0)
+	dp->adp = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
+	if (IS_ERR(dp->adp)) {
+		ret = PTR_ERR(dp->adp);
 		goto err_unreg_psr;
+	}
 	return 0;
 
 err_unreg_psr:
@@ -407,7 +403,7 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
 
 	rockchip_drm_psr_unregister(&dp->encoder);
 
-	analogix_dp_unbind(dev, master, data);
+	analogix_dp_unbind(dp->adp);
 	clk_disable_unprepare(dp->pclk);
 }
 
@@ -435,11 +431,6 @@ static int rockchip_dp_probe(struct platform_device *pdev)
 
 	dp->plat_data.panel = panel;
 
-	/*
-	 * We just use the drvdata until driver run into component
-	 * add function, and then we would set drvdata to null, so
-	 * that analogix dp driver could take charge of the drvdata.
-	 */
 	platform_set_drvdata(pdev, dp);
 
 	return component_add(dev, &rockchip_dp_component_ops);
@@ -452,10 +443,26 @@ static int rockchip_dp_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int rockchip_dp_suspend(struct device *dev)
+{
+	struct rockchip_dp_device *dp = dev_get_drvdata(dev);
+
+	return analogix_dp_suspend(dp->adp);
+}
+
+static int rockchip_dp_resume(struct device *dev)
+{
+	struct rockchip_dp_device *dp = dev_get_drvdata(dev);
+
+	return analogix_dp_resume(dp->adp);
+}
+#endif
+
 static const struct dev_pm_ops rockchip_dp_pm_ops = {
 #ifdef CONFIG_PM_SLEEP
-	.suspend = analogix_dp_suspend,
-	.resume_early = analogix_dp_resume,
+	.suspend = rockchip_dp_suspend,
+	.resume_early = rockchip_dp_resume,
 #endif
 };
 
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index c99d6eaef1ac..5518fc75dd6e 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -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);
-- 
2.11.0

[RFC PATCH v4 7/8] pwm: Add dummy pwmchip for orphan pwms

[Jeffy Chen]

When the pwm driver is unbound while the pwm is still requested, the
pwm core would not actually remove the pwmchip(return -EBUSY instead).

So it would hold some references to the invalid resources(e.g. pwmchip).

And the customer who requested the pwm would have those references too,
and may crash the kernel when trying to access them later.

Add a dummy pwmchip, and assign orphan pwms to it to avoid that.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v4:
Fix compile warning.

Changes in v3:
Assign orphan pwms to dummy pwmchip instead of adding device link in the
customer driver.

Changes in v2: None

 drivers/pwm/core.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 75 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 1581f6ab1b1f..0b6697bc8ab8 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -41,6 +41,21 @@ static LIST_HEAD(pwm_chips);
 static DECLARE_BITMAP(allocated_pwms, MAX_PWMS);
 static RADIX_TREE(pwm_tree, GFP_KERNEL);
 
+static int dummy_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			   struct pwm_state *state)
+{
+	return -ENOTSUPP;
+}
+
+static const struct pwm_ops dummy_pwm_ops = {
+	.apply = dummy_pwm_apply,
+	.owner = THIS_MODULE,
+};
+
+static struct device dummy_pwm_dev = {
+	.init_name = "dummy-pwm",
+};
+
 static struct pwm_device *pwm_to_device(unsigned int pwm)
 {
 	return radix_tree_lookup(&pwm_tree, pwm);
@@ -334,6 +349,33 @@ int pwmchip_add(struct pwm_chip *chip)
 }
 EXPORT_SYMBOL_GPL(pwmchip_add);
 
+static int pwmchip_convert_dummy(struct pwm_chip *chip)
+{
+	struct pwm_chip *dummy;
+	unsigned int i;
+
+	dummy = kzalloc(sizeof(*dummy), GFP_KERNEL);
+	if (!dummy)
+		return -ENOMEM;
+
+	dummy->dev = &dummy_pwm_dev;
+	dummy->ops = &dummy_pwm_ops;
+	dummy->npwm = chip->npwm;
+	dummy->pwms = chip->pwms;
+
+	INIT_LIST_HEAD(&dummy->list);
+	list_add(&dummy->list, &pwm_chips);
+
+	for (i = 0; i < dummy->npwm; i++) {
+		struct pwm_device *pwm = &dummy->pwms[i];
+
+		pwm->chip = dummy;
+		pwm->state.enabled = false;
+	}
+
+	return 0;
+}
+
 /**
  * pwmchip_remove() - remove a PWM chip
  * @chip: the PWM chip to remove
@@ -346,7 +388,7 @@ EXPORT_SYMBOL_GPL(pwmchip_add);
 int pwmchip_remove(struct pwm_chip *chip)
 {
 	unsigned int i;
-	int ret = 0;
+	int ret = 0, requested = 0;
 
 	pwmchip_sysfs_unexport_children(chip);
 
@@ -356,8 +398,12 @@ int pwmchip_remove(struct pwm_chip *chip)
 		struct pwm_device *pwm = &chip->pwms[i];
 
 		if (test_bit(PWMF_REQUESTED, &pwm->flags)) {
-			ret = -EBUSY;
-			goto out;
+			requested = 1;
+
+			if (pwm->chip->ops->free)
+				pwm->chip->ops->free(pwm->chip, pwm);
+
+			module_put(pwm->chip->ops->owner);
 		}
 	}
 
@@ -366,11 +412,13 @@ int pwmchip_remove(struct pwm_chip *chip)
 	if (IS_ENABLED(CONFIG_OF))
 		of_pwmchip_remove(chip);
 
-	free_pwms(chip);
+	if (requested)
+		pwmchip_convert_dummy(chip);
+	else
+		free_pwms(chip);
 
 	pwmchip_sysfs_unexport(chip);
 
-out:
 	mutex_unlock(&pwm_lock);
 	return ret;
 }
@@ -855,6 +903,24 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
 }
 EXPORT_SYMBOL_GPL(pwm_get);
 
+static int pwmchip_remove_dummy(struct pwm_chip *dummy)
+{
+	unsigned int i;
+
+	for (i = 0; i < dummy->npwm; i++) {
+		struct pwm_device *pwm = &dummy->pwms[i];
+
+		if (test_bit(PWMF_REQUESTED, &pwm->flags))
+			return -EBUSY;
+	}
+
+	free_pwms(dummy);
+	list_del_init(&dummy->list);
+	kfree(dummy);
+
+	return 0;
+}
+
 /**
  * pwm_put() - release a PWM device
  * @pwm: PWM device
@@ -876,7 +942,10 @@ void pwm_put(struct pwm_device *pwm)
 
 	pwm->label = NULL;
 
-	module_put(pwm->chip->ops->owner);
+	if (pwm->chip->ops == &dummy_pwm_ops)
+		pwmchip_remove_dummy(pwm->chip);
+	else
+		module_put(pwm->chip->ops->owner);
 out:
 	mutex_unlock(&pwm_lock);
 }
-- 
2.11.0

[RFC PATCH v4 8/8] drm/rockchip: Add device links for master and components

[Jeffy Chen]

Since we are trying to access components' resources in the master's
suspend/resume PM callbacks(e.g. panel), add device links to correct
the suspend/resume and shutdown ordering.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2:
Use device link to correct the suspend/resume and shutdown ordering,
instead of converting rockchip spi's suspend/resume PM callbacks to
late suspend/resume PM callbacks.

 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 76d63de5921d..af18967f699b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -337,6 +337,8 @@ static struct component_match *rockchip_drm_match_add(struct device *dev)
 
 			if (!d)
 				break;
+
+			device_link_add(dev, d, DL_FLAG_STATELESS);
 			component_match_add(dev, &match, compare_dev, d);
 		} while (true);
 	}
@@ -406,6 +408,7 @@ static int rockchip_drm_platform_of_probe(struct device *dev)
 static int rockchip_drm_platform_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct device_link *link;
 	struct component_match *match = NULL;
 	int ret;
 
@@ -414,16 +417,31 @@ static int rockchip_drm_platform_probe(struct platform_device *pdev)
 		return ret;
 
 	match = rockchip_drm_match_add(dev);
-	if (IS_ERR(match))
-		return PTR_ERR(match);
+	if (IS_ERR(match)) {
+		ret = PTR_ERR(match);
+		goto err_cleanup_dev_links;
+	}
 
-	return component_master_add_with_match(dev, &rockchip_drm_ops, match);
+	ret = component_master_add_with_match(dev, &rockchip_drm_ops, match);
+	if (ret < 0)
+		goto err_cleanup_dev_links;
+
+	return 0;
+err_cleanup_dev_links:
+	list_for_each_entry(link, &dev->links.consumers, s_node)
+		device_link_del(link);
+	return ret;
 }
 
 static int rockchip_drm_platform_remove(struct platform_device *pdev)
 {
+	struct device_link *link;
+
 	component_master_del(&pdev->dev, &rockchip_drm_ops);
 
+	list_for_each_entry(link, &pdev->dev.links.consumers, s_node)
+		device_link_del(link);
+
 	return 0;
 }
 
-- 
2.11.0

Re: [RFC PATCH v4 7/8] pwm: Add dummy pwmchip for orphan pwms

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 908 bytes --]

On Tue, Oct 17, 2017 at 06:16:23PM +0800, Jeffy Chen wrote:
> When the pwm driver is unbound while the pwm is still requested, the
> pwm core would not actually remove the pwmchip(return -EBUSY instead).
> 
> So it would hold some references to the invalid resources(e.g. pwmchip).
> 
> And the customer who requested the pwm would have those references too,
> and may crash the kernel when trying to access them later.
> 
> Add a dummy pwmchip, and assign orphan pwms to it to avoid that.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> Changes in v4:
> Fix compile warning.
> 
> Changes in v3:
> Assign orphan pwms to dummy pwmchip instead of adding device link in the
> customer driver.

What happened to this? Device links were specifically designed to avoid
situations like these.

A dummy PWM chip doesn't seem like the right solution to this.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH v4 7/8] pwm: Add dummy pwmchip for orphan pwms

[Brian Norris]

Hi,

On Tue, Oct 17, 2017 at 02:40:31PM +0200, Thierry Reding wrote:
> On Tue, Oct 17, 2017 at 06:16:23PM +0800, Jeffy Chen wrote:
> > When the pwm driver is unbound while the pwm is still requested, the
> > pwm core would not actually remove the pwmchip(return -EBUSY instead).
> > 
> > So it would hold some references to the invalid resources(e.g. pwmchip).
> > 
> > And the customer who requested the pwm would have those references too,
> > and may crash the kernel when trying to access them later.
> > 
> > Add a dummy pwmchip, and assign orphan pwms to it to avoid that.
> > 
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > ---
> > 
> > Changes in v4:
> > Fix compile warning.
> > 
> > Changes in v3:
> > Assign orphan pwms to dummy pwmchip instead of adding device link in the
> > customer driver.
> 
> What happened to this? Device links were specifically designed to avoid
> situations like these.

I think Jeffy came up with this as an odd response to my suggestion on
v2 that we could just handle the device links in the PWM core. I don't
fully understand why the complete change in direction...

BTW, since you seem to have an opinion about device links: is it
expected that all consumer drivers will make explicit calls to
device_link_add()? I thought this should be avoided, if possible (e.g.,
this can be handled in pwm_get()).

> A dummy PWM chip doesn't seem like the right solution to this.

Agreed.

Brian

Re: [RFC PATCH v4 2/8] drm/rockchip: analogix_dp: Fix error handling path

[Sean Paul]

On Tue, Oct 17, 2017 at 06:16:18PM +0800, Jeffy Chen wrote:
> Add missing error handling in rockchip_dp_bind().
> 
> Fixes: 9e32e16e9e98 ("drm: rockchip: dp: add rockchip platform dp driver")
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index 4d3f6ad0abdd..4b689c0f3fc1 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -371,7 +371,7 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
>  	ret = rockchip_dp_drm_create_encoder(dp);
>  	if (ret) {
>  		DRM_ERROR("failed to create drm encoder\n");
> -		return ret;
> +		goto err_disable_pclk;
>  	}
>  
>  	dp->plat_data.encoder = &dp->encoder;
> @@ -387,7 +387,17 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
>  
>  	rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);
>  
> -	return analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
> +	ret = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
> +	if (ret < 0)
> +		goto err_unreg_psr;
> +	return 0;
> +
> +err_unreg_psr:
> +	rockchip_drm_psr_unregister(&dp->encoder);
> +	rockchip_dp_drm_encoder_destroy(&dp->encoder);
> +err_disable_pclk:
> +	clk_disable_unprepare(dp->pclk);

Hi Jeffy,
This part of the cleanup is handling things setup by rockchip_dp_init().
However if someone adds something to rockchip_dp_init(), it's not obvious that
it should be cleaned up here and in unbind(). I'd suggest rebasing this on
a new patch which folds everything rockchip_dp_init() does into
rockchip_dp_bind(), then it will be obvious what needs to be cleaned up.

Sean


> +	return ret;
>  }
>  
>  static void rockchip_dp_unbind(struct device *dev, struct device *master,
> -- 
> 2.11.0
> 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [RFC PATCH v4 3/8] drm/rockchip: dw-mipi-dsi: Fix error handling path

[Sean Paul]

On Tue, Oct 17, 2017 at 06:16:19PM +0800, Jeffy Chen wrote:
> 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 <jeffy.chen@rock-chips.com>
> ---
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index b15755b6129c..a17ff0f6f489 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> @@ -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;
>  	}
>  
>  	pm_runtime_enable(dev);
> @@ -1292,23 +1292,24 @@ 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);
>  	return 0;
>  
> -err_mipi_dsi_host:
> +err_unreg_mipi_dsi_host:
>  	mipi_dsi_host_unregister(&dsi->dsi_host);
> -err_cleanup:
> +err_disable_pm_runtime:
> +	pm_runtime_disable(dev);
>  	drm_encoder_cleanup(&dsi->encoder);
>  	drm_connector_cleanup(&dsi->connector);

Should you update these to call the destroy hook like in unbind()?

Sean

> -err_pllref:
> +err_disable_pllref:
>  	clk_disable_unprepare(dsi->pllref_clk);
>  	return ret;
>  }
> @@ -1320,6 +1321,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);
>  }
>  
> -- 
> 2.11.0
> 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [RFC PATCH v4 4/8] drm/rockchip: dw_hdmi: Fix error handling path

[Sean Paul]

On Tue, Oct 17, 2017 at 06:16:20PM +0800, Jeffy Chen wrote:
> Add missing clk_disable_unprepare() in bind()'s error handling path.

This also isn't disabled in unbind(), is that intentional?

> 
> Fixes: 12b9f204e804 ("drm: bridge/dw_hdmi: add rockchip rk3288 support")
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index 1eb02a82fd91..582283da7861 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -383,8 +383,10 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>  	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
>  	 * which would have called the encoder cleanup.  Do it manually.
>  	 */
> -	if (ret)
> +	if (ret) {
>  		drm_encoder_cleanup(encoder);
> +		clk_disable_unprepare(hdmi->vpll_clk);

Same comment with respect to rockchip_hdmi_parse_dt(). This bug would have
probably been avoided if the contents of rockchip_hdmi_parse_dt() were inline
with dw_hdmi_rockchip_bind(), since then it's obvious what needs to be cleaned
up.

Sean

> +	}
>  
>  	return ret;
>  }
> -- 
> 2.11.0
> 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [RFC PATCH v4 5/8] drm/rockchip: inno_hdmi: Fix error handling path

[Sean Paul]

On Tue, Oct 17, 2017 at 06:16:21PM +0800, Jeffy Chen wrote:
> Add missing error handling in bind().
> 
> Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/gpu/drm/rockchip/inno_hdmi.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> index ee584d87111f..9c258b05dfa5 100644
> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> @@ -851,8 +851,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);
>  
> @@ -860,7 +862,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;
>  	}
>  
>  	/*
> @@ -874,7 +876,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);
>  
> @@ -884,7 +886,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:
> +	drm_connector_cleanup(&hdmi->connector);
> +	drm_encoder_cleanup(&hdmi->encoder);

Same question regarding cleanup vs destroy.

> +err_put_adapter:
> +	i2c_put_adapter(hdmi->ddc);
> +err_disable_clk:
> +	clk_disable_unprepare(hdmi->pclk);

I also noticed the order of these two functions is reversed in unbind(). Can you
please update the order to match?

Sean

>  	return ret;
>  }
>  
> -- 
> 2.11.0
> 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [RFC PATCH v4 6/8] drm/bridge/analogix: Do not use device's drvdata

[Sean Paul]

On Tue, Oct 17, 2017 at 06:16:22PM +0800, Jeffy Chen wrote:
> From: Tomasz Figa <tfiga@chromium.org>
> 
> 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 <tfiga@chromium.org>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  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    | 47 +++++++++++---------
>  include/drm/bridge/analogix_dp.h                   | 19 ++++----
>  4 files changed, 73 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 5dd3f1cd074a..74d274b6d31d 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -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;
>  
> @@ -1281,8 +1278,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;
> @@ -1292,14 +1290,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;
> @@ -1316,7 +1312,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)) {
> @@ -1330,14 +1326,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);
> @@ -1346,7 +1342,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");
>  
> @@ -1367,7 +1363,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;
> @@ -1379,7 +1375,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);
> @@ -1420,7 +1416,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:
>  
> @@ -1428,15 +1424,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);
> @@ -1449,16 +1442,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) {
> @@ -1470,9 +1461,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);
> diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
> index 39629e7a80b9..f7e5b2c405ed 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp.c
> @@ -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
>  
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index 4b689c0f3fc1..b5f39bf59234 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -77,6 +77,7 @@ struct rockchip_dp_device {
>  
>  	const struct rockchip_dp_chip_data *data;
>  
> +	struct analogix_dp_device *adp;
>  	struct analogix_dp_plat_data plat_data;
>  };
>  
> @@ -85,7 +86,7 @@ static void analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
>  	struct rockchip_dp_device *dp = to_dp(encoder);
>  	unsigned long flags;
>  
> -	if (!analogix_dp_psr_supported(dp->dev))
> +	if (!analogix_dp_psr_supported(dp->adp))
>  		return;
>  
>  	DRM_DEV_DEBUG(dp->dev, "%s PSR...\n", enabled ? "Entry" : "Exit");
> @@ -116,9 +117,9 @@ static void analogix_dp_psr_work(struct work_struct *work)
>  
>  	spin_lock_irqsave(&dp->psr_lock, flags);
>  	if (dp->psr_state == EDP_VSC_PSR_STATE_ACTIVE)
> -		analogix_dp_enable_psr(dp->dev);
> +		analogix_dp_enable_psr(dp->adp);
>  	else
> -		analogix_dp_disable_psr(dp->dev);
> +		analogix_dp_disable_psr(dp->adp);
>  	spin_unlock_irqrestore(&dp->psr_lock, flags);
>  }
>  
> @@ -350,13 +351,6 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
>  	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_data = of_device_get_match_data(dev);
>  	if (!dp_data)
>  		return -ENODEV;
> @@ -387,9 +381,11 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
>  
>  	rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);
>  
> -	ret = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
> -	if (ret < 0)
> +	dp->adp = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
> +	if (IS_ERR(dp->adp)) {
> +		ret = PTR_ERR(dp->adp);
>  		goto err_unreg_psr;
> +	}
>  	return 0;
>  
>  err_unreg_psr:
> @@ -407,7 +403,7 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
>  
>  	rockchip_drm_psr_unregister(&dp->encoder);
>  
> -	analogix_dp_unbind(dev, master, data);
> +	analogix_dp_unbind(dp->adp);
>  	clk_disable_unprepare(dp->pclk);
>  }
>  
> @@ -435,11 +431,6 @@ static int rockchip_dp_probe(struct platform_device *pdev)
>  
>  	dp->plat_data.panel = panel;
>  
> -	/*
> -	 * We just use the drvdata until driver run into component
> -	 * add function, and then we would set drvdata to null, so
> -	 * that analogix dp driver could take charge of the drvdata.
> -	 */
>  	platform_set_drvdata(pdev, dp);
>  
>  	return component_add(dev, &rockchip_dp_component_ops);
> @@ -452,10 +443,26 @@ static int rockchip_dp_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int rockchip_dp_suspend(struct device *dev)
> +{
> +	struct rockchip_dp_device *dp = dev_get_drvdata(dev);
> +
> +	return analogix_dp_suspend(dp->adp);
> +}
> +
> +static int rockchip_dp_resume(struct device *dev)
> +{
> +	struct rockchip_dp_device *dp = dev_get_drvdata(dev);
> +
> +	return analogix_dp_resume(dp->adp);
> +}
> +#endif
> +
>  static const struct dev_pm_ops rockchip_dp_pm_ops = {
>  #ifdef CONFIG_PM_SLEEP
> -	.suspend = analogix_dp_suspend,
> -	.resume_early = analogix_dp_resume,
> +	.suspend = rockchip_dp_suspend,
> +	.resume_early = rockchip_dp_resume,
>  #endif
>  };
>  
> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> index c99d6eaef1ac..5518fc75dd6e 100644
> --- a/include/drm/bridge/analogix_dp.h
> +++ b/include/drm/bridge/analogix_dp.h
> @@ -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);
> -- 
> 2.11.0
> 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [RFC PATCH v4 7/8] pwm: Add dummy pwmchip for orphan pwms

[Dmitry Torokhov]

On Tue, Oct 17, 2017 at 10:04 AM, Brian Norris <briannorris@chromium.org> wrote:
> Hi,
>
> On Tue, Oct 17, 2017 at 02:40:31PM +0200, Thierry Reding wrote:
>> On Tue, Oct 17, 2017 at 06:16:23PM +0800, Jeffy Chen wrote:
>> > When the pwm driver is unbound while the pwm is still requested, the
>> > pwm core would not actually remove the pwmchip(return -EBUSY instead).
>> >
>> > So it would hold some references to the invalid resources(e.g. pwmchip).
>> >
>> > And the customer who requested the pwm would have those references too,
>> > and may crash the kernel when trying to access them later.
>> >
>> > Add a dummy pwmchip, and assign orphan pwms to it to avoid that.
>> >
>> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> > ---
>> >
>> > Changes in v4:
>> > Fix compile warning.
>> >
>> > Changes in v3:
>> > Assign orphan pwms to dummy pwmchip instead of adding device link in the
>> > customer driver.
>>
>> What happened to this? Device links were specifically designed to avoid
>> situations like these.
>
> I think Jeffy came up with this as an odd response to my suggestion on
> v2 that we could just handle the device links in the PWM core. I don't
> fully understand why the complete change in direction...
>
> BTW, since you seem to have an opinion about device links: is it
> expected that all consumer drivers will make explicit calls to
> device_link_add()? I thought this should be avoided, if possible (e.g.,
> this can be handled in pwm_get()).

Ideally we would not have this in core kernel API (pwm_get, gpiod_get,
regulator_get, etc) but retrieve it form the firmware (device tree,
ACPI) and use this data not only on suspend/resume but for probing as
well. *How exactly* can we do that is still not clear though, so maybe
we could plug the biggest holes by actually adding device links calls
to the main devm_<object>_get() users...

Thanks.

-- 
Dmitry

Re: [RFC PATCH v4 8/8] drm/rockchip: Add device links for master and components

[Sean Paul]

On Tue, Oct 17, 2017 at 06:16:24PM +0800, Jeffy Chen wrote:
> Since we are trying to access components' resources in the master's
> suspend/resume PM callbacks(e.g. panel), add device links to correct
> the suspend/resume and shutdown ordering.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2:
> Use device link to correct the suspend/resume and shutdown ordering,
> instead of converting rockchip spi's suspend/resume PM callbacks to
> late suspend/resume PM callbacks.
> 
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index 76d63de5921d..af18967f699b 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -337,6 +337,8 @@ static struct component_match *rockchip_drm_match_add(struct device *dev)
>  
>  			if (!d)
>  				break;
> +
> +			device_link_add(dev, d, DL_FLAG_STATELESS);
>  			component_match_add(dev, &match, compare_dev, d);
>  		} while (true);
>  	}
> @@ -406,6 +408,7 @@ static int rockchip_drm_platform_of_probe(struct device *dev)
>  static int rockchip_drm_platform_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	struct device_link *link;
>  	struct component_match *match = NULL;
>  	int ret;
>  
> @@ -414,16 +417,31 @@ static int rockchip_drm_platform_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	match = rockchip_drm_match_add(dev);
> -	if (IS_ERR(match))
> -		return PTR_ERR(match);
> +	if (IS_ERR(match)) {
> +		ret = PTR_ERR(match);
> +		goto err_cleanup_dev_links;

This cleanup should take place in rockchip_drm_match_add(). The review theme for
this entire series is that, when possible, we should cleanup things where they
are initialized.

Since you'll also need to clean up the links elsewhere, consider adding a helper
function to do the cleanup (rockchip_drm_match_remove or similar) and calling it
where needed.

Sean

> +	}
>  
> -	return component_master_add_with_match(dev, &rockchip_drm_ops, match);
> +	ret = component_master_add_with_match(dev, &rockchip_drm_ops, match);
> +	if (ret < 0)
> +		goto err_cleanup_dev_links;
> +
> +	return 0;
> +err_cleanup_dev_links:
> +	list_for_each_entry(link, &dev->links.consumers, s_node)
> +		device_link_del(link);
> +	return ret;
>  }
>  
>  static int rockchip_drm_platform_remove(struct platform_device *pdev)
>  {
> +	struct device_link *link;
> +
>  	component_master_del(&pdev->dev, &rockchip_drm_ops);
>  
> +	list_for_each_entry(link, &pdev->dev.links.consumers, s_node)
> +		device_link_del(link);
> +
>  	return 0;
>  }
>  
> -- 
> 2.11.0
> 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [RFC PATCH v4 7/8] pwm: Add dummy pwmchip for orphan pwms

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]

On Tue, Oct 17, 2017 at 11:24:24AM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 17, 2017 at 10:04 AM, Brian Norris <briannorris@chromium.org> wrote:

> > BTW, since you seem to have an opinion about device links: is it
> > expected that all consumer drivers will make explicit calls to
> > device_link_add()? I thought this should be avoided, if possible (e.g.,
> > this can be handled in pwm_get()).

> Ideally we would not have this in core kernel API (pwm_get, gpiod_get,
> regulator_get, etc) but retrieve it form the firmware (device tree,
> ACPI) and use this data not only on suspend/resume but for probing as

Right, the major initial push here was for ordering of probes so doing
it in subsystems or drivers is too late.

> well. *How exactly* can we do that is still not clear though, so maybe
> we could plug the biggest holes by actually adding device links calls
> to the main devm_<object>_get() users...

I would expect we can get a long way in the DT by doing a pass over the
tree and adding links between device nodes in cases where phandle
references exist.  There is a potential issue with circular links which
I'm just going to handwave away right now but I'd expect that to help
otherwise.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH v4 7/8] pwm: Add dummy pwmchip for orphan pwms

[Brian Norris]

On Tue, Oct 17, 2017 at 07:46:03PM +0100, Mark Brown wrote:
> On Tue, Oct 17, 2017 at 11:24:24AM -0700, Dmitry Torokhov wrote:
> > On Tue, Oct 17, 2017 at 10:04 AM, Brian Norris <briannorris@chromium.org> wrote:
> 
> > > BTW, since you seem to have an opinion about device links: is it
> > > expected that all consumer drivers will make explicit calls to
> > > device_link_add()? I thought this should be avoided, if possible (e.g.,
> > > this can be handled in pwm_get()).
> 
> > Ideally we would not have this in core kernel API (pwm_get, gpiod_get,
> > regulator_get, etc) but retrieve it form the firmware (device tree,
> > ACPI) and use this data not only on suspend/resume but for probing as
> 
> Right, the major initial push here was for ordering of probes so doing
> it in subsystems or drivers is too late.
> 
> > well. *How exactly* can we do that is still not clear though, so maybe
> > we could plug the biggest holes by actually adding device links calls
> > to the main devm_<object>_get() users...
> 
> I would expect we can get a long way in the DT by doing a pass over the
> tree and adding links between device nodes in cases where phandle
> references exist.  There is a potential issue with circular links which
> I'm just going to handwave away right now but I'd expect that to help
> otherwise.

But I didn't think FDTs encoded type info. So you don't really know
whether a phandle is a phandle -- it's just an int (which happens to
have a corresponding property in some other node). Are we trusting our
DT bindings well enough to say that, for example, we know that in any
given device node, a property like 'pwms' must be a phandle to a PWM
provider? OK, maybe 'pwms' is a bad example (it's unlikely to get
reused, and it has a companion '#pwm-cells' property), but grepping the
DT bindings directory shows a ton of one-off properties that contain
phandles.

Brian

Re: [RFC PATCH v4 7/8] pwm: Add dummy pwmchip for orphan pwms

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1212 bytes --]

On Tue, Oct 17, 2017 at 11:53:01AM -0700, Brian Norris wrote:
> On Tue, Oct 17, 2017 at 07:46:03PM +0100, Mark Brown wrote:

> > I would expect we can get a long way in the DT by doing a pass over the
> > tree and adding links between device nodes in cases where phandle
> > references exist.  There is a potential issue with circular links which
> > I'm just going to handwave away right now but I'd expect that to help
> > otherwise.

> But I didn't think FDTs encoded type info. So you don't really know
> whether a phandle is a phandle -- it's just an int (which happens to
> have a corresponding property in some other node). Are we trusting our
> DT bindings well enough to say that, for example, we know that in any
> given device node, a property like 'pwms' must be a phandle to a PWM
> provider? OK, maybe 'pwms' is a bad example (it's unlikely to get
> reused, and it has a companion '#pwm-cells' property), but grepping the
> DT bindings directory shows a ton of one-off properties that contain
> phandles.

If we're going with the 90% thing we can probably get a long way with a
whitelist of properties, and we'll be able to take that a lot further
with the validatable schemas if they ever happen.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH v4 6/8] drm/bridge/analogix: Do not use device's drvdata

[Jingoo Han]

On Tuesday, October 17, 2017 6:16 AM, Jeffy Chen wrote:
> 
> From: Tomasz Figa <tfiga@chromium.org>
> 
> 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 <tfiga@chromium.org>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Acked-by: Jingoo Han <jingoohan1@gmail.com>

Best regards,
Jingoo Han

> ---
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  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    | 47
+++++++++++-------
> --
>  include/drm/bridge/analogix_dp.h                   | 19 ++++----
>  4 files changed, 73 insertions(+), 69 deletions(-)
> 

Re: [RFC PATCH v4 6/8] drm/bridge/analogix: Do not use device's drvdata

[Archit Taneja]

Hi,

On 10/18/2017 05:13 AM, Jingoo Han wrote:
> On Tuesday, October 17, 2017 6:16 AM, Jeffy Chen wrote:
>>
>> From: Tomasz Figa <tfiga@chromium.org>
>>
>> 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 <tfiga@chromium.org>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> 

This depends on previous patches of the series. I guess it would be easier
to queue this to drm-misc as a part of the eDP support series. For that:

Acked-by: Archit Taneja <architt@codeaurora.org>

> Acked-by: Jingoo Han <jingoohan1@gmail.com>
> 
> Best regards,
> Jingoo Han
> 
>> ---
>>
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>>   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    | 47
> +++++++++++-------
>> --
>>   include/drm/bridge/analogix_dp.h                   | 19 ++++----
>>   4 files changed, 73 insertions(+), 69 deletions(-)
>>
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH v4 7/8] pwm: Add dummy pwmchip for orphan pwms

[jeffy]

Hi guys,

On 10/18/2017 03:05 AM, Mark Brown wrote:
> On Tue, Oct 17, 2017 at 11:53:01AM -0700, Brian Norris wrote:
>> On Tue, Oct 17, 2017 at 07:46:03PM +0100, Mark Brown wrote:
>
>>> I would expect we can get a long way in the DT by doing a pass over the
>>> tree and adding links between device nodes in cases where phandle
>>> references exist.  There is a potential issue with circular links which
>>> I'm just going to handwave away right now but I'd expect that to help
>>> otherwise.
>
>> But I didn't think FDTs encoded type info. So you don't really know
>> whether a phandle is a phandle -- it's just an int (which happens to
>> have a corresponding property in some other node). Are we trusting our
>> DT bindings well enough to say that, for example, we know that in any
>> given device node, a property like 'pwms' must be a phandle to a PWM
>> provider? OK, maybe 'pwms' is a bad example (it's unlikely to get
>> reused, and it has a companion '#pwm-cells' property), but grepping the
>> DT bindings directory shows a ton of one-off properties that contain
>> phandles.
>
> If we're going with the 90% thing we can probably get a long way with a
> whitelist of properties, and we'll be able to take that a lot further
> with the validatable schemas if they ever happen.
>

so it looks like we are going to use device link in common code to fix 
this issue(and also other dependency issue), then i will drop this patch 
and the followed rockchip drm device link patch in next version :)


also the reason why i try to use dummy chip instead is that:

currently we have these devices: rockchip spi device(master) -> 
cros_ec_spi device(child) -> cros_ec_pwm(spi based pwm) -> pwm_bl

i added device link to cros_ec_pwm and pwm_bl, that works well for 
unbind/bind cros_ec_pwm device case, but there's a warning when i try to 
unbind cros_ec_spi:


static void device_links_purge(struct device *dev)
{
...
         list_for_each_entry_safe_reverse(link, ln, 
&dev->links.consumers, s_node) {
                 WARN_ON(link->status != DL_STATE_DORMANT &&
                         link->status != DL_STATE_NONE);        <--- hit 
warning here!
                 __device_link_del(link);
         }


but i checked again, that could due to the way spi core unregester 
children devices. maybe it need to call device_release_driver

Re: [RFC PATCH v4 4/8] drm/rockchip: dw_hdmi: Fix error handling path

[jeffy]

Hi Sean,

Thanks for your review.

On 10/18/2017 02:10 AM, Sean Paul wrote:
> On Tue, Oct 17, 2017 at 06:16:20PM +0800, Jeffy Chen wrote:
>> >Add missing clk_disable_unprepare() in bind()'s error handling path.
> This also isn't disabled in unbind(), is that intentional?
>
i wasn't able to do that because of dw_hdmi drvdata.

i've modified it as tomasz did for dp bridge, and fixed the unbind() in 
the next version :)