support for AD5820 camera auto-focus coil

drivers/media/i2c/ad5820.c new file +14 -0

+static int ad5820_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct ad5820_device *coil =
+		container_of(ctrl->handler, struct ad5820_device, ctrls);
+	u32 code;
+	int r = 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_FOCUS_ABSOLUTE:
+		coil->focus_absolute = ctrl->val;
+		return ad5820_update_hw(coil);
+
+	case V4L2_CID_FOCUS_AD5820_RAMP_TIME:
+		code = RAMP_US_TO_CODE(ctrl->val);

drivers/media/i2c/ad5820.c new file +15 -0

+static int ad5820_probe(struct i2c_client *client,
+			const struct i2c_device_id *devid)
+{
+	struct ad5820_device *coil;
+	int ret = 0;
+
+	coil = kzalloc(sizeof(*coil), GFP_KERNEL);
+	if (coil == NULL)
+		return -ENOMEM;
+
+	mutex_init(&coil->power_lock);
+
+	v4l2_i2c_subdev_init(&coil->subdev, client, &ad5820_ops);
+	coil->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	coil->subdev.internal_ops = &ad5820_internal_ops;

drivers/media/i2c/ad5820.c new file +10 -0

+static int __init ad5820_init(void)
+{
+	int rval;
+
+	rval = i2c_add_driver(&ad5820_i2c_driver);
+	if (rval)
+		printk(KERN_INFO "%s: failed registering " AD5820_NAME "\n",
+		       __func__);
+
+	return rval;

Use module_i2c_driver() instead.

This adds support for AD5820 autofocus coil, found for example in Nokia N900 smartphone.

Signed-off-by Pavel Machek <[email protected]>

---
v2: simple cleanups, fix error paths, simplify probe

I think it is ready. Please apply :-).

drivers/media/i2c/Kconfig +7 -0

index 993dc50..77313a1 100644
@@ -279,6 +279,13 @@ config VIDEO_ML86V7667
  	  To compile this driver as a module, choose M here: the
  	  module will be called ml86v7667.
 
+config VIDEO_AD5820
+	tristate "AD5820 lens voice coil support"
+	depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
+	---help---
+	  This is a driver for the AD5820 camera lens voice coil.
+	  It is used for example in Nokia N900 (RX-51).
+
  config VIDEO_SAA7110
  	tristate "Philips SAA7110 video decoder"
  	depends on VIDEO_V4L2 && I2C

drivers/media/i2c/Makefile +1 -0

index 94f2c99..34434ae 100644
@@ -19,6 +20,7 @@ obj-$(CONFIG_VIDEO_SAA717X) += saa717x.o
  obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
  obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
  obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
+obj-$(CONFIG_VIDEO_AD5820)  += ad5820.o
  obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
  obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
  obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o

drivers/media/i2c/ad5820.c new file +19 -0

new file mode 100644
index 0000000..7725829
@@ -0,0 +1,416 @@
+/*
+ * drivers/media/i2c/ad5820.c
+ *
+ * AD5820 DAC driver for camera voice coil focus.
+ *
+ * Copyright (C) 2008 Nokia Corporation
+ * Copyright (C) 2007 Texas Instruments
+ * Copyright (C) 2016 Pavel Machek <[email protected]>
+ *
+ * Contact: Tuukka Toivonen
+ *          Sakari Ailus
+ *
+ * Based on af_d88.c by Texas Instruments.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but

drivers/media/i2c/ad5820.c new file +71 -0

+#include <media/v4l2-device.h>
+
+#define CODE_TO_RAMP_US(s)	((s) == 0 ? 0 : (1 << ((s) - 1)) * 50)
+#define RAMP_US_TO_CODE(c)	fls(((c) + ((c)>>1)) / 50)
+
+/**
+ * @brief I2C write using i2c_transfer().
+ * @param coil - the driver data structure
+ * @param data - register value to be written
+ * @returns nonnegative on success, negative if failed
+ */
+static int ad5820_write(struct ad5820_device *coil, u16 data)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&coil->subdev);
+	struct i2c_msg msg;
+	int r;
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	data = cpu_to_be16(data);
+	msg.addr  = client->addr;
+	msg.flags = 0;
+	msg.len   = 2;
+	msg.buf   = (u8 *)&data;
+
+	r = i2c_transfer(client->adapter, &msg, 1);
+	if (r < 0) {
+		dev_err(&client->dev, "write failed, error %d\n", r);
+		return r;
+	}
+
+	return 0;
+}
+
+/*
+ * Calculate status word and write it to the device based on current
+ * values of V4L2 controls. It is assumed that the stored V4L2 control
+ * values are properly limited and rounded.
+ */
+static int ad5820_update_hw(struct ad5820_device *coil)
+{
+	u16 status;
+
+	status = RAMP_US_TO_CODE(coil->focus_ramp_time);
+	status |= coil->focus_ramp_mode
+		? AD5820_RAMP_MODE_64_16 : AD5820_RAMP_MODE_LINEAR;
+	status |= coil->focus_absolute << AD5820_DAC_SHIFT;
+
+	if (coil->standby)
+		status |= AD5820_POWER_DOWN;
+
+	return ad5820_write(coil, status);
+}
+
+/*
+ * Power handling
+ */
+static int ad5820_power_off(struct ad5820_device *coil, int standby)
+{
+	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);
+	}
+

drivers/media/i2c/ad5820.c new file +43 -0

