[Intel-gfx] [RFC 0/6] Rearranging PPS related code

[Daniel Vetter]

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

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch