[flashrom] [PATCH] Simplify chunking logic

Michael Karcher flashrom at mkarcher.dialup.fu-berlin.de
Tue Jul 13 17:22:19 CEST 2010


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





More information about the flashrom mailing list