+	ret = regulator_enable(coil->vana);
+	if (ret < 0)
+		return ret;
+
+	if (restore) {
+		/* Restore the hardware settings. */
+		coil->standby = 0;
+		ret = ad5820_update_hw(coil);
+		if (ret)
+			goto fail;
+	}
+	return 0;
+
+fail:
+	coil->standby = 1;
+	regulator_disable(coil->vana);
+
+	return ret;
+}
+
+/*
+ * V4L2 controls
+ */
+static int ad5820_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct ad5820_device *coil =
+		container_of(ctrl->handler, struct ad5820_device, ctrls);
+	u32 code;
+
+	switch (ctrl->id) {
+	case V4L2_CID_FOCUS_ABSOLUTE:
+		coil->focus_absolute = ctrl->val;
+		return ad5820_update_hw(coil);
+
+	case V4L2_CID_FOCUS_AD5820_RAMP_TIME:
+		code = RAMP_US_TO_CODE(ctrl->val);
+		ctrl->val = CODE_TO_RAMP_US(code);
+		coil->focus_ramp_time = ctrl->val;
+		break;
+
+	case V4L2_CID_FOCUS_AD5820_RAMP_MODE:
+		coil->focus_ramp_mode = ctrl->val;
+		break;

drivers/media/i2c/ad5820.c new file +56 -0

+	"Linear ramp",
+	"64/16 ramp",
+};
+
+static const struct v4l2_ctrl_config ad5820_ctrls[] = {
+	{
+		.ops		= &ad5820_ctrl_ops,
+		.id		= V4L2_CID_FOCUS_AD5820_RAMP_TIME,
+		.type		= V4L2_CTRL_TYPE_INTEGER,
+		.name		= "Focus ramping time [us]",
+		.min		= 0,
+		.max		= 3200,
+		.step		= 50,
+		.def		= 0,
+		.flags		= 0,
+	},
+	{
+		.ops		= &ad5820_ctrl_ops,
+		.id		= V4L2_CID_FOCUS_AD5820_RAMP_MODE,
+		.type		= V4L2_CTRL_TYPE_MENU,
+		.name		= "Focus ramping mode",
+		.min		= 0,
+		.max		= ARRAY_SIZE(ad5820_focus_menu),
+		.step		= 0,
+		.def		= 0,
+		.flags		= 0,
+		.qmenu		= ad5820_focus_menu,
+	},
+};
+
+
+static int ad5820_init_controls(struct ad5820_device *coil)
+{
+	unsigned int i;
+
+	v4l2_ctrl_handler_init(&coil->ctrls, ARRAY_SIZE(ad5820_ctrls) + 1);
+
+	/*
+	 * V4L2_CID_FOCUS_ABSOLUTE
+	 *
+	 * Minimum current is 0 mA, maximum is 100 mA. Thus, 1 code is
+	 * equivalent to 100/1023 = 0.0978 mA. Nevertheless, we do not use [mA]
+	 * for focus position, because it is meaningless for user. Meaningful
+	 * would be to use focus distance or even its inverse, but since the
+	 * driver doesn't have sufficiently knowledge to do the conversion, we
+	 * will just use abstract codes here. In any case, smaller value = focus
+	 * position farther from camera. The default zero value means focus at
+	 * infinity, and also least current consumption.
+	 */
+	v4l2_ctrl_new_std(&coil->ctrls, &ad5820_ctrl_ops,
+			  V4L2_CID_FOCUS_ABSOLUTE, 0, 1023, 1, 0);
+
+	/* V4L2_CID_TEST_PATTERN and V4L2_CID_MODE_* */
+	for (i = 0; i < ARRAY_SIZE(ad5820_ctrls); ++i)
+		v4l2_ctrl_new_custom(&coil->ctrls, &ad5820_ctrls[i], NULL);
+

drivers/media/i2c/ad5820.c new file +103 -0

+
+/*
+ * V4L2 subdev operations
+ */
+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);
+}
+
+static int
+ad5820_set_power(struct v4l2_subdev *subdev, int on)
+{
+	struct ad5820_device *coil = to_ad5820_device(subdev);
+	int ret = 0;
+
+	mutex_lock(&coil->power_lock);
+
+	/*
+	 * If the power count is modified from 0 to != 0 or from != 0 to 0,
+	 * update the power state.
+	 */
+	if (coil->power_count == !on) {
+		ret = on ? ad5820_power_on(coil, 1) : ad5820_power_off(coil, 1);
+		if (ret < 0)
+			goto done;
+	}
+
+	/* Update the power count. */
+	coil->power_count += on ? 1 : -1;
+	WARN_ON(coil->power_count < 0);
+
+done:
+	mutex_unlock(&coil->power_lock);
+	return ret;
+}
+
+static int ad5820_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	return ad5820_set_power(sd, 1);
+}
+
+static int ad5820_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	return ad5820_set_power(sd, 0);
+}
+
+static const struct v4l2_subdev_core_ops ad5820_core_ops = {
+	.s_power = ad5820_set_power,
+};
+
+static const struct v4l2_subdev_ops ad5820_ops = {
+	.core = &ad5820_core_ops,
+};
+
+static const struct v4l2_subdev_internal_ops ad5820_internal_ops = {
+	.registered = ad5820_registered,
+	.open = ad5820_open,
+	.close = ad5820_close,
+};
+
+/*
+ * I2C driver
+ */
+#ifdef CONFIG_PM
+
+static int ad5820_suspend(struct device *dev)
+{
+	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
+	struct ad5820_device *coil = to_ad5820_device(subdev);
+
+	if (!coil->power_count)
+		return 0;
+
+	return ad5820_power_off(coil, 0);
+}
+
+static int ad5820_resume(struct device *dev)
+{
+	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
+	struct ad5820_device *coil = to_ad5820_device(subdev);
+
+	if (!coil->power_count)
+		return 0;
+
+	return ad5820_power_on(coil, 1);
+}
+
+#else
+
+#define ad5820_suspend	NULL
+#define ad5820_resume	NULL
+

drivers/media/i2c/ad5820.c new file +14 -0

+		return -ENOMEM;
+
+	mutex_init(&coil->power_lock);
+
+	v4l2_i2c_subdev_init(&coil->subdev, client, &ad5820_ops);
+	coil->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	coil->subdev.internal_ops = &ad5820_internal_ops;
+	strcpy(coil->subdev.name, "ad5820 focus");
+
+	ret = media_entity_pads_init(&coil->subdev.entity, 0, NULL);
+	if (ret < 0)
+		goto free;
+
+	ret = v4l2_async_register_subdev(&coil->subdev);

drivers/media/i2c/ad5820.c new file +16 -0

+	return 0;
+}
+
+static const struct i2c_device_id ad5820_id_table[] = {
+	{ AD5820_NAME, 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
+
+static SIMPLE_DEV_PM_OPS(ad5820_pm, ad5820_suspend, ad5820_resume);
+
+static struct i2c_driver ad5820_i2c_driver = {
+	.driver		= {
+		.name	= AD5820_NAME,
+		.pm	= &ad5820_pm,
+	},

This adds support for AD5820 autofocus coil, found for example in Nokia N900 smartphone.

Signed-off-by Pavel Machek <[email protected]>

---
v2: simple cleanups, fix error paths, simplify probe

v3: more cleanups, remove printk, add include

drivers/media/i2c/Kconfig +7 -0

index 993dc50..77313a1 100644
@@ -279,6 +279,13 @@ config VIDEO_ML86V7667
  	  To compile this driver as a module, choose M here: the
  	  module will be called ml86v7667.
 
+config VIDEO_AD5820
+	tristate "AD5820 lens voice coil support"
+	depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
+	---help---
+	  This is a driver for the AD5820 camera lens voice coil.
+	  It is used for example in Nokia N900 (RX-51).
+
  config VIDEO_SAA7110
  	tristate "Philips SAA7110 video decoder"
  	depends on VIDEO_V4L2 && I2C

drivers/media/i2c/Makefile +1 -0

index 94f2c99..34434ae 100644
@@ -19,6 +20,7 @@ obj-$(CONFIG_VIDEO_SAA717X) += saa717x.o
  obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
  obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
  obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
+obj-$(CONFIG_VIDEO_AD5820)  += ad5820.o
  obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
  obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
  obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o

drivers/media/i2c/ad5820.c new file +19 -0

new file mode 100644
index 0000000..7725829
@@ -0,0 +1,416 @@
+/*
+ * drivers/media/i2c/ad5820.c
+ *
+ * AD5820 DAC driver for camera voice coil focus.
+ *
+ * Copyright (C) 2008 Nokia Corporation
+ * Copyright (C) 2007 Texas Instruments
+ * Copyright (C) 2016 Pavel Machek <[email protected]>
+ *
+ * Contact: Tuukka Toivonen
+ *          Sakari Ailus
+ *
+ * Based on af_d88.c by Texas Instruments.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but

drivers/media/i2c/ad5820.c new file +205 -0

+#include <linux/bitops.h>
+#include <linux/kernel.h>
+#include <linux/regulator/consumer.h>
+
+#include <media/ad5820.h>
+#include <media/v4l2-device.h>
+
+#define CODE_TO_RAMP_US(s)	((s) == 0 ? 0 : (1 << ((s) - 1)) * 50)
+#define RAMP_US_TO_CODE(c)	fls(((c) + ((c)>>1)) / 50)
+
+/**
+ * @brief I2C write using i2c_transfer().
+ * @param coil - the driver data structure
+ * @param data - register value to be written
+ * @returns nonnegative on success, negative if failed
+ */
+static int ad5820_write(struct ad5820_device *coil, u16 data)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&coil->subdev);
+	struct i2c_msg msg;
+	int r;
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	data = cpu_to_be16(data);
+	msg.addr  = client->addr;
+	msg.flags = 0;
+	msg.len   = 2;
+	msg.buf   = (u8 *)&data;
+
+	r = i2c_transfer(client->adapter, &msg, 1);
+	if (r < 0) {
+		dev_err(&client->dev, "write failed, error %d\n", r);
+		return r;
+	}
+
+	return 0;
+}
+
+/*
+ * Calculate status word and write it to the device based on current
+ * values of V4L2 controls. It is assumed that the stored V4L2 control
+ * values are properly limited and rounded.
+ */
+static int ad5820_update_hw(struct ad5820_device *coil)
+{
+	u16 status;
+
+	status = RAMP_US_TO_CODE(coil->focus_ramp_time);
+	status |= coil->focus_ramp_mode
+		? AD5820_RAMP_MODE_64_16 : AD5820_RAMP_MODE_LINEAR;
+	status |= coil->focus_absolute << AD5820_DAC_SHIFT;
+
+	if (coil->standby)
+		status |= AD5820_POWER_DOWN;
+
+	return ad5820_write(coil, status);
+}
+
+/*
+ * Power handling
+ */
+static int ad5820_power_off(struct ad5820_device *coil, int standby)
+{
+	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);
+
+	return ret;
+}
+
+static int ad5820_power_on(struct ad5820_device *coil, int restore)
+{
+	int ret;
+
+	printk("ad5820_power_on: 1\n");
+	ret = regulator_enable(coil->vana);
+	if (ret < 0)
+		return ret;
+
+	if (restore) {
+		/* Restore the hardware settings. */
+		coil->standby = 0;
+		ret = ad5820_update_hw(coil);
+		if (ret)
+			goto fail;
+	}
+	return 0;
+
+fail:
+	coil->standby = 1;
+	regulator_disable(coil->vana);
+
+	return ret;
+}
+
+/*
+ * V4L2 controls
+ */
+static int ad5820_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct ad5820_device *coil =
+		container_of(ctrl->handler, struct ad5820_device, ctrls);
+	u32 code;
+
+	switch (ctrl->id) {
+	case V4L2_CID_FOCUS_ABSOLUTE:
+		coil->focus_absolute = ctrl->val;
+		return ad5820_update_hw(coil);
+
+	case V4L2_CID_FOCUS_AD5820_RAMP_TIME:
+		code = RAMP_US_TO_CODE(ctrl->val);
+		ctrl->val = CODE_TO_RAMP_US(code);
+		coil->focus_ramp_time = ctrl->val;
+		break;
+
+	case V4L2_CID_FOCUS_AD5820_RAMP_MODE:
+		coil->focus_ramp_mode = ctrl->val;
+		break;
+	}
+
+	return 0;
+}
+
+static const struct v4l2_ctrl_ops ad5820_ctrl_ops = {
+	.s_ctrl = ad5820_set_ctrl,
+};
+
+static const char *ad5820_focus_menu[] = {
+	"Linear ramp",
+	"64/16 ramp",
+};
+
+static const struct v4l2_ctrl_config ad5820_ctrls[] = {
+	{
+		.ops		= &ad5820_ctrl_ops,
+		.id		= V4L2_CID_FOCUS_AD5820_RAMP_TIME,
+		.type		= V4L2_CTRL_TYPE_INTEGER,
+		.name		= "Focus ramping time [us]",
+		.min		= 0,
+		.max		= 3200,
+		.step		= 50,
+		.def		= 0,
+		.flags		= 0,
+	},
+	{
+		.ops		= &ad5820_ctrl_ops,
+		.id		= V4L2_CID_FOCUS_AD5820_RAMP_MODE,
+		.type		= V4L2_CTRL_TYPE_MENU,
+		.name		= "Focus ramping mode",
+		.min		= 0,
+		.max		= ARRAY_SIZE(ad5820_focus_menu),
+		.step		= 0,
+		.def		= 0,
+		.flags		= 0,
+		.qmenu		= ad5820_focus_menu,
+	},
+};
+
+
+static int ad5820_init_controls(struct ad5820_device *coil)
+{
+	unsigned int i;
+
+	v4l2_ctrl_handler_init(&coil->ctrls, ARRAY_SIZE(ad5820_ctrls) + 1);
+
+	/*
+	 * V4L2_CID_FOCUS_ABSOLUTE
+	 *
+	 * Minimum current is 0 mA, maximum is 100 mA. Thus, 1 code is
+	 * equivalent to 100/1023 = 0.0978 mA. Nevertheless, we do not use [mA]
+	 * for focus position, because it is meaningless for user. Meaningful
+	 * would be to use focus distance or even its inverse, but since the
+	 * driver doesn't have sufficiently knowledge to do the conversion, we
+	 * will just use abstract codes here. In any case, smaller value = focus
+	 * position farther from camera. The default zero value means focus at
+	 * infinity, and also least current consumption.
+	 */
+	v4l2_ctrl_new_std(&coil->ctrls, &ad5820_ctrl_ops,
+			  V4L2_CID_FOCUS_ABSOLUTE, 0, 1023, 1, 0);
+
+	/* V4L2_CID_TEST_PATTERN and V4L2_CID_MODE_* */
+	for (i = 0; i < ARRAY_SIZE(ad5820_ctrls); ++i)
+		v4l2_ctrl_new_custom(&coil->ctrls, &ad5820_ctrls[i], NULL);
+
+	if (coil->ctrls.error)
+		return coil->ctrls.error;
+
+	coil->focus_absolute = 0;
+	coil->focus_ramp_time = 0;
+	coil->focus_ramp_mode = 0;
+
+	coil->subdev.ctrl_handler = &coil->ctrls;
+	return 0;
+}

drivers/media/i2c/ad5820.c new file +92 -0

+	if (IS_ERR(coil->vana)) {
+		dev_err(&client->dev, "could not get regulator for vana\n");
+		return -ENODEV;
+	}
+
+	return ad5820_init_controls(coil);
+}
+
+static int
+ad5820_set_power(struct v4l2_subdev *subdev, int on)
+{
+	struct ad5820_device *coil = to_ad5820_device(subdev);
+	int ret = 0;
+
+	mutex_lock(&coil->power_lock);
+
+	/*
+	 * If the power count is modified from 0 to != 0 or from != 0 to 0,
+	 * update the power state.
+	 */
+	if (coil->power_count == !on) {
+		ret = on ? ad5820_power_on(coil, 1) : ad5820_power_off(coil, 1);
+		if (ret < 0)
+			goto done;
+	}
+
+	/* Update the power count. */
+	coil->power_count += on ? 1 : -1;
+	WARN_ON(coil->power_count < 0);
+
+done:
+	mutex_unlock(&coil->power_lock);
+	return ret;
+}
+
+static int ad5820_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	return ad5820_set_power(sd, 1);
+}
+
+static int ad5820_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	return ad5820_set_power(sd, 0);
+}
+
+static const struct v4l2_subdev_core_ops ad5820_core_ops = {
+	.s_power = ad5820_set_power,
+};
+
+static const struct v4l2_subdev_ops ad5820_ops = {
+	.core = &ad5820_core_ops,
+};
+
+static const struct v4l2_subdev_internal_ops ad5820_internal_ops = {
+	.registered = ad5820_registered,
+	.open = ad5820_open,
+	.close = ad5820_close,
+};
+
+/*
+ * I2C driver
+ */
+#ifdef CONFIG_PM
+
+static int ad5820_suspend(struct device *dev)
+{
+	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
+	struct ad5820_device *coil = to_ad5820_device(subdev);
+
+	if (!coil->power_count)
+		return 0;
+
+	return ad5820_power_off(coil, 0);
+}
+
+static int ad5820_resume(struct device *dev)
+{
+	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
+	struct ad5820_device *coil = to_ad5820_device(subdev);
+
+	if (!coil->power_count)
+		return 0;
+
+	return ad5820_power_on(coil, 1);
+}
+
+#else
+
+#define ad5820_suspend	NULL
+#define ad5820_resume	NULL

drivers/media/i2c/ad5820.c new file +64 -0

+	if (coil == NULL)
+		return -ENOMEM;
+
+	mutex_init(&coil->power_lock);
+
+	v4l2_i2c_subdev_init(&coil->subdev, client, &ad5820_ops);
+	coil->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	coil->subdev.internal_ops = &ad5820_internal_ops;
+	strcpy(coil->subdev.name, "ad5820 focus");
+
+	ret = media_entity_pads_init(&coil->subdev.entity, 0, NULL);
+	if (ret < 0)
+		goto free;
+
+	ret = v4l2_async_register_subdev(&coil->subdev);
+	if (ret < 0)
+		goto cleanup;
+
+	return ret;
+cleanup:
+	media_entity_cleanup(&coil->subdev.entity);
+free:
+	kfree(coil);
+	return ret;
+}
+
+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);
+
+	kfree(coil);
+	return 0;
+}
+
+static const struct i2c_device_id ad5820_id_table[] = {
+	{ AD5820_NAME, 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
+
+static SIMPLE_DEV_PM_OPS(ad5820_pm, ad5820_suspend, ad5820_resume);
+
+static struct i2c_driver ad5820_i2c_driver = {
+	.driver		= {
+		.name	= AD5820_NAME,
+		.pm	= &ad5820_pm,
+	},
+	.probe		= ad5820_probe,
+	.remove		= __exit_p(ad5820_remove),
+	.id_table	= ad5820_id_table,
+};
+
+module_i2c_driver(ad5820_i2c_driver);
+
+MODULE_AUTHOR("Tuukka Toivonen");
+MODULE_DESCRIPTION("AD5820 camera lens driver");
+MODULE_LICENSE("GPL");

include/media/ad5820.h new file +34 -0

new file mode 100644
index 0000000..f5a1565
@@ -0,0 +1,70 @@
+/*
+ * include/media/ad5820.h
+ *
+ * Copyright (C) 2008 Nokia Corporation
+ * Copyright (C) 2007 Texas Instruments
+ *
+ * Contact: Tuukka Toivonen <[email protected]>
+ *          Sakari Ailus <[email protected]>
+ *
+ * Based on af_d88.c by Texas Instruments.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#ifndef AD5820_H
+#define AD5820_H
+
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/videodev2.h>
+
+#include <media/v4l2-ctrls.h>

include/media/ad5820.h new file +15 -0

+#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;

include/media/ad5820.h new file +1 -0

+#endif /* AD5820_H */

Quoted 38 lines

include/media/ad5820.h new file +32 -0

new file mode 100644
index 0000000..f5a1565
@@ -0,0 +1,70 @@
+/*
+ * include/media/ad5820.h
+ *
+ * Copyright (C) 2008 Nokia Corporation
+ * Copyright (C) 2007 Texas Instruments
+ *
+ * Contact: Tuukka Toivonen <[email protected]>
+ *          Sakari Ailus <[email protected]>
+ *
+ * Based on af_d88.c by Texas Instruments.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#ifndef AD5820_H
+#define AD5820_H
+
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/videodev2.h>

include/media/ad5820.h line 40

Quoted 8 lines

include/media/ad5820.h new file +8 -0

+
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-subdev.h>
+
+struct regulator;
+
+#define AD5820_NAME		"ad5820"
+#define AD5820_I2C_ADDR		(0x18 >> 1)

include/media/ad5820.h line 44

Quoted 3 lines

include/media/ad5820.h new file +3 -0

+/* Register definitions */
+#define AD5820_POWER_DOWN		(1 << 15)
+#define AD5820_DAC_SHIFT		4

Do those defines really belong here? Isn't it better if they are moved to ad5820.c?

include/media/ad5820.h line 50

Quoted 6 lines

include/media/ad5820.h new file +6 -0

+#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);
+};

Quoted 12 lines

include/media/ad5820.h new file +12 -0

+#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;
+

include/media/ad5820.h line 69

Quoted 6 lines

include/media/ad5820.h new file +6 -0

+	struct mutex power_lock;
+	int power_count;
+
+	int standby : 1;
+};
+

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

Quoted 4 lines

include/media/ad5820.h new file +1 -0

+#endif /* AD5820_H */

drivers/media/i2c/ad5820.c line 250

Quoted 6 lines

drivers/media/i2c/ad5820.c new file +6 -0

+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()?

Quoted 19 lines

include/media/ad5820.h new file +19 -0

+#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;
+

include/media/ad5820.h line 69

Quoted 6 lines

include/media/ad5820.h new file +6 -0

+	struct mutex power_lock;
+	int power_count;
+
+	int standby : 1;
+};
+

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

Hi!

Quoted 8 lines

drivers/media/i2c/ad5820.c line 250

Quoted 6 lines

drivers/media/i2c/ad5820.c new file +6 -0

+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.

Quoted 19 lines

Quoted 19 lines

include/media/ad5820.h new file +19 -0

+#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;
+

Quoted 8 lines

include/media/ad5820.h line 69

Quoted 6 lines

include/media/ad5820.h new file +6 -0

+	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

Quoted 4 lines

Quoted 1 line

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.

Hi!

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

devm_regulator_get()?

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

Quoted 3 lines

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.

Quoted 18 lines

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;
	}

Quoted 5 lines

	mutex_init(&coil->power_lock);

...

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

Something like this?

drivers/media/i2c/ad5820.c +10 -18

index f956bd3..f871366 100644
@@ -8,7 +8,7 @@
   * Copyright (C) 2016 Pavel Machek <[email protected]>
   *
   * 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;
  }
 

This adds support for AD5820 autofocus coil, found for example in Nokia N900 smartphone.

Signed-off-by Pavel Machek <[email protected]>

---
v2: simple cleanups, fix error paths, simplify probe
v3: more cleanups, remove printk, add include
v4: remove header file.

drivers/media/i2c/Kconfig +7 -0

index 993dc50..77313a1 100644
@@ -279,6 +279,13 @@ config VIDEO_ML86V7667
 	  To compile this driver as a module, choose M here: the
 	  module will be called ml86v7667.
 
+config VIDEO_AD5820
+	tristate "AD5820 lens voice coil support"
+	depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
+	---help---
+	  This is a driver for the AD5820 camera lens voice coil.
+	  It is used for example in Nokia N900 (RX-51).
+
 config VIDEO_SAA7110
 	tristate "Philips SAA7110 video decoder"
 	depends on VIDEO_V4L2 && I2C

drivers/media/i2c/Makefile +1 -0

index 94f2c99..34434ae 100644
@@ -19,6 +20,7 @@ obj-$(CONFIG_VIDEO_SAA717X) += saa717x.o
 obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
 obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
 obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
+obj-$(CONFIG_VIDEO_AD5820)  += ad5820.o
 obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
 obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
 obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o

drivers/media/i2c/ad5820.c new file +1 -0

new file mode 100644
index 0000000..f956bd3
@@ -0,0 +1,438 @@
+/*

drivers/media/i2c/ad5820.c new file +8 -0

+ *
+ * Based on af_d88.c by Texas Instruments.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but

drivers/media/i2c/ad5820.c new file +28 -0

+
+#include <media/v4l2-device.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-subdev.h>
+
+#define AD5820_NAME		"ad5820"
+
+/* Register definitions */
+#define AD5820_POWER_DOWN		(1 << 15)
+#define AD5820_DAC_SHIFT		4
+#define AD5820_RAMP_MODE_LINEAR		(0 << 3)
+#define AD5820_RAMP_MODE_64_16		(1 << 3)
+
+#define CODE_TO_RAMP_US(s)	((s) == 0 ? 0 : (1 << ((s) - 1)) * 50)
+#define RAMP_US_TO_CODE(c)	fls(((c) + ((c)>>1)) / 50)
+
+#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;
+

drivers/media/i2c/ad5820.c new file +53 -0

+ * @returns nonnegative on success, negative if failed
+ */
+static int ad5820_write(struct ad5820_device *coil, u16 data)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&coil->subdev);
+	struct i2c_msg msg;
+	int r;
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	data = cpu_to_be16(data);
+	msg.addr  = client->addr;
+	msg.flags = 0;
+	msg.len   = 2;
+	msg.buf   = (u8 *)&data;
+
+	r = i2c_transfer(client->adapter, &msg, 1);
+	if (r < 0) {
+		dev_err(&client->dev, "write failed, error %d\n", r);
+		return r;
+	}
+
+	return 0;
+}
+
+/*
+ * Calculate status word and write it to the device based on current
+ * values of V4L2 controls. It is assumed that the stored V4L2 control
+ * values are properly limited and rounded.
+ */
+static int ad5820_update_hw(struct ad5820_device *coil)
+{
+	u16 status;
+
+	status = RAMP_US_TO_CODE(coil->focus_ramp_time);
+	status |= coil->focus_ramp_mode
+		? AD5820_RAMP_MODE_64_16 : AD5820_RAMP_MODE_LINEAR;
+	status |= coil->focus_absolute << AD5820_DAC_SHIFT;
+
+	if (coil->standby)
+		status |= AD5820_POWER_DOWN;
+
+	return ad5820_write(coil, status);
+}
+
+/*
+ * Power handling
+ */
+static int ad5820_power_off(struct ad5820_device *coil, int standby)
+{
+	int ret = 0;
+

drivers/media/i2c/ad5820.c new file +128 -0

+
+	return ret;
+}
+
+static int ad5820_power_on(struct ad5820_device *coil, int restore)
+{
+	int ret;
+
+	ret = regulator_enable(coil->vana);
+	if (ret < 0)
+		return ret;
+
+	if (restore) {
+		/* Restore the hardware settings. */
+		coil->standby = 0;
+		ret = ad5820_update_hw(coil);
+		if (ret)
+			goto fail;
+	}
+	return 0;
+
+fail:
+	coil->standby = 1;
+	regulator_disable(coil->vana);
+
+	return ret;
+}
+
+/*
+ * V4L2 controls
+ */
+static int ad5820_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct ad5820_device *coil =
+		container_of(ctrl->handler, struct ad5820_device, ctrls);
+	u32 code;
+
+	switch (ctrl->id) {
+	case V4L2_CID_FOCUS_ABSOLUTE:
+		coil->focus_absolute = ctrl->val;
+		return ad5820_update_hw(coil);
+
+	case V4L2_CID_FOCUS_AD5820_RAMP_TIME:
+		code = RAMP_US_TO_CODE(ctrl->val);
+		ctrl->val = CODE_TO_RAMP_US(code);
+		coil->focus_ramp_time = ctrl->val;
+		break;
+
+	case V4L2_CID_FOCUS_AD5820_RAMP_MODE:
+		coil->focus_ramp_mode = ctrl->val;
+		break;
+	}
+
+	return 0;
+}
+
+static const struct v4l2_ctrl_ops ad5820_ctrl_ops = {
+	.s_ctrl = ad5820_set_ctrl,
+};
+
+static const char * const ad5820_focus_menu[] = {
+	"Linear ramp",
+	"64/16 ramp",
+};
+
+static const struct v4l2_ctrl_config ad5820_ctrls[] = {
+	{
+		.ops		= &ad5820_ctrl_ops,
+		.id		= V4L2_CID_FOCUS_AD5820_RAMP_TIME,
+		.type		= V4L2_CTRL_TYPE_INTEGER,
+		.name		= "Focus ramping time [us]",
+		.min		= 0,
+		.max		= 3200,
+		.step		= 50,
+		.def		= 0,
+		.flags		= 0,
+	},
+	{
+		.ops		= &ad5820_ctrl_ops,
+		.id		= V4L2_CID_FOCUS_AD5820_RAMP_MODE,
+		.type		= V4L2_CTRL_TYPE_MENU,
+		.name		= "Focus ramping mode",
+		.min		= 0,
+		.max		= ARRAY_SIZE(ad5820_focus_menu),
+		.step		= 0,
+		.def		= 0,
+		.flags		= 0,
+		.qmenu		= ad5820_focus_menu,
+	},
+};
+
+
+static int ad5820_init_controls(struct ad5820_device *coil)
+{
+	unsigned int i;
+
+	v4l2_ctrl_handler_init(&coil->ctrls, ARRAY_SIZE(ad5820_ctrls) + 1);
+
+	/*
+	 * V4L2_CID_FOCUS_ABSOLUTE
+	 *
+	 * Minimum current is 0 mA, maximum is 100 mA. Thus, 1 code is
+	 * equivalent to 100/1023 = 0.0978 mA. Nevertheless, we do not use [mA]
+	 * for focus position, because it is meaningless for user. Meaningful
+	 * would be to use focus distance or even its inverse, but since the
+	 * driver doesn't have sufficiently knowledge to do the conversion, we
+	 * will just use abstract codes here. In any case, smaller value = focus
+	 * position farther from camera. The default zero value means focus at
+	 * infinity, and also least current consumption.
+	 */
+	v4l2_ctrl_new_std(&coil->ctrls, &ad5820_ctrl_ops,
+			  V4L2_CID_FOCUS_ABSOLUTE, 0, 1023, 1, 0);
+
+	/* V4L2_CID_TEST_PATTERN and V4L2_CID_MODE_* */
+	for (i = 0; i < ARRAY_SIZE(ad5820_ctrls); ++i)
+		v4l2_ctrl_new_custom(&coil->ctrls, &ad5820_ctrls[i], NULL);
+
+	if (coil->ctrls.error)
+		return coil->ctrls.error;
+
+	coil->focus_absolute = 0;
+	coil->focus_ramp_time = 0;
+	coil->focus_ramp_mode = 0;
+
+	coil->subdev.ctrl_handler = &coil->ctrls;
+
+	return 0;
+}

drivers/media/i2c/ad5820.c new file +87 -0

+	}
+
+	return ad5820_init_controls(coil);
+}
+
+static int
+ad5820_set_power(struct v4l2_subdev *subdev, int on)
+{
+	struct ad5820_device *coil = to_ad5820_device(subdev);
+	int ret = 0;
+
+	mutex_lock(&coil->power_lock);
+
+	/*
+	 * If the power count is modified from 0 to != 0 or from != 0 to 0,
+	 * update the power state.
+	 */
+	if (coil->power_count == !on) {
+		ret = on ? ad5820_power_on(coil, 1) : ad5820_power_off(coil, 1);
+		if (ret < 0)
+			goto done;
+	}
+
+	/* Update the power count. */
+	coil->power_count += on ? 1 : -1;
+	WARN_ON(coil->power_count < 0);
+
+done:
+	mutex_unlock(&coil->power_lock);
+	return ret;
+}
+
+static int ad5820_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	return ad5820_set_power(sd, 1);
+}
+
+static int ad5820_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	return ad5820_set_power(sd, 0);
+}
+
+static const struct v4l2_subdev_core_ops ad5820_core_ops = {
+	.s_power = ad5820_set_power,
+};
+
+static const struct v4l2_subdev_ops ad5820_ops = {
+	.core = &ad5820_core_ops,
+};
+
+static const struct v4l2_subdev_internal_ops ad5820_internal_ops = {
+	.registered = ad5820_registered,
+	.open = ad5820_open,
+	.close = ad5820_close,
+};
+
+/*
+ * I2C driver
+ */
+#ifdef CONFIG_PM
+
+static int ad5820_suspend(struct device *dev)
+{
+	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
+	struct ad5820_device *coil = to_ad5820_device(subdev);
+
+	if (!coil->power_count)
+		return 0;
+
+	return ad5820_power_off(coil, 0);
+}
+
+static int ad5820_resume(struct device *dev)
+{
+	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
+	struct ad5820_device *coil = to_ad5820_device(subdev);
+
+	if (!coil->power_count)
+		return 0;
+
+	return ad5820_power_on(coil, 1);
+}
+
+#else
+

drivers/media/i2c/ad5820.c new file +29 -0

+	if (!coil)
+		return -ENOMEM;
+
+	mutex_init(&coil->power_lock);
+
+	v4l2_i2c_subdev_init(&coil->subdev, client, &ad5820_ops);
+	coil->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	coil->subdev.internal_ops = &ad5820_internal_ops;
+	strcpy(coil->subdev.name, "ad5820 focus");
+
+	ret = media_entity_pads_init(&coil->subdev.entity, 0, NULL);
+	if (ret < 0)
+		goto free;
+
+	ret = v4l2_async_register_subdev(&coil->subdev);
+	if (ret < 0)
+		goto cleanup;
+
+	return ret;
+
+cleanup:
+	media_entity_cleanup(&coil->subdev.entity);
+
+free:
+	kfree(coil);
+
+	return ret;
+}
+

drivers/media/i2c/ad5820.c new file +29 -0

+
+	kfree(coil);
+
+	return 0;
+}
+
+static const struct i2c_device_id ad5820_id_table[] = {
+	{ AD5820_NAME, 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
+
+static SIMPLE_DEV_PM_OPS(ad5820_pm, ad5820_suspend, ad5820_resume);
+
+static struct i2c_driver ad5820_i2c_driver = {
+	.driver		= {
+		.name	= AD5820_NAME,
+		.pm	= &ad5820_pm,
+	},
+	.probe		= ad5820_probe,
+	.remove		= __exit_p(ad5820_remove),
+	.id_table	= ad5820_id_table,
+};
+
+module_i2c_driver(ad5820_i2c_driver);
+
+MODULE_AUTHOR("Tuukka Toivonen");
+MODULE_DESCRIPTION("AD5820 camera lens driver");
+MODULE_LICENSE("GPL");

Quoted 2 lines

+ * Contact: Tuukka Toivonen
+ * Sakari Ailus

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

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

drivers/media/i2c/ad5820.c line 29

Quoted 5 lines

drivers/media/i2c/ad5820.c new file +5 -0

+#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.

Quoted 4 lines

+/**
+ * @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.

drivers/media/i2c/ad5820.c line 130

Quoted 12 lines

drivers/media/i2c/ad5820.c new file +12 -0

+	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.

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

drivers/media/i2c/ad5820.c line 268

Quoted 4 lines

drivers/media/i2c/ad5820.c new file +4 -0

+	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?

drivers/media/i2c/ad5820.c line 271

Quoted 3 lines

drivers/media/i2c/ad5820.c new file +3 -0

+	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.

drivers/media/i2c/ad5820.c line 368

Quoted 5 lines

drivers/media/i2c/ad5820.c new file +5 -0

+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.

drivers/media/i2c/ad5820.c line 370

Quoted 2 lines

drivers/media/i2c/ad5820.c new file +2 -0

+
+	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. :-)

drivers/media/i2c/ad5820.c line 409

Quoted 10 lines

drivers/media/i2c/ad5820.c new file +10 -0

+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.

This adds support for AD5820 autofocus coil, found for example in Nokia N900 smartphone.

Signed-off-by Pavel Machek <[email protected]>

Hi Pavel,

Quoted 4 lines · Pavel Machek

This adds support for AD5820 autofocus coil, found for example in Nokia N900 smartphone.

Quoted 1 line

Signed-off-by Pavel Machek <[email protected]>

The patch looks good to me but then I came to think of a question I've missed so far: is there DT documentation somewhere for the properties used by the driver? I might put that to a separate patch, and cc the relevant list.

Quoted 7 lines · Sakari Ailus

Hi Pavel,

Quoted 4 lines · Pavel Machek

This adds support for AD5820 autofocus coil, found for example in Nokia N900 smartphone.

Quoted 6 lines

Quoted 1 line

Signed-off-by Pavel Machek <[email protected]>

The patch looks good to me but then I came to think of a question I've missed so far: is there DT documentation somewhere for the properties used by the driver? I might put that to a separate patch, and cc the relevant list.

Well, it does not use any dt properties. So there's not really much to discuss with dt people...

Maybe "ad5820" needs to go to list of simple i2c drivers somewhere, but...

Hi Pavel,

Quoted 3 lines

Well, it does not use any dt properties. So there's not really much to discuss with dt people...

Quoted 2 lines

Maybe "ad5820" needs to go to list of simple i2c drivers somewhere, but...

It's an I2C device and it does use a regulator. Not a lot, though, these are both quite basic stuff. This should still be documented as the people who write the DT bindings (in general) aren't expected to read driver code as well. That's at least my understanding.

Documentation/devicetree/bindings/media/i2c/ad5820.txt new file +3 -0

@@ -0,0 +1,20 @@
+* Analog Devices AD5820 autofocus coil
+
+Required Properties:

Documentation/devicetree/bindings/media/i2c/ad5820.txt new file +7 -0

+       ad5820: dac at 0c {
+               compatible = "adi,ad5820";
+               reg = <0x0c>;
+
+               VANA-supply = <&vaux4>;
+       };
+

Quoted 19 lines · Pavel Machek

Quoted 5 lines · Sakari Ailus

Hi Pavel,

Quoted 3 lines

Well, it does not use any dt properties. So there's not really much to discuss with dt people...

Quoted 7 lines

Quoted 2 lines

Maybe "ad5820" needs to go to list of simple i2c drivers somewhere, but...

It's an I2C device and it does use a regulator. Not a lot, though, these are both quite basic stuff. This should still be documented as the people who write the DT bindings (in general) aren't expected to read driver code as well. That's at least my understanding.

Yep, you are right, I forgot about the regulator. Something like this?

Thanks,
Pavel

Documentation/devicetree/bindings/media/i2c/ad5820.txt

Quoted 5 lines

Documentation/devicetree/bindings/media/i2c/ad5820.txt new file +0 -0

new file mode 100644
index 0000000..87c98f1

I might use the compatible string as such as a part of the file name. Up to you.

Quoted 11 lines

Documentation/devicetree/bindings/media/i2c/ad5820.txt new file +10 -0

@@ -0,0 +1,20 @@
+* Analog Devices AD5820 autofocus coil
+
+Required Properties:
+
+  - compatible: Must contain "adi,ad5820"
+
+  - reg: I2C slave address
+
+  - VANA-supply: supply of voltage for VANA pin
+

Documentation/devicetree/bindings/media/i2c/ad5820.txt line 13

Quoted 3 lines

Documentation/devicetree/bindings/media/i2c/ad5820.txt new file +3 -0

+Example:
+
+       /* D/A converter for auto-focus */

There is definitely D/A conversion happening there but I'm not sure I'd characterise the device as such. They're typically called "voice coil drivers", perhaps because the devices are similar to a parts of a loudspeaker.

Quoted 20 lines · Sakari Ailus

Quoted 19 lines · Pavel Machek

Quoted 5 lines · Sakari Ailus

Hi Pavel,

Quoted 3 lines

Well, it does not use any dt properties. So there's not really much to discuss with dt people...

Quoted 7 lines

Quoted 2 lines

Maybe "ad5820" needs to go to list of simple i2c drivers somewhere, but...

It's an I2C device and it does use a regulator. Not a lot, though, these are both quite basic stuff. This should still be documented as the people who write the DT bindings (in general) aren't expected to read driver code as well. That's at least my understanding.

Yep, you are right, I forgot about the regulator. Something like this?

Thanks,
Pavel

Quoted 8 lines

Documentation/devicetree/bindings/media/i2c/ad5820.txt

Quoted 5 lines

Documentation/devicetree/bindings/media/i2c/ad5820.txt new file +0 -0

new file mode 100644
index 0000000..87c98f1

I might use the compatible string as such as a part of the file name. Up to you.

Sorry, can't do that, for consistency with other ad* files in the directory.

Quoted 11 lines

Quoted 11 lines

Documentation/devicetree/bindings/media/i2c/ad5820.txt new file +10 -0

@@ -0,0 +1,20 @@
+* Analog Devices AD5820 autofocus coil
+
+Required Properties:
+
+  - compatible: Must contain "adi,ad5820"
+
+  - reg: I2C slave address
+
+  - VANA-supply: supply of voltage for VANA pin
+

Quoted 8 lines

Documentation/devicetree/bindings/media/i2c/ad5820.txt line 13

Quoted 3 lines

Documentation/devicetree/bindings/media/i2c/ad5820.txt new file +3 -0

+Example:
+
+       /* D/A converter for auto-focus */

There is definitely D/A conversion happening there but I'm not sure I'd characterise the device as such. They're typically called "voice coil drivers", perhaps because the devices are similar to a parts of a loudspeaker.

Well, I'm pretty sure I did not invent that comment, but I can drop it.

Add documentation for ad5820 device tree binding.

Signed-off-by Pavel Machek <[email protected]>

Quoted 4 lines · Pavel Machek

Add documentation for ad5820 device tree binding.

Signed-off-by Pavel Machek <[email protected]>

Thanks, Pavel!!

Can I pick the two patches (this one + the driver) or would you like to send a pull request? In the latter case you can add:

Acked-by Sakari Ailus <[email protected]>

Add userspace API definitions.

Signed-off-by Pavel Machek <[email protected]>

include/uapi/linux/v4l2-controls.h +0 -0

index b6a357a..23011cc 100644

Quoted 12 lines

Add userspace API definitions.

Signed-off-by Pavel Machek <[email protected]>

include/uapi/linux/v4l2-controls.h +0 -0

index b6a357a..23011cc 100644
@@ -974,4 +975,9 @@ enum v4l2_detect_md_mode {
 #define V4L2_CID_DETECT_MD_THRESHOLD_GRID	(V4L2_CID_DETECT_CLASS_BASE + 3)
 #define V4L2_CID_DETECT_MD_REGION_GRID		(V4L2_CID_DETECT_CLASS_BASE + 4)
 

include/uapi/linux/v4l2-controls.h enum v4l2_detect_md_mode { · line 983

Quoted 7 lines

include/uapi/linux/v4l2-controls.h +5 -0

+/* Control IDs specific to the AD5820 driver as defined by V4L2 */
+#define V4L2_CID_FOCUS_AD5820_BASE 	(V4L2_CTRL_CLASS_CAMERA | 0x10af)
+#define V4L2_CID_FOCUS_AD5820_RAMP_TIME	(V4L2_CID_FOCUS_AD5820_BASE+0)
+#define V4L2_CID_FOCUS_AD5820_RAMP_MODE	(V4L2_CID_FOCUS_AD5820_BASE+1)
+
 #endif

Sakari, what about adding those as standard camera controls? It seems ad5820
is not the only VCM driver to implement "antiringing" controls, http://rohmfs.rohm.com/en/products/databook/datasheet/ic/motor/mobile_module/bu64241gwz-e.pdf
is another example I found by quick search.

What about:

#define V4L2_CID_FOCUS_STEP_MODE xxx
enum v4l2_cid_focus_step_mode {
V4L2_CID_FOCUS_STEP_MODE_DIRECT,
V4L2_CID_FOCUS_STEP_MODE_LINEAR,
V4L2_CID_FOCUS_STEP_MODE_AUTO
};
#define V4L2_CID_FOCUS_STEP_TIME xxx+1

Also, how the userspace(or the kernel) is notified by v4l that there is an event? The point is - I think it is a good idea to notify when VCM has completed its movement, we can start a timer based on the current position,

Hi,

Quoted 12 lines · Pavel Machek

Add userspace API definitions.

Signed-off-by Pavel Machek <[email protected]>

include/uapi/linux/v4l2-controls.h +0 -0

index b6a357a..23011cc 100644
@@ -974,4 +975,9 @@ enum v4l2_detect_md_mode {
 #define V4L2_CID_DETECT_MD_THRESHOLD_GRID	(V4L2_CID_DETECT_CLASS_BASE + 3)
 #define V4L2_CID_DETECT_MD_REGION_GRID		(V4L2_CID_DETECT_CLASS_BASE + 4)
 

include/uapi/linux/v4l2-controls.h enum v4l2_detect_md_mode { · line 979

Quoted 2 lines

include/uapi/linux/v4l2-controls.h +2 -0

+/* Control IDs specific to the AD5820 driver as defined by V4L2 */
+#define V4L2_CID_FOCUS_AD5820_BASE 	(V4L2_CTRL_CLASS_CAMERA | 0x10af)

include/uapi/linux/v4l2-controls.h enum v4l2_detect_md_mode { · line 983

Quoted 5 lines

include/uapi/linux/v4l2-controls.h +3 -0

+#define V4L2_CID_FOCUS_AD5820_RAMP_TIME	(V4L2_CID_FOCUS_AD5820_BASE+0)
+#define V4L2_CID_FOCUS_AD5820_RAMP_MODE	(V4L2_CID_FOCUS_AD5820_BASE+1)
+
 #endif

Sakari, what about adding those as standard camera controls? It seems ad5820
is not the only VCM driver to implement "antiringing" controls, http://rohmfs.rohm.com/en/products/databook/datasheet/ic/motor/mobile_module/bu64241gwz-e.pdf
is another example I found by quick search.

What about:

#define V4L2_CID_FOCUS_STEP_MODE xxx
enum v4l2_cid_focus_step_mode {
V4L2_CID_FOCUS_STEP_MODE_DIRECT,
V4L2_CID_FOCUS_STEP_MODE_LINEAR,
V4L2_CID_FOCUS_STEP_MODE_AUTO
};
#define V4L2_CID_FOCUS_STEP_TIME xxx+1

Also, how the userspace(or the kernel) is notified by v4l that there is an event? The point is - I think it is a good idea to notify when VCM has completed its movement, we can start a timer based on the current position, mode, step time etc and notify after the pre-calculated movement time.

Quoted 6 lines

include/uapi/linux/v4l2-controls.h enum v4l2_detect_md_mode { · line 979

Quoted 6 lines

include/uapi/linux/v4l2-controls.h +2 -0

@@ -974,4 +975,9 @@ enum v4l2_detect_md_mode {
 #define V4L2_CID_DETECT_MD_THRESHOLD_GRID	(V4L2_CID_DETECT_CLASS_BASE + 3)
 #define V4L2_CID_DETECT_MD_REGION_GRID		(V4L2_CID_DETECT_CLASS_BASE + 4)
 
+/* Control IDs specific to the AD5820 driver as defined by V4L2 */
+#define V4L2_CID_FOCUS_AD5820_BASE 	(V4L2_CTRL_CLASS_CAMERA | 0x10af)

Please check V4L2_CID_USER_*_BASE. That's how custom controls are handled nowadays.

Now that I think about this, the original implementation in N900 very likely did not use either of the two controls; the device driver was still written to provide access to full capabilities of the chip. And that one had no continuous AF.

I might as well drop the two controls, up to you. If someone ever needs them they can always be reintroduced. I'd be happy to get a new patch, the current driver patch does not compile (just tried) as the definitions of these controls are missing.

Hi!

Quoted 9 lines

Quoted 6 lines

include/uapi/linux/v4l2-controls.h enum v4l2_detect_md_mode { · line 979

Quoted 6 lines

include/uapi/linux/v4l2-controls.h +2 -0

@@ -974,4 +975,9 @@ enum v4l2_detect_md_mode {
 #define V4L2_CID_DETECT_MD_THRESHOLD_GRID	(V4L2_CID_DETECT_CLASS_BASE + 3)
 #define V4L2_CID_DETECT_MD_REGION_GRID		(V4L2_CID_DETECT_CLASS_BASE + 4)
 
+/* Control IDs specific to the AD5820 driver as defined by V4L2 */
+#define V4L2_CID_FOCUS_AD5820_BASE 	(V4L2_CTRL_CLASS_CAMERA | 0x10af)

Please check V4L2_CID_USER_*_BASE. That's how custom controls are handled nowadays.

Let me see...

Quoted 4 lines

Now that I think about this, the original implementation in N900 very likely did not use either of the two controls; the device driver was still written to provide access to full capabilities of the chip. And that one had no continuous AF.

I'm not sure about the original implementation, but fcam-dev library (which is our best chance for usable camera) does use both:

pavel at duo:~/g/fcam-dev$ grep -ri RAMP_TIME . ./.svn/pristine/05/0574680922f59e07bd49e16a951d69421690a323.svn-base:
int val = ioctlSet(V4L2_CID_FOCUS_AD5820_RAMP_TIME, 1000000.0f/diopterRateToTickRate(speed));
./src/N900/Lens.cpp: int val =
ioctlSet(V4L2_CID_FOCUS_AD5820_RAMP_TIME, 1000000.0f/diopterRateToTickRate(speed));
pavel at duo:~/g/fcam-dev$ grep -ri RAMP_MODE . ./.svn/pristine/05/0574680922f59e07bd49e16a951d69421690a323.svn-base:
ioctlSet(V4L2_CID_FOCUS_AD5820_RAMP_MODE, 0);
./src/N900/Lens.cpp: ioctlSet(V4L2_CID_FOCUS_AD5820_RAMP_MODE, 0);
pavel at duo:~/g/fcam-dev$

Quoted 4 lines

I might as well drop the two controls, up to you. If someone ever needs them they can always be reintroduced. I'd be happy to get a new patch, the current driver patch does not compile (just tried) as the definitions of these controls are missing.

I'd prefer to keep the controls, as we have userspace using them. I got it to compile but have yet to get it to work (subdevs split, so it needs some modifications).

Quoted 12 lines

Quoted 12 lines

Add userspace API definitions.

Signed-off-by Pavel Machek <[email protected]>

include/uapi/linux/v4l2-controls.h +0 -0

index b6a357a..23011cc 100644
@@ -974,4 +975,9 @@ enum v4l2_detect_md_mode {
 #define V4L2_CID_DETECT_MD_THRESHOLD_GRID	(V4L2_CID_DETECT_CLASS_BASE + 3)
 #define V4L2_CID_DETECT_MD_REGION_GRID		(V4L2_CID_DETECT_CLASS_BASE + 4)
 

Quoted 2 lines

include/uapi/linux/v4l2-controls.h enum v4l2_detect_md_mode { · line 979

Quoted 2 lines

include/uapi/linux/v4l2-controls.h +2 -0

+/* Control IDs specific to the AD5820 driver as defined by V4L2 */
+#define V4L2_CID_FOCUS_AD5820_BASE 	(V4L2_CTRL_CLASS_CAMERA | 0x10af)

Please check V4L2_CID_USER_*_BASE. That's how custom controls are handled nowadays.

Hi!

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Add userspace API definitions.

Signed-off-by Pavel Machek <[email protected]>

include/uapi/linux/v4l2-controls.h +0 -0

index b6a357a..23011cc 100644
@@ -974,4 +975,9 @@ enum v4l2_detect_md_mode {
 #define V4L2_CID_DETECT_MD_THRESHOLD_GRID	(V4L2_CID_DETECT_CLASS_BASE + 3)
 #define V4L2_CID_DETECT_MD_REGION_GRID		(V4L2_CID_DETECT_CLASS_BASE + 4)
 

Quoted 5 lines

Quoted 2 lines

include/uapi/linux/v4l2-controls.h enum v4l2_detect_md_mode { · line 979

Quoted 2 lines

include/uapi/linux/v4l2-controls.h +2 -0

+/* Control IDs specific to the AD5820 driver as defined by V4L2 */
+#define V4L2_CID_FOCUS_AD5820_BASE 	(V4L2_CTRL_CLASS_CAMERA | 0x10af)

Please check V4L2_CID_USER_*_BASE. That's how custom controls are handled nowadays.

So something like this?

Thanks,
Pavel

drivers/media/i2c/ad5820.c +0 -0

index 2efa5dc1..b04b471 100644

drivers/media/i2c/ad5820.c +0 -0

 #define CODE_TO_RAMP_US(s)	((s) == 0 ? 0 : (1 << ((s) - 1)) * 50)
 #define RAMP_US_TO_CODE(c)	fls(((c) + ((c)>>1)) / 50)
 

include/uapi/linux/v4l2-controls.h +4 -5

index 23011cc..4b24546 100644
@@ -181,6 +181,10 @@ enum v4l2_colorfx {
  * We reserve 16 controls for this driver. */
 #define V4L2_CID_USER_TC358743_BASE		(V4L2_CID_USER_BASE + 0x1080)
 
+/* The base for the ad5820 driver controls.
+ * We reserve 16 controls for this driver. */
+#define V4L2_CID_USER_AD5820_BASE		(V4L2_CID_USER_BASE + 0x1090)
+
 /* MPEG-class control IDs */
 /* The MPEG controls are applicable to all codec controls
  * and the 'MPEG' part of the define is historical */

@@ -986,9 +990,4 @@ enum v4l2_detect_md_mode {
 #define V4L2_CID_MODE_SENSITIVITY		(V4L2_CID_MODE_CLASS_BASE+6)
 #define V4L2_CID_MODE_OPSYSCLOCK		(V4L2_CID_MODE_CLASS_BASE+7)
 
-/* Control IDs specific to the AD5820 driver as defined by V4L2 */
-#define V4L2_CID_FOCUS_AD5820_BASE 		(V4L2_CTRL_CLASS_CAMERA | 0x10af)
-#define V4L2_CID_FOCUS_AD5820_RAMP_TIME		(V4L2_CID_FOCUS_AD5820_BASE+0)
-#define V4L2_CID_FOCUS_AD5820_RAMP_MODE		(V4L2_CID_FOCUS_AD5820_BASE+1)
-
 #endif

Hi Pavel,

Quoted 33 lines · Pavel Machek

Hi!

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Add userspace API definitions.

Signed-off-by Pavel Machek <[email protected]>

include/uapi/linux/v4l2-controls.h +0 -0

index b6a357a..23011cc 100644
@@ -974,4 +975,9 @@ enum v4l2_detect_md_mode {
 #define V4L2_CID_DETECT_MD_THRESHOLD_GRID	(V4L2_CID_DETECT_CLASS_BASE + 3)
 #define V4L2_CID_DETECT_MD_REGION_GRID		(V4L2_CID_DETECT_CLASS_BASE + 4)
 

Quoted 5 lines

Quoted 2 lines

include/uapi/linux/v4l2-controls.h enum v4l2_detect_md_mode { · line 979

Quoted 2 lines

include/uapi/linux/v4l2-controls.h +2 -0

+/* Control IDs specific to the AD5820 driver as defined by V4L2 */
+#define V4L2_CID_FOCUS_AD5820_BASE 	(V4L2_CTRL_CLASS_CAMERA | 0x10af)

Please check V4L2_CID_USER_*_BASE. That's how custom controls are handled nowadays.

So something like this?

Thanks,
Pavel

drivers/media/i2c/ad5820.c +0 -0

index 2efa5dc1..b04b471 100644
@@ -40,6 +40,11 @@
 #define AD5820_RAMP_MODE_LINEAR		(0 << 3)
 #define AD5820_RAMP_MODE_64_16		(1 << 3)
 

drivers/media/i2c/ad5820.c line 47

Quoted 5 lines

drivers/media/i2c/ad5820.c +5 -0

+/* Control IDs specific to the AD5820 driver as defined by V4L2 */
+#define V4L2_CID_FOCUS_AD5820_RAMP_TIME		(V4L2_CID_USER_AD5820_BASE+0)
+#define V4L2_CID_FOCUS_AD5820_RAMP_MODE		(V4L2_CID_FOCUS_AD5820_BASE+1)
+
+

We could still define these in a header file that can be included by the user space. Please use V4L2_CID_AD5820 prefix.

A separate header file should be used,
e.g. include/uapi/linux/ad5820.h.

Quoted 36 lines · Sakari Ailus

Hi Pavel,

Quoted 33 lines · Pavel Machek

Hi!

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Add userspace API definitions.

Signed-off-by Pavel Machek <[email protected]>

include/uapi/linux/v4l2-controls.h +0 -0

index b6a357a..23011cc 100644
@@ -974,4 +975,9 @@ enum v4l2_detect_md_mode {
 #define V4L2_CID_DETECT_MD_THRESHOLD_GRID	(V4L2_CID_DETECT_CLASS_BASE + 3)
 #define V4L2_CID_DETECT_MD_REGION_GRID		(V4L2_CID_DETECT_CLASS_BASE + 4)
 

Quoted 5 lines

Quoted 2 lines

include/uapi/linux/v4l2-controls.h enum v4l2_detect_md_mode { · line 979

Quoted 2 lines

include/uapi/linux/v4l2-controls.h +2 -0

+/* Control IDs specific to the AD5820 driver as defined by V4L2 */
+#define V4L2_CID_FOCUS_AD5820_BASE 	(V4L2_CTRL_CLASS_CAMERA | 0x10af)

Please check V4L2_CID_USER_*_BASE. That's how custom controls are handled nowadays.

So something like this?

Thanks,
Pavel

drivers/media/i2c/ad5820.c +0 -0

index 2efa5dc1..b04b471 100644
@@ -40,6 +40,11 @@
 #define AD5820_RAMP_MODE_LINEAR		(0 << 3)
 #define AD5820_RAMP_MODE_64_16		(1 << 3)
 

Quoted 8 lines

drivers/media/i2c/ad5820.c line 47

Quoted 5 lines

drivers/media/i2c/ad5820.c +5 -0

+/* Control IDs specific to the AD5820 driver as defined by V4L2 */
+#define V4L2_CID_FOCUS_AD5820_RAMP_TIME		(V4L2_CID_USER_AD5820_BASE+0)
+#define V4L2_CID_FOCUS_AD5820_RAMP_MODE		(V4L2_CID_FOCUS_AD5820_BASE+1)
+
+

We could still define these in a header file that can be included by the user space. Please use V4L2_CID_AD5820 prefix.

Not V4L2_CID_USER_AD5820...?

Quoted 3 lines

A separate header file should be used,
e.g. include/uapi/linux/ad5820.h.

Ok, separate header file for 2 lines seemed like a bit of overkill, but why not.