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

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

[Uwe Kleine-König]

Since 39b2bbe3d715 (gpio: add flags argument to gpiod_get*() functions)
which appeared in v3.17-rc1, the gpiod_get* functions take an additional
parameter that allows to specify direction and initial value for output.
Moreover use devm_gpiod_get_optional instead of ignoring all errors
returned by devm_gpiod_get and simplify accordingly.

The result is more strict error handling which is good.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

with the local variable introduced to not keep an error value in the
driver struct this doesn't have a positive diffstat anymore. I still
think assigning directly to priv->gpiod_reset would fine because having
the priv->gpiod_reset NULL is neither more nor less right that
ERR_PTR(-ESOMETHING).

Note that my motivation for this change is to get all drivers use the
currently optional fourth parameter to stop it being optional which
needs hard to understand cpp trickery.

Best regards
Uwe

 drivers/net/phy/at803x.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index f80e19ac6704..fabf11d32d27 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -192,16 +192,17 @@ static int at803x_probe(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->dev;
 	struct at803x_priv *priv;
+	struct gpio_desc *gpiod_reset;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	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);
+	gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(gpiod_reset))
+		return PTR_ERR(gpiod_reset);
+
+	priv->gpiod_reset = gpiod_reset;
 
 	phydev->priv = priv;
 
-- 
2.1.4

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

[David Miller]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Date: Tue, 31 Mar 2015 22:08:45 +0200

> I still think assigning directly to priv->gpiod_reset would fine
> because having the priv->gpiod_reset NULL is neither more nor less
> right that ERR_PTR(-ESOMETHING).

You really want to push my buttons don't you? :-/

What if someone adds a new resource allocation after this thing and
then adds an unwind path where something has to be done to the GPIOD
as part of cleanup?

What does your error pointer cause to happen to an unsuspecting person
creating such a change?

Why isn't this obvious to you as a good coding practice that is
future proof against any changes to this code?

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

[Uwe Kleine-König]

On Tue, Mar 31, 2015 at 07:28:38PM -0400, David Miller wrote:
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Date: Tue, 31 Mar 2015 22:08:45 +0200
> 
> > I still think assigning directly to priv->gpiod_reset would fine
> > because having the priv->gpiod_reset NULL is neither more nor less
> > right that ERR_PTR(-ESOMETHING).
> 
> You really want to push my buttons don't you? :-/
> 
> What if someone adds a new resource allocation after this thing and
> then adds an unwind path where something has to be done to the GPIOD
> as part of cleanup?
As I assume that v2 still addresses your concerns it would be ok for me
to not fully agree with you about defensive coding style.
What about you?

Best regards
Uwe

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

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

[David Miller]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Date: Wed, 1 Apr 2015 09:00:01 +0200

> On Tue, Mar 31, 2015 at 07:28:38PM -0400, David Miller wrote:
>> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> Date: Tue, 31 Mar 2015 22:08:45 +0200
>> 
>> > I still think assigning directly to priv->gpiod_reset would fine
>> > because having the priv->gpiod_reset NULL is neither more nor less
>> > right that ERR_PTR(-ESOMETHING).
>> 
>> You really want to push my buttons don't you? :-/
>> 
>> What if someone adds a new resource allocation after this thing and
>> then adds an unwind path where something has to be done to the GPIOD
>> as part of cleanup?
> As I assume that v2 still addresses your concerns it would be ok for me
> to not fully agree with you about defensive coding style.
> What about you?

The v2 patch is fine.

What isn't fine is that in the future you'll keep submitting patches that
create dangerous situations, and I intend to keep rejecting them until
you stop putting error pointers into dynamically allocated memory.