On Wed, Sep 19, 2007 at 07:14:33AM -0500, Josh Boyer wrote:
> On Wed, 19 Sep 2007 14:16:46 +1000
> David Gibson <[EMAIL PROTECTED]> wrote:
>
> > This patch includes a whole batch of smallish cleanups for
> > drivers/mtd/physmap_of.c.
> >
> > - A bunch of uneeded #includes are removed
> > - We switch to the modern linux/of.h etc. in place of
> > asm/prom.h
> > - Use some helper macros to avoid some ugly inline #ifdefs
> > - A few lines of unreachable code are removed
> > - A number of indentation / line-wrapping fixes
> > - More consistent use of kernel idioms such as if (!p) instead
> > of if (p == NULL)
> > - Clarify some printk()s and other informative strings.
>
> Most of that looks good. Just a couple issues below. Mostly it
> doesn't apply cleanly to my tree because you didn't base if off of the
> patch I sent out last week to fix optional partitions.
Ah, oops. Didn't remember that patch.
> > - (the big one) Despite the name, this driver really has
> > nothing to do with drivers/mtd/physmap.c. The fact that the flash
> > chips must be physically direct mapped is a constrant, but doesn't
> > really say anything about the actual purpose of this driver, which is
> > to instantiate MTD devices based on information from the device tree.
> > Therefore the physmap name is replaced everywhere within the file with
> > "of_flash". The file itself and the Kconfig option is not renamed for
> > now (so that the diff is actually a diff). That can come later.
>
> Later when? If we're all in agreement, then why don't we just apply
> your patch after you fix it up and I can move the file in my git tree.
> That way we don't forget about it.
Ok by me. Well, except that it will need Makefile and Kconfig changes
for that, too. I was going to send another patch with that once this
one was reviewed and merged.
[snip]
> > @@ -56,11 +53,9 @@ static int parse_obsolete_partitions(str
> >
> > nr_parts = plen / sizeof(part[0]);
> >
> > - info->parts = kzalloc(nr_parts * sizeof(struct mtd_partition),
> > GFP_KERNEL);
> > - if (!info->parts) {
> > - printk(KERN_ERR "Can't allocate the flash partition data!\n");
> > + info->parts = kzalloc(nr_parts * sizeof(*info->parts), GFP_KERNEL);
> > + if (!info->parts)
> > return -ENOMEM;
>
> You dropped the printk here. Any particular reason why?
I thought the the -ENOMEM was specific enough; we don't give
explanatory messages for other ENOMEM conditions in the driver.
> >
> > names = of_get_property(dp, "partition-names", &plen);
> >
> > @@ -86,8 +81,8 @@ static int parse_obsolete_partitions(str
> > return nr_parts;
> > }
> >
> > -static int __devinit process_partitions(struct physmap_flash_info *info,
> > - struct of_device *dev)
> > +static int __devinit parse_partitions(struct of_flash *info,
> > + struct of_device *dev)
> > {
> > const char *partname;
> > static const char *part_probe_types[]
> > @@ -109,87 +104,68 @@ static int __devinit process_partitions(
> > for (pp = dp->child; pp; pp = pp->sibling)
> > nr_parts++;
> >
> > - if (nr_parts) {
> > - info->parts = kzalloc(nr_parts * sizeof(struct mtd_partition),
> > - GFP_KERNEL);
> > - if (!info->parts) {
> > - printk(KERN_ERR "Can't allocate the flash partition
> > data!\n");
> > - return -ENOMEM;
> > - }
> > + if (nr_parts == 0)
> > + return parse_obsolete_partitions(dev, info, dp);
>
> You reintroduced the optional partitions bug I fixed with:
>
> http://git.infradead.org/?p=users/jwboyer/powerpc.git;a=commit;h=7cafc8f8c89d1f49039b7c345ca832fbd2d1e639
Ah yes. Actually on consideration, your patch seems a roundabout way
of fixing it. I'm inclined instead to change
parse_obsolete_partitions() to return 0 instead of -ENOENT.
I guess I'll fold that fix in.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
