diff mbox

[04/14] drm/i915: Clean up the .get_cdclk() assignment if ladder

Message ID 20170120182205.8141-5-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä Jan. 20, 2017, 6:21 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Let's clean up the mess we have in the if ladder that assigns the
.get_cdclk() hooks. The grouping of the platforms by the function
results in a thing that's not really legible, so let's do it the
other way around and order the if ladder by platform and duplicate
whatever assignments we need.

To further avoid confusion with the function names let's rename
them to just fixed_<freq>_get_cdclk(). The other option would
be to duplicate the functions entirely but it seems quite
pointless to do that since each one just returns a fixed value.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 17 deletions(-)

Comments

David Weinehall Jan. 24, 2017, 12:29 p.m. UTC | #1 
On Fri, Jan 20, 2017 at 08:21:55PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Let's clean up the mess we have in the if ladder that assigns the
> .get_cdclk() hooks. The grouping of the platforms by the function
> results in a thing that's not really legible, so let's do it the
> other way around and order the if ladder by platform and duplicate
> whatever assignments we need.
> 
> To further avoid confusion with the function names let's rename
> them to just fixed_<freq>_get_cdclk(). The other option would
> be to duplicate the functions entirely but it seems quite
> pointless to do that since each one just returns a fixed value.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bd0fdc04bc92..668c61ad45a6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7366,22 +7366,22 @@ static int valleyview_get_cdclk(struct drm_i915_private *dev_priv)
>  				      CCK_DISPLAY_CLOCK_CONTROL);
>  }
>  
> -static int ilk_get_cdclk(struct drm_i915_private *dev_priv)
> +static int fixed_450mhz_get_cdclk(struct drm_i915_private *dev_priv)
>  {
>  	return 450000;
>  }
>  
> -static int i945_get_cdclk(struct drm_i915_private *dev_priv)
> +static int fixed_400mhz_get_cdclk(struct drm_i915_private *dev_priv)
>  {
>  	return 400000;
>  }
>  
> -static int i915_get_cdclk(struct drm_i915_private *dev_priv)
> +static int fixed_333mhz_get_cdclk(struct drm_i915_private *dev_priv)
>  {
>  	return 333333;
>  }
>  
> -static int i9xx_misc_get_cdclk(struct drm_i915_private *dev_priv)
> +static int fixed_200mhz_get_cdclk(struct drm_i915_private *dev_priv)
>  {
>  	return 200000;
>  }
> @@ -7431,7 +7431,7 @@ static int i915gm_get_cdclk(struct drm_i915_private *dev_priv)
>  	}
>  }
>  
> -static int i865_get_cdclk(struct drm_i915_private *dev_priv)
> +static int fixed_266mhz_get_cdclk(struct drm_i915_private *dev_priv)
>  {
>  	return 266667;
>  }

How about reordering this and the one above to have the clocks in
strictly descending order?

> @@ -7474,7 +7474,7 @@ static int i85x_get_cdclk(struct drm_i915_private *dev_priv)
>  	return 0;
>  }

Similarly, move this one down.

