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 <cont...@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.
_______________________________________________
flashrom mailing list
flashrom@flashrom.org
http://www.flashrom.org/mailman/listinfo/flashrom
