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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

On Tue, Mar 31, 2015 at 12:57:54PM -0400, David Miller wrote:
> 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.
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/  |