On 01/27/2011 07:55 PM, root wrote:
From: Joe Xue lgxue@hotmail.com
You have to set you Signe-off-by in your patch. Please add also a commit message to explain what your patch is doing, and on which board you tested this driver. And set the net custodian as CC, this helps to get faster review.
--- a/drivers/net/Makefile +++ b/drivers/net/Makefile @@ -83,6 +83,7 @@ COBJS-$(CONFIG_TSI108_ETH) += tsi108_eth.o COBJS-$(CONFIG_ULI526X) += uli526x.o COBJS-$(CONFIG_VSC7385_ENET) += vsc7385.o COBJS-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o +COBJS-$(CONFIG_DRIVER_AX88183) += ax88183.o
Please maintain the list sorted.
@@ -0,0 +1,265 @@
+/*
- (C) Copyright 2010 Joe Xue lgxue@hotmail.com
- This file is released under the terms of GPL v2 and any later version.
- See the file COPYING in the root directory of the source tree for details.
+*/
The header is not enough. Please add coherent header with license, as you can see for other drivers. This must be fixed globally.
+/*
- This driver works on 32bit mode
- AX88783 has two ethernet ports, we only use port 0 in u-boot
- */
I suggest you add this comment to the README file, too,
+#include <common.h> +#include <command.h> +#include <net.h> +#include <linux/mii.h> +#include <miiphy.h> +#include <config.h> +#include "ax88183.h"
+static int ax88183_init(bd_t *bd) +{
- volatile unsigned int tmp;
- int i;
- /* reset chip */
- tmp = CR;
- CR = (tmp & ~CR_CHIP_RESET);
u-boot does not not accept anymore to write/read into registers using offsets and volatile variables. Please define an appropriate structure for your chip, and use read/write accessors to work with registers. This must be fixed globally in all your code.
I do not know this chip. However, u-boot has already a ax88180 driver. Would be not possible to modify this driver to make it suitable for your chip, too ?
Other remarks: it seems to me your driver does not support CONFIG_MULTI, as it should do. And it must call eth_register() to be inserted as device at runtime. Please check with other network drivers.
- udelay(1000);
- CR = (tmp | CR_CHIP_RESET);
- /* disable interrupt */
- IMSR = IMSR_MASK_ALL;
- /* set mac address*/
- unsigned char mactmp[4];
- unsigned int * mac = (unsigned int *)mactmp;
- mactmp[0] = bd->bi_enetaddr[5];
- mactmp[1] = bd->bi_enetaddr[4];
- mactmp[2] = bd->bi_enetaddr[3];
- mactmp[3] = bd->bi_enetaddr[2];
This is not correct anymore. You do not have to set the mac from the bd. Instead of that, you have to provide in your initialization function a struct eth_device that should be passed to eth_register().
+int eth_init (bd_t * bd) +{
- char *s, *e;
- int i;
- s = getenv ("ethaddr");
The driver must not call getenv explicitely.
+#if 0
- printf ("\tHW MAC address: "
"%02X:%02X:%02X:%02X:%02X:%02X\n",bd->bi_enetaddr[0], bd->bi_enetaddr[1],bd->bi_enetaddr[2], bd->bi_enetaddr[3],bd->bi_enetaddr[4], bd->bi_enetaddr[5] );+#endif
do not add dead code. Instead of that, you can use the debug() function to trace.
+/*
- *-------------------------------------------------------------------------
- Registers
- *-------------------------------------------------------------------------
- */
+#define PCR (*(volatile unsigned int *)(CONFIG_AX88183_BASE + 0x0004))
Replace all defines with a C structure describing your chip.
Best regards, Stefano Babic