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

[Thomas Chou <thomas@wytron.com.tw>]

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)

>>
>> +    return num_tses;
> This return value is meaningless, as mentioned above.
Will return 0.

>>
>> +
>> +    /* 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