Re: [PATCH] net: phy: at803x: simplify using devm_gpiod_get_optional and its 4th argument

[David Miller <davem@davemloft.net>]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Date: Fri, 27 Mar 2015 20:25:02 +0100

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