[flashrom] Add support for unlock and write to Atmel AT26F040

[Stefan Tauner]

On Tue, 02 Aug 2011 09:30:52 +0100
Tim Small <tim at 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!

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner