From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marek.vasut@gmail.com>
Date: Mon, 22 Aug 2011 18:07:26 +0200
Subject: [U-Boot] [PATCH 1/3] net: Adds Fast Ethernet Controller driver
	for Armada100
In-Reply-To: <201108221202.28078.vapier@gentoo.org>
References: <n> <1313989919-3779-1-git-send-email-ajay.bhargav@einfochips.com>
	<201108221202.28078.vapier@gentoo.org>
Message-ID: <201108221807.26804.marek.vasut@gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Monday, August 22, 2011 06:02:26 PM Mike Frysinger wrote:
> On Monday, August 22, 2011 01:11:57 Ajay Bhargav wrote:
> > +	writel((u32) darmdfec->htpr, &regs->htpr);
> 
> do you really need to cast it yourself ?  seems to show up a lot in this
> file.
> 

[...]

> make the reg base a parameter to armada100_fec_initialize()
> 
> > +	darmdfec = malloc(sizeof(struct armdfec_device));
> 
> sizeof(*darmdfec)

Why are you against sizeof(struct ...) ?
> 
> > +	while (!eth_getenv_enetaddr(s, dev->enetaddr)) {
> > +		/* Generate Private MAC addr if not set */
> > +		dev->enetaddr[0] = 0x00;
> > +		dev->enetaddr[1] = 0x50;
> > +		dev->enetaddr[2] = 0x43;
> > +#if defined(CONFIG_SKIP_LOCAL_MAC_RANDOMIZATION)
> > +		/* Generate fixed lower MAC half */
> > +		dev->enetaddr[3] = 0x11;
> > +		dev->enetaddr[4] = 0x22;
> > +		dev->enetaddr[5] = 0x33;
> > +#else
> > +		/* Generate random lower MAC half */
> > +		dev->enetaddr[3] = get_random_byte((u8)read_timer());
> > +		dev->enetaddr[4] = get_random_byte(dev->enetaddr[3]);
> > +		dev->enetaddr[5] = get_random_byte(dev->enetaddr[4]);
> > +#endif
> > +		eth_setenv_enetaddr(s, dev->enetaddr);
> > +	}
> 
> NAK on this whole thing.  initialize dev->write_hwaddr and that is the only
> thing you should do.  the higher eth layers will take care of calling that
> as necessary.

ACK on the comment here.