[U-Boot-Users] [PATCH v2 4/7] add SMSC LAN9x1x Network driver

[Ben Warren]

Hi Guennadi,

Guennadi Liakhovetski wrote:
> From: Sascha Hauer <s.hauer at pengutronix.de>
>
> This patch adds a driver for the following smsc network controllers:
> LAN9115
> LAN9116
> LAN9117
> LAN9215
> LAN9216
> LAN9217
>
>   
How many of these have been tested, and on what platforms.  I'm asking 
because the code seems to assume a 32-bit interface and these aren't all 
32-bit chips.
> Signed-off-by: Sascha Hauer <s.hauer at pengutronix.de>
> Signed-off-by: Guennadi Liakhovetski <lg at denx.de>
>
> ---
>
> Changes since v1: Removed C++ style comments
>
>  drivers/net/Makefile  |    1 +
>  drivers/net/smc911x.c |  668 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 669 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/net/smc911x.c
>
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index 320dc3e..9482398 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -54,6 +54,7 @@ COBJS-y += rtl8139.o
>  COBJS-y += rtl8169.o
>  COBJS-y += s3c4510b_eth.o
>  COBJS-y += smc91111.o
> +COBJS-y += smc911x.o
>  COBJS-y += tigon3.o
>  COBJS-y += tsec.o
>  COBJS-y += tsi108_eth.o
> diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c
> new file mode 100644
> index 0000000..5830368
> --- /dev/null
> +++ b/drivers/net/smc911x.c
> @@ -0,0 +1,667 @@
> +/*
> + * SMSC LAN9[12]1[567] Network driver
> + *
> + * (c) 2007 Pengutronix, Sascha Hauer <s.hauer at pengutronix.de>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h>
> +
> +#ifdef CONFIG_DRIVER_SMC911X
> +
>   
This should be moved to the Makefile.  Looks like I beat J-C to this one...
> +#include <command.h>
> +#include <net.h>
> +#include <miiphy.h>
> +
> +#define mdelay(n)       udelay((n)*1000)
> +
> +#define __REG(x)     (*((volatile u32 *)(x)))
> +
>   
See, you're assuming 32-bit accesses.  That should be configurable, I think
> +/* Below are the register offsets and bit definitions
> + * of the Lan911x memory space
> + */
> +#define RX_DATA_FIFO		 __REG(CONFIG_DRIVER_SMC911X_BASE + 0x00)
> +
> +#define TX_DATA_FIFO		 __REG(CONFIG_DRIVER_SMC911X_BASE + 0x20)
> +#define	TX_CMD_A_INT_ON_COMP			(0x80000000)
> +#define	TX_CMD_A_INT_BUF_END_ALGN		(0x03000000)
> +#define	TX_CMD_A_INT_4_BYTE_ALGN		(0x00000000)
> +#define	TX_CMD_A_INT_16_BYTE_ALGN		(0x01000000)
> +#define	TX_CMD_A_INT_32_BYTE_ALGN		(0x02000000)
> +#define	TX_CMD_A_INT_DATA_OFFSET		(0x001F0000)
> +#define	TX_CMD_A_INT_FIRST_SEG			(0x00002000)
> +#define	TX_CMD_A_INT_LAST_SEG			(0x00001000)
> +#define	TX_CMD_A_BUF_SIZE			(0x000007FF)
> +#define	TX_CMD_B_PKT_TAG			(0xFFFF0000)
> +#define	TX_CMD_B_ADD_CRC_DISABLE		(0x00002000)
> +#define	TX_CMD_B_DISABLE_PADDING		(0x00001000)
> +#define	TX_CMD_B_PKT_BYTE_LENGTH		(0x000007FF)
> +
>   
Register and bitfield definitions should be in a header file.  More 
generally, only register addresses and bitfields should be
defined.  Using macros to encapsulate both address and function is bad 
form, IMHO.  More on that later.
<snip>
> +
> +#define DRIVERNAME "smc911x"
> +
> +u32 smc911x_get_mac_csr(u8 reg)
> +{
> +	while (MAC_CSR_CMD & MAC_CSR_CMD_CSR_BUSY);
>   
Using macros like this is both unreadable and hard to debug.  Instead, 
consider something like:

    while (reg_read(MAC_CSR) & MAC_CSR_BUSY));

IMHO, one-liner while loops are bad too, but that's debatable.

I haven't even gotten into the functionality, because I think there's a 
lot of work to be done just in coding style before we look at substance.

regards,
Ben