Re: [PATCH] 2/3 mtd: add support for flash on the SEGA Dreamcast Visual Memory Unit

["Jörn Engel" <joern@logfs.org>]

[-- Attachment #1: Type: text/plain, Size: 809 bytes --]

On Mon, 24 March 2008 13:58:40 +0000, Adrian McMenamin wrote:
> 
> Tempting as it is to continue the war, discretion will be the better
> part of valour here and I will give you the last word.
> 
> Of course I don't mind you sending patches. Indeed it would be very
> helpful and generous of you to do so.

Good.  The first four shouldn't change any behaviour.  Number five flips
positive error returns into negative ones.  I believe the old behaviour
is a bug.  Worth a second look to make sure.

All five patches are attached.  Hope that doesn't cause any problems.

Jörn

-- 
There are two ways of constructing a software design: one way is to make
it so simple that there are obviously no deficiencies, and the other is
to make it so complicated that there are no obvious deficiencies.
-- C. A. R. Hoare

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: cu1.patch --]
[-- Type: text/x-diff; charset=utf-8, Size: 638 bytes --]


Directly include two headers.  While unnecessary on sh, this allows the driver
to be compiled on i386 for those lacking a cross-compiler setup.

Signed-off-by: Jörn Engel <joern@logfs.org>
---

 drivers/mtd/maps/vmu_flash.c |    2 ++
 1 file changed, 2 insertions(+)

--- maple/drivers/mtd/maps/vmu_flash.c~cu1	2008-03-24 14:36:51.000000000 +0100
+++ maple/drivers/mtd/maps/vmu_flash.c	2008-03-24 14:39:05.000000000 +0100
@@ -12,6 +12,8 @@
  * GNU General Public Licence
  */
 #include <linux/init.h>
+#include <linux/sched.h>
+#include <linux/jiffies.h>
 #include <linux/maple.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/map.h>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: cu2.patch --]
[-- Type: text/x-diff; charset=utf-8, Size: 2136 bytes --]


Minimal changes to make sparse happier.

Signed-off-by: Jörn Engel <joern@logfs.org>
---

 drivers/mtd/maps/vmu_flash.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

