[U-Boot] [PATCH] mgcoge, mgsuvd: added support for the IVM EEprom.

Wolfgang Denk
2008-09-29T09:10:14Z

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

--
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: wd at denx.de
Lots of folks confuse bad management with destiny. -- Frank Hubbard