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.