Re: [PATCH v4 1/3] drm/bridge/synopsys: dsi: stop clobbering drvdata

Nickey Yang <nickey.yang@xxxxxxxxxxxxxx> · Tue, 5 Dec 2017 17:14:11 +0800

Hi Philippe,


On 2017年12月01日 18:07, Philippe CORNU wrote:
Hi Nickey,

On 12/01/2017 10:11 AM, Nickey Yang wrote:
Hi Philippe,


On 2017年12月01日 16:32, Philippe CORNU wrote:
Dear Nickey,

Many thanks for your patch.

I am sorry to say that but you can not add my "Acked-by" to this patch
because this code is different from the "original" one from Brian (which
got my "Acked-by").
I'm sorry I didn't think much about it, Thank you for correcting me.
Sometimes it is not an issue because differences are not important but
in this particular case, the code is really different: you have remove
platform_set_drvdata() & platform_get_drvdata() in the stm part.

Could you please go back to the original code or propose me an updated
version of this code.
Could you help update new version of this code(stm part) and then test on
stm platform?
I think you can simply goes back to the original version from Brian (see
the discussion thread in https://patchwork.kernel.org/patch/10078493/)
unless you have specific/good reasons for modifying the code as you did.
mmm,I'm sorry, I feel a little puzzled. Do you means we should abandon
Brian's patch (https://patchwork.kernel.org/patch/10078493/)?
I think we need to adjust stm part because  dw_mipi_dsi_stm.c calls
bridge's drivers if we want merge Brian's patch.


Thanks
Many thanks,
Philippe :)

Thanks,
Nickey.
Many thanks,

Philippe :-)


On 12/01/2017 04:58 AM, Nickey Yang wrote:
From: Brian Norris<briannorris@xxxxxxxxxxxx>

Bridge drivers/helpers shouldn't be clobbering the drvdata, since a
parent driver might need to own this. Instead, let's return our
'dw_mipi_dsi' object and have callers pass that back to us for removal.

Signed-off-by: Brian Norris<briannorris@xxxxxxxxxxxx>
Signed-off-by: Nickey Yang<nickey.yang@xxxxxxxxxxxxxx>
Reviewed-by: Matthias Kaehlcke<mka@xxxxxxxxxxxx>
Reviewed-by: Archit Taneja<architt@xxxxxxxxxxxxxx>
Acked-by: Philippe Cornu<philippe.cornu@xxxxxx>
Link:https://patchwork.kernel.org/patch/10078493/

---
Changes in v4:
- Add From tag,update subject line
- keep patch "drm/stm: dsi: Adjust dw_mipi_dsi_probe and remove"
     in this piece together.

    drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 36 ++++++++++-----------------
    drivers/gpu/drm/stm/dw_mipi_dsi-stm.c         |  8 +++---
    include/drm/bridge/dw_mipi_dsi.h              | 17 ++++++++-----
    3 files changed, 29 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index d9cca4f..c39c7dc 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -922,8 +922,6 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge)
    	dsi->bridge.of_node = pdev->dev.of_node;
    #endif
    
-	dev_set_drvdata(dev, dsi);
-
    	return dsi;
    }
    
@@ -935,23 +933,16 @@ static void __dw_mipi_dsi_remove(struct dw_mipi_dsi *dsi)
    /*
     * Probe/remove API, used from platforms based on the DRM bridge API.
     */
-int dw_mipi_dsi_probe(struct platform_device *pdev,
-		      const struct dw_mipi_dsi_plat_data *plat_data)
+struct dw_mipi_dsi *
+dw_mipi_dsi_probe(struct platform_device *pdev,
+		  const struct dw_mipi_dsi_plat_data *plat_data)
    {
-	struct dw_mipi_dsi *dsi;
-
-	dsi = __dw_mipi_dsi_probe(pdev, plat_data);
-	if (IS_ERR(dsi))
-		return PTR_ERR(dsi);
-
-	return 0;
+	return __dw_mipi_dsi_probe(pdev, plat_data);
    }
    EXPORT_SYMBOL_GPL(dw_mipi_dsi_probe);
    
-void dw_mipi_dsi_remove(struct platform_device *pdev)
+void dw_mipi_dsi_remove(struct dw_mipi_dsi *dsi)
    {
-	struct dw_mipi_dsi *dsi = platform_get_drvdata(pdev);
-
    	mipi_dsi_host_unregister(&dsi->dsi_host);
    
    	__dw_mipi_dsi_remove(dsi);
@@ -961,31 +952,30 @@ void dw_mipi_dsi_remove(struct platform_device *pdev)
    /*
     * Bind/unbind API, used from platforms based on the component framework.
     */
-int dw_mipi_dsi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
-		     const struct dw_mipi_dsi_plat_data *plat_data)
+struct dw_mipi_dsi *
+dw_mipi_dsi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
+		 const struct dw_mipi_dsi_plat_data *plat_data)
    {
    	struct dw_mipi_dsi *dsi;
    	int ret;
    
    	dsi = __dw_mipi_dsi_probe(pdev, plat_data);
    	if (IS_ERR(dsi))
-		return PTR_ERR(dsi);
+		return dsi;
    
    	ret = drm_bridge_attach(encoder, &dsi->bridge, NULL);
    	if (ret) {
-		dw_mipi_dsi_remove(pdev);
+		dw_mipi_dsi_remove(dsi);
    		DRM_ERROR("Failed to initialize bridge with drm\n");
-		return ret;
+		return ERR_PTR(ret);
    	}
    
-	return 0;
+	return dsi;
    }
    EXPORT_SYMBOL_GPL(dw_mipi_dsi_bind);
    
-void dw_mipi_dsi_unbind(struct device *dev)
+void dw_mipi_dsi_unbind(struct dw_mipi_dsi *dsi)
    {
-	struct dw_mipi_dsi *dsi = dev_get_drvdata(dev);
-
    	__dw_mipi_dsi_remove(dsi);
    }
    EXPORT_SYMBOL_GPL(dw_mipi_dsi_unbind);
diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
index e5b6310..80f9950 100644
--- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
+++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
@@ -66,6 +66,7 @@ enum dsi_color {
    struct dw_mipi_dsi_stm {
    	void __iomem *base;
    	struct clk *pllref_clk;
+	struct dw_mipi_dsi *dmd;
    };
    
    static inline void dsi_write(struct dw_mipi_dsi_stm *dsi, u32 reg, u32 val)
@@ -318,10 +319,11 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
    	dw_mipi_dsi_stm_plat_data.base = dsi->base;
    	dw_mipi_dsi_stm_plat_data.priv_data = dsi;
    
-	ret = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data);
-	if (ret) {
+	dsi->dmd = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data);
+	if (IS_ERR(dsi->dmd)) {
    		DRM_ERROR("Failed to initialize mipi dsi host\n");
    		clk_disable_unprepare(dsi->pllref_clk);
+		return PTR_ERR(dsi->dmd);
    	}
    
    	return ret;
@@ -332,7 +334,7 @@ static int dw_mipi_dsi_stm_remove(struct platform_device *pdev)
    	struct dw_mipi_dsi_stm *dsi = dw_mipi_dsi_stm_plat_data.priv_data;
    
    	clk_disable_unprepare(dsi->pllref_clk);
-	dw_mipi_dsi_remove(pdev);
+	dw_mipi_dsi_remove(dsi->dmd);
    
    	return 0;
    }
diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h
index 9b30fec..d9c6d54 100644
--- a/include/drm/bridge/dw_mipi_dsi.h
+++ b/include/drm/bridge/dw_mipi_dsi.h
@@ -10,6 +10,8 @@
    #ifndef __DW_MIPI_DSI__
    #define __DW_MIPI_DSI__
    
+struct dw_mipi_dsi;
+
    struct dw_mipi_dsi_phy_ops {
    	int (*init)(void *priv_data);
    	int (*get_lane_mbps)(void *priv_data, struct drm_display_mode *mode,
@@ -29,11 +31,14 @@ struct dw_mipi_dsi_plat_data {
    	void *priv_data;
    };
    
-int dw_mipi_dsi_probe(struct platform_device *pdev,
-		      const struct dw_mipi_dsi_plat_data *plat_data);
-void dw_mipi_dsi_remove(struct platform_device *pdev);
-int dw_mipi_dsi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
-		     const struct dw_mipi_dsi_plat_data *plat_data);
-void dw_mipi_dsi_unbind(struct device *dev);
+struct dw_mipi_dsi *dw_mipi_dsi_probe(struct platform_device *pdev,
+				      const struct dw_mipi_dsi_plat_data
+				      *plat_data);
+void dw_mipi_dsi_remove(struct dw_mipi_dsi *dsi);
+struct dw_mipi_dsi *dw_mipi_dsi_bind(struct platform_device *pdev,
+				     struct drm_encoder *encoder,
+				     const struct dw_mipi_dsi_plat_data
+				     *plat_data);
+void dw_mipi_dsi_unbind(struct dw_mipi_dsi *dsi);
    
    #endif /* __DW_MIPI_DSI__ */


_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel







