Re: [PATCH 1/1] [MTD/MAPS] Blackfin Async Flash Maps: Handle the case where flash memory and ethernet mac/phy are mapped onto the same async bank

Previous thread: [PATCH] doc: Test-by? by Hidetoshi Seto on Monday, May 12, 2008 - 9:36 pm. (2 messages)

Next thread: [PATCH] flag parameters: paccept w/out set_restore_sigmask by Ulrich Drepper on Monday, May 12, 2008 - 10:05 pm. (1 message)

From: Mike Frysinger <vapier.adi@gmail.com>

[try #3] rename bf5xx-flash to bfin-async-flash
 - move all kconfig board settings into board resources
 - fixup casting style according to lkml feedback
 - rewrite driver so that it can handle arbitrary of
   instances according to the declared platform resources

[try #2]
Remove useless SSYNC() as Will said

[try #1]
The BF533-STAMP does this for example.
All board-specific configuration goes in your board resources file.

Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Cc: Joern Engel <joern@logfs.org>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/mtd/maps/Kconfig            |   11 ++
 drivers/mtd/maps/Makefile           |    1 +
 drivers/mtd/maps/bfin-async-flash.c |  235 +++++++++++++++++++++++++++++++++++
 3 files changed, 247 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mtd/maps/bfin-async-flash.c

diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
index 17bc87a..d4878eb 100644
--- a/drivers/mtd/maps/Kconfig
+++ b/drivers/mtd/maps/Kconfig
@@ -517,6 +517,17 @@ config MTD_PCMCIA_ANONYMOUS
 
 	  If unsure, say N.
 
+config MTD_BFIN_ASYNC
+	tristate "Blackfin BF533-STAMP Flash Chip Support"
+	depends on BFIN533_STAMP && MTD_CFI
+	select MTD_PARTITIONS
+	default y
+	help
+	  Map driver which allows for simultaneous utilization of
+	  ethernet and CFI parallel flash.
+
+	  If compiled as a module, it will be called bfin-async-flash.
+
 config MTD_UCLINUX
 	tristate "Generic uClinux RAM/ROM filesystem support"
 	depends on MTD_PARTITIONS && !MMU
diff --git a/drivers/mtd/maps/Makefile b/drivers/mtd/maps/Makefile
index 957fb5f..4ed1672 100644
--- a/drivers/mtd/maps/Makefile
+++ b/drivers/mtd/maps/Makefile
@@ -67,3 +67,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_BFIN_ASYNC)	+= bfin-async-flash.o
diff --git ...

The pointer casts are superfluous.  Linus prefers variable declarations
up front (sorry for my bad example).  The "+ i" in the last conditional
is a bit dangerous as any changes to the loop can break it.  Linux code
has lots of churn and not everyone is careful enough to spot such
subtleties.
And I believe you can improve performance by killing the put_unaligned
in the loop.  So if we put it all together the end result should be
something like this:

static void bfin_copy_from(struct map_info *map, void *to, unsigned long from, ssize_t len)
{
	size_t i;
	map_word test;
	u8 *byte;
	u16 *dst;

	if ((unsigned long)to & 0x1) {
		byte = to;
		test = bfin_read(map, from);
		*byte = test.x[0] >> 8;
		to++;
		from++;
		len--;
	}

	for (i = 0; i < len; i += 2) {
		dst = to + i;
		test = bfin_read(map, from + i);
		*dst = test.x[0];
	}

	if ((len & 0x1) {
		byte = to + len - 1;
		test = bfin_read(map, from + len - 1);
		*byte = test.x[0] & 0xff;
	}
}


You didn't need the cast above.  Why here?

Otherwise looks good to me.

Jörn

-- 
Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it.
-- Brian W. Kernighan
--


left over due to "from" being unsigned long ... forgot that "to" is a


*shrug* it's always been there.  i'll drop it.

thanks!
-mike
--


Can you elaborate a bit?

Jörn

-- 
It is better to die of hunger having lived without grief and fear,
than to live with a troubled spirit amid abundance.
-- Epictetus
--


it's still used, therefore the definition cannot be dropped.  just
look at the probe function.
-mike
--


That bit is obvious.  But why do you use pr_devinit() instead of
printk()?  What does it gain you?

Jörn

-- 
Das Aufregende am Schreiben ist es, eine Ordnung zu schaffen, wo
vorher keine existiert hat.
-- Doris Lessing
--


why do we have __init and __devinit in the first place ?
-mike
--


Ah.  Now I finally understand what this code does.

Might be worth moving it to include/linux/kernel.h, along with
pr_init().  And watch an avalanche of janitor patches use these new
toys. :)

Jörn

-- 
Joern's library part 3:
http://inst.eecs.berkeley.edu/~cs152/fa05/handouts/clark-test.pdf
--


Yes, I agree. Mike, could you consider providing a patch to add
pr_devinit to common code?
IMO, your weapon can be shared by others.

Thanks
-Bryan
--


actually, i think it'll be simpler to just scrap the whole function:
static void bfin_copy_from(struct map_info *map, void *to, unsigned
long from, ssize_t len)
{
    struct async_state *state = (struct async_state *)map->map_priv_1;

    switch_to_flash(state);

    memcpy(to, map->virt + from, len);

    switch_back(state);
}
-mike
--

Previous thread: [PATCH] doc: Test-by? by Hidetoshi Seto on Monday, May 12, 2008 - 9:36 pm. (2 messages)

Next thread: [PATCH] flag parameters: paccept w/out set_restore_sigmask by Ulrich Drepper on Monday, May 12, 2008 - 10:05 pm. (1 message)