--- maple/drivers/mtd/maps/vmu_flash.c~cu2	2008-03-24 14:39:05.000000000 +0100
+++ maple/drivers/mtd/maps/vmu_flash.c	2008-03-24 14:44:24.000000000 +0100
@@ -136,7 +136,7 @@ static int maple_vmu_read_block(unsigned
 	struct mdev_part *mpart;
 	struct maple_device *mdev;
 	int partition, error = 0, locking;
-	void *sendbuf;
+	__be32 *sendbuf;
 
 	mpart = mtd->priv;
 	mdev = mpart->mdev;
@@ -162,8 +162,8 @@ static int maple_vmu_read_block(unsigned
 		goto fail_nosendbuf;
 	}
 
-	((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
-	((unsigned long *)sendbuf)[1] = cpu_to_be32(partition << 24 | num);
+	sendbuf[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
+	sendbuf[1] = cpu_to_be32(partition << 24 | num);
 
 	mdev->mq->sendbuf = sendbuf;
 	block_read = 0;
@@ -207,7 +207,7 @@ static int maple_vmu_write_block(unsigne
 	struct mdev_part *mpart;
 	struct maple_device *mdev;
 	int partition, error, locking, x, phaselen;
-	void *sendbuf;
+	__be32 *sendbuf;
 
 	mpart = mtd->priv;
 	mdev = mpart->mdev;
@@ -224,7 +224,7 @@ static int maple_vmu_write_block(unsigne
 		goto fail_nosendbuf;
 	}
 
-	((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
+	sendbuf[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
 
 	for (x = 0; x < card->writecnt; x++) {
 		/* take the lock to protect the contents of sendbuf */
@@ -233,8 +233,7 @@ static int maple_vmu_write_block(unsigne
 			error = -EBUSY;
 			goto fail_nolock;
 		}
-		((unsigned long *)sendbuf)[1] =
-			cpu_to_be32(partition << 24 | x << 16 | num);
+		sendbuf[1] = cpu_to_be32(partition << 24 | x << 16 | num);
 		memcpy(sendbuf + 8, buf + phaselen * x, phaselen);
 		mdev->mq->sendbuf = sendbuf;
 		maple_add_packet(mdev->mq);
@@ -467,7 +466,7 @@ failed:
 
 static int vmu_flash_erase(struct mtd_info *mtd, struct erase_info *erase)
 {
-	int z;
+	size_t z;
 	erase->state = MTD_ERASING;
 	vmu_flash_write(mtd, erase->addr, erase->len, &z, "\0");
 	erase->state = MTD_ERASE_DONE;

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: cu3.patch --]
[-- Type: text/x-diff; charset=utf-8, Size: 2848 bytes --]


Shrink the goto logic where easily possible.  Changing the return type to int
allows vmu_flash_read_char() to follow normal Linux style.

Signed-off-by: Jörn Engel <joern@logfs.org>
---

 drivers/mtd/maps/vmu_flash.c |   37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

--- maple/drivers/mtd/maps/vmu_flash.c~cu3	2008-03-24 14:44:50.000000000 +0100
+++ maple/drivers/mtd/maps/vmu_flash.c	2008-03-24 14:55:22.000000000 +0100
@@ -76,22 +76,19 @@ static struct vmu_block *ofs_to_block(un
 	card = mdev->private_data;
 
 	if (src_ofs >= ((card->parts)[partition]).numblocks * card->blocklen)
-		goto failed;
+		return NULL;
 
 	num = src_ofs / card->blocklen;
 	if (num > ((card->parts)[partition]).numblocks)
-		goto failed;
+		return NULL;
 
 	vblock = kmalloc(sizeof(struct vmu_block), GFP_KERNEL);
 	if (!vblock)
-		goto failed;
+		return NULL;
 
 	vblock->num = num;
 	vblock->ofs = src_ofs % card->blocklen;
 	return vblock;
-
-failed:
-	return NULL;
 }
 
 
@@ -264,15 +261,15 @@ fail_nosendbuf:
 }
 
 /* mtd function to simulate reading byte by byte */
-static unsigned char vmu_flash_read_char(unsigned long ofs, int *retval,
+static int 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;
+	unsigned char *buf;
+	int partition, ret;
 
 	mpart = mtd->priv;
 	mdev = mpart->mdev;
@@ -282,33 +279,27 @@ static unsigned char vmu_flash_read_char
 	buf = kmalloc(card->blocklen, GFP_KERNEL);
 	if (!buf) {
 		*retval = 1;
-		error = -ENOMEM;
-		goto fail_buffer;
+		return -ENOMEM;
 	}
 
 	vblock = ofs_to_block(ofs, mtd, partition);
 	if (!vblock) {
 		*retval = 3;
-		error = -ENOMEM;
+		ret = -ENOMEM;
 		goto invalid_block;
 	}
 
-	error = maple_vmu_read_block(vblock->num, buf, mtd);
-	if (error) {
+	ret = maple_vmu_read_block(vblock->num, buf, mtd);
+	if (ret) {
 		*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;
+	return ret;
 }
 
 /* mtd higher order function to read flash */
@@ -321,7 +312,7 @@ static int vmu_flash_read(struct mtd_inf
 	struct vmu_cache *pcache;
 	struct vmu_block *vblock = NULL;
 	int index = 0, retval, partition, leftover, numblocks;
-	unsigned char cx;
+	int cx;
 
 	mpart = mtd->priv;
 	mdev = mpart->mdev;
@@ -364,9 +355,9 @@ static int vmu_flash_read(struct mtd_inf
 		} else {
 			/* Not cached so read from the device */
 			cx = vmu_flash_read_char(from + index, &retval, mtd);
-			if (retval) {
+			if (cx < 0) {
 				*retlen = index;
-				return -EIO;
+				return cx;
 			}
 			memset(buf + index, cx, 1);
 			index++;

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: cu4.patch --]
[-- Type: text/x-diff; charset=utf-8, Size: 2606 bytes --]


Trivial comment reformatting.  Old style was uncommon for Linux.

Signed-off-by: Jörn Engel <joern@logfs.org>
---

 drivers/mtd/maps/vmu_flash.c |   22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

--- maple/drivers/mtd/maps/vmu_flash.c~cu4	2008-03-24 14:55:22.000000000 +0100
+++ maple/drivers/mtd/maps/vmu_flash.c	2008-03-24 14:57:38.000000000 +0100
@@ -140,10 +140,10 @@ static int maple_vmu_read_block(unsigned
 	partition = mpart->partition;
 	card = mdev->private_data;
 
-	/***
-	* Maple devices include a mutex to ensure packets injected into
-	* the wait queue are not corrupted via scans for hotplug events etc
-	*/
+	/*
+	 * Maple devices include a mutex to ensure packets injected into
+	 * the wait queue are not corrupted via scans for hotplug events etc
+	 */
 	locking = mutex_lock_interruptible(&mdev->mq->mutex);
 	if (locking) {
 		printk(KERN_INFO "Maple: VMU at (%d, %d) is locked -"
@@ -235,9 +235,9 @@ static int maple_vmu_write_block(unsigne
 		mdev->mq->sendbuf = sendbuf;
 		maple_add_packet(mdev->mq);
 	}
-	/***
-	* The Maple bus driver will unlock the mutex once the command
-	* has been processed, so we'll just sleep waiting for the unlock */
+	/*
+	 * The Maple bus driver will unlock the mutex once the command
+	 * has been processed, so we'll just sleep waiting for the unlock */
 	locking = mutex_lock_interruptible(&mdev->mq->mutex);
 	if (locking) {
 		error = -EBUSY;
@@ -592,8 +592,8 @@ static int vmu_connect(struct maple_devi
 
 	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 */
+	 * 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;
 
@@ -613,14 +613,14 @@ static int vmu_connect(struct maple_devi
 
 	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 */
+	 * 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*/
+	/* 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) {

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: cu5.patch --]
[-- Type: text/x-diff; charset=utf-8, Size: 1435 bytes --]


Change return values.
- Turn -1 into -EIO.  Possibly not the best value, but better than -EPERM.
- Change -error to error to return negative error values, as is standard.

Signed-off-by: Jörn Engel <joern@logfs.org>
---

 drivers/mtd/maps/vmu_flash.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)


--- maple/drivers/mtd/maps/vmu_flash.c~cu5	2008-03-24 14:57:38.000000000 +0100
+++ maple/drivers/mtd/maps/vmu_flash.c	2008-03-24 15:02:36.000000000 +0100
@@ -320,12 +320,12 @@ static int vmu_flash_read(struct mtd_inf
 	card = mdev->private_data;
 
 	if (len < 1)
-		return -1;
+		return -EIO;
 	numblocks = card->parts[partition].numblocks;
 	if (from + len > numblocks * card->blocklen)
 		len = numblocks * card->blocklen - from;
 	if (len == 0)
-		return -1;
+		return -EIO;
 	/* Have we cached this bit already? */
 	pcache = (card->parts[partition]).pcache;
 	do {
@@ -388,14 +388,14 @@ static int vmu_flash_write(struct mtd_in
 
 	/* simple sanity checks */
 	if (len < 1) {
-		error = -1;
+		error = -EIO;
 		goto failed;
 	}
 	numblocks = card->parts[partition].numblocks;
 	if (to + len > numblocks * card->blocklen)
 		len = numblocks * card->blocklen - to;
 	if (len == 0) {
-		error = -1;
+		error = -EIO;
 		goto failed;
 	}
 
@@ -661,7 +661,7 @@ fail_mtd_info:
 fail_partitions:
 	kfree(card);
 fail_nomem:
-	return -error;
+	return error;
 }
 
 static void vmu_disconnect(struct maple_device *mdev)