[Fwd: Re: [PATCH] 2/2 mtd: Add support for the Dreamcast VMU flash]

[Fwd: Re: [PATCH] 2/2 mtd: Add support for the Dreamcast VMU flash]

[Adrian McMenamin]

---

This builds on (though I essentially rewrote most of it) a driver Paul
Mundt and I wrote way back when to support the memory component of the
SEGA Dreamcast's Visual Memory Unit.

The device is accessed via the Dreamcast's maple bus.

Signed-off-by: Adrian McMenamin <adrian@mcmen.demon.co.uk>
---
diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
index 12c2536..c89a610 100644
--- a/drivers/mtd/maps/Kconfig
+++ b/drivers/mtd/maps/Kconfig
@@ -590,5 +590,19 @@ config MTD_PLATRAM
 
 	  This selection automatically selects the map_ram driver.
 
+config MTD_VMU_FLASH
+	tristate "Dreamcast Visual Memory devices and clones"
+	depends on SH_DREAMCAST
+	select MAPLE
+	help
+	  Flash map driver for the SEGA Dreamcast Visual Memory Unit
+	  and clones.
+
+	  Most Dreamcast users will have one of these and so will want
+	  to say Y here.
+
+	  To compile this driver as a module choose M here: the module
+	  will be called vmu_flash.
+
 endmenu
 
diff --git a/drivers/mtd/maps/Makefile b/drivers/mtd/maps/Makefile
index a9cbe80..d5802d2 100644
--- a/drivers/mtd/maps/Makefile
+++ b/drivers/mtd/maps/Makefile
@@ -68,3 +68,4 @@ obj-$(CONFIG_MTD_PLATRAM)	+= plat-ram.o
 obj-$(CONFIG_MTD_OMAP_NOR)	+= omap_nor.o
 obj-$(CONFIG_MTD_MTX1)		+= mtx-1_flash.o
 obj-$(CONFIG_MTD_INTEL_VR_NOR)	+= intel_vr_nor.o
