[U-Boot] [PATCHv2 1/3] A320: driver for FTMAC100 ethernet controller

[Po-Yu Chuang]

Dear Jean-Christophe,

2009/7/8 Jean-Christophe PLAGNIOL-VILLARD <plagnioj at 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