From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ben Warren <biggerbadderben@gmail.com>
Date: Thu, 09 Jul 2009 22:32:43 -0700
Subject: [U-Boot] [PATCHv2 1/3] A320: driver for FTMAC100 ethernet
	controller
In-Reply-To: <ec1aee9f0907082328w6d4ee403y90d62c5de5a66698@mail.gmail.com>
References: <ec1aee9f0907010412t656c1bc2o34f386c59baf4199@mail.gmail.com>	
	<20090708111210.GM30395@game.jcrosoft.org>
	<ec1aee9f0907082328w6d4ee403y90d62c5de5a66698@mail.gmail.com>
Message-ID: <4A56D27B.5050309@gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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