+obj-$(CONFIG_MTD_VMU_FLASH)	+= vmu_flash.o
diff --git a/drivers/mtd/maps/vmu_flash.c b/drivers/mtd/maps/vmu_flash.c
new file mode 100644
index 0000000..ce31134
--- /dev/null
+++ b/drivers/mtd/maps/vmu_flash.c
@@ -0,0 +1,746 @@
+/* vmu-flash.c
+ * Driver for SEGA Dreamcast Visual Memory Unit
+ *
+ * Copyright Adrian McMenamin 2008
+ *
+ * Substantial parts based on code:
+ * Copyright Paul Mundt 2001
+ * Copyright Adrian McMenamin 2002
+ *
+ *
+ * Licensed under version 2 of the
+ * GNU General Public Licence
+ */
+#include <linux/init.h>
+#include <linux/maple.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/map.h>
+#include <asm/mach/maple.h>
+
+static DECLARE_WAIT_QUEUE_HEAD(vmu_read);
+static int block_read;
+
+struct vmu_cache {
+	char *buffer;			/* Cache */
+	unsigned int block;		/* Which block was cached */
+	unsigned long jiffies_atc;	/* Whn was it cached? */
+	int valid;
+};
+
+struct mdev_part {
+	struct maple_device *mdev;
+	int partition;
+};
+
+struct vmupart {
+	u16 user_blocks;
+	u16 root_block;
+	u16 numblocks;
+	char *name;
+	struct vmu_cache *pcache;
+};
+
+struct memcard {
+	u16 tempA;
+	u16 tempB;
+	u32 partitions;
+	u32 blocklen;
+	u32 writecnt;
+	u32 readcnt;
+	u32 removeable;
+	int partition;
+	void *sendbuf;
+	void *blockread;
+	struct vmupart *parts;
+	struct mtd_info *mtd;
+};
+
+struct vmu_block {
+	unsigned int num; /* block number */
+	unsigned int ofs; /* block offset */
+};
+
+static struct vmu_block *ofs_to_block(unsigned long src_ofs,
+	struct mtd_info *mtd, int partition)
+{
+	struct vmu_block *vblock;
+	struct maple_device *mdev;
+	struct memcard *card;
+	struct mdev_part *mpart;
+
+	mpart = mtd->priv;
+	mdev = mpart->mdev;
+	card = mdev->private_data;
+	vblock = kmalloc(sizeof(struct vmu_block), GFP_KERNEL);
+	if (!vblock)
+		goto simple_failed;
+
+	if (src_ofs >= ((card->parts)[partition]).numblocks * card->blocklen)
+		goto failed;
+
+	vblock->num = src_ofs / card->blocklen;
+
+	if (vblock->num > ((card->parts)[partition]).numblocks)
+		goto failed;
+
+	vblock->ofs = src_ofs % card->blocklen;
+	return vblock;
+
+failed:
+	kfree(vblock);
+simple_failed:
+	return NULL;
+}
+
+
+/* Maple bus callback function for reads */
+static void vmu_blockread(struct mapleq *mq)
+{
+	struct maple_device *mdev;
+	struct memcard *card;
+	struct mtd_info *mtd;
+	struct vmu_cache *pcache;
+	struct mdev_part *mpart;
+	int partition;
+
+	mdev = mq->dev;
+	card = mdev->private_data;
+	card->blockread = kmalloc(card->blocklen, GFP_KERNEL);
+	if (!card->blockread)
+		return;
+	memcpy(card->blockread, mq->recvbuf + 12, card->blocklen);
+	block_read = 1;
+	mtd = card->mtd;
+	mpart = mtd->priv;
+	partition = mpart->partition;
+	pcache = (card->parts[partition]).pcache;
+	if (!pcache->buffer)
+		pcache->buffer = kmalloc(card->blocklen, GFP_KERNEL);
+		if (!pcache->buffer)
+			return;
+	memcpy(pcache->buffer, card->blockread, card->blocklen);
+	pcache->block = ((unsigned char *)mq->recvbuf)[11] & 0xFF;
+	pcache->jiffies_atc = jiffies;
+	pcache->valid = 1;
+	wake_up_interruptible(&vmu_read);
+}
+
+/* Interface with maple bus to read bytes */
+static int maple_vmu_read_block(unsigned int num, unsigned char *buf,
+	struct mtd_info *mtd)
+{
+	struct memcard *card;
+	struct mdev_part *mpart;
+	struct maple_device *mdev;
+	int partition, error, locking;
+	void *sendbuf;
+
+	mpart = mtd->priv;
+	mdev = mpart->mdev;
+	partition = mpart->partition;
+	card = mdev->private_data;
+
+	/* wait for the mutex to be available */
+	locking = down_interruptible(&(mdev->mq->sem));
+	if (locking) {
+		printk(KERN_INFO "Maple: VMU at (%d, %d) is locked -"
+			" aborting read\n", mdev->unit, mdev->port);
+		goto fail_nosendbuf;
+	}
+	mdev->mq->command = MAPLE_COMMAND_BREAD;
+	mdev->mq->length = 2;
+
+	sendbuf = kzalloc(mdev->mq->length * 4, GFP_KERNEL);
+	if (!sendbuf) {
+		error = -ENOMEM;
+		goto fail_nosendbuf;
+	}
+
+	((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
+	((unsigned long *)sendbuf)[1] = cpu_to_be32(partition << 24 | num);
+
+	mdev->mq->sendbuf = sendbuf;
+	block_read = 0;
+
+	maple_getcond_callback(mdev, vmu_blockread, 0, MAPLE_FUNC_MEMCARD);
+	maple_add_packet(mdev->mq);
+	wait_event_interruptible_timeout(vmu_read, block_read, HZ * 4);
+	if (block_read == 0) {
+		printk(KERN_INFO "Maple: VMU read failed on block 0x%X\n", num);
+		goto fail_blockread;
+	}
+
+	memcpy(buf, card->blockread, card->blocklen);
+	kfree(card->blockread);
+	kfree(sendbuf);
+
+	return 0;
+
+fail_blockread:
+	kfree(sendbuf);
+fail_nosendbuf:
+	return -1;
+}
+
+/* communicate with maple bus for phased writing */
+static int maple_vmu_write_block(unsigned int num, const unsigned char *buf,
+	struct mtd_info *mtd)
+{
+	struct memcard *card;
+	struct mdev_part *mpart;
+	struct maple_device *mdev;
+	int partition, error, locking, x, phaselen;
+	void *sendbuf;
+
+	mpart = mtd->priv;
+	mdev = mpart->mdev;
+	partition = mpart->partition;
+	card = mdev->private_data;
+
+	phaselen = card->blocklen/card->writecnt;
+	mdev->mq->command = MAPLE_COMMAND_BWRITE;
+	mdev->mq->length = phaselen / 4 + 2;
+
+	sendbuf = kmalloc(mdev->mq->length * 4, GFP_KERNEL);
+	if (!sendbuf) {
+		error = -ENOMEM;
+		goto fail_nosendbuf;
+	}
+
+	((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
+
+	for (x = 0; x < card->writecnt; x++) {
+		/* take the lock to protect the contents of sendbuf */
+		locking = down_interruptible(&mdev->mq->sem);
+		if (locking) {
+			error = -EBUSY;
+			goto fail_nolock;
+		}
+		((unsigned long *)sendbuf)[1] =
+			cpu_to_be32(partition << 24 | x << 16 | num);
+		memcpy(sendbuf + 8, buf + phaselen * x, phaselen);
+		mdev->mq->sendbuf = sendbuf;
+		/* wait for the mutex to be available */
+		maple_add_packet(mdev->mq);
+	}
+	/* wait until command is processed */
+	locking = down_interruptible(&mdev->mq->sem);
+	if (locking) {
+		error = -EBUSY;
+		goto fail_nolock;
+	}
+	up(&mdev->mq->sem);
+
+	kfree(sendbuf);
+
+	return card->blocklen;
+
+fail_nolock:
+	printk(KERN_INFO "Maple: VMU at (%d, %d) is locked -"
+		" aborting write\n", mdev->unit, mdev->port);
+	kfree(sendbuf);
+fail_nosendbuf:
+	printk("Maple: VMU (%d, %d): write failed\n", mdev->port, mdev->unit);
+	return error;
+}
+
+/* mtd function to simulate reading byte by byte */
+static unsigned char vmu_flash_read_char(unsigned long ofs, int *retval,
+	struct mtd_info *mtd)
+{
+	struct vmu_block *vblock;
+	struct memcard *card;
+	struct mdev_part *mpart;
+	struct maple_device *mdev;
+	unsigned char *buf, ret;
+	int partition, error;
+
+	mpart = mtd->priv;
+	mdev = mpart->mdev;
+	partition = mpart->partition;
+	card = mdev->private_data;
+	*retval =  0;
+	buf = kmalloc(card->blocklen, GFP_KERNEL);
+	if (!buf) {
+		*retval = 1;
+		error = ENOMEM;
+		goto fail_buffer;
+	}
+
+	vblock = ofs_to_block(ofs, mtd, partition);
+	if (!vblock) {
+		*retval = 3;
+		error = ENOMEM;
+		goto invalid_block;
+	}
+
+	error = maple_vmu_read_block(vblock->num, buf, mtd);
+	if (error) {
+		*retval = 2;
+		goto failed_block;
+	}
+	ret = buf[vblock->ofs];
+	kfree(buf);
+	kfree(vblock);
+	return ret;
+
+failed_block:
+	kfree(vblock);
+invalid_block:
+	kfree(buf);
+fail_buffer:
+	return -error;
+}
+
+/* mtd higher order function to read flash */
+static int vmu_flash_read(struct mtd_info *mtd, loff_t from, size_t len,
+	size_t *retlen,  u_char *buf)
+{
+	struct maple_device *mdev;
+	struct memcard *card;
+	struct mdev_part *mpart;
+	struct vmu_cache *pcache;
+	struct vmu_block *vblock = NULL;
+	int index = 0, retval, partition, leftover, numblocks;
+	unsigned char cx;
+
+	mpart = mtd->priv;
+	mdev = mpart->mdev;
+	partition = mpart->partition;
+	card = mdev->private_data;
+
+	if (len < 1)
+		return -1;
+	numblocks = card->parts[partition].numblocks;
+	if (from + len > numblocks * card->blocklen)
+		len = numblocks * card->blocklen - from;
+	if (len == 0)
+		return -1;
+	/* Have we cached this bit already? */
+	pcache = (card->parts[partition]).pcache;
+	do {
+		if (pcache->valid &&
+		   time_before(jiffies, pcache->jiffies_atc + HZ)) {
+			vblock = ofs_to_block(from + index, mtd, partition);
+			if (!vblock)
+				return -ENOMEM;
+			if (pcache->block == vblock->num) {
+				/*we have cached it, so do necessary copying */
+				leftover = card->blocklen - vblock->ofs;
+				if (leftover > len - index) {
+					/* only a bit of this block to copy */
+					memcpy(buf + index,
+						pcache->buffer + vblock->ofs,
+						len - index);
+					index = len;
+					kfree(vblock);
+					break;
+				}
+				memcpy(buf + index, pcache->buffer +
+					vblock->ofs, leftover);
+				index += leftover;
+			} else {
+				kfree(vblock);
+				vblock = NULL;
+			}
+		}
+		if (!vblock) {
+			/* Not cached */
+			cx = vmu_flash_read_char(from + index, &retval, mtd);
+			if (retval) {
+				*retlen = index;
+				return -1;
+			}
+			memset(buf + index, cx, 1);
+			index++;
+		}
+		kfree(vblock);
+		vblock = NULL;
+	} while (len > index);
+	*retlen = index;
+
+	return 0;
+}
+
+static int vmu_flash_write(struct mtd_info *mtd, loff_t to, size_t len,
+	size_t *retlen, const u_char *buf)
+{
+	struct maple_device *mdev;
+	struct memcard *card;
+	struct mdev_part *mpart;
+	int index = 0, retval, partition, error = 0, numblocks;
+	struct vmu_cache *pcache;
+	struct vmu_block *vblock;
+	unsigned char *buffer;
+
+	mpart = mtd->priv;
+	mdev = mpart->mdev;
+	partition = mpart->partition;
+	card = mdev->private_data;
+
+	/* simple sanity checks */
+	if (len < 1) {
+		error = -1;
+		goto failed;
+	}
+	numblocks = card->parts[partition].numblocks;
+	if (to + len > numblocks * card->blocklen)
+		len = numblocks * card->blocklen - to;
+	if (len == 0) {
+		error = -1;
+		goto failed;
+	}
+
+	vblock = ofs_to_block(to, mtd, partition);
+	if (!vblock) {
+		error = -ENOMEM;
+		goto failed;
+	}
+
+	buffer = kmalloc(card->blocklen, GFP_KERNEL);
+	if (!buffer) {
+		error = -ENOMEM;
+		goto fail_buffer;
+	}
+
+	do {
+
+		/* Read in the block we are to write to */
+		if (maple_vmu_read_block(vblock->num, buffer, mtd)) {
+			error = -EIO;
+			goto fail_io;
+		}
+
+		do {
+			buffer[vblock->ofs] = buf[index];
+			vblock->ofs++;
+			index++;
+			if (index >= len)
+				break;
+		} while (vblock->ofs < card->blocklen);
+		/* write out new buffer */
+		retval = maple_vmu_write_block(vblock->num, buffer, mtd);
+		/* invalidare the cache */
+		pcache = (card->parts[partition]).pcache;
+		pcache->valid = 0;
+
+		if (retval != card->blocklen) {
+			error = -EIO;
+			goto fail_io;
+		}
+
+		vblock->num++;
+		vblock->ofs = 0;
+	} while (len > index);
+
+	kfree(buffer);
+	*retlen = index;
+	kfree(vblock);
+	return 0;
+
+fail_io:
+	kfree(buffer);
+fail_buffer:
+	kfree(vblock);
+failed:
+	printk(KERN_INFO "Maple: VMU write failing with error %d\n", error);
+	return error;
+}
+
+static int vmu_flash_erase(struct mtd_info *mtd, struct erase_info *erase)
+{
+	int z;
+	erase->state = MTD_ERASING;
+	vmu_flash_write(mtd, erase->addr, erase->len, &z, "\0");
+	erase->state = MTD_ERASE_DONE;
+	if (erase->callback)
+		(erase->callback) (erase);
+	return 0;
+}
+
+static void vmu_flash_sync(struct mtd_info *mtd)
+{
+	/* Do nothing */
+}
+
+/* Maple bus callback function to recursively query hardware details */
+static void vmu_queryblocks(struct mapleq *mq)
+{
+	struct maple_device *mdev;
+	unsigned short *res;
+	struct memcard *card;
+	void *sendbuf;
+	struct vmu_cache *pcache;
+	struct mdev_part *mpart;
+	struct mtd_info *mtd_cur;
+	struct vmupart *part_cur;
+	int error, locking;
+
+	mdev = mq->dev;
+	card = mdev->private_data;
+	res = mq->recvbuf;
+	card->tempA = res[12];
+	card->tempB = res[6];
+
+	printk(KERN_INFO "Maple: VMU device at partition %d has %d user "
+		"blocks with a root block at %d\n", card->partition,
+		card->tempA, card->tempB);
+
+	part_cur = &((card->parts)[card->partition]);
+	part_cur->user_blocks = card->tempA;
+	part_cur->root_block = card->tempB;
+	part_cur->numblocks = card->tempB + 1;
+	part_cur->name = kmalloc(12, GFP_KERNEL);
+	if (!part_cur->name)
+		goto fail_name;
+
+	sprintf(part_cur->name, "vmu%d.%d.%d",
+		mdev->port, mdev->unit, card->partition);
+	mtd_cur = &((card->mtd)[card->partition]);
+	mtd_cur->name = part_cur->name;
+	mtd_cur->type = MTD_NORFLASH;
+	mtd_cur->flags = MTD_CAP_NORFLASH;
+	mtd_cur->size = part_cur->numblocks * card->blocklen;
+	mtd_cur->erasesize = card->blocklen;
+	mtd_cur->write = vmu_flash_write;
+	mtd_cur->read = vmu_flash_read;
+	mtd_cur->erase = vmu_flash_erase;
+	mtd_cur->sync = vmu_flash_sync;
+	mtd_cur->writesize = card->blocklen;
+
+	mpart = kmalloc(sizeof(struct mdev_part), GFP_KERNEL);
+	if (!mpart)
+		goto fail_mpart;
+
+	mpart->mdev = mdev;
+	mpart->partition = card->partition;
+	mtd_cur->priv = mpart;
+	mtd_cur->owner = THIS_MODULE;
+
+	pcache = kmalloc(sizeof(struct vmu_cache), GFP_KERNEL);
+	if (!pcache)
+		goto fail_cache_create;
+	pcache->buffer = NULL;
+	pcache->valid = 0;
+	part_cur->pcache = pcache;
+
+	error = add_mtd_device(mtd_cur);
+	if (error)
+		goto fail_mtd_register;
+
+	kfree(card->sendbuf);
+	maple_getcond_callback(mdev, NULL, 0,
+		MAPLE_FUNC_MEMCARD);
+
+	if (++(card->partition) < card->partitions) {
+		mdev->mq->command = MAPLE_COMMAND_GETMINFO;
+		mdev->mq->length = 2;
+
+		sendbuf = kzalloc(mdev->mq->length * 4, GFP_KERNEL);
+		if (!sendbuf) {
+			error = -ENOMEM;
+			goto fail_nosendbuf;
+		}
+		card->sendbuf = sendbuf;
+		mdev->mq->sendbuf = sendbuf;
+		maple_getcond_callback(mdev, vmu_queryblocks, 0,
+			MAPLE_FUNC_MEMCARD);
+
+		locking = down_interruptible(&(mdev->mq->sem));
+		if (!locking)
+			maple_add_packet(mdev->mq);
+	}
+
+	return;
+
+fail_mtd_register:
+	printk("mtd: Could not register maple device at (%d, %d)\n",
+		mdev->port, mdev->unit);
+	for (error = 0; error <= card->partition; error++) {
+		kfree(((card->parts)[error]).pcache);
+		((card->parts)[error]).pcache = NULL;
+	}
+fail_cache_create:
+	kfree(card->sendbuf);
+fail_mpart:
+	for (error = 0; error <= card->partition; error++) {
+		kfree(((card->mtd)[error]).priv);
+		((card->mtd)[error]).priv = NULL;
+	}
+fail_nosendbuf:
+	maple_getcond_callback(mdev, NULL, 0,
+		MAPLE_FUNC_MEMCARD);
+	kfree(part_cur->name);
+fail_name:
+	return;
+}
+
+static int vmu_connect(struct maple_device *mdev)
+{
+	unsigned long test_flash_data, basic_flash_data;
+	int c, locking, error = 0;
+	struct memcard *card;
+	void *sendbuf;
+
+	test_flash_data = be32_to_cpu(mdev->devinfo.function);
+	/* Need to count how many bits are set - to find out which
+	* function_data element has details of the memory card:
+	* using Brian Kernighan's/Peter Wegner's method */
+	for (c = 0; test_flash_data; c++)
+		test_flash_data &= test_flash_data - 1;
+
+	basic_flash_data = be32_to_cpu(mdev->devinfo.function_data[c - 1]);
+
+	card = kmalloc(sizeof(struct memcard), GFP_KERNEL);
+	if (!card) {
+		error = ENOMEM;
+		goto fail_nomem;
+	}
+
+	card->partitions = ((basic_flash_data >> 24) & 0xFF) + 1;
+	card->blocklen = (((basic_flash_data >> 16) & 0xFF) + 1) << 5;
+	card->writecnt = (basic_flash_data >> 12) & 0xF;
+	card->readcnt = (basic_flash_data >> 8) & 0xF;
+	card->removeable = (basic_flash_data >> 7) & 1;
+
+	card->partition = 0;
+	/* Not sure there are actually any multi-partition devices in the
+	* real world, but the hardware supports them, so, so will we */
+	card->parts = kmalloc(sizeof(struct vmupart) * card->partitions,
+		GFP_KERNEL);
+	if (!card->parts) {
+		error = -ENOMEM;
+		goto fail_partitions;
+	}
+	/*kzalloc this to ensure safe kfree-ing of NULL mparts on error*/
+	card->mtd = kzalloc(sizeof(struct mtd_info) * card->partitions,
+		GFP_KERNEL);
+	if (!card->mtd) {
+		error = -ENOMEM;
+		goto fail_mtd_info;
+	}
+
+	mdev->private_data = card;
+
+	/* Now query the device to find out about blocks */
+	mdev->mq->command = MAPLE_COMMAND_GETMINFO;
+	mdev->mq->length = 2;
+
+	sendbuf = kzalloc(mdev->mq->length * 4, GFP_KERNEL);
+	if (!sendbuf) {
+		error = -ENOMEM;
+		goto fail_nosendbuf;
+	}
+	card->sendbuf = sendbuf;
+	mdev->mq->sendbuf = sendbuf;
+
+	((unsigned long *)(mdev->mq->sendbuf))[0] =
+		cpu_to_be32(MAPLE_FUNC_MEMCARD);
+
+	maple_getcond_callback(mdev, vmu_queryblocks, 0,
+		MAPLE_FUNC_MEMCARD);
+
+	locking = down_interruptible(&(mdev->mq->sem));
+	if (!locking)
+		maple_add_packet(mdev->mq);
+
+	return 0;
+
+fail_nosendbuf:
+	kfree(card->mtd);
+fail_mtd_info:
+	kfree(card->parts);
+fail_partitions:
+	kfree(card);
+fail_nomem:
+	return -error;
+}
+
+static void vmu_disconnect(struct maple_device *mdev)
+{
+	struct memcard *card;
+	int x, locking;
+
+	locking = down_interruptible(&mdev->mq->sem);
+	if (locking) {
+		printk(KERN_INFO "Maple: Could not disconnect VMU device at:"
+			"(%d, %d)\n", mdev->port, mdev->unit);
+		return;
+	}
+	mdev->callback = NULL;
+	card = mdev->private_data;
+	for (x = 0; x < card->partitions; x++) {
+		del_mtd_device(&((card->mtd)[x]));
+		kfree(((card->parts)[x]).name);
+	}
+	kfree(card->parts);
+	kfree(card->mtd);
+	kfree(card);
+	up(&mdev->mq->sem);
+
+}
+
+static int probe_maple_vmu(struct device *dev)
+{
+	int error;
+	struct maple_device *mdev = to_maple_dev(dev);
+	struct maple_driver *mdrv = to_maple_driver(dev->driver);
+
+	error = vmu_connect(mdev);
+	if (error)
+		return error;
+
+	mdev->driver = mdrv;
+
+	return 0;
+}
+
+static int remove_maple_vmu(struct device *dev)
+{
+	struct maple_device *mdev = to_maple_dev(dev);
+
+	vmu_disconnect(mdev);
+	return 0;
+}
+
+static struct maple_driver vmu_flash_driver = {
+	.function =	MAPLE_FUNC_MEMCARD,
+	.connect =	vmu_connect,
+	.disconnect =	vmu_disconnect,
+	.drv = {
+		.name = "Dreamcast_visual_memory",
+		.probe = probe_maple_vmu,
+		.remove = remove_maple_vmu,
+	},
+};
+
+static int unplug_vmu_flash(struct device *dev, void *ignored)
+{
+	struct maple_device *mdev;
+
+	mdev = to_maple_dev(dev);
+	if ((mdev->function & MAPLE_FUNC_MEMCARD)
+		&& (mdev->driver == &vmu_flash_driver))
+		remove_maple_vmu(dev);
+
+	return 0;
+}
+
+static int __init vmu_flash_map_init(void)
+{
+	maple_driver_register(&vmu_flash_driver.drv);
+	return 0;
+}
+
+static void __exit vmu_flash_map_exit(void)
+{
+	bus_for_each_dev(&maple_bus_type, NULL, NULL, unplug_vmu_flash);
+	driver_unregister(&vmu_flash_driver.drv);
+}
+
+module_init(vmu_flash_map_init);
+module_exit(vmu_flash_map_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Adrian McMenamin, Paul Mundt");
+MODULE_DESCRIPTION("Flash mapping for Sega Dreamcast visual memory");

Re: [Fwd: Re: [PATCH] 2/2 mtd: Add support for the Dreamcast VMU flash]

[Jörn Engel]

Some details I noticed on first glance.

On Wed, 19 March 2008 21:20:41 +0000, Adrian McMenamin wrote:
> 
> +/* Interface with maple bus to read bytes */
> +static int maple_vmu_read_block(unsigned int num, unsigned char *buf,
> +	struct mtd_info *mtd)
> +{
> +	struct memcard *card;
> +	struct mdev_part *mpart;
> +	struct maple_device *mdev;
> +	int partition, error, locking;
> +	void *sendbuf;
> +
> +	mpart = mtd->priv;
> +	mdev = mpart->mdev;
> +	partition = mpart->partition;
> +	card = mdev->private_data;
> +
> +	/* wait for the mutex to be available */
> +	locking = down_interruptible(&(mdev->mq->sem));
> +	if (locking) {
> +		printk(KERN_INFO "Maple: VMU at (%d, %d) is locked -"
> +			" aborting read\n", mdev->unit, mdev->port);
> +		goto fail_nosendbuf;
> +	}
> +	mdev->mq->command = MAPLE_COMMAND_BREAD;
> +	mdev->mq->length = 2;
> +
> +	sendbuf = kzalloc(mdev->mq->length * 4, GFP_KERNEL);
> +	if (!sendbuf) {
> +		error = -ENOMEM;
> +		goto fail_nosendbuf;
> +	}
> +
> +	((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
> +	((unsigned long *)sendbuf)[1] = cpu_to_be32(partition << 24 | num);

unsigned long is larger than 32bit on many architectures.  Better
variant:
	__be32 *sendbuf;
	...
	sendbuf[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
	sendbuf[1] = cpu_to_be32(partition << 24 | num);

Sparse would have noticed this problem as well.  Might be a good idea to
$ make C=1 drivers/mtd/maps/vmu_flash.o
and take a look what other problems show up.

> +	mdev->mq->sendbuf = sendbuf;

Possibly the big-endian annotations need to trickly though the layers
here as well.

> +/* mtd function to simulate reading byte by byte */
> +static unsigned char vmu_flash_read_char(unsigned long ofs, int *retval,
> +	struct mtd_info *mtd)
> +{
> +	struct vmu_block *vblock;
> +	struct memcard *card;
> +	struct mdev_part *mpart;
> +	struct maple_device *mdev;
> +	unsigned char *buf, ret;
> +	int partition, error;
> +
> +	mpart = mtd->priv;
> +	mdev = mpart->mdev;
> +	partition = mpart->partition;
> +	card = mdev->private_data;
> +	*retval =  0;
> +	buf = kmalloc(card->blocklen, GFP_KERNEL);
> +	if (!buf) {
> +		*retval = 1;
> +		error = ENOMEM;
> +		goto fail_buffer;
> +	}
> +
> +	vblock = ofs_to_block(ofs, mtd, partition);
> +	if (!vblock) {
> +		*retval = 3;
> +		error = ENOMEM;
> +		goto invalid_block;
> +	}
> +
> +	error = maple_vmu_read_block(vblock->num, buf, mtd);
> +	if (error) {
> +		*retval = 2;
> +		goto failed_block;
> +	}
> +	ret = buf[vblock->ofs];
> +	kfree(buf);
> +	kfree(vblock);
> +	return ret;
> +
> +failed_block:
> +	kfree(vblock);
> +invalid_block:
> +	kfree(buf);
> +fail_buffer:
> +	return -error;

Yikes.  Errnos greater 127 do exist.  Better return a signed int and
interpret at error if <0.

You could also combine the two sets of kfree() into one, btw.

> +	do {
> +		if (pcache->valid &&
> +		   time_before(jiffies, pcache->jiffies_atc + HZ)) {
> +			vblock = ofs_to_block(from + index, mtd, partition);
> +			if (!vblock)
> +				return -ENOMEM;
> +			if (pcache->block == vblock->num) {
> +				/*we have cached it, so do necessary copying */
> +				leftover = card->blocklen - vblock->ofs;
> +				if (leftover > len - index) {
> +					/* only a bit of this block to copy */
> +					memcpy(buf + index,
> +						pcache->buffer + vblock->ofs,
> +						len - index);

Five levels of indentation are a strong indication that this code is
buggy.  I don't understand how it behaves in all corner cases and I bet
neither do you.

> +	do {
> +
> +		/* Read in the block we are to write to */
> +		if (maple_vmu_read_block(vblock->num, buffer, mtd)) {
> +			error = -EIO;
> +			goto fail_io;
> +		}
> +
> +		do {
> +			buffer[vblock->ofs] = buf[index];
> +			vblock->ofs++;
> +			index++;
> +			if (index >= len)
> +				break;
> +		} while (vblock->ofs < card->blocklen);
> +		/* write out new buffer */
> +		retval = maple_vmu_write_block(vblock->num, buffer, mtd);
> +		/* invalidare the cache */
> +		pcache = (card->parts[partition]).pcache;
> +		pcache->valid = 0;
> +
> +		if (retval != card->blocklen) {
> +			error = -EIO;
> +			goto fail_io;
> +		}
> +
> +		vblock->num++;
> +		vblock->ofs = 0;
> +	} while (len > index);

And here is an example of essentially the same loop in a fashion mere
mortals can understand.  Much better. ;)

> +MODULE_AUTHOR("Adrian McMenamin, Paul Mundt");

I believe the main use of MODULE_AUTHOR is to figure out who to send bug
reports/patches to, not to contain every possible copyright holder.  So
possibly just your name here would be more appropriate.

Jörn

-- 
A victorious army first wins and then seeks battle.
-- Sun Tzu

Re: [Fwd: Re: [PATCH] 2/2 mtd: Add support for the Dreamcast VMU flash]

[Jörn Engel]

On Thu, 20 March 2008 11:56:54 -0000, Adrian McMenamin wrote:
> >> +
> >> +	((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
> >> +	((unsigned long *)sendbuf)[1] = cpu_to_be32(partition << 24 | num);
> >
> > unsigned long is larger than 32bit on many architectures.  Better
> > variant:
> > 	__be32 *sendbuf;
> 
> 
> No, it's not. This is part of a bus subsystem where different parts of the
> incoming packets may or may not be in big endian format. Eg for block
> writes the then the opening blocks may be in big endian format but the
> data to be written is not.

Data to be written is usually treated as opaque void* pointer.  You
don't need to know what's in it, just the pointer and the length.  But
once you dereference the data, it is no longer opaque and you better
know its endianness.

So why do you use void* for data you actually dereference?

> If you insist on leaving open the possibility that someone might decide to
> build the electronics to attach these devices to a 64 bit machine
> (unlikely, I'd suggest) then u32 is the way to go - but are you really
> concerned that might happen?

Not too much, but there are examples where such things did happen.
emac3 is one.

> >> +fail_buffer:
> >> +	return -error;
> >
> > Yikes.  Errnos greater 127 do exist.  Better return a signed int and
> > interpret at error if <0.
> >
> 
> There is an error here, actually, but not as you describe. However as the
> errors defined in errno-base are all positive integers I don't see how
> your proposal works.

A couple of lines down you have "return -ENOMEM;".  Notice the "-"?  As
a general rule, negative return values in the kernel are errors,
positive or zero are non-errors.  Exceptions exist, but they tend to
surprise users and cause subtle bugs.

> > You could also combine the two sets of kfree() into one, btw.
> >
> >> +	do {
> >> +		if (pcache->valid &&
> >> +		   time_before(jiffies, pcache->jiffies_atc + HZ)) {
> >> +			vblock = ofs_to_block(from + index, mtd, partition);
> >> +			if (!vblock)
> >> +				return -ENOMEM;
> >> +			if (pcache->block == vblock->num) {
> >> +				/*we have cached it, so do necessary copying */
> >> +				leftover = card->blocklen - vblock->ofs;
> >> +				if (leftover > len - index) {
> >> +					/* only a bit of this block to copy */
> >> +					memcpy(buf + index,
> >> +						pcache->buffer + vblock->ofs,
> >> +						len - index);
> >
> > Five levels of indentation are a strong indication that this code is
> > buggy.  I don't understand how it behaves in all corner cases and I bet
> > neither do you.
> 
> It's not buggy - and it seems pretty clear to me what is happening here.
> What's the issue?

Every single time I've had to deal with such levels of indentation
before I have found a bug.  No exceptions.  And every single time it was
a _lot_ of work because the code was too complicated for my little brain
to completely understand.  My preferred method is to rework the code
until it is simple enough even for me.

But as long as you are willing to maintain this beast and deal with any
bug reports, I don't mind either way.

Jörn

-- 
The only good bug is a dead bug.
-- Starship Troopers

Re: [Fwd: Re: [PATCH] 2/2 mtd: Add support for the Dreamcast VMU flash]

[Adrian McMenamin]

On Thu, March 20, 2008 10:03 am, Jörn Engel wrote:
> Some details I noticed on first glance.
>
> On Wed, 19 March 2008 21:20:41 +0000, Adrian McMenamin wrote:
>>
>> +/* Interface with maple bus to read bytes */
>> +static int maple_vmu_read_block(unsigned int num, unsigned char *buf,
>> +	struct mtd_info *mtd)
>> +{
>> +	struct memcard *card;
>> +	struct mdev_part *mpart;
>> +	struct maple_device *mdev;
>> +	int partition, error, locking;
>> +	void *sendbuf;
>> +
>> +	mpart = mtd->priv;
>> +	mdev = mpart->mdev;
>> +	partition = mpart->partition;
>> +	card = mdev->private_data;
>> +
>> +	/* wait for the mutex to be available */
>> +	locking = down_interruptible(&(mdev->mq->sem));
>> +	if (locking) {
>> +		printk(KERN_INFO "Maple: VMU at (%d, %d) is locked -"
>> +			" aborting read\n", mdev->unit, mdev->port);
>> +		goto fail_nosendbuf;
>> +	}
>> +	mdev->mq->command = MAPLE_COMMAND_BREAD;
>> +	mdev->mq->length = 2;
>> +
>> +	sendbuf = kzalloc(mdev->mq->length * 4, GFP_KERNEL);
>> +	if (!sendbuf) {
>> +		error = -ENOMEM;
>> +		goto fail_nosendbuf;
>> +	}
>> +
>> +	((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
>> +	((unsigned long *)sendbuf)[1] = cpu_to_be32(partition << 24 | num);
>
> unsigned long is larger than 32bit on many architectures.  Better
> variant:
> 	__be32 *sendbuf;


No, it's not. This is part of a bus subsystem where different parts of the
incoming packets may or may not be in big endian format. Eg for block
writes the then the opening blocks may be in big endian format but the
data to be written is not.

If you insist on leaving open the possibility that someone might decide to
build the electronics to attach these devices to a 64 bit machine
(unlikely, I'd suggest) then u32 is the way to go - but are you really
concerned that might happen?



>> +fail_buffer:
>> +	return -error;
>
> Yikes.  Errnos greater 127 do exist.  Better return a signed int and
> interpret at error if <0.
>

There is an error here, actually, but not as you describe. However as the
errors defined in errno-base are all positive integers I don't see how
your proposal works.

> You could also combine the two sets of kfree() into one, btw.
>
>> +	do {
>> +		if (pcache->valid &&
>> +		   time_before(jiffies, pcache->jiffies_atc + HZ)) {
>> +			vblock = ofs_to_block(from + index, mtd, partition);
>> +			if (!vblock)
>> +				return -ENOMEM;
>> +			if (pcache->block == vblock->num) {
>> +				/*we have cached it, so do necessary copying */
>> +				leftover = card->blocklen - vblock->ofs;
>> +				if (leftover > len - index) {
>> +					/* only a bit of this block to copy */
>> +					memcpy(buf + index,
>> +						pcache->buffer + vblock->ofs,
>> +						len - index);
>
> Five levels of indentation are a strong indication that this code is
> buggy.  I don't understand how it behaves in all corner cases and I bet
> neither do you.
>

It's not buggy - and it seems pretty clear to me what is happening here.
What's the issue?


>> +	do {
>> +
>> +		/* Read in the block we are to write to */
>> +		if (maple_vmu_read_block(vblock->num, buffer, mtd)) {
>> +			error = -EIO;
>> +			goto fail_io;
>> +		}
>> +
>> +		do {
>> +			buffer[vblock->ofs] = buf[index];
>> +			vblock->ofs++;
>> +			index++;
>> +			if (index >= len)
>> +				break;
>> +		} while (vblock->ofs < card->blocklen);
>> +		/* write out new buffer */
>> +		retval = maple_vmu_write_block(vblock->num, buffer, mtd);
>> +		/* invalidare the cache */
>> +		pcache = (card->parts[partition]).pcache;
>> +		pcache->valid = 0;
>> +
>> +		if (retval != card->blocklen) {
>> +			error = -EIO;
>> +			goto fail_io;
>> +		}
>> +
>> +		vblock->num++;
>> +		vblock->ofs = 0;
>> +	} while (len > index);
>
> And here is an example of essentially the same loop in a fashion mere
> mortals can understand.  Much better. ;)


That's because the writes, for fairly obvious reasons (ie once you've
written to a block you've changed it), are not cached in the same way.

>
>> +MODULE_AUTHOR("Adrian McMenamin, Paul Mundt");
>
> I believe the main use of MODULE_AUTHOR is to figure out who to send bug
> reports/patches to, not to contain every possible copyright holder.  So
> possibly just your name here would be more appropriate.
>
>
fair enough.

Re: [Fwd: Re: [PATCH] 2/2 mtd: Add support for the Dreamcast VMU flash]

[Jörn Engel]

On Thu, 20 March 2008 12:56:42 -0000, Adrian McMenamin wrote:
> On Thu, March 20, 2008 11:20 am, Jörn Engel wrote:
> > On Thu, 20 March 2008 11:56:54 -0000, Adrian McMenamin wrote:
> >> >> +
> >> >> +	((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
> >> >> +	((unsigned long *)sendbuf)[1] = cpu_to_be32(partition << 24 | num);
> > So why do you use void* for data you actually dereference?
> 
> Because for other devices data can be passed as 16 bit structures as well
> as 32 bit ones.

Hogwash!  If you ever deal with 16bit structures here you have a bug.
cpu_to_be32() only makes sense when actually dealing with __be32.
Either your variable is and should be called __be32, or it isn't and you
have a bug right in front of you.

Jörn

-- 
It does not matter how slowly you go, so long as you do not stop.
-- Confucius

Re: [Fwd: Re: [PATCH] 2/2 mtd: Add support for the Dreamcast VMU flash]

[Adrian McMenamin]

On Thu, March 20, 2008 11:20 am, Jörn Engel wrote:
> On Thu, 20 March 2008 11:56:54 -0000, Adrian McMenamin wrote:
>> >> +
>> >> +	((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
>> >> +	((unsigned long *)sendbuf)[1] = cpu_to_be32(partition << 24 | num);
>> >
>> > unsigned long is larger than 32bit on many architectures.  Better
>> > variant:
>> > 	__be32 *sendbuf;
>>
>>
>> No, it's not. This is part of a bus subsystem where different parts of
>> the
>> incoming packets may or may not be in big endian format. Eg for block
>> writes the then the opening blocks may be in big endian format but the
>> data to be written is not.
>
> Data to be written is usually treated as opaque void* pointer.  You
> don't need to know what's in it, just the pointer and the length.  But
> once you dereference the data, it is no longer opaque and you better
> know its endianness.
>
> So why do you use void* for data you actually dereference?


Because for other devices data can be passed as 16 bit structures as well
as 32 bit ones.


>.
>
> A couple of lines down you have "return -ENOMEM;".  Notice the "-"?  As
> a general rule, negative return values in the kernel are errors,
> positive or zero are non-errors.  Exceptions exist, but they tend to
> surprise users and cause subtle bugs.
>

Ah. I understand your point better now. I'd basically saw that too - but I
didn't realise that is what you meant.

Re: [Fwd: Re: [PATCH] 2/2 mtd: Add support for the Dreamcast VMU flash]

[Adrian McMenamin]

On Thu, 2008-03-20 at 13:31 +0100, Jörn Engel wrote:
> On Thu, 20 March 2008 12:56:42 -0000, Adrian McMenamin wrote:
> > On Thu, March 20, 2008 11:20 am, Jörn Engel wrote:
> > > On Thu, 20 March 2008 11:56:54 -0000, Adrian McMenamin wrote:
> > >> >> +
> > >> >> +	((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
> > >> >> +	((unsigned long *)sendbuf)[1] = cpu_to_be32(partition << 24 | num);
> > > So why do you use void* for data you actually dereference?
> > 
> > Because for other devices data can be passed as 16 bit structures as well
> > as 32 bit ones.
> 
> Hogwash!  If you ever deal with 16bit structures here you have a bug.
> cpu_to_be32() only makes sense when actually dealing with __be32.
> Either your variable is and should be called __be32, or it isn't and you
> have a bug right in front of you.

You don't seem to have grasped that this is one type of device on a
proprietary bus which has other (types of) devices. I keep the code
consistent so that the API runs the same across the different devices -
makes the code easier to understand and maintain.

And given that this is also the variable that points to the memory block
that also includes the 8 bit based date that gets read in for block
writes I it makes perfect sense to have this as a void*.

Re: [Fwd: Re: [PATCH] 2/2 mtd: Add support for the Dreamcast VMU flash]

[Jörn Engel]

On Thu, 20 March 2008 20:11:01 +0000, Adrian McMenamin wrote:
> 
> You don't seem to have grasped that this is one type of device on a
> proprietary bus which has other (types of) devices. I keep the code
> consistent so that the API runs the same across the different devices -
> makes the code easier to understand and maintain.
> 
> And given that this is also the variable that points to the memory block
> that also includes the 8 bit based date that gets read in for block
> writes I it makes perfect sense to have this as a void*.

Maybe this is a misunderstanding.  If you are arguing about this bit
several mails back:
---<snip>---
> +     mdev->mq->sendbuf = sendbuf;

Possibly the big-endian annotations need to trickly though the layers
here as well.
---<snap>---
Then I agree.  For a bus driver that only takes opaque data and moves it
between device driver and hardware, void* is the type to choose.  But if
you are arguing about the actual device driver, I couldn't disagree
more.

Please take a look at the function below, maybe it becomes clearer then.

static int maple_vmu_read_block(unsigned int num, unsigned char *buf,
       struct mtd_info *mtd)
{
       struct memcard *card;
       struct mdev_part *mpart;
       struct maple_device *mdev;
       int partition, error, locking;
       __be32 *sendbuf;

       mpart = mtd->priv;
       mdev = mpart->mdev;
       partition = mpart->partition;
       card = mdev->private_data;

       /* wait for the mutex to be available */
       locking = down_interruptible(&(mdev->mq->sem));
       if (locking) {
               printk(KERN_INFO "Maple: VMU at (%d, %d) is locked -"
                       " aborting read\n", mdev->unit, mdev->port);
               return -EIO;
       }
       mdev->mq->command = MAPLE_COMMAND_BREAD;
       mdev->mq->length = 2;

       sendbuf = kzalloc(mdev->mq->length * 4, GFP_KERNEL);
       if (!sendbuf)
               return -ENOMEM;
       
       sendbuf[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
       sendbuf[1] = cpu_to_be32(partition << 24 | num);
  
       mdev->mq->sendbuf = sendbuf;
       block_read = 0;
       
       maple_getcond_callback(mdev, vmu_blockread, 0, MAPLE_FUNC_MEMCARD);
       maple_add_packet(mdev->mq);
       wait_event_interruptible_timeout(vmu_read, block_read, HZ * 4);
       if (block_read == 0) {
               printk(KERN_INFO "Maple: VMU read failed on block 0x%X\n", num);
	       error = -EIO;
               goto out;
       }
       /* FIXME: we kfree a data structure that was allocated elsewhere.
        * Either move allocation and freeing to the same function or
	* thoroughly document this to avoid subtle bugs after future
	* code changes.
	*/
       memcpy(buf, card->blockread, card->blocklen);
       kfree(card->blockread);

       error = 0;
out:
       kfree(sendbuf);
       return error;
}

And after doing these changes a couple more problems stuck out like sore
thumbs, f.e. the FIXME above.  But we can discuss those later.

Jörn

-- 
If you're willing to restrict the flexibility of your approach,
you can almost always do something better.
-- John Carmack

Re: [Fwd: Re: [PATCH] 2/2 mtd: Add support for the Dreamcast VMU flash]

[Adrian McMenamin]

On Thu, 2008-03-20 at 22:58 +0100, Jörn Engel wrote:
> On Thu, 20 March 2008 20:11:01 +0000, Adrian McMenamin wrote:
> > 
> > You don't seem to have grasped that this is one type of device on a
> > proprietary bus which has other (types of) devices. I keep the code
> > consistent so that the API runs the same across the different devices -
> > makes the code easier to understand and maintain.
> > 
> > And given that this is also the variable that points to the memory block
> > that also includes the 8 bit based date that gets read in for block
> > writes I it makes perfect sense to have this as a void*.
> 
> Maybe this is a misunderstanding.  If you are arguing about this bit
> several mails back:
> ---<snip>---
> > +     mdev->mq->sendbuf = sendbuf;
> 
> Possibly the big-endian annotations need to trickly though the layers
> here as well.
> ---<snap>---
> Then I agree.  For a bus driver that only takes opaque data and moves it
> between device driver and hardware, void* is the type to choose.  But if
> you are arguing about the actual device driver, I couldn't disagree
> more.


I'm sorry, but the comment above <snap> isn't good english. What do you
mean? 

> 
> Please take a look at the function below, maybe it becomes clearer then.
> 
> static int maple_vmu_read_block(unsigned int num, unsigned char *buf,
>        struct mtd_info *mtd)
> {
>        struct memcard *card;
>        struct mdev_part *mpart;
>        struct maple_device *mdev;
>        int partition, error, locking;
>        __be32 *sendbuf;
> 
>        mpart = mtd->priv;
>        mdev = mpart->mdev;
>        partition = mpart->partition;
>        card = mdev->private_data;
> 
>        /* wait for the mutex to be available */
>        locking = down_interruptible(&(mdev->mq->sem));
>        if (locking) {
>                printk(KERN_INFO "Maple: VMU at (%d, %d) is locked -"
>                        " aborting read\n", mdev->unit, mdev->port);
>                return -EIO;
>        }
>        mdev->mq->command = MAPLE_COMMAND_BREAD;
>        mdev->mq->length = 2;
> 
>        sendbuf = kzalloc(mdev->mq->length * 4, GFP_KERNEL);
>        if (!sendbuf)
>                return -ENOMEM;
>        
>        sendbuf[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
>        sendbuf[1] = cpu_to_be32(partition << 24 | num);
>   
>        mdev->mq->sendbuf = sendbuf;
>        block_read = 0;
>        
>        maple_getcond_callback(mdev, vmu_blockread, 0, MAPLE_FUNC_MEMCARD);
>        maple_add_packet(mdev->mq);
>        wait_event_interruptible_timeout(vmu_read, block_read, HZ * 4);
>        if (block_read == 0) {
>                printk(KERN_INFO "Maple: VMU read failed on block 0x%X\n", num);
> 	       error = -EIO;
>                goto out;
>        }
>        /* FIXME: we kfree a data structure that was allocated elsewhere.
>         * Either move allocation and freeing to the same function or
> 	* thoroughly document this to avoid subtle bugs after future
> 	* code changes.
> 	*/
>        memcpy(buf, card->blockread, card->blocklen);
>        kfree(card->blockread);
> 
>        error = 0;
> out:
>        kfree(sendbuf);
>        return error;
> }
> 
> And after doing these changes a couple more problems stuck out like sore
> thumbs, f.e. the FIXME above.  But we can discuss those later.



What is the issue? That the code is wrong or the documentation is
inadequate?

Re: [Fwd: Re: [PATCH] 2/2 mtd: Add support for the Dreamcast VMU flash]

[Jörn Engel]

On Thu, 20 March 2008 22:27:57 +0000, Adrian McMenamin wrote:
> 
> I'm sorry, but the comment above <snap> isn't good english. What do you
> mean? 

Ok, I give up.  There are far too many misunderstandings between us for
any sort of useful discussion.  No matter who is to blame, it would be
pointless to continue.

Maybe someone else has better luck than me.

Jörn

-- 
Eighty percent of success is showing up.
-- Woody Allen