On Tue, Aug 27, 2013 at 05:27:21AM +0000, Biswas, KoushikX wrote: > Hi Paulo, > There are some changes in voltage swing levels for Haswell platform. In > current implementation, macro name are given after the swing values. But > the values are changed for Haswell. This comment is added to avoid the > confusion during signal level debug or other similar kind of activity. If the swing/preemph meanings of the values changed, then I think we should updated the #defines. If needed we need to add a new table, which with our current code structure should be fairly ok to do. Generally comments should only explain high-level stuff or tricky stuff, but not the details of the code for two reaons: 1. code should be self-explaining. 2. comments don't get run and so bitrot way too quickly compared to code. -Daniel > > > Thanks and regards, > > Koushik > > -----Original Message----- > From: Paulo Zanoni [mailto:przanoni@xxxxxxxxx] > Sent: Tuesday, August 27, 2013 12:46 AM > To: Biswas, KoushikX > Cc: Intel Graphics Development > Subject: Re: [PATCH] [VPG HSW-A] drm/i915: BUN vol4g[DevHSW] > > 2013/8/26 Koushik Biswas <koushikx.biswas@xxxxxxxxx>: > > From: koushik <koushikx.biswas@xxxxxxxxx> > > > > WW43 2012 - DDI buffer translation corrections > > WW36 2012 - Added HDMI voltage swing (not implemented > > for HDMI) > > > > Added comments with voltage swing, pre-emphasis, transition and > > non-transition values in form of table for reference. This values are > > applicable only for HSW platform. > > > But why exactly do we need this comment? We already have the > intel_hsw_signal_levels() function (inside intel_dp.c) which is basically the implementation of your comment. > > > > > > Signed-off-by: koushik <koushikx.biswas@xxxxxxxxx> > > Change-Id: I0cff220c7d047f41b2a96b3e3880b4029550d458 > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 31 +++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_dp.c | 32 ++++++++++++++++++++++++++++++++ > > 2 files changed, 63 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > b/drivers/gpu/drm/i915/intel_ddi.c > > index 5131517..0de236e 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -128,6 +128,37 @@ void intel_prepare_ddi(struct drm_device *dev) > > intel_prepare_ddi_buffers(dev, PORT_E, true); } > > > > + > > +/* Updating the new table in comments as it doesn't cause any logic > > +change */ > > + > > +/************* For HSW Voltage swing levels > > +***************************/ > > +/******************************************************************** > > +**/ > > +/*___________________________________________________________________ > > +_*/ /*|Entry|Voltage|Pre-emphasis|Non-Transition|Transition > > +|Pre-emphasis|*/ > > +/*| |Swing |level |mV diff p-p |mV diff p-p|db |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|0 |0 |0 |400 |400 |0 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|1 |0 |1 |400 |600 |3.5 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|2 |0 |2 |400 |800 |6 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|3 |0 |3 |400 |1000 |8 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|4 |1 |0 |600 |600 |0 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|5 |1 |1 |600 |900 |3.5 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|6 |1 |2 |600 |1000 |4.5 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|7 |2 |0 |800 |800 |0 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|8 |2 |1 |1000 |1000 |2 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|9 | Entry 9 is only used for HDMI and DVI |*/ > > +/*|------------------------------------------------------------------ > > +|*/ > > +/******************************************************************** > > +**/ > > + > > static const long hsw_ddi_buf_ctl_values[] = { > > DDI_BUF_EMP_400MV_0DB_HSW, > > DDI_BUF_EMP_400MV_3_5DB_HSW, > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c index 9fd7f90..fa73fb1 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1809,6 +1809,38 @@ intel_gen7_edp_signal_levels(uint8_t train_set) > > } > > } > > > > + > > +/* Updating the new table in comments as it doesn't cause any logic > > +change */ > > + > > +/************* For HSW Voltage swing levels > > +***************************/ > > +/******************************************************************** > > +**/ > > +/*___________________________________________________________________ > > +_*/ /*|Entry|Voltage|Pre-emphasis|Non-Transition|Transition > > +|Pre-emphasis|*/ > > +/*| |Swing |level |mV diff p-p |mV diff p-p|db |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|0 |0 |0 |400 |400 |0 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|1 |0 |1 |400 |600 |3.5 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|2 |0 |2 |400 |800 |6 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|3 |0 |3 |400 |1000 |8 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|4 |1 |0 |600 |600 |0 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|5 |1 |1 |600 |900 |3.5 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|6 |1 |2 |600 |1000 |4.5 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|7 |2 |0 |800 |800 |0 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|8 |2 |1 |1000 |1000 |2 |*/ > > +/*|------------------------------------------------------------------|*/ > > +/*|9 | Entry 9 is only used for HDMI and DVI |*/ > > +/*|------------------------------------------------------------------ > > +|*/ > > +/******************************************************************** > > +**/ > > + > > + > > /* Gen7.5's (HSW) DP voltage swing and pre-emphasis control */ > > static uint32_t intel_hsw_signal_levels(uint8_t train_set) > > -- > > 1.7.9.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
