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@xxxxxxxxx> > > > > > > > > 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@xxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
