Dear Jean-Christophe, 2009/7/8 Jean-Christophe PLAGNIOL-VILLARD <plagn...@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? >> 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. >> + 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot