Re: [PATCH 2/8] drm/i915/skl: Implement enable/disable for Display C5 sttate.

From: Daniel Vetter <daniel@ffwll.ch>
To: Imre Deak <imre.deak@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/8] drm/i915/skl: Implement enable/disable for Display C5 sttate.
Date: Tue, 14 Apr 2015 13:50:53 +0200	[thread overview]
Message-ID: <20150414115053.GI6092@phenom.ffwll.local> (raw)
In-Reply-To: <1428933097.29827.89.camel@intel.com>

On Mon, Apr 13, 2015 at 04:51:37PM +0300, Imre Deak wrote:
> On ma, 2015-04-13 at 14:17 +0100, Damien Lespiau wrote:
> > On Thu, Apr 02, 2015 at 06:58:22PM +0300, Imre Deak wrote:
> > > On ke, 2015-04-01 at 16:22 +0530, Animesh Manna wrote:
> > > > From: "A.Sunil Kamath" <sunil.kamath@intel.com>
> > > > 
> > > > This patch just implements the basic enable and disable
> > > > functions of DC5 state which is needed for both SKL and BXT.
> > > 
> > > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > 
> > For the record, this patch generates compilation warnings when applied
> > on its own:
> > 
> > drivers/gpu/drm/i915/intel_runtime_pm.c:368:13: warning: ‘gen9_enable_dc5’ defined but not used [-Wunused-function]
> >  static void gen9_enable_dc5(struct drm_i915_private *dev_priv)
> >              ^
> > drivers/gpu/drm/i915/intel_runtime_pm.c:386:13: warning: ‘gen9_disable_dc5’ defined but not used [-Wunused-function]
> >  static void gen9_disable_dc5(struct drm_i915_private *dev_priv)
> >              ^
> > 
> > Generally speaking, in a series, each step should compile without warning and
> > result in a working driver (for bisectability).
> 
> Yes, agreed. Splitting the changes into patches could've been done in
> better a way. I also commented on a related topic of adding something in
> the patchset and removing it later. We'll try to pay more attention to
> this in the future.
> 
> Animesh, if you resend this patchset anyway I think you could switch the
> order of 2/8 and 3/8 and add the calls to the above functions in this
> patch to get rid of the warnings. Also please make sure that things
> compile without a warning after each patch as Damien suggested.

Yeah generally I don't like when patches add functions and structures and
don't use them - in the end it's fairly hard to review something if you
don't know how it's getting called used, which means you can't read the
patches linearly. And the point of splitting them is to give reviewers
small digestable chunks instead of the full thing.

Imo it's better to split things the other way round, i.e. wire up stub
functions at first, then fill them out. Instead of the first patch adding
the implementation and the 2nd one wiring things up.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx