| Message ID | 1512551301-12946-2-git-send-email-nickey.yang@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 06, 2017 at 05:08:19PM +0800, Nickey Yang wrote: > From: Brian Norris <briannorris@chromium.org> > > 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> > Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com> > Link:https://patchwork.kernel.org/patch/10078493/ > > --- > Changes > > v4: > - Add From tag,update subject line > - keep patch "drm/stm: dsi: Adjust dw_mipi_dsi_probe and remove" > in this piece together. > > v5: > - remove Review & Ack tag > - fix remove() directly referencing the static > dw_mipi_dsi_stm_plat_data struct. You've gotten this back into basically the shape I sent v1 in, so I guess this deserves an: Acked-by: Brian Norris <briannorris@chromium.org> :) Only net changes: you renamed the 'dsi' field to 'dmd' (Dwc Mipi Device?), and my 'return 0' changed back to the original 'return ret' at the end of probe(). Doesn't make any functional difference, so fine with me. If everybody's just as happy with this as with the first version...should it just be applied now, regardless of the status of the next 2 patches? Could help reduce the discussion (and confusion) surface. Regards, Brian > drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 36 ++++++++++----------------- > drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 10 +++++--- > include/drm/bridge/dw_mipi_dsi.h | 17 ++++++++----- > 3 files changed, 30 insertions(+), 33 deletions(-) ...
Hi Nickey, platform_set_drvdata() is still missing in your version. Thanks, Philippe :-) On 12/06/2017 10:39 PM, Brian Norris wrote: > On Wed, Dec 06, 2017 at 05:08:19PM +0800, Nickey Yang wrote: >> From: Brian Norris <briannorris@chromium.org> >> >> 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> >> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com> >> Link:https://patchwork.kernel.org/patch/10078493/ >> >> --- >> Changes >> >> v4: >> - Add From tag,update subject line >> - keep patch "drm/stm: dsi: Adjust dw_mipi_dsi_probe and remove" >> in this piece together. >> >> v5: >> - remove Review & Ack tag >> - fix remove() directly referencing the static >> dw_mipi_dsi_stm_plat_data struct. > > You've gotten this back into basically the shape I sent v1 in, so I > guess this deserves an: > > Acked-by: Brian Norris <briannorris@chromium.org> > > :) > > Only net changes: you renamed the 'dsi' field to 'dmd' (Dwc Mipi > Device?), and my 'return 0' changed back to the original 'return ret' at > the end of probe(). Doesn't make any functional difference, so fine with > me. > > If everybody's just as happy with this as with the first > version...should it just be applied now, regardless of the status of the > next 2 patches? Could help reduce the discussion (and confusion) > surface. > > Regards, > Brian > >> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 36 ++++++++++----------------- >> drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 10 +++++--- >> include/drm/bridge/dw_mipi_dsi.h | 17 ++++++++----- >> 3 files changed, 30 insertions(+), 33 deletions(-) > ... >
On Thu, Dec 07, 2017 at 11:41:17AM +0000, Philippe CORNU wrote: > platform_set_drvdata() is still missing in your version. Ugh...indeed. > On 12/06/2017 10:39 PM, Brian Norris wrote: > > On Wed, Dec 06, 2017 at 05:08:19PM +0800, Nickey Yang wrote: > >> From: Brian Norris <briannorris@chromium.org> > >> > >> 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> > >> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com> > >> Link:https://patchwork.kernel.org/patch/10078493/ > >> > >> --- > >> Changes > >> > >> v4: > >> - Add From tag,update subject line > >> - keep patch "drm/stm: dsi: Adjust dw_mipi_dsi_probe and remove" > >> in this piece together. > >> > >> v5: > >> - remove Review & Ack tag > >> - fix remove() directly referencing the static > >> dw_mipi_dsi_stm_plat_data struct. > > > > You've gotten this back into basically the shape I sent v1 in, so I > > guess this deserves an: > > > > Acked-by: Brian Norris <briannorris@chromium.org> ...and I even diffed the two directly and didn't notice that one missing at first :( So instead, make that: Nacked-by: Brian Norris <briannorris@chromium.org> Can you *please* just use my patch and reword the commit message/subject per the original comments on version 1? You're screwing it up every time you try to change it...
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..05787fa 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; @@ -329,10 +331,10 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev) 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->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__ */