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

[Ian Abbott]

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