[flashrom] [PATCH/RFC] finish jedec converstion

[Carl-Daniel Hailfinger]

On 23.01.2010 08:46, Sean Nelson wrote:
> Complete the addition of Feature Bits for all Jedec based chips.
> Add Zero entries for FWH, SPI and "Odd Jedec" (FEATURE_ADDR_555) chips.
> Add FEATURE_SHORT_RESET
> fix feature_bits to 0 for SST49LF040B
>
> rewrite jedec functions to use getaddrmask
>
> convert write_49f002 to write_jedec_1
> convert write_w39v040c to write_jedec_1
> convert probe_w39v040c to probe_jedec
> convert write_49lf040 to write_jedec_1
> convert write_pm29f002 to write_jedec
> convert write_29f040b to write_jedec_1
> convert probe_29f040b to probe_jedec
> convert erase_chip_29f040b to erase_chip_block_jedec
> convert erase_sector_29f040b to erase_sector_jedec
> convert write_m29f002b to write_jedec
> convert write_m29f002t to write_jedec
> convert *_29f002 to *_jedec
>
> decouple unused files from Makefile:
> am29f040b.c
> en29f002a.c
> m29f002.c
> mx29f002.c
> pm29f002.c
> sst49lf040.c
> w39v040c.c
> w49f002u.c
>
> Signed-off-by: Sean Nelson <audiohacked at gmail.com>

In general, I like it. Some comments, though.


>  #define FEATURE_REGISTERMAP	(1 << 0)
>  #define FEATURE_BYTEWRITES	(1 << 1)
> +#define FEATURE_SHORT_RESET	(1 << 4)
>   

Can you please introduce FEATURE_EITHER_RESET as well for chips which
support short and long reset? Feel free to define it either to (1<<4) or
(0<<4), the point is just to keep this information around in case we
ever switch to probe-once.


>  #define FEATURE_ADDR_FULL	(0 << 2)
>  #define FEATURE_ADDR_MASK	(3 << 2)
> +#define FEATURE_ADDR_2AA	(1 << 2)
> +#define FEATURE_ADDR_AAA	(2 << 2)
> +#define FEATURE_ADDR_555	0
>   

FEATURE_ADDR_555 should be FEATURE_ADDR_SHIFTED. Coincidentally, once
you evaluate this bit, you can use the _common functions with a
mask+shift parameter you posted earlier and which were rejected as too
complicated for a first step. Now we're doing the second step and for
that mask+shift is entirely appropriate. FEATURE_ADDR_SHIFTED would set
shift=1, and all others would set shift=0.


> +#define FEATURE_ADDR_FWH	0
> +#define FEATURE_ADDR_SPI	0
>   

I don't really get the point of those.

Regards,
Carl-Daniel

-- 
Developer quote of the year:
"We are juggling too many chainsaws and flaming arrows and tigers."