On 24.05.2016 12:04, Pavel Machek wrote:

Hi!

quoted

quoted

+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");

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:

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.

quoted

quoted

+#define AD5820_RAMP_MODE_LINEAR		(0 << 3)
+#define AD5820_RAMP_MODE_64_16		(1 << 3)
+
+struct ad5820_platform_data {
+	int (*set_xshutdown)(struct v4l2_subdev *subdev, int set);
+};
+
+#define to_ad5820_device(sd)	container_of(sd, struct ad5820_device, subdev)
+
+struct ad5820_device {
+	struct v4l2_subdev subdev;
+	struct ad5820_platform_data *platform_data;
+	struct regulator *vana;
+
+	struct v4l2_ctrl_handler ctrls;
+	u32 focus_absolute;
+	u32 focus_ramp_time;
+	u32 focus_ramp_mode;
+
+	struct mutex power_lock;
+	int power_count;
+
+	int standby : 1;
+};
+

The same for struct ad5820_device, is it really part of the public API?

Let me check what can be done with it.
									Pavel