On Sat, Aug 31, 2013 at 01:57:44AM +0800, Chon Ming Lee wrote:
> eDP 1.4 supports 4-5 extra link rates in additional to current 2 link
> rate.  Create a structure to store the DPLL divisor data to improve
> readability.
> 
> Signed-off-by: Chon Ming Lee <chon.ming....@intel.com>

This is a neat idea and I agree it'll be much better when we add edp link
rates. Small comments below:

> ---
>  drivers/gpu/drm/i915/intel_dp.c |   48 +++++++++++++++++++-------------------
>  1 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 2151d13..ab8a5ff 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -38,6 +38,19 @@
>  
>  #define DP_LINK_CHECK_TIMEOUT        (10 * 1000)
>  
> +struct dp_link_dpll{
> +     int link_bw;
> +     struct dpll dpll;
> +};
> +
> +static const struct dp_link_dpll gen4_dpll[] = 
> +                             {{ DP_LINK_BW_1_62, {2,23,8,2,10,0,0,0,0}},
> +                             { DP_LINK_BW_2_7,  {1,14,2,1,10,0,0,0,0}}};

Usually we only indent by one tab here. Also please use c99 initializers
and you don't need to explicitly initialize to 0. And a bit more space
helps readbility further imo:

static const struct dp_link_dpll gen4_dpll[] = 
{
        { DP_LINK_BW_1_62,
                { .p1 = 2, .p2 = 10, .n = 2, .m1 = 23, .m2 = 8 }},
        /* more ... */
};


> +
> +static const struct dp_link_dpll pch_dpll[] = 
> +                             {{ DP_LINK_BW_1_62, {1,12,9,2,10,0,0,0,0}},
> +                             { DP_LINK_BW_2_7,  {2,14,8,1,10,0,0,0,0}}};
> +
>  /**
>   * is_edp - is the given port attached to an eDP panel (either CPU or PCH)
>   * @intel_dp: DP struct
> @@ -649,37 +662,24 @@ intel_dp_set_clock(struct intel_encoder *encoder,
>                  struct intel_crtc_config *pipe_config, int link_bw)
>  {
>       struct drm_device *dev = encoder->base.dev;
> +     int i;
>  
>       if (IS_G4X(dev)) {
> -             if (link_bw == DP_LINK_BW_1_62) {
> -                     pipe_config->dpll.p1 = 2;
> -                     pipe_config->dpll.p2 = 10;
> -                     pipe_config->dpll.n = 2;
> -                     pipe_config->dpll.m1 = 23;
> -                     pipe_config->dpll.m2 = 8;
> -             } else {
> -                     pipe_config->dpll.p1 = 1;
> -                     pipe_config->dpll.p2 = 10;
> -                     pipe_config->dpll.n = 1;
> -                     pipe_config->dpll.m1 = 14;
> -                     pipe_config->dpll.m2 = 2;
> +             for(i = 0; i < sizeof(gen4_dpll) / sizeof(struct dp_link_dpll); 
> i++) {
> +                     if (link_bw == gen4_dpll[i].link_bw){
> +                             pipe_config->dpll = gen4_dpll[i].dpll;
> +                             break;
> +                     }
>               }
>               pipe_config->clock_set = true;
>       } else if (IS_HASWELL(dev)) {
>               /* Haswell has special-purpose DP DDI clocks. */
>       } else if (HAS_PCH_SPLIT(dev)) {
> -             if (link_bw == DP_LINK_BW_1_62) {
> -                     pipe_config->dpll.n = 1;
> -                     pipe_config->dpll.p1 = 2;
> -                     pipe_config->dpll.p2 = 10;
> -                     pipe_config->dpll.m1 = 12;
> -                     pipe_config->dpll.m2 = 9;
> -             } else {
> -                     pipe_config->dpll.n = 2;
> -                     pipe_config->dpll.p1 = 1;
> -                     pipe_config->dpll.p2 = 10;
> -                     pipe_config->dpll.m1 = 14;
> -                     pipe_config->dpll.m2 = 8;
> +             for(i = 0; i < sizeof(pch_dpll) / sizeof(struct dp_link_dpll); 
> i++) {
> +                     if (link_bw == pch_dpll[i].link_bw){ 
> +                             pipe_config->dpll = pch_dpll[i].dpll;
> +                             break;
> +                     }

I'd add temporary variables here to first select the right platform (and
size of the array) and then only have one for loop to fish out the right
value.

Cheers, Daniel

>               }
>               pipe_config->clock_set = true;
>       } else if (IS_VALLEYVIEW(dev)) {
> -- 
> 1.7.7.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to