Re: [PATCH 04/20] staging: comedi: drivers: re-do macros for PLX PCI 9080 LASxRR values

[Ian Abbott]

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@xxxxxxxxx>
> > ---
>
> [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@xxxxxxxxx> )=-
-=(                          Web: http://www.mev.co.uk/  )=-