[U-Boot] [PATCH 3/7] added Mercury EP2500 board support It uses the mcf5282 processor with real time clock and EEPROM.

[Wolfgang Denk]

Dear "David Wu",

In message <op.va4iz1obqigx4y at cyprus.local> you wrote:
> 
> > Your Subject (= commit message tile) is WAY too long.
> 
> Let me know the size of subject line.

Quoting the "SubmittingPatches" document:

	The "summary phrase" in the email's Subject should concisely
	describe the patch which that email contains.  The "summary
	phrase" should not be a filename.  Do not use the same "summary
	phrase" for every patch in a whole patch series (where a "patch
	series" is an ordered sequence of multiple, related patches).

	Bear in mind that the "summary phrase" of your email becomes a
	globally-unique identifier for that patch.  It propagates all the way 
	into the git changelog.  The "summary phrase" may later be used in
	developer discussions which refer to the patch.  People will want to
	google for the "summary phrase" to read discussion regarding that
	patch.  It will also be the only thing that people may quickly see 
	when, two or three months later, they are going through perhaps
	thousands of patches using tools such as "gitk" or "git log 
	--oneline".

	For these reasons, the "summary" must be no more than 70-75
	characters, and it must describe both what the patch changes, as well
	as why the patch might be necessary.  It is challenging to be both
	succinct and descriptive, but that is what a well-written summary
	should do. 



> >> +	/*
> >> +	 * When buff returns all 0xFF the EEPROM has not
> >> +	 * been programed with a valid MAC. In this case
> >> +	 * we set enet to all 0x00 as 0xFF is not valid
> >> +	 * for this usage model.
> >> +	 */
> >> +	if (buff[0] == 0xff && buff[1] == 0xff && buff[2] == 0xff &&
> >> +	    buff[3] == 0xff && buff[4] == 0xff && buff[5] == 0xff) {
> >
> > We have standard functions for such checks. Please use them.
> It would be nice if you can let me know the function. I am grep'ing in
> u-boot and it's so hard to find it.

-> grep valid include/net.h
#define VLAN_IDMASK     0x0fff                  /* mask of valid vlan id        */
 * is_valid_ether_addr - Determine if the given Ethernet address is
 * valid
 * Return true if the address is valid.
static inline int is_valid_ether_addr(const u8 * addr)
------------------^^^^^^^^^^^^^^^^^^^


> >> +	if (!eth_getenv_enetaddr("ethaddr", enetaddr)) {
> >> +		board_get_enetaddr(enetaddr);
> >> +		eth_setenv_enetaddr("ethaddr", enetaddr);
> >
> > Please see recent discussion about this topic.
> Could you let me know the thread -- I am new here.

Search for "add write_hwaddr support" (respective commits have just
been pulled into the master branch).

> >> +void i2c_init_board(void)
> >> +{
> >> +	struct fsl_i2c *dev;
> >> +
> >> +	dev = (struct fsl_i2c *)(CONFIG_SYS_IMMR + CONFIG_SYS_I2C_OFFSET);
> >> +
> >> +	if (readb(&dev->sr) & I2C_SR_MBB) {
> >> +		writeb(0, &dev->cr);
> >> +		writeb(0xa0, &dev->cr);
> >> +		readb(&dev->dr);
> >> +		writeb(0x0, &dev->sr);
> >> +		writeb(0, &dev->cr);
> >> +		writeb(I2C_CR_MEN, &dev->cr);
> >
> > Please add comments what you are doing. Replace 0x0A by a symbolic
> You mean comments for "writeb(0xa0, &dev->cr)" -- OK.

Yes, plus replacing 0x0A by a symbolic constant.

> > name. Use a consistent style (i. e. wither use "0x0" or "0", but
> > don't mix both styles.
> >> +#ifdef CONFIG_MCFFEC
> >> +#	define CONFIG_ETHADDR	00:00:00:00:00:00
> >> +#	define CONFIG_IPADDR	192.168.1.2
> >> +#	define CONFIG_NETMASK	255.255.255.0
> >> +#	define CONFIG_SERVERIP	192.168.1.1
> >> +#	define CONFIG_GATEWAYIP	192.168.1.1
> >> +#	define CONFIG_OVERWRITE_ETHADDR_ONCE
> >> +/*#define CONFIG_ENV_OVERWRITE */
> >
> > Please remove this, it will not be accepted.
> Are you talking about the last line. Sure I can remove it.
> Otherwise would you mind tell me the reason? format is wrong, some defines
> are wrong or ifdef (or use if defined instead)?

I mean the whole block. We don't accept any static network
initalizations. They may work for you, they don't work for others.

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
Imitation is the sincerest form of plagarism.