| Message ID | 20200622093744.13685-6-brgl@bgdev.pl |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: correctly model the PHY voltage supply in DT | expand |
On Mon, Jun 22, 2020 at 11:37:34AM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Currently we only call phy_device_reset() if the PHY driver implements > the probe() callback. This is not mandatory and many drivers (e.g. > realtek) don't need probe() for most devices but still can have reset > GPIOs defined. There's no reason to depend on the presence of probe() > here so pull the reset code out of the if clause. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On 6/22/20 2:37 AM, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Currently we only call phy_device_reset() if the PHY driver implements > the probe() callback. This is not mandatory and many drivers (e.g. > realtek) don't need probe() for most devices but still can have reset > GPIOs defined. There's no reason to depend on the presence of probe() > here so pull the reset code out of the if clause. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> OK, but now let's imagine that a PHY device has two or more reset lines, one of them is going to be managed by the core PHY library and the rest is going to be under the responsibility of the PHY driver, that does not sound intuitive or convenient at all. This is a hypothetical case, but it could conceivable happen, so how about adding a flag to the driver that says "let me manage it a all"?
wt., 23 cze 2020 o 21:14 Florian Fainelli <f.fainelli@gmail.com> napisał(a): > > On 6/22/20 2:37 AM, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Currently we only call phy_device_reset() if the PHY driver implements > > the probe() callback. This is not mandatory and many drivers (e.g. > > realtek) don't need probe() for most devices but still can have reset > > GPIOs defined. There's no reason to depend on the presence of probe() > > here so pull the reset code out of the if clause. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > OK, but now let's imagine that a PHY device has two or more reset lines, > one of them is going to be managed by the core PHY library and the rest > is going to be under the responsibility of the PHY driver, that does not > sound intuitive or convenient at all. This is a hypothetical case, but > it could conceivable happen, so how about adding a flag to the driver > that says "let me manage it a all"? This sounds good as a new feature idea but doesn't seem to be related to what this patch is trying to do. The only thing it does is improve the current behavior. I'll note your point for the future work on the pre-probe stage. Bartosz
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 1b4df12c70ad..f6985db08340 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -2690,16 +2690,13 @@ static int phy_probe(struct device *dev) mutex_lock(&phydev->lock); - if (phydev->drv->probe) { - /* Deassert the reset signal */ - phy_device_reset(phydev, 0); + /* Deassert the reset signal */ + phy_device_reset(phydev, 0); + if (phydev->drv->probe) { err = phydev->drv->probe(phydev); - if (err) { - /* Assert the reset signal */ - phy_device_reset(phydev, 1); + if (err) goto out; - } } /* Start out supporting everything. Eventually, @@ -2761,6 +2758,10 @@ static int phy_probe(struct device *dev) phydev->state = PHY_READY; out: + /* Assert the reset signal */ + if (err) + phy_device_reset(phydev, 1); + mutex_unlock(&phydev->lock); return err; @@ -2779,12 +2780,12 @@ static int phy_remove(struct device *dev) sfp_bus_del_upstream(phydev->sfp_bus); phydev->sfp_bus = NULL; - if (phydev->drv && phydev->drv->remove) { + if (phydev->drv && phydev->drv->remove) phydev->drv->remove(phydev); - /* Assert the reset signal */ - phy_device_reset(phydev, 1); - } + /* Assert the reset signal */ + phy_device_reset(phydev, 1); + phydev->drv = NULL; return 0;