Re: [PATCH 3/7] drm/i915: keep freq/opcode conversion function more generic

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jul 11, 2014 at 09:56:35AM +0530, Deepak S wrote:
> 
> On Wednesday 09 July 2014 05:33 PM, Daniel Vetter wrote:
> >On Thu, Jul 10, 2014 at 01:16:23PM +0530, deepak.s@linux.intel.com wrote:
> >>From: Deepak S <deepak.s@linux.intel.com>
> >>
> >>Since freq/encode conversion formula changes from platform to platform,
> >>create a generic wrapper function and having platform check inside this
> >>help to simpilfy adding newer platform freq/opcode conversion.
> >>
> >>Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> >>---
> >>  drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++-----
> >>  drivers/gpu/drm/i915/i915_drv.h     |  4 +--
> >>  drivers/gpu/drm/i915/i915_sysfs.c   | 18 ++++++-------
> >>  drivers/gpu/drm/i915/intel_pm.c     | 52 +++++++++++++++++++++++++++----------
> >>  4 files changed, 56 insertions(+), 32 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >>index b3b56c4..dd7078d 100644
> >>--- a/drivers/gpu/drm/i915/i915_debugfs.c
> >>+++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >>@@ -1140,14 +1140,14 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
> >>  		val = valleyview_rps_max_freq(dev_priv);
> >>  		seq_printf(m, "max GPU freq: %d MHz\n",
> >>-			   vlv_gpu_freq(dev_priv, val));
> >>+			   intel_gpu_freq(dev_priv, val));
> >intel_ is a bit too generic a prefix for a function which seems to be only
> >used on byt+chv. I'd just add a if (IS_CHERRYVIEW) ... else /* vlv code */
> >to both functions and not extract further.
> >
> >Aside: Since marketing stopped using vlv and switched to byt we're using
> >vlv for code shared by byt and chv and byt_ for byt-only code. Helps a bit
> >to keep things appart.
> >-Daniel
> 
> Ok. Will it be Ok to use "vlv_gpu_freq" and have BYT and CHV check under
> this function?

Yeah, that's my idea.

> The reason why i made more generic is it will help us to add conversion
> logic for future platforms

We can look at this again when it happpens. With the current code this
doesn't include desktop rps so the intel_ prefix was a bit confusing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch