> -----Original Message-----
> From: Nikula, Jani <[email protected]>
> Sent: Wednesday, September 30, 2026 1:35 PM
> To: Manna, Animesh <[email protected]>; intel-
> [email protected]; [email protected]; dri-
> [email protected]
> Cc: Hogander, Jouni <[email protected]>; Kandpal, Suraj
> <[email protected]>; Manna, Animesh <[email protected]>
> Subject: Re: [PATCH v6 02/18] drm/i915/alpm: Move alpm sink capability
> readout into a separate function
>
> On Tue, 15 Sep 2026, Animesh Manna <[email protected]> wrote:
> > Abstract ALPM DPCD initialization into its own function.
> >
> > v2:
> > - Improve commit description. [Suraj, Jouni]
> >
> > Cc: Jouni Högander <[email protected]>
> > Signed-off-by: Animesh Manna <[email protected]>
> > ---
> > 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?!
false indicates that it is not correctly initialized.
Got your concern, will try to keep return value as int.
Regards,
Animesh
>
>
> 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
