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

[Matthias Kaehlcke <mka@chromium.org>]

On Thu, Feb 27, 2020 at 01:54:33PM -0800, Matthias Kaehlcke wrote:
> 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.

correction: with 'power' being uninitialized, which isn't a good idea
either.

> 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;
> > +}