[PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Brian Norris]

Hi,

I included some comments along with some of you questions and tried to
summarize and answer more at the end. The end comments may explain the
middle...

On Wed, Jul 6, 2011 at 11:58 PM, Artem Bityutskiy <dedekind1 at gmail.com> wrote:
> On Wed, 2011-07-06 at 11:51 -0700, Brian Norris wrote:
>> It seems that, according to the following comment, the master MTD node
>> (which is passed to most of our functions) is never registered and so
>> does not have valid information for printing dev_* information:
>>      /* NOTE:  we don't arrange MTDs as a tree; it'd be error-prone
>>       * to have the same data be in two different partitions.
>>       */
>
> Sorry, I still don't get it, why mtd partitions do not have names. The
> discussion above is about who will be the parent, while we care about
> making dev_dbg(mtd->dev, ...) print names.

The *partitions* have names, but the master does not.

We end up with a device tree that's something like this:

     <physical_device>
     |_ mtd0 (slave->dev)
     |_ mtd0ro
     |_ mtd1
     |_ mtd1ro
     |_ ...
     ...
     master mtd->dev?

where each partition (and RO counterpart) in the tree is registered
using add_mtd_partitions() and each partition's parent is the physical
device. The master mtd->dev is floating in space, since it isn't
registered anywhere.

I think I meant to copy a different code comment onto the previous
e-mail as well:

     /* ...
      * We don't register the master, or expect the caller to have done so,
      * for reasons of data integrity.
      */

So the master mtd->dev, which is used in much of our MTD
initialization code, is never named and registered properly.

> if we look at 'add_mtd_device()' in mtdcore.c, we see something like
> full initialization:
>
>        /* Caller should have set dev.parent to match the
>         * physical device.
>         */
>        mtd->dev.type = &mtd_devtype;
>        mtd->dev.class = &mtd_class;
>        mtd->dev.devt = MTD_DEVT(i);
>        dev_set_name(&mtd->dev, "mtd%d", i);
>        dev_set_drvdata(&mtd->dev, mtd);
>        if (device_register(&mtd->dev) != 0)
>                goto fail_added;
>
>        if (MTD_DEVT(i))
>                device_create(&mtd_class, mtd->dev.parent,
>                              MTD_DEVT(i) + 1,
>                              NULL, "mtd%dro", i);

When called from mtd_add_partitions, this is only called for each
partition, not the master. That is the crux of the problem.

> And if we look at 'mtd_add_partition()', we see that 'add_mtd_device()'
> is also called, so mtd->dev should be initialized.
>
> What exactly is the problem?

I believe the problem is simply that partitions are named and
registered, whereas the master is not. During initialization, all
debug messages, etc. are run through the master MTD, not the named
partitions. This gives the "(null):" dev_* messages I saw on bootup.
I'm not sure if the master device causes much trouble after
initialization...I think it kinda fades away and leaves the partitions
for interaction with the user.

FYI, I'm looking mostly at cases where there *is* a partition layout.
In cases where there are no partitions, the driver will usually just
call "add_mtd_device" on the master MTD, and so this device be named
and registered properly. This isn't so much an issue, except that it
provides inconsistency between setups that use partitions and those
that don't. (Couldn't we just force everyone to use partitions? Users
who don't need them could just form a single partition for the whole
device...)

Brian