[PATCH] eXcite nand flash driver

[PATCH] eXcite nand flash driver

[Thomas Koeller]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 10036 bytes --]

This is a nand flash driver for the eXcite series of intelligent
cameras manufactured by Basler Vision Technologies AG.

Signed-off-by: Thomas Koeller <thomas.koeller@baslerweb.com>
---
 drivers/mtd/nand/Kconfig            |   18 +++-
 drivers/mtd/nand/Makefile           |    1 +
 drivers/mtd/nand/excite_nandflash.c |  259 
+++++++++++++++++++++++++++++++++++
 3 files changed, 277 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 358f55a..5b50396 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -216,10 +216,26 @@ config MTD_NAND_DISKONCHIP_BBTWRITE
 	  Even if you leave this disabled, you can enable BBT writes at module
 	  load time (assuming you build diskonchip as a module) with the module
 	  parameter "inftl_bbt_write=1".
-
+	  
 config MTD_NAND_SHARPSL
 	tristate "Support for NAND Flash on Sharp SL Series (C7xx + others)"
 	depends on MTD_NAND && ARCH_PXA
+ 
+config MTD_NAND_BASLER_EXCITE
+	tristate  "Support for NAND Flash on Basler eXcite"
+	depends on MTD_NAND && BASLER_EXCITE
+	help
+          This enables the driver for the NAND flash device found on the
+          Basler eXcite Smart Camera. If built as a module, the driver
+	  will be named "excite_nandflash.ko".
+
+config MTD_NAND_BASLER_EXCITE
+	tristate  "Support for NAND Flash on Basler eXcite"
+	depends on MTD_NAND && BASLER_EXCITE
+	help
+          This enables the driver for the NAND flash device found on the
+          Basler eXcite Smart Camera. If built as a module, the driver
+	  will be named "excite_nandflash.ko".
 
 config MTD_NAND_CAFE
        tristate "NAND support for OLPC CAFÉ chip"
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index f7a53f0..80f1dfc 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_MTD_NAND_NANDSIM)		+= nands
 obj-$(CONFIG_MTD_NAND_CS553X)		+= cs553x_nand.o
 obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
 obj-$(CONFIG_MTD_NAND_AT91)		+= at91_nand.o
+obj-$(CONFIG_MTD_NAND_BASLER_EXCITE)	+= excite_nandflash.o
 
 nand-objs := nand_base.o nand_bbt.o
 cafe_nand-objs := cafe.o cafe_ecc.o
diff --git a/drivers/mtd/nand/excite_nandflash.c 
b/drivers/mtd/nand/excite_nandflash.c
new file mode 100644
index 0000000..d683659
--- /dev/null
+++ b/drivers/mtd/nand/excite_nandflash.c
@@ -0,0 +1,259 @@
+/*
+*  Copyright (C) 2005 - 2007 by Basler Vision Technologies AG
+*  Author: Thomas Koeller <thomas.koeller.qbaslerweb.com>
+*  Original code by Thies Moeller <thies.moeller@baslerweb.com>
+*
+*  This program is free software; you can redistribute it and/or modify
+*  it under the terms of the GNU General Public License as published by
+*  the Free Software Foundation; either version 2 of the License, or
+*  (at your option) any later version.
+*
+*  This program is distributed in the hope that it will be useful,
+*  but WITHOUT ANY WARRANTY; without even the implied warranty of
+*  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+*  GNU General Public License for more details.
+*
+*  You should have received a copy of the GNU General Public License
+*  along with this program; if not, write to the Free Software
+*  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+*/
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/ioport.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/nand_ecc.h>
+#include <linux/mtd/partitions.h>
+
+#include <asm/io.h>
+#include <asm/rm9k-ocd.h>
+
+#include <excite_nandflash.h>
+
+#define EXCITE_NANDFLASH_VERSION "0.1"
+
+/* I/O register offsets */
+#define EXCITE_NANDFLASH_DATA_BYTE   0x00
+#define EXCITE_NANDFLASH_STATUS_BYTE 0x0c
+#define EXCITE_NANDFLASH_ADDR_BYTE   0x10
+#define EXCITE_NANDFLASH_CMD_BYTE    0x14
+
+#define io_readb(__a__)		__raw_readb((__a__))
+#define io_writeb(__v__, __a__)	__raw_writeb((__v__), (__a__))
+
+typedef void __iomem *io_reg_t;
+
+/* prefix for debug output */
+static const char module_id[] = "excite_nandflash";
+
+/*
+ * partition definition
+ */
+static const struct mtd_partition partition_info[] = {
+	{
+		.name = "eXcite RootFS",
+		.offset = 0,
+		.size = MTDPART_SIZ_FULL
+	}
+};
+
+static inline const struct resource *excite_nand_get_resource
+    (struct platform_device *d, unsigned long flags, const char *basename) {
+	const char fmt[] = "%s_%u";
+	char buf[80];
+
+	if (unlikely
+	    (snprintf(buf, sizeof buf, fmt, basename, d->id) >= sizeof buf))
+		return NULL;
+	return platform_get_resource_byname(d, flags, buf);
+}
+
+static inline io_reg_t
+excite_nand_map_regs(struct platform_device *d, const char *basename)
+{
+	void *result = NULL;
+	const struct resource *const r =
+	    excite_nand_get_resource(d, IORESOURCE_MEM, basename);
+	if (likely(r))
+		result = ioremap_nocache(r->start, r->end + 1 - r->start);
+	return result;
+}
+
+/* controller and mtd information */
+struct excite_nand_drvdata {
+	struct mtd_info board_mtd;
+	struct nand_chip board_chip;
+	io_reg_t regs;
+};
+
+/* command and control functions */
+static void excite_nand_control(struct mtd_info *mtd, int cmd,
+				       unsigned int ctrl)
+{
+	io_reg_t regs =
+	    container_of(mtd, struct excite_nand_drvdata, board_mtd)->regs;
+	static void __iomem *tgt = NULL;
+
+	switch (ctrl) {
+	case NAND_CTRL_CHANGE | NAND_CTRL_CLE:
+		tgt = regs + EXCITE_NANDFLASH_CMD_BYTE;
+		break;
+	case NAND_CTRL_CHANGE | NAND_CTRL_ALE:
+		tgt = regs + EXCITE_NANDFLASH_ADDR_BYTE;
+		break;
+	case NAND_CTRL_CHANGE | NAND_NCE:
+		tgt = regs + EXCITE_NANDFLASH_DATA_BYTE;
+		break;
+	}
+
+	if (cmd != NAND_CMD_NONE)
+		io_writeb(cmd, tgt);
+}
+
+/* excite_nand_devready()
+ *
+ * returns 0 if the nand is busy, 1 if it is ready
+ */
+static int excite_nand_devready(struct mtd_info *mtd)
+{
+	struct excite_nand_drvdata * const drvdata =
+	    container_of(mtd, struct excite_nand_drvdata, board_mtd);
+	return io_readb(drvdata->regs + EXCITE_NANDFLASH_STATUS_BYTE);
+}
+
+/* excite_nand_remove
+ *
+ * called by device layer to remove the driver
+ * the binding to the mtd and all allocated
+ * resources are released
+ */
+static int __exit excite_nand_remove(struct device *dev)
+{
+	struct excite_nand_drvdata * const this = dev_get_drvdata(dev);
+
+	dev_set_drvdata(dev, NULL);
+
+	if (unlikely(!this)) {
+		printk(KERN_ERR "%s: called %s without private data!!",
+		       module_id, __func__);
+		return -EINVAL;
+	}
+
+	/* first thing we need to do is release our mtd
+	 * then go through freeing the resource used
+	 */
+	nand_release(&this->board_mtd);
+
+	/* free the common resources */
+	if (likely(this->regs)) {
+		iounmap(this->regs);
+		this->regs = NULL;
+	}
+
+	kfree(this);
+
+	DEBUG(MTD_DEBUG_LEVEL1, "%s: removed\n", module_id);
+	return 0;
+}
+
+/* excite_nand_probe
+ *
+ * called by device layer when it finds a device matching
+ * one our driver can handled. This code checks to see if
+ * it can allocate all necessary resources then calls the
+ * nand layer to look for devices
+*/
+static int __init excite_nand_probe(struct device *dev)
+{
+	struct platform_device * const pdev = to_platform_device(dev);
+
+	struct excite_nand_drvdata *drvdata;	/* private driver data     */
+	struct nand_chip *board_chip;	/* private flash chip data */
+	struct mtd_info *board_mtd;	/* mtd info for this board */
+	int scan_res;
+
+	drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
+	if (unlikely(!drvdata)) {
+		printk(KERN_ERR "%s: no memory for drvdata\n",
+		       module_id);
+		return -ENOMEM;
+	}
+
+	/* bind private data into driver */
+	dev_set_drvdata(dev, drvdata);
+
+	/* allocate and map the resource */
+	drvdata->regs =
+		excite_nand_map_regs(pdev, EXCITE_NANDFLASH_RESOURCE_REGS);
+
+	if (unlikely(!drvdata->regs)) {
+		printk(KERN_ERR "%s: cannot reserve register region\n",
+		       module_id);
+		kfree(drvdata);
+		return -ENXIO;
+	}
+
+	/* initialise our chip */
+	board_chip = &drvdata->board_chip;
+	board_chip->IO_ADDR_R = board_chip->IO_ADDR_W =
+		drvdata->regs + EXCITE_NANDFLASH_DATA_BYTE;
+	board_chip->cmd_ctrl = excite_nand_control;
+	board_chip->dev_ready = excite_nand_devready;
+	board_chip->chip_delay = 25;
+	board_chip->ecc.mode = NAND_ECC_SOFT;
+
+	/* link chip to mtd */
+	board_mtd = &drvdata->board_mtd;
+	board_mtd->priv = board_chip;
+
+	DEBUG(MTD_DEBUG_LEVEL2, "%s: device scan\n", module_id);
+	scan_res = nand_scan(&drvdata->board_mtd, 1);
+
+	if (likely(!scan_res)) {
+		DEBUG(MTD_DEBUG_LEVEL2, "%s: register partitions\n", module_id);
+		add_mtd_partitions(&drvdata->board_mtd, partition_info,
+				   sizeof partition_info / sizeof partition_info[0]);
+	} else {
+		iounmap(drvdata->regs);
+		kfree(drvdata);
+		printk(KERN_ERR "%s: device scan failed\n", module_id);
+		return -EIO;
+	}
+	return 0;
+}
+
+static struct device_driver excite_nand_driver = {
+	.name = "excite_nand",
+	.bus = &platform_bus_type,
+	.probe = excite_nand_probe,
+	.remove = __exit_p(excite_nand_remove)
+};
+
+static int __init excite_nand_init(void)
+{
+	pr_info("Basler eXcite nand flash driver Version "
+		EXCITE_NANDFLASH_VERSION "\n");
+	return driver_register(&excite_nand_driver);
+}
+
+static void __exit excite_nand_exit(void)
+{
+	driver_unregister(&excite_nand_driver);
+}
+
+module_init(excite_nand_init);
+module_exit(excite_nand_exit);
+
+MODULE_AUTHOR("Thomas Koeller <thomas.koeller@baslerweb.com>");
+MODULE_DESCRIPTION("Basler eXcite NAND-Flash driver");
+MODULE_LICENSE("GPL");
+MODULE_VERSION(EXCITE_NANDFLASH_VERSION)
-- 
1.4.3

Re: [PATCH] eXcite nand flash driver

[Ulrich Eckhardt]

Just some cosmetic notes ...

