Re: [PATCH 1/2] MIPS: OCTEON: Add register definitions for SPI host hardware.

[Linus Walleij <linus.walleij@linaro.org>]

On Fri, May 11, 2012 at 11:34 PM, David Daney <ddaney.cavm@gmail.com> wrote:

> From: David Daney <david.daney@cavium.com>
>
> Needed by SPI driver.

That's not very verbose, plese tell atleast tell which SPI driver it's for.

> +union cvmx_mpi_cfg {
> +       uint64_t u64;

The kernel already has a type (in <linux/kernel.h>) used in many places in the
kernel, called "u64" so this gets very confusing.

Can you call it something else?

BTW: you could then s/uint64_t/u64 and use that u64.
(Some don't like the three-letter types so no big deal.)

> +       struct cvmx_mpi_cfg_s {
> +#ifdef __BIG_ENDIAN_BITFIELD
> +               uint64_t reserved_29_63:35;
> +               uint64_t clkdiv:13;
> +               uint64_t csena3:1;
> +               uint64_t csena2:1;
> +               uint64_t csena1:1;
> +               uint64_t csena0:1;
> +               uint64_t cslate:1;
> +               uint64_t tritx:1;
> +               uint64_t idleclks:2;
> +               uint64_t cshi:1;
> +               uint64_t csena:1;
> +               uint64_t int_ena:1;
> +               uint64_t lsbfirst:1;
> +               uint64_t wireor:1;
> +               uint64_t clk_cont:1;
> +               uint64_t idlelo:1;
> +               uint64_t enable:1;
> +#else
> +               uint64_t enable:1;
> +               uint64_t idlelo:1;
> +               uint64_t clk_cont:1;
> +               uint64_t wireor:1;
> +               uint64_t lsbfirst:1;
> +               uint64_t int_ena:1;
> +               uint64_t csena:1;
> +               uint64_t cshi:1;
> +               uint64_t idleclks:2;
> +               uint64_t tritx:1;
> +               uint64_t cslate:1;
> +               uint64_t csena0:1;
> +               uint64_t csena1:1;
> +               uint64_t csena2:1;
> +               uint64_t csena3:1;
> +               uint64_t clkdiv:13;
> +               uint64_t reserved_29_63:35;
> +#endif

This boggles my mind, but I see there are many drivers doing this,
but using uint64_t looks overly scary.

Can't you break it apart using a set of u32's like in
drivers/net/ethernet/micrel/ksz884x.c?

Yours,
Linus Walleij