[U-Boot] [PATCH 4/4] net: Add LL TEMAC driver to u-boot

[Wolfgang Denk]

Dear Michal Simek,

sorry for the long delay.  We're still lacking a (new) network
custodian...

In message <1280753377-2894-4-git-send-email-monstr at monstr.eu> you wrote:
> Add Xilinx LL Temac driver to u-boot.
> 
> Signed-off-by: Michal Simek <monstr at monstr.eu>
> +++ b/drivers/net/xilinx_ll_temac.c
> @@ -0,0 +1,561 @@
> +/*
> + *

Drop blank line.

> + * Copyright (C) 2008 - 2009 Michal Simek <monstr at monstr.eu>
> + * June 2008 Microblaze optimalization, FIFO mode support

2009? Not 2010 ?


> +/* XPS_LL_TEMAC direct registers definition */
> +#define TEMAC_RAF0		(dev->iobase + 0x00)
> +#define TEMAC_TPF0		(dev->iobase + 0x04)
> +#define TEMAC_IFGP0		(dev->iobase + 0x08)
> +#define TEMAC_IS0		(dev->iobase + 0x0c)
> +#define TEMAC_IP0		(dev->iobase + 0x10)
> +#define TEMAC_IE0		(dev->iobase + 0x14)
> +
> +#define TEMAC_MSW0		(dev->iobase + 0x20)
> +#define TEMAC_LSW0		(dev->iobase + 0x24)
> +#define TEMAC_CTL0		(dev->iobase + 0x28)
> +#define TEMAC_RDY0		(dev->iobase + 0x2c)

Please use a C struct to describe the register layout.

> +/* XPS_LL_TEMAC indirect registers offset definition */
> +
> +#define RCW0	0x200
> +#define RCW1	0x240
> +#define TC	0x280
> +#define FCC	0x2c0
> +#define EMMC	0x300
> +#define PHYC	0x320
> +#define MC	0x340
> +#define UAW0	0x380
> +#define UAW1	0x384
> +#define MAW0	0x388
> +#define MAW1	0x38c
> +#define AFM	0x390
> +#define TIS	0x3a0
> +#define TIE	0x3a4
> +#define MIIMWD	0x3b0
> +#define MIIMAI	0x3b4

Ditto.


> +	out_be32((u32 *)TEMAC_LSW0, phy_data);
> +	out_be32((u32 *)TEMAC_CTL0, CNTLREG_WRITE_ENABLE_MASK | MIIMWD);
> +	out_be32((u32 *)TEMAC_LSW0, (phy_addr << 5) | (reg_addr));
> +	out_be32((u32 *)TEMAC_CTL0, \
> +			CNTLREG_WRITE_ENABLE_MASK | MIIMAI | (emac << 10));
> +	while (!(in_be32((u32 *)TEMAC_RDY0) & XTE_RSE_MIIM_WR_MASK))

The need to have all these casts should ring some alarm bell to you.
Please use a proper C struct instead.

> +	out_be32((u32 *)TEMAC_LSW0, reg_data);
> +	out_be32((u32 *)TEMAC_CTL0, \
> +			CNTLREG_WRITE_ENABLE_MASK | (emac << 10) | reg_offset);

Drop the backslash.

> +		debug ("%d: 0x%x ", j, result);
> +	}
> +	debug ("\n");

No spaces between function name and '('. Please fix globally.
Consider running your patch through checkpatch...

> +	while (retries-- &&
> +		((xps_ll_temac_hostif_get(dev, 0, phy_addr, 1) & 0x24) == 0x24))
> +		;

Bad indentation.


> +	if (i == 0x7c0a3) {
...
> +	if (i == 0x1410cc2) {

Please use self-explaining symbolic names for these magic numbers,
and/or add sufficient comments what these mean.

> +static void debugll(int count)
> +{
> +	printf ("%d fifo isr 0x%08x, fifo_ier 0x%08x, fifo_rdfr 0x%08x, "
> +		"fifo_rdfo 0x%08x fifo_rlr 0x%08x\n", count, ll_fifo->isr, \
> +		ll_fifo->ier, ll_fifo->rdfr, ll_fifo->rdfo, ll_fifo->rlf);

Drop the backslash.  Please check & fix globally.


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
What can it profit a man to gain the whole world and to come  to  his
property with a gastric ulcer, a blown prostate, and bifocals?
                                     -- John Steinbeck, _Cannery Row_