[Intel-gfx] [PATCH 32/43] drm/i915: Clean up LVDS register handling

[Ville Syrjälä]

On Sun, Nov 01, 2015 at 04:33:57PM +0100, Lukas Wunner wrote:
> Hi Ville,
> 
> On Fri, Sep 18, 2015 at 08:03:45PM +0300, ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > 
> > Keep single 'lvds_reg' and 'lvds' variable around in
> > intel_lvds_init(), and read it just once at the start.
> 
> Hm, is it intentional that you didn't also replace this register readout
> at the end of the function?
> 
> 	lvds_encoder->a3_power = I915_READ(lvds_encoder->reg) &
> 				 LVDS_A3_POWER_MASK;

Simply didn't notice that bit.

> 
> (Sorry, I only noticed this today, post-merge, while rebasing my LVDS
> reprobing stuff.)
> 
> Best regards,
> 
> Lukas
> 
> > 
> > Also intel_lvds_get_config() doesn't need to figure out which reg to use
> > since it can just consult lvds_encoder->reg.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_lvds.c | 30 ++++++++++++++----------------
> >  1 file changed, 14 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index 2c2d1f0..35bad71 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -98,15 +98,11 @@ static void intel_lvds_get_config(struct intel_encoder *encoder,
> >  {
> >  	struct drm_device *dev = encoder->base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	u32 lvds_reg, tmp, flags = 0;
> > +	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
> > +	u32 tmp, flags = 0;
> >  	int dotclock;
> >  
> > -	if (HAS_PCH_SPLIT(dev))
> > -		lvds_reg = PCH_LVDS;
> > -	else
> > -		lvds_reg = LVDS;
> > -
> > -	tmp = I915_READ(lvds_reg);
> > +	tmp = I915_READ(lvds_encoder->reg);
> >  	if (tmp & LVDS_HSYNC_POLARITY)
> >  		flags |= DRM_MODE_FLAG_NHSYNC;
> >  	else
> > @@ -944,6 +940,7 @@ void intel_lvds_init(struct drm_device *dev)
> >  	struct drm_display_mode *downclock_mode = NULL;
> >  	struct edid *edid;
> >  	struct drm_crtc *crtc;
> > +	u32 lvds_reg;
> >  	u32 lvds;
> >  	int pipe;
> >  	u8 pin;
> > @@ -966,8 +963,15 @@ void intel_lvds_init(struct drm_device *dev)
> >  	if (dmi_check_system(intel_no_lvds))
> >  		return;
> >  
> > +	if (HAS_PCH_SPLIT(dev))
> > +		lvds_reg = PCH_LVDS;
> > +	else
> > +		lvds_reg = LVDS;
> > +
> > +	lvds = I915_READ(lvds_reg);
> > +
> >  	if (HAS_PCH_SPLIT(dev)) {
> > -		if ((I915_READ(PCH_LVDS) & LVDS_DETECTED) == 0)
> > +		if ((lvds & LVDS_DETECTED) == 0)
> >  			return;
> >  		if (dev_priv->vbt.edp_support) {
> >  			DRM_DEBUG_KMS("disable LVDS for eDP support\n");
> > @@ -977,8 +981,7 @@ void intel_lvds_init(struct drm_device *dev)
> >  
> >  	pin = GMBUS_PIN_PANEL;
> >  	if (!lvds_is_present_in_vbt(dev, &pin)) {
> > -		u32 reg = HAS_PCH_SPLIT(dev) ? PCH_LVDS : LVDS;
> > -		if ((I915_READ(reg) & LVDS_PORT_EN) == 0) {
> > +		if ((lvds & LVDS_PORT_EN) == 0) {
> >  			DRM_DEBUG_KMS("LVDS is not present in VBT\n");
> >  			return;
> >  		}
> > @@ -1055,11 +1058,7 @@ void intel_lvds_init(struct drm_device *dev)
> >  	connector->interlace_allowed = false;
> >  	connector->doublescan_allowed = false;
> >  
> > -	if (HAS_PCH_SPLIT(dev)) {
> > -		lvds_encoder->reg = PCH_LVDS;
> > -	} else {
> > -		lvds_encoder->reg = LVDS;
> > -	}
> > +	lvds_encoder->reg = lvds_reg;
> >  
> >  	/* create the scaling mode property */
> >  	drm_mode_create_scaling_mode_property(dev);
> > @@ -1140,7 +1139,6 @@ void intel_lvds_init(struct drm_device *dev)
> >  	if (HAS_PCH_SPLIT(dev))
> >  		goto failed;
> >  
> > -	lvds = I915_READ(LVDS);
> >  	pipe = (lvds & LVDS_PIPEB_SELECT) ? 1 : 0;
> >  	crtc = intel_get_crtc_for_pipe(dev, pipe);
> >  
> > -- 
> > 2.4.6
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC