Hi Peter,
On 17.06.2010 16:41, Peter Lemenkov wrote:
Signed-off-by: Peter Lemenkov lemenkov@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