[flashrom] [PATCH 2/3] Flag-driven erased bit value

[Nico Huber]

On 10.10.2015 16:20, Paul Kocialkowski wrote:
> Most flash chips are erased to ones and programmed to zeros. However, some other
> flash chips, such as the ENE KB9012 internal flash, work the opposite way.
> 
> Signed-off-by: Paul Kocialkowski <contact at paulk.fr>
Looks good, some comments below.

Nico

> ---
>  flash.h    |  3 ++-
>  flashrom.c | 42 ++++++++++++++++++++++--------------------
>  2 files changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/flash.h b/flash.h
> index 24861ba..3d14d56 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -123,6 +123,7 @@ enum write_granularity {
>  #define FEATURE_WRSR_EITHER	(FEATURE_WRSR_EWSR | FEATURE_WRSR_WREN)
>  #define FEATURE_OTP		(1 << 8)
>  #define FEATURE_QPI		(1 << 9)
> +#define FEATURE_ERASED_ZERO	(1 << 10)
>  
>  enum test_state {
>  	OK = 0,
> @@ -275,7 +276,7 @@ int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f
>  int read_flash_to_file(struct flashctx *flash, const char *filename);
>  char *extract_param(const char *const *haystack, const char *needle, const char *delim);
>  int verify_range(struct flashctx *flash, const uint8_t *cmpbuf, unsigned int start, unsigned int len);
> -int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum write_granularity gran);
> +int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum write_granularity gran, uint8_t erased);
When reading `erased` I though it sounds like a boolean. Maybe call it
`erased_value`?

>  void print_version(void);
>  void print_buildinfo(void);
>  void print_banner(void);
> diff --git a/flashrom.c b/flashrom.c
> index c9c7e31..e463a18 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -669,7 +669,7 @@ static int compare_range(const uint8_t *wantbuf, const uint8_t *havebuf, unsigne
>  
>  /* start is an offset to the base address of the flash chip */
>  int check_erased_range(struct flashctx *flash, unsigned int start,
> -		       unsigned int len)
> +		       unsigned int len, uint8_t erased)
There's already `struct flashctx *` in the signature, doesn't that have
the information this time, already?

[...]

> @@ -1424,6 +1424,7 @@ static int erase_and_write_block_helper(struct flashctx *flash,
>  	unsigned int starthere = 0, lenhere = 0;
>  	int ret = 0, skip = 1, writecount = 0;
>  	enum write_granularity gran = flash->chip->gran;
> +	uint8_t erased = (flash->chip->feature_bits & FEATURE_ERASED_ZERO) ? 0x00 : 0xff;
You could make the transformation a macro or inline function, that would
save you from writing it out in full again. Also, as this variable never
changes later, it could be declared `const`. Hmmm, that also applies to
the added parameters.