On 10.07.2010 20:16, Michael Karcher wrote:
Am Samstag, den 22.05.2010, 03:26 +0200 schrieb Carl-Daniel Hailfinger:
[as requested on IRC, this is not a full review, but two things not directly related to the patch stand out I don't want to leave uncommented]
int it8716f_spi_chip_write_256(struct flashchip *flash, uint8_t *buf) {
int total_size = 1024 * flash->total_size; int i;
/*
- IT8716F only allows maximum of 512 kb SPI chip size for memory
- mapped access.
*/
if ((programmer == PROGRAMMER_IT87SPI) || (total_size > 512 * 1024)) {
- if ((programmer == PROGRAMMER_IT87SPI) || (flash->total_size * 1024 > 512 * 1024)) {
why do you have to test for the programmer type here? It seems like it8716f* functions are only ever called if programmer is PROGRAMMER_IT87SPI.
Historical baggage, and a corner case. PROGRAMMER_IT87SPI means someone used "-p it87_spi" instead of "-p internal". That's for machines which used it87_spi before we had autodetection (and used board enabled instead), and it also is for machines where the IT87* Super I/O does not perform flash translation, but where someone hooked up a flash chip anyway and wants to use IT87 in pure command mode and avoid memmapped mode. To be honest, I think we can kill that special case. I don't know of anyone who actually uses it nowadays.
+/* real chunksize is 1, logical chunksize is 64k */ int write_coreboot_m29f400bt(struct flashchip *flash, uint8_t *buf) { chipaddr bios = flash->virtual_memory;
The M29F400 stuff is completely broken. We use write_coreboot_m29f400bt everywhere and write_m29f400bt is dead code. But write_coreboot_m29f400bt does just write the lower half of the chip (below the dashed line in the diagram).
I think I'll just kill that function.
Regards, Carl-Daniel