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

Paul Kocialkowski contact at paulk.fr
Thu Oct 22 09:04:57 CEST 2015


Hi Nico,

Le mercredi 14 octobre 2015 à 21:59 +0200, Nico Huber a écrit :
> 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.

Thanks for the review Nico, I'll submit v2 of this patch with your
comments in mind.

> > ---
> >  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`?

Good point.

> >  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?

It does indeed, and I agree we'd better not pass extra arguments when
those can be figured out from what we already have.
Having a macro for erased_value will help.

> [...]
> 
> > @@ -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.

Agreed, I will make those changes, probably with a macro.

-- 
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis
on freedom and privacy/security.

Website: https://www.replicant.us/
Blog: https://blog.replicant.us/
Wiki/tracker/forums: https://redmine.replicant.us/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20151022/868cf199/attachment.asc>


More information about the flashrom mailing list