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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxx
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
