Re: [PATCH 01/13] drm/i915: Make the DP rates int instead of uint32_t

[Todd Previte <tprevite@xxxxxxxxx>]

On 3/12/2015 8:10 AM, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> No point in using uint32_t here, just plain old int will do.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/intel_dp.c | 24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 33d5877..1fa8cc1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -85,10 +85,9 @@ static const struct dp_link_dpll chv_dpll[] = {
>   		{ .p1 = 2, .p2 = 1, .n = 1, .m1 = 2, .m2 = 0x6c00000 } }
>   };
>   /* Skylake supports following rates */
> -static const uint32_t gen9_rates[] = { 162000, 216000, 270000, 324000,
> -					432000, 540000 };
> -
> -static const uint32_t default_rates[] = { 162000, 270000, 540000 };
> +static const int gen9_rates[] = { 162000, 216000, 270000,
> +				  324000, 432000, 540000 };
> +static const int default_rates[] = { 162000, 270000, 540000 };
>   
>   /**
>    * is_edp - is the given port attached to an eDP panel (either CPU or PCH)
> @@ -1139,7 +1138,7 @@ hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config, int link_bw)
>   }
>   
>   static int
> -intel_read_sink_rates(struct intel_dp *intel_dp, uint32_t *sink_rates)
> +intel_read_sink_rates(struct intel_dp *intel_dp, int *sink_rates)
>   {
>   	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>   	int i = 0;
> @@ -1166,7 +1165,7 @@ intel_read_sink_rates(struct intel_dp *intel_dp, uint32_t *sink_rates)
>   }
>   
>   static int
> -intel_read_source_rates(struct intel_dp *intel_dp, uint32_t *source_rates)
> +intel_read_source_rates(struct intel_dp *intel_dp, int *source_rates)
>   {
>   	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>   	int i;
> @@ -1217,8 +1216,9 @@ intel_dp_set_clock(struct intel_encoder *encoder,
>   	}
>   }
>   
> -static int intel_supported_rates(const uint32_t *source_rates, int source_len,
> -const uint32_t *sink_rates, int sink_len, uint32_t *supported_rates)
> +static int intel_supported_rates(const int *source_rates, int source_len,
> +				 const int *sink_rates, int sink_len,
> +				 int *supported_rates)
>   {
>   	int i = 0, j = 0, k = 0;
>   
> @@ -1245,7 +1245,7 @@ const uint32_t *sink_rates, int sink_len, uint32_t *supported_rates)
>   	return k;
>   }
>   
> -static int rate_to_index(uint32_t find, const uint32_t *rates)
> +static int rate_to_index(int find, const int *rates)
>   {
>   	int i = 0;
>   
> @@ -1275,9 +1275,9 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>   	int max_clock;
>   	int bpp, mode_rate;
>   	int link_avail, link_clock;
> -	uint32_t sink_rates[8];
> -	uint32_t supported_rates[8] = {0};
> -	uint32_t source_rates[8];
> +	int sink_rates[8];
> +	int supported_rates[8] = {0};
> +	int source_rates[8];
>   	int source_len, sink_len, supported_len;
>   
>   	sink_len = intel_read_sink_rates(intel_dp, sink_rates);
Since you're going through the work to redo all the link rates, does it 
make more sense to have the numerical rates defined in an enum then 
referenced by enumerated type in the arrays? Functionally it really 
won't make any difference but it might make it easier to understand 
which platforms support which rates at a glance. Certainly not a 
blocking request, just a thought.

Otherwise this looks fine.

Reviewed-by: Todd Previte <tprevite@xxxxxxxxx>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx