Dear Heiko Schocher,
In message 48E08A41.1090708@denx.de you wrote:
The EEprom contains some Manufacturerinformation, which are read from u-boot at boot time, and saved in same Environmentvars.
Signed-off-by: Heiko Schocher hs@denx.de
board/keymile/common/common.c | 285 +++++++++++++++++++++++++++++++++++++++++ include/configs/mgcoge.h | 5 + include/configs/mgsuvd.h | 5 + lib_ppc/board.c | 6 + 4 files changed, 301 insertions(+), 0 deletions(-)
diff --git a/board/keymile/common/common.c b/board/keymile/common/common.c index a6ed379..25c5900 100644 --- a/board/keymile/common/common.c +++ b/board/keymile/common/common.c @@ -38,6 +38,291 @@ extern int i2c_mgsuvd_read (void); #endif
+int ivm_calc_crc (unsigned char *buf, int len) +{
- const unsigned short cCrc16Table[16] = {
0x0000, 0xCC01, 0xD801, 0x1400,0xF001, 0x3C00, 0x2800, 0xE401,0xA001, 0x6C00, 0x7800, 0xB401,0x5000, 0x9C01, 0x8801, 0x4400};
If we need support for CRC16, it should be factored out and move to lib_generic.
+#define BTChar unsigned char
Please get rid of this #define.
+static char toAscii(char c) +{
- return (c<' ' || c>'~') ? '.' : c;
+}
Mixed case identifiers like "toAscii" are deprecated.
Also, the name is misleading as it can be easily confised with the standard function toascii() which implements a different function.
+{
- int xcode = 0;
- BTChar cr = '\r';
- /* Semikolon char */
- BTChar sc = ';';
Come on. Do we really need variables for these? And do you think that "sc" is easier to read or understand than ';'?
Please drop these.
- /* Number of CR found */
- unsigned long crFound = 0;
- /* Current address */
- unsigned long address = INVENTORYDATAADDRESS;
- /* String length */
- unsigned long strSize = 0;
- /* Number of CR to skip */
- unsigned long nbrOfCR = aType;
- /* Semicolon to end */
- int endWithSemikolon = 0;
This is difficult to read. Please put the comments in the same line as the variable declarations, and get rid of these mixed case identifiers.
- switch (aType)
- {
Incorrect brace style. See also rest of file.
/* Copy the IVM string in the corresponding string */for (; (buf[address] != cr) && /* not cr found */((buf[address] != sc) || (!endWithSemikolon)) && /* not semikolon found */
Maximum line length exceeded.
- /* IVM_MacAddress */
- sprintf ((char *)valbuf, "%02X:%02X:%02X:%02X:%02X:%02X",
buf[1],buf[2],buf[3],buf[4],buf[5],buf[6]);- setenv ("IVM_MacAddress", (char *)valbuf);
Do we really need this? We already have "ethaddr" ?
- if (getenv ("ethaddr") == NULL)
setenv ((char *)"ethaddr", (char *)valbuf);
Oops? You are even aware of this.
Please avoid duplicate data.
+int ivm_analyse_eeprom (unsigned char *buf, int len)
...analyze...
- unsigned short val;
- unsigned char valbuf[CFG_IVM_EEPROM_PAGE_LEN];
- unsigned char *tmp;
- /* used Code from ivm/src/devsrv_baseproductdata.cpp */
What is ivm/src/devsrv_baseproductdata.cpp ?
- ivm_get_value (buf, CFG_IVM_EEPROM_PAGE_LEN, "IVM_BoardId", 0, 1);
- val = ivm_get_value (buf, CFG_IVM_EEPROM_PAGE_LEN, "IVM_HWKey", 6, 1);
- if (val != 0xffff) {
sprintf ((char *)valbuf, "%x", ((val /100) % 10));setenv ("IVM_HWVariant", (char *)valbuf);sprintf ((char *)valbuf, "%x", (val % 100));setenv ("IVM_HWVersion", (char *)valbuf);- }
You're really polluting the environment here.
Is this reasonable?
Best regards,
Wolfgang Denk