From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Kannan, Vandana" Subject: Re: [RFC 0/6] Rearranging PPS related code Date: Mon, 13 Oct 2014 10:25:55 +0530 Message-ID: <543B5B5B.6090708@intel.com> References: <1412694584-743-1-git-send-email-vandana.kannan@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id BEB12896F7 for ; Sun, 12 Oct 2014 21:55:58 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: intel-gfx List-Id: intel-gfx@lists.freedesktop.org On 09-Oct-14 9:17 PM, Daniel Vetter wrote: > On Tue, Oct 07, 2014 at 08:39:38PM +0530, Vandana Kannan wrote: >> Since panel power sequencing is a feature common to all internal displays, >> moving relevant code to intel_panel.c. This patch series contains changes >> to setup PPS data and program register values as required. >> >> The implementation follows the model used for backlight funcs >> (as suggested by Daniel) which splits the changes based on platform. >> As of now, changes have been made considering only eDP. >> >> TODO:- >> 1. To accomodate software delays where applicable, placeholders have been >> created in i915_display_funcs, but have not been defined yet. >> 2. Integrate MIPI PPS delays once 1. is done. >> 3. PPS delays would be required for LVDS as well. The existing file >> intel_lvds.c does not make use of the delays. > > I haven't taken a in-depth look at your patches since I'm travelling. > But one thing I've noticed while scrolling through them is that you > first add most of the new code in a few patches, and then remove the > old one in follow-up patches. This makes reviewing patches a lot > harder. > > A better way to split refactoring patch series is to do the split > per-function. So for this series here maybe have a patch for the pps > init changes, the functions to enable/disable power, and so on. And > the important part is that you add/remove/change the code in one patch > for a given function so that the actual change can be reviewed. So the > new vtable functions should grow out of the existing code. > > And if you need to split code this should always be done in 2 steps: > First make a verbatim copy with the new names, then refactor both > copies to be platform specific. Of course if the code you copy is just > a few lines that can be done in one step, but as soon as you can't > read both functions completely in the diff context you should do the > split. Jani has definitely overstretched the limit with his bachlight > patches, but occasionally I let things slip through ;-) > > Yours, Daniel > Hi Daniel, Thanks for your inputs. Agree with your comments, will make changes to the patches accordingly and resend. Thanks, Vandana >> >> Vandana Kannan (6): >> drm/i915: Create PPS related struct and func pointers >> drm/i915: Define PPS setup functions >> drm/i915: Program PPS registers >> drm/i915: Removing refs to intel_dp_panel_power_sequencer >> drm/i915: Replace all refs to intel_dp delays >> drm/i915: Modify refs to intel dp timestamps >> >> drivers/gpu/drm/i915/i915_drv.h | 15 ++ >> drivers/gpu/drm/i915/intel_display.c | 1 + >> drivers/gpu/drm/i915/intel_dp.c | 275 ++++++----------------------------- >> drivers/gpu/drm/i915/intel_drv.h | 32 +++- >> drivers/gpu/drm/i915/intel_panel.c | 260 +++++++++++++++++++++++++++++++++ >> 5 files changed, 346 insertions(+), 237 deletions(-) >> >> -- >> 2.0.1 >> >