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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
