Hi Guennadi,
Guennadi Liakhovetski wrote:
> From: Sascha Hauer <[EMAIL PROTECTED]>
>
> 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 <[EMAIL PROTECTED]>
> Signed-off-by: Guennadi Liakhovetski <[EMAIL PROTECTED]>
>
> ---
>
> 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 <[EMAIL PROTECTED]>
> + *
> + * 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
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
U-Boot-Users mailing list
U-Boot-Users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/u-boot-users
