Re: [DPU PATCH v3 3/5] drm/msm/dp: add displayPort driver support

[Matthias Kaehlcke <mka@chromium.org>]

On Mon, Dec 02, 2019 at 01:48:57PM +0000, Chandan Uddaraju wrote:
> Add the needed displayPort files to enable DP driver
> on msm target.
> 
> "dp_display" module is the main module that calls into
> other sub-modules. "dp_drm" file represents the interface
> between DRM framework and DP driver.
> 
> changes in v2:
> -- Update copyright markings on all relevant files.
> -- Change pr_err() to DRM_ERROR()
> -- Use APIs directly instead of function pointers.
> -- Use drm_display_mode structure to store link parameters in the driver.
> -- Use macros for register definitions instead of hardcoded values.
> -- Replace writel_relaxed/readl_relaxed with writel/readl
>    and remove memory barriers.
> -- Remove unnecessary NULL checks.
> -- Use drm helper functions for dpcd read/write.
> -- Use DRM_DEBUG_DP for debug msgs.
> 
> changes in V3:
> -- Removed changes in dpu_io_util.[ch]
> -- Added locking around "is_connected" flag and removed atomic_set()
> -- Removed the argument validation checks in all the static functions
>    except initialization functions and few API calls across msm/dp files
> -- Removed hardcoded values for register reads/writes
> -- Removed vreg related generic structures.
> -- Added return values where ever necessary.
> -- Updated dp_ctrl_on function.
> -- Calling the ctrl specific catalog functions directly instead of
>    function pointers.
> -- Added seperate change that adds standard value in drm_dp_helper file.
> -- Added separate change in this list that is used to initialize
>    displayport in DPU driver.
> -- Added change to use drm_dp_get_adjust_request_voltage() function.
> 
> Signed-off-by: Chandan Uddaraju <chandanu@codeaurora.org>
> ---
> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
>
> ...
>
> +int dp_power_init(struct dp_power *dp_power, bool flip)
> +{
> +	int rc = 0;
> +	struct dp_power_private *power;
> +
> +	if (!dp_power) {
> +		DRM_ERROR("invalid power data\n");
> +		rc = -EINVAL;
> +		goto exit;
> +	}

drive-by comment:

this would lead to calling 'pm_runtime_put_sync(&power->pdev->dev)'
below with 'power' being NULL, which doesn't seem a good idea.

It is probably sane to expect that 'dp_power' is not NULL, if that's
the case the check can be removed. Otherwise the function should just
return -EINVAL instead of jumping to 'exit'.

> +
> +	power = container_of(dp_power, struct dp_power_private, dp_power);
> +
> +	pm_runtime_get_sync(&power->pdev->dev);
> +	rc = dp_power_regulator_enable(power);
> +	if (rc) {
> +		DRM_ERROR("failed to enable regulators, %d\n", rc);
> +		goto exit;
> +	}
> +
> +	rc = dp_power_pinctrl_set(power, true);
> +	if (rc) {
> +		DRM_ERROR("failed to set pinctrl state, %d\n", rc);
> +		goto err_pinctrl;
> +	}
> +
> +	rc = dp_power_config_gpios(power, flip);
> +	if (rc) {
> +		DRM_ERROR("failed to enable gpios, %d\n", rc);
> +		goto err_gpio;
> +	}
> +
> +	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 err_clk;
> +	}
> +
> +	return 0;
> +
> +err_clk:
> +	dp_power_disable_gpios(power);
> +err_gpio:
> +	dp_power_pinctrl_set(power, false);
> +err_pinctrl:
> +	dp_power_regulator_disable(power);
> +exit:
> +	pm_runtime_put_sync(&power->pdev->dev);
> +	return rc;
> +}