[PATCH v6 00/10] rockchip: kevin: Enable edp display

[PATCH v6 00/10] 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 v6:
Don't change order of rockchip_drm_psr_register().

Changes in v5:
Call the destroy hook in the error handling path like in unbind().
Call the destroy hook in the error handling path like in unbind().
Update cleanup order in unbind().
Add disable to unbind(), and inline clk_prepare_enable() with bind().

Jeffy Chen (10):
  arm64: dts: rockchip: Enable edp disaplay on kevin
  drm/rockchip: analogix_dp: Remove unnecessary init code
  drm/bridge: analogix: Do not use device's drvdata
  drm/bridge: analogix_dp: Fix connector and encoder cleanup
  drm/rockchip: analogix_dp: Add a sanity check for
    rockchip_drm_psr_register()
  drm/rockchip: dw-mipi-dsi: Fix error handling path
  drm/rockchip: inno_hdmi: Fix error handling path
  drm/bridge/synopsys: dw-hdmi: Add missing bridge detach
  drm/bridge/synopsys: dw-hdmi: Do not use device's drvdata
  drm/rockchip: dw_hdmi: Fix error handling path

 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 | 52 +++++-------
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          | 53 ++++++------
 drivers/gpu/drm/exynos/exynos_dp.c                 | 29 ++++---
 drivers/gpu/drm/imx/dw_hdmi-imx.c                  | 22 +++--
 drivers/gpu/drm/meson/meson_dw_hdmi.c              | 20 +++--
 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c             | 14 +++-
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 95 +++++++++++-----------
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c             | 21 +++--
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        | 39 +++++----
 drivers/gpu/drm/rockchip/inno_hdmi.c               | 22 +++--
 include/drm/bridge/analogix_dp.h                   | 19 +++--
 include/drm/bridge/dw_hdmi.h                       | 17 ++--
 14 files changed, 265 insertions(+), 183 deletions(-)

-- 
2.11.0

[PATCH v6 01/10] 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 v6: None
Changes in v5: 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

Re: [PATCH v6 01/10] arm64: dts: rockchip: Enable edp disaplay on kevin

[Enric Balletbo Serra]

Hi all,

2017-10-19 5:48 GMT+02:00 Jeffy Chen <jeffy.chen@rock-chips.com>:
> 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 v6: None
> Changes in v5: 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
>
>

I just tested this patch on top of current mainline (4.15-rc1+) and works so,

Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Re: [PATCH v6 01/10] arm64: dts: rockchip: Enable edp disaplay on kevin

[Heiko Stuebner]

Am Donnerstag, 19. Oktober 2017, 11:48:03 CET schrieb 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>

applied for 4.16 with Enric's Tested-tag and after also
seeing a bit of output on the edp.


Thanks
Heiko

[PATCH v6 02/10] drm/rockchip: analogix_dp: Remove unnecessary init code

[Jeffy Chen]

Remove unnecessary init code, since we would do it in the power_on()
callback.

Also move of parse code to probe().

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

Changes in v6: None
Changes in v5: None

 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 27 ++++++-------------------
 1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 4d3f6ad0abdd..8cae5ad926cd 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -269,7 +269,7 @@ static struct drm_encoder_funcs rockchip_dp_encoder_funcs = {
 	.destroy = rockchip_dp_drm_encoder_destroy,
 };
 
-static int rockchip_dp_init(struct rockchip_dp_device *dp)
+static int rockchip_dp_of_probe(struct rockchip_dp_device *dp)
 {
 	struct device *dev = dp->dev;
 	struct device_node *np = dev->of_node;
@@ -303,19 +303,6 @@ static int rockchip_dp_init(struct rockchip_dp_device *dp)
 		return PTR_ERR(dp->rst);
 	}
 
-	ret = clk_prepare_enable(dp->pclk);
-	if (ret < 0) {
-		DRM_DEV_ERROR(dp->dev, "failed to enable pclk %d\n", ret);
-		return ret;
-	}
-
-	ret = rockchip_dp_pre_init(dp);
-	if (ret < 0) {
-		DRM_DEV_ERROR(dp->dev, "failed to pre init %d\n", ret);
-		clk_disable_unprepare(dp->pclk);
-		return ret;
-	}
-
 	return 0;
 }
 
@@ -361,10 +348,6 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 	if (!dp_data)
 		return -ENODEV;
 
-	ret = rockchip_dp_init(dp);
-	if (ret < 0)
-		return ret;
-
 	dp->data = dp_data;
 	dp->drm_dev = drm_dev;
 
@@ -398,7 +381,6 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
 	rockchip_drm_psr_unregister(&dp->encoder);
 
 	analogix_dp_unbind(dev, master, data);
