Cleanups for physmap_of.c

Cleanups for physmap_of.c

[David Gibson]

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.	
	- (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.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Index: working-2.6/drivers/mtd/maps/physmap_of.c
===================================================================
--- working-2.6.orig/drivers/mtd/maps/physmap_of.c	2007-09-14 14:24:06.000000000 +1000
+++ working-2.6/drivers/mtd/maps/physmap_of.c	2007-09-19 13:59:23.000000000 +1000
@@ -1,5 +1,5 @@
 /*
- * Normal mappings of chips in physical memory for OF devices
+ * Flash mappings described by the OF (or flattened) device tree
  *
  * Copyright (C) 2006 MontaVista Software Inc.
  * Author: Vitaly Wool <vwool@ru.mvista.com>
@@ -15,20 +15,15 @@
 
 #include <linux/module.h>
 #include <linux/types.h>
-#include <linux/kernel.h>
 #include <linux/init.h>
-#include <linux/slab.h>
 #include <linux/device.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/map.h>
 #include <linux/mtd/partitions.h>
-#include <linux/mtd/physmap.h>
-#include <asm/io.h>
-#include <asm/prom.h>
-#include <asm/of_device.h>
-#include <asm/of_platform.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
 
-struct physmap_flash_info {
+struct of_flash {
 	struct mtd_info		*mtd;
 	struct map_info		map;
 	struct resource		*res;
@@ -38,8 +33,10 @@ struct physmap_flash_info {
 };
 
 #ifdef CONFIG_MTD_PARTITIONS
+#define OF_FLASH_PARTS(info)	((info)->parts)
+
 static int parse_obsolete_partitions(struct of_device *dev,
-				     struct physmap_flash_info *info,
+				     struct of_flash *info,
 				     struct device_node *dp)
 {
 	int i, plen, nr_parts;
@@ -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;
-	}
 
 	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);
 
-		for (pp = dp->child, i = 0 ; pp; pp = pp->sibling, i++) {
-			const u32 *reg;
-			int len;
-
-			reg = of_get_property(pp, "reg", &len);
-			if (!reg || (len != 2*sizeof(u32))) {
-				dev_err(&dev->dev, "Invalid 'reg' on %s\n",
-					dp->full_name);
-				kfree(info->parts);
-				info->parts = NULL;
-				return -EINVAL;
-			}
-			info->parts[i].offset = reg[0];
-			info->parts[i].size = reg[1];
-
-			partname = of_get_property(pp, "label", &len);
-			if (!partname)
-				partname = of_get_property(pp, "name", &len);
-			info->parts[i].name = (char *)partname;
+	info->parts = kzalloc(nr_parts * sizeof(*info->parts),
+			      GFP_KERNEL);
+	if (!info->parts)
+		return -ENOMEM;
 
-			if (of_get_property(pp, "read-only", &len))
-				info->parts[i].mask_flags = MTD_WRITEABLE;
+	for (pp = dp->child, i = 0; pp; pp = pp->sibling, i++) {
+		const u32 *reg;
+		int len;
+
+		reg = of_get_property(pp, "reg", &len);
+		if (!reg || (len != 2*sizeof(u32))) {
+			dev_err(&dev->dev, "Invalid 'reg' on %s\n",
+				dp->full_name);
+			kfree(info->parts);
+			info->parts = NULL;
+			return -EINVAL;
 		}
-	} else {
-		nr_parts = parse_obsolete_partitions(dev, info, dp);
-	}
+		info->parts[i].offset = reg[0];
+		info->parts[i].size = reg[1];
 
-	if (nr_parts < 0)
-		return nr_parts;
+		partname = of_get_property(pp, "label", &len);
+		if (!partname)
+			partname = of_get_property(pp, "name", &len);
+		info->parts[i].name = (char *)partname;
 
-	if (nr_parts > 0)
-		add_mtd_partitions(info->mtd, info->parts, nr_parts);
-	else
-		add_mtd_device(info->mtd);
+		if (of_get_property(pp, "read-only", &len))
+			info->parts[i].mask_flags = MTD_WRITEABLE;
+	}
 
-	return 0;
+	return nr_parts;
 }
 #else /* MTD_PARTITIONS */
-static int __devinit process_partitions(struct physmap_flash_info *info,
-					struct device_node *dev)
-{
-	add_mtd_device(info->mtd);
-	return 0;
-}
+#define	OF_FLASH_PARTS(info)		(0)
+#define parse_partitions(info, dev)	(0)
 #endif /* MTD_PARTITIONS */
 
-static int of_physmap_remove(struct of_device *dev)
+static int of_flash_remove(struct of_device *dev)
 {
-	struct physmap_flash_info *info;
+	struct of_flash *info;
 
 	info = dev_get_drvdata(&dev->dev);
-	if (info == NULL)
+	if (!info)
 		return 0;
 	dev_set_drvdata(&dev->dev, NULL);
 
-	if (info->mtd != NULL) {
-#ifdef CONFIG_MTD_PARTITIONS
-		if (info->parts) {
+	if (info->mtd) {
+		if (OF_FLASH_PARTS(info)) {
 			del_mtd_partitions(info->mtd);
-			kfree(info->parts);
+			kfree(OF_FLASH_PARTS(info));
 		} else {
 			del_mtd_device(info->mtd);
 		}
-#else
-		del_mtd_device(info->mtd);
-#endif
 		map_destroy(info->mtd);
 	}
 
-	if (info->map.virt != NULL)
+	if (info->map.virt)
 		iounmap(info->map.virt);
 
-	if (info->res != NULL) {
+	if (info->res) {
 		release_resource(info->res);
 		kfree(info->res);
 	}
@@ -227,52 +203,49 @@ static struct mtd_info * __devinit obsol
 		return do_map_probe("jedec_probe", map);
 	} else {
 		if (strcmp(of_probe, "ROM") != 0)
-			dev_dbg(&dev->dev, "obsolete_probe: don't know probe type "
-				"'%s', mapping as rom\n", of_probe);
+			dev_warn(&dev->dev, "obsolete_probe: don't know probe "
+				 "type '%s', mapping as rom\n", of_probe);
 		return do_map_probe("mtd_rom", map);
 	}
 }
 
-static int __devinit of_physmap_probe(struct of_device *dev, const struct of_device_id *match)
+static int __devinit of_flash_probe(struct of_device *dev,
+				    const struct of_device_id *match)
 {
 	struct device_node *dp = dev->node;
 	struct resource res;
-	struct physmap_flash_info *info;
-	const char *probe_type = (const char *)match->data;
+	struct of_flash *info;
+	const char *probe_type = match->data;
 	const u32 *width;
 	int err;
 
+	err = -ENXIO;
 	if (of_address_to_resource(dp, 0, &res)) {
-		dev_err(&dev->dev, "Can't get the flash mapping!\n");
-		err = -EINVAL;
+		dev_err(&dev->dev, "Can't get IO address from device tree\n");
 		goto err_out;
 	}
 
-       	dev_dbg(&dev->dev, "physmap flash device: %.8llx at %.8llx\n",
-	    (unsigned long long)res.end - res.start + 1,
-	    (unsigned long long)res.start);
-
-	info = kzalloc(sizeof(struct physmap_flash_info), GFP_KERNEL);
-	if (info == NULL) {
-		err = -ENOMEM;
+       	dev_dbg(&dev->dev, "of_flash device: %.8llx-%.8llx\n",
+		(unsigned long long)res.start, (unsigned long long)res.end);
+
+	err = -ENOMEM;
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
 		goto err_out;
-	}
 	memset(info, 0, sizeof(*info));
 
 	dev_set_drvdata(&dev->dev, info);
 
+	err = -EBUSY;
 	info->res = request_mem_region(res.start, res.end - res.start + 1,
-			dev->dev.bus_id);
-	if (info->res == NULL) {
-		dev_err(&dev->dev, "Could not reserve memory region\n");
-		err = -ENOMEM;
+				       dev->dev.bus_id);
+	if (!info->res)
 		goto err_out;
-	}
 
+	err = -ENXIO;
 	width = of_get_property(dp, "bank-width", NULL);
-	if (width == NULL) {
-		dev_err(&dev->dev, "Can't get the flash bank width!\n");
-		err = -EINVAL;
+	if (!width) {
+		dev_err(&dev->dev, "Can't get bank width from device tree\n");
 		goto err_out;
 	}
 
@@ -281,10 +254,10 @@ static int __devinit of_physmap_probe(st
 	info->map.size = res.end - res.start + 1;
 	info->map.bankwidth = *width;
 
+	err = -ENOMEM;
 	info->map.virt = ioremap(info->map.phys, info->map.size);
-	if (info->map.virt == NULL) {
-		dev_err(&dev->dev, "Failed to ioremap flash region\n");
-		err = EIO;
+	if (!info->map.virt) {
+		dev_err(&dev->dev, "Failed to ioremap() flash region\n");
 		goto err_out;
 	}
 
@@ -295,25 +268,30 @@ static int __devinit of_physmap_probe(st
 	else
 		info->mtd = obsolete_probe(dev, &info->map);
 
-	if (info->mtd == NULL) {
-		dev_err(&dev->dev, "map_probe failed\n");
-		err = -ENXIO;
+	err = -ENXIO;
+	if (!info->mtd) {
+		dev_err(&dev->dev, "do_map_probe() failed\n");
 		goto err_out;
 	}
 	info->mtd->owner = THIS_MODULE;
 
-	return process_partitions(info, dev);
+	err = parse_partitions(info, dev);
+	if (err < 0)
+		goto err_out;
 
-err_out:
-	of_physmap_remove(dev);
-	return err;
+	if (err > 0)
+		add_mtd_partitions(info->mtd, OF_FLASH_PARTS(info), err);
+	else
+		add_mtd_device(info->mtd);
 
 	return 0;
 
-
+err_out:
+	of_flash_remove(dev);
+	return err;
 }
 
-static struct of_device_id of_physmap_match[] = {
+static struct of_device_id of_flash_match[] = {
 	{
 		.compatible	= "cfi-flash",
 		.data		= (void *)"cfi_probe",
@@ -335,30 +313,28 @@ static struct of_device_id of_physmap_ma
 	},
 	{ },
 };
+MODULE_DEVICE_TABLE(of, of_flash_match);
 
-MODULE_DEVICE_TABLE(of, of_physmap_match);
-
-
-static struct of_platform_driver of_physmap_flash_driver = {
-	.name		= "physmap-flash",
-	.match_table	= of_physmap_match,
-	.probe		= of_physmap_probe,
-	.remove		= of_physmap_remove,
+static struct of_platform_driver of_flash_driver = {
+	.name		= "of-flash",
+	.match_table	= of_flash_match,
+	.probe		= of_flash_probe,
+	.remove		= of_flash_remove,
 };
 
-static int __init of_physmap_init(void)
+static int __init of_flash_init(void)
 {
-	return of_register_platform_driver(&of_physmap_flash_driver);
+	return of_register_platform_driver(&of_flash_driver);
 }
 
-static void __exit of_physmap_exit(void)
+static void __exit of_flash_exit(void)
 {
-	of_unregister_platform_driver(&of_physmap_flash_driver);
+	of_unregister_platform_driver(&of_flash_driver);
 }
 
-module_init(of_physmap_init);
-module_exit(of_physmap_exit);
+module_init(of_flash_init);
+module_exit(of_flash_exit);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Vitaly Wool <vwool@ru.mvista.com>");
-MODULE_DESCRIPTION("Configurable MTD map driver for OF");
+MODULE_DESCRIPTION("Device tree based MTD map driver");

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

Re: Cleanups for physmap_of.c

[Josh Boyer]

On Wed, 19 Sep 2007 14:16:46 +1000
David Gibson <david@gibson.dropbear.id.au> 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.

> 	- (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.

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Index: working-2.6/drivers/mtd/maps/physmap_of.c
> ===================================================================
> --- working-2.6.orig/drivers/mtd/maps/physmap_of.c	2007-09-14 14:24:06.000000000 +1000
> +++ working-2.6/drivers/mtd/maps/physmap_of.c	2007-09-19 13:59:23.000000000 +1000
> @@ -1,5 +1,5 @@
>  /*
> - * Normal mappings of chips in physical memory for OF devices
> + * Flash mappings described by the OF (or flattened) device tree
>   *
>   * Copyright (C) 2006 MontaVista Software Inc.
>   * Author: Vitaly Wool <vwool@ru.mvista.com>
> @@ -15,20 +15,15 @@
> 
>  #include <linux/module.h>
>  #include <linux/types.h>
> -#include <linux/kernel.h>
>  #include <linux/init.h>
> -#include <linux/slab.h>
>  #include <linux/device.h>
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/map.h>
>  #include <linux/mtd/partitions.h>
> -#include <linux/mtd/physmap.h>
> -#include <asm/io.h>
> -#include <asm/prom.h>
> -#include <asm/of_device.h>
> -#include <asm/of_platform.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> 
> -struct physmap_flash_info {
> +struct of_flash {
>  	struct mtd_info		*mtd;
>  	struct map_info		map;
>  	struct resource		*res;
> @@ -38,8 +33,10 @@ struct physmap_flash_info {
>  };
> 
>  #ifdef CONFIG_MTD_PARTITIONS
> +#define OF_FLASH_PARTS(info)	((info)->parts)
> +
>  static int parse_obsolete_partitions(struct of_device *dev,
> -				     struct physmap_flash_info *info,
> +				     struct of_flash *info,
>  				     struct device_node *dp)
>  {
>  	int i, plen, nr_parts;
> @@ -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?

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

josh

Re: Cleanups for physmap_of.c

[David Gibson]

On Wed, Sep 19, 2007 at 07:14:33AM -0500, Josh Boyer wrote:
> On Wed, 19 Sep 2007 14:16:46 +1000
> David Gibson <david@gibson.dropbear.id.au> 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