On Mon, 16 Apr 2012 21:08:22 -0300 Eugeni Dodonov <eugeni at dodonov.net> wrote: > On Mon, Apr 16, 2012 at 20:23, Chris Wilson > <chris at chris-wilson.co.uk>wrote: > > > On Mon, 16 Apr 2012 20:06:01 -0300, Eugeni Dodonov < > > eugeni.dodonov at intel.com> wrote: > > > As previously discussed on irc with Daniel, Ben and Jesse, This > > > patch moves the power-related functionality into intel_pm module, > > > aiming at simplifying the intel_display code and make it less > > > cluttered. > > > > > > The functionality affected by this move are: clock gating, rc6 > > > and rps, display watermarks, display fifo-related items, cxsr and > > > FBC. > > > > > > This simplifies intel_display by +/- 2800 lines of code and > > > centralizes most of the power-related items in one place to > > > simplify future hardware enablement and subsystems interaction. > > > > Diff is making this really difficult to verify that no functional > > change is taking place. Suggestions? A series of small commits? > > > > I am fine with making it into a small series of commits which move 1 > subsystem at a time (fbc, cxsr, rc6, fifo, wm, ...). Would everyone be > happy with that? I think Daniel wanted to move everything in 1 commit, > hence this initial approach from my side, but yes, as functions are > spread all around the place it makes very hard to review the patch. > > But in general, does this idea of intel_pm.c looks interesting, or > anyone has anything against it? > I like intel_pm.c I don't have an opinion yet on 1 big commit or many small commits. I'd like if you made a description not just of what has been moved, but what belongs in this file (as a comment in the .c file). Also in the bikeshed category: I don't feel like FBC belongs here. I also don't know about display watermarks, and certain fifo things. Similarly with fifo related stuff, some may belong, some may not - I haven't actually looked at the patch. Anyway, when/if danvet deals with this, I like the idea very much so it's Acked-by: Ben Widawsky <benjamin.widawsky at intel.com>
