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