Am Samstag, den 10.07.2010, 19:41 +0200 schrieb Carl-Daniel Hailfinger:
On 10.07.2010 19:21, Michael Karcher wrote:
I think "spi_chip_write_1_range" makes a better name than "spi_chip_write_1_new". But if your plan is to remove the old spi_chip_write_1 function and rename this function at the same time to "spi_chip_write_1", I don't really mind the temporary name.
Yes, the plan is to kill the wrapper ASAP once the other chips are converted as well.
OK, sounds sensible.
=================================================================== --- flashrom-partial_write_spi_intermediate/flashchips.c (Revision 1072) +++ flashrom-partial_write_spi_intermediate/flashchips.c (Arbeitskopie) @@ -1392,7 +1392,7 @@ .block_erase = spi_block_erase_c7, } },
.write = spi_aai_write,
.write = spi_chip_write_1,You do this change to accommodate for the changed signature of spi_aai_write, I think. But you lose the information that this chip used AAI writes this way. Would adding a comment like "/* AAI capable */" be sensible?
AFAIK all write_1 chips can do AAI. Right now AAI is broken and I'm waiting for an ack for patch 1548. Switching to write_1 instead of AAI allows me to avoid creating a wrapper for AAI.
What is missing for 1548? Looks like just the formal ack, as the patch seems to already be tested by den_m. In that case, I would do a review of that patch.
-int spi_chip_write_256(struct flashchip *flash, uint8_t *buf) +/* real chunksize is up to 256, logical chunksize is 256 */ +int spi_chip_write_256_new(struct flashchip *flash, uint8_t *buf, int start, int len) { if (!spi_programmer[spi_controller].write_256) { msg_perr("%s called, but SPI page write is unsupported on this "
Oops! The comment says that single-byte programming is used on non-256-capable masters, but the code does not handle the fallback to single bytes. I also am unable to find any other place in flashrom that checks for the block write capability before calling this function. Is that a bug?
Mh. That's very non-obvious. The spi_programmer array has a .write_256 member for every controller. In case the controller can't do anything except 1-byte writes, we set .write_256 = spi_chip_write_1_new
Not every controller. SPI_CONTROLLER_DUMMY really has NULL in this field. Your patch does not change that. And please kill the test if we are sure that write_256 is always non-null.
This reminds me of patch 1371. I'm all for getting that patch in before this patch and not move around the erase-on-write logic just to kill it. I will send a review soon.
This patch is basically a part of patch 1371 with less bugs, more consistency and working backwards compatibility to make merging and reviewing easier.
I would consider it just the other way around. This patch is about partial write, and includes parts of the semantically unrelated patch about having erase outside of the write functions. Of course, one can also add the write area infrastructure before removing the erase code from the write code, but it seemed to make sense to me to first yank out erase and ad-hoc partial write from the chip driver's write functions, then convert them to a partial-write interface and finally put everything below the generalized erase-block walking.
I don't like the introduction of two new wrappers either, but those are a lot less of a problem than the existing per-controller wrappers and removing them later will be a lot less invasive than 1371.
If your plan is to base a new 1371 on top of this patch, I'm perfectly fine with these wrappers.
As soon as we have explicit erase, this check is too late in the write function. Don't we even have the same problem even at read time?
Yes, this should be handled with max_rom_decode instead. I will send a followup patch for max_rom_decode.spi in wbsio and kill the wbsio-specific read+write.
Great!
Regards, Michael Karcher