Re: [PATCH 2/5] drm/i915/bxt: add missing DDI PLL registers to the state checking

Daniel Vetter
2015-06-24T10:36:40Z

On Wed, Jun 24, 2015 at 01:19:10PM +0300, Imre Deak wrote:
> On ke, 2015-06-24 at 15:37 +0530, Jindal, Sonika wrote:
> > On 6/18/2015 7:55 PM, Imre Deak wrote:
> > > @@ -2427,8 +2431,8 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > temp = I915_READ(BXT_PORT_PLL_EBB_4(port));
> > > temp |= PORT_PLL_RECALIBRATE;
> > > I915_WRITE(BXT_PORT_PLL_EBB_4(port), temp);
> > > - /* Enable 10 bit clock */
> > I think it is OK to keep this comment here because all the steps are
> > mentioned in comments, even "disable 10 bit clock".
>
> Yea, but some of those comments just say what is really obvious from the
> code right afterwards.

Concur with Imre here, comments shouldn't ever explain what the code does,
but why. If you need to explain what the code does in comments, then it
needs to be refactored (helper function with clear name extracted, better
naming of variables/defines, ...).

Imo almost all the comments in this code should be remove because they're
obvious.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation