Re: [PATCH] net: phy: at803x: simplify using devm_gpiod_get_optional and its 4th argument
From: David Miller <davem@davemloft.net>
Date: 2015-03-31 16:57:56
From: Uwe Kleine-König <redacted> Date: Fri, 27 Mar 2015 20:25:02 +0100
quoted hunk ↗ jump to hunk
@@ -197,15 +197,12 @@ static int at803x_probe(struct phy_device *phydev) if (!priv) return -ENOMEM; - priv->gpiod_reset = devm_gpiod_get(dev, "reset"); - if (IS_ERR(priv->gpiod_reset)) - priv->gpiod_reset = NULL; - else - gpiod_direction_output(priv->gpiod_reset, 1); + priv->gpiod_reset = devm_gpiod_get_optional(dev, "reset", + GPIOD_OUT_HIGH); phydev->priv = priv; - return 0; + return IS_ERR(priv->gpiod_reset); } static int at803x_config_init(struct phy_device *phydev)
This isn't right. The current code is necessary, don't change it. Your "simplification" adds three new bugs: 1) It potentially leaves an error pointer in priv->gpiod_reset and I explicitly tell people to NEVER do this as it tests as non-NULL by cleanup code and therefore might be mistakenly used. 2) It returns the wrong error. IS_ERR() is either true or false, but if you wanted to do this right you would return PTR_ERR() if IS_ERR() were true or zero. 3) Clearly this code intended to continue trying and succeed the probe even if getting "reset" failed, your changes no longer do this. I really hate changes like this, don't try to be too cute unless you fully understand the full repurcussions and the reasons why someone did things one way or another. Leaving error pointers live in datastructures is never to be done, and if I see patches adding code doing it I will reject it on the spot.