[U-Boot-Users] [PATCH] AX88180: new gigabit network driver

[Wolfgang Denk]

In message <1212296315-10081-1-git-send-email-vapier at gentoo.org> you wrote:
> From: Michael Hennerich <michael.hennerich at analog.com>
> 
> A new driver for the AXIS AX88180 gigabit ethernet chip.
...
> +++ b/drivers/net/ax88180.c
> @@ -0,0 +1,943 @@
> +/* ax88180: ASIX AX88180 Non-PCI Gigabit Ethernet u-boot driver
> + * Licensed under the GPL-2.
> + */
> +/*
> + * ========================================================================
> + * ASIX AX88180 Non-PCI 16/32-bit Gigabit Ethernet Linux Driver
> + *
> + * The AX88180 Ethernet controller is high performance and highly
> + * integrated local CPU bus Ethernet controllers with embedded 40K bytes
> + * SRAM and supports both 16-bit and 32-bit SRAM-Like interfaces
> + * for any embedded systems.
> + * The AX88180 is a single chip 10/100/1000Mbps Gigabit Ethernet controller
> + * that supports both MII and RGMII interfaces and is compliant to
> + * IEEE 802.3, IEEE 802.3u and IEEE 802.3z standards.
> + *
> + * Please visit ASIX's web site (http://www.asix.com.tw) for more details.
> + *
> + * Module Name : ax88180.c
> + * Purpose     : This file is the main file.
> + * Author      : Allan Chou <allan at asix.com.tw>
> + * Date        : 2006-09-06
> + * Notes       :
> + * History     :
> + * $Log:$
> + * 1.0.0	2006-09-06
> + * New release for AX88180 US2 chip.

Who holds the Copyright for this code?


...
> +#define READ_RXBUF(data) data = readw(AX88180_BASE + RXBUFFER_START + RXBUF_OFFSET_16BIT)
> +#define WRITE_TXBUF(data) writew(data, AX88180_BASE + TXBUFFER_START + TXBUF_OFFSET_16BIT)
> +#define READ_MACREG(regaddr, regdata) regdata = readw(AX88180_BASE + MACREG_OFFSET_16BIT + regaddr)
> +#define WRITE_MACREG(regaddr, regdata) writew(regdata, AX88180_BASE + MACREG_OFFSET_16BIT + regaddr);
> +#else				/* defined(__bfin__) */
> +#define READ_RXBUF(data)	data = *(volatile unsigned short *)(AX88180_BASE + RXBUFFER_START)
> +#define WRITE_TXBUF(data)	*(volatile unsigned short *)(AX88180_BASE + TXBUFFER_START) = data
> +#define READ_MACREG(regaddr, regdata) regdata = *(volatile unsigned short*)(AX88180_BASE + regaddr)
> +#define WRITE_MACREG(regaddr, regdata) *(volatile unsigned short*)(AX88180_BASE + regaddr) = regdata;

Lines MUCH too long!!! ditto in rest of file.

> +	for (k1 = 0; k1 < 10000; k1++) { \
> +		READ_MACREG(MDIOCTRL, tmpval1); \
> +		if ((tmpval1 & READ_PHY) == 0) { \
> +			break; \
> +		} \
> +		udelay(1); \
> +	} \
> +	READ_MACREG(MDIODP, regdata); \
> +}
> +#define WRITE_PHYREG(phyaddr, regaddr, regdata) { \
> +	unsigned long tmpval2, k2; \
> +	WRITE_MACREG(MDIODP, regdata); \
> +	WRITE_MACREG(MDIOCTRL, WRITE_PHY | (regaddr << 8) | phyaddr); \
> +	for (k2 = 0; k2 < 10000; k2++) { \
> +		READ_MACREG(MDIOCTRL, tmpval2); \
> +		if ((tmpval2 & WRITE_PHY) == 0) { \
> +			break; \
> +		} \
> +		udelay(1); \
> +	} \
> +}

I have no ide where this code is coming from, but I cannot  help  but
asking myself if the author of this code ever heard about the concept
of functions?

> +#define RESET_MAC { \
...
> +#define RESET_PHY { \
...
> +#define	INIT_TXRX_VARIABLES { \
...
> +#define	DISPLAY_ALLMACREG { \
...
> +#define	DISPLAY_ALLPHYREG { \

And so on - all the macros shall be changed into functions.

Stopped reviewing here.

Please fix and resubmit - as is, this code has no chance to  go  into
mainline.

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
A well-written program is its own heaven; a poorly-written program is
its own hell.             -- Geoffrey James, "The Tao of Programming"