-	clk_disable_unprepare(dp->pclk);
 }
 
 static const struct component_ops rockchip_dp_component_ops = {
@@ -414,7 +396,7 @@ static int rockchip_dp_probe(struct platform_device *pdev)
 	int ret;
 
 	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	dp = devm_kzalloc(dev, sizeof(*dp), GFP_KERNEL);
@@ -422,9 +404,12 @@ static int rockchip_dp_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	dp->dev = dev;
-
 	dp->plat_data.panel = panel;
 
+	ret = rockchip_dp_of_probe(dp);
+	if (ret < 0)
+		return ret;
+
 	/*
 	 * We just use the drvdata until driver run into component
 	 * add function, and then we would set drvdata to null, so
-- 
2.11.0

Re: [PATCH v6 02/10] drm/rockchip: analogix_dp: Remove unnecessary init code

[Mark yao]

On 2017年10月19日 11:48, Jeffy Chen wrote:
> Remove unnecessary init code, since we would do it in the power_on()
> callback.
>
> Also move of parse code to probe().
>
> Fixes: 9e32e16e9e98 ("drm: rockchip: dp: add rockchip platform dp driver")
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>

Looks good for me
Acked-by: Mark Yao <mark.yao@rock-chips.com>

> ---
>
> Changes in v6: None
> Changes in v5: None
>
>   drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 27 ++++++-------------------
>   1 file changed, 6 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index 4d3f6ad0abdd..8cae5ad926cd 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -269,7 +269,7 @@ static struct drm_encoder_funcs rockchip_dp_encoder_funcs = {
>   	.destroy = rockchip_dp_drm_encoder_destroy,
>   };
>   
> -static int rockchip_dp_init(struct rockchip_dp_device *dp)
> +static int rockchip_dp_of_probe(struct rockchip_dp_device *dp)
>   {
>   	struct device *dev = dp->dev;
>   	struct device_node *np = dev->of_node;
> @@ -303,19 +303,6 @@ static int rockchip_dp_init(struct rockchip_dp_device *dp)
>   		return PTR_ERR(dp->rst);
>   	}
>   
> -	ret = clk_prepare_enable(dp->pclk);
> -	if (ret < 0) {
> -		DRM_DEV_ERROR(dp->dev, "failed to enable pclk %d\n", ret);
> -		return ret;
> -	}
> -
> -	ret = rockchip_dp_pre_init(dp);
> -	if (ret < 0) {
> -		DRM_DEV_ERROR(dp->dev, "failed to pre init %d\n", ret);
> -		clk_disable_unprepare(dp->pclk);
> -		return ret;
> -	}
> -
>   	return 0;
>   }
>   
> @@ -361,10 +348,6 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
>   	if (!dp_data)
>   		return -ENODEV;
>   
> -	ret = rockchip_dp_init(dp);
> -	if (ret < 0)
> -		return ret;
> -
>   	dp->data = dp_data;
>   	dp->drm_dev = drm_dev;
>   
> @@ -398,7 +381,6 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
>   	rockchip_drm_psr_unregister(&dp->encoder);
>   
>   	analogix_dp_unbind(dev, master, data);
> -	clk_disable_unprepare(dp->pclk);
>   }
>   
>   static const struct component_ops rockchip_dp_component_ops = {
> @@ -414,7 +396,7 @@ static int rockchip_dp_probe(struct platform_device *pdev)
>   	int ret;
>   
>   	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
> -	if (ret)
> +	if (ret < 0)
>   		return ret;
>   
>   	dp = devm_kzalloc(dev, sizeof(*dp), GFP_KERNEL);
> @@ -422,9 +404,12 @@ static int rockchip_dp_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   
>   	dp->dev = dev;
> -
>   	dp->plat_data.panel = panel;
>   
> +	ret = rockchip_dp_of_probe(dp);
> +	if (ret < 0)
> +		return ret;
> +
>   	/*
>   	 * We just use the drvdata until driver run into component
>   	 * add function, and then we would set drvdata to null, so

Re: [PATCH v6 02/10] drm/rockchip: analogix_dp: Remove unnecessary init code

[Sean Paul]

On Fri, Oct 20, 2017 at 09:48:35AM +0800, Mark yao wrote:
> On 2017年10月19日 11:48, Jeffy Chen wrote:
> > Remove unnecessary init code, since we would do it in the power_on()
> > callback.
> > 
> > Also move of parse code to probe().
> > 
> > Fixes: 9e32e16e9e98 ("drm: rockchip: dp: add rockchip platform dp driver")
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> 
> Looks good for me
> Acked-by: Mark Yao <mark.yao@rock-chips.com>

Pushed to -misc-next

Thanks,

Sean

> 
> > ---
> > 
> > Changes in v6: None
> > Changes in v5: None
> > 
> >   drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 27 ++++++-------------------
> >   1 file changed, 6 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > index 4d3f6ad0abdd..8cae5ad926cd 100644
> > --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > @@ -269,7 +269,7 @@ static struct drm_encoder_funcs rockchip_dp_encoder_funcs = {
> >   	.destroy = rockchip_dp_drm_encoder_destroy,
> >   };
> > -static int rockchip_dp_init(struct rockchip_dp_device *dp)
> > +static int rockchip_dp_of_probe(struct rockchip_dp_device *dp)
> >   {
> >   	struct device *dev = dp->dev;
> >   	struct device_node *np = dev->of_node;
> > @@ -303,19 +303,6 @@ static int rockchip_dp_init(struct rockchip_dp_device *dp)
> >   		return PTR_ERR(dp->rst);
> >   	}
> > -	ret = clk_prepare_enable(dp->pclk);
> > -	if (ret < 0) {
> > -		DRM_DEV_ERROR(dp->dev, "failed to enable pclk %d\n", ret);
> > -		return ret;
> > -	}
> > -
> > -	ret = rockchip_dp_pre_init(dp);
> > -	if (ret < 0) {
> > -		DRM_DEV_ERROR(dp->dev, "failed to pre init %d\n", ret);
> > -		clk_disable_unprepare(dp->pclk);
> > -		return ret;
> > -	}
> > -
> >   	return 0;
> >   }
> > @@ -361,10 +348,6 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
> >   	if (!dp_data)
> >   		return -ENODEV;
> > -	ret = rockchip_dp_init(dp);
> > -	if (ret < 0)
> > -		return ret;
> > -
> >   	dp->data = dp_data;
> >   	dp->drm_dev = drm_dev;
> > @@ -398,7 +381,6 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
> >   	rockchip_drm_psr_unregister(&dp->encoder);
> >   	analogix_dp_unbind(dev, master, data);
> > -	clk_disable_unprepare(dp->pclk);
> >   }
> >   static const struct component_ops rockchip_dp_component_ops = {
> > @@ -414,7 +396,7 @@ static int rockchip_dp_probe(struct platform_device *pdev)
> >   	int ret;
> >   	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
> > -	if (ret)
> > +	if (ret < 0)
> >   		return ret;
> >   	dp = devm_kzalloc(dev, sizeof(*dp), GFP_KERNEL);
> > @@ -422,9 +404,12 @@ static int rockchip_dp_probe(struct platform_device *pdev)
> >   		return -ENOMEM;
> >   	dp->dev = dev;
> > -
> >   	dp->plat_data.panel = panel;
> > +	ret = rockchip_dp_of_probe(dp);
> > +	if (ret < 0)
> > +		return ret;
> > +
> >   	/*
> >   	 * We just use the drvdata until driver run into component
> >   	 * add function, and then we would set drvdata to null, so
> 

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

[PATCH v6 03/10] drm/bridge: analogix: Do not use device's drvdata

[Jeffy Chen]

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>
Acked-by: Jingoo Han <jingoohan1@gmail.com>
Acked-by: Archit Taneja <architt@codeaurora.org>
---

Changes in v6: None
Changes in v5: 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    | 49 ++++++++++++---------
 include/drm/bridge/analogix_dp.h                   | 19 ++++----
 4 files changed, 74 insertions(+), 70 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 8cae5ad926cd..fa0365de31d2 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);
 }
 
@@ -273,7 +274,6 @@ static int rockchip_dp_of_probe(struct rockchip_dp_device *dp)
 {
 	struct device *dev = dp->dev;
 	struct device_node *np = dev->of_node;
-	int ret;
 
 	dp->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
 	if (IS_ERR(dp->grf)) {
@@ -337,13 +337,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;
@@ -370,7 +363,11 @@ 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);
+	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 rockchip_dp_unbind(struct device *dev, struct device *master,
@@ -379,8 +376,7 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
 	struct rockchip_dp_device *dp = dev_get_drvdata(dev);
 
 	rockchip_drm_psr_unregister(&dp->encoder);
-
-	analogix_dp_unbind(dev, master, data);
+	analogix_dp_unbind(dp->adp);
 }
 
 static const struct component_ops rockchip_dp_component_ops = {
@@ -410,11 +406,6 @@ static int rockchip_dp_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	/*
-	 * 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);
@@ -427,10 +418,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

Re: [PATCH v6 03/10] drm/bridge: analogix: Do not use device's drvdata

[Sean Paul]

On Thu, Oct 19, 2017 at 11:48:05AM +0800, Jeffy Chen wrote:

This patch has somehow lost it's original author. I assume this is not
intentional.

Sean

> 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>
> Acked-by: Jingoo Han <jingoohan1@gmail.com>
> Acked-by: Archit Taneja <architt@codeaurora.org>
> ---
> 
> Changes in v6: None
> Changes in v5: 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    | 49 ++++++++++++---------
>  include/drm/bridge/analogix_dp.h                   | 19 ++++----
>  4 files changed, 74 insertions(+), 70 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 8cae5ad926cd..fa0365de31d2 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);
>  }
>  
> @@ -273,7 +274,6 @@ static int rockchip_dp_of_probe(struct rockchip_dp_device *dp)
>  {
>  	struct device *dev = dp->dev;
>  	struct device_node *np = dev->of_node;
> -	int ret;
>  
>  	dp->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
>  	if (IS_ERR(dp->grf)) {
> @@ -337,13 +337,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;
> @@ -370,7 +363,11 @@ 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);
> +	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 rockchip_dp_unbind(struct device *dev, struct device *master,
> @@ -379,8 +376,7 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
>  	struct rockchip_dp_device *dp = dev_get_drvdata(dev);
>  
>  	rockchip_drm_psr_unregister(&dp->encoder);
> -
> -	analogix_dp_unbind(dev, master, data);
> +	analogix_dp_unbind(dp->adp);
>  }
>  
>  static const struct component_ops rockchip_dp_component_ops = {
> @@ -410,11 +406,6 @@ static int rockchip_dp_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> -	/*
> -	 * 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);
> @@ -427,10 +418,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: [PATCH v6 03/10] drm/bridge: analogix: Do not use device's drvdata

[jeffy]

Hi Sean,

On 10/21/2017 12:52 AM, Sean Paul wrote:
> On Thu, Oct 19, 2017 at 11:48:05AM +0800, Jeffy Chen wrote:
>
> This patch has somehow lost it's original author. I assume this is not
> intentional.

oops, sorry, guess i used some wrongly command during the rebasing... 
will fix that.

[PATCH v6 04/10] drm/bridge: analogix_dp: Fix connector and encoder cleanup

[Jeffy Chen]

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

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---

Changes in v6:
Don't change order of rockchip_drm_psr_register().

Changes in v5: None

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  2 --
 drivers/gpu/drm/exynos/exynos_dp.c                 |  7 +++++--
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 12 +++++-------
 3 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 74d274b6d31d..3f910ab36ff6 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1409,7 +1409,6 @@ analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 	ret = analogix_dp_create_bridge(drm_dev, dp);
 	if (ret) {
 		DRM_ERROR("failed to create bridge (%d)\n", ret);
-		drm_encoder_cleanup(dp->encoder);
 		goto err_disable_pm_runtime;
 	}
 
@@ -1432,7 +1431,6 @@ void analogix_dp_unbind(struct analogix_dp_device *dp)
 {
 	analogix_dp_bridge_disable(dp->bridge);
 	dp->connector.funcs->destroy(&dp->connector);
-	dp->encoder->funcs->destroy(dp->encoder);
 
 	if (dp->plat_data->panel) {
 		if (drm_panel_unprepare(dp->plat_data->panel))
diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
index f7e5b2c405ed..33319a858f3a 100644
--- a/drivers/gpu/drm/exynos/exynos_dp.c
+++ b/drivers/gpu/drm/exynos/exynos_dp.c
@@ -185,8 +185,10 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
 	dp->plat_data.encoder = encoder;
 
 	dp->adp = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
-	if (IS_ERR(dp->adp))
+	if (IS_ERR(dp->adp)) {
+		dp->encoder.funcs->destroy(&dp->encoder);
 		return PTR_ERR(dp->adp);
+	}
 
 	return 0;
 }
@@ -196,7 +198,8 @@ static void exynos_dp_unbind(struct device *dev, struct device *master,
 {
 	struct exynos_dp_device *dp = dev_get_drvdata(dev);
 
-	return analogix_dp_unbind(dp->adp);
+	analogix_dp_unbind(dp->adp);
+	dp->encoder.funcs->destroy(&dp->encoder);
 }
 
 static const struct component_ops exynos_dp_ops = {
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index fa0365de31d2..117585df73e1 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -261,13 +261,8 @@ static struct drm_encoder_helper_funcs rockchip_dp_encoder_helper_funcs = {
 	.atomic_check = rockchip_dp_drm_encoder_atomic_check,
 };
 
-static void rockchip_dp_drm_encoder_destroy(struct drm_encoder *encoder)
-{
-	drm_encoder_cleanup(encoder);
-}
-
 static struct drm_encoder_funcs rockchip_dp_encoder_funcs = {
-	.destroy = rockchip_dp_drm_encoder_destroy,
+	.destroy = drm_encoder_cleanup,
 };
 
 static int rockchip_dp_of_probe(struct rockchip_dp_device *dp)
@@ -364,8 +359,10 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 	rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);
 
 	dp->adp = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
-	if (IS_ERR(dp->adp))
+	if (IS_ERR(dp->adp)) {
+		dp->encoder.funcs->destroy(&dp->encoder);
 		return PTR_ERR(dp->adp);
+	}
 
 	return 0;
 }
@@ -377,6 +374,7 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
 
 	rockchip_drm_psr_unregister(&dp->encoder);
 	analogix_dp_unbind(dp->adp);
+	dp->encoder.funcs->destroy(&dp->encoder);
 }
 
 static const struct component_ops rockchip_dp_component_ops = {
-- 
2.11.0

[PATCH v6 05/10] drm/rockchip: analogix_dp: Add a sanity check for rockchip_drm_psr_register()

[Jeffy Chen]

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

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

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

Changes in v6: None
Changes in v5: None

 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 117585df73e1..bd3567ad8b53 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -356,15 +356,22 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 	dp->psr_state = ~EDP_VSC_PSR_STATE_ACTIVE;
 	INIT_WORK(&dp->psr_work, analogix_dp_psr_work);
 
-	rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);
+	ret = rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);
+	if (ret < 0)
+		goto err_cleanup_encoder;
 
 	dp->adp = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
 	if (IS_ERR(dp->adp)) {
-		dp->encoder.funcs->destroy(&dp->encoder);
-		return PTR_ERR(dp->adp);
+		ret = PTR_ERR(dp->adp);
+		goto err_unreg_psr;
 	}
 
 	return 0;
+err_unreg_psr:
+	rockchip_drm_psr_unregister(&dp->encoder);
+err_cleanup_encoder:
+	dp->encoder.funcs->destroy(&dp->encoder);
+	return ret;
 }
 
 static void rockchip_dp_unbind(struct device *dev, struct device *master,
@@ -372,8 +379,8 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
 {
 	struct rockchip_dp_device *dp = dev_get_drvdata(dev);
 
-	rockchip_drm_psr_unregister(&dp->encoder);
 	analogix_dp_unbind(dp->adp);
+	rockchip_drm_psr_unregister(&dp->encoder);
 	dp->encoder.funcs->destroy(&dp->encoder);
 }
 
-- 
2.11.0

Re: [PATCH v6 05/10] drm/rockchip: analogix_dp: Add a sanity check for rockchip_drm_psr_register()

[Mark yao]

On 2017年10月19日 11:48, Jeffy Chen wrote:
> The rockchip_drm_psr_register() can fail, so add a sanity check for that.
>
> Also reorder the calls in unbind() to match bind().
>
> Signed-off-by: Jeffy Chen<jeffy.chen@rock-chips.com>

Looks good for me
Acked-by: Mark Yao <mark.yao@rock-chips.com>

Mark

[PATCH v6 06/10] 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 v6: None
Changes in v5:
Call the destroy hook in the error handling path like in unbind().

 drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
index b15755b6129c..e72d4e2b61aa 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:
-	drm_encoder_cleanup(&dsi->encoder);
-	drm_connector_cleanup(&dsi->connector);
-err_pllref:
+err_disable_pm_runtime:
+	pm_runtime_disable(dev);
+	dsi->connector.funcs->destroy(&dsi->connector);
+	dsi->encoder.funcs->destroy(&dsi->encoder);
+err_disable_pllref:
 	clk_disable_unprepare(dsi->pllref_clk);
 	return ret;
 }
@@ -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

Re: [PATCH v6 06/10] drm/rockchip: dw-mipi-dsi: Fix error handling path

[Mark yao]

On 2017年10月19日 11:48, 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>

Looks good for me
Acked-by: Mark Yao <mark.yao@rock-chips.com>

Mark

[PATCH v6 07/10] 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 v6: None
Changes in v5:
Call the destroy hook in the error handling path like in unbind().
Update cleanup order in unbind().

 drivers/gpu/drm/rockchip/inno_hdmi.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index ee584d87111f..9a96ff6b022b 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:
+	hdmi->connector.funcs->destroy(&hdmi->connector);
+	hdmi->encoder.funcs->destroy(&hdmi->encoder);
+err_put_adapter:
+	i2c_put_adapter(hdmi->ddc);
+err_disable_clk:
+	clk_disable_unprepare(hdmi->pclk);
 	return ret;
 }
 
@@ -896,8 +908,8 @@ static void inno_hdmi_unbind(struct device *dev, struct device *master,
 	hdmi->connector.funcs->destroy(&hdmi->connector);
 	hdmi->encoder.funcs->destroy(&hdmi->encoder);
 
-	clk_disable_unprepare(hdmi->pclk);
 	i2c_put_adapter(hdmi->ddc);
+	clk_disable_unprepare(hdmi->pclk);
 }
 
 static const struct component_ops inno_hdmi_ops = {
-- 
2.11.0

Re: [PATCH v6 07/10] drm/rockchip: inno_hdmi: Fix error handling path

[Mark yao]

On 2017年10月19日 11:48, 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>

Looks good for me
Acked-by: Mark Yao <mark.yao@rock-chips.com>

Mark

[PATCH v6 08/10] drm/bridge/synopsys: dw-hdmi: Add missing bridge detach

[Jeffy Chen]

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

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

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

Changes in v6: None
Changes in v5: None

 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index bf14214fa464..ff1b3d2b5d06 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -1966,6 +1966,13 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge)
 	return 0;
 }
 
+static void dw_hdmi_bridge_detach(struct drm_bridge *bridge)
+{
+	struct dw_hdmi *hdmi = bridge->driver_private;
+
+	drm_connector_cleanup(&hdmi->connector);
+}
+
 static enum drm_mode_status
 dw_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
 			  const struct drm_display_mode *mode)
@@ -2022,6 +2029,7 @@ static void dw_hdmi_bridge_enable(struct drm_bridge *bridge)
 
 static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = {
 	.attach = dw_hdmi_bridge_attach,
+	.detach = dw_hdmi_bridge_detach,
 	.enable = dw_hdmi_bridge_enable,
 	.disable = dw_hdmi_bridge_disable,
 	.mode_set = dw_hdmi_bridge_mode_set,
@@ -2591,7 +2599,7 @@ int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
 
 	ret = drm_bridge_attach(encoder, &hdmi->bridge, NULL);
 	if (ret) {
-		dw_hdmi_remove(pdev);
+		__dw_hdmi_remove(hdmi);
 		DRM_ERROR("Failed to initialize bridge with drm\n");
 		return ret;
 	}
-- 
2.11.0

[PATCH v6 09/10] drm/bridge/synopsys: dw-hdmi: Do not use device's drvdata

[Jeffy Chen]

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

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
---

Changes in v6: None
Changes in v5: None

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

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index ff1b3d2b5d06..6fbfafc5832b 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2072,7 +2072,7 @@ static irqreturn_t dw_hdmi_hardirq(int irq, void *dev_id)
 	return ret;
 }
 
-void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense)
+void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense)
 {
 	mutex_lock(&hdmi->mutex);
 
@@ -2098,13 +2098,6 @@ void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense)
 	}
 	mutex_unlock(&hdmi->mutex);
 }
-
-void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense)
-{
-	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
-
-	__dw_hdmi_setup_rx_sense(hdmi, hpd, rx_sense);
-}
 EXPORT_SYMBOL_GPL(dw_hdmi_setup_rx_sense);
 
 static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
@@ -2140,9 +2133,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
 	 */
 	if (intr_stat &
 	    (HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_IH_PHY_STAT0_HPD)) {
-		__dw_hdmi_setup_rx_sense(hdmi,
-					 phy_stat & HDMI_PHY_HPD,
-					 phy_stat & HDMI_PHY_RX_SENSE);
+		dw_hdmi_setup_rx_sense(hdmi, phy_stat & HDMI_PHY_HPD,
+				       phy_stat & HDMI_PHY_RX_SENSE);
 
 		if ((phy_stat & (HDMI_PHY_RX_SENSE | HDMI_PHY_HPD)) == 0)
 			cec_notifier_set_phys_addr(hdmi->cec_notifier,
@@ -2512,8 +2504,6 @@ __dw_hdmi_probe(struct platform_device *pdev,
 	if (hdmi->i2c)
 		dw_hdmi_i2c_init(hdmi);
 
-	platform_set_drvdata(pdev, hdmi);
-
 	return hdmi;
 
 err_iahb:
@@ -2559,25 +2549,23 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi)
 /* -----------------------------------------------------------------------------
  * Probe/remove API, used from platforms based on the DRM bridge API.
  */
-int dw_hdmi_probe(struct platform_device *pdev,
-		  const struct dw_hdmi_plat_data *plat_data)
+struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
+			      const struct dw_hdmi_plat_data *plat_data)
 {
 	struct dw_hdmi *hdmi;
 
 	hdmi = __dw_hdmi_probe(pdev, plat_data);
 	if (IS_ERR(hdmi))
-		return PTR_ERR(hdmi);
+		return hdmi;
 
 	drm_bridge_add(&hdmi->bridge);
 
-	return 0;
+	return hdmi;
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_probe);
 
-void dw_hdmi_remove(struct platform_device *pdev)
+void dw_hdmi_remove(struct dw_hdmi *hdmi)
 {
-	struct dw_hdmi *hdmi = platform_get_drvdata(pdev);
-
 	drm_bridge_remove(&hdmi->bridge);
 
 	__dw_hdmi_remove(hdmi);
@@ -2587,31 +2575,30 @@ EXPORT_SYMBOL_GPL(dw_hdmi_remove);
 /* -----------------------------------------------------------------------------
  * Bind/unbind API, used from platforms based on the component framework.
  */
-int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
-		 const struct dw_hdmi_plat_data *plat_data)
+struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev,
+			    struct drm_encoder *encoder,
+			    const struct dw_hdmi_plat_data *plat_data)
 {
 	struct dw_hdmi *hdmi;
 	int ret;
 
 	hdmi = __dw_hdmi_probe(pdev, plat_data);
 	if (IS_ERR(hdmi))
-		return PTR_ERR(hdmi);
+		return hdmi;
 
 	ret = drm_bridge_attach(encoder, &hdmi->bridge, NULL);
 	if (ret) {
 		__dw_hdmi_remove(hdmi);
 		DRM_ERROR("Failed to initialize bridge with drm\n");
-		return ret;
+		return ERR_PTR(ret);
 	}
 
-	return 0;
+	return hdmi;
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_bind);
 
-void dw_hdmi_unbind(struct device *dev)
+void dw_hdmi_unbind(struct dw_hdmi *hdmi)
 {
-	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
-
 	__dw_hdmi_remove(hdmi);
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index b62763aa8706..b01d03e02ce0 100644
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -26,6 +26,8 @@ struct imx_hdmi {
 	struct device *dev;
 	struct drm_encoder encoder;
 	struct regmap *regmap;
+
+	struct dw_hdmi *hdmi;
 };
 
 static inline struct imx_hdmi *enc_to_imx_hdmi(struct drm_encoder *e)
@@ -239,22 +241,24 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 	drm_encoder_init(drm, encoder, &dw_hdmi_imx_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS, NULL);
 
-	ret = dw_hdmi_bind(pdev, encoder, plat_data);
+	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
+	if (IS_ERR(hdmi->hdmi)) {
+		encoder->funcs->destroy(encoder);
+		return PTR_ERR(hdmi->hdmi);
+	}
 
-	/*
-	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
-	 * which would have called the encoder cleanup.  Do it manually.
-	 */
-	if (ret)
-		drm_encoder_cleanup(encoder);
+	dev_set_drvdata(dev, hdmi);
 
-	return ret;
+	return 0;
 }
 
 static void dw_hdmi_imx_unbind(struct device *dev, struct device *master,
 			       void *data)
 {
-	return dw_hdmi_unbind(dev);
+	struct imx_hdmi *hdmi = dev_get_drvdata(dev);
+
+	dw_hdmi_unbind(hdmi->hdmi);
+	hdmi->encoder.funcs->destroy(&hdmi->encoder);
 }
 
 static const struct component_ops dw_hdmi_imx_ops = {
diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index cef414466f9f..fc29f69f7108 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -138,6 +138,8 @@ struct meson_dw_hdmi {
 	struct clk *hdmi_pclk;
 	struct clk *venci_clk;
 	u32 irq_stat;
+
+	struct dw_hdmi *hdmi;
 };
 #define encoder_to_meson_dw_hdmi(x) \
 	container_of(x, struct meson_dw_hdmi, encoder)
@@ -526,7 +528,7 @@ static irqreturn_t dw_hdmi_top_thread_irq(int irq, void *dev_id)
 		if (stat & HDMITX_TOP_INTR_HPD_RISE)
 			hpd_connected = true;
 
-		dw_hdmi_setup_rx_sense(dw_hdmi->dev, hpd_connected,
+		dw_hdmi_setup_rx_sense(dw_hdmi->hdmi, hpd_connected,
 				       hpd_connected);
 
 		drm_helper_hpd_irq_event(dw_hdmi->encoder.dev);
@@ -865,9 +867,14 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
 	dw_plat_data->input_bus_format = MEDIA_BUS_FMT_YUV8_1X24;
 	dw_plat_data->input_bus_encoding = V4L2_YCBCR_ENC_709;
 
-	ret = dw_hdmi_bind(pdev, encoder, &meson_dw_hdmi->dw_plat_data);
-	if (ret)
-		return ret;
+	meson_dw_hdmi->hdmi = dw_hdmi_bind(pdev, encoder,
+					   &meson_dw_hdmi->dw_plat_data);
+	if (IS_ERR(meson_dw_hdmi->hdmi)) {
+		encoder->funcs->destroy(encoder);
+		return PTR_ERR(meson_dw_hdmi->hdmi);
+	}
+
+	dev_set_drvdata(dev, meson_dw_hdmi);
 
 	DRM_DEBUG_DRIVER("HDMI controller initialized\n");
 
@@ -877,7 +884,10 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
 static void meson_dw_hdmi_unbind(struct device *dev, struct device *master,
 				   void *data)
 {
-	dw_hdmi_unbind(dev);
+	struct meson_dw_hdmi *meson_dw_hdmi = dev_get_drvdata(dev);
+
+	dw_hdmi_unbind(meson_dw_hdmi->hdmi);
+	meson_dw_hdmi->encoder.funcs->destroy(&meson_dw_hdmi->encoder);
 }
 
 static const struct component_ops meson_dw_hdmi_ops = {
diff --git a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
index dc85b53d58ef..76210ae25094 100644
--- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
+++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
@@ -68,12 +68,22 @@ static const struct dw_hdmi_plat_data rcar_dw_hdmi_plat_data = {
 
 static int rcar_dw_hdmi_probe(struct platform_device *pdev)
 {
-	return dw_hdmi_probe(pdev, &rcar_dw_hdmi_plat_data);
+	struct dw_hdmi *hdmi;
+
+	hdmi = dw_hdmi_probe(pdev, &rcar_dw_hdmi_plat_data);
+	if (IS_ERR(hdmi))
+		return PTR_ERR(hdmi);
+
+	platform_set_drvdata(pdev, hdmi);
+
+	return 0;
 }
 
 static int rcar_dw_hdmi_remove(struct platform_device *pdev)
 {
-	dw_hdmi_remove(pdev);
+	struct dw_hdmi *hdmi = platform_get_drvdata(pdev);
+
+	dw_hdmi_remove(hdmi);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 1eb02a82fd91..791ab938f998 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -48,6 +48,8 @@ struct rockchip_hdmi {
 	const struct rockchip_hdmi_chip_data *chip_data;
 	struct clk *vpll_clk;
 	struct clk *grf_clk;
+
+	struct dw_hdmi *hdmi;
 };
 
 #define to_rockchip_hdmi(x)	container_of(x, struct rockchip_hdmi, x)
@@ -164,7 +166,6 @@ static const struct dw_hdmi_phy_config rockchip_phy_config[] = {
 static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 {
 	struct device_node *np = hdmi->dev->of_node;
-	int ret;
 
 	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
 	if (IS_ERR(hdmi->regmap)) {
@@ -377,22 +378,24 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	drm_encoder_init(drm, encoder, &dw_hdmi_rockchip_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS, NULL);
 
-	ret = dw_hdmi_bind(pdev, encoder, plat_data);
+	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
+	if (IS_ERR(hdmi->hdmi)) {
+		encoder->funcs->destroy(encoder);
+		return PTR_ERR(hdmi->hdmi);
+	}
 
-	/*
-	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
-	 * which would have called the encoder cleanup.  Do it manually.
-	 */
-	if (ret)
-		drm_encoder_cleanup(encoder);
+	dev_set_drvdata(dev, hdmi);
 
-	return ret;
+	return 0;
 }
 
 static void dw_hdmi_rockchip_unbind(struct device *dev, struct device *master,
 				    void *data)
 {
-	return dw_hdmi_unbind(dev);
+	struct rockchip_hdmi *hdmi = dev_get_drvdata(dev);
+
+	dw_hdmi_unbind(hdmi->hdmi);
+	hdmi->encoder.funcs->destroy(&hdmi->encoder);
 }
 
 static const struct component_ops dw_hdmi_rockchip_ops = {
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 182f83283e24..29b8900caaf7 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -143,14 +143,15 @@ struct dw_hdmi_plat_data {
 			     unsigned long mpixelclock);
 };
 
-int dw_hdmi_probe(struct platform_device *pdev,
-		  const struct dw_hdmi_plat_data *plat_data);
-void dw_hdmi_remove(struct platform_device *pdev);
-void dw_hdmi_unbind(struct device *dev);
-int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
-		 const struct dw_hdmi_plat_data *plat_data);
-
-void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense);
+struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
+			      const struct dw_hdmi_plat_data *plat_data);
+void dw_hdmi_remove(struct dw_hdmi *hdmi);
+void dw_hdmi_unbind(struct dw_hdmi *hdmi);
+struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev,
+			     struct drm_encoder *encoder,
+			     const struct dw_hdmi_plat_data *plat_data);
+
+void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense);
 
 void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
 void dw_hdmi_audio_enable(struct dw_hdmi *hdmi);
-- 
2.11.0

[PATCH v6 10/10] drm/rockchip: dw_hdmi: Fix error handling path

[Jeffy Chen]

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

Also inline clk_prepare_enable() with bind().

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

Changes in v6: None
Changes in v5:
Add disable to unbind(), and inline clk_prepare_enable() with bind().

 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 791ab938f998..e936dfe6c03d 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -193,13 +193,6 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 		return PTR_ERR(hdmi->grf_clk);
 	}
 
-	ret = clk_prepare_enable(hdmi->vpll_clk);
-	if (ret) {
-		DRM_DEV_ERROR(hdmi->dev,
-			      "Failed to enable HDMI vpll: %d\n", ret);
-		return ret;
-	}
-
 	return 0;
 }
 
@@ -374,6 +367,13 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 		return ret;
 	}
 
