Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3a5d2a34-bdc3-ac64-f69a-c7d99ff2f275@linaro.org>
Date:   Sat, 20 May 2023 04:20:20 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH 2/3] drm/msm/dp: Clean up logs dp_power module
Content-Language: en-GB
To:     Bjorn Andersson <quic_bjorande@quicinc.com>,
        Rob Clark <robdclark@gmail.com>,
        Abhinav Kumar <quic_abhinavk@quicinc.com>,
        Kuogee Hsieh <quic_khsieh@quicinc.com>
Cc:     Johan Hovold <johan+linaro@kernel.org>,
        Vinod Polimera <quic_vpolimer@quicinc.com>,
        Sankeerth Billakanti <quic_sbillaka@quicinc.com>,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org
References: <20230515030256.300104-1-quic_bjorande@quicinc.com>
 <20230515030256.300104-2-quic_bjorande@quicinc.com>
From:   Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
In-Reply-To: <20230515030256.300104-2-quic_bjorande@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 15/05/2023 06:02, Bjorn Andersson wrote:
> The clk_bulk API already provides error messages indicating which
> specific clock in the request for which the operation failed, further
> more these errors are associated with the specific DisplayPort
> controller (rather than the shared drm_device). The additional error
> messages int he dp_power module does thereby not provide any benefit.
> 
> While at it, none of the dp_power handles passed to these functions are
> dynamic in nature, so there should not be any need for runtime checking
> them. Drop these as well.

It might have been slightly better to have this split into two patches, 
but as this is a debug/logs only, it's fine with me.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_power.c | 62 +++++--------------------------
>   1 file changed, 9 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
> index c0aaabb03389..031d2eefef07 100644
> --- a/drivers/gpu/drm/msm/dp/dp_power.c
> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> @@ -35,25 +35,16 @@ static int dp_power_clk_init(struct dp_power_private *power)
>   	stream = &power->parser->mp[DP_STREAM_PM];
>   
>   	rc = devm_clk_bulk_get(dev, core->num_clk, core->clocks);
> -	if (rc) {
> -		DRM_ERROR("failed to get %s clk. err=%d\n",
> -			dp_parser_pm_name(DP_CORE_PM), rc);
> +	if (rc)
>   		return rc;
> -	}
>   
>   	rc = devm_clk_bulk_get(dev, ctrl->num_clk, ctrl->clocks);
> -	if (rc) {
> -		DRM_ERROR("failed to get %s clk. err=%d\n",
> -			dp_parser_pm_name(DP_CTRL_PM), rc);
> +	if (rc)
>   		return -ENODEV;
> -	}
>   
>   	rc = devm_clk_bulk_get(dev, stream->num_clk, stream->clocks);
> -	if (rc) {
> -		DRM_ERROR("failed to get %s clk. err=%d\n",
> -			dp_parser_pm_name(DP_CTRL_PM), rc);
> +	if (rc)
>   		return -ENODEV;
> -	}
>   
>   	return 0;
>   }
> @@ -121,11 +112,9 @@ int dp_power_clk_enable(struct dp_power *dp_power,
>   			mp = &power->parser->mp[DP_CORE_PM];
>   
>   			rc = clk_bulk_prepare_enable(mp->num_clk, mp->clocks);
> -			if (rc) {
> -				DRM_ERROR("fail to enable clks: %s. err=%d\n",
> -					dp_parser_pm_name(DP_CORE_PM), rc);
> +			if (rc)
>   				return rc;
> -			}
> +
>   			dp_power->core_clks_on = true;
>   		}
>   	}
> @@ -133,10 +122,8 @@ int dp_power_clk_enable(struct dp_power *dp_power,
>   	mp = &power->parser->mp[pm_type];
>   	if (enable) {
>   		rc = clk_bulk_prepare_enable(mp->num_clk, mp->clocks);
> -		if (rc) {
> -			DRM_ERROR("failed to enable clks, err: %d\n", rc);
> +		if (rc)
>   			return rc;
> -		}
>   	} else {
>   		clk_bulk_disable_unprepare(mp->num_clk, mp->clocks);
>   	}
> @@ -162,34 +149,19 @@ int dp_power_clk_enable(struct dp_power *dp_power,
>   
>   int dp_power_client_init(struct dp_power *dp_power)
>   {
> -	int rc = 0;
>   	struct dp_power_private *power;
>   
> -	if (!dp_power) {
> -		DRM_ERROR("invalid power data\n");
> -		return -EINVAL;
> -	}
> -
>   	power = container_of(dp_power, struct dp_power_private, dp_power);
>   
>   	pm_runtime_enable(&power->pdev->dev);
>   
> -	rc = dp_power_clk_init(power);
> -	if (rc)
> -		DRM_ERROR("failed to init clocks %d\n", rc);
> -
> -	return rc;
> +	return dp_power_clk_init(power);
>   }
>   
>   void dp_power_client_deinit(struct dp_power *dp_power)
>   {
>   	struct dp_power_private *power;
>   
> -	if (!dp_power) {
> -		DRM_ERROR("invalid power data\n");
> -		return;
> -	}
> -
>   	power = container_of(dp_power, struct dp_power_private, dp_power);
>   
>   	pm_runtime_disable(&power->pdev->dev);
> @@ -200,25 +172,14 @@ int dp_power_init(struct dp_power *dp_power, bool flip)
>   	int rc = 0;
>   	struct dp_power_private *power = NULL;
>   
> -	if (!dp_power) {
> -		DRM_ERROR("invalid power data\n");
> -		return -EINVAL;
> -	}
> -
>   	power = container_of(dp_power, struct dp_power_private, dp_power);
>   
>   	pm_runtime_get_sync(&power->pdev->dev);
>   
>   	rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
> -	if (rc) {
> -		DRM_ERROR("failed to enable DP core clocks, %d\n", rc);
> -		goto exit;
> -	}
> -
> -	return 0;
> +	if (rc)
> +		pm_runtime_put_sync(&power->pdev->dev);
>   
> -exit:
> -	pm_runtime_put_sync(&power->pdev->dev);
>   	return rc;
>   }
>   
> @@ -238,11 +199,6 @@ struct dp_power *dp_power_get(struct device *dev, struct dp_parser *parser)
>   	struct dp_power_private *power;
>   	struct dp_power *dp_power;
>   
> -	if (!parser) {
> -		DRM_ERROR("invalid input\n");
> -		return ERR_PTR(-EINVAL);
> -	}
> -
>   	power = devm_kzalloc(&parser->pdev->dev, sizeof(*power), GFP_KERNEL);
>   	if (!power)
>   		return ERR_PTR(-ENOMEM);

-- 
With best wishes
Dmitry