[Intel-gfx] [PATCH 3/5] drm/i915/guc: Extract param logic form guc_init

[Arkadiusz Hiler]

On Wed, Feb 15, 2017 at 02:48:59PM +0200, Joonas Lahtinen wrote:
> On ti, 2017-02-14 at 20:37 +0100, Michal Wajdeczko wrote:
> > On Tue, Feb 14, 2017 at 05:15:39PM +0100, Arkadiusz Hiler wrote:
> > > 
> > > Let intel_guc_init() focus on determining and fetching the correct
> > > firmware.
> > > 
> > > This patch introduces intel_sanitize_uc_params() that is called from
> > > intel_uc_init().
> > > 
> > > Then, if we have GuC, we can call intel_guc_init() conditionally and we
> > > do not have to do internal checks.
> > > 
> > > Cc: Michal Winiarski <michal.winiarski at intel.com>
> > > Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> > > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> 
> <SNIP>
> 
> > > @@ -726,7 +726,7 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> > >  }
> > >  
> > >  /**
> > > - * intel_guc_init() - define parameters and fetch firmware
> > > + * intel_guc_init() - determine and fetch firmware
> > 
> > Again, can we have function name that corresponds to its purpose.
> > Comment alone is not sufficient
> 
> I second Michal here, the naming of the functions is rather poor.
> Something like intel_guc_fetch_firmware would be more descriptive.
> 
> Making it overly generic when we don't have other use yet, will cause
> more churn when we get more use (because fortune telling was never our
> thing). I'd rather see rather standalone functions, which are then be
> called from central point where more can be added, without changing the
> existing ones. So this applies to intel_uc_init, too.
> 
> It'll be easier to add these generic functions iff we start repeating
> calls to a bunch of functions, and we can then see patterns emerging.

So, couple of possibilities I see:

First:
  a) rename intel_?uc_init to intel_?uc_fw_fetch
  b) embed dev_priv in intel_uc_fw
  c) make them take intel_uc_fw struct only

  intel_uc_fw_ corresponds to struct we have, so it seems better fitting
  than what Joonas proposed.

  now we have intel_uc_fw_fetch that does the actual fetching, which is
  confusing, but we can rename it to something like
  intel_uc_fw_perform_fetch or something similar.


Second:
  Go with what we have and rename it intel_?uc_fw_init as Michal
  proposed.


What do you think?

-- 
Cheers,
Arek