lkml.org 
[lkml]   [2016]   [May]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 04/20] staging: comedi: drivers: re-do macros for PLX PCI 9080 LASxRR values
From
Date
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/ )=-

\
 
 \ /
  Last update: 2016-05-20 19:41    [W:0.058 / U:0.516 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site