Re: [PATCHv3 wl-drv-next 1/2] add basic register-field manipulation macros

On Tue, 5 Jul 2016 10:56:13 +0000, David Laight wrote:

From: Jakub Kicinski

quoted

Sent: 01 July 2016 22:27

C bitfields are problematic and best avoided.  Developers
interacting with hardware registers find themselves searching
for easy-to-use alternatives.  Common approach is to define
structures or sets of macros containing mask and shift pair.
Operations on the register are then performed as follows:

 field = (reg >> shift) & mask;

 reg &= ~(mask << shift);
 reg |= (field & mask) << shift;

Defining shift and mask separately is tedious.  Ivo van Doorn
came up with an idea of computing them at compilation time
based on a single shifted mask (later refined by Felix) which
can be used like this:

 #define REG_FIELD 0x000ff000

 field = FIELD_GET(REG_FIELD, reg);

 reg &= ~REG_FIELD;
 reg |= FIELD_PUT(REG_FIELD, field);  

My problem with these sort of 'helpers' is that they make it much harder
to read code unless you happen to know exactly what the helpers do.
Unexpected issues (like values being sign extended) can be hard to spot.

A lot of the time you can make things simpler by not doing the shifts
(ie define shifted constants).

I think creating a standard set of macros in a global header file is
actually helping the problems you raise.  One is much more likely to
know exactly what a common macro is doing than some driver-specific
ad hoc macro.  Common macros are also much more likely to be well
tested making "unexpected issues" less likely to appear.

Defining constants is fine in 20% of cases when you have only a small
set of meaningful values (e.g. what to do for a 8 bit delay or
priority field?)  Besides when fields are not aligned to 4 bits it's
hard to tell from the shifted value what the base was.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html