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@xxxxxxxxx> > > > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> > > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> > > > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@xxxxxxxxx> > > <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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
