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

[Wolfgang Denk]

Dear Heiko Schocher,

In message <48E08A41.1090708 at 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 at 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