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
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
