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@chromium.org --- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 36 ++++++++++----------------- drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 14 +++++++---- include/drm/bridge/dw_mipi_dsi.h | 17 ++++++++----- 3 files changed, 33 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index d9cca4fd66ec..c39c7dce20ed 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 @@ __dw_mipi_dsi_probe(struct platform_device *pdev, 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 @@ EXPORT_SYMBOL_GPL(dw_mipi_dsi_remove); /* * 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 e5b6310240fe..7ed0ef7f6ec2 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 *dsi; };
static inline void dsi_write(struct dw_mipi_dsi_stm *dsi, u32 reg, u32 val) @@ -318,21 +319,24 @@ 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) { + platform_set_drvdata(pdev, dsi); + + dsi->dsi = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data); + if (IS_ERR(dsi->dsi)) { DRM_ERROR("Failed to initialize mipi dsi host\n"); clk_disable_unprepare(dsi->pllref_clk); + return PTR_ERR(dsi->dsi); }
- return ret; + return 0; }
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; + struct dw_mipi_dsi_stm *dsi = platform_get_drvdata(pdev);
clk_disable_unprepare(dsi->pllref_clk); - dw_mipi_dsi_remove(pdev); + dw_mipi_dsi_remove(dsi->dsi);
return 0; } diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h index 9b30fec302c8..d9c6d549f971 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__ */
El Mon, Nov 27, 2017 at 05:05:38PM -0800 Brian Norris ha dit:
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@chromium.org
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 36 ++++++++++----------------- drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 14 +++++++---- include/drm/bridge/dw_mipi_dsi.h | 17 ++++++++----- 3 files changed, 33 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index d9cca4fd66ec..c39c7dce20ed 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 @@ __dw_mipi_dsi_probe(struct platform_device *pdev, 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 @@ EXPORT_SYMBOL_GPL(dw_mipi_dsi_remove); /*
- 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);
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 e5b6310240fe..7ed0ef7f6ec2 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 *dsi;
};
static inline void dsi_write(struct dw_mipi_dsi_stm *dsi, u32 reg, u32 val) @@ -318,21 +319,24 @@ 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) {
- platform_set_drvdata(pdev, dsi);
- dsi->dsi = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data);
- if (IS_ERR(dsi->dsi)) { DRM_ERROR("Failed to initialize mipi dsi host\n"); clk_disable_unprepare(dsi->pllref_clk);
return PTR_ERR(dsi->dsi);
- return ret;
- return 0;
}
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;
struct dw_mipi_dsi_stm *dsi = platform_get_drvdata(pdev);
clk_disable_unprepare(dsi->pllref_clk);
- dw_mipi_dsi_remove(pdev);
dw_mipi_dsi_remove(dsi->dsi);
return 0;
} diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h index 9b30fec302c8..d9c6d549f971 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__ */
I don't claim to have expertise in this subsystem, but FWIW:
Reviewed-by: Matthias Kaehlcke mka@chromium.org
On 11/28/2017 06:35 AM, Brian Norris wrote:
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.
Reviewed-by: Archit Taneja architt@codeaurora.org
Signed-off-by: Brian Norris briannorris@chromium.org
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 36 ++++++++++----------------- drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 14 +++++++---- include/drm/bridge/dw_mipi_dsi.h | 17 ++++++++----- 3 files changed, 33 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index d9cca4fd66ec..c39c7dce20ed 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 @@ __dw_mipi_dsi_probe(struct platform_device *pdev, 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 @@ EXPORT_SYMBOL_GPL(dw_mipi_dsi_remove); /*
- 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);
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 e5b6310240fe..7ed0ef7f6ec2 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 *dsi; };
static inline void dsi_write(struct dw_mipi_dsi_stm *dsi, u32 reg, u32 val)
@@ -318,21 +319,24 @@ 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) {
- platform_set_drvdata(pdev, dsi);
- dsi->dsi = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data);
- if (IS_ERR(dsi->dsi)) { DRM_ERROR("Failed to initialize mipi dsi host\n"); clk_disable_unprepare(dsi->pllref_clk);
return PTR_ERR(dsi->dsi);
- return ret;
return 0; }
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;
struct dw_mipi_dsi_stm *dsi = platform_get_drvdata(pdev);
clk_disable_unprepare(dsi->pllref_clk);
- dw_mipi_dsi_remove(pdev);
dw_mipi_dsi_remove(dsi->dsi);
return 0; }
diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h index 9b30fec302c8..d9c6d549f971 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__ */
Hi Brian,
On 11/28/2017 02:05 AM, Brian Norris wrote:
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.
And many thanks for this cleanup. (please update the headline with "synopsys")
Successfully tested on stm.
Acked-by: Philippe Cornu philippe.cornu@st.com
Many thanks, Philippe :-)
Signed-off-by: Brian Norris briannorris@chromium.org
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 36 ++++++++++----------------- drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 14 +++++++---- include/drm/bridge/dw_mipi_dsi.h | 17 ++++++++----- 3 files changed, 33 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index d9cca4fd66ec..c39c7dce20ed 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 @@ __dw_mipi_dsi_probe(struct platform_device *pdev, 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 @@ EXPORT_SYMBOL_GPL(dw_mipi_dsi_remove); /*
- 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);
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 e5b6310240fe..7ed0ef7f6ec2 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 *dsi; };
static inline void dsi_write(struct dw_mipi_dsi_stm *dsi, u32 reg, u32 val)
@@ -318,21 +319,24 @@ 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) {
- platform_set_drvdata(pdev, dsi);
- dsi->dsi = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data);
- if (IS_ERR(dsi->dsi)) { DRM_ERROR("Failed to initialize mipi dsi host\n"); clk_disable_unprepare(dsi->pllref_clk);
return PTR_ERR(dsi->dsi);
- return ret;
return 0; }
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;
struct dw_mipi_dsi_stm *dsi = platform_get_drvdata(pdev);
clk_disable_unprepare(dsi->pllref_clk);
- dw_mipi_dsi_remove(pdev);
dw_mipi_dsi_remove(dsi->dsi);
return 0; }
diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h index 9b30fec302c8..d9c6d549f971 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__ */
Hi Archit, Andrzej & Laurent,
Regarding this patch from Brian, I think it could be nice to merge it (1xAcked-by, 2xReviewed-by).
Could you please have a look?
Only the small "typo" in the headline needs to be changed.
Many thanks, Philippe :-)
On 11/28/2017 10:34 AM, Philippe CORNU wrote:
Hi Brian,
On 11/28/2017 02:05 AM, Brian Norris wrote:
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.
And many thanks for this cleanup. (please update the headline with "synopsys")
Successfully tested on stm.
Acked-by: Philippe Cornu philippe.cornu@st.com
Many thanks, Philippe :-)
Signed-off-by: Brian Norris briannorris@chromium.org
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 36 ++++++++++----------------- drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 14 +++++++---- include/drm/bridge/dw_mipi_dsi.h | 17 ++++++++----- 3 files changed, 33 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index d9cca4fd66ec..c39c7dce20ed 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 @@ __dw_mipi_dsi_probe(struct platform_device *pdev, 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 @@ EXPORT_SYMBOL_GPL(dw_mipi_dsi_remove); /* * 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 e5b6310240fe..7ed0ef7f6ec2 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 *dsi; }; static inline void dsi_write(struct dw_mipi_dsi_stm *dsi, u32 reg, u32 val) @@ -318,21 +319,24 @@ 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) { + platform_set_drvdata(pdev, dsi);
+ dsi->dsi = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data); + if (IS_ERR(dsi->dsi)) { DRM_ERROR("Failed to initialize mipi dsi host\n"); clk_disable_unprepare(dsi->pllref_clk); + return PTR_ERR(dsi->dsi); } - return ret; + return 0; } 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; + struct dw_mipi_dsi_stm *dsi = platform_get_drvdata(pdev); clk_disable_unprepare(dsi->pllref_clk); - dw_mipi_dsi_remove(pdev); + dw_mipi_dsi_remove(dsi->dsi); return 0; } diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h index 9b30fec302c8..d9c6d549f971 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__ */
Hi Philippe,
On Tuesday, 9 January 2018 15:01:02 EET Philippe CORNU wrote:
Hi Archit, Andrzej & Laurent,
Regarding this patch from Brian, I think it could be nice to merge it (1xAcked-by, 2xReviewed-by).
Could you please have a look?
Only the small "typo" in the headline needs to be changed.
Many thanks,
I've just replied to Brian in this mail thread. Sorry for the delay, it slipped through the cracks. Thank you for pinging me.
On 11/28/2017 10:34 AM, Philippe CORNU wrote:
Hi Brian,
On 11/28/2017 02:05 AM, Brian Norris wrote:
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.
And many thanks for this cleanup. (please update the headline with "synopsys")
Successfully tested on stm.
Acked-by: Philippe Cornu philippe.cornu@st.com
Many thanks, Philippe :-)
Signed-off-by: Brian Norris briannorris@chromium.org
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 36 ++++++++++----------------- drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 14 +++++++---- include/drm/bridge/dw_mipi_dsi.h | 17 ++++++++----- 3 files changed, 33 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index d9cca4fd66ec..c39c7dce20ed 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 @@ __dw_mipi_dsi_probe(struct platform_device *pdev, 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 @@ EXPORT_SYMBOL_GPL(dw_mipi_dsi_remove); /*
- 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)
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);diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c index e5b6310240fe..7ed0ef7f6ec2 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 *dsi; }; static inline void dsi_write(struct dw_mipi_dsi_stm *dsi, u32 reg,
u32 val) @@ -318,21 +319,24 @@ 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) {
- platform_set_drvdata(pdev, dsi);
- dsi->dsi = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data);
- if (IS_ERR(dsi->dsi)) { DRM_ERROR("Failed to initialize mipi dsi host\n"); clk_disable_unprepare(dsi->pllref_clk);
return PTR_ERR(dsi->dsi); }
- return ret;
- return 0; } 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;
- struct dw_mipi_dsi_stm *dsi = platform_get_drvdata(pdev); clk_disable_unprepare(dsi->pllref_clk);
- dw_mipi_dsi_remove(pdev);
- dw_mipi_dsi_remove(dsi->dsi); return 0; }
diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h index 9b30fec302c8..d9c6d549f971 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__ */
On 09.01.2018 14:37, Laurent Pinchart wrote:
Hi Philippe,
On Tuesday, 9 January 2018 15:01:02 EET Philippe CORNU wrote:
Hi Archit, Andrzej & Laurent,
Regarding this patch from Brian, I think it could be nice to merge it (1xAcked-by, 2xReviewed-by).
Could you please have a look?
Only the small "typo" in the headline needs to be changed.
Many thanks,
I've just replied to Brian in this mail thread. Sorry for the delay, it slipped through the cracks. Thank you for pinging me.
I have just pushed the patch to drm-misc-next. I am sorry, if you wanted to polish it more, from the thread it looked it can be merged as is.
Regards Andrzej
On 11/28/2017 10:34 AM, Philippe CORNU wrote:
Hi Brian,
On 11/28/2017 02:05 AM, Brian Norris wrote:
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.
And many thanks for this cleanup. (please update the headline with "synopsys")
Successfully tested on stm.
Acked-by: Philippe Cornu philippe.cornu@st.com
Many thanks, Philippe :-)
Signed-off-by: Brian Norris briannorris@chromium.org
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 36 ++++++++++----------------- drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 14 +++++++---- include/drm/bridge/dw_mipi_dsi.h | 17 ++++++++----- 3 files changed, 33 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index d9cca4fd66ec..c39c7dce20ed 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 @@ __dw_mipi_dsi_probe(struct platform_device *pdev, 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 @@ EXPORT_SYMBOL_GPL(dw_mipi_dsi_remove); /*
- 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)
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);diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c index e5b6310240fe..7ed0ef7f6ec2 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 *dsi; }; static inline void dsi_write(struct dw_mipi_dsi_stm *dsi, u32 reg,
u32 val) @@ -318,21 +319,24 @@ 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) {
- platform_set_drvdata(pdev, dsi);
- dsi->dsi = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data);
- if (IS_ERR(dsi->dsi)) { DRM_ERROR("Failed to initialize mipi dsi host\n"); clk_disable_unprepare(dsi->pllref_clk);
return PTR_ERR(dsi->dsi); }
- return ret;
- return 0; } 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;
- struct dw_mipi_dsi_stm *dsi = platform_get_drvdata(pdev); clk_disable_unprepare(dsi->pllref_clk);
- dw_mipi_dsi_remove(pdev);
- dw_mipi_dsi_remove(dsi->dsi); return 0; }
diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h index 9b30fec302c8..d9c6d549f971 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__ */
Hi Brian,
Thank you for the patch.
I'd mention dw-mipi-dsi in the subject line as the directory contains the dw- hdmi driver as well that this patch doesn't touch.
On Tuesday, 28 November 2017 03:05:38 EET Brian Norris wrote:
Bridge drivers/helpers shouldn't be clobbering the drvdata, since a parent driver might need to own this.
By parent driver I assume you mean a glue driver that binds to the SoC- specific compatible string for the DSI transmitter.
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@chromium.org
Wouldn't it be cleaner to embed the dw_mipi_dsi structure in the parent- specific data structure (struct dw_mipi_dsi_stm and struct dw_mipi_dsi_rockchip when the "[PATCH v3 0/5] Update ROCKCHIP DSI driver that uses dw-mipi-dsi bridge" patch series will land) instead of allocating it dynamically ? We would then have a single object to track.
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 36 ++++++++++-------------- drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 14 +++++++---- include/drm/bridge/dw_mipi_dsi.h | 17 ++++++++----- 3 files changed, 33 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index d9cca4fd66ec..c39c7dce20ed 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 @@ __dw_mipi_dsi_probe(struct platform_device *pdev, 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 @@ EXPORT_SYMBOL_GPL(dw_mipi_dsi_remove); /*
- 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);
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 e5b6310240fe..7ed0ef7f6ec2 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 *dsi;
};
static inline void dsi_write(struct dw_mipi_dsi_stm *dsi, u32 reg, u32 val) @@ -318,21 +319,24 @@ 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) {
- platform_set_drvdata(pdev, dsi);
- dsi->dsi = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data);
- if (IS_ERR(dsi->dsi)) { DRM_ERROR("Failed to initialize mipi dsi host\n"); clk_disable_unprepare(dsi->pllref_clk);
return PTR_ERR(dsi->dsi);
- return ret;
- return 0;
}
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;
struct dw_mipi_dsi_stm *dsi = platform_get_drvdata(pdev);
clk_disable_unprepare(dsi->pllref_clk);
- dw_mipi_dsi_remove(pdev);
dw_mipi_dsi_remove(dsi->dsi);
return 0;
} diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h index 9b30fec302c8..d9c6d549f971 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__ */
Hi Laurent,
On Tue, Nov 28, 2017 at 02:51:46PM +0200, Laurent Pinchart wrote:
Hi Brian,
Thank you for the patch.
I'd mention dw-mipi-dsi in the subject line as the directory contains the dw- hdmi driver as well that this patch doesn't touch.
Yep. Does it need another tag in the subject? e.g., '.../dw-mipi-dsi:'?
On Tuesday, 28 November 2017 03:05:38 EET Brian Norris wrote:
Bridge drivers/helpers shouldn't be clobbering the drvdata, since a parent driver might need to own this.
By parent driver I assume you mean a glue driver that binds to the SoC- specific compatible string for the DSI transmitter.
Indeed. Nickey picked this up for his Rockchip driver submission, but maybe we should reword the commit message a bit.
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@chromium.org
Wouldn't it be cleaner to embed the dw_mipi_dsi structure in the parent- specific data structure (struct dw_mipi_dsi_stm and struct dw_mipi_dsi_rockchip when the "[PATCH v3 0/5] Update ROCKCHIP DSI driver that uses dw-mipi-dsi bridge" patch series will land) instead of allocating it dynamically ? We would then have a single object to track.
I suppose we could do that too. But that would require exposing the whole layout of 'struct dw_mipi_dsi' to users. Do we want to sacrifice the enforced separation for a little bit of nicer object handling?
Also, this was modeled a bit after the similar rework needed to untangle the drvdata handling in the Rockchip analogix DP driver vs. the analogix bridge DP code:
[PATCH v6 03/10] drm/bridge: analogix: Do not use device's drvdata https://patchwork.kernel.org/patch/10015875/
Brian
Hi Brian,
On Tuesday, 28 November 2017 20:21:23 EET Brian Norris wrote:
On Tue, Nov 28, 2017 at 02:51:46PM +0200, Laurent Pinchart wrote:
Hi Brian,
Thank you for the patch.
I'd mention dw-mipi-dsi in the subject line as the directory contains the dw-hdmi driver as well that this patch doesn't touch.
Yep. Does it need another tag in the subject? e.g., '.../dw-mipi-dsi:'?
On Tuesday, 28 November 2017 03:05:38 EET Brian Norris wrote:
Bridge drivers/helpers shouldn't be clobbering the drvdata, since a parent driver might need to own this.
By parent driver I assume you mean a glue driver that binds to the SoC- specific compatible string for the DSI transmitter.
Indeed. Nickey picked this up for his Rockchip driver submission, but maybe we should reword the commit message a bit.
How about "drm: dw-mipi-dsi: Stop clobbering drvdata" ?
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@chromium.org
Wouldn't it be cleaner to embed the dw_mipi_dsi structure in the parent- specific data structure (struct dw_mipi_dsi_stm and struct dw_mipi_dsi_rockchip when the "[PATCH v3 0/5] Update ROCKCHIP DSI driver that uses dw-mipi-dsi bridge" patch series will land) instead of allocating it dynamically ? We would then have a single object to track.
I suppose we could do that too. But that would require exposing the whole layout of 'struct dw_mipi_dsi' to users. Do we want to sacrifice the enforced separation for a little bit of nicer object handling?
I certainly don't think we should go for spaghetti code with all objects accessing each other :) On the other hand, we're talking about C code, and we thus have no way to enforce access restrictions in the compiler, so it's a lost battle anyway. I don't see an issue with exposing the object in the sense of moving its definition to a header file if it results in cleaner code. I think we need to trust developers not to abuse internal APIs, and if they do, catch it during review.
Also, this was modeled a bit after the similar rework needed to untangle the drvdata handling in the Rockchip analogix DP driver vs. the analogix bridge DP code:
[PATCH v6 03/10] drm/bridge: analogix: Do not use device's drvdata https://patchwork.kernel.org/patch/10015875/
dri-devel@lists.freedesktop.org
-
Andrzej Hajda -
Archit Taneja -
Brian Norris -
Laurent Pinchart -
Matthias Kaehlcke -
Philippe CORNU