Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1404448 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 04/20] staging: comedi: drivers: re-do macros for PLX PCI 9080 LASxRR values Ian Abbott <abbotti@mev.co.uk> - 2016-05-20 15:50 +0200
    RE: [PATCH 04/20] staging: comedi: drivers: re-do macros for PLX PCI  9080 LASxRR values Hartley Sweeten <HartleyS@visionengravers.com> - 2016-05-20 18:40 +0200
      Re: [PATCH 04/20] staging: comedi: drivers: re-do macros for PLX PCI  9080 LASxRR values Ian Abbott <abbotti@mev.co.uk> - 2016-05-20 19:20 +0200
        RE: [PATCH 04/20] staging: comedi: drivers: re-do macros for PLX PCI  9080 LASxRR values Hartley Sweeten <HartleyS@visionengravers.com> - 2016-05-20 20:10 +0200
          Re: [PATCH 04/20] staging: comedi: drivers: re-do macros for PLX PCI  9080 LASxRR values Ian Abbott <abbotti@mev.co.uk> - 2016-05-23 13:30 +0200

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

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>
---
 drivers/staging/comedi/drivers/cb_pcidas64.c |  4 ++--
 drivers/staging/comedi/drivers/plx9080.h     | 18 ++++++++++--------
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c b/drivers/staging/comedi/drivers/cb_pcidas64.c
index d9468db..59d81e8 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -4004,13 +4004,13 @@ static int auto_attach(struct comedi_device *dev,
 
 	/*  figure out what local addresses are */
 	local_range = readl(devpriv->plx9080_iobase + PLX_REG_LAS0RR) &
-		      LRNG_MEM_MASK;
+		      PLX_LASRR_MEM_MASK;
 	local_decode = readl(devpriv->plx9080_iobase + PLX_REG_LAS0BA) &
 		       local_range & LMAP_MEM_MASK;
 	devpriv->local0_iobase = ((uint32_t)devpriv->main_phys_iobase &
 				  ~local_range) | local_decode;
 	local_range = readl(devpriv->plx9080_iobase + PLX_REG_LAS1RR) &
-		      LRNG_MEM_MASK;
+		      PLX_LASRR_MEM_MASK;
 	local_decode = readl(devpriv->plx9080_iobase + PLX_REG_LAS1BA) &
 		       local_range & LMAP_MEM_MASK;
 	devpriv->local1_iobase = ((uint32_t)devpriv->dio_counter_phys_iobase &
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 */
+#define PLX_LASRR_MLOC_MASK	GENMASK(2, 1)	/* Memory location bits */
+#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)
 
 /* Local Address Space 0 Local Base Address (Remap) Register */
 #define PLX_REG_LAS0BA		0x0004
-- 
2.8.1

[toc] | [next] | [standalone]

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

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.

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.

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

[toc] | [prev] | [next] | [standalone]

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

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/  )=-

[toc] | [prev] | [next] | [standalone]

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

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.

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.

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.

> 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.

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.

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web