On Fri, 30 Aug 2013, Chon Ming Lee <chon.ming.lee@xxxxxxxxx> 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.
DP 1.2 already supports 3 link rates, right?
> Signed-off-by: Chon Ming Lee <chon.ming.lee@xxxxxxxxx>
> ---
> 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{
Missing space before {.
> + 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}}};
> +
> +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}}};
> +
Please switch these to use C99 designated initializers for clarity,
something like this:
static const struct dp_link_dpll gen4_dpll[] = {
{
.link_bw = DP_LINK_BW_1_62,
.dpll = {
.n = 2,
.m1 = 23, m2 = 8,
p1 = 2, p2 = 10,
},
}, {
.link_bw = DP_LINK_BW_2_7,
.dpll = {
.n = 1,
.m1 = 14, m2 = 2,
p1 = 1, p2 = 10,
},
}
};
> /**
> * 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++) {
Please use i < ARRAY_SIZE(gen4_dpll).
> + if (link_bw == gen4_dpll[i].link_bw){
> + pipe_config->dpll = gen4_dpll[i].dpll;
> + break;
> + }
> }
The old if-else used some values even if link_bw was bogus. I'm not sure
how likely that is. But if the link_bw is not found in the table for
some obscure reason (e.g. the hw doesn't support the link rate), this
fails. Perhaps we should have a test for i == ARRAY_SIZE(gen4_dpll) here
and complain loudly if we hit that, and perhaps use a fallback value.
> 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;
> + }
> }
Same here.
BR,
Jani.
> pipe_config->clock_set = true;
> } else if (IS_VALLEYVIEW(dev)) {
> --
> 1.7.7.6
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
