On 11.07.2010 18:42, Michael Karcher wrote:
Read function tested for chunk size 62 and with start offset 3, works as expected.
Signed-off-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
spi25.c | 80 +++++++++++++++++++++++++------------------------------------- 1 files changed, 32 insertions(+), 48 deletions(-)
diff --git a/spi25.c b/spi25.c index 51be397..260dd1c 100644 --- a/spi25.c +++ b/spi25.c @@ -879,35 +879,27 @@ int spi_nbyte_read(int address, uint8_t *bytes, int len) int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize) { int rc = 0;
- int i, j, starthere, lenhere;
- int ofs, startofs, endofs; /* offsets relative to page begin */ int page_size = flash->page_size;
- int toread;
- /* Warning: This loop has a very unusual condition and body.
* The loop needs to go through each page with at least one affected* byte. The lowest page number is (start / page_size) since that* division rounds down. The highest page number we want is the page* where the last byte of the range lives. That last byte has the* address (start + len - 1), thus the highest page number is* (start + len - 1) / page_size. Since we want to include that last* page as well, the loop condition uses <=.*/- for (i = start / page_size; i <= (start + len - 1) / page_size; i++) {
/* Byte position of the first byte in the range in this page. *//* starthere is an offset to the base address of the chip. */starthere = max(start, i * page_size);/* Length of bytes in the range in this page. */lenhere = min(start + len, (i + 1) * page_size) - starthere;for (j = 0; j < lenhere; j += chunksize) {toread = min(chunksize, lenhere - j);rc = spi_nbyte_read(starthere + j, buf + starthere - start + j, toread);
- int pageaddr, toread;
/* startofs is an offset to current page address. */
- startofs = start % page_size;
- /* iterate over all affected pages */
- for (pageaddr = start - startofs; pageaddr < start + len;
pageaddr += page_size) {
Please use tabs here, at least for the first 8-space blocks.
endofs = min(page_size, start + len - pageaddr);
endofs is a misnomer. The correct name would be one_after_endofs, but that name is ugly. What about
/* Number of bytes to write in this page. */ lenhere = min(page_size, start + len - pageaddr) - startofs;
and using (startofs + lenhere) in place of endofs everywhere?
/* iterate over all chunks in the current page */for (ofs = startofs; ofs < endofs; ofs += chunksize) {toread = min(chunksize, endofs - ofs);rc = spi_nbyte_read(pageaddr + ofs, buf, toread);buf += toread; if (rc)
break;
goto leave_page_loop;
return rc;
}
if (rc)break;
startofs = 0; /* all further pages processed from start */+leave_page_loop:
Not needed if you return rc immediately in the error case.
return rc; }
Here is my variant of your code. Feel free to ignore it completely, I just wanted to send it for reference.
int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize) { int rc = 0; int j, lenhere; int page_size = flash->page_size; int pageaddr, toread, startofs;
/* This loop needs to go through each page with at least one affected * byte. */ startofs = start % page_size; for (pageaddr = start - startofs; pageaddr < start + len; pageaddr += page_size) { /* startofs is an offset to current page address. */ /* Number of bytes to write in this page. */ lenhere = min(page_size, start + len - pageaddr) - startofs; for (j = startofs; j < startofs + lenhere; j += chunksize) { toread = min(chunksize, startofs + lenhere - j); rc = spi_nbyte_read(pageaddr + j, buf, toread); if (rc) return rc; buf += toread; } startofs = 0; }
return rc; }
Regards, Carl-Daniel
Am Dienstag, den 13.07.2010, 16:04 +0200 schrieb Carl-Daniel Hailfinger:
- int i, j, starthere, lenhere;
- int ofs, startofs, endofs; /* offsets relative to page begin */ int page_size = flash->page_size;
- int toread;
- /* Warning: This loop has a very unusual condition and body.
* The loop needs to go through each page with at least one affected* byte. The lowest page number is (start / page_size) since that* division rounds down. The highest page number we want is the page* where the last byte of the range lives. That last byte has the* address (start + len - 1), thus the highest page number is* (start + len - 1) / page_size. Since we want to include that last* page as well, the loop condition uses <=.*/- for (i = start / page_size; i <= (start + len - 1) / page_size; i++) {
/* Byte position of the first byte in the range in this page. *//* starthere is an offset to the base address of the chip. */starthere = max(start, i * page_size);/* Length of bytes in the range in this page. */lenhere = min(start + len, (i + 1) * page_size) - starthere;for (j = 0; j < lenhere; j += chunksize) {toread = min(chunksize, lenhere - j);rc = spi_nbyte_read(starthere + j, buf + starthere - start + j, toread);
- int pageaddr, toread;
/* startofs is an offset to current page address. */
See the comment at the variable declaration (which is quite near to here if the long comment has been snipped. I don't think I have to repeat that here.
Please use tabs here, at least for the first 8-space blocks.
Right, fixed. Checked with an editor that shows spaces.
endofs = min(page_size, start + len - pageaddr);endofs is a misnomer. The correct name would be one_after_endofs, but that name is ugly. What about
/* Number of bytes to write in this page. */ lenhere = min(page_size, start + len - pageaddr) - startofs;
and using (startofs + lenhere) in place of endofs everywhere?
One could do that, but as we are not going to use the length as is, but always (lenhere + startofs) I consider subtracting startofs here just to add it on all uses of the variable overly contrieved.
I think it would be better to spend energy in finding a better name than to circumvent the use of a value that would be needed just because we can't really name it. What about "past_end_ofs", together, accompanied by introducing an underscore in start_ofs too?
int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize) { int rc = 0; int j, lenhere; int page_size = flash->page_size; int pageaddr, toread, startofs;
/* This loop needs to go through each page with at least one affected * byte. */ startofs = start % page_size; for (pageaddr = start - startofs; pageaddr < start + len; pageaddr += page_size) { /* startofs is an offset to current page address. */ /* Number of bytes to write in this page. */ lenhere = min(page_size, start + len - pageaddr) - startofs; for (j = startofs; j < startofs + lenhere; j += chunksize) {
As this double-loop construction is a bit contrieved, I really prefer having loop variables as meaningfull as possible. This is why I renamed "j" to "ofs", so that we know it contains an offset which has to be added to a the page address to produce an eeprom address.
Another consideration would be to split the chunking logic from the read/write calls, just like you just proposed for the eraseblock walker. This prevents having the chunking logic twice. But as I know that I am prone to adding too many layers of abstraction, I want to hear a second oppinion on that.
Regards, Michael Karcher
-
Carl-Daniel Hailfinger -
Michael Karcher