On Thursday 08 February 2007 01:57, Thomas Koeller wrote:
> @@ -216,10 +216,26 @@ config MTD_NAND_DISKONCHIP_BBTWRITE
>  	  Even if you leave this disabled, you can enable BBT writes at module
>  	  load time (assuming you build diskonchip as a module) with the module
>  	  parameter "inftl_bbt_write=1".
> -
> +

This just inserts whitespace.

>         tristate "NAND support for OLPC CAFÉ chip"

The accent on the E might cause problems, but I'm not sure.

> +#define io_readb(__a__)		__raw_readb((__a__))
> +#define io_writeb(__v__, __a__)	__raw_writeb((__v__), (__a__))

I would have used an inline function. Also, aren't there functions to use IOs 
already? What's the reason here?

> +/* prefix for debug output */
> +static const char module_id[] = "excite_nandflash";

I'd have used

  static const char* module_id = "excite_nandflash";

except if you need the sizeof it to capture the length.

> +static inline const struct resource *excite_nand_get_resource
> +    (struct platform_device *d, unsigned long flags, const char *basename){
> +	const char fmt[] = "%s_%u";

Here, too, is no need for a local buffer, just use a pointer.

> +/* excite_nand_probe
> + *
> + * called by device layer when it finds a device matching
> + * one our driver can handled. [...]

"... when it finds a device our driver can handle."

Otherwise I'd say it looks very clean, better than quite some other stuff I 
have seen.

Uli

**************************************************************************************
           Visit our website at <http://www.satorlaser.de/>
**************************************************************************************
Diese E-Mail einschließlich sämtlicher Anhänge ist nur für den Adressaten bestimmt und kann vertrauliche Informationen enthalten. Bitte benachrichtigen Sie den Absender umgehend, falls Sie nicht der beabsichtigte Empfänger sein sollten. Die E-Mail ist in diesem Fall zu löschen und darf weder gelesen, weitergeleitet, veröffentlicht oder anderweitig benutzt werden.
E-Mails können durch Dritte gelesen werden und Viren sowie nichtautorisierte Änderungen enthalten. Sator Laser GmbH ist für diese Folgen nicht verantwortlich.

**************************************************************************************

Re: [PATCH] eXcite nand flash driver

[Thomas Koeller]

On Thursday 08 February 2007 09:20, Ulrich Eckhardt wrote:
> Just some cosmetic notes ...
>
> On Thursday 08 February 2007 01:57, Thomas Koeller wrote:
> > @@ -216,10 +216,26 @@ config MTD_NAND_DISKONCHIP_BBTWRITE
> >  	  Even if you leave this disabled, you can enable BBT writes at module
> >  	  load time (assuming you build diskonchip as a module) with the module
> >  	  parameter "inftl_bbt_write=1".
> > -
> > +
>
> This just inserts whitespace.

No, it actually _removes_ whitspace.

>
> >         tristate "NAND support for OLPC CAFÉ chip"
>
> The accent on the E might cause problems, but I'm not sure.

This has been in there before.

>
> > +#define io_readb(__a__)		__raw_readb((__a__))
> > +#define io_writeb(__v__, __a__)	__raw_writeb((__v__), (__a__))
>
> I would have used an inline function. Also, aren't there functions to use
> IOs already? What's the reason here?
>
> > +/* prefix for debug output */
> > +static const char module_id[] = "excite_nandflash";
>
> I'd have used
>
>   static const char* module_id = "excite_nandflash";
>
> except if you need the sizeof it to capture the length.

Then I'd have used
	static const char * const module_id = "excite_nandflash";

but what is the advantage of doing so?

>
> > +static inline const struct resource *excite_nand_get_resource
> > +    (struct platform_device *d, unsigned long flags, const char
> > *basename){ +	const char fmt[] = "%s_%u";
>
> Here, too, is no need for a local buffer, just use a pointer.
>
> > +/* excite_nand_probe
> > + *
> > + * called by device layer when it finds a device matching
> > + * one our driver can handled. [...]
>
> "... when it finds a device our driver can handle."

Right.

>
> Otherwise I'd say it looks very clean, better than quite some other stuff I
> have seen.
>
> Uli
>

Thanks for your comments!


_______________________________

Thomas Köller, Software Developer

Basler Vision Technologies
An der Strusbek 60-62
22926 Ahrensburg
Germany

Tel. +49 (0) 4102 - 463 390
Fax +49 (0) 4102 - 463 390

mailto:thomas.koeller@baslerweb.com
http://www.baslerweb.com

Vorstand: Dr.-Ing. Dietmar Ley (Vorsitzender) · John P. Jennings · Peter Krumhoff · Aufsichtsratsvorsitzender: Norbert Basler
Basler AG · Amtsgericht Ahrensburg HRB 4090 · Ust-IdNr.: DE 135 098 121 · Steuer-Nr.: 30 292 04497

_______________________________

Re: [PATCH] eXcite nand flash driver

[Josh Boyer]

On Thu, 2007-02-08 at 01:57 +0100, Thomas Koeller wrote:
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 358f55a..5b50396 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -216,10 +216,26 @@ config MTD_NAND_DISKONCHIP_BBTWRITE
>  	  Even if you leave this disabled, you can enable BBT writes at module
>  	  load time (assuming you build diskonchip as a module) with the module
>  	  parameter "inftl_bbt_write=1".
> -
> +	  
>  config MTD_NAND_SHARPSL
>  	tristate "Support for NAND Flash on Sharp SL Series (C7xx + others)"
>  	depends on MTD_NAND && ARCH_PXA
> + 
> +config MTD_NAND_BASLER_EXCITE
> +	tristate  "Support for NAND Flash on Basler eXcite"
> +	depends on MTD_NAND && BASLER_EXCITE
> +	help
> +          This enables the driver for the NAND flash device found on the
> +          Basler eXcite Smart Camera. If built as a module, the driver
> +	  will be named "excite_nandflash.ko".
> +
> +config MTD_NAND_BASLER_EXCITE
> +	tristate  "Support for NAND Flash on Basler eXcite"
> +	depends on MTD_NAND && BASLER_EXCITE
> +	help
> +          This enables the driver for the NAND flash device found on the
> +          Basler eXcite Smart Camera. If built as a module, the driver
> +	  will be named "excite_nandflash.ko".

You have the same config option twice...  Cut and paste error?

> diff --git a/drivers/mtd/nand/excite_nandflash.c 
> b/drivers/mtd/nand/excite_nandflash.c
> new file mode 100644
> index 0000000..d683659
> --- /dev/null
> +++ b/drivers/mtd/nand/excite_nandflash.c

<snip>

> +
> +#define io_readb(__a__)		__raw_readb((__a__))
> +#define io_writeb(__v__, __a__)	__raw_writeb((__v__), (__a__))

Do you really need these defines?  Why can't you call
__raw_{readb,writeb} directly?

> +
> +typedef void __iomem *io_reg_t;

Ugh... typdef?

<snip>

> +static inline io_reg_t
> +excite_nand_map_regs(struct platform_device *d, const char *basename)
> +{
> +	void *result = NULL;
> +	const struct resource *const r =
> +	    excite_nand_get_resource(d, IORESOURCE_MEM, basename);
> +	if (likely(r))
> +		result = ioremap_nocache(r->start, r->end + 1 - r->start);

Does this likely really buy you anything?  I would think doing the
converse (if any sort of *likely at all) would be better.

<snip>

> + */
> +static int __exit excite_nand_remove(struct device *dev)
> +{
> +	struct excite_nand_drvdata * const this = dev_get_drvdata(dev);
> +
> +	dev_set_drvdata(dev, NULL);
> +
> +	if (unlikely(!this)) {
> +		printk(KERN_ERR "%s: called %s without private data!!",
> +		       module_id, __func__);
> +		return -EINVAL;
> +	}
> +
> +	/* first thing we need to do is release our mtd
> +	 * then go through freeing the resource used
> +	 */
> +	nand_release(&this->board_mtd);
> +
> +	/* free the common resources */
> +	if (likely(this->regs)) {
> +		iounmap(this->regs);
> +		this->regs = NULL;
> +	}

Same likely usage comment as above.

josh

Re: [PATCH] eXcite nand flash driver

[Josh Boyer]

On Thu, 2007-02-08 at 01:57 +0100, Thomas Koeller wrote:
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 358f55a..5b50396 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -216,10 +216,26 @@ config MTD_NAND_DISKONCHIP_BBTWRITE
>  	  Even if you leave this disabled, you can enable BBT writes at module
>  	  load time (assuming you build diskonchip as a module) with the module
>  	  parameter "inftl_bbt_write=1".
> -
> +	  
>  config MTD_NAND_SHARPSL
>  	tristate "Support for NAND Flash on Sharp SL Series (C7xx + others)"
>  	depends on MTD_NAND && ARCH_PXA
> + 
> +config MTD_NAND_BASLER_EXCITE
> +	tristate  "Support for NAND Flash on Basler eXcite"
> +	depends on MTD_NAND && BASLER_EXCITE
> +	help
> +          This enables the driver for the NAND flash device found on the
> +          Basler eXcite Smart Camera. If built as a module, the driver
> +	  will be named "excite_nandflash.ko".
> +
> +config MTD_NAND_BASLER_EXCITE
> +	tristate  "Support for NAND Flash on Basler eXcite"
> +	depends on MTD_NAND && BASLER_EXCITE
> +	help
> +          This enables the driver for the NAND flash device found on the
> +          Basler eXcite Smart Camera. If built as a module, the driver
> +	  will be named "excite_nandflash.ko".

You have the same config option twice...  Cut and paste error?

> diff --git a/drivers/mtd/nand/excite_nandflash.c 
> b/drivers/mtd/nand/excite_nandflash.c
> new file mode 100644
> index 0000000..d683659
> --- /dev/null
> +++ b/drivers/mtd/nand/excite_nandflash.c

<snip>

> +
> +#define io_readb(__a__)		__raw_readb((__a__))
> +#define io_writeb(__v__, __a__)	__raw_writeb((__v__), (__a__))

Do you really need these defines?  Why can't you call
__raw_{readb,writeb} directly?

> +
> +typedef void __iomem *io_reg_t;

Ugh... typdef?

<snip>

> +static inline io_reg_t
> +excite_nand_map_regs(struct platform_device *d, const char *basename)
> +{
> +	void *result = NULL;
> +	const struct resource *const r =
> +	    excite_nand_get_resource(d, IORESOURCE_MEM, basename);
> +	if (likely(r))
> +		result = ioremap_nocache(r->start, r->end + 1 - r->start);

Does this likely really buy you anything?  I would think doing the
converse (if any sort of *likely at all) would be better.

<snip>

> + */
> +static int __exit excite_nand_remove(struct device *dev)
> +{
> +	struct excite_nand_drvdata * const this = dev_get_drvdata(dev);
> +
> +	dev_set_drvdata(dev, NULL);
> +
> +	if (unlikely(!this)) {
> +		printk(KERN_ERR "%s: called %s without private data!!",
> +		       module_id, __func__);
> +		return -EINVAL;
> +	}
> +
> +	/* first thing we need to do is release our mtd
> +	 * then go through freeing the resource used
> +	 */
> +	nand_release(&this->board_mtd);
> +
> +	/* free the common resources */
> +	if (likely(this->regs)) {
> +		iounmap(this->regs);
> +		this->regs = NULL;
> +	}

Same likely usage comment as above.

josh

Re: [PATCH] eXcite nand flash driver

[Artem Bityutskiy]

On Thu, 2007-02-08 at 07:50 -0600, Josh Boyer wrote:
> > +	/* free the common resources */
> > +	if (likely(this->regs)) {
> > +		iounmap(this->regs);
> > +		this->regs = NULL;
> > +	}
> 
> Same likely usage comment as above.

I agree, this function will be called one or very few times, so  this
hint is not reasonable in this case.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH] eXcite nand flash driver

[Artem Bityutskiy]

On Thu, 2007-02-08 at 07:50 -0600, Josh Boyer wrote:
> > +	/* free the common resources */
> > +	if (likely(this->regs)) {
> > +		iounmap(this->regs);
> > +		this->regs = NULL;
> > +	}
> 
> Same likely usage comment as above.

I agree, this function will be called one or very few times, so  this
hint is not reasonable in this case.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH] eXcite nand flash driver

[Thomas Gleixner]

On Thu, 2007-02-08 at 01:57 +0100, Thomas Koeller wrote:

> +static inline const struct resource *excite_nand_get_resource
> +    (struct platform_device *d, unsigned long flags, const char *basename) {

Move curly bracket to new line.

> +	const char fmt[] = "%s_%u";

Please move the format into the snprintf

> +	char buf[80];
> +
> +	if (unlikely
> +	    (snprintf(buf, sizeof buf, fmt, basename, d->id) >= sizeof buf))
> +		return NULL;

Useless unlikely

> +	return platform_get_resource_byname(d, flags, buf);
> +}
> +
> +static inline io_reg_t
> +excite_nand_map_regs(struct platform_device *d, const char *basename)
> +{
> +	void *result = NULL;
> +	const struct resource *const r =
> +	    excite_nand_get_resource(d, IORESOURCE_MEM, basename);

Blank line between variable declaration and code.

> +	if (likely(r))
> +		result = ioremap_nocache(r->start, r->end + 1 - r->start);

Useless likely

> +	return result;
> +}


> +/* command and control functions */
> +static void excite_nand_control(struct mtd_info *mtd, int cmd,
> +				       unsigned int ctrl)
> +{
> +	io_reg_t regs =
> +	    container_of(mtd, struct excite_nand_drvdata, board_mtd)->regs;
> +	static void __iomem *tgt = NULL;
> +
> +	switch (ctrl) {
> +	case NAND_CTRL_CHANGE | NAND_CTRL_CLE:
> +		tgt = regs + EXCITE_NANDFLASH_CMD_BYTE;
> +		break;
> +	case NAND_CTRL_CHANGE | NAND_CTRL_ALE:
> +		tgt = regs + EXCITE_NANDFLASH_ADDR_BYTE;
> +		break;
> +	case NAND_CTRL_CHANGE | NAND_NCE:
> +		tgt = regs + EXCITE_NANDFLASH_DATA_BYTE;
> +		break;
> +	}

Err, did this ever work ? I doubt it. From nand_base.c:

                chip->cmd_ctrl(mtd, page_addr, ctrl);                                                                                                        
                ctrl &= ~NAND_CTRL_CHANGE;                                                                                                                   
                chip->cmd_ctrl(mtd, page_addr >> 8, ctrl);  

So I expect an OOPS happens on a regular base.

> +static int excite_nand_devready(struct mtd_info *mtd)
> +{
> +	struct excite_nand_drvdata * const drvdata =
> +	    container_of(mtd, struct excite_nand_drvdata, board_mtd);

Blank line missing

> +	return io_readb(drvdata->regs + EXCITE_NANDFLASH_STATUS_BYTE);
> +}
> +
> +/* excite_nand_remove
> + *
> + * called by device layer to remove the driver
> + * the binding to the mtd and all allocated
> + * resources are released
> + */
> +static int __exit excite_nand_remove(struct device *dev)
> +{
> +	struct excite_nand_drvdata * const this = dev_get_drvdata(dev);
> +
> +	dev_set_drvdata(dev, NULL);
> +
> +	if (unlikely(!this)) {
> +		printk(KERN_ERR "%s: called %s without private data!!",
> +		       module_id, __func__);
> +		return -EINVAL;
> +	}
> +
> +	/* first thing we need to do is release our mtd
> +	 * then go through freeing the resource used
> +	 */
> +	nand_release(&this->board_mtd);
> +
> +	/* free the common resources */
> +	if (likely(this->regs)) {

Why should you ever come here with no mapping ?

	tglx

Re: [PATCH] eXcite nand flash driver

[Thomas Gleixner]

On Thu, 2007-02-08 at 01:57 +0100, Thomas Koeller wrote:

> +static inline const struct resource *excite_nand_get_resource
> +    (struct platform_device *d, unsigned long flags, const char *basename) {

Move curly bracket to new line.

> +	const char fmt[] = "%s_%u";

Please move the format into the snprintf

> +	char buf[80];
> +
> +	if (unlikely
> +	    (snprintf(buf, sizeof buf, fmt, basename, d->id) >= sizeof buf))
> +		return NULL;

Useless unlikely

> +	return platform_get_resource_byname(d, flags, buf);
> +}
> +
> +static inline io_reg_t
> +excite_nand_map_regs(struct platform_device *d, const char *basename)
> +{
> +	void *result = NULL;
> +	const struct resource *const r =
> +	    excite_nand_get_resource(d, IORESOURCE_MEM, basename);

Blank line between variable declaration and code.

> +	if (likely(r))
> +		result = ioremap_nocache(r->start, r->end + 1 - r->start);

Useless likely

> +	return result;
> +}


> +/* command and control functions */
> +static void excite_nand_control(struct mtd_info *mtd, int cmd,
> +				       unsigned int ctrl)
> +{
> +	io_reg_t regs =
> +	    container_of(mtd, struct excite_nand_drvdata, board_mtd)->regs;
> +	static void __iomem *tgt = NULL;
> +
> +	switch (ctrl) {
> +	case NAND_CTRL_CHANGE | NAND_CTRL_CLE:
> +		tgt = regs + EXCITE_NANDFLASH_CMD_BYTE;
> +		break;
> +	case NAND_CTRL_CHANGE | NAND_CTRL_ALE:
> +		tgt = regs + EXCITE_NANDFLASH_ADDR_BYTE;
> +		break;
> +	case NAND_CTRL_CHANGE | NAND_NCE:
> +		tgt = regs + EXCITE_NANDFLASH_DATA_BYTE;
> +		break;
> +	}

Err, did this ever work ? I doubt it. From nand_base.c:

                chip->cmd_ctrl(mtd, page_addr, ctrl);                                                                                                        
                ctrl &= ~NAND_CTRL_CHANGE;                                                                                                                   
                chip->cmd_ctrl(mtd, page_addr >> 8, ctrl);  

So I expect an OOPS happens on a regular base.

> +static int excite_nand_devready(struct mtd_info *mtd)
> +{
> +	struct excite_nand_drvdata * const drvdata =
> +	    container_of(mtd, struct excite_nand_drvdata, board_mtd);

Blank line missing

> +	return io_readb(drvdata->regs + EXCITE_NANDFLASH_STATUS_BYTE);
> +}
> +
> +/* excite_nand_remove
> + *
> + * called by device layer to remove the driver
> + * the binding to the mtd and all allocated
> + * resources are released
> + */
> +static int __exit excite_nand_remove(struct device *dev)
> +{
> +	struct excite_nand_drvdata * const this = dev_get_drvdata(dev);
> +
> +	dev_set_drvdata(dev, NULL);
> +
> +	if (unlikely(!this)) {
> +		printk(KERN_ERR "%s: called %s without private data!!",
> +		       module_id, __func__);
> +		return -EINVAL;
> +	}
> +
> +	/* first thing we need to do is release our mtd
> +	 * then go through freeing the resource used
> +	 */
> +	nand_release(&this->board_mtd);
> +
> +	/* free the common resources */
> +	if (likely(this->regs)) {

Why should you ever come here with no mapping ?

	tglx

Re: [PATCH] eXcite nand flash driver

[Thomas Koeller]

On Donnerstag, 8. Februar 2007, Thomas Gleixner wrote:
> > +/* command and control functions */
> > +static void excite_nand_control(struct mtd_info *mtd, int cmd,
> > +				       unsigned int ctrl)
> > +{
> > +	io_reg_t regs =
> > +	    container_of(mtd, struct excite_nand_drvdata, board_mtd)->regs;
> > +	static void __iomem *tgt = NULL;
> > +
> > +	switch (ctrl) {
> > +	case NAND_CTRL_CHANGE | NAND_CTRL_CLE:
> > +		tgt = regs + EXCITE_NANDFLASH_CMD_BYTE;
> > +		break;
> > +	case NAND_CTRL_CHANGE | NAND_CTRL_ALE:
> > +		tgt = regs + EXCITE_NANDFLASH_ADDR_BYTE;
> > +		break;
> > +	case NAND_CTRL_CHANGE | NAND_NCE:
> > +		tgt = regs + EXCITE_NANDFLASH_DATA_BYTE;
> > +		break;
> > +	}
>
> Err, did this ever work ? I doubt it. From nand_base.c:
>
>                 chip->cmd_ctrl(mtd, page_addr, ctrl);
>                 ctrl &= ~NAND_CTRL_CHANGE;
>                 chip->cmd_ctrl(mtd, page_addr >> 8, ctrl);
>
> So I expect an OOPS happens on a regular base.
>

I guess it is the 'static void __iomem *tgt = NULL' part that worries
you? Think about it, that value is never used.

However, I admit it is somewhat unclean, and therefore I am changing
it. Updated patch follows.

tk

Re: [PATCH] eXcite nand flash driver

[Thomas Koeller]

On Donnerstag, 8. Februar 2007, Thomas Gleixner wrote:
> > +/* command and control functions */
> > +static void excite_nand_control(struct mtd_info *mtd, int cmd,
> > +				       unsigned int ctrl)
> > +{
> > +	io_reg_t regs =
> > +	    container_of(mtd, struct excite_nand_drvdata, board_mtd)->regs;
> > +	static void __iomem *tgt = NULL;
> > +
> > +	switch (ctrl) {
> > +	case NAND_CTRL_CHANGE | NAND_CTRL_CLE:
> > +		tgt = regs + EXCITE_NANDFLASH_CMD_BYTE;
> > +		break;
> > +	case NAND_CTRL_CHANGE | NAND_CTRL_ALE:
> > +		tgt = regs + EXCITE_NANDFLASH_ADDR_BYTE;
> > +		break;
> > +	case NAND_CTRL_CHANGE | NAND_NCE:
> > +		tgt = regs + EXCITE_NANDFLASH_DATA_BYTE;
> > +		break;
> > +	}
>
> Err, did this ever work ? I doubt it. From nand_base.c:
>
>                 chip->cmd_ctrl(mtd, page_addr, ctrl);
>                 ctrl &= ~NAND_CTRL_CHANGE;
>                 chip->cmd_ctrl(mtd, page_addr >> 8, ctrl);
>
> So I expect an OOPS happens on a regular base.
>

I guess it is the 'static void __iomem *tgt = NULL' part that worries
you? Think about it, that value is never used.

However, I admit it is somewhat unclean, and therefore I am changing
it. Updated patch follows.

tk

Re: [PATCH] eXcite nand flash driver

[Thomas Gleixner]

On Sat, 2007-02-10 at 11:22 +0100, Thomas Koeller wrote:
> > So I expect an OOPS happens on a regular base.
> I guess it is the 'static void __iomem *tgt = NULL' part that worries
> you? Think about it, that value is never used.

True.

> However, I admit it is somewhat unclean, and therefore I am changing
> it. Updated patch follows.

Yes, it is confusing and looks horrible.

	tglx

Re: [PATCH] eXcite nand flash driver

[Thomas Gleixner]

On Sat, 2007-02-10 at 11:22 +0100, Thomas Koeller wrote:
> > So I expect an OOPS happens on a regular base.
> I guess it is the 'static void __iomem *tgt = NULL' part that worries
> you? Think about it, that value is never used.

True.

> However, I admit it is somewhat unclean, and therefore I am changing
> it. Updated patch follows.

Yes, it is confusing and looks horrible.

	tglx

Re: [PATCH] eXcite nand flash driver

[Thomas Gleixner]

On Sat, 2007-02-10 at 11:21 +0100, Thomas Koeller wrote:
> This is a nand flash driver for the eXcite series of intelligent
> cameras manufactured by Basler Vision Technologies AG.
>
> Signed-off-by: Thomas Koeller <thomas.koeller@baslerweb.com>

Looks much better now. David, can you pull that one into your tree
please ?

	tglx

[PATCH] eXcite nand flash driver

[Thomas Koeller]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 9241 bytes --]

This is a nand flash driver for the eXcite series of intelligent
cameras manufactured by Basler Vision Technologies AG.

Signed-off-by: Thomas Koeller <thomas.koeller@baslerweb.com>
---
 drivers/mtd/nand/Kconfig            |    8 +
 drivers/mtd/nand/Makefile           |    1 +
 drivers/mtd/nand/excite_nandflash.c |  248 
+++++++++++++++++++++++++++++++++++
 3 files changed, 257 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 358f55a..d188263 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -221,6 +221,14 @@ config MTD_NAND_SHARPSL
 	tristate "Support for NAND Flash on Sharp SL Series (C7xx + others)"
 	depends on MTD_NAND && ARCH_PXA
 
+config MTD_NAND_BASLER_EXCITE
+	tristate  "Support for NAND Flash on Basler eXcite"
+	depends on MTD_NAND && BASLER_EXCITE
+	help
+          This enables the driver for the NAND flash device found on the
+          Basler eXcite Smart Camera. If built as a module, the driver
+          will be named "excite_nandflash.ko".
+
 config MTD_NAND_CAFE
        tristate "NAND support for OLPC CAFÉ chip"
        depends on PCI
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index f7a53f0..80f1dfc 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_MTD_NAND_NANDSIM)		+= nands
 obj-$(CONFIG_MTD_NAND_CS553X)		+= cs553x_nand.o
 obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
 obj-$(CONFIG_MTD_NAND_AT91)		+= at91_nand.o
+obj-$(CONFIG_MTD_NAND_BASLER_EXCITE)	+= excite_nandflash.o
 
 nand-objs := nand_base.o nand_bbt.o
 cafe_nand-objs := cafe.o cafe_ecc.o
diff --git a/drivers/mtd/nand/excite_nandflash.c 
b/drivers/mtd/nand/excite_nandflash.c
new file mode 100644
index 0000000..7e9afc4
--- /dev/null
+++ b/drivers/mtd/nand/excite_nandflash.c
@@ -0,0 +1,248 @@
+/*
+*  Copyright (C) 2005 - 2007 by Basler Vision Technologies AG
+*  Author: Thomas Koeller <thomas.koeller.qbaslerweb.com>
+*  Original code by Thies Moeller <thies.moeller@baslerweb.com>
+*
+*  This program is free software; you can redistribute it and/or modify
+*  it under the terms of the GNU General Public License as published by
+*  the Free Software Foundation; either version 2 of the License, or
+*  (at your option) any later version.
+*
+*  This program is distributed in the hope that it will be useful,
+*  but WITHOUT ANY WARRANTY; without even the implied warranty of
+*  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+*  GNU General Public License for more details.
+*
+*  You should have received a copy of the GNU General Public License
+*  along with this program; if not, write to the Free Software
+*  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+*/
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/ioport.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/nand_ecc.h>
+#include <linux/mtd/partitions.h>
+
+#include <asm/io.h>
+#include <asm/rm9k-ocd.h>
+
+#include <excite_nandflash.h>
+
+#define EXCITE_NANDFLASH_VERSION "0.1"
+
+/* I/O register offsets */
+#define EXCITE_NANDFLASH_DATA_BYTE   0x00
+#define EXCITE_NANDFLASH_STATUS_BYTE 0x0c
+#define EXCITE_NANDFLASH_ADDR_BYTE   0x10
+#define EXCITE_NANDFLASH_CMD_BYTE    0x14
+
+/* prefix for debug output */
+static const char module_id[] = "excite_nandflash";
+
+/*
+ * partition definition
+ */
+static const struct mtd_partition partition_info[] = {
+	{
+		.name = "eXcite RootFS",
+		.offset = 0,
+		.size = MTDPART_SIZ_FULL
+	}
+};
+
+static inline const struct resource *
+excite_nand_get_resource(struct platform_device *d, unsigned long flags,
+			 const char *basename)
+{
+	char buf[80];
+
+	if (snprintf(buf, sizeof buf, "%s_%u", basename, d->id) >= sizeof buf)
+		return NULL;
+	return platform_get_resource_byname(d, flags, buf);
+}
+
+static inline void __iomem *
+excite_nand_map_regs(struct platform_device *d, const char *basename)
+{
+	void *result = NULL;
+	const struct resource *const r =
+	    excite_nand_get_resource(d, IORESOURCE_MEM, basename);
+
+	if (r)
+		result = ioremap_nocache(r->start, r->end + 1 - r->start);
+	return result;
+}
+
+/* controller and mtd information */
+struct excite_nand_drvdata {
+	struct mtd_info board_mtd;
+	struct nand_chip board_chip;
+	void __iomem *regs;
+	void __iomem *tgt;
+};
+
+/* Control function */
+static void excite_nand_control(struct mtd_info *mtd, int cmd,
+				       unsigned int ctrl)
+{
+	struct excite_nand_drvdata * const d =
+	    container_of(mtd, struct excite_nand_drvdata, board_mtd);
+
+	switch (ctrl) {
+	case NAND_CTRL_CHANGE | NAND_CTRL_CLE:
+		d->tgt = d->regs + EXCITE_NANDFLASH_CMD_BYTE;
+		break;
+	case NAND_CTRL_CHANGE | NAND_CTRL_ALE:
+		d->tgt = d->regs + EXCITE_NANDFLASH_ADDR_BYTE;
+		break;
+	case NAND_CTRL_CHANGE | NAND_NCE:
+		d->tgt = d->regs + EXCITE_NANDFLASH_DATA_BYTE;
+		break;
+	}
+
+	if (cmd != NAND_CMD_NONE)
+		__raw_writeb(cmd, d->tgt);
+}
+
+/* Return 0 if flash is busy, 1 if ready */
+static int excite_nand_devready(struct mtd_info *mtd)
+{
+	struct excite_nand_drvdata * const drvdata =
+	    container_of(mtd, struct excite_nand_drvdata, board_mtd);
+
+	return __raw_readb(drvdata->regs + EXCITE_NANDFLASH_STATUS_BYTE);
+}
+
+/*
+ * Called by device layer to remove the driver.
+ * The binding to the mtd and all allocated
+ * resources are released.
+ */
+static int __exit excite_nand_remove(struct device *dev)
+{
+	struct excite_nand_drvdata * const this = dev_get_drvdata(dev);
+
+	dev_set_drvdata(dev, NULL);
+
+	if (unlikely(!this)) {
+		printk(KERN_ERR "%s: called %s without private data!!",
+		       module_id, __func__);
+		return -EINVAL;
+	}
+
+	/* first thing we need to do is release our mtd
+	 * then go through freeing the resource used
+	 */
+	nand_release(&this->board_mtd);
+
+	/* free the common resources */
+	iounmap(this->regs);
+	kfree(this);
+
+	DEBUG(MTD_DEBUG_LEVEL1, "%s: removed\n", module_id);
+	return 0;
+}
+
+/*
+ * Called by device layer when it finds a device matching
+ * one our driver can handle. This code checks to see if
+ * it can allocate all necessary resources then calls the
+ * nand layer to look for devices.
+*/
+static int __init excite_nand_probe(struct device *dev)
+{
+	struct platform_device * const pdev = to_platform_device(dev);
+	struct excite_nand_drvdata *drvdata;	/* private driver data */
+	struct nand_chip *board_chip;	/* private flash chip data */
+	struct mtd_info *board_mtd;	/* mtd info for this board */
+	int scan_res;
+
+	drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
+	if (unlikely(!drvdata)) {
+		printk(KERN_ERR "%s: no memory for drvdata\n",
+		       module_id);
+		return -ENOMEM;
+	}
+
+	/* bind private data into driver */
+	dev_set_drvdata(dev, drvdata);
+
+	/* allocate and map the resource */
+	drvdata->regs =
+		excite_nand_map_regs(pdev, EXCITE_NANDFLASH_RESOURCE_REGS);
+
+	if (unlikely(!drvdata->regs)) {
+		printk(KERN_ERR "%s: cannot reserve register region\n",
+		       module_id);
+		kfree(drvdata);
+		return -ENXIO;
+	}
+	
+	drvdata->tgt = drvdata->regs + EXCITE_NANDFLASH_DATA_BYTE;
+
+	/* initialise our chip */
+	board_chip = &drvdata->board_chip;
+	board_chip->IO_ADDR_R = board_chip->IO_ADDR_W =
+		drvdata->regs + EXCITE_NANDFLASH_DATA_BYTE;
+	board_chip->cmd_ctrl = excite_nand_control;
+	board_chip->dev_ready = excite_nand_devready;
+	board_chip->chip_delay = 25;
+	board_chip->ecc.mode = NAND_ECC_SOFT;
+
+	/* link chip to mtd */
+	board_mtd = &drvdata->board_mtd;
+	board_mtd->priv = board_chip;
+
+	DEBUG(MTD_DEBUG_LEVEL2, "%s: device scan\n", module_id);
+	scan_res = nand_scan(&drvdata->board_mtd, 1);
+
+	if (likely(!scan_res)) {
+		DEBUG(MTD_DEBUG_LEVEL2, "%s: register partitions\n", module_id);
+		add_mtd_partitions(&drvdata->board_mtd, partition_info,
+				   sizeof partition_info / sizeof partition_info[0]);
+	} else {
+		iounmap(drvdata->regs);
+		kfree(drvdata);
+		printk(KERN_ERR "%s: device scan failed\n", module_id);
+		return -EIO;
+	}
+	return 0;
+}
+
+static struct device_driver excite_nand_driver = {
+	.name = "excite_nand",
+	.bus = &platform_bus_type,
+	.probe = excite_nand_probe,
+	.remove = __exit_p(excite_nand_remove)
+};
+
+static int __init excite_nand_init(void)
+{
+	pr_info("Basler eXcite nand flash driver Version "
+		EXCITE_NANDFLASH_VERSION "\n");
+	return driver_register(&excite_nand_driver);
+}
+
+static void __exit excite_nand_exit(void)
+{
+	driver_unregister(&excite_nand_driver);
+}
+
+module_init(excite_nand_init);
+module_exit(excite_nand_exit);
+
+MODULE_AUTHOR("Thomas Koeller <thomas.koeller@baslerweb.com>");
+MODULE_DESCRIPTION("Basler eXcite NAND-Flash driver");
+MODULE_LICENSE("GPL");
+MODULE_VERSION(EXCITE_NANDFLASH_VERSION)
-- 
1.4.3