On 06/15/2016 03:41 PM, marcin.krzemin...@nokia.com wrote: > From: Marcin Krzeminski <marcin.krzemin...@nokia.com> > > Instead of always reading and comparing jededc ID, > replace it by function. > > Signed-off-by: Marcin Krzeminski <marcin.krzemin...@nokia.com>
Looks good to me. Some minor comments below. Thanks, C. > --- > hw/block/m25p80.c | 49 ++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 40 insertions(+), 9 deletions(-) > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c > index 4c856f5..15765f5 100644 > --- a/hw/block/m25p80.c > +++ b/hw/block/m25p80.c > @@ -307,6 +307,14 @@ typedef enum { > STATE_READING_DATA, > } CMDState; > > +typedef enum { > + MAN_SPANSION, > + MAN_MACRONIX, > + MAN_NUMONYX, > + MAN_WINBOND, > + MAN_GENERIC, > +} Manufacturer; > + > typedef struct Flash { > SSISlave parent_obj; > > @@ -350,6 +358,22 @@ typedef struct M25P80Class { > #define M25P80_GET_CLASS(obj) \ > OBJECT_GET_CLASS(M25P80Class, (obj), TYPE_M25P80) > > +static inline Manufacturer get_man(Flash *s) > +{ > + switch (((s->pi->jedec >> 16) & 0xFF)) { > + case 0x20: > + return MAN_NUMONYX; > + case 0xEF: > + return MAN_WINBOND; > + case 0x01: > + return MAN_SPANSION; > + case 0xC2: > + return MAN_MACRONIX; > + default: > + return MAN_GENERIC; > + } > +} > + > static void blk_sync_complete(void *opaque, int ret) > { > /* do nothing. Masters do not directly interact with the backing store, > @@ -562,7 +586,8 @@ static void reset_memory(Flash *s) > s->write_enable = false; > s->reset_enable = false; > > - if (((s->pi->jedec >> 16) & 0xFF) == JEDEC_NUMONYX) { We could have kept the if () > + switch (get_man(s)) { > + case MAN_NUMONYX: > s->volatile_cfg = 0; > s->volatile_cfg |= VCFG_DUMMY; > s->volatile_cfg |= VCFG_WRAP_SEQUENTIAL; > @@ -594,6 +619,9 @@ static void reset_memory(Flash *s) > if (!(s->nonvolatile_cfg & NVCFG_LOWER_SEGMENT_MASK)) { > s->ear = CFG_UPPER_128MB_SEG_ENABLED; > } > + break; > + default: > + break; > } > > DB_PRINT_L(0, "Reset done.\n"); > @@ -634,9 +662,12 @@ static void decode_new_cmd(Flash *s, uint32_t value) > case QOR: > case QOR4: > s->needed_bytes = get_addr_length(s); > - if (((s->pi->jedec >> 16) & 0xFF) == JEDEC_NUMONYX) { > - /* Dummy cycles modeled with bytes writes instead of bits */ why remove the comment ? > + switch (get_man(s)) { > + case MAN_NUMONYX: > s->needed_bytes += extract32(s->volatile_cfg, 4, 4); > + break; > + default: > + break; > } > s->pos = 0; > s->len = 0; > @@ -645,9 +676,9 @@ static void decode_new_cmd(Flash *s, uint32_t value) > > case DIOR: > case DIOR4: > - switch ((s->pi->jedec >> 16) & 0xFF) { > - case JEDEC_WINBOND: > - case JEDEC_SPANSION: > + switch (get_man(s)) { > + case MAN_WINBOND: > + case MAN_SPANSION: > s->needed_bytes = 4; > break; > default: > @@ -662,9 +693,9 @@ static void decode_new_cmd(Flash *s, uint32_t value) > > case QIOR: > case QIOR4: > - switch ((s->pi->jedec >> 16) & 0xFF) { > - case JEDEC_WINBOND: > - case JEDEC_SPANSION: > + switch (get_man(s)) { > + case MAN_WINBOND: > + case MAN_SPANSION: > s->needed_bytes = 6; > break; > default: >