Messages in this thread | | | Subject | Re: [PATCH 04/20] staging: comedi: drivers: re-do macros for PLX PCI 9080 LASxRR values | From | Ian Abbott <> | Date | Fri, 20 May 2016 18:17:52 +0100 |
| |
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. >> >> Signed-off-by: Ian Abbott <abbotti@mev.co.uk> >> --- > > [snip] > >> diff --git a/drivers/staging/comedi/drivers/plx9080.h b/drivers/staging/comedi/drivers/plx9080.h >> index 92d2480..8788117 100644 >> --- a/drivers/staging/comedi/drivers/plx9080.h >> +++ b/drivers/staging/comedi/drivers/plx9080.h >> @@ -54,14 +54,16 @@ struct plx_dma_desc { >> /* Local Address Space 1 Range Register */ >> #define PLX_REG_LAS1RR 0x00f0 >> >> -#define LRNG_IO 0x00000001 /* Map to: 1=I/O, 0=Mem */ >> -#define LRNG_ANY32 0x00000000 /* Locate anywhere in 32 bit */ >> -#define LRNG_LT1MB 0x00000002 /* Locate in 1st meg */ >> -#define LRNG_ANY64 0x00000004 /* Locate anywhere in 64 bit */ >> -/* bits that specify range for memory io */ >> -#define LRNG_MEM_MASK 0xfffffff0 >> -/* bits that specify range for normal io */ >> -#define LRNG_IO_MASK 0xfffffffc >> +#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!
FWIW, all the constants end up with the same type (unsigned long) this way.
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.
> These bit define the memory space encoding. I would prefer something > like this: > > #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_LASRR_MLOC_MASK GENMASK(2, 1) /* Memory location bits */ > > I guess the GENMASK() macro is common but it's currently > not used by any of the comedi code.
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.
> > Using the macro above, the 'mask' would be: > > #define PLX_LASSR_MLOC_MASK PLX_LASSR_MLOC(3) > >> +#define PLX_LASRR_PREFETCH BIT(3) /* Memory is prefetchable */ >> +/* bits that specify range for memory space decode bits */ >> +#define PLX_LASRR_MEM_MASK GENMASK(31, 4) >> +/* bits that specify range for i/o space decode bits */ >> +#define PLX_LASRR_IO_MASK GENMASK(31, 2) > > I suppose the GENMASK() use makes sense for these. > > Regards, > Hartley >
-- -=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=- -=( Web: http://www.mev.co.uk/ )=-
| |