On Mon, 2022-09-12 at 14:18 +0300, Ville Syrjala wrote: > From: Ville Syrjälä <[email protected]> > > Pull the eDP backlight setup ino its own function. No > reason to pollute intel_edp_init_connector() with all > the mundane details. > > Signed-off-by: Ville Syrjälä <[email protected]> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 51 +++++++++++++++---------- > 1 file changed, 30 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index a5eca5396fed..de5a4d2df78e 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -5217,6 +5217,35 @@ intel_edp_add_properties(struct intel_dp *intel_dp) > fixed_mode->vdisplay); > } > > +static void intel_edp_backlight_setup(struct intel_dp *intel_dp, > + struct intel_connector *connector) > +{ > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > + enum pipe pipe = INVALID_PIPE; > + > + if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
Isn't this too restrictive? Isn't there another way to check whether the hardware supports backlight? > + /* > + * Figure out the current pipe for the initial backlight setup. > + * If the current pipe isn't valid, try the PPS pipe, and if > that > + * fails just assume pipe A. > + */ > + pipe = vlv_active_pipe(intel_dp); > + > + if (pipe != PIPE_A && pipe != PIPE_B) > + pipe = intel_dp->pps.pps_pipe; > + > + if (pipe != PIPE_A && pipe != PIPE_B) > + pipe = PIPE_A; > + > + drm_dbg_kms(&i915->drm, > + "[CONNECTOR:%d:%s] using pipe %c for initial > backlight setup\n", > + connector->base.base.id, connector->base.name, > + pipe_name(pipe)); > + } > + > + intel_backlight_setup(connector, pipe); In most cases we will call intel_backlight_setup() with INVALID_PIPE. Wouldn't it be better to skip this call completely in that case? -- Cheers, Luca.
