| Message ID | 20230523064310.3005-3-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | gnss: updates to support the Renesas KingFisher board | expand |
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
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); }
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(-)