On Tue, Mar 6, 2018 at 1:16 AM, Darren Hart <dvhart@xxxxxxxxxxxxx> wrote: > On Sun, Mar 04, 2018 at 08:44:26PM +0100, Michał Kępień wrote: >> > On Wed, Feb 28, 2018 at 06:08:52PM +0200, Andy Shevchenko wrote: >> > > On Tue, Feb 27, 2018 at 11:15 PM, Micha?? K??pie?? <kernel@xxxxxxxxxx> wrote: >> > > > Various functions exposed by the firmware through the FUNC interface >> > > > tend to use a consistent set of integers for denoting the type of >> > > > operation to be performed for a specified feature. Use named constants >> > > > instead of integers in each call_fext_func() invocation in order to more >> > > > clearly convey the intent of each call. >> > > > >> > > > Note that FUNC_FLAGS is a bit peculiar: >> > > >> > > > +/* FUNC interface - operations */ >> > > > +#define OP_GET BIT(1) >> > > > +#define OP_GET_CAPS 0 >> > > > +#define OP_GET_EVENTS BIT(0) >> > > > +#define OP_GET_EXT BIT(2) >> > > > +#define OP_SET BIT(0) >> > > > +#define OP_SET_EXT (BIT(2) | BIT(0)) >> > > >> > > Hmm... this looks unordered a bit. >> > >> > It seems to be ordered alphabetically on the identifier. Andy, is it >> > preferred to order defines like this based on resolved numeric order? >> >> Just to expand on what Jonathan wrote above: if you take a peek at the >> end result of the patch series, you will notice a pattern: constants in >> each section are ordered alphabetically by their name. I wanted all >> sections to be consistently ordered. If you would rather have me order >> things by the bit index, sure, no problem, just please note that the >> order above is not accidental. > > Hrm. In my experience it is more typical to order by value (bit), that's a > little less obvious when using the BIT()|BIT() macros though. So long as it's > consistent, I think that's what matters most. What I'm trying to tell is about consistency of style. So, imagine if we have two bitfields in some register, one with one bit and the other with two. And for latter one only 3 states are possible (00, 10, 11), so, REG1_FLDA BIT(0) REG1_FLDB_STATE0 0 REG1_FLDB_STATE2 BIT(2) REG1_FLDB_STATE3 BIT(2) | BIT(3) // or 0x6, or (3 << 1) Above is not what I would like to see. The consistent may be one of the following REG1_FLDA BIT(0) REG1_FLDB_STATE0 (0 << 1) REG1_FLDB_STATE2 (2 << 1) REG1_FLDB_STATE3 (3 << 1) or (implying that in the code _SHIFT is used) REG1_FLDA BIT(0) REG1_FLDB_SHIFT 1 REG1_FLDB_STATE0 0 REG1_FLDB_STATE2 2 REG1_FLDB_STATE3 3 or (perhaps less wanted) REG1_FLDA (1 << 0) REG1_FLDB_STATE0 (0 << 1) REG1_FLDB_STATE2 (2 << 1) REG1_FLDB_STATE3 (3 << 1) -- With Best Regards, Andy Shevchenko
