Re: [PATCH 01/19] drm/bridge: cdns-mhdp8546: Improve error reporting in remove callback

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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[] = {

-- 
Regards,

Laurent Pinchart