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

Paul Kocialkowski contact at paulk.fr
Thu Nov 5 12:59:08 CET 2015


Le dimanche 01 novembre 2015 à 16:11 +0100, Nico Huber a écrit :
> On 24.10.2015 13:24, 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>
> Two comments below.

Thanks for the review Nico!

> > ---
> >  flash.h    |  5 ++++-
> >  flashrom.c | 39 +++++++++++++++++++++------------------
> >  2 files changed, 25 insertions(+), 19 deletions(-)
> > 
> > diff --git a/flash.h b/flash.h
> > index 24861ba..55d02de 100644
> > --- a/flash.h
> > +++ b/flash.h
> > @@ -123,6 +123,9 @@ 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)
> > +
> > +#define ERASED_VALUE(flash)	((flash->chip->feature_bits & FEATURE_ERASED_ZERO) ? 0x00 : 0xff)
> For macros, it's a good habit to put references to a pointer parameter
> in parantheses. This prevents problems when the actual argument is not a
> plain identifier but an expression with lower precedence operators. So
> something like
>   ERASED_VALUE(*pflash)
> wouldn't work with the definition above, but with
>   #define ERASED_VALUE(flash)  (((flash)-> ...
> it would.

Good point!

> >  
> >  enum test_state {
> >  	OK = 0,
> > @@ -275,7 +278,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, const uint8_t erased_value);
> Minor: Actually, the const in `const uint8_t erased_value` isn't part of
> the contract and can be left out here. It just doesn't matter to the
> caller.

Well, it doesn't matter to have the const in the prototype, but it is
meaningful in the function definition, and I'd rather have the prototype
match the actual function definition, for consistency and easiness (to
keep the habit of simply using copy-paste for prototypes).
-------------- 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/20151105/273f397a/attachment.asc>


More information about the flashrom mailing list