[U-Boot] [PATCH] NET: add ENC28J60 driver using SPI framework

[Mike Frysinger]

On Wednesday, August 25, 2010 08:47:39 Reinhard Meyer wrote:
> From: Reinhard Meyer <info at emk-elektronik.de>
> 
> Signed-off-by: Reinhard Meyer <u-boot at emk-elektronik.de>

these two fields should match (e-mail)

also, this patch seems to depend on some other change not in mainline or Ben's 
net tree ...

> +#include <config.h>
> +#include <common.h>

the common.h header takes care of config.h already

> +#include <asm/arch/hardware.h>

err, what ?

> +#define ENC28J60_DRIVERNAME "enc"

this is used in only one place, so no point in a define

> +#define FAILSAFE_VALUE 5000

this isnt used anywhere

> +static u8 enc_r8 (u16 regNo);
> +static void enc_rbuf (u16 length, u8 *pBuff);

this whole file needs to be checked for function style.  no space after the 
function name.

also, drop the camel case and hungarian notation.  use "reg" and "buff".

> +/* NetReceive() requires the packet to be word aligned! */
> +static u8 packet[ENC_MAX_FRM_LEN] __attribute__ ((aligned(4)));

why do you need to declare your own buffer ?  the common code already sets up 
one for you and it uses the standard 1518 size (PKTSIZE).

> +int enc28j60_init (struct eth_device *dev, bd_t * bis)

all of the enc28j60 funcs that go into eth_device dont need to be public.  so 
add static to the init/halt/recv/send funcs.

also, it's "bd_t *bis"

> +	slave = spi_setup_slave(ENC_SPI_BUS, ENC_CS_NUM, ENC_SPI_CLOCK,
> SPI_MODE_0);

pass the bus/cs/speed/mode in to the initialize function and store it in the 
per-device state.  then you can work fine with multiple enc28j60 devices in 
one board.

along those lines, all of the local enc funcs should be changed to take the 
private state and operate off that instead of random global variables.

> +	spi_claim_bus (slave);

missing check on the return value

> +	do
> +	{
> +		estatVal = enc_r8(CTL_REG_ESTAT);
> +	} while ((estatVal & 0x08) || (~estatVal & ENC_ESTAT_CLKRDY));

the bit inverting ("~") should be on the mask so it's done at compile time 
instead of forcing the board to do it at runtime

> +#ifdef CONFIG_NET_MULTI
> +	memcpy(enetaddr, eth_get_dev()->enetaddr, 6);
> +#else
> +	eth_getenv_enetaddr("ethaddr", enetaddr);
> +#endif
> +	enc_init(enetaddr);

this should be one line:
	enc_init(dev->enetaddr);

also, nowhere in this init func can i see error checking that the enc28j60 
device is actually out there.  the enc_init() should return an error if the 
expected phy's/etc... dont match, and this init func should key off that.

> +static void enc_init (u8 *pEthAddr)
> +{
> +	u16 phid1 = 0;
> +	u16 phid2 = 0;

these are always set up below, so no point in setting them to 0

> +static void phy_write(u8 addr, u16 data)
>
> +	while((enc_r8(CTL_REG_MISTAT) & ENC_MISTAT_BUSY) != 0) {

missing a space after that "while"

> +	sprintf(dev->name, "%s%hu", ENC28J60_DRIVERNAME, dev_num);

once this func is fixed to take the spi bus/cs, the name can be based on that 
instead of an arbitrary integer.
	"enc%i.%i", bus, cs

> --- /dev/null
> +++ b/drivers/net/enc28j60.h

this header is missing ifdef protection against multiple inclusion

> --- a/include/netdev.h
> +++ b/include/netdev.h
>
> +int enc28j60_initialize(int id);

this doesnt match the prototype in the source.  you should have the .c file 
include netdev.h so that desyncs are caught & prevented in the future.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100825/87dd332f/attachment.pgp