On Tue, Mar 31, 2015 at 12:57:54PM -0400, David Miller wrote:

From: Uwe Kleine-König <redacted>
Date: Fri, 27 Mar 2015 20:25:02 +0100

quoted

@@ -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.

If priv->gpiod_reset is an error value it makes the probe routine return
this error (after point 2 is addressed), which should end the lifetime
of the structure containing the value.

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.

Ah right, I should use PTR_ERR_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.

It uses the _optional variant of devm_gpiod_get now, which returns NULL if
devm_gpiod_get would return -ENOENT. For all other errors returned by
devm_gpiod_get the code that is currently in place is wrong to ignore
them.

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.

I think I did understand the code, so I will resend with
PTR_ERR_OR_ZERO assuming it was you who didn't understood my change
regarding the other two issues you pointed out.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |