Re: [PATCH v6 02/18] drm/i915/alpm: Move alpm sink capability readout into a separate function

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 15 Sep 2026, Animesh Manna <animesh.manna@intel.com> wrote:
> Abstract ALPM DPCD initialization into its own function.
>
> v2:
> - Improve commit description. [Suraj, Jouni]
>
> Cc: Jouni Högander <jouni.hogander@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_alpm.c | 11 +++++++++++
>  drivers/gpu/drm/i915/display/intel_alpm.h |  1 +
>  drivers/gpu/drm/i915/display/intel_dp.c   |  6 +-----
>  3 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 10943539bc7c..a6743fe62d48 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -43,6 +43,17 @@ bool intel_alpm_is_alpm_aux_less(struct intel_dp *intel_dp,
>  		(crtc_state->has_lobf && intel_alpm_aux_less_wake_supported(intel_dp));
>  }
>  
> +bool intel_alpm_init_dpcd(struct intel_dp *intel_dp)
> +{
> +	u8 dpcd;
> +
> +	if (drm_dp_dpcd_read_byte(&intel_dp->aux, DP_RECEIVER_ALPM_CAP, &dpcd) < 0)
> +		return false;
> +
> +	intel_dp->alpm_dpcd = dpcd;
> +	return true;
> +}

Why not propagate the original error instead of flattening to a boolean?
What does boolean true/false mean for a function that's not a predicate
function but "init"?

From a maintenance perspective, the mixing of bool vs. int return values
is a problem because you never know what is being checked for in the
caller side:

	if (!intel_alpm_init_dpcd())

Does that return bool and false means failure? Or does that return int
and false means success?!


BR,
Jani.


> +
>  void intel_alpm_init(struct intel_dp *intel_dp)
>  {
>  	mutex_init(&intel_dp->alpm.lock);
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h b/drivers/gpu/drm/i915/display/intel_alpm.h
> index f8f605d94f96..56c3e1482e37 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.h
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.h
> @@ -15,6 +15,7 @@ struct intel_connector;
>  struct intel_atomic_state;
>  struct intel_crtc;
>  
> +bool intel_alpm_init_dpcd(struct intel_dp *intel_dp);
>  void intel_alpm_init(struct intel_dp *intel_dp);
>  bool intel_alpm_compute_params(struct intel_dp *intel_dp,
>  			       struct intel_crtc_state *crtc_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 0cd5e6b5034c..650c8b39270b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4783,7 +4783,6 @@ static bool
>  intel_edp_init_dpcd(struct intel_dp *intel_dp, struct intel_connector *connector)
>  {
>  	struct intel_display *display = to_intel_display(intel_dp);
> -	int ret;
>  	u8 dprx;
>  
>  	/* this function is meant to be called only once */
> @@ -4829,10 +4828,7 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp, struct intel_connector *connector
>  	 */
>  	intel_dp_init_source_oui(intel_dp);
>  
> -	/* Read the ALPM DPCD caps */
> -	ret = drm_dp_dpcd_read_byte(&intel_dp->aux, DP_RECEIVER_ALPM_CAP,
> -				    &intel_dp->alpm_dpcd);
> -	if (ret < 0)
> +	if (!intel_alpm_init_dpcd(intel_dp))
>  		return false;
>  
>  	/*

-- 
Jani Nikula, Intel