Re: [PATCH] [VPG HSW-A] drm/i915: BUN vol4g[DevHSW]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux