On 01/27/2011 07:55 PM, root wrote:
> From: Joe Xue <lg...@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 <lg...@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
--
=====================================================================
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
=====================================================================
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
