| Message ID | 20230318190804.234610-2-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | drm/bridge: Convert to platform remove callback returning void | expand |
Hi Uwe, Thank you for the patch. On Sat, Mar 18, 2023 at 08:07:46PM +0100, Uwe Kleine-König wrote: > Replace the generic error message issued by the driver core when the remove > callback returns non-zero ("remove callback returned a non-zero value. This > will be ignored.") by a message that tells the actual problem. > > Also simplify a bit by checking the return value of wait_event_timeout a > bit later. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > .../drm/bridge/cadence/cdns-mhdp8546-core.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > index f6822dfa3805..d74c6fa30ccc 100644 > --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > @@ -2574,7 +2574,6 @@ static int cdns_mhdp_remove(struct platform_device *pdev) > { > struct cdns_mhdp_device *mhdp = platform_get_drvdata(pdev); > unsigned long timeout = msecs_to_jiffies(100); > - bool stop_fw = false; > int ret; > > drm_bridge_remove(&mhdp->bridge); > @@ -2582,18 +2581,19 @@ static int cdns_mhdp_remove(struct platform_device *pdev) > ret = wait_event_timeout(mhdp->fw_load_wq, > mhdp->hw_state == MHDP_HW_READY, > timeout); > - if (ret == 0) > - dev_err(mhdp->dev, "%s: Timeout waiting for fw loading\n", > - __func__); > - else > - stop_fw = true; > - > spin_lock(&mhdp->start_lock); > mhdp->hw_state = MHDP_HW_STOPPED; > spin_unlock(&mhdp->start_lock); > > - if (stop_fw) > + if (ret == 0) { > + dev_err(mhdp->dev, "%s: Timeout waiting for fw loading\n", > + __func__); > + } else { > ret = cdns_mhdp_set_firmware_active(mhdp, false); > + if (ret) > + dev_err(mhdp->dev, "Failed to stop firmware (%pe)\n", > + ERR_PTR(ret)); Why not simply dev_err(mhdp->dev, "Failed to stop firmware (%d)\n", ret); ? Apart from that, Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > + } > > phy_exit(mhdp->phy); > > @@ -2609,7 +2609,7 @@ static int cdns_mhdp_remove(struct platform_device *pdev) > > clk_disable_unprepare(mhdp->clk); > > - return ret; > + return 0; > } > > static const struct of_device_id mhdp_ids[] = {
On Sun, Mar 19, 2023 at 03:13:01PM +0200, Laurent Pinchart wrote: > Hi Uwe, > > Thank you for the patch. > > On Sat, Mar 18, 2023 at 08:07:46PM +0100, Uwe Kleine-König wrote: > > Replace the generic error message issued by the driver core when the remove > > callback returns non-zero ("remove callback returned a non-zero value. This > > will be ignored.") by a message that tells the actual problem. > > > > Also simplify a bit by checking the return value of wait_event_timeout a > > bit later. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > .../drm/bridge/cadence/cdns-mhdp8546-core.c | 18 +++++++++--------- > > 1 file changed, 9 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > > index f6822dfa3805..d74c6fa30ccc 100644 > > --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > > @@ -2574,7 +2574,6 @@ static int cdns_mhdp_remove(struct platform_device *pdev) > > { > > struct cdns_mhdp_device *mhdp = platform_get_drvdata(pdev); > > unsigned long timeout = msecs_to_jiffies(100); > > - bool stop_fw = false; > > int ret; > > > > drm_bridge_remove(&mhdp->bridge); > > @@ -2582,18 +2581,19 @@ static int cdns_mhdp_remove(struct platform_device *pdev) > > ret = wait_event_timeout(mhdp->fw_load_wq, > > mhdp->hw_state == MHDP_HW_READY, > > timeout); > > - if (ret == 0) > > - dev_err(mhdp->dev, "%s: Timeout waiting for fw loading\n", > > - __func__); > > - else > > - stop_fw = true; > > - > > spin_lock(&mhdp->start_lock); > > mhdp->hw_state = MHDP_HW_STOPPED; > > spin_unlock(&mhdp->start_lock); > > > > - if (stop_fw) > > + if (ret == 0) { > > + dev_err(mhdp->dev, "%s: Timeout waiting for fw loading\n", > > + __func__); > > + } else { > > ret = cdns_mhdp_set_firmware_active(mhdp, false); > > + if (ret) > > + dev_err(mhdp->dev, "Failed to stop firmware (%pe)\n", > > + ERR_PTR(ret)); > > Why not simply > dev_err(mhdp->dev, "Failed to stop firmware (%d)\n", > ret); > > ? Apart from that, %pe is superior to %d because Failed to stop firmware (EIO) is easier to understand for humans than Failed to stop firmware (-5) . Don't you agree? Best regards Uwe
Hi Uwe, On Sun, Mar 19, 2023 at 02:59:21PM +0100, Uwe Kleine-König wrote: > On Sun, Mar 19, 2023 at 03:13:01PM +0200, Laurent Pinchart wrote: > > On Sat, Mar 18, 2023 at 08:07:46PM +0100, Uwe Kleine-König wrote: > > > Replace the generic error message issued by the driver core when the remove > > > callback returns non-zero ("remove callback returned a non-zero value. This > > > will be ignored.") by a message that tells the actual problem. > > > > > > Also simplify a bit by checking the return value of wait_event_timeout a > > > bit later. > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > --- > > > .../drm/bridge/cadence/cdns-mhdp8546-core.c | 18 +++++++++--------- > > > 1 file changed, 9 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > > > index f6822dfa3805..d74c6fa30ccc 100644 > > > --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > > > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > > > @@ -2574,7 +2574,6 @@ static int cdns_mhdp_remove(struct platform_device *pdev) > > > { > > > struct cdns_mhdp_device *mhdp = platform_get_drvdata(pdev); > > > unsigned long timeout = msecs_to_jiffies(100); > > > - bool stop_fw = false; > > > int ret; > > > > > > drm_bridge_remove(&mhdp->bridge); > > > @@ -2582,18 +2581,19 @@ static int cdns_mhdp_remove(struct platform_device *pdev) > > > ret = wait_event_timeout(mhdp->fw_load_wq, > > > mhdp->hw_state == MHDP_HW_READY, > > > timeout); > > > - if (ret == 0) > > > - dev_err(mhdp->dev, "%s: Timeout waiting for fw loading\n", > > > - __func__); > > > - else > > > - stop_fw = true; > > > - > > > spin_lock(&mhdp->start_lock); > > > mhdp->hw_state = MHDP_HW_STOPPED; > > > spin_unlock(&mhdp->start_lock); > > > > > > - if (stop_fw) > > > + if (ret == 0) { > > > + dev_err(mhdp->dev, "%s: Timeout waiting for fw loading\n", > > > + __func__); > > > + } else { > > > ret = cdns_mhdp_set_firmware_active(mhdp, false); > > > + if (ret) > > > + dev_err(mhdp->dev, "Failed to stop firmware (%pe)\n", > > > + ERR_PTR(ret)); > > > > Why not simply > > dev_err(mhdp->dev, "Failed to stop firmware (%d)\n", > > ret); > > > > ? Apart from that, > > %pe is superior to %d because > > Failed to stop firmware (EIO) > > is easier to understand for humans than > > Failed to stop firmware (-5) > > . Don't you agree? Partly :) When I try to match error codes received in userspace with kernel log messages, or debug core dumps, numerical errors are easier. At other times, the error name is likely better. I don't have a string preference here.
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c index f6822dfa3805..d74c6fa30ccc 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c @@ -2574,7 +2574,6 @@ static int cdns_mhdp_remove(struct platform_device *pdev) { struct cdns_mhdp_device *mhdp = platform_get_drvdata(pdev); unsigned long timeout = msecs_to_jiffies(100); - bool stop_fw = false; int ret; drm_bridge_remove(&mhdp->bridge); @@ -2582,18 +2581,19 @@ static int cdns_mhdp_remove(struct platform_device *pdev) ret = wait_event_timeout(mhdp->fw_load_wq, mhdp->hw_state == MHDP_HW_READY, timeout); - if (ret == 0) - dev_err(mhdp->dev, "%s: Timeout waiting for fw loading\n", - __func__); - else - stop_fw = true; - spin_lock(&mhdp->start_lock); mhdp->hw_state = MHDP_HW_STOPPED; spin_unlock(&mhdp->start_lock); - if (stop_fw) + if (ret == 0) { + dev_err(mhdp->dev, "%s: Timeout waiting for fw loading\n", + __func__); + } else { ret = cdns_mhdp_set_firmware_active(mhdp, false); + if (ret) + dev_err(mhdp->dev, "Failed to stop firmware (%pe)\n", + ERR_PTR(ret)); + } phy_exit(mhdp->phy); @@ -2609,7 +2609,7 @@ static int cdns_mhdp_remove(struct platform_device *pdev) clk_disable_unprepare(mhdp->clk); - return ret; + return 0; } static const struct of_device_id mhdp_ids[] = {
Replace the generic error message issued by the driver core when the remove callback returns non-zero ("remove callback returned a non-zero value. This will be ignored.") by a message that tells the actual problem. Also simplify a bit by checking the return value of wait_event_timeout a bit later. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- .../drm/bridge/cadence/cdns-mhdp8546-core.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)