| Message ID | 20100605140017.GA2750@riccoc20.at.omicron.at |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Richard Cochran <richardcochran@gmail.com> Date: Sat, 5 Jun 2010 16:00:17 +0200 > On Wed, Jun 02, 2010 at 02:32:11PM -0500, Andy Fleming wrote: >> Yeah, I was clearly not thinking clearly. dev_flags will be >> overwritten, and is not meant for this. I believe, what we should do >> is add a "port" field to the PHY device, and if PCR_FIBER_SELECT is >> set, then set the port field to PORT_FIBRE. I'm not entirely clear on >> the semantics of that field in the ethtool cmd, but it seems like the >> right idea. > > Here is another try. Is that more like it? I think this is overkill. One, and only one, PHY driver wants to maintain a boolean state and we're adding a full 32-bit flags member to the generic PHY device struct to accomodate this? Andy if you have ideas to use that state via ethtool or whatever in the future, you come back to us when the future arrives and you've implemented the code in the generic PHY lib code to do that stuff. As it stands right now, that code doesn't exist so accomodating it is just silly. I also think worrying about the phy_dev->priv pointer being misused in the future inside of this driver is a completely bogus argument by any measure. We have many cases all over the kernel tree, in drivers and elsewhere, where we encode integer states in what are normally pointer values when we need to maintain a small piece of state and don't need to do a full blown memory allocation to allocate a piece of extra memory to hold that state. Richard, please just resubmit your original patch and I will apply it. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 7, 2010 at 3:18 AM, David Miller <davem@davemloft.net> wrote: > From: Richard Cochran <richardcochran@gmail.com> > Date: Sat, 5 Jun 2010 16:00:17 +0200 > >> On Wed, Jun 02, 2010 at 02:32:11PM -0500, Andy Fleming wrote: >>> Yeah, I was clearly not thinking clearly. dev_flags will be >>> overwritten, and is not meant for this. I believe, what we should do >>> is add a "port" field to the PHY device, and if PCR_FIBER_SELECT is >>> set, then set the port field to PORT_FIBRE. I'm not entirely clear on >>> the semantics of that field in the ethtool cmd, but it seems like the >>> right idea. >> >> Here is another try. Is that more like it? > > I think this is overkill. Well, I meant for it to be the same as the ethtool one, and not a flags field. > > One, and only one, PHY driver wants to maintain a boolean state and > we're adding a full 32-bit flags member to the generic PHY device > struct to accomodate this? To be fair, this is a generic problem, with a simple solution. I was suggesting that a tiny patch would pave the way for others to follow suit. Instead, now a tiny patch will do something strange and incomprehensible, and then will be changed later when some arbitrary threshold is reached of PHY drivers needing to know what type of port they are hooked up to. > > Andy if you have ideas to use that state via ethtool or whatever in > the future, you come back to us when the future arrives and you've > implemented the code in the generic PHY lib code to do that stuff. Is there some reason for resistance to taking small steps in the right direction of an obviously good destination (recording the port type)? At the very least, can I ask that instead of assigning phydev->priv to the address of the probe function, that we do something like: phydev->priv = (void *) PCR_FIBER_SELECT; And then check to make sure it is that value? It's still hacky (IMHO), but at least it's self-documenting. > > As it stands right now, that code doesn't exist so accomodating it is > just silly. > > I also think worrying about the phy_dev->priv pointer being misused in > the future inside of this driver is a completely bogus argument by any > measure. > > We have many cases all over the kernel tree, in drivers and elsewhere, > where we encode integer states in what are normally pointer values > when we need to maintain a small piece of state and don't need to do a > full blown memory allocation to allocate a piece of extra memory to > hold that state. I would consider it a case of last resort, for when you need to avoid a memory allocation for performance reasons, and you must use a generic structure for a non-generic task. This is something detected in slow-path code, and is a generic task, so we're only hitting 1/3 of those conditions. I won't speculate as to how many of those other cases in the tree I would find annoying. ;) Andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c index 8ee929b..ef4a320 100644 --- a/drivers/net/phy/lxt.c +++ b/drivers/net/phy/lxt.c @@ -53,6 +53,9 @@ #define MII_LXT971_ISR 19 /* Interrupt Status Register */ +/* register definitions for the 973 */ +#define MII_LXT973_PCR 16 /* Port Configuration Register */ +#define PCR_FIBER_SELECT 1 MODULE_DESCRIPTION("Intel LXT PHY driver"); MODULE_AUTHOR("Andy Fleming"); @@ -119,6 +122,34 @@ static int lxt971_config_intr(struct phy_device *phydev) return err; } +static int lxt973_probe(struct phy_device *phydev) +{ + int val = phy_read(phydev, MII_LXT973_PCR); + + if (val & PCR_FIBER_SELECT) { + /* + * If fiber is selected, then the only correct setting + * is 100Mbps, full duplex, and auto negotiation off. + */ + val = phy_read(phydev, MII_BMCR); + val |= (BMCR_SPEED100 | BMCR_FULLDPLX); + val &= ~BMCR_ANENABLE; + phy_write(phydev, MII_BMCR, val); + /* Remember that the port is in fiber mode. */ + phydev->port_flags |= PHY_PORT_FIBER; + } else { + phydev->port_flags &= ~PHY_PORT_FIBER; + } + return 0; +} + +static int lxt973_config_aneg(struct phy_device *phydev) +{ + /* Do nothing if port is in fiber mode. */ + return phydev->port_flags & PHY_PORT_FIBER ? + 0 : genphy_config_aneg(phydev); +} + static struct phy_driver lxt970_driver = { .phy_id = 0x78100000, .name = "LXT970", @@ -146,6 +177,18 @@ static struct phy_driver lxt971_driver = { .driver = { .owner = THIS_MODULE,}, }; +static struct phy_driver lxt973_driver = { + .phy_id = 0x00137a10, + .name = "LXT973", + .phy_id_mask = 0xfffffff0, + .features = PHY_BASIC_FEATURES, + .flags = 0, + .probe = lxt973_probe, + .config_aneg = lxt973_config_aneg, + .read_status = genphy_read_status, + .driver = { .owner = THIS_MODULE,}, +}; + static int __init lxt_init(void) { int ret; @@ -157,9 +200,15 @@ static int __init lxt_init(void) ret = phy_driver_register(&lxt971_driver); if (ret) goto err2; + + ret = phy_driver_register(&lxt973_driver); + if (ret) + goto err3; return 0; - err2: + err3: + phy_driver_unregister(&lxt971_driver); + err2: phy_driver_unregister(&lxt970_driver); err1: return ret; @@ -169,6 +218,7 @@ static void __exit lxt_exit(void) { phy_driver_unregister(&lxt970_driver); phy_driver_unregister(&lxt971_driver); + phy_driver_unregister(&lxt973_driver); } module_init(lxt_init); diff --git a/include/linux/phy.h b/include/linux/phy.h index 1c75b6b..602228c 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -234,6 +234,11 @@ enum phy_state { PHY_RESUMING }; +/* + * PHY_PORT_xxx: flags to describe the port's fixed attributes. + */ +#define PHY_PORT_FIBER 0x00000001 /* Port has a fiber optic transceiver */ + /* phy_device: An instance of a PHY * * drv: Pointer to the driver for this PHY instance @@ -246,6 +251,7 @@ enum phy_state { * link_timeout: The number of timer firings to wait before the * giving up on the current attempt at acquiring a link * irq: IRQ number of the PHY's interrupt (-1 if none) + * port_flags: Bit field of PHY_PORT_xxx flags * phy_timer: The timer for handling the state machine * phy_queue: A work_queue for the interrupt * attached_dev: The attached enet driver's device instance ptr @@ -314,6 +320,8 @@ struct phy_device { */ int irq; + int port_flags; + /* private data pointer */ /* For use by PHYs to maintain extra state */ void *priv;