On 20/05/16 18:52, Hartley Sweeten wrote:
On Friday, May 20, 2016 10:18 AM, Ian Abbott wrote:
On 20/05/16 17:37, Hartley Sweeten wrote:
On Friday, May 20, 2016 6:49 AM, Ian Abbott wrote:
Rename the macros for the PLX PCI 9080 LAS0RR and LAS1RR registers in
"plx9080.h", using the prefix `PLX_LASRR_`. Make use of the `BIT(x)`
and `GENMASK(h,l)` macros to define the values.
Define a macro `PLX_LASRR_PREFETCH` for the "prefetchable memory" bit in
this register, and define a macro `PLX_LASRR_MLOC_MASK` to mask the PCI
memory location control bits.
[snip]
+#define PLX_LASRR_IO BIT(0) /* Map to: 1=I/O, 0=Mem */
+#define PLX_LASRR_ANY32 (BIT(1) * 0) /* Locate anywhere in 32 bit */
+#define PLX_LASRR_LT1MB (BIT(1) * 1) /* Locate in 1st meg */
+#define PLX_LASRR_ANY64 (BIT(1) * 2) /* Locate anywhere in 64 bit */
The (BIT(n) * x) looks ugly.
You won't like the remaining patches then!
You are correct... ;-)
FWIW, all the constants end up with the same type (unsigned long) this way.
That's probably good but it sure makes the defines look ugly, and a bit hard to
understand imoh. You also don't know what the 'max' value for the bit-field
is without further digging.
Where the values are just predefined constants, you don't need to know
the 'max' value. For cases where the value is a macro parameter, I
ANDed the value with a bit-mask, although calling the macro with an
out-of-range value is a bad idea anyway!
I applied your whole series to see what the final header looks like. To me it
actually looks worse than the original.
The original had a number of whitespace issues that made it hard to follow and
the defines were lacking namespace. Personally I also don't can for all the enums
since the symbols are not actually used as enums just as raw values. But the 'bit'
usage of the registers was fairly clear.
With your series applied the whtespace and namespace issues are addressed.
You also converted all the enums to defines which is great. But the 'bit' usage
now is a bit muddled. I really don't care for the (BIT(n) * (x)) stuff. There are
also the various, unused and unnecessary, <foo>_SHIFT defines. Those just
add additional cruft.
The PLX_FOO_BAR_SHIFT defines are there to make it possible to extract
the field value from the register value using (reg_val &
PLX_FOO_BAR_MASK) >> PLX_FOO_BAR_SHIFT. I only did that when the field
value is parameterized in the PLX_FOO_BAR(x) macro. The alternative
would be to define PLX_FOO_TO_BAR(r) macros to do the same thing. I had
to do that anyway for the 'PAFL' field of the DMPBAM register due to its
unusual layout.
I'm also not sure if all the bits require a comment. They seem to clutter the
header. Datasheets for the PLX-9080 are easy to find. Maybe just have a
comment for each register and remove all the bit comments.
(You used to have to create an account on plxtech.com to download the
datasheets, but that is no longer necessary since it became part of Avago.)
Some of the abbreviations used in the macro names are a bit contrived
for brevity, so I think the comments help to pin them down in the
datasheet. Of course, the comments are no substitution for the actual
datasheet.
I have been looking for a solution to the problem where random people
change something like this:
#define MY_COOLREG_VAL_FOO (0 << 5)
#define MY_COOLREG_VAL_BAR (1 << 5)
#define MY_COOLREG_VAL_BAZ (2 << 5)
to:
#define MY_COOLREG_VAL_FOO (0 << 5)
#define MY_COOLREG_VAL_BAR BIT(5)
#define MY_COOLREG_VAL_BAZ (2 << 5)
and this seemed like one way to do it.
Like I stated previously, I prefer something like this for the multi-bit
fields of a register.
#define PLX_LASSR_MLOC(x) (((x) & 0x3) << 1)
#define PLX_LASSR_MLOC_ANY32 PLX_LASSR_MLOC(0)
#define PLX_LASSR_MLOC_LT1MB PLX_LASSR_MLOC(1)
#define PLX_LASSR_MLOC_ANY64 PLX_LASSR_MLOC(2)
#define PLX_LASSR_MLOC_MASK PLX_LASSR_MLOC(3)
It is handy when matching it up with the data sheet though. I have a
field that occupies bits 2 and 1. It also doesn't expose a fairly
useless PLX_LASRR_MLOC() macro to the user of the header file.
The (BIT(n) * (x)) just looks odd.
To be honest, I think the only benefit of using BIT(n) rather than 1 <<
n is that it forces some type consistency, particularly when the value
being shifted is a plain 'int' and there is some possibility of shifting
beyond bit 30.
With your way of doing it, you need to start adding type casts if the
field extends into bit 31.
The GENMASK() for a multi-bit field also makes it more difficult to
figure out what the maximum value for the field is when there are
more than just a few bits and the lower bit is not 0.
That's true, although in cases where it matters (where the value is
supplied as a parameter), the maximum value is in a bit-mask. One thing
about the GENMASK() macro is that it ties in nicely with how the
datasheet defines the multi-bit fields.
Anyway.. Technically it looks like your series doesn't break anything
I just don't feel that it adds much clarity.
I'm still looking it over... Maybe I'll change my mind... ;-)
Regards,
Hartley
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@xxxxxxxxx> )=-
-=( Web: http://www.mev.co.uk/ )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
