| Message ID | 20170327171929.122384-1-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On ma, 2017-03-27 at 17:19 +0000, Michal Wajdeczko wrote: > The file fits better. Also use "<invalid>" for invalid case. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> <SNIP> > @@ -26,6 +26,24 @@ > #include "intel_uc.h" > #include <linux/firmware.h> > > + > +/* User-friendly representation of an enum */ > +const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status) This could be static inline in the .h too. > +{ > + switch (status) { > + case INTEL_UC_FIRMWARE_FAIL: > + return "FAIL"; > + case INTEL_UC_FIRMWARE_NONE: > + return "NONE"; > + case INTEL_UC_FIRMWARE_PENDING: > + return "PENDING"; > + case INTEL_UC_FIRMWARE_SUCCESS: > + return "SUCCESS"; > + default: Add MISSING_CASE(status); here. > + return "<invalid>"; > + } > +} With the MISSING_CASE, this is; Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Regards, Joonas
On Tue, Mar 28, 2017 at 10:05:31AM +0300, Joonas Lahtinen wrote: > On ma, 2017-03-27 at 17:19 +0000, Michal Wajdeczko wrote: > > The file fits better. Also use "<invalid>" for invalid case. > > > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > <SNIP> > > > @@ -26,6 +26,24 @@ > > #include "intel_uc.h" > > #include <linux/firmware.h> > > > > + > > +/* User-friendly representation of an enum */ > > +const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status) > > This could be static inline in the .h too. > > > +{ > > + switch (status) { > > + case INTEL_UC_FIRMWARE_FAIL: > > + return "FAIL"; > > + case INTEL_UC_FIRMWARE_NONE: > > + return "NONE"; > > + case INTEL_UC_FIRMWARE_PENDING: > > + return "PENDING"; > > + case INTEL_UC_FIRMWARE_SUCCESS: > > + return "SUCCESS"; > > + default: > > Add MISSING_CASE(status); here. This will require another patch that will move definition of MISSING_CASE macro from i915_drv.h to i915_utils.h as intel_uc.h is included now ahead of MISSING_CASE definition... stay tuned ;) -Michal > > > + return "<invalid>"; > > + } > > +} > > With the MISSING_CASE, this is; > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Regards, Joonas > -- > Joonas Lahtinen > Open Source Technology Center > Intel Corporation
On Tue, Mar 28, 2017 at 10:27:28AM +0200, Michal Wajdeczko wrote: > On Tue, Mar 28, 2017 at 10:05:31AM +0300, Joonas Lahtinen wrote: > > On ma, 2017-03-27 at 17:19 +0000, Michal Wajdeczko wrote: > > > The file fits better. Also use "<invalid>" for invalid case. > > > > > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > > <SNIP> > > > > > @@ -26,6 +26,24 @@ > > > #include "intel_uc.h" > > > #include <linux/firmware.h> > > > > > > + > > > +/* User-friendly representation of an enum */ > > > +const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status) > > > > This could be static inline in the .h too. > > > > > +{ > > > + switch (status) { > > > + case INTEL_UC_FIRMWARE_FAIL: > > > + return "FAIL"; > > > + case INTEL_UC_FIRMWARE_NONE: > > > + return "NONE"; > > > + case INTEL_UC_FIRMWARE_PENDING: > > > + return "PENDING"; > > > + case INTEL_UC_FIRMWARE_SUCCESS: > > > + return "SUCCESS"; > > > + default: > > > > Add MISSING_CASE(status); here. > > This will require another patch that will move definition of > MISSING_CASE macro from i915_drv.h to i915_utils.h as intel_uc.h > is included now ahead of MISSING_CASE definition... stay tuned ;) > There is also other option: we can drop "default" case and then rely on the compiler to complain at build time when we miss any enum. -Michal
>-----Original Message----- >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of >Michal Wajdeczko >Sent: Tuesday, March 28, 2017 5:51 AM >To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >Cc: intel-gfx@lists.freedesktop.org >Subject: Re: [Intel-gfx] [PATCH 1/5] drm/i915/uc: Move intel_uc_fw_status_repr() >to intel_uc.c > >On Tue, Mar 28, 2017 at 10:27:28AM +0200, Michal Wajdeczko wrote: >> On Tue, Mar 28, 2017 at 10:05:31AM +0300, Joonas Lahtinen wrote: >> > On ma, 2017-03-27 at 17:19 +0000, Michal Wajdeczko wrote: >> > > The file fits better. Also use "<invalid>" for invalid case. >> > > >> > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> > > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> >> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> > >> > <SNIP> >> > >> > > @@ -26,6 +26,24 @@ >> > > #include "intel_uc.h" >> > > #include <linux/firmware.h> >> > > >> > > + >> > > +/* User-friendly representation of an enum */ const char >> > > +*intel_uc_fw_status_repr(enum intel_uc_fw_status status) >> > >> > This could be static inline in the .h too. >> > >> > > +{ >> > > + switch (status) { >> > > + case INTEL_UC_FIRMWARE_FAIL: >> > > + return "FAIL"; >> > > + case INTEL_UC_FIRMWARE_NONE: >> > > + return "NONE"; >> > > + case INTEL_UC_FIRMWARE_PENDING: >> > > + return "PENDING"; >> > > + case INTEL_UC_FIRMWARE_SUCCESS: >> > > + return "SUCCESS"; >> > > + default: >> > >> > Add MISSING_CASE(status); here. >> >> This will require another patch that will move definition of >> MISSING_CASE macro from i915_drv.h to i915_utils.h as intel_uc.h is >> included now ahead of MISSING_CASE definition... stay tuned ;) >> > >There is also other option: we can drop "default" case and then rely on the >compiler to complain at build time when we miss any enum. But wont having a default option make it more readable and friendly? Just a thought.... I like it the way it is now..... Anusha >-Michal > >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 7d92321..8a1a023 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -73,22 +73,6 @@ MODULE_FIRMWARE(I915_BXT_GUC_UCODE); #define I915_KBL_GUC_UCODE GUC_FW_PATH(kbl, KBL_FW_MAJOR, KBL_FW_MINOR) MODULE_FIRMWARE(I915_KBL_GUC_UCODE); -/* User-friendly representation of an enum */ -const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status) -{ - switch (status) { - case INTEL_UC_FIRMWARE_FAIL: - return "FAIL"; - case INTEL_UC_FIRMWARE_NONE: - return "NONE"; - case INTEL_UC_FIRMWARE_PENDING: - return "PENDING"; - case INTEL_UC_FIRMWARE_SUCCESS: - return "SUCCESS"; - default: - return "UNKNOWN!"; - } -}; static u32 get_gttype(struct drm_i915_private *dev_priv) { diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index c767dc3..bbfbda4 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -26,6 +26,24 @@ #include "intel_uc.h" #include <linux/firmware.h> + +/* User-friendly representation of an enum */ +const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status) +{ + switch (status) { + case INTEL_UC_FIRMWARE_FAIL: + return "FAIL"; + case INTEL_UC_FIRMWARE_NONE: + return "NONE"; + case INTEL_UC_FIRMWARE_PENDING: + return "PENDING"; + case INTEL_UC_FIRMWARE_SUCCESS: + return "SUCCESS"; + default: + return "<invalid>"; + } +} + /* Reset GuC providing us with fresh state for both GuC and HuC. */ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv) diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index 087192d..dfa0812 100644 --- a/drivers/gpu/drm/i915/intel_uc.h +++ b/drivers/gpu/drm/i915/intel_uc.h @@ -191,6 +191,7 @@ struct intel_huc { }; /* intel_uc.c */ +const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status); void intel_uc_sanitize_options(struct drm_i915_private *dev_priv); void intel_uc_init_early(struct drm_i915_private *dev_priv); void intel_uc_init_fw(struct drm_i915_private *dev_priv); @@ -207,7 +208,6 @@ static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 l /* intel_guc_loader.c */ int intel_guc_select_fw(struct intel_guc *guc); int intel_guc_init_hw(struct intel_guc *guc); -const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status); int intel_guc_suspend(struct drm_i915_private *dev_priv); int intel_guc_resume(struct drm_i915_private *dev_priv); u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
The file fits better. Also use "<invalid>" for invalid case. Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/intel_guc_loader.c | 16 ---------------- drivers/gpu/drm/i915/intel_uc.c | 18 ++++++++++++++++++ drivers/gpu/drm/i915/intel_uc.h | 2 +- 3 files changed, 19 insertions(+), 17 deletions(-)