Hi Po-Yu Chang,
Po-Yu Chuang wrote:
Dear Jean-Christophe,
2009/7/8 Jean-Christophe PLAGNIOL-VILLARD plagnioj@jcrosoft.com:
On 19:12 Wed 01 Jul , Po-Yu Chuang wrote:
This patch adds an FTMAC100 ethernet driver for Faraday A320 evaluation board.
it's seem good for me but I'll add it after the core soc
Thank you for your detailed review. I will resubmit this patch with new soc patch.
diff --git a/drivers/net/Makefile b/drivers/net/Makefile index c6097c3..8edf529 100644 --- a/drivers/net/Makefile +++ b/drivers/net/Makefile @@ -38,6 +38,7 @@ COBJS-$(CONFIG_E1000) += e1000.o COBJS-$(CONFIG_EEPRO100) += eepro100.o COBJS-$(CONFIG_ENC28J60) += enc28j60.o COBJS-$(CONFIG_FSLDMAFEC) += fsl_mcdmafec.o mcfmii.o +COBJS-$(CONFIG_DRIVER_FTMAC100) += ftmac100.o
please remove the DRIVER_
OK, but some recent patches use DRIVER_ naming.
CONFIG_DRIVER_TI_EMAC CONFIG_DRIVER_AX88180
Is it not the preferred style?
I don't have a strong opinion, but don't think the 'DRIVER' part adds anything useful, so I guess remove.
COBJS-$(CONFIG_GRETH) += greth.o COBJS-$(CONFIG_INCA_IP_SWITCH) += inca-ip_sw.o COBJS-$(CONFIG_KIRKWOOD_EGIGA) += kirkwood_egiga.o diff --git a/drivers/net/ftmac100.c b/drivers/net/ftmac100.c new file mode 100644 index 0000000..3057822 --- /dev/null +++ b/drivers/net/ftmac100.c +static void ftmac100_reset (struct eth_device *dev) +{
volatile struct ftmac100 *ftmac100 = (struct ftmac100 *)dev->iobase;do you really need to have all the struct volatile?
I had submitted a v3 of this driver which removed unnecessary volatiles according to the warnings told by checkpatch.pl.
http://lists.denx.de/pipermail/u-boot/2009-July/055421.html
Please ignore it, since I need to resubmit a new patch later.
debug ("%s()\n", __func__);writel (FTMAC100_MACCR_SW_RST, &ftmac100->maccr);while (readl (&ftmac100->maccr) & FTMAC100_MACCR_SW_RST) ;+}
+/*
- Set MAC address
- */
+int ftmac100_initialize (bd_t * bd) +{
struct eth_device *dev;struct ftmac100_data *priv;if (!(dev = malloc (sizeof *dev))) {use calloc instead
Is it preferred way? No driver in driver/net/ uses calloc.
malloc() is OK.
printf ("%s(): failed to allocate dev\n", __func__);goto out;}/* Transmit and receive descriptors should align to 16 bytes */if (!(priv = memalign (16, sizeof (struct ftmac100_data)))) {printf ("%s(): failed to allocate priv\n", __func__);goto free_dev;}memset (dev, 0, sizeof (*dev));memset (priv, 0, sizeof (*priv));sprintf (dev->name, "FTMAC100");dev->iobase = CONFIG_SYS_MAC100_BASE;dev->init = ftmac100_init;please use tab for indent
ok, fixed.
dev->halt = ftmac100_halt;dev->send = ftmac100_send;dev->recv = ftmac100_recv;dev->priv = priv;best regards, Po-Yu Chuang
Looking forward to the next spin.
regards, B en