[PATCH V2] mtd: bcm47part driver for BCM47XX chipsets

[Rafał Miłecki]

Thanks for comments!

2012/8/26 Kevin Cernekee <cernekee at gmail.com>:
> 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.

Personally I prefer that "bcm47" over "bcm47xx". 2 minor advantages for me:
1) It's shorted but still unique
2) We have chars after digits in it's name, it's easier to read than
"xxpart" or "xxnflash"

David: AFAIK you're the maintainer of mtd? Could you share your opinion?

I won't argue if you guys prefer "bcm47xx" :)


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

> sizeof(u8) probably isn't needed.

I hoped it will make reading the code easier. Multiplying should be
optimized by compiler. But can drop this if you think it's not worth
it.


>> +               /* 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.

Agree.


>> +                       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?

Yes.


> 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++;
> }

Ouch, you just reminded me I've forgotten to drop mask_flags in few
places. Masking MTD_WRITEABLE is needed for only ~half of our cases.
So if we follow your way now, we have to pass two arguments to the
generic partition handler: "name" and "mask_flags". But maybe I can
use some additional function for that? It should de-duplicate some
code and should work even for code inside "trx" parser.


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

Theoretically yes, but I think it doesn't make much sense. It's for
detecting type of partition, so it wouldn't have nice struct. It
usually would have only one valid field per type. So it would be
mostly like
struct partitions_ids {
    union {
        u32 possible_nvram_fourcc;
        u32 possible_pot_fourcc;
        u32 possible_trx_fourcc;
    },
    u32 possible_pot_fourcc2;
    u32 pad[2]
    u32 possible_ml_fourcc;
    u32 possible_ml_fourcc2;
}

I really don't like idea of such a struct.

-- 
Rafał