[PATCH RESEND v2 1/2] drm/bridge: dw-hdmi: Don't cleanup i2c adapter and ddc ptr in __dw_hdmi_probe() bailout path
It's unnecessary to cleanup the i2c adapter and the ddc pointer in the bailout path of __dw_hdmi_probe(), since the adapter is not added and the ddc pointer is not set.
Fixes: a23d6265f033 (drm: bridge: dw-hdmi: Extract PHY interrupt setup to a function) Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Boris Brezillon boris.brezillon@collabora.com Cc: Jerome Brunet jbrunet@baylibre.com Cc: Cheng-Yi Chiang cychiang@chromium.org Cc: Dariusz Marcinkiewicz darekm@google.com Cc: Archit Taneja architt@codeaurora.org Cc: Jose Abreu joabreu@synopsys.com Cc: dri-devel@lists.freedesktop.org Cc: NXP Linux Team linux-imx@nxp.com Signed-off-by: Liu Ying victor.liu@nxp.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- v1->v2: * Add Laurent's R-b tag.
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 6148a02..137b6eb 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -3441,11 +3441,6 @@ __dw_hdmi_probe(struct platform_device *pdev, return hdmi;
err_iahb: - if (hdmi->i2c) { - i2c_del_adapter(&hdmi->i2c->adap); - hdmi->ddc = NULL; - } - clk_disable_unprepare(hdmi->iahb_clk); if (hdmi->cec_clk) clk_disable_unprepare(hdmi->cec_clk);
It doesn't hurt to add the bridge in the global bridge list also for platform specific dw-hdmi drivers which are based on the component framework. This can be achieved by moving the drm_bridge_add() function call from dw_hdmi_probe() to __dw_hdmi_probe(). A counterpart movement for drm_bridge_remove() is also needed then. Moreover, since drm_bridge_add() initializes &bridge->hpd_mutex, this may help those platform specific dw-hdmi drivers(based on the component framework) avoid accessing the uninitialized mutex in drm_bridge_hpd_notify() which is called in dw_hdmi_irq(). Putting drm_bridge_add() in __dw_hdmi_probe() just before it returns successfully should bring no logic change for platforms based on the DRM bridge API, which is a good choice from safety point of view. Also, __dw_hdmi_probe() is renamed to dw_hdmi_probe() since dw_hdmi_probe() does nothing else but calling __dw_hdmi_probe(). Similar renaming applies to the __dw_hdmi_remove()/dw_hdmi_remove() pair.
Fixes: ec971aaa6775 ("drm: bridge: dw-hdmi: Make connector creation optional") Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Boris Brezillon boris.brezillon@collabora.com Cc: Jerome Brunet jbrunet@baylibre.com Cc: Cheng-Yi Chiang cychiang@chromium.org Cc: Dariusz Marcinkiewicz darekm@google.com Cc: Archit Taneja architt@codeaurora.org Cc: Jose Abreu joabreu@synopsys.com Cc: Sam Ravnborg sam@ravnborg.org Cc: dri-devel@lists.freedesktop.org Cc: NXP Linux Team linux-imx@nxp.com Signed-off-by: Liu Ying victor.liu@nxp.com --- v1->v2: * Put drm_bridge_add() in __dw_hdmi_probe() just before it returns successfully so that the bridge shows up in the global bridge list late enough to avoid potential race condition with other display drivers. (Laurent) * Rename __dw_hdmi_probe/remove() to dw_hdmi_probe/remove() respectively. (Laurent) * Cc'ed Sam.
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 44 +++++++++---------------------- 1 file changed, 13 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 137b6eb..748df1c 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -3179,9 +3179,11 @@ static void dw_hdmi_init_hw(struct dw_hdmi *hdmi) hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data); }
-static struct dw_hdmi * -__dw_hdmi_probe(struct platform_device *pdev, - const struct dw_hdmi_plat_data *plat_data) +/* ----------------------------------------------------------------------------- + * Probe/remove API, used from platforms based on the DRM bridge API. + */ +struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev, + const struct dw_hdmi_plat_data *plat_data) { struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; @@ -3438,6 +3440,8 @@ __dw_hdmi_probe(struct platform_device *pdev, hdmi->cec = platform_device_register_full(&pdevinfo); }
+ drm_bridge_add(&hdmi->bridge); + return hdmi;
err_iahb: @@ -3451,9 +3455,12 @@ __dw_hdmi_probe(struct platform_device *pdev,
return ERR_PTR(ret); } +EXPORT_SYMBOL_GPL(dw_hdmi_probe);
-static void __dw_hdmi_remove(struct dw_hdmi *hdmi) +void dw_hdmi_remove(struct dw_hdmi *hdmi) { + drm_bridge_remove(&hdmi->bridge); + if (hdmi->audio && !IS_ERR(hdmi->audio)) platform_device_unregister(hdmi->audio); if (!IS_ERR(hdmi->cec)) @@ -3472,31 +3479,6 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi) else i2c_put_adapter(hdmi->ddc); } - -/* ----------------------------------------------------------------------------- - * Probe/remove API, used from platforms based on the DRM bridge API. - */ -struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev, - const struct dw_hdmi_plat_data *plat_data) -{ - struct dw_hdmi *hdmi; - - hdmi = __dw_hdmi_probe(pdev, plat_data); - if (IS_ERR(hdmi)) - return hdmi; - - drm_bridge_add(&hdmi->bridge); - - return hdmi; -} -EXPORT_SYMBOL_GPL(dw_hdmi_probe); - -void dw_hdmi_remove(struct dw_hdmi *hdmi) -{ - drm_bridge_remove(&hdmi->bridge); - - __dw_hdmi_remove(hdmi); -} EXPORT_SYMBOL_GPL(dw_hdmi_remove);
/* ----------------------------------------------------------------------------- @@ -3509,7 +3491,7 @@ struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev, struct dw_hdmi *hdmi; int ret;
- hdmi = __dw_hdmi_probe(pdev, plat_data); + hdmi = dw_hdmi_probe(pdev, plat_data); if (IS_ERR(hdmi)) return hdmi;
@@ -3526,7 +3508,7 @@ EXPORT_SYMBOL_GPL(dw_hdmi_bind);
void dw_hdmi_unbind(struct dw_hdmi *hdmi) { - __dw_hdmi_remove(hdmi); + dw_hdmi_remove(hdmi); } EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
On Thu, Jul 09, 2020 at 10:02:36AM +0800, Liu Ying wrote:
It doesn't hurt to add the bridge in the global bridge list also for platform specific dw-hdmi drivers which are based on the component framework. This can be achieved by moving the drm_bridge_add() function call from dw_hdmi_probe() to __dw_hdmi_probe(). A counterpart movement for drm_bridge_remove() is also needed then. Moreover, since drm_bridge_add() initializes &bridge->hpd_mutex, this may help those platform specific dw-hdmi drivers(based on the component framework) avoid accessing the uninitialized mutex in drm_bridge_hpd_notify() which is called in dw_hdmi_irq(). Putting drm_bridge_add() in __dw_hdmi_probe() just before it returns successfully should bring no logic change for platforms based on the DRM bridge API, which is a good choice from safety point of view. Also, __dw_hdmi_probe() is renamed to dw_hdmi_probe() since dw_hdmi_probe() does nothing else but calling __dw_hdmi_probe(). Similar renaming applies to the __dw_hdmi_remove()/dw_hdmi_remove() pair.
Hmm, it took me some attempts to read this. Anyway, applied as-is to drm-misc-next.
Sam
Fixes: ec971aaa6775 ("drm: bridge: dw-hdmi: Make connector creation optional") Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Boris Brezillon boris.brezillon@collabora.com Cc: Jerome Brunet jbrunet@baylibre.com Cc: Cheng-Yi Chiang cychiang@chromium.org Cc: Dariusz Marcinkiewicz darekm@google.com Cc: Archit Taneja architt@codeaurora.org Cc: Jose Abreu joabreu@synopsys.com Cc: Sam Ravnborg sam@ravnborg.org Cc: dri-devel@lists.freedesktop.org Cc: NXP Linux Team linux-imx@nxp.com Signed-off-by: Liu Ying victor.liu@nxp.com
v1->v2:
- Put drm_bridge_add() in __dw_hdmi_probe() just before it returns successfully so that the bridge shows up in the global bridge list late enough to avoid potential race condition with other display drivers. (Laurent)
- Rename __dw_hdmi_probe/remove() to dw_hdmi_probe/remove() respectively. (Laurent)
- Cc'ed Sam.
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 44 +++++++++---------------------- 1 file changed, 13 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 137b6eb..748df1c 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -3179,9 +3179,11 @@ static void dw_hdmi_init_hw(struct dw_hdmi *hdmi) hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data); }
-static struct dw_hdmi * -__dw_hdmi_probe(struct platform_device *pdev,
const struct dw_hdmi_plat_data *plat_data)+/* -----------------------------------------------------------------------------
- Probe/remove API, used from platforms based on the DRM bridge API.
- */
+struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
const struct dw_hdmi_plat_data *plat_data){ struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; @@ -3438,6 +3440,8 @@ __dw_hdmi_probe(struct platform_device *pdev, hdmi->cec = platform_device_register_full(&pdevinfo); }
- drm_bridge_add(&hdmi->bridge);
- return hdmi;
err_iahb: @@ -3451,9 +3455,12 @@ __dw_hdmi_probe(struct platform_device *pdev,
return ERR_PTR(ret); } +EXPORT_SYMBOL_GPL(dw_hdmi_probe);
-static void __dw_hdmi_remove(struct dw_hdmi *hdmi) +void dw_hdmi_remove(struct dw_hdmi *hdmi) {
- drm_bridge_remove(&hdmi->bridge);
- if (hdmi->audio && !IS_ERR(hdmi->audio)) platform_device_unregister(hdmi->audio); if (!IS_ERR(hdmi->cec))
@@ -3472,31 +3479,6 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi) else i2c_put_adapter(hdmi->ddc); }
-/* -----------------------------------------------------------------------------
- Probe/remove API, used from platforms based on the DRM bridge API.
- */
-struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
const struct dw_hdmi_plat_data *plat_data)-{
- struct dw_hdmi *hdmi;
- hdmi = __dw_hdmi_probe(pdev, plat_data);
- if (IS_ERR(hdmi))
return hdmi;- drm_bridge_add(&hdmi->bridge);
- return hdmi;
-} -EXPORT_SYMBOL_GPL(dw_hdmi_probe);
-void dw_hdmi_remove(struct dw_hdmi *hdmi) -{
- drm_bridge_remove(&hdmi->bridge);
- __dw_hdmi_remove(hdmi);
-} EXPORT_SYMBOL_GPL(dw_hdmi_remove);
/* ----------------------------------------------------------------------------- @@ -3509,7 +3491,7 @@ struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev, struct dw_hdmi *hdmi; int ret;
- hdmi = __dw_hdmi_probe(pdev, plat_data);
- hdmi = dw_hdmi_probe(pdev, plat_data); if (IS_ERR(hdmi)) return hdmi;
@@ -3526,7 +3508,7 @@ EXPORT_SYMBOL_GPL(dw_hdmi_bind);
void dw_hdmi_unbind(struct dw_hdmi *hdmi) {
- __dw_hdmi_remove(hdmi);
- dw_hdmi_remove(hdmi);
} EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
-- 2.7.4
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, 2020-07-10 at 19:32 +0200, Sam Ravnborg wrote:
On Thu, Jul 09, 2020 at 10:02:36AM +0800, Liu Ying wrote:
It doesn't hurt to add the bridge in the global bridge list also for platform specific dw-hdmi drivers which are based on the component framework. This can be achieved by moving the drm_bridge_add() function call from dw_hdmi_probe() to __dw_hdmi_probe(). A counterpart movement for drm_bridge_remove() is also needed then. Moreover, since drm_bridge_add() initializes &bridge->hpd_mutex, this may help those platform specific dw-hdmi drivers(based on the component framework) avoid accessing the uninitialized mutex in drm_bridge_hpd_notify() which is called in dw_hdmi_irq(). Putting drm_bridge_add() in __dw_hdmi_probe() just before it returns successfully should bring no logic change for platforms based on the DRM bridge API, which is a good choice from safety point of view. Also, __dw_hdmi_probe() is renamed to dw_hdmi_probe() since dw_hdmi_probe() does nothing else but calling __dw_hdmi_probe(). Similar renaming applies to the __dw_hdmi_remove()/dw_hdmi_remove() pair.
Hmm, it took me some attempts to read this. Anyway, applied as-is to drm-misc-next.
Thank you, Sam.
Liu Ying
Sam
Fixes: ec971aaa6775 ("drm: bridge: dw-hdmi: Make connector creation optional") Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Boris Brezillon boris.brezillon@collabora.com Cc: Jerome Brunet jbrunet@baylibre.com Cc: Cheng-Yi Chiang cychiang@chromium.org Cc: Dariusz Marcinkiewicz darekm@google.com Cc: Archit Taneja architt@codeaurora.org Cc: Jose Abreu joabreu@synopsys.com Cc: Sam Ravnborg sam@ravnborg.org Cc: dri-devel@lists.freedesktop.org Cc: NXP Linux Team linux-imx@nxp.com Signed-off-by: Liu Ying victor.liu@nxp.com
v1->v2:
- Put drm_bridge_add() in __dw_hdmi_probe() just before it returns successfully so that the bridge shows up in the global bridge
list late enough to avoid potential race condition with other display drivers. (Laurent)
- Rename __dw_hdmi_probe/remove() to dw_hdmi_probe/remove() respectively. (Laurent)
- Cc'ed Sam.
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 44 +++++++++--------
1 file changed, 13 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 137b6eb..748df1c 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -3179,9 +3179,11 @@ static void dw_hdmi_init_hw(struct dw_hdmi *hdmi) hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data); }
-static struct dw_hdmi * -__dw_hdmi_probe(struct platform_device *pdev,
const struct dw_hdmi_plat_data *plat_data)+/* -------------------------------------------------------------
- Probe/remove API, used from platforms based on the DRM bridge
API.
- */
+struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
const struct dw_hdmi_plat_data*plat_data) { struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; @@ -3438,6 +3440,8 @@ __dw_hdmi_probe(struct platform_device *pdev, hdmi->cec = platform_device_register_full(&pdevinfo); }
- drm_bridge_add(&hdmi->bridge);
- return hdmi;
err_iahb: @@ -3451,9 +3455,12 @@ __dw_hdmi_probe(struct platform_device *pdev,
return ERR_PTR(ret); } +EXPORT_SYMBOL_GPL(dw_hdmi_probe);
-static void __dw_hdmi_remove(struct dw_hdmi *hdmi) +void dw_hdmi_remove(struct dw_hdmi *hdmi) {
- drm_bridge_remove(&hdmi->bridge);
- if (hdmi->audio && !IS_ERR(hdmi->audio)) platform_device_unregister(hdmi->audio); if (!IS_ERR(hdmi->cec))
@@ -3472,31 +3479,6 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi) else i2c_put_adapter(hdmi->ddc); }
-/* -------------------------------------------------------------
- Probe/remove API, used from platforms based on the DRM bridge
API.
- */
-struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
const struct dw_hdmi_plat_data*plat_data) -{
- struct dw_hdmi *hdmi;
- hdmi = __dw_hdmi_probe(pdev, plat_data);
- if (IS_ERR(hdmi))
return hdmi;- drm_bridge_add(&hdmi->bridge);
- return hdmi;
-} -EXPORT_SYMBOL_GPL(dw_hdmi_probe);
-void dw_hdmi_remove(struct dw_hdmi *hdmi) -{
- drm_bridge_remove(&hdmi->bridge);
- __dw_hdmi_remove(hdmi);
-} EXPORT_SYMBOL_GPL(dw_hdmi_remove);
/* -------------------------------------------------------------
@@ -3509,7 +3491,7 @@ struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev, struct dw_hdmi *hdmi; int ret;
- hdmi = __dw_hdmi_probe(pdev, plat_data);
- hdmi = dw_hdmi_probe(pdev, plat_data); if (IS_ERR(hdmi)) return hdmi;
@@ -3526,7 +3508,7 @@ EXPORT_SYMBOL_GPL(dw_hdmi_bind);
void dw_hdmi_unbind(struct dw_hdmi *hdmi) {
- __dw_hdmi_remove(hdmi);
- dw_hdmi_remove(hdmi);
} EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
-- 2.7.4
dri-devel mailing list dri-devel@lists.freedesktop.org
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...
On Thu, Jul 09, 2020 at 10:02:35AM +0800, Liu Ying wrote:
It's unnecessary to cleanup the i2c adapter and the ddc pointer in the bailout path of __dw_hdmi_probe(), since the adapter is not added and the ddc pointer is not set.
Fixes: a23d6265f033 (drm: bridge: dw-hdmi: Extract PHY interrupt setup to a function) Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Boris Brezillon boris.brezillon@collabora.com Cc: Jerome Brunet jbrunet@baylibre.com Cc: Cheng-Yi Chiang cychiang@chromium.org Cc: Dariusz Marcinkiewicz darekm@google.com Cc: Archit Taneja architt@codeaurora.org Cc: Jose Abreu joabreu@synopsys.com Cc: dri-devel@lists.freedesktop.org Cc: NXP Linux Team linux-imx@nxp.com Signed-off-by: Liu Ying victor.liu@nxp.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Thanks, applied to drm-misc-next.
Sam
v1->v2:
- Add Laurent's R-b tag.
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 6148a02..137b6eb 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -3441,11 +3441,6 @@ __dw_hdmi_probe(struct platform_device *pdev, return hdmi;
err_iahb:
- if (hdmi->i2c) {
i2c_del_adapter(&hdmi->i2c->adap);hdmi->ddc = NULL;- }
- clk_disable_unprepare(hdmi->iahb_clk); if (hdmi->cec_clk) clk_disable_unprepare(hdmi->cec_clk);
-- 2.7.4
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Liu Ying -
Sam Ravnborg