Re: [PATCH 3/5] drm/i915/guc: Extract param logic form guc_init

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@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.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx