Re: [RFC 0/6] Rearranging PPS related code

["Kannan, Vandana" <vandana.kannan@intel.com>]

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
>>
>