Re: [Intel-gfx] [PATCH 10/13] drm/i915/display: clean up intel_PLL_is_valid()

2020-03-25 Thread Jani Nikula 
On Wed, 25 Mar 2020, Daniel Vetter  wrote:
> On Fri, Mar 20, 2020 at 04:36:35PM +0200, Jani Nikula wrote:
>> Drop useless macro hiding the return. Fix superfluous whitespace. Rename
>> function to all lowercase.
>> 
>> Signed-off-by: Jani Nikula 
>
> We do lose the debug output, but then I don't think we'll do much bug
> hunting in here anytime soon, it's all fairly old gmch-style display
> stuff. Also just realized I'm still pining for an intel_gmch_pll.c ...

We don't lose the output, because the DRM_DEBUG(s) is wrapped in
comments... I didn't see the value in having these, as it's part of the
normal flow.

BR,
Jani.


>
> Reviewed-by: Daniel Vetter 
>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display.c | 40 ++--
>>  1 file changed, 19 insertions(+), 21 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
>> b/drivers/gpu/drm/i915/display/intel_display.c
>> index 37bd7ce88ecd..6af8d43ceb0c 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -620,45 +620,43 @@ int chv_calc_dpll_params(int refclk, struct dpll 
>> *clock)
>>  return clock->dot / 5;
>>  }
>>  
>> -#define INTELPllInvalid(s)   do { /* DRM_DEBUG(s); */ return false; } while 
>> (0)
>> -
>>  /*
>>   * Returns whether the given set of divisors are valid for a given refclk 
>> with
>>   * the given connectors.
>>   */
>> -static bool intel_PLL_is_valid(struct drm_i915_private *dev_priv,
>> +static bool intel_pll_is_valid(struct drm_i915_private *dev_priv,
>> const struct intel_limit *limit,
>> const struct dpll *clock)
>>  {
>> -if (clock->n   < limit->n.min   || limit->n.max   < clock->n)
>> -INTELPllInvalid("n out of range\n");
>> -if (clock->p1  < limit->p1.min  || limit->p1.max  < clock->p1)
>> -INTELPllInvalid("p1 out of range\n");
>> -if (clock->m2  < limit->m2.min  || limit->m2.max  < clock->m2)
>> -INTELPllInvalid("m2 out of range\n");
>> -if (clock->m1  < limit->m1.min  || limit->m1.max  < clock->m1)
>> -INTELPllInvalid("m1 out of range\n");
>> +if (clock->n < limit->n.min || limit->n.max < clock->n)
>> +return false;
>> +if (clock->p1 < limit->p1.min || limit->p1.max < clock->p1)
>> +return false;
>> +if (clock->m2 < limit->m2.min || limit->m2.max < clock->m2)
>> +return false;
>> +if (clock->m1 < limit->m1.min || limit->m1.max < clock->m1)
>> +return false;
>>  
>>  if (!IS_PINEVIEW(dev_priv) && !IS_VALLEYVIEW(dev_priv) &&
>>  !IS_CHERRYVIEW(dev_priv) && !IS_GEN9_LP(dev_priv))
>>  if (clock->m1 <= clock->m2)
>> -INTELPllInvalid("m1 <= m2\n");
>> +return false;
>>  
>>  if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv) &&
>>  !IS_GEN9_LP(dev_priv)) {
>>  if (clock->p < limit->p.min || limit->p.max < clock->p)
>> -INTELPllInvalid("p out of range\n");
>> +return false;
>>  if (clock->m < limit->m.min || limit->m.max < clock->m)
>> -INTELPllInvalid("m out of range\n");
>> +return false;
>>  }
>>  
>>  if (clock->vco < limit->vco.min || limit->vco.max < clock->vco)
>> -INTELPllInvalid("vco out of range\n");
>> +return false;
>>  /* XXX: We may need to be checking "Dot clock" depending on the 
>> multiplier,
>>   * connector, etc., rather than just a single range.
>>   */
>>  if (clock->dot < limit->dot.min || limit->dot.max < clock->dot)
>> -INTELPllInvalid("dot out of range\n");
>> +return false;
>>  
>>  return true;
>>  }
>> @@ -725,7 +723,7 @@ i9xx_find_best_dpll(const struct intel_limit *limit,
>>  int this_err;
>>  
>>  i9xx_calc_dpll_params(refclk, );
>> -if (!intel_PLL_is_valid(to_i915(dev),
>> +if (!intel_pll_is_valid(to_i915(dev),
>>  limit,
>>  ))
>>  continue;
>> @@ -781,7 +779,7 @@ pnv_find_best_dpll(const struct intel_limit *limit,
>>  int this_err;
>>  
>>  pnv_calc_dpll_params(refclk, );
>> -if (!intel_PLL_is_valid(to_i915(dev),
>> +if (!intel_pll_is_valid(to_i915(dev),
>>  limit,
>>  ))
>>  continue;
>> @@ -842,7 +840,7 @@ g4x_find_best_dpll(const struct

Re: [Intel-gfx] [PATCH 10/13] drm/i915/display: clean up intel_PLL_is_valid()

2020-03-25 Thread Daniel Vetter 
On Fri, Mar 20, 2020 at 04:36:35PM +0200, Jani Nikula wrote:
> Drop useless macro hiding the return. Fix superfluous whitespace. Rename
> function to all lowercase.
> 
> Signed-off-by: Jani Nikula 

We do lose the debug output, but then I don't think we'll do much bug
hunting in here anytime soon, it's all fairly old gmch-style display
stuff. Also just realized I'm still pining for an intel_gmch_pll.c ...

Reviewed-by: Daniel Vetter 

> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 40 ++--
>  1 file changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 37bd7ce88ecd..6af8d43ceb0c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -620,45 +620,43 @@ int chv_calc_dpll_params(int refclk, struct dpll *clock)
>   return clock->dot / 5;
>  }
>  
> -#define INTELPllInvalid(s)   do { /* DRM_DEBUG(s); */ return false; } while 
> (0)
> -
>  /*
>   * Returns whether the given set of divisors are valid for a given refclk 
> with
>   * the given connectors.
>   */
> -static bool intel_PLL_is_valid(struct drm_i915_private *dev_priv,
> +static bool intel_pll_is_valid(struct drm_i915_private *dev_priv,
>  const struct intel_limit *limit,
>  const struct dpll *clock)
>  {
> - if (clock->n   < limit->n.min   || limit->n.max   < clock->n)
> - INTELPllInvalid("n out of range\n");
> - if (clock->p1  < limit->p1.min  || limit->p1.max  < clock->p1)
> - INTELPllInvalid("p1 out of range\n");
> - if (clock->m2  < limit->m2.min  || limit->m2.max  < clock->m2)
> - INTELPllInvalid("m2 out of range\n");
> - if (clock->m1  < limit->m1.min  || limit->m1.max  < clock->m1)
> - INTELPllInvalid("m1 out of range\n");
> + if (clock->n < limit->n.min || limit->n.max < clock->n)
> + return false;
> + if (clock->p1 < limit->p1.min || limit->p1.max < clock->p1)
> + return false;
> + if (clock->m2 < limit->m2.min || limit->m2.max < clock->m2)
> + return false;
> + if (clock->m1 < limit->m1.min || limit->m1.max < clock->m1)
> + return false;
>  
>   if (!IS_PINEVIEW(dev_priv) && !IS_VALLEYVIEW(dev_priv) &&
>   !IS_CHERRYVIEW(dev_priv) && !IS_GEN9_LP(dev_priv))
>   if (clock->m1 <= clock->m2)
> - INTELPllInvalid("m1 <= m2\n");
> + return false;
>  
>   if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv) &&
>   !IS_GEN9_LP(dev_priv)) {
>   if (clock->p < limit->p.min || limit->p.max < clock->p)
> - INTELPllInvalid("p out of range\n");
> + return false;
>   if (clock->m < limit->m.min || limit->m.max < clock->m)
> - INTELPllInvalid("m out of range\n");
> + return false;
>   }
>  
>   if (clock->vco < limit->vco.min || limit->vco.max < clock->vco)
> - INTELPllInvalid("vco out of range\n");
> + return false;
>   /* XXX: We may need to be checking "Dot clock" depending on the 
> multiplier,
>* connector, etc., rather than just a single range.
>*/
>   if (clock->dot < limit->dot.min || limit->dot.max < clock->dot)
> - INTELPllInvalid("dot out of range\n");
> + return false;
>  
>   return true;
>  }
> @@ -725,7 +723,7 @@ i9xx_find_best_dpll(const struct intel_limit *limit,
>   int this_err;
>  
>   i9xx_calc_dpll_params(refclk, );
> - if (!intel_PLL_is_valid(to_i915(dev),
> + if (!intel_pll_is_valid(to_i915(dev),
>   limit,
>   ))
>   continue;
> @@ -781,7 +779,7 @@ pnv_find_best_dpll(const struct intel_limit *limit,
>   int this_err;
>  
>   pnv_calc_dpll_params(refclk, );
> - if (!intel_PLL_is_valid(to_i915(dev),
> + if (!intel_pll_is_valid(to_i915(dev),
>   limit,
>   ))
>   continue;
> @@ -842,7 +840,7 @@ g4x_find_best_dpll(const struct intel_limit *limit,
>   int this_err;
>  
>   i9xx_calc_dpll_params(refclk, );
> - if (!intel_PLL_is_valid(to_i915(dev),
> + if

[Intel-gfx] [PATCH 10/13] drm/i915/display: clean up intel_PLL_is_valid()

2020-03-20 Thread Jani Nikula 
Drop useless macro hiding the return. Fix superfluous whitespace. Rename
function to all lowercase.

Signed-off-by: Jani Nikula 
---
 drivers/gpu/drm/i915/display/intel_display.c | 40 ++--
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
b/drivers/gpu/drm/i915/display/intel_display.c
index 37bd7ce88ecd..6af8d43ceb0c 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -620,45 +620,43 @@ int chv_calc_dpll_params(int refclk, struct dpll *clock)
return clock->dot / 5;
 }
 
-#define INTELPllInvalid(s)   do { /* DRM_DEBUG(s); */ return false; } while (0)
-
 /*
  * Returns whether the given set of divisors are valid for a given refclk with
  * the given connectors.
  */
-static bool intel_PLL_is_valid(struct drm_i915_private *dev_priv,
+static bool intel_pll_is_valid(struct drm_i915_private *dev_priv,
   const struct intel_limit *limit,
   const struct dpll *clock)
 {
-   if (clock->n   < limit->n.min   || limit->n.max   < clock->n)
-   INTELPllInvalid("n out of range\n");
-   if (clock->p1  < limit->p1.min  || limit->p1.max  < clock->p1)
-   INTELPllInvalid("p1 out of range\n");
-   if (clock->m2  < limit->m2.min  || limit->m2.max  < clock->m2)
-   INTELPllInvalid("m2 out of range\n");
-   if (clock->m1  < limit->m1.min  || limit->m1.max  < clock->m1)
-   INTELPllInvalid("m1 out of range\n");
+   if (clock->n < limit->n.min || limit->n.max < clock->n)
+   return false;
+   if (clock->p1 < limit->p1.min || limit->p1.max < clock->p1)
+   return false;
+   if (clock->m2 < limit->m2.min || limit->m2.max < clock->m2)
+   return false;
+   if (clock->m1 < limit->m1.min || limit->m1.max < clock->m1)
+   return false;
 
if (!IS_PINEVIEW(dev_priv) && !IS_VALLEYVIEW(dev_priv) &&
!IS_CHERRYVIEW(dev_priv) && !IS_GEN9_LP(dev_priv))
if (clock->m1 <= clock->m2)
-   INTELPllInvalid("m1 <= m2\n");
+   return false;
 
if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv) &&
!IS_GEN9_LP(dev_priv)) {
if (clock->p < limit->p.min || limit->p.max < clock->p)
-   INTELPllInvalid("p out of range\n");
+   return false;
if (clock->m < limit->m.min || limit->m.max < clock->m)
-   INTELPllInvalid("m out of range\n");
+   return false;
}
 
if (clock->vco < limit->vco.min || limit->vco.max < clock->vco)
-   INTELPllInvalid("vco out of range\n");
+   return false;
/* XXX: We may need to be checking "Dot clock" depending on the 
multiplier,
 * connector, etc., rather than just a single range.
 */
if (clock->dot < limit->dot.min || limit->dot.max < clock->dot)
-   INTELPllInvalid("dot out of range\n");
+   return false;
 
return true;
 }
@@ -725,7 +723,7 @@ i9xx_find_best_dpll(const struct intel_limit *limit,
int this_err;
 
i9xx_calc_dpll_params(refclk, );
-   if (!intel_PLL_is_valid(to_i915(dev),
+   if (!intel_pll_is_valid(to_i915(dev),
limit,
))
continue;
@@ -781,7 +779,7 @@ pnv_find_best_dpll(const struct intel_limit *limit,
int this_err;
 
pnv_calc_dpll_params(refclk, );
-   if (!intel_PLL_is_valid(to_i915(dev),
+   if (!intel_pll_is_valid(to_i915(dev),
limit,
))
continue;
@@ -842,7 +840,7 @@ g4x_find_best_dpll(const struct intel_limit *limit,
int this_err;
 
i9xx_calc_dpll_params(refclk, );
-   if (!intel_PLL_is_valid(to_i915(dev),
+   if (!intel_pll_is_valid(to_i915(dev),
limit,
))
continue;
@@ -939,7 +937,7 @@ vlv_find_best_dpll(const struct intel_limit *limit,
 
vlv_calc_dpll_params(refclk, );
 
-