diff mbox

[32/43] drm/i915: Clean up LVDS register handling

Message ID 1442595836-23981-33-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä Sept. 18, 2015, 5:03 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Keep single 'lvds_reg' and 'lvds' variable around in
intel_lvds_init(), and read it just once at the start.

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@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lvds.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

Comments

Jesse Barnes Oct. 12, 2015, 4:09 p.m. UTC | #1 
On 09/18/2015 10:03 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Keep single 'lvds_reg' and 'lvds' variable around in
> intel_lvds_init(), and read it just once at the start.
> 
> 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@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);
>  
> 

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Lukas Wunner Nov. 1, 2015, 3:33 p.m. UTC | #2 
Hi Ville,

On Fri, Sep 18, 2015 at 08:03:45PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@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;

(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@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Nov. 4, 2015, 4:59 p.m. UTC | #3 
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@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@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@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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

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);