[flashrom] [PATCH 1/1] Sort flashchips defines alphabetically

[Carl-Daniel Hailfinger]

Hi Peter,

On 17.06.2010 16:41, Peter Lemenkov wrote:
> Signed-off-by: Peter Lemenkov <lemenkov at gmail.com>
>   

Thanks for your patch.
Review follows.


> --- a/flashchips.h
> +++ b/flashchips.h
> @@ -253,17 +253,17 @@
>  #define IM_29F004T		0xAF
>  
>  #define INTEL_ID		0x89	/* Intel */
> -#define I_82802AB		0xAD
> -#define I_82802AC		0xAC
>  #define E_28F004S5		0xA7
>  #define E_28F008S5		0xA6
>  #define E_28F016S5		0xAA
> -#define P28F001BXT		0x94	/* 28F001BX-T */
> +#define I_82802AB		0xAD
> +#define I_82802AC		0xAC
>  #define P28F001BXB		0x95	/* 28F001BX-B */
> -#define P28F004BT		0x78	/* 28F004BV/BE-T */
> +#define P28F001BXT		0x94	/* 28F001BX-T */
>  #define P28F004BB		0x79	/* 28F004BV/BE-B */
> -#define P28F400BT		0x70	/* 28F400BV/CV/CE-T */
> +#define P28F004BT		0x78	/* 28F004BV/BE-T */
>  #define P28F400BB		0x71	/* 28F400BV/CV/CE-B */
> +#define P28F400BT		0x70	/* 28F400BV/CV/CE-T */
>  #define SHARP_LH28F008SA	0xA2	/* Sharp chip, Intel Vendor ID */
>  #define SHARP_LH28F008SC	0xA6	/* Sharp chip, Intel Vendor ID */
>  
>   

The Intel IDs have inconsistent naming even if you ignore the Sharp IDs.
Why are some called I_foo and others E_foo and others Pfoo? That makes
no sense at all. This is not your fault, but since you're touching these
definitions anyway, could you convert them to a common INTEL_ prefix?

The rest of the patch makes sense, but there are a few problems I'm not
sure how to fix:

Quite a few vendors have multiple chips with the same ID. Right now we
simply have only one #define for such an ID, and use it for all chips
with that ID. A comment on the right side of that ID says something like
"Same as ...". The problem is that the flash ID list grew organically,
so sometimes the IDs for one chip family have names picked randomly from
the different subfamilies.

Sorting. You propose alphabetical sorting in this example:
>  #define EN_25F05		0x3110
>  #define EN_25F10		0x3111
> +#define EN_25F16		0x3115
>  #define EN_25F20		0x3112
> +#define EN_25F32		0x3116
>  #define EN_25F40		0x3113
>  #define EN_25F80		0x3114
> -#define EN_25F16		0x3115
> -#define EN_25F32		0x3116
>   

Right now the IDs are ordered by chip size which has a few advantages:
You can spot holes in the numeric IDs easily (10,11,12,13,14,15,16), and
you can spot holes in the symbolic names easily (05,10,20,40,80,16,32).
An alphabetical ordering gives us a different numeric ID order
(10,11,15,12,16,13,14) and a different symbolic name order
(05,10,16,20,32,40,80). While those orders follow alphanumeric sort
rules, we should also check if they are convenient for humans. Please
note that the ordering difference gets more complicated once we add the
EN_25F64 and EN_25F128.

Comments?

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/