[U-Boot] [PATCH v4 tabify] net: add altera triple speeds ethernet mac driver

[Ben Warren]

Hi Thomas,

On 4/6/2010 9:01 PM, Thomas Chou wrote:
> Hi Ben,
>
> Thanks.
>
> On 04/05/2010 02:19 PM, Ben Warren wrote:
>>
>>> +
>>> +static int tse_eth_send(struct eth_device *dev, volatile void *packet,
>>> +            int length);
>>> +static int tse_eth_rx(struct eth_device *dev);
>>> +static void tse_eth_halt(struct eth_device *dev);
>>> +static void tse_eth_reset(struct eth_device *dev);
>>> +static int tse_eth_init(struct eth_device *dev, bd_t *bd);
>>> +
>>> +static int tse_mdio_read(struct altera_tse_priv *priv, unsigned int 
>>> regnum);
>>> +static int tse_mdio_write(struct altera_tse_priv *priv, unsigned 
>>> int regnum,
>>> +           unsigned int value);
>>
>> Are these prototypes really needed?  If so, please re-order the code 
>> so they're not.
> OK. I will reorder the code so that they will be not needed.
>
>>>
>>> +/* This is a generic routine that the SGDMA mode-specific routines
>>> + * call to populate a descriptor.
>>> + * arg1        :pointer to first SGDMA descriptor.
>>> + * arg2        :pointer to next  SGDMA descriptor.
>>> + * arg3        :Address to where data to be written.
>>> + * arg4        :Address from where data to be read.
>>> + * arg5        :no of byte to transaction.
>>> + * arg6        :variable indicating to generate start of packet or not
>>> + * arg7        :read fixed
>>> + * arg8        :write fixed
>>> + * arg9        :read burst
>>> + * arg10    :write burst
>>> + * arg11    :atlantic_channel number
>>> + */
>> 11 arguments??? Seriously???
> It might be simpler if I fold this call into the callers.
>>>
>>> +/* TSE init code */
>>> +int altera_tse_init(bd_t *bis, int num_tses)
>> The naming convention that we use is xxx_initialize(), or 
>> xxx_register(), although I prefer the former.  If you're not using 
>> *bis, don't pass it in.
>
>>>
>>> +    for (num = 0; num<  num_tses; num++) {
>> You don't use the 'num' variable.  As such, this driver doesn't 
>> support more than one instance.  Once you add true multi-instance 
>> support, the preferred way to do this is to call this function for 
>> each instance, passing in the appropriate addressing information.
> This driver needs several components and several base addresses. Can I 
> pass them in a structure?
>
> int altera_tse_initialize(u8 dev_num, void *base_info)
>
There's precedent for that (tsec, for example), but I'm not a big fan.  
If it's a small number (maybe no more than 4), pass each as an 
argument.  If you need a struct, though, you know the type, so don't use 
void*.
>>>
>>> +    return num_tses;
>> This return value is meaningless, as mentioned above.
> Will return 0.
>
No, return the number of interfaces initialized.  In your case, it will 
be 1, although I'd prefer to see this as a true multi-capable driver.
>>>
>>> +
>>> +    /* Set the MAC address */
>>> +    debug("Setting MAC address to 0x%x%x%x%x%x%x\n",
>>> +          dev->enetaddr[5], dev->enetaddr[4],
>>> +          dev->enetaddr[3], dev->enetaddr[2],
>>> +          dev->enetaddr[1], dev->enetaddr[0]);
>>> +    mac_dev->mac_addr_0 = ((dev->enetaddr[3])<<  24 |
>>> +                   (dev->enetaddr[2])<<  16 |
>>> +                   (dev->enetaddr[1])<<  8 | (dev->enetaddr[0]));
>>> +
>>> +    mac_dev->mac_addr_1 = ((dev->enetaddr[5]<<  8 |
>>> +                (dev->enetaddr[4]))&  0xFFFF);
>>> +
>>> +    /* Set the MAC address */
>>> +    mac_dev->supp_mac_addr_0_0 = mac_dev->mac_addr_0;
>>> +    mac_dev->supp_mac_addr_0_1 = mac_dev->mac_addr_1;
>>> +
>>> +    /* Set the MAC address */
>>> +    mac_dev->supp_mac_addr_1_0 = mac_dev->mac_addr_0;
>>> +    mac_dev->supp_mac_addr_1_1 = mac_dev->mac_addr_1;
>>> +
>>> +    /* Set the MAC address */
>>> +    mac_dev->supp_mac_addr_2_0 = mac_dev->mac_addr_0;
>>> +    mac_dev->supp_mac_addr_2_1 = mac_dev->mac_addr_1;
>>> +
>>> +    /* Set the MAC address */
>>> +    mac_dev->supp_mac_addr_3_0 = mac_dev->mac_addr_0;
>>> +    mac_dev->supp_mac_addr_3_1 = mac_dev->mac_addr_1;
>>> +
>> Please put the MAC address programming code in a separate function, 
>> taking a eth_dev * as parameter.  It may save you work later.
> OK.
>
>>>
>>>   /* Driver initialization prototypes */
>>>   int au1x00_enet_initialize(bd_t*);
>>> +int altera_tse_init(bd_t *bis, int num_tses);
>> Alphabetical order, please.
> OK.
>>>   int at91emac_register(bd_t *bis, unsigned long iobase);
>>>   int bfin_EMAC_initialize(bd_t *bis);
>>>   int cs8900_initialize(u8 dev_num, int base_addr);
>>
>
> Best regards,
> Thomas
regards,
Ben