> -static int i830_get_cdclk(struct drm_i915_private *dev_priv)
> +static int fixed_133mhz_get_cdclk(struct drm_i915_private *dev_priv)
>  {
>  	return 133333;
>  }
> @@ -16180,32 +16180,39 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
>  		dev_priv->display.get_cdclk = haswell_get_cdclk;
>  	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		dev_priv->display.get_cdclk = valleyview_get_cdclk;
> +	else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv))
> +		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
>  	else if (IS_GEN5(dev_priv))
> -		dev_priv->display.get_cdclk = ilk_get_cdclk;
> -	else if (IS_I945G(dev_priv) || IS_I965G(dev_priv) ||
> -		 IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv))
> -		dev_priv->display.get_cdclk = i945_get_cdclk;
> +		dev_priv->display.get_cdclk = fixed_450mhz_get_cdclk;
>  	else if (IS_GM45(dev_priv))
>  		dev_priv->display.get_cdclk = gm45_get_cdclk;
> +	else if (IS_G4X(dev_priv))
> +		dev_priv->display.get_cdclk = g33_get_cdclk;
>  	else if (IS_I965GM(dev_priv))
>  		dev_priv->display.get_cdclk = i965gm_get_cdclk;
> +	else if (IS_I965G(dev_priv))
> +		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
>  	else if (IS_PINEVIEW(dev_priv))
>  		dev_priv->display.get_cdclk = pnv_get_cdclk;
> -	else if (IS_G33(dev_priv) || IS_G4X(dev_priv))
> +	else if (IS_G33(dev_priv))
>  		dev_priv->display.get_cdclk = g33_get_cdclk;
> -	else if (IS_I915G(dev_priv))
> -		dev_priv->display.get_cdclk = i915_get_cdclk;
> -	else if (IS_I945GM(dev_priv) || IS_I845G(dev_priv))
> -		dev_priv->display.get_cdclk = i9xx_misc_get_cdclk;
> +	else if (IS_I945GM(dev_priv))
> +		dev_priv->display.get_cdclk = fixed_200mhz_get_cdclk;
> +	else if (IS_I945G(dev_priv))
> +		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
>  	else if (IS_I915GM(dev_priv))
>  		dev_priv->display.get_cdclk = i915gm_get_cdclk;
> +	else if (IS_I915G(dev_priv))
> +		dev_priv->display.get_cdclk = fixed_333mhz_get_cdclk;
>  	else if (IS_I865G(dev_priv))
> -		dev_priv->display.get_cdclk = i865_get_cdclk;
> +		dev_priv->display.get_cdclk = fixed_266mhz_get_cdclk;
>  	else if (IS_I85X(dev_priv))
>  		dev_priv->display.get_cdclk = i85x_get_cdclk;
> +	else  if (IS_I845G(dev_priv))
> +		dev_priv->display.get_cdclk = fixed_200mhz_get_cdclk;
>  	else { /* 830 */
>  		WARN(!IS_I830(dev_priv), "Unknown platform. Assuming 133 MHz CDCLK\n");
> -		dev_priv->display.get_cdclk = i830_get_cdclk;
> +		dev_priv->display.get_cdclk = fixed_133mhz_get_cdclk;
>  	}
>  
>  	if (IS_GEN5(dev_priv)) {
> -- 
> 2.10.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Jan. 25, 2017, 1:53 p.m. UTC | #2 
On Tue, Jan 24, 2017 at 02:29:00PM +0200, David Weinehall wrote:
> On Fri, Jan 20, 2017 at 08:21:55PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Let's clean up the mess we have in the if ladder that assigns the
> > .get_cdclk() hooks. The grouping of the platforms by the function
> > results in a thing that's not really legible, so let's do it the
> > other way around and order the if ladder by platform and duplicate
> > whatever assignments we need.
> > 
> > To further avoid confusion with the function names let's rename
> > them to just fixed_<freq>_get_cdclk(). The other option would
> > be to duplicate the functions entirely but it seems quite
> > pointless to do that since each one just returns a fixed value.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++---------------
> >  1 file changed, 24 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index bd0fdc04bc92..668c61ad45a6 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -7366,22 +7366,22 @@ static int valleyview_get_cdclk(struct drm_i915_private *dev_priv)
> >  				      CCK_DISPLAY_CLOCK_CONTROL);
> >  }
> >  
> > -static int ilk_get_cdclk(struct drm_i915_private *dev_priv)
> > +static int fixed_450mhz_get_cdclk(struct drm_i915_private *dev_priv)
> >  {
> >  	return 450000;
> >  }
> >  
> > -static int i945_get_cdclk(struct drm_i915_private *dev_priv)
> > +static int fixed_400mhz_get_cdclk(struct drm_i915_private *dev_priv)
> >  {
> >  	return 400000;
> >  }
> >  
> > -static int i915_get_cdclk(struct drm_i915_private *dev_priv)
> > +static int fixed_333mhz_get_cdclk(struct drm_i915_private *dev_priv)
> >  {
> >  	return 333333;
> >  }
> >  
> > -static int i9xx_misc_get_cdclk(struct drm_i915_private *dev_priv)
> > +static int fixed_200mhz_get_cdclk(struct drm_i915_private *dev_priv)
> >  {
> >  	return 200000;
> >  }
> > @@ -7431,7 +7431,7 @@ static int i915gm_get_cdclk(struct drm_i915_private *dev_priv)
> >  	}
> >  }
> >  
> > -static int i865_get_cdclk(struct drm_i915_private *dev_priv)
> > +static int fixed_266mhz_get_cdclk(struct drm_i915_private *dev_priv)
> >  {
> >  	return 266667;
> >  }
> 
> How about reordering this and the one above to have the clocks in
> strictly descending order?

