[U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100

[Ajay Bhargav <ajay.bhargav@einfochips.com>]

----- "Mike Frysinger" <vapier@gentoo.org> wrote:

> On Wednesday, August 24, 2011 09:07:18 Ajay Bhargav wrote:
> > +       darmdfec->p_rxdesc = (struct rx_desc *) memalign(PKTALIGN,
> > +                       ARMDFEC_RXQ_DESC_ALIGNED_SIZE * RINGSZ +
> 1);
> 
> memalign() returns a void*, so you shouldnt need to cast its return
> value (you 
> do this a couple of times)
> 
> > +	/* Read mac from env if available */
> > +	eth_getenv_enetaddr("ethaddr", dev->enetaddr);
> 
> you shouldnt need to do this.  the higher layers will take care of
> this for 
> you when you set write_hwaddr
> 

I do not have a hardware storage for MAC on my controller. write_hwaddr
is not needed for me.

> also, it seems like some of my previous feedback wasnt addressed ?
> 

I might have missed some points. My apologies.

> > +     while (cmd_sts & BUF_OWNED_BY_DMA) {
> > ...
> > +     };
> 
> no semi-colon needed
> 
> > +int armada100_fec_initialize()
> > +{
> > ...
> > +     darmdfec->regs = (void *) ARMD1_FEC_BASE;
> 
> make the reg base a parameter to armada100_fec_initialize()
> 

This driver is for Armada100 series and base address is same for
the whole series, so i did not feel passing it as a parameter. Can
you please tell me if there is any specific reason for the same?

> > +#if defined(CONFIG_PHY_BASE_ADR)
> > +     miiphy_write(dev->name, PHY_ADR_REQ, PHY_ADR_REQ,
> > +                  (u16) CONFIG_PHY_BASE_ADR);
> > +#else
> > +     /* Search phy address from range 0-31 */
> > +     phy_adr = ethernet_phy_detect(dev);
> > +     if (phy_adr < 0) {
> > +             printf("Error: PHY not detected at address range
> 0-31\n");
> > +             return -1;
> > +     } else {
> > +             debug("PHY detected at addr %d\n", phy_adr);
> > +             miiphy_write(dev->name, PHY_ADR_REQ, PHY_ADR_REQ,
> > +                          (u16) phy_adr);
> > +     }
> > +#endif
> 
> this should be done in the armdfec_init() func, not the initialize
> func
> -mike

Okay.. will move it there.

Thanks & Regards,
Ajay Bhargav