Dear Heiko Schocher,
In message <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
> ---
> 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
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [EMAIL PROTECTED]
Lots of folks confuse bad management with destiny. -- Frank Hubbard
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
