[PATCH V2] mtd: bcm47part driver for BCM47XX chipsets

[Kevin Cernekee]

On Sat, Aug 25, 2012 at 11:50 AM, Rafał Miłecki <zajec5 at gmail.com> wrote:
> +config MTD_BCM47_PARTS
> +       tristate "BCM47XX partitioning support"
> +       depends on BCM47XX

You may want to change "bcm47" to "bcm47xx", for consistency with the
other bcm47xx/bcm63xx references found throughout the tree.

> +       u8 *buf;
[...]
> +       buf = kzalloc(sizeof(*buf) * BCM47PART_BYTES_TO_READ, GFP_KERNEL);

sizeof(u8) probably isn't needed.

> +               /* CFE has small NVRAM at 0x400 */
> +               fourcc = (u32 *)&buf[0x400];
> +               if (*fourcc == NVRAM_HEADER) {
> +                       parts[curr_part].name = "boot";
> +                       parts[curr_part].mask_flags = MTD_WRITEABLE;
> +                       parts[curr_part].offset = offset;
> +                       curr_part++;
> +               }
> +
> +               /* Standard NVRAM */
> +               fourcc = (u32 *)&buf[0x000];
> +               if (*fourcc == NVRAM_HEADER) {
> +                       parts[curr_part].name = "nvram";
> +                       parts[curr_part].mask_flags = MTD_WRITEABLE;
> +                       parts[curr_part].offset = offset;
> +                       curr_part++;
> +               }
> +
> +               /* board_data starts with board_id which differs across boards,
> +                * but we can use 'MPFR' (hopefully) magic at 0x100 */
> +               fourcc = (u32 *)&buf[0x100];
> +               if (*fourcc == 0x5246504D) { /* MPFR */
> +                       parts[curr_part].name = "board_data";
> +                       parts[curr_part].mask_flags = MTD_WRITEABLE;
> +                       parts[curr_part].offset = offset;
> +                       curr_part++;
> +               }
> +
> +               /* POT(TOP) */
> +               fourcc = (u32 *)&buf[0x000];
> +               fourcc2 = (u32 *)&buf[0x004];
> +               if (*fourcc == 0x54544f50 &&             /* POTT */
> +                   (*fourcc2 & 0xFFFF) == 0x504f) {     /* OP */
> +                       parts[curr_part].name = "POT";
> +                       parts[curr_part].mask_flags = MTD_WRITEABLE;
> +                       parts[curr_part].offset = offset;
> +                       curr_part++;
> +               }
> +
> +               /* ML */
> +               fourcc = (u32 *)&buf[0x010];
> +               fourcc2 = (u32 *)&buf[0x014];
> +               if (*fourcc == 0x39685a42 && *fourcc2 == 0x26594131) {

I would suggest using symbolic names for the constants, even if it's
just something like ML_MAGIC1.

> +                       parts[curr_part].name = "ML";
> +                       parts[curr_part].mask_flags = MTD_WRITEABLE;
> +                       parts[curr_part].offset = offset;
> +                       curr_part++;
> +               }

Are all of these cases mutually exclusive, given that they use the same offset?

It would be nice to reduce the amount of duplicated code if possible.
Maybe something as simple as:

char *name = NULL;
if (some_condition)
    name = "POT";
else if (some_other_condition)
    name = "ML";
[...]

if (name) {
    parts[curr_part].name = name;
    parts[curr_part].mask_flags = MTD_WRITEABLE;
    parts[curr_part].offset = offset;
    curr_part++;
}


Can the contents of buf[] be represented in a struct?