On Tue, 02 Aug 2011 09:30:52 +0100 Tim Small tim@buttersideup.com wrote:
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).
i did not suggest macros as my favorite solution, but it would at least cut the line count down. the problem with macros and variables is that those who know the bits or have the datasheets in front of them (which should be the vast majority), would have to look at the declarations to verify that they are correct. this could be mitigated by having a strict naming policy everywhere so that the variables are named so that regular readers instantly know what they mean and that they are correct. this needs lots of discussions and compromises and there are a huge number of names in datasheets that are not consistent between vendors or models (like the sequential write mode vs. AAI you have encountered). of course having magic numbers inline in code is a risk, but i think all flashrom developers think it is the way to go most of the time.
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.
sure it is a good idea to annotate those (this should be done as uniformly as possible to ease further integration, please see the .voltage annotations added recently for examples). it would be nice to benchmark AAI vs. page write. one could of course calculate the theoretical bandwidth i.e. number of bytes transferred for different numbers of application data and alignments too. /me postpones that till we have support for multiple write functions. :)
Thanks for the review. ACK on all other comments!
np, thanks for your work!