[PATCH v2] net: mdio: switch to using gpiod_get_optional()

[PATCH v2] net: mdio: switch to using gpiod_get_optional()

[Dmitry Torokhov]

The MDIO device reset line is optional and now that gpiod_get_optional()
returns proper value when GPIO support is compiled out, there is no
reason to use fwnode_get_named_gpiod() that I plan to hide away.

Let's switch to using more standard gpiod_get_optional() and
gpiod_set_consumer_name() to keep the nice "PHY reset" label.

Also there is no reason to only try to fetch the reset GPIO when we have
OF node, gpiolib can fetch GPIO data from firmwares as well.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

Note this is an update to a patch titled "[PATCH 05/11] net: mdio:
switch to using fwnode_gpiod_get_index()" that no longer uses the new
proposed API and instead works with already existing ones.

 drivers/net/phy/mdio_bus.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index ce940871331e..2e29ab841b4d 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -42,21 +42,17 @@
 
 static int mdiobus_register_gpiod(struct mdio_device *mdiodev)
 {
-	struct gpio_desc *gpiod = NULL;
+	int error;
 
 	/* Deassert the optional reset signal */
-	if (mdiodev->dev.of_node)
-		gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode,
-					       "reset-gpios", 0, GPIOD_OUT_LOW,
-					       "PHY reset");
-	if (IS_ERR(gpiod)) {
-		if (PTR_ERR(gpiod) == -ENOENT || PTR_ERR(gpiod) == -ENOSYS)
-			gpiod = NULL;
-		else
-			return PTR_ERR(gpiod);
-	}
-
-	mdiodev->reset_gpio = gpiod;
+	mdiodev->reset_gpio = gpiod_get_optional(&mdiodev->dev,
+						 "reset", GPIOD_OUT_LOW);
+	error = PTR_ERR_OR_ZERO(mdiodev->reset_gpio);
+	if (error)
+		return error;
+
+	if (mdiodev->reset_gpio)
+		gpiod_set_consumer_name(mdiodev->reset_gpio, "PHY reset");
 
 	return 0;
 }
-- 
2.23.0.237.gc6a4ce50a0-goog


-- 
Dmitry

Re: [PATCH v2] net: mdio: switch to using gpiod_get_optional()

[Andrew Lunn]

On Fri, Sep 13, 2019 at 03:55:47PM -0700, Dmitry Torokhov wrote:
> The MDIO device reset line is optional and now that gpiod_get_optional()
> returns proper value when GPIO support is compiled out, there is no
> reason to use fwnode_get_named_gpiod() that I plan to hide away.
> 
> Let's switch to using more standard gpiod_get_optional() and
> gpiod_set_consumer_name() to keep the nice "PHY reset" label.
> 
> Also there is no reason to only try to fetch the reset GPIO when we have
> OF node, gpiolib can fetch GPIO data from firmwares as well.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2] net: mdio: switch to using gpiod_get_optional()

[Andy Shevchenko]

On Fri, Sep 13, 2019 at 03:55:47PM -0700, Dmitry Torokhov wrote:
> The MDIO device reset line is optional and now that gpiod_get_optional()
> returns proper value when GPIO support is compiled out, there is no
> reason to use fwnode_get_named_gpiod() that I plan to hide away.
> 
> Let's switch to using more standard gpiod_get_optional() and
> gpiod_set_consumer_name() to keep the nice "PHY reset" label.
> 
> Also there is no reason to only try to fetch the reset GPIO when we have
> OF node, gpiolib can fetch GPIO data from firmwares as well.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

But see comment below.

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> 
> Note this is an update to a patch titled "[PATCH 05/11] net: mdio:
> switch to using fwnode_gpiod_get_index()" that no longer uses the new
> proposed API and instead works with already existing ones.
> 
>  drivers/net/phy/mdio_bus.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index ce940871331e..2e29ab841b4d 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -42,21 +42,17 @@
>  
>  static int mdiobus_register_gpiod(struct mdio_device *mdiodev)
>  {
> -	struct gpio_desc *gpiod = NULL;
> +	int error;
>  
>  	/* Deassert the optional reset signal */
> -	if (mdiodev->dev.of_node)
> -		gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode,
> -					       "reset-gpios", 0, GPIOD_OUT_LOW,
> -					       "PHY reset");
> -	if (IS_ERR(gpiod)) {
> -		if (PTR_ERR(gpiod) == -ENOENT || PTR_ERR(gpiod) == -ENOSYS)
> -			gpiod = NULL;
> -		else
> -			return PTR_ERR(gpiod);
> -	}
> -
> -	mdiodev->reset_gpio = gpiod;
> +	mdiodev->reset_gpio = gpiod_get_optional(&mdiodev->dev,
> +						 "reset", GPIOD_OUT_LOW);
> +	error = PTR_ERR_OR_ZERO(mdiodev->reset_gpio);
> +	if (error)
> +		return error;
> +

> +	if (mdiodev->reset_gpio)

This is redundant check.

> +		gpiod_set_consumer_name(mdiodev->reset_gpio, "PHY reset");

>  	return 0;
>  }
> -- 
> 2.23.0.237.gc6a4ce50a0-goog
> 
> 
> -- 
> Dmitry

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] net: mdio: switch to using gpiod_get_optional()

[Dmitry Torokhov]

On Sat, Sep 14, 2019 at 08:09:33PM +0300, Andy Shevchenko wrote:
> On Fri, Sep 13, 2019 at 03:55:47PM -0700, Dmitry Torokhov wrote:
> > The MDIO device reset line is optional and now that gpiod_get_optional()
> > returns proper value when GPIO support is compiled out, there is no
> > reason to use fwnode_get_named_gpiod() that I plan to hide away.
> > 
> > Let's switch to using more standard gpiod_get_optional() and
> > gpiod_set_consumer_name() to keep the nice "PHY reset" label.
> > 
> > Also there is no reason to only try to fetch the reset GPIO when we have
> > OF node, gpiolib can fetch GPIO data from firmwares as well.
> > 
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks Andy.

> 
> But see comment below.
> 

> > +	mdiodev->reset_gpio = gpiod_get_optional(&mdiodev->dev,
> > +						 "reset", GPIOD_OUT_LOW);
> > +	error = PTR_ERR_OR_ZERO(mdiodev->reset_gpio);
> > +	if (error)
> > +		return error;
> > +
> 
> > +	if (mdiodev->reset_gpio)
> 
> This is redundant check.

I see that gpiod_* API handle NULL desc and usually return immediately,
but frankly I am not that comfortable with it. I'm OK with functions
that free/destroy objects that recognize NULL resources, but it is
unusual for other types of APIs.

Thanks.

-- 
Dmitry

Re: [PATCH v2] net: mdio: switch to using gpiod_get_optional()

[Andy Shevchenko]

On Sun, Sep 15, 2019 at 9:26 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Sat, Sep 14, 2019 at 08:09:33PM +0300, Andy Shevchenko wrote:
> > On Fri, Sep 13, 2019 at 03:55:47PM -0700, Dmitry Torokhov wrote:

> > > +   mdiodev->reset_gpio = gpiod_get_optional(&mdiodev->dev,
> > > +                                            "reset", GPIOD_OUT_LOW);
> > > +   error = PTR_ERR_OR_ZERO(mdiodev->reset_gpio);
> > > +   if (error)
> > > +           return error;

> > > +   if (mdiodev->reset_gpio)
> >
> > This is redundant check.
>
> I see that gpiod_* API handle NULL desc and usually return immediately,
> but frankly I am not that comfortable with it. I'm OK with functions
> that free/destroy objects that recognize NULL resources,

> but it is
> unusual for other types of APIs.

CLK, reset, ... frameworks do check for NULL pointer since they
provide an *_optional() getters. So, it's not unusual.
--
With Best Regards,
Andy Shevchenko