v_bckp shall always be enabled as long as the device exists. We now have
a regulator helper for that, use it.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/gnss/ubx.c | 26 ++++----------------------
 1 file changed, 4 insertions(+), 22 deletions(-)

diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c
index c01e1e875727..c8d027973b85 100644
--- a/drivers/gnss/ubx.c
+++ b/drivers/gnss/ubx.c
@@ -17,7 +17,6 @@
 #include "serial.h"
 
 struct ubx_data {
-	struct regulator *v_bckp;
 	struct regulator *vcc;
 };
 
@@ -106,30 +105,16 @@ static int ubx_probe(struct serdev_device *serdev)
 		goto err_free_gserial;
 	}
 
-	data->v_bckp = devm_regulator_get_optional(&serdev->dev, info->v_bckp_name);
-	if (IS_ERR(data->v_bckp)) {
-		ret = PTR_ERR(data->v_bckp);
-		if (ret == -ENODEV)
-			data->v_bckp = NULL;
-		else
-			goto err_free_gserial;
-	}
-
-	if (data->v_bckp) {
-		ret = regulator_enable(data->v_bckp);
-		if (ret)
-			goto err_free_gserial;
-	}
+	ret = devm_regulator_get_enable_optional(&serdev->dev, info->v_bckp_name);
+	if (ret < 0 && ret != -ENODEV)
+		goto err_free_gserial;
 
 	ret = gnss_serial_register(gserial);
 	if (ret)
-		goto err_disable_v_bckp;
+		goto err_free_gserial;
 
 	return 0;
 
-err_disable_v_bckp:
-	if (data->v_bckp)
-		regulator_disable(data->v_bckp);
 err_free_gserial:
 	gnss_serial_free(gserial);
 
@@ -139,11 +124,8 @@ static int ubx_probe(struct serdev_device *serdev)
 static void ubx_remove(struct serdev_device *serdev)
 {
 	struct gnss_serial *gserial = serdev_device_get_drvdata(serdev);
-	struct ubx_data *data = gnss_serial_get_drvdata(gserial);
 
 	gnss_serial_deregister(gserial);
-	if (data->v_bckp)
-		regulator_disable(data->v_bckp);
 	gnss_serial_free(gserial);
 }
 
-- 
2.35.1

On Tue, May 23, 2023 at 08:43:07AM +0200, Wolfram Sang wrote:
> v_bckp shall always be enabled as long as the device exists. We now have
> a regulator helper for that, use it.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/gnss/ubx.c | 26 ++++----------------------
>  1 file changed, 4 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c
> index c01e1e875727..c8d027973b85 100644
> --- a/drivers/gnss/ubx.c
> +++ b/drivers/gnss/ubx.c
> @@ -17,7 +17,6 @@
>  #include "serial.h"
>  
>  struct ubx_data {
> -	struct regulator *v_bckp;
>  	struct regulator *vcc;
>  };
>  
> @@ -106,30 +105,16 @@ static int ubx_probe(struct serdev_device *serdev)
>  		goto err_free_gserial;
>  	}
>  
> -	data->v_bckp = devm_regulator_get_optional(&serdev->dev, info->v_bckp_name);
> -	if (IS_ERR(data->v_bckp)) {
> -		ret = PTR_ERR(data->v_bckp);
> -		if (ret == -ENODEV)
> -			data->v_bckp = NULL;
> -		else
> -			goto err_free_gserial;
> -	}
> -
> -	if (data->v_bckp) {
> -		ret = regulator_enable(data->v_bckp);
> -		if (ret)
> -			goto err_free_gserial;
> -	}
> +	ret = devm_regulator_get_enable_optional(&serdev->dev, info->v_bckp_name);
> +	if (ret < 0 && ret != -ENODEV)
> +		goto err_free_gserial;

I'm a bit torn about this one as I'm generally sceptical of devres and
especially helpers that enable or register resources, which just tends to
lead to subtle bugs.

But if there's one place where this new helper, which importantly does
not return a pointer to the regulator, may be useful I guess it's here.

Care to respin this one after dropping the merge patch?

Johan

> I'm a bit torn about this one as I'm generally sceptical of devres and
> especially helpers that enable or register resources, which just tends to
> lead to subtle bugs.

It is good to think twice with devres, but I also really like this
helper. En-/Disabling the regulator matches the life cycle of the device
itself. The boilerplate code it removes tends also to be error prone.

> Care to respin this one after dropping the merge patch?

Sure, I'll respin this series right away

On Tue, Jun 20, 2023 at 11:04:27AM +0200, Wolfram Sang wrote:
> > I'm a bit torn about this one as I'm generally sceptical of devres and
> > especially helpers that enable or register resources, which just tends to
> > lead to subtle bugs.
> 
> It is good to think twice with devres, but I also really like this
> helper. En-/Disabling the regulator matches the life cycle of the device
> itself. The boilerplate code it removes tends also to be error prone.

So can the "trival" devres conversions be:

	https://lore.kernel.org/lkml/ZJFqCQ8bbBoX3l1g@hovoldconsulting.com

I meant to CC you on my reply there.

Johan