[PATCH 1/5] drm/i915/uc: Move intel_uc_fw_status_repr() to intel_uc.c

[PATCH 1/5] drm/i915/uc: Move intel_uc_fw_status_repr() to intel_uc.c

[Michal Wajdeczko]

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(-)

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);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/uc: Move intel_uc_fw_status_repr() to intel_uc.c

[Joonas Lahtinen]

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
-- 
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

Re: [PATCH 1/5] drm/i915/uc: Move intel_uc_fw_status_repr() to intel_uc.c

[Michal Wajdeczko]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/uc: Move intel_uc_fw_status_repr() to intel_uc.c

[Michal Wajdeczko]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/uc: Move intel_uc_fw_status_repr() to intel_uc.c

[Srivatsa, Anusha]

>-----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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx