Re: [RESEND PATCH v2 4/5] drm/bridge/analogix: Do not use device's drvdata

[Andrzej Hajda]

On 16.10.2017 12:06, Jeffy Chen wrote:
> From: Tomasz Figa 
>
> 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 
> Signed-off-by: Jeffy Chen 

Thanks for making it a bit saner.

Reviewed-by: Andrzej Hajda 

 --
Regards
Andrzej
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RESEND PATCH v2 4/5] drm/bridge/analogix: Do not use device's drvdata

[Jeffy Chen]

From: Tomasz Figa 

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 
Signed-off-by: Jeffy Chen 
---

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 = >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(>dev, "dp");
if (IS_ERR(dp->clock)) {
dev_err(>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(>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(>dev, "failed to get hpd gpio\n");
-   return ret;
+   return ERR_PTR(ret);
}
dp->irq