It gets reordered in the patch that moves it all into a new file.

> 
> > @@ -7474,7 +7474,7 @@ static int i85x_get_cdclk(struct drm_i915_private *dev_priv)
> >  	return 0;
> >  }
> 
> Similarly, move this one down.
> 
> > -static int i830_get_cdclk(struct drm_i915_private *dev_priv)
> > +static int fixed_133mhz_get_cdclk(struct drm_i915_private *dev_priv)
> >  {
> >  	return 133333;
> >  }
> > @@ -16180,32 +16180,39 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
> >  		dev_priv->display.get_cdclk = haswell_get_cdclk;
> >  	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >  		dev_priv->display.get_cdclk = valleyview_get_cdclk;
> > +	else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv))
> > +		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
> >  	else if (IS_GEN5(dev_priv))
> > -		dev_priv->display.get_cdclk = ilk_get_cdclk;
> > -	else if (IS_I945G(dev_priv) || IS_I965G(dev_priv) ||
> > -		 IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv))
> > -		dev_priv->display.get_cdclk = i945_get_cdclk;
> > +		dev_priv->display.get_cdclk = fixed_450mhz_get_cdclk;
> >  	else if (IS_GM45(dev_priv))
> >  		dev_priv->display.get_cdclk = gm45_get_cdclk;
> > +	else if (IS_G4X(dev_priv))
> > +		dev_priv->display.get_cdclk = g33_get_cdclk;
> >  	else if (IS_I965GM(dev_priv))
> >  		dev_priv->display.get_cdclk = i965gm_get_cdclk;
> > +	else if (IS_I965G(dev_priv))
> > +		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
> >  	else if (IS_PINEVIEW(dev_priv))
> >  		dev_priv->display.get_cdclk = pnv_get_cdclk;
> > -	else if (IS_G33(dev_priv) || IS_G4X(dev_priv))
> > +	else if (IS_G33(dev_priv))
> >  		dev_priv->display.get_cdclk = g33_get_cdclk;
> > -	else if (IS_I915G(dev_priv))
> > -		dev_priv->display.get_cdclk = i915_get_cdclk;
> > -	else if (IS_I945GM(dev_priv) || IS_I845G(dev_priv))
> > -		dev_priv->display.get_cdclk = i9xx_misc_get_cdclk;
> > +	else if (IS_I945GM(dev_priv))
> > +		dev_priv->display.get_cdclk = fixed_200mhz_get_cdclk;
> > +	else if (IS_I945G(dev_priv))
> > +		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
> >  	else if (IS_I915GM(dev_priv))
> >  		dev_priv->display.get_cdclk = i915gm_get_cdclk;
> > +	else if (IS_I915G(dev_priv))
> > +		dev_priv->display.get_cdclk = fixed_333mhz_get_cdclk;
> >  	else if (IS_I865G(dev_priv))
> > -		dev_priv->display.get_cdclk = i865_get_cdclk;
> > +		dev_priv->display.get_cdclk = fixed_266mhz_get_cdclk;
> >  	else if (IS_I85X(dev_priv))
> >  		dev_priv->display.get_cdclk = i85x_get_cdclk;
> > +	else  if (IS_I845G(dev_priv))
> > +		dev_priv->display.get_cdclk = fixed_200mhz_get_cdclk;
> >  	else { /* 830 */
> >  		WARN(!IS_I830(dev_priv), "Unknown platform. Assuming 133 MHz CDCLK\n");
> > -		dev_priv->display.get_cdclk = i830_get_cdclk;
> > +		dev_priv->display.get_cdclk = fixed_133mhz_get_cdclk;
> >  	}
> >  
> >  	if (IS_GEN5(dev_priv)) {
> > -- 
> > 2.10.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bd0fdc04bc92..668c61ad45a6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7366,22 +7366,22 @@  static int valleyview_get_cdclk(struct drm_i915_private *dev_priv)
 				      CCK_DISPLAY_CLOCK_CONTROL);
 }
 
-static int ilk_get_cdclk(struct drm_i915_private *dev_priv)
+static int fixed_450mhz_get_cdclk(struct drm_i915_private *dev_priv)
 {
 	return 450000;
 }
 
-static int i945_get_cdclk(struct drm_i915_private *dev_priv)
+static int fixed_400mhz_get_cdclk(struct drm_i915_private *dev_priv)
 {
 	return 400000;
 }
 
-static int i915_get_cdclk(struct drm_i915_private *dev_priv)
+static int fixed_333mhz_get_cdclk(struct drm_i915_private *dev_priv)
 {
 	return 333333;
 }
 
-static int i9xx_misc_get_cdclk(struct drm_i915_private *dev_priv)
+static int fixed_200mhz_get_cdclk(struct drm_i915_private *dev_priv)
 {
 	return 200000;
 }
@@ -7431,7 +7431,7 @@  static int i915gm_get_cdclk(struct drm_i915_private *dev_priv)
 	}
 }
 
-static int i865_get_cdclk(struct drm_i915_private *dev_priv)
+static int fixed_266mhz_get_cdclk(struct drm_i915_private *dev_priv)
 {
 	return 266667;
 }
@@ -7474,7 +7474,7 @@  static int i85x_get_cdclk(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
-static int i830_get_cdclk(struct drm_i915_private *dev_priv)
+static int fixed_133mhz_get_cdclk(struct drm_i915_private *dev_priv)
 {
 	return 133333;
 }
@@ -16180,32 +16180,39 @@  void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 		dev_priv->display.get_cdclk = haswell_get_cdclk;
 	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		dev_priv->display.get_cdclk = valleyview_get_cdclk;
+	else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv))
+		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
 	else if (IS_GEN5(dev_priv))
-		dev_priv->display.get_cdclk = ilk_get_cdclk;
-	else if (IS_I945G(dev_priv) || IS_I965G(dev_priv) ||
-		 IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv))
-		dev_priv->display.get_cdclk = i945_get_cdclk;
+		dev_priv->display.get_cdclk = fixed_450mhz_get_cdclk;
 	else if (IS_GM45(dev_priv))
 		dev_priv->display.get_cdclk = gm45_get_cdclk;
+	else if (IS_G4X(dev_priv))
+		dev_priv->display.get_cdclk = g33_get_cdclk;
 	else if (IS_I965GM(dev_priv))
 		dev_priv->display.get_cdclk = i965gm_get_cdclk;
+	else if (IS_I965G(dev_priv))
+		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
 	else if (IS_PINEVIEW(dev_priv))
 		dev_priv->display.get_cdclk = pnv_get_cdclk;
-	else if (IS_G33(dev_priv) || IS_G4X(dev_priv))
+	else if (IS_G33(dev_priv))
 		dev_priv->display.get_cdclk = g33_get_cdclk;
-	else if (IS_I915G(dev_priv))
-		dev_priv->display.get_cdclk = i915_get_cdclk;
-	else if (IS_I945GM(dev_priv) || IS_I845G(dev_priv))
-		dev_priv->display.get_cdclk = i9xx_misc_get_cdclk;
+	else if (IS_I945GM(dev_priv))
+		dev_priv->display.get_cdclk = fixed_200mhz_get_cdclk;
+	else if (IS_I945G(dev_priv))
+		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
 	else if (IS_I915GM(dev_priv))
 		dev_priv->display.get_cdclk = i915gm_get_cdclk;
+	else if (IS_I915G(dev_priv))
+		dev_priv->display.get_cdclk = fixed_333mhz_get_cdclk;
 	else if (IS_I865G(dev_priv))
-		dev_priv->display.get_cdclk = i865_get_cdclk;
+		dev_priv->display.get_cdclk = fixed_266mhz_get_cdclk;
 	else if (IS_I85X(dev_priv))
 		dev_priv->display.get_cdclk = i85x_get_cdclk;
+	else  if (IS_I845G(dev_priv))
+		dev_priv->display.get_cdclk = fixed_200mhz_get_cdclk;
 	else { /* 830 */
 		WARN(!IS_I830(dev_priv), "Unknown platform. Assuming 133 MHz CDCLK\n");
-		dev_priv->display.get_cdclk = i830_get_cdclk;
+		dev_priv->display.get_cdclk = fixed_133mhz_get_cdclk;
 	}
 
 	if (IS_GEN5(dev_priv)) {