Re: [PATCH 2/4] drm/i915: add intel_encoder_power_get/put

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 24, 2014 at 05:11:10PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 24, 2014 at 10:49:28AM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > This was suggested by Chris on his review to the first version of
> > "drm/i915: get power domain in case the BIOS enabled eDP VDD". Well,
> > at least that's what I understood from his comment :)
> > 
> > v2: - Make it intel_encoder_power_get/put
> >     - Don't call the new functions on functions where we need both
> >       get() and put(), so we don't end up calling
> >       intel_display_port_power_domain() more than needed.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  | 19 +++++--------------
> >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
> >  drivers/gpu/drm/i915/intel_pm.c  | 24 ++++++++++++++++++++++++
> >  3 files changed, 31 insertions(+), 14 deletions(-)
> > 
> > 
> > I am not really sure if this patch is worth it... I certainly won't be mad if we
> > drop it. The diffstat is not good.
> 
> I'm not sure either. In fact I had this same idea already when I saw
> Imre's power well patches for the first time, but then I thought that
> it doesn't help that much and decided to keep the idea to myself.

If you guys ask about my color choices I have to admit that I'm not a huge
fan of intel_display_port_power_domain. In roughly object-oriented code
functions with giant switch statements which use the object class to
decide what to do tend to be a red flag ...

In case you care for a reference see the switch statement code smell in
Martin Fowler's refactoring.

But that was just my bikeshed, tbh I don't care strongly enough here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch