Hi Stefan, Thanks for the review, I'll see if I have time to spin another patch.
>> > + const uint8_t stat_bit_WPP = (1 << 4); >> > carldani has preferred shorter variables in the past. we usually don't > use such constants and verbose comments (although it would help newbies > to understand the code). Yep, that was my aim. > i think anyone able to read a datasheet > would prefer these inline (if not they should be macros instead). > look at the other functions in at25.c for examples. > OK, cool. I'm happy to go with house style - tho' macros always seem like a bit of an anachronism with modern compilers (and are debugger-hostile of course). > would that buy us anything if used? > > + /* Supports spi_at26_write_sequential too, but it's untested */ > Probably not, but carldani said on IRC (at the time I wrote the original patch) that he'd like to add a mechanism to support multiple alternative write modes in the future, so this comment was with the aim of making that transition easier. >> > - .write = NULL /* Incompatible Page write */, >> > + .unlock = spi_disable_blockprotect_at26f040, >> > + /* also supports spi_chip_write_1, but nothing else */ >> > have you tested spi_chip_write_1? > Yes I have, and it worked (I implemented the write_sequential code whilst I was waiting for the spi_chip_write_1 test to complete!). Thanks for the review. ACK on all other comments! Tim.
_______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
