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(-)
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,
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
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); }
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
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; }
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
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 :)
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; }
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
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);
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);@@ -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);
analogix_dp_disable_psr(dp->dev);
analogix_dp_disable_psr(dp->adp);}
@@ -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);- } 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
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(-)
Hi,
On 10/18/2017 05:13 AM, Jingoo Han wrote:
On Tuesday, October 17, 2017 6:16 AM, Jeffy Chen wrote:
From: Tomasz Figa tfiga@chromium.org
The driver that instantiates the bridge should own the drvdata, as all driver model callbacks (probe, remove, shutdown, PM ops, etc.) are also owned by its driver struct. Moreover, storing two different pointer types in driver data depending on driver initialization status is barely a good practice and in fact has led to many bugs in this driver.
Let's clean up this mess and change Analogix entry points to simply accept some opaque struct pointer, adjusting their users at the same time to avoid breaking the compilation.
Signed-off-by: Tomasz Figa tfiga@chromium.org Signed-off-by: Jeffy Chen jeffy.chen@rock-chips.com Reviewed-by: Andrzej Hajda a.hajda@samsung.com
This depends on previous patches of the series. I guess it would be easier to queue this to drm-misc as a part of the eDP support series. For that:
Acked-by: Archit Taneja architt@codeaurora.org
Acked-by: Jingoo Han jingoohan1@gmail.com
Best regards, Jingoo Han
Changes in v4: None Changes in v3: None Changes in v2: None
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 50 +++++++++--------
drivers/gpu/drm/exynos/exynos_dp.c | 26 ++++++----- drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 47
+++++++++++-------
-- include/drm/bridge/analogix_dp.h | 19 ++++---- 4 files changed, 73 insertions(+), 69 deletions(-)
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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; }
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);@@ -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
dri-devel@lists.freedesktop.org
-
Archit Taneja -
jeffy -
Jeffy Chen
-
Jingoo Han -
Sean Paul