Hi Grant, thanks a lot for the review!
On Tue, May 19, 2009 at 08:57:15AM -0600, Grant Likely wrote: > On Wed, Jan 21, 2009 at 11:41 AM, Wolfram Sang <w.s...@pengutronix.de> wrote: > > Create an of-aware driver using the now exported generic functions from > > plat-ram.c. A typical oftree snipplet for it looks like this: > > > > s...@04e00000 { > > compatible = "mtd-ram"; > > reg = <0x04e00000 0x00200000>; > > bank-width = <2>; > > }; > > > > Partitions on this device are not yet supported. > > This is a new device tree binding. It must be documented in > Documentation/powerpc/dts-bindings and posted to > devicetree-disc...@ozlabs.org for review. Yeah, noticed that myself. I wanted to start another try to mainline this driver next week, then also asking devicetree-discuss; will do it happily this week, too :) > > > diff --git a/drivers/mtd/maps/Makefile b/drivers/mtd/maps/Makefile > > index 6d9ba35..f8e3908 100644 > > --- a/drivers/mtd/maps/Makefile > > +++ b/drivers/mtd/maps/Makefile > > @@ -58,6 +58,7 @@ obj-$(CONFIG_MTD_WRSBC8260) += wr_sbc82xx_flash.o > > obj-$(CONFIG_MTD_DMV182) += dmv182.o > > obj-$(CONFIG_MTD_SHARP_SL) += sharpsl-flash.o > > obj-$(CONFIG_MTD_PLATRAM) += plat-ram.o > > +obj-$(CONFIG_MTD_OFRAM) += of-ram.o > > obj-$(CONFIG_MTD_OMAP_NOR) += omap_nor.o > > obj-$(CONFIG_MTD_INTEL_VR_NOR) += intel_vr_nor.o > > obj-$(CONFIG_MTD_BFIN_ASYNC) += bfin-async-flash.o > > diff --git a/drivers/mtd/maps/of-ram.c b/drivers/mtd/maps/of-ram.c > > new file mode 100644 > > index 0000000..4d334a9 > > --- /dev/null > > +++ b/drivers/mtd/maps/of-ram.c > > @@ -0,0 +1,120 @@ > > +/* > > + * drivers/mtd/maps/of-ram.c > > + * > > + * Generic of device based RAM map > > + * > > + * Copyright (C) 2009 Wolfram Sang, Pengutronix > > + * > > + * Using plat-ram.c by Ben Dooks > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms of the GNU General Public License version 2 as > > published by > > + * the Free Software Foundation. > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/types.h> > > +#include <linux/init.h> > > +#include <linux/kernel.h> > > +#include <linux/device.h> > > +#include <linux/platform_device.h> > > +#include <linux/of_device.h> > > +#include <linux/of_platform.h> > > +#include <linux/mtd/plat-ram.h> > > + > > +static int of_ram_probe(struct of_device *op, const struct of_device_id > > *match) > > __devinit Oops, missed all of those. > > > +{ > > + struct platdata_mtd_ram *pdata; > > + struct resource *resource; > > + const char *name; > > + const u32 *bankwidth; > > + int ret = -ENOMEM; > > + > > + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL); > > + if (!pdata) > > + goto bad0; > > + op->dev.platform_data = pdata; > > + > > + name = of_get_property(op->node, "name", NULL); > > + if (!name) { > > + ret = -ENOENT; > > + dev_dbg(&op->dev, "could not get node name\n"); > > + goto bad1; > > + } > > + > > + resource = kzalloc(sizeof(struct resource), GFP_KERNEL); > > + if (!resource) > > + goto bad1; > > Why isn't resource on the stack? Point taken. > > > + > > + if (of_address_to_resource(op->node, 0, resource)) { > > + ret = -ENXIO; > > + dev_dbg(&op->dev, "could not create resource\n"); > > + goto bad2; > > + } > > + > > + bankwidth = of_get_property(op->node, "bank-width", NULL); > > + if (*bankwidth == 0) { > > + ret = -ENOENT; > > + dev_dbg(&op->dev, "bank width not set\n"); > > + goto bad2; > > + } > > + pdata->bankwidth = *bankwidth; > > + > > + ret = __platram_probe(&op->dev, name, resource, pdata); > > + kfree(resource); > > + > > + if (ret) > > + goto bad1; > > + > > + return 0; > > + > > + bad2: > > + kfree(resource); > > + bad1: > > + op->dev.platform_data = NULL; > > + kfree(pdata); > > + bad0: > > + return ret; > > I'd rather see more meaningful labels. ie "err_kalloc:" and > "err_platram_probe:". Okay. > > > +} > > + > > +static int of_ram_remove(struct of_device *op) > > __devexit > > > +{ > > + int ret; > > + struct platdata_mtd_ram *pdata = op->dev.platform_data; > > + > > + ret = __platram_remove(&op->dev); > > + op->dev.platform_data = NULL; > > + kfree(pdata); > > + return ret; > > +} > > + > > +static struct of_device_id of_ram_match[] = { > > + { .compatible = "mtd-ram", }, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, of_ram_match); > > + > > +static struct of_platform_driver of_ram_driver = { > > __devinitdata > > > + .owner = THIS_MODULE, > > + .name = "of-ram", > > This name is too generic for an of_platform device. 'mtd' needs to be > in there somewhere. Will "of-mtd-ram" do? (I'd think so) > > > + .match_table = of_ram_match, > > + .probe = of_ram_probe, > > + .remove = of_ram_remove, > > __devexit_p() > > > +}; > > + > > +static int __init of_ram_init(void) > > +{ > > + return of_register_platform_driver(&of_ram_driver); > > +} > > + > > +static void __exit of_ram_exit(void) > > +{ > > + of_unregister_platform_driver(&of_ram_driver); > > +} > > + > > +module_init(of_ram_init); > > Put the module_init() statement directly after the of_ram_init() definition. > > > +module_exit(of_ram_exit); > > + > > +MODULE_AUTHOR("Wolfram Sang"); > > +MODULE_DESCRIPTION("MTD OF RAM map driver"); > > +MODULE_LICENSE("GPL v2"); > > -- > > 1.5.6.5 > > > > _______________________________________________ > > Linuxppc-dev mailing list > > Linuxppc-dev@ozlabs.org > > https://ozlabs.org/mailman/listinfo/linuxppc-dev > > > > > > -- > Grant Likely, B.Sc., P.Eng. > Secret Lab Technologies Ltd. Will send V2 after some tests tomorrow... Thanks, Wolfram -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ |
signature.asc
Description: Digital signature
_______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev