Re: [PATCH v2 1/2] powerpc/powernv: Add poweroff (EPOW, DPO) events support for PowerNV platform

Accidentally hit the reply button instead of reply all. Re sending.

On 12-May-2015, at 11:55, trigg [off-list ref] wrote:
=20
Hi Stewart,
=20
On Tue, May 12, 2015 at 4:01 AM, Stewart Smith
[off-list ref] wrote:

quoted

trigg [off-list ref] writes:

quoted

quoted

--- a/arch/powerpc/include/asm/opal-api.h
+++ b/arch/powerpc/include/asm/opal-api.h

@@ -730,6 +730,36 @@ struct opal_i2c_request {
       __be64 buffer_ra;               /* Buffer real address */

};
=20
+/*
+ * EPOW status sharing (OPAL and the host)
+ *
+ * The host will pass on OPAL, a buffer of length OPAL_EPOW_MAX_CLASSE=

S

quoted

quoted

quoted

+ * to fetch system wide EPOW status. Each element in the returned buff=

er

quoted

quoted

quoted

+ * will contain bitwise EPOW status for each EPOW sub class.
+ */
+
+/* EPOW types */
+enum OpalEpow {
+       OPAL_EPOW_POWER         =3D 0,    /* Power EPOW */
+       OPAL_EPOW_TEMP          =3D 1,    /* Temperature EPOW */
+       OPAL_EPOW_COOLING       =3D 2,    /* Cooling EPOW */
+       OPAL_MAX_EPOW_CLASSES   =3D 3,    /* Max EPOW categories */
+};

Dont explicitly assign sequential numbers in an enum. Its taken care
of by the compiler.

=20
This header is shared with firmware, and the exact values of each item
does matter as it's ABI.

=20
This enum has no semantic difference with a series of #defines. So you
are better
off using #defines for this. Using enums with explicitly defined
values beats the whole
purpose of using them.
=20

quoted

The explicit numbers means that people think
twice before inserting a new value in the middle and subtley breaking
firmware ABI.

=20
I disagree. It wont prevent (even worsen) errors like two
enum-constants being explicitly defined to same values.
With implicit numbering you can atleast be sure that each
enum-constant will be unique.