On 25.03.2010 10:46, Michael Karcher wrote:
Am Donnerstag, den 25.03.2010, 03:10 +0100 schrieb Carl-Daniel Hailfinger:
- /* 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) {towrite = min(chunksize, lenhere - j);rc = spi_nbyte_program(starthere + j, buf + starthere - start + j, towrite);if (rc)break;while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)programmer_delay(10);}if (rc)break;- }
i would write this as (carful, untested):
startofs = start % pagesize; /* offset relative to page begin */ /* iterate over all pages to write */ for(pageaddr = start - startofs; pageaddr < start + len; pageaddr += page_size) { endofs = min(pagesize, start + len - pageaddr); /* iterate over chunks in this page */ for(j = startofs; j < endofs; j += chunksize) { towrite = min(chunksize, endofs - j); rc = spi_nbyte_program(pageaddr + j, buf, towrite); buf += towrite; if (rc) goto leave_page_loop; /* Copy/paste wait here */ } startofs = 0; /* all pages but the first need to be programmed from the beginning */ }leave_page_loop:
I prefer your logic over mine, but my logic has been tested by David Hendricks, and that's why I committed mine.
If you could send your code as patch for spi_read_chunked and spi_write_chunked, I'd be really happy.
Your logic seem right, too, so this is a matter of taste. I think my version needs less explanation, that's why I would prefer it that way.
Fully agreed. We want your logic.
We should aim to remove all the foo_spi_write_n functions and put the max write size into the spi controller structure. This probably means to write one entry for Intel (64) and one for VIA (16). Currently, some of the write functions do erase, others don't. Some of the write functions do unprotect, others don't. But that's for a later patch.
Will send that patch in a few minutes.
Regards, Carl-Daniel