On Tue, 2008-10-21 at 12:08 -0500, Andy Fleming wrote: Hi Andy, thanks for reviewing. > > +config BCM63XX_PHY > > + tristate "Drivers for Broadcom 63xx SOCs internal PHY" > > + depends on BCM63XX > This is probably right, but just to check: These PHYs will never be > used outside of the BCM63xx family? Correct, the PHY is actually inside the SOC. > > + /* Mask interrupts globally. */ > > + reg |= MII_BCM63XX_IR_GMASK; > > + err = phy_write(phydev, MII_BCM63XX_IR, reg); > > + if (err < 0) > > + return err; > > + > > + /* Unmask events we are interested in */ > > + reg = ~(MII_BCM63XX_IR_DUPLEX | > > + MII_BCM63XX_IR_SPEED | > > + MII_BCM63XX_IR_LINK) | > > + MII_BCM63XX_IR_EN; > > You just cleared the global interrupt mask. I have two problems with > that: That should be '&=' yes, and I could do one write instead of two. Yet the current code does not clear the global interrupt mask, IR_GMASK bit is still set, so interrupts are disabled after init. I will fix that, it seems another bit in this register controls a LED, I should not force it to 1. > The other comment I have is that these probably should go in the > broadcom.c file. I'm not deeply tied to the notion of one file per > company, but it has become the way it is done. Ok will do, I just hope the file won't become too big, that would be quite wasted space since there is no chance to find the other PHYs on any bcm63xx boards. Thanks -- Maxime
