[PATCH] Make nand block functions use provided byte/word helpers.

[PATCH] Make nand block functions use provided byte/word helpers.

[Jonathan McDowell]

Hi.

I've been writing a NAND driver for the flash on the Amstrad E3. One of
the peculiarities of this device is that the write & read enable lines
are on a latch, rather than strobed by the act of reading/writing from
the data latch. As such I've got custom read_byte/write_byte functions
defined. However the nand_*_buf functions in drivers/mtd/nand/nand_base.c
are all appropriate, except for the fact they call readb/writeb
themselves, instead of using this->read_byte or this->write_byte. The
patch below changes them to use these functions, meaning a driver just
needs to define read_byte and write_byte functions and gains all the
nand_*_buf functions free.

Signed-off-by: Jonathan McDowell <noodles@earth.li>


----------
--- linux-2.6.15/drivers/mtd/nand/nand_base.c.orig	2006-02-28 20:41:54.000000000 +0000
+++ linux-2.6.15/drivers/mtd/nand/nand_base.c	2006-02-28 20:46:44.000000000 +0000
@@ -302,7 +302,7 @@ static void nand_write_buf(struct mtd_in
 	struct nand_chip *this = mtd->priv;
 
 	for (i=0; i<len; i++)
-		writeb(buf[i], this->IO_ADDR_W);
+		this->write_byte(mtd, buf[i]);
 }
 
 /**
@@ -319,7 +319,7 @@ static void nand_read_buf(struct mtd_inf
 	struct nand_chip *this = mtd->priv;
 
 	for (i=0; i<len; i++)
-		buf[i] = readb(this->IO_ADDR_R);
+		buf[i] = this->read_byte(mtd);
 }
 
 /**
@@ -336,7 +336,7 @@ static int nand_verify_buf(struct mtd_in
 	struct nand_chip *this = mtd->priv;
 
 	for (i=0; i<len; i++)
-		if (buf[i] != readb(this->IO_ADDR_R))
+		if (buf[i] != this->read_byte(mtd))
 			return -EFAULT;
 
 	return 0;
@@ -358,7 +358,7 @@ static void nand_write_buf16(struct mtd_
 	len >>= 1;
 
 	for (i=0; i<len; i++)
-		writew(p[i], this->IO_ADDR_W);
+		this->write_word(mtd, p[i]);
 
 }
 
@@ -378,7 +378,7 @@ static void nand_read_buf16(struct mtd_i
 	len >>= 1;
 
 	for (i=0; i<len; i++)
-		p[i] = readw(this->IO_ADDR_R);
+		p[i] = this->read_word(mtd);
 }
 
 /**
@@ -397,7 +397,7 @@ static int nand_verify_buf16(struct mtd_
 	len >>= 1;
 
 	for (i=0; i<len; i++)
-		if (p[i] != readw(this->IO_ADDR_R))
+		if (p[i] != this->read_word(mtd))
 			return -EFAULT;
 
 	return 0;
----------

J.

-- 
 [   There are always at least two ways to program the same thing.    ]
 [ http://www.blackcatnetworks.co.uk/ - IPv6 enabled ADSL/dialup/colo ]

Re: [PATCH] Make nand block functions use provided byte/word helpers.

[Ben Dooks]

On Tue, Feb 28, 2006 at 08:59:03PM +0000, Jonathan McDowell wrote:
> Hi.
> 
> I've been writing a NAND driver for the flash on the Amstrad E3. One of
> the peculiarities of this device is that the write & read enable lines
> are on a latch, rather than strobed by the act of reading/writing from
> the data latch. As such I've got custom read_byte/write_byte functions
> defined. However the nand_*_buf functions in drivers/mtd/nand/nand_base.c
> are all appropriate, except for the fact they call readb/writeb
> themselves, instead of using this->read_byte or this->write_byte. The
> patch below changes them to use these functions, meaning a driver just
> needs to define read_byte and write_byte functions and gains all the
> nand_*_buf functions free.

Why not make life easier on everyone else by over-riding the
functions for read/write buffer (etc) in the nand driver... less
intrusive into the core code!

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [PATCH] Make nand block functions use provided byte/word helpers.

[Jonathan McDowell]

On Tue, Feb 28, 2006 at 10:12:43PM +0000, Ben Dooks wrote:
> On Tue, Feb 28, 2006 at 08:59:03PM +0000, Jonathan McDowell wrote:
> > I've been writing a NAND driver for the flash on the Amstrad E3. One of
> > the peculiarities of this device is that the write & read enable lines
> > are on a latch, rather than strobed by the act of reading/writing from
> > the data latch. As such I've got custom read_byte/write_byte functions
> > defined. However the nand_*_buf functions in drivers/mtd/nand/nand_base.c
> > are all appropriate, except for the fact they call readb/writeb
> > themselves, instead of using this->read_byte or this->write_byte. The
> > patch below changes them to use these functions, meaning a driver just
> > needs to define read_byte and write_byte functions and gains all the
> > nand_*_buf functions free.
> 
> Why not make life easier on everyone else by over-riding the
> functions for read/write buffer (etc) in the nand driver... less
> intrusive into the core code!

If the patch is deemed too intrusive then that's what I'll do; I nearly
did so originally but when I caught myself cut and pasting the code from
nand_base I thought my patch was the cleaner way.

The patch shouldn't cause any extra work for anyone that I can see and I
don't think it's intrusive at all; I submitted it because I figured it
might save someone else some work down the line as well.

J.

-- 
noodles is fat
This .sig brought to you by the letter M and the number  5
Product of the Republic of HuggieTag

Re: [PATCH] Make nand block functions use provided byte/word helpers.

[Ben Dooks]

On Tue, Feb 28, 2006 at 10:25:59PM +0000, Jonathan McDowell wrote:
> On Tue, Feb 28, 2006 at 10:12:43PM +0000, Ben Dooks wrote:
> > On Tue, Feb 28, 2006 at 08:59:03PM +0000, Jonathan McDowell wrote:
> > > I've been writing a NAND driver for the flash on the Amstrad E3. One of
> > > the peculiarities of this device is that the write & read enable lines
> > > are on a latch, rather than strobed by the act of reading/writing from
> > > the data latch. As such I've got custom read_byte/write_byte functions
> > > defined. However the nand_*_buf functions in drivers/mtd/nand/nand_base.c
> > > are all appropriate, except for the fact they call readb/writeb
> > > themselves, instead of using this->read_byte or this->write_byte. The
> > > patch below changes them to use these functions, meaning a driver just
> > > needs to define read_byte and write_byte functions and gains all the
> > > nand_*_buf functions free.
> > 
> > Why not make life easier on everyone else by over-riding the
> > functions for read/write buffer (etc) in the nand driver... less
> > intrusive into the core code!
> 
> If the patch is deemed too intrusive then that's what I'll do; I nearly
> did so originally but when I caught myself cut and pasting the code from
> nand_base I thought my patch was the cleaner way.
> 
> The patch shouldn't cause any extra work for anyone that I can see and I
> don't think it's intrusive at all; I submitted it because I figured it
> might save someone else some work down the line as well.

Well, a small iritation is that it adds work for the read and 
write byte function, as a function call adds instructions to
the path. Also, do you want to check that it dosen't break
any existing drivers?

Secondly, you'll probably have a better piece of code for the
E3 driver if you did the block calls with only one setup for
the r/w enable lines per block, instead of checking them for
every byte done. You may even want to use readsb/writesb or
DMA to accelerate the operation.

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [PATCH] Make nand block functions use provided byte/word helpers.

[Jonathan McDowell]

On Wed, Mar 01, 2006 at 11:50:20AM +0000, Ben Dooks wrote:
> On Tue, Feb 28, 2006 at 10:25:59PM +0000, Jonathan McDowell wrote:
> > On Tue, Feb 28, 2006 at 10:12:43PM +0000, Ben Dooks wrote:
> > > On Tue, Feb 28, 2006 at 08:59:03PM +0000, Jonathan McDowell wrote:
> > > > I've been writing a NAND driver for the flash on the Amstrad E3.
> > > > One of the peculiarities of this device is that the write & read
> > > > enable lines are on a latch, rather than strobed by the act of
> > > > reading/writing from the data latch. As such I've got custom
> > > > read_byte/write_byte functions defined. However the nand_*_buf
> > > > functions in drivers/mtd/nand/nand_base.c are all appropriate,
> > > > except for the fact they call readb/writeb themselves, instead
> > > > of using this->read_byte or this->write_byte. The patch below
> > > > changes them to use these functions, meaning a driver just needs
> > > > to define read_byte and write_byte functions and gains all the
> > > > nand_*_buf functions free.
> > > 
> > > Why not make life easier on everyone else by over-riding the
> > > functions for read/write buffer (etc) in the nand driver... less
> > > intrusive into the core code!
> > 
> > If the patch is deemed too intrusive then that's what I'll do; I
> > nearly did so originally but when I caught myself cut and pasting
> > the code from nand_base I thought my patch was the cleaner way.
> > 
> > The patch shouldn't cause any extra work for anyone that I can see
> > and I don't think it's intrusive at all; I submitted it because I
> > figured it might save someone else some work down the line as well.
> 
> Well, a small iritation is that it adds work for the read and write
> byte function, as a function call adds instructions to the path.

Well, the _buf functions rather than the read/write_byte functions, but
yes, there will be extra instructions in the path.

> Also, do you want to check that it dosen't break any existing drivers?

Hard to do without the hardware, but I did check out the existing
drivers (from mtd-snapshot-20060222 mtd/drivers/mtd/nand/):

at91_nand.c
	Doesn't override any of the _buf functions, will end up being
	hit by the extra instructions in the path.
au1550nd.c
	Overrides the _buf functions, unaffected.
autcpu12.c
	Doesn't override any of the _buf functions, will end up being
	hit by the extra instructions in the path.
diskonchip.c
	Overrides the _buf functions, unaffected.
edb7312.c
	Doesn't override any of the _buf functions, will end up being
	hit by the extra instructions in the path.
h1910.c
	Doesn't override any of the _buf functions, will end up being
	hit by the extra instructions in the path.
rtc_from4.c
	Doesn't override any of the _buf functions, will end up being
	hit by the extra instructions in the path.
s3c2410.c
	Overrides the _buf functions, unaffected.
sharpsl.c
	Doesn't override any of the _buf functions, will end up being
	hit by the extra instructions in the path.
spia.c
	Doesn't override any of the _buf functions, will end up being
	hit by the extra instructions in the path.
toto.c
	Doesn't override any of the _buf functions, will end up being
	hit by the extra instructions in the path.
tx4925ndfmc.c
	Overrides the _buf functions.
tx4938ndfmc.c
	Overrides the _buf functions.

So 8 drivers that will have the extra instructions in the call path, 5
that already provide their own. Nothing overrides the
read_byte/write_byte functions that doesn't also override the _buf
functions that I can see, so there should be no change in behaviour.

> Secondly, you'll probably have a better piece of code for the E3
> driver if you did the block calls with only one setup for the r/w
> enable lines per block, instead of checking them for every byte done.
> You may even want to use readsb/writesb or DMA to accelerate the
> operation.

It's not a case of checking the lines; the problem is that all of NCE,
NWE and NRE are hooked up to a latch, rather than processor control
lines. My understanding is that NCE or NWE/NRE need pulled high between
writes/reads and the only way to do this is output to the control latch.
As such I don't think readsb/writesb or DMA are able to help me out?

J.

-- 
Web [            noodles is almost too good to be true             ]
site: http:// [                                          ]       Made by
www.earth.li/~noodles/  [                      ]         HuggieTag 0.0.23