Hello Sergei, [we already talked about this patch in #armlinux, I'm now just forwarding my comments on the list. Background was that I sent an easier and less complete patch with the same idea. See http://patchwork.ozlabs.org/patch/621418/] [added Linus Walleij to Cc, there is a question for you/him below] On Fri, Apr 29, 2016 at 01:12:54AM +0300, Sergei Shtylyov wrote: > --- net-next.orig/Documentation/devicetree/bindings/net/phy.txt > +++ net-next/Documentation/devicetree/bindings/net/phy.txt > @@ -35,6 +35,8 @@ Optional Properties: > - broken-turn-around: If set, indicates the PHY device does not correctly > release the turn around line low at the end of a MDIO transaction. > > +- reset-gpios: The GPIO phandle and specifier for the PHY reset signal. > + > Example: > > ethernet-phy@0 { This is great. > Index: net-next/drivers/net/phy/at803x.c > =================================================================== > --- net-next.orig/drivers/net/phy/at803x.c > +++ net-next/drivers/net/phy/at803x.c > @@ -65,7 +65,6 @@ MODULE_LICENSE("GPL"); > [...] My patch breaks this driver. I wasn't aware of it. > [...] > Index: net-next/drivers/net/phy/mdio_device.c > =================================================================== > --- net-next.orig/drivers/net/phy/mdio_device.c > +++ net-next/drivers/net/phy/mdio_device.c > [...] > @@ -103,6 +105,13 @@ void mdio_device_remove(struct mdio_devi > } > EXPORT_SYMBOL(mdio_device_remove); > > +void mdio_device_reset(struct mdio_device *mdiodev, int value) > +{ > + if (mdiodev->reset) > + gpiod_set_value(mdiodev->reset, value); Before v4.6-rc1~108^2~91 it was not necessary to check for the first parameter being non-NULL before calling gpiod_set_value. Linus, did you change this on purpose? > +} > +EXPORT_SYMBOL(mdio_device_reset); > + > /** > * mdio_probe - probe an MDIO device > * @dev: device to probe > @@ -117,9 +126,16 @@ static int mdio_probe(struct device *dev > struct mdio_driver *mdiodrv = to_mdio_driver(drv); > int err = 0; > > - if (mdiodrv->probe) > + if (mdiodrv->probe) { > + /* Deassert the reset signal */ > + mdio_device_reset(mdiodev, 0); > + > err = mdiodrv->probe(mdiodev); > > + /* Assert the reset signal */ > + mdio_device_reset(mdiodev, 1); I wonder if it's safe to do this in general. What if ->probe does something with the phy that is lost by resetting but that is relied on later? > + } > + > return err; > } > [...] > Index: net-next/drivers/of/of_mdio.c > =================================================================== > --- net-next.orig/drivers/of/of_mdio.c > +++ net-next/drivers/of/of_mdio.c > @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n > static void of_mdiobus_register_phy(struct mii_bus *mdio, > struct device_node *child, u32 addr) > { > + struct gpio_desc *gpiod; > struct phy_device *phy; > bool is_c45; > int rc; > @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru > is_c45 = of_device_is_compatible(child, > "ethernet-phy-ieee802.3-c45"); > > + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios"); > + /* Deassert the reset signal */ > + if (!IS_ERR(gpiod)) > + gpiod_direction_output(gpiod, 0); This is wrong I think. You must only ignore -ENODEV, all other error codes should be passed to the caller. (I see that's not trivial because of_mdiobus_register_phy returns void.) In my patch I used devm_gpiod_get_array which has the nice property that I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime of the gpio to the device which is nice and IMHO the right direction for the phylib (i.e. better embracing of the device model). This cannot be used here easily however because there is no struct device yet and this is only created after the phy id is determined. The phy id is either read from the device tree or must be read from the phy which might fail if reset is not deasserted. Principally there is no reason however that the phy_id must be known before the struct device is created however. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
