Coding style question

Coding style question

[Pavel Pisa]

Hello All,

I have question if next style of macros definitions
for hardware registers is acceptable (tastefull for
maintainers) for Linux kernel.

/* Register offset against peripheral base */
#define SUBSYSTEM_REGISTER_o 0x00000

/* The register field mask */
#define REGISTER_FUNCTION_m 0x00000

/* The register field starting bit */
#define REGISTER_FUNCTION_b 0x00

I am used to use this over many of our embedded
targets mainly developed without operating system.
I would like to use this for Linux drivers as well.

The naming convention has advantage, that it
prevents mistakes, when mask value (*_m) is used
in bit set/clear function without notice instead
of bitnumber (*_b).

The SUBSYSTEM_REGISTER -> REGISTER_FUNCTION
prevents mistakes, when field defined for one register
is used with incorrect register by mistake.

I would like to use this style in i.MX SDHC to
allow support both controllers on MX21.
I would like to use it in other areas as well.

There are tightly copled two macros for preparation
and acquisition of muti-bit masked fields values

#define __val2mfld(mask,val) (((mask)&~((mask)<<1))*(val)&(mask))

#define __mfld2val(mask,val) (((val)&(mask))/((mask)&~((mask)<<1)))

__raw_writel(REGISTER_SINGLEBIT_m |
             __val2mfld(REGISTER_MULTIBIT_m,value),
             periph_base + SUBSYSTEM_REGISTER_o)

x = __mfld2val(REGISTER_MULTIBIT_m,
           _raw_readl(periph_base + SUBSYSTEM_REGISTER_o))

The macros seems to be complicated, but they generate optimal
code for constant fields masks. I am not aware, that there
is some commonly available alternative in Linux kernel
header files.

Suggestions, names corrections etc. are welcomed.
If you think, that it is not good idea to introduce
yet another style, I would try to follow actual style
found in each source file. If you prefer some already
utilized style, direct me to right examples, please.

Best wishes

              Pavel Pisa

Re: Coding style question

[Jesper Juhl]

On 09/02/07, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
> Hello All,
>
> I have question if next style of macros definitions
> for hardware registers is acceptable (tastefull for
> maintainers) for Linux kernel.
>

It is generally preferred to keep macro names all uppercase.
Also, when possible, static inline functions are preferred over macros.

[...snip...]
> found in each source file. If you prefer some already
> utilized style, direct me to right examples, please.
>
Documentation/CodingStyle : Chapter 12: Macros, Enums and RTL

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: Coding style question

[Stefan Richter]

Pavel Pisa wrote:
> There are tightly copled two macros for preparation
> and acquisition of muti-bit masked fields values
> 
> #define __val2mfld(mask,val) (((mask)&~((mask)<<1))*(val)&(mask))
> 
> #define __mfld2val(mask,val) (((val)&(mask))/((mask)&~((mask)<<1)))

The macro names are awkward.
http://lxr.linux.no/source/Documentation/CodingStyle?v=2.6.18#L133

Consider longer names (and to partially counteract column consumption,
omit the leading underscores) or at least spell their purpose out in a
short comment at the macro definitions.
-- 
Stefan Richter
-=====-=-=== --=- -=--=
http://arcgraph.de/sr/

Re: Coding style question

[Haavard Skinnemoen]

On 2/9/07, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:

> #define __val2mfld(mask,val) (((mask)&~((mask)<<1))*(val)&(mask))
>
> #define __mfld2val(mask,val) (((val)&(mask))/((mask)&~((mask)<<1)))

Looks a bit similar to the style I tend to use a lot:

/* Bit manipulation macros */
#define MACB_BIT(name)                                  \
        (1 << MACB_##name##_OFFSET)
#define MACB_BF(name,value)                             \
        (((value) & ((1 << MACB_##name##_SIZE) - 1))    \
         << MACB_##name##_OFFSET)
#define MACB_BFEXT(name,value)\
        (((value) >> MACB_##name##_OFFSET)              \
         & ((1 << MACB_##name##_SIZE) - 1))
#define MACB_BFINS(name,value,old)                      \
        (((old) & ~(((1 << MACB_##name##_SIZE) - 1)     \
                    << MACB_##name##_OFFSET))           \
         | MACB_BF(name,value))

where BF stands for bitfield, EXT for extract and INS for insert.

The macros are butt ugly, but code using them is hopefully quite easy
to read (I'm of course not qualified to judge code I wrote myself.)
The somewhat excessive pasting ensures that if you ever switch the
name and value arguments, the compiler will let you know.

Example usage:

macb_writel(bp, REG, MACB_BF(FIELD, value));
regval = macb_readl(bp, REG);
value = MACB_BFEXT(FIELD, regval);

Haavard