Messages in this thread | | | Date | Tue, 20 Jun 2023 10:57:45 +0200 | From | Johan Hovold <> | Subject | Re: [PATCH] gnss: Use devm_regulator_get_enable_optional() |
| |
On Sat, Jun 17, 2023 at 11:57:09AM +0200, Christophe JAILLET wrote: > Use devm_regulator_get_enable_optional() instead of hand writing it. It > saves some line of code. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > Note that regulator_disable() is now called after gnss_serial_free() in > the error handling path of the probe and in the remove function, but it > looks harmless to me.
Yeah, that bit should be fine.
> --- > drivers/gnss/mtk.c | 26 ++++---------------------- > 1 file changed, 4 insertions(+), 22 deletions(-) > > diff --git a/drivers/gnss/mtk.c b/drivers/gnss/mtk.c > index c62b1211f4fe..d3d31295d4e0 100644 > --- a/drivers/gnss/mtk.c > +++ b/drivers/gnss/mtk.c > @@ -17,7 +17,6 @@ > #include "serial.h" > > struct mtk_data { > - struct regulator *vbackup; > struct regulator *vcc; > }; > > @@ -87,30 +86,16 @@ static int mtk_probe(struct serdev_device *serdev) > goto err_free_gserial; > } > > - data->vbackup = devm_regulator_get_optional(&serdev->dev, "vbackup"); > - if (IS_ERR(data->vbackup)) { > - ret = PTR_ERR(data->vbackup); > - if (ret == -ENODEV) > - data->vbackup = NULL; > - else > - goto err_free_gserial; > - } > - > - if (data->vbackup) { > - ret = regulator_enable(data->vbackup); > - if (ret) > - goto err_free_gserial; > - } > + ret = devm_regulator_get_enable_optional(&serdev->dev, "vbackup"); > + if (ret) > + goto err_free_gserial;
But this breaks the driver as the new helper still returns -ENODEV when the optional is resource is not present.
Wolfram already suggested using this new helper here:
https://lore.kernel.org/lkml/20230523064310.3005-3-wsa+renesas@sang-engineering.com
and also got the error handling right even if that patch will require a respin for other reasons.
As I mentioned in my reply to Wolfram, I'm generally sceptical of helpers like this one, but in this case where there are no dependencies on other resources I guess it's ok.
Johan
| |