[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 at 1 {
+			reg = <1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			edp_out_panel: endpoint at 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 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

[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 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

[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 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

[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 :)

[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 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

[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 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

[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(-)
> 

[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 at 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

[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

[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