Hi!

quoted

+ * Contact: Tuukka Toivonen
+ *          Sakari Ailus

Could you put the e-mail addresses back, please?

Tuukka's e-mail is tuukkat76 at gmail.com .

Ok.

quoted

+#include <linux/module.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/regulator/consumer.h>

Alphabetical order would be nice. The same below.

I was afraid you'd ask. Ok.

quoted

+/**
+ * @brief I2C write using i2c_transfer().
+ * @param coil - the driver data structure
+ * @param data - register value to be written

This does not look entirely right. But you could just remove the entire
comment. It's useless.

Ok.

quoted

+	int ret = 0;
+
+	/*
+	 * Go to standby first as real power off my be denied by the hardware
+	 * (single power line control for both coil and sensor).
+	 */
+	if (standby) {
+		coil->standby = 1;
+		ret = ad5820_update_hw(coil);
+	}
+
+	ret |= regulator_disable(coil->vana);

This is actually an error code and you shouldn't use or to combine two error
codes. The result will make no sense.

It might be the driver did this in the past but it should not be done. The
right thing, as elsewhere, is to assign the value to ret and check it. The
assigment in declaration may be dropped as well.

Yeah, its broken. Let me fix it. 

I think this happens in a few places in the driver.

Actually this was the only place left.

quoted

+	struct ad5820_device *coil = to_ad5820_device(subdev);
+	struct i2c_client *client = v4l2_get_subdevdata(subdev);
+
+	coil->vana = regulator_get(&client->dev, "VANA");

Is there a reason not to acquire this in probe instead?

Yeah, new version will do that (already done, Dmitry was faster).

quoted

+	if (IS_ERR(coil->vana)) {
+		dev_err(&client->dev, "could not get regulator for vana\n");
+		return -ENODEV;

I wonder if -EPROBE_DEFER might be the right error code here.

..and should handle PROBE_DEFER, too.

quoted

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

No need to assign ret here.

Ok.

quoted

+
+	coil = kzalloc(sizeof(*coil), GFP_KERNEL);

You could use devm_kzalloc() here and drop kfree() below and in _remove().

The driver might be actually older than the devm_*() functions. Not sure. At
least they were not widely used back then. :-)

Already done, Dmitry was faster.

quoted

+static int __exit ad5820_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
+	struct ad5820_device *coil = to_ad5820_device(subdev);
+
+	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);

mutex_destroy(&coil->power_lock);

Here. And in probe() error paths as well.

Ok. Can do, altrough it is pretty much a NOP in the error paths.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html