Re: [PATCHv3] support for AD5820 camera auto-focus coil

[Ivaylo Dimitrov]

On 24.05.2016 23:20, Pavel Machek wrote:
Hi!

devm_regulator_get()?

I'd rather avoid devm_ here. Driver is simple enough to allow it.


Now thinking about it, what would happen here if regulator_get() returns
-EPROBE_DEFER? Wouldn't it be better to move regulator_get to the probe()
function, something like:

Ok, I can do it.

Oh, and don't try to complain about newlines before returns. It looks
better this way.


All I complain about is missing empty line before a final return at the 
end of a function :). I guess it is a matter of preference though.

static int ad5820_probe(struct i2c_client *client,
                        const struct i2c_device_id *devid)
{
        struct ad5820_device *coil;
        int ret = 0;

        coil = devm_kzalloc(sizeof(*coil), GFP_KERNEL);
        if (coil == NULL)
                return -ENOMEM;

        coil->vana = devm_regulator_get(&client->dev, NULL);
        if (IS_ERR(coil->vana)) {
                ret = PTR_ERR(coil->vana);
                if (ret != -EPROBE_DEFER)
                        dev_err(&client->dev, "could not get regulator for 
vana\n");
                return ret;
        }

        mutex_init(&coil->power_lock);
...

with the appropriate changes to remove() because of the devm API
                        usage.

Something like this?


Didn't try to apply it, but yes, looks right. Besides the missing
mutex_destroy(&coil->power_lock); pointed out by Sakari.

diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index f956bd3..f871366 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -8,7 +8,7 @@
   * Copyright (C) 2016 Pavel Machek <pa...@ucw.cz>
   *
   * Contact: Tuukka Toivonen
- *          Sakari Ailus
+ *         Sakari Ailus
   *
   * Based on af_d88.c by Texas Instruments.
   *
@@ -263,13 +263,6 @@ static int ad5820_init_controls(struct ad5820_device *coil)
  static int ad5820_registered(struct v4l2_subdev *subdev)
  {
        struct ad5820_device *coil = to_ad5820_device(subdev);
-       struct i2c_client *client = v4l2_get_subdevdata(subdev);
-
-       coil->vana = regulator_get(&client->dev, "VANA");
-       if (IS_ERR(coil->vana)) {
-               dev_err(&client->dev, "could not get regulator for vana\n");
-               return -ENODEV;
-       }

        return ad5820_init_controls(coil);
  }
@@ -367,10 +360,18 @@ static int ad5820_probe(struct i2c_client *client,
        struct ad5820_device *coil;
        int ret = 0;

-       coil = kzalloc(sizeof(*coil), GFP_KERNEL);
+       coil = devm_kzalloc(sizeof(*coil), GFP_KERNEL);
        if (!coil)
                return -ENOMEM;

+       coil->vana = devm_regulator_get(&client->dev, NULL);
+       if (IS_ERR(coil->vana)) {
+               ret = PTR_ERR(coil->vana);
+               if (ret != -EPROBE_DEFER)
+                       dev_err(&client->dev, "could not get regulator for 
vana\n");
+               return ret;
+       }
+       
        mutex_init(&coil->power_lock);

        v4l2_i2c_subdev_init(&coil->subdev, client, &ad5820_ops);
@@ -390,10 +391,6 @@ static int ad5820_probe(struct i2c_client *client,

  cleanup:
        media_entity_cleanup(&coil->subdev.entity);
-
-free:
-       kfree(coil);
-
        return ret;
  }

@@ -405,11 +402,6 @@ static int __exit ad5820_remove(struct i2c_client *client)
        v4l2_device_unregister_subdev(&coil->subdev);
        v4l2_ctrl_handler_free(&coil->ctrls);
        media_entity_cleanup(&coil->subdev.entity);
-       if (coil->vana)
-               regulator_put(coil->vana);
-
-       kfree(coil);
-
        return 0;
  }