+	ret = clk_prepare_enable(hdmi->vpll_clk);
+	if (ret) {
+		DRM_DEV_ERROR(hdmi->dev,
+			      "Failed to enable HDMI vpll: %d\n", ret);
+		return ret;
+	}
+
 	drm_encoder_helper_add(encoder, &dw_hdmi_rockchip_encoder_helper_funcs);
 	drm_encoder_init(drm, encoder, &dw_hdmi_rockchip_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS, NULL);
@@ -381,6 +381,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
 	if (IS_ERR(hdmi->hdmi)) {
 		encoder->funcs->destroy(encoder);
+		clk_disable_unprepare(hdmi->vpll_clk);
 		return PTR_ERR(hdmi->hdmi);
 	}
 
@@ -396,6 +397,7 @@ static void dw_hdmi_rockchip_unbind(struct device *dev, struct device *master,
 
 	dw_hdmi_unbind(hdmi->hdmi);
 	hdmi->encoder.funcs->destroy(&hdmi->encoder);
+	clk_disable_unprepare(hdmi->vpll_clk);
 }
 
 static const struct component_ops dw_hdmi_rockchip_ops = {
-- 
2.11.0

Re: [PATCH v6 10/10] drm/rockchip: dw_hdmi: Fix error handling path

[Mark yao]

On 2017年10月19日 11:48, Jeffy Chen wrote:
> Add missing clk_disable_unprepare() in bind()'s error handling path and
> unbind().
>
> Also inline clk_prepare_enable() with bind().
>
> Fixes: 12b9f204e804 ("drm: bridge/dw_hdmi: add rockchip rk3288 support")
> Signed-off-by: Jeffy Chen<jeffy.chen@rock-chips.com>

Looks good for me
Acked-by: Mark Yao <mark.yao@rock-chips.com>

Mark

Re: [PATCH v6 00/10] rockchip: kevin: Enable edp display

[Sean Paul]

On Thu, Oct 19, 2017 at 11:48:02AM +0800, Jeffy Chen wrote:
> 
> Make edp display works on chromebook kevin(at least for boot animation).
> 
> Also solve some issues i meet during the bringup.
> 
> Changes in v6:
> Don't change order of rockchip_drm_psr_register().
> 
> Changes in v5:
> Call the destroy hook in the error handling path like in unbind().
> Call the destroy hook in the error handling path like in unbind().
> Update cleanup order in unbind().
> Add disable to unbind(), and inline clk_prepare_enable() with bind().
> 
> Jeffy Chen (10):
>   arm64: dts: rockchip: Enable edp disaplay on kevin
>   drm/rockchip: analogix_dp: Remove unnecessary init code
>   drm/bridge: analogix: Do not use device's drvdata
>   drm/bridge: analogix_dp: Fix connector and encoder cleanup
>   drm/rockchip: analogix_dp: Add a sanity check for
>     rockchip_drm_psr_register()
>   drm/rockchip: dw-mipi-dsi: Fix error handling path
>   drm/rockchip: inno_hdmi: Fix error handling path
>   drm/bridge/synopsys: dw-hdmi: Add missing bridge detach
>   drm/bridge/synopsys: dw-hdmi: Do not use device's drvdata
>   drm/rockchip: dw_hdmi: Fix error handling path

Hi Jeffy,
I've pushed 2/10. Once you fix 3/10, I can push 3-7. I'd like someone familiar
with synopsys to review 8-9 before pushing those. Since 10 depends on 9, it'll
be blocked on synopsys review.

Sean

> 
>  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 | 52 +++++-------
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          | 53 ++++++------
>  drivers/gpu/drm/exynos/exynos_dp.c                 | 29 ++++---
>  drivers/gpu/drm/imx/dw_hdmi-imx.c                  | 22 +++--
>  drivers/gpu/drm/meson/meson_dw_hdmi.c              | 20 +++--
>  drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c             | 14 +++-
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 95 +++++++++++-----------
>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c             | 21 +++--
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        | 39 +++++----
>  drivers/gpu/drm/rockchip/inno_hdmi.c               | 22 +++--
>  include/drm/bridge/analogix_dp.h                   | 19 +++--
>  include/drm/bridge/dw_hdmi.h                       | 17 ++--
>  14 files changed, 265 insertions(+), 183 deletions(-)
> 
> -- 
> 2.11.0
> 
> 

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

Re: [PATCH v6 00/10] rockchip: kevin: Enable edp display

[Heiko Stuebner]

Hi Jeffy, Sean,

Am Donnerstag, 19. Oktober 2017, 11:48:02 CET schrieb 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 v6:
> Don't change order of rockchip_drm_psr_register().
> 
> Changes in v5:
> Call the destroy hook in the error handling path like in unbind().
> Call the destroy hook in the error handling path like in unbind().
> Update cleanup order in unbind().
> Add disable to unbind(), and inline clk_prepare_enable() with bind().
> 
> Jeffy Chen (10):
>   arm64: dts: rockchip: Enable edp disaplay on kevin
>   drm/rockchip: analogix_dp: Remove unnecessary init code
>   drm/bridge: analogix: Do not use device's drvdata
>   drm/bridge: analogix_dp: Fix connector and encoder cleanup
>   drm/rockchip: analogix_dp: Add a sanity check for
>     rockchip_drm_psr_register()
>   drm/rockchip: dw-mipi-dsi: Fix error handling path
>   drm/rockchip: inno_hdmi: Fix error handling path
>   drm/bridge/synopsys: dw-hdmi: Add missing bridge detach
>   drm/bridge/synopsys: dw-hdmi: Do not use device's drvdata
>   drm/rockchip: dw_hdmi: Fix error handling path

As I was just looking at the edp dts change in patch1 again, does this
series also contain a fix for the issue below [0] ?

I'm still seeing this on 4.14-rc6 with the most recent drm tree merged in.


Heiko

[0]

[   27.960120] BUG: scheduling while atomic: kworker/1:1/68/0x00000002
[   27.974429] Modules linked in: rockchipdrm dw_hdmi analogix_dp drm_kms_helper panel_simple crc32_ce drm crct10dif_ce rockchip_saradc pwm_bl pwm_cros_ec rockchip_thermal ip_tables x_tabl
es ipv6 smsc95xx smsc75xx ax88179_178a asix usbnet phy_rockchip_pcie pcie_rockchip
[   28.008769] CPU: 1 PID: 68 Comm: kworker/1:1 Tainted: G        W       4.14.0-rc7-03201-g12490811b353 #559
[   28.008774] Hardware name: Google Kevin (DT)
[   28.008825] Workqueue: events analogix_dp_psr_work [rockchipdrm]
[   28.008828] Call trace:
[   28.008838] [<ffff000008088d60>] dump_backtrace+0x0/0x378
[   28.008842] [<ffff0000080890ec>] show_stack+0x14/0x20
[   28.008847] [<ffff0000089b04d8>] dump_stack+0x9c/0xbc
[   28.008852] [<ffff0000080ec4b4>] __schedule_bug+0x4c/0x70
[   28.008856] [<ffff0000089c4828>] __schedule+0x558/0x5e8
[   28.008859] [<ffff0000089c48f0>] schedule+0x38/0xa0
[   28.008864] [<ffff0000089c8194>] schedule_hrtimeout_range_clock+0x84/0xe8
[   28.008867] [<ffff0000089c8208>] schedule_hrtimeout_range+0x10/0x18
[   28.008870] [<ffff0000089c7c74>] usleep_range+0x64/0x78
[   28.008882] [<ffff000000ef56e4>] analogix_dp_transfer+0x16c/0xa88 [analogix_dp]
[   28.008891] [<ffff000000ef2190>] analogix_dpaux_transfer+0x10/0x18 [analogix_dp]
[   28.008950] [<ffff000000ddbb1c>] drm_dp_dpcd_access+0x4c/0xf8 [drm_kms_helper]
[   28.008994] [<ffff000000ddbbe4>] drm_dp_dpcd_write+0x1c/0x28 [drm_kms_helper]
[   28.009002] [<ffff000000ef2130>] analogix_dp_disable_psr+0x60/0xb0 [analogix_dp]
[   28.009036] [<ffff000000f8398c>] analogix_dp_psr_work+0x4c/0xc0 [rockchipdrm]
[   28.009040] [<ffff0000080dff44>] process_one_work+0x1d4/0x348
[   28.009043] [<ffff0000080e0100>] worker_thread+0x48/0x470
[   28.009048] [<ffff0000080e63d4>] kthread+0x12c/0x130
[   28.009052] [<ffff000008084b88>] ret_from_fork+0x10/0x18

Re: [PATCH v6 00/10] rockchip: kevin: Enable edp display

[JeffyChen]

Hi Heiko,

On 10/31/2017 07:01 AM, Heiko Stuebner wrote:
> As I was just looking at the edp dts change in patch1 again, does this
> series also contain a fix for the issue below [0] ?
>
> I'm still seeing this on 4.14-rc6 with the most recent drm tree merged in.
>
i saw that too, it should due to our psr code...i think Zain has solved 
these in chromeos kernel, i will ask Zain if he have time to upstream 
them, or maybe i'll try to upstream them.
>
> Heiko
>
> [0]
>
> [   27.960120] BUG: scheduling while atomic: kworker/1:1/68/0x00000002
> [   27.974429] Modules linked in: rockchipdrm dw_hdmi analogix_dp drm_kms_helper panel_simple crc32_ce drm crct10dif_ce rockchip_saradc pwm_bl pwm_cros_ec rockchip_thermal ip_tables x_tabl
> es ipv6 smsc95xx smsc75xx ax88179_178a asix usbnet phy_rockchip_pcie pcie_rockchip
> [   28.008769] CPU: 1 PID: 68 Comm: kworker/1:1 Tainted: G        W       4.14.0-rc7-03201-g12490811b353 #559
> [   28.008774] Hardware name: Google Kevin (DT)
> [   28.008825] Workqueue: events analogix_dp_psr_work [rockchipdrm]
> [   28.008828] Call trace:
> [   28.008838] [<ffff000008088d60>] dump_backtrace+0x0/0x378
> [   28.008842] [<ffff0000080890ec>] show_stack+0x14/0x20
> [   28.008847] [<ffff0000089b04d8>] dump_stack+0x9c/0xbc
> [   28.008852] [<ffff0000080ec4b4>] __schedule_bug+0x4c/0x70
> [   28.008856] [<ffff0000089c4828>] __schedule+0x558/0x5e8
> [   28.008859] [<ffff0000089c48f0>] schedule+0x38/0xa0
> [   28.008864] [<ffff0000089c8194>] schedule_hrtimeout_range_clock+0x84/0xe8
> [   28.008867] [<ffff0000089c8208>] schedule_hrtimeout_range+0x10/0x18
> [   28.008870] [<ffff0000089c7c74>] usleep_range+0x64/0x78
> [   28.008882] [<ffff000000ef56e4>] analogix_dp_transfer+0x16c/0xa88 [analogix_dp]
> [   28.008891] [<ffff000000ef2190>] analogix_dpaux_transfer+0x10/0x18 [analogix_dp]
> [   28.008950] [<ffff000000ddbb1c>] drm_dp_dpcd_access+0x4c/0xf8 [drm_kms_helper]
> [   28.008994] [<ffff000000ddbbe4>] drm_dp_dpcd_write+0x1c/0x28 [drm_kms_helper]
> [   28.009002] [<ffff000000ef2130>] analogix_dp_disable_psr+0x60/0xb0 [analogix_dp]
> [   28.009036] [<ffff000000f8398c>] analogix_dp_psr_work+0x4c/0xc0 [rockchipdrm]
> [   28.009040] [<ffff0000080dff44>] process_one_work+0x1d4/0x348
> [   28.009043] [<ffff0000080e0100>] worker_thread+0x48/0x470
> [   28.009048] [<ffff0000080e63d4>] kthread+0x12c/0x130
> [   28.009052] [<ffff000008084b88>] ret_from_fork+0x10/0x18
>
>
>
>

Re: [PATCH v6 00/10] rockchip: kevin: Enable edp display

[Sean Paul]

On Tue, Oct 31, 2017 at 12:37:43PM +0800, JeffyChen wrote:
> Hi Heiko,
> 
> On 10/31/2017 07:01 AM, Heiko Stuebner wrote:
> > As I was just looking at the edp dts change in patch1 again, does this
> > series also contain a fix for the issue below [0] ?
> > 
> > I'm still seeing this on 4.14-rc6 with the most recent drm tree merged in.
> > 
> i saw that too, it should due to our psr code...i think Zain has solved
> these in chromeos kernel, i will ask Zain if he have time to upstream them,
> or maybe i'll try to upstream them.

You need the patchset where I've refactored the psr locking/workers. I have a
version of it based on Heiko's tree at
https://cgit.freedesktop.org/~seanpaul/dogwood/log/?h=rk3399-display

With this kernel, the backlight comes on, but I don't have anything on the
display (which is why I didn't post it). I'll try putting this set on top and
see what happens.

Sean

> > 
> > Heiko
> > 
> > [0]
> > 
> > [   27.960120] BUG: scheduling while atomic: kworker/1:1/68/0x00000002
> > [   27.974429] Modules linked in: rockchipdrm dw_hdmi analogix_dp drm_kms_helper panel_simple crc32_ce drm crct10dif_ce rockchip_saradc pwm_bl pwm_cros_ec rockchip_thermal ip_tables x_tabl
> > es ipv6 smsc95xx smsc75xx ax88179_178a asix usbnet phy_rockchip_pcie pcie_rockchip
> > [   28.008769] CPU: 1 PID: 68 Comm: kworker/1:1 Tainted: G        W       4.14.0-rc7-03201-g12490811b353 #559
> > [   28.008774] Hardware name: Google Kevin (DT)
> > [   28.008825] Workqueue: events analogix_dp_psr_work [rockchipdrm]
> > [   28.008828] Call trace:
> > [   28.008838] [<ffff000008088d60>] dump_backtrace+0x0/0x378
> > [   28.008842] [<ffff0000080890ec>] show_stack+0x14/0x20
> > [   28.008847] [<ffff0000089b04d8>] dump_stack+0x9c/0xbc
> > [   28.008852] [<ffff0000080ec4b4>] __schedule_bug+0x4c/0x70
> > [   28.008856] [<ffff0000089c4828>] __schedule+0x558/0x5e8
> > [   28.008859] [<ffff0000089c48f0>] schedule+0x38/0xa0
> > [   28.008864] [<ffff0000089c8194>] schedule_hrtimeout_range_clock+0x84/0xe8
> > [   28.008867] [<ffff0000089c8208>] schedule_hrtimeout_range+0x10/0x18
> > [   28.008870] [<ffff0000089c7c74>] usleep_range+0x64/0x78
> > [   28.008882] [<ffff000000ef56e4>] analogix_dp_transfer+0x16c/0xa88 [analogix_dp]
> > [   28.008891] [<ffff000000ef2190>] analogix_dpaux_transfer+0x10/0x18 [analogix_dp]
> > [   28.008950] [<ffff000000ddbb1c>] drm_dp_dpcd_access+0x4c/0xf8 [drm_kms_helper]
> > [   28.008994] [<ffff000000ddbbe4>] drm_dp_dpcd_write+0x1c/0x28 [drm_kms_helper]
> > [   28.009002] [<ffff000000ef2130>] analogix_dp_disable_psr+0x60/0xb0 [analogix_dp]
> > [   28.009036] [<ffff000000f8398c>] analogix_dp_psr_work+0x4c/0xc0 [rockchipdrm]
> > [   28.009040] [<ffff0000080dff44>] process_one_work+0x1d4/0x348
> > [   28.009043] [<ffff0000080e0100>] worker_thread+0x48/0x470
> > [   28.009048] [<ffff0000080e63d4>] kthread+0x12c/0x130
> > [   28.009052] [<ffff000008084b88>] ret_from_fork+0x10/0x18
> > 
> > 
> > 
> > 
> 
> 

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

Re: [PATCH v6 00/10] rockchip: kevin: Enable edp display

[Enric Balletbo Serra]

Dear all,

2017-11-01 20:33 GMT+01:00 Sean Paul <seanpaul@chromium.org>:
> On Tue, Oct 31, 2017 at 12:37:43PM +0800, JeffyChen wrote:
>> Hi Heiko,
>>
>> On 10/31/2017 07:01 AM, Heiko Stuebner wrote:
>> > As I was just looking at the edp dts change in patch1 again, does this
>> > series also contain a fix for the issue below [0] ?
>> >
>> > I'm still seeing this on 4.14-rc6 with the most recent drm tree merged in.
>> >
>> i saw that too, it should due to our psr code...i think Zain has solved
>> these in chromeos kernel, i will ask Zain if he have time to upstream them,
>> or maybe i'll try to upstream them.
>
> You need the patchset where I've refactored the psr locking/workers. I have a
> version of it based on Heiko's tree at
> https://cgit.freedesktop.org/~seanpaul/dogwood/log/?h=rk3399-display
>
> With this kernel, the backlight comes on, but I don't have anything on the
> display (which is why I didn't post it). I'll try putting this set on top and
> see what happens.
>

There is a patch in the ML sent by Emil [1], similar to the Sean
patch, that solves the issue.

And I can confirm that the Jeffy's patches + Emil patch makes the
display work on kevin or current rc8

[1] https://patchwork.kernel.org/patch/9985237/

Enric

> Sean
>
>> >
>> > Heiko
>> >
>> > [0]
>> >
>> > [   27.960120] BUG: scheduling while atomic: kworker/1:1/68/0x00000002
>> > [   27.974429] Modules linked in: rockchipdrm dw_hdmi analogix_dp drm_kms_helper panel_simple crc32_ce drm crct10dif_ce rockchip_saradc pwm_bl pwm_cros_ec rockchip_thermal ip_tables x_tabl
>> > es ipv6 smsc95xx smsc75xx ax88179_178a asix usbnet phy_rockchip_pcie pcie_rockchip
>> > [   28.008769] CPU: 1 PID: 68 Comm: kworker/1:1 Tainted: G        W       4.14.0-rc7-03201-g12490811b353 #559
>> > [   28.008774] Hardware name: Google Kevin (DT)
>> > [   28.008825] Workqueue: events analogix_dp_psr_work [rockchipdrm]
>> > [   28.008828] Call trace:
>> > [   28.008838] [<ffff000008088d60>] dump_backtrace+0x0/0x378
>> > [   28.008842] [<ffff0000080890ec>] show_stack+0x14/0x20
>> > [   28.008847] [<ffff0000089b04d8>] dump_stack+0x9c/0xbc
>> > [   28.008852] [<ffff0000080ec4b4>] __schedule_bug+0x4c/0x70
>> > [   28.008856] [<ffff0000089c4828>] __schedule+0x558/0x5e8
>> > [   28.008859] [<ffff0000089c48f0>] schedule+0x38/0xa0
>> > [   28.008864] [<ffff0000089c8194>] schedule_hrtimeout_range_clock+0x84/0xe8
>> > [   28.008867] [<ffff0000089c8208>] schedule_hrtimeout_range+0x10/0x18
>> > [   28.008870] [<ffff0000089c7c74>] usleep_range+0x64/0x78
>> > [   28.008882] [<ffff000000ef56e4>] analogix_dp_transfer+0x16c/0xa88 [analogix_dp]
>> > [   28.008891] [<ffff000000ef2190>] analogix_dpaux_transfer+0x10/0x18 [analogix_dp]
>> > [   28.008950] [<ffff000000ddbb1c>] drm_dp_dpcd_access+0x4c/0xf8 [drm_kms_helper]
>> > [   28.008994] [<ffff000000ddbbe4>] drm_dp_dpcd_write+0x1c/0x28 [drm_kms_helper]
>> > [   28.009002] [<ffff000000ef2130>] analogix_dp_disable_psr+0x60/0xb0 [analogix_dp]
>> > [   28.009036] [<ffff000000f8398c>] analogix_dp_psr_work+0x4c/0xc0 [rockchipdrm]
>> > [   28.009040] [<ffff0000080dff44>] process_one_work+0x1d4/0x348
>> > [   28.009043] [<ffff0000080e0100>] worker_thread+0x48/0x470
>> > [   28.009048] [<ffff0000080e63d4>] kthread+0x12c/0x130
>> > [   28.009052] [<ffff000008084b88>] ret_from_fork+0x10/0x18
>> >
>> >
>> >
>> >
>>
>>
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel