Re: [PATCH v9 3/7] drm/i915/tgl: Enable DC3CO state in "DC Off" power well

[Imre Deak <imre.deak@intel.com>]

On Fri, Sep 27, 2019 at 04:40:59PM +0300, Jani Nikula wrote:
> On Fri, 27 Sep 2019, Imre Deak <imre.deak@intel.com> wrote:
> > On Fri, Sep 27, 2019 at 02:07:43PM +0300, Imre Deak wrote:
> >> On Thu, Sep 26, 2019 at 08:26:17PM +0530, Anshuman Gupta wrote:
> >> > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state)
> >
> > We need a documentation for exported functions.
> 
> And really you should make an effort to *NOT* expose platform specific
> functions from your C modules. Yes, we have some, but the direction
> should be the opposite of adding more.
> 
> I'll be more strict about this going forward. We need to improve the
> interfaces we have.

Yes, the original idea was to add a new power well for the DC3co power
state, which would allow us to use the existing get/put interface. That
doesn't work too well though, because we can have only one of the states
enabled, so we went with having only one DC power well and a new API to
set its target DC state.

> 
> >> > @@ -256,6 +257,7 @@ void intel_display_power_suspend_late(struct drm_i915_private *i915);
> >> >  void intel_display_power_resume_early(struct drm_i915_private *i915);
> >> >  void intel_display_power_suspend(struct drm_i915_private *i915);
> >> >  void intel_display_power_resume(struct drm_i915_private *i915);
> >> > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state);
> 
> This sticks out like a sore thumb.

Yep, so the rule is to have a uniform prefix for all exported functions,
so we could rename this to intel_display_power_set_dc_target(),

> And you're not even using the function outside of intel_display_power.h!

It's used, but the use is added only later. I agree that, as commented
elsewhere, new functions should be added in the same patch where they
are used. 

> BR,
> Jani.
> 
> 
> >> >  
> >> >  const char *
> >> >  intel_display_power_domain_str(enum intel_display_power_domain domain);
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx