[omap3-isp-live] Autofocus buffer interpretation of H3A engine

[omap3-isp-live] Autofocus buffer interpretation of H3A engine

[Florian Neuhaus]

Hi Laurent

I am adapting your current omap3-isp-live application to fit our needs (http://git.ideasonboard.org/omap3-isp-live.git/commit/619164a994c8d878249d6c1b8b16c27074a04209).
For this I need to implement an autofocus algorithm to control an attached actuator. 
Let me summarize what components I use:

- beagleboard-xm
- mt9p031 on leopardimaging li-5m03
- own developed voice-coil-motor (vcm) driven by current regulator ad5821
- kernel 3.5 out of the linux-omap tree

To generate a control signal for my vcm, I need to interpret the values generated by 
the AF engine of the omap3isp, h3a module. I see, that you have already implemented 
a working auto-exposure functionality with the AEWB module - so I have done the same 
with the AF module. But now I have problems to interpret the generated values in omap3_isp_af_process. Do you know how the paxel-buffer is structured? 
I see that the received buffer has the size of

buf_size = af->paxel.h_cnt * af->paxel.v_cnt * OMAP3ISP_AF_PAXEL_SIZE;

where  OMAP3ISP_AF_PAXEL_SIZE is 48 bytes, but what is in the 48 bytes?

Have you made some progress in implementing an autofocus algorithm?

Thank you for your help and greetings,
Florian

Re: [omap3-isp-live] Autofocus buffer interpretation of H3A engine

[Laurent Pinchart]

Hi Florian,

On Thursday 01 November 2012 11:17:44 Florian Neuhaus wrote:
> Hi Laurent
> 
> I am adapting your current omap3-isp-live application to fit our needs
> (http://git.ideasonboard.org/omap3-isp-live.git/commit/619164a994c8d878249d
> 6c1b8b16c27074a04209). For this I need to implement an autofocus algorithm
> to control an attached actuator. Let me summarize what components I use:
> 
> - beagleboard-xm
> - mt9p031 on leopardimaging li-5m03
> - own developed voice-coil-motor (vcm) driven by current regulator ad5821

The AD5821 is similar to the AD5820, for which I have a driver that I need to 
clean up and post. Would you be interested in that ?

> - kernel 3.5 out of the linux-omap tree
> 
> To generate a control signal for my vcm, I need to interpret the values
> generated by the AF engine of the omap3isp, h3a module. I see, that you
> have already implemented a working auto-exposure functionality with the
> AEWB module - so I have done the same with the AF module. But now I have
> problems to interpret the generated values in omap3_isp_af_process. Do you
> know how the paxel-buffer is structured?

Even though that buffer structure is pretty simple, I'm afraid I can't provide 
that information as it's covered by an NDA.

Note that a description of the buffer's contents won't help you much, as you 
wouldn't know how to compute the IIR filters coefficients.

There's two way to move forward here. We could ask TI to release the required 
information to produce an open-source AF implementation, or we could try to 
figure it out ourselves. Looking at the FCam project, I've found

http://vcs.maemo.org/svn/fcam/fcam-dev/tags/1.1.0/src/N900/V4L2Sensor.cpp

Does that help figuring out what the buffer contains ?

> I see that the received buffer has the size of
> 
> buf_size = af->paxel.h_cnt * af->paxel.v_cnt * OMAP3ISP_AF_PAXEL_SIZE;
> 
> where  OMAP3ISP_AF_PAXEL_SIZE is 48 bytes, but what is in the 48 bytes?
> 
> Have you made some progress in implementing an autofocus algorithm?

I haven't started, and it's currently not on my to-do list I'm afraid.

-- 
Regards,

Laurent Pinchart

AW: [omap3-isp-live] Autofocus buffer interpretation of H3A engine

[Florian Neuhaus]

Hi Laurent,

Laurent Pinchart wrote on 2012-11-04:

> The AD5821 is similar to the AD5820, for which I have a driver that I need to
> clean up and post. Would you be interested in that ?
Yes, you can send me the driver. Just as a note:
I (probably) found an error in the current ad5398 and ad5821 driver 
http://lxr.free-electrons.com/source/drivers/regulator/ad5398.c
It seems that the enable and disable functions are switched
(at least for the ad5821). Also it's not possible to set the maximum current.
I've done a patch but didn't have the time to submit it. Is this the right place
for it?

> Even though that buffer structure is pretty simple, I'm afraid I can't provide
> that information as it's covered by an NDA.
I have written the TI-support and here's the answer:
START QUOTE
" After checking it seems that we unfortunately do not make the DM37x H3A documention available. 
Implementing Autofocus is quite complexe and the documentation is likely not going to provide enough help. 
A lot of experience is required to handle the mecanics and control loop aspect for the Autofocus.

We have partners that do have experience with the H3A module and that might be able to help. 
For example MMS that mentionned is the below document:
     http://www.ti.com/lit/ml/swpt052/swpt052.pdf
Also Leopard Imaging has experience on the H3A:
    https://www.leopardimaging.com/Services.html
END QUOTE

It's sad that they can't provide any further information, because we are not that far
to get this stuff working...

> could try to figure it out ourselves. Looking at the FCam project, I've
> found
> 
> http://vcs.maemo.org/svn/fcam/fcam-
> dev/tags/1.1.0/src/N900/V4L2Sensor.cpp
> 
> Does that help figuring out what the buffer contains ?

That helps a lot! Thank you. I found also a little info here:
http://www.mail-archive.com/davinci-linux-open-source@linux.davincidsp.com/msg18438.html

> I haven't started, and it's currently not on my to-do list I'm afraid.
That's too bad, as your help is always appreciated. Have you already seen my patch for the
rotation in your omap3-isp-live program? I have seen also another little issue, if you are
interested (snapshot_init should be after aewb as the output format changes during
snapshot-init).

I am now off for 3 weeks (annual refresher course in the Swiss armed forces).

Regards,
Florian

[PATCH] ad5820: Voice coil motor controller driver

[Laurent Pinchart]

The AD5820 is a voice coil motor controller typically used to control
lens position in digital cameras.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/Kconfig  |    9 +
 drivers/media/i2c/Makefile |    1 +
 drivers/media/i2c/ad5820.c |  496 ++++++++++++++++++++++++++++++++++++++++++++
 include/media/ad5820.h     |   30 +++
 4 files changed, 536 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/i2c/ad5820.c
 create mode 100644 include/media/ad5820.h

Hi Florian,

This is the ad5820 driver I've told you about. The code is compile-tested only
as I haven't had time to try it on an N900 (the only device I own that
includes an ad5820).

It should be quite easy to adapt the driver to support both the ad5820 and the
ad5821. Would you have time to give it a try ?

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 24d78e2..65597cf 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -534,6 +534,15 @@ config VIDEO_AS3645A
 	  This is a driver for the AS3645A and LM3555 flash controllers. It has
 	  build in control for flash, torch and indicator LEDs.
 
+comment "Lens controllers"
+
+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 RX51.
+
 comment "Video improvement chips"
 
 config VIDEO_UPD64031A
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index b1d62df..975cfb8 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_VIDEO_SR030PC30)	+= sr030pc30.o
 obj-$(CONFIG_VIDEO_NOON010PC30)	+= noon010pc30.o
 obj-$(CONFIG_VIDEO_S5K6AA)	+= s5k6aa.o
 obj-$(CONFIG_VIDEO_S5K4ECGX)	+= s5k4ecgx.o
+obj-$(CONFIG_VIDEO_AD5820)	+= ad5820.o
 obj-$(CONFIG_VIDEO_ADP1653)	+= adp1653.o
 obj-$(CONFIG_VIDEO_AS3645A)	+= as3645a.o
 obj-$(CONFIG_VIDEO_SMIAPP_PLL)	+= smiapp-pll.o
diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
new file mode 100644
index 0000000..995774f
--- /dev/null
+++ b/drivers/media/i2c/ad5820.c
@@ -0,0 +1,496 @@
+/*
+ * AD5820 DAC driver for camera voice coil focus.
+ *
+ * Copyright (C) 2008 Nokia Corporation
+ * Copyright (C) 2007 Texas Instruments
+ *
+ * Contact: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ *          Sakari Ailus <sakari.ailus@iki.fi>
+ *
+ * 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.
+ */
+
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+
+#include <media/ad5820.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-subdev.h>
+
+/* 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)
+
+struct ad5820_device {
+	struct v4l2_subdev subdev;
+
+	struct regulator *vana;
+	int xshutdown;
+
+	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;
+};
+
+#define to_ad5820_device(sd)	container_of(sd, struct ad5820_device, subdev)
+
+/**
+ * @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;
+}
+
+/**
+ * @brief I2C read using i2c_transfer().
+ * @param coil - the driver data structure
+ * @returns unsigned 16-bit register value on success, negative if failed
+ */
+static int ad5820_read(struct ad5820_device *coil)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&coil->subdev);
+	struct i2c_msg msg;
+	int r;
+	u16 data = 0;
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	msg.addr  = client->addr;
+	msg.flags = I2C_M_RD;
+	msg.len   = 2;
+	msg.buf   = (u8 *)&data;
+
+	r = i2c_transfer(client->adapter, &msg, 1);
+	if (r < 0) {
+		dev_err(&client->dev, "read failed, error %d\n", r);
+		return r;
+	}
+
+	return be16_to_cpu(data);
+}
+
+/* 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 void ad5820_power_off(struct ad5820_device *coil)
+{
+	if (coil->xshutdown != -1)
+		gpio_set_value(coil->xshutdown, 0);
+
+	regulator_disable(coil->vana);
+}
+
+static int ad5820_power_on(struct ad5820_device *coil)
+{
+	int ret;
+
+	ret = regulator_enable(coil->vana);
+	if (ret < 0)
+		return ret;
+
+	if (coil->xshutdown != -1)
+		gpio_set_value(coil->xshutdown, 1);
+
+	return 0;
+}
+
+/* --------------------------------------------------------------------------
+ * V4L2 controls
+ */
+
+#define V4L2_CID_FOCUS_AD5820_RAMP_TIME		(V4L2_CID_USER_BASE | 0x1000)
+#define V4L2_CID_FOCUS_AD5820_RAMP_MODE		(V4L2_CID_USER_BASE | 0x1001)
+
+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);
+		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 r;
+}
+
+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,
+	},
+};
+
+/* --------------------------------------------------------------------------
+ * V4L2 subdev operations
+ */
+
+static int __ad5820_set_power(struct ad5820_device *coil, bool on)
+{
+	int ret;
+
+	if (!on) {
+		/* Go to standby first as real power off my be denied by the
+		 * hardware (single power line control for both coil and
+		 * sensor).
+		 */
+		coil->standby = 1;
+		ad5820_update_hw(coil);
+		ad5820_power_off(coil);
+		return 0;
+	}
+
+	ret = ad5820_power_on(coil);
+	if (ret < 0)
+		return ret;
+
+	/* Restore the hardware settings. */
+	coil->standby = 0;
+	ret = ad5820_update_hw(coil);
+	if (ret < 0) {
+		coil->standby = 1;
+		return ret;
+	}
+
+	return 0;
+}
+
+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 = __ad5820_set_power(coil, !!on);
+		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 = {
+	.open = ad5820_open,
+	.close = ad5820_close,
+};
+
+/* --------------------------------------------------------------------------
+ * I2C driver
+ */
+#ifdef CONFIG_PM
+
+static int ad5820_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_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;
+
+	ad5820_power_off(coil);
+	return 0;
+}
+
+static int ad5820_resume(struct device *dev)
+{
+	struct i2c_client *client = to_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_set_power(coil, true);
+}
+
+#endif /* CONFIG_PM */
+
+static int ad5820_detect(struct ad5820_device *coil)
+{
+	const u16 status = AD5820_POWER_DOWN | 0x3ff0;
+	int ret;
+
+	if (ad5820_power_on(coil) < 0)
+		return -ENODEV;
+
+	if (ad5820_write(coil, status) || ad5820_read(coil) != status) {
+		ad5820_power_off(coil);
+		return -ENODEV;
+	}
+
+	__ad5820_set_power(coil, false);
+	return ret;
+}
+
+static int ad5820_probe(struct i2c_client *client,
+			const struct i2c_device_id *devid)
+{
+	struct ad5820_platform_data *pdata = client->dev.platform_data;
+	struct ad5820_device *coil;
+	unsigned int i;
+	int ret = -ENODEV;
+
+	coil = kzalloc(sizeof(*coil), GFP_KERNEL);
+	if (coil == NULL)
+		return -ENOMEM;
+
+	mutex_init(&coil->power_lock);
+	coil->focus_absolute = 0;
+	coil->focus_ramp_time = 0;
+	coil->focus_ramp_mode = 0;
+	coil->xshutdown = -1;
+
+	coil->vana = regulator_get(&client->dev, "VANA");
+	if (IS_ERR(coil->vana)) {
+		dev_err(&client->dev, "could not get regulator for vana\n");
+		goto done;
+	}
+
+	if (pdata && pdata->xshutdown != -1) {
+		ret = gpio_request_one(pdata->xshutdown, GPIOF_OUT_INIT_LOW,
+				       "ad5820_xshutdown");
+		if (ret < 0)
+			goto done;
+
+		coil->xshutdown = pdata->xshutdown;
+	}
+
+	ret = ad5820_detect(coil);
+	if (ret < 0) {
+		dev_err(&client->dev, "not detected\n");
+		goto done;
+	}
+
+	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) {
+		ret = coil->ctrls.error;
+		goto done;
+	}
+
+	coil->subdev.ctrl_handler = &coil->ctrls;
+
+	v4l2_i2c_subdev_init(&coil->subdev, client, &ad5820_ops);
+	coil->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	coil->subdev.internal_ops = &ad5820_internal_ops;
+
+	ret = media_entity_init(&coil->subdev.entity, 0, NULL, 0);
+	if (ret < 0)
+		goto done;
+
+	ret = 0;
+
+done:
+	if (ret < 0) {
+		v4l2_ctrl_handler_free(&coil->ctrls);
+		media_entity_cleanup(&coil->subdev.entity);
+
+		if (coil->xshutdown != -1)
+			gpio_free(coil->xshutdown);
+		regulator_put(coil->vana);
+
+		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->xshutdown != -1)
+		gpio_free(coil->xshutdown);
+	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_ops, ad5820_suspend, ad5820_resume);
+
+static struct i2c_driver ad5820_i2c_driver = {
+	.driver		= {
+		.name	= AD5820_NAME,
+		.pm	= &ad5820_pm_ops,
+	},
+	.probe		= ad5820_probe,
+	.remove		= __exit_p(ad5820_remove),
+	.id_table	= ad5820_id_table,
+};
+
+module_i2c_driver(ad5820_i2c_driver);
+
+MODULE_AUTHOR("Tuukka Toivonen <tuukka.o.toivonen@nokia.com>");
+MODULE_DESCRIPTION("AD5820 camera lens driver");
+MODULE_LICENSE("GPL");
diff --git a/include/media/ad5820.h b/include/media/ad5820.h
new file mode 100644
index 0000000..2c28131
--- /dev/null
+++ b/include/media/ad5820.h
@@ -0,0 +1,30 @@
+/*
+ * AD5820 DAC driver for camera voice coil focus.
+ *
+ * Copyright (C) 2008 Nokia Corporation
+ * Copyright (C) 2007 Texas Instruments
+ *
+ * Contact: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ *          Sakari Ailus <sakari.ailus@iki.fi>
+ *
+ * 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.
+ */
+
+#ifndef __AD5820_H__
+#define __AD5820_H__
+
+#define AD5820_NAME		"ad5820"
+#define AD5820_I2C_ADDR		(0x18 >> 1)
+
+struct ad5820_platform_data {
+	int xshutdown;
+};
+
+#endif /* __AD5820_H__ */
-- 
Regards,

Laurent Pinchart

Re: AW: [omap3-isp-live] Autofocus buffer interpretation of H3A engine

[Laurent Pinchart]

Hi Florian,

On Friday 09 November 2012 15:57:58 Florian Neuhaus wrote:
> Laurent Pinchart wrote on 2012-11-04:
> > The AD5821 is similar to the AD5820, for which I have a driver that I need
> > to clean up and post. Would you be interested in that ?
> 
> Yes, you can send me the driver.

I've just sent it to you and CC'ed the list.

> Just as a note:
> I (probably) found an error in the current ad5398 and ad5821 driver
> http://lxr.free-electrons.com/source/drivers/regulator/ad5398.c
> It seems that the enable and disable functions are switched
> (at least for the ad5821).

I think you're right.

> Also it's not possible to set the maximum current. I've done a patch but
> didn't have the time to submit it. Is this the right place for it?

As the ad5398 driver implements the regulator driver API you should send the 
patch to the appropriate mailing lists.

$ ./scripts/get_maintainer.pl -f drivers/regulator/ad5398.c 
Michael Hennerich <michael.hennerich@analog.com> (supporter:AD5398 CURRENT 
RE...)
Liam Girdwood <lrg@ti.com> (supporter:VOLTAGE AND CURRE...)
Mark Brown <broonie@opensource.wolfsonmicro.com> (supporter:VOLTAGE AND 
CURRE...)
device-drivers-devel@blackfin.uclinux.org (open list:AD5398 CURRENT RE...)
linux-kernel@vger.kernel.org (open list)

> > Even though that buffer structure is pretty simple, I'm afraid I can't
> > provide that information as it's covered by an NDA.
> 
> I have written the TI-support and here's the answer:
> START QUOTE
> " After checking it seems that we unfortunately do not make the DM37x H3A
> documention available. Implementing Autofocus is quite complexe and the
> documentation is likely not going to provide enough help. A lot of
> experience is required to handle the mecanics and control loop aspect for
> the Autofocus.
> 
> We have partners that do have experience with the H3A module and that might
> be able to help. For example MMS that mentionned is the below document:
>      http://www.ti.com/lit/ml/swpt052/swpt052.pdf
> Also Leopard Imaging has experience on the H3A:
>     https://www.leopardimaging.com/Services.html
> END QUOTE
> 
> It's sad that they can't provide any further information, because we are not
> that far to get this stuff working...

Try pushing a bit more, sometimes it helps. Ask what it would take to get 
access to the documentation, possibly as a partner.

> > could try to figure it out ourselves. Looking at the FCam project, I've
> > found
> > 
> > http://vcs.maemo.org/svn/fcam/fcam-
> > dev/tags/1.1.0/src/N900/V4L2Sensor.cpp
> > 
> > Does that help figuring out what the buffer contains ?
> 
> That helps a lot! Thank you. I found also a little info here:
> http://www.mail-archive.com/davinci-linux-open-source@linux.davincidsp.com/m
> sg18438.html

If you can figure out the format and the configuration parameters from the 
above sources, I would greatly appreciate if you could write a bit of 
documentation and send a patch for Documentation/video4linux/omap3isp.txt. I 
unfortunately can't do it myself without TI's approval as I've had access to 
the documentation.

> > I haven't started, and it's currently not on my to-do list I'm afraid.
> 
> That's too bad, as your help is always appreciated.

Spare time is unfortunately limited :-)

> Have you already seen my patch for the rotation in your omap3-isp-live
> program?

It's still in my mailbox, I'll try to process it in the near future.

> I have seen also another little issue, if you are interested (snapshot_init
> should be after aewb as the output format changes during snapshot-init).

Sure, please send it.

> I am now off for 3 weeks (annual refresher course in the Swiss armed
> forces).

-- 
Regards,

Laurent Pinchart

Re: [PATCH] ad5820: Voice coil motor controller driver

[Sakari Ailus]

Hi Laurent,

Thanks for the patch!

On Thu, Nov 15, 2012 at 01:15:46AM +0100, Laurent Pinchart wrote:
> The AD5820 is a voice coil motor controller typically used to control
> lens position in digital cameras.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/i2c/Kconfig  |    9 +
>  drivers/media/i2c/Makefile |    1 +
>  drivers/media/i2c/ad5820.c |  496 ++++++++++++++++++++++++++++++++++++++++++++
>  include/media/ad5820.h     |   30 +++
>  4 files changed, 536 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/i2c/ad5820.c
>  create mode 100644 include/media/ad5820.h
> 
> Hi Florian,
> 
> This is the ad5820 driver I've told you about. The code is compile-tested only
> as I haven't had time to try it on an N900 (the only device I own that
> includes an ad5820).
> 
> It should be quite easy to adapt the driver to support both the ad5820 and the
> ad5821. Would you have time to give it a try ?
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 24d78e2..65597cf 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -534,6 +534,15 @@ config VIDEO_AS3645A
>  	  This is a driver for the AS3645A and LM3555 flash controllers. It has
>  	  build in control for flash, torch and indicator LEDs.
>  
> +comment "Lens controllers"
> +
> +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 RX51.
> +
>  comment "Video improvement chips"
>  
>  config VIDEO_UPD64031A
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index b1d62df..975cfb8 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_VIDEO_SR030PC30)	+= sr030pc30.o
>  obj-$(CONFIG_VIDEO_NOON010PC30)	+= noon010pc30.o
>  obj-$(CONFIG_VIDEO_S5K6AA)	+= s5k6aa.o
>  obj-$(CONFIG_VIDEO_S5K4ECGX)	+= s5k4ecgx.o
> +obj-$(CONFIG_VIDEO_AD5820)	+= ad5820.o
>  obj-$(CONFIG_VIDEO_ADP1653)	+= adp1653.o
>  obj-$(CONFIG_VIDEO_AS3645A)	+= as3645a.o
>  obj-$(CONFIG_VIDEO_SMIAPP_PLL)	+= smiapp-pll.o
> diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
> new file mode 100644
> index 0000000..995774f
> --- /dev/null
> +++ b/drivers/media/i2c/ad5820.c
> @@ -0,0 +1,496 @@
> +/*
> + * AD5820 DAC driver for camera voice coil focus.
> + *
> + * Copyright (C) 2008 Nokia Corporation
> + * Copyright (C) 2007 Texas Instruments

Wow. This is old.

> + * Contact: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + *          Sakari Ailus <sakari.ailus@iki.fi>
> + *
> + * 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.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/gpio.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>

Do you need all of these? E.g. delay.h and sched.h are probably not needed.

> +
> +#include <media/ad5820.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-subdev.h>
> +
> +/* 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)
> +
> +struct ad5820_device {
> +	struct v4l2_subdev subdev;
> +
> +	struct regulator *vana;
> +	int xshutdown;
> +
> +	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;
> +};
> +
> +#define to_ad5820_device(sd)	container_of(sd, struct ad5820_device, subdev)
> +
> +/**
> + * @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);

I know we've gotten used to i2c_transfer(), but wouldn't
i2c_master_{send,recv}() be better for the task?

> +	if (r < 0) {
> +		dev_err(&client->dev, "write failed, error %d\n", r);
> +		return r;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * @brief I2C read using i2c_transfer().
> + * @param coil - the driver data structure
> + * @returns unsigned 16-bit register value on success, negative if failed
> + */
> +static int ad5820_read(struct ad5820_device *coil)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&coil->subdev);
> +	struct i2c_msg msg;
> +	int r;
> +	u16 data = 0;
> +
> +	if (!client->adapter)
> +		return -ENODEV;
> +
> +	msg.addr  = client->addr;
> +	msg.flags = I2C_M_RD;
> +	msg.len   = 2;
> +	msg.buf   = (u8 *)&data;
> +
> +	r = i2c_transfer(client->adapter, &msg, 1);
> +	if (r < 0) {
> +		dev_err(&client->dev, "read failed, error %d\n", r);
> +		return r;
> +	}
> +
> +	return be16_to_cpu(data);
> +}
> +
> +/* 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 void ad5820_power_off(struct ad5820_device *coil)
> +{
> +	if (coil->xshutdown != -1)
> +		gpio_set_value(coil->xshutdown, 0);
> +
> +	regulator_disable(coil->vana);
> +}
> +
> +static int ad5820_power_on(struct ad5820_device *coil)
> +{
> +	int ret;
> +
> +	ret = regulator_enable(coil->vana);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (coil->xshutdown != -1)
> +		gpio_set_value(coil->xshutdown, 1);
> +
> +	return 0;
> +}
> +
> +/* --------------------------------------------------------------------------
> + * V4L2 controls
> + */
> +
> +#define V4L2_CID_FOCUS_AD5820_RAMP_TIME		(V4L2_CID_USER_BASE | 0x1000)
> +#define V4L2_CID_FOCUS_AD5820_RAMP_MODE		(V4L2_CID_USER_BASE | 0x1001)
> +
> +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);
> +		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 r;
> +}
> +
> +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,
> +	},
> +};
> +
> +/* --------------------------------------------------------------------------
> + * V4L2 subdev operations
> + */
> +
> +static int __ad5820_set_power(struct ad5820_device *coil, bool on)
> +{
> +	int ret;
> +
> +	if (!on) {
> +		/* Go to standby first as real power off my be denied by the
> +		 * hardware (single power line control for both coil and
> +		 * sensor).
> +		 */
> +		coil->standby = 1;
> +		ad5820_update_hw(coil);
> +		ad5820_power_off(coil);
> +		return 0;
> +	}
> +
> +	ret = ad5820_power_on(coil);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Restore the hardware settings. */
> +	coil->standby = 0;
> +	ret = ad5820_update_hw(coil);
> +	if (ret < 0) {
> +		coil->standby = 1;
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +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 = __ad5820_set_power(coil, !!on);
> +		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 = {
> +	.open = ad5820_open,
> +	.close = ad5820_close,
> +};
> +
> +/* --------------------------------------------------------------------------
> + * I2C driver
> + */
> +#ifdef CONFIG_PM
> +
> +static int ad5820_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_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;
> +
> +	ad5820_power_off(coil);
> +	return 0;
> +}
> +
> +static int ad5820_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_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_set_power(coil, true);
> +}
> +
> +#endif /* CONFIG_PM */
> +
> +static int ad5820_detect(struct ad5820_device *coil)
> +{
> +	const u16 status = AD5820_POWER_DOWN | 0x3ff0;
> +	int ret;
> +
> +	if (ad5820_power_on(coil) < 0)
> +		return -ENODEV;
> +
> +	if (ad5820_write(coil, status) || ad5820_read(coil) != status) {
> +		ad5820_power_off(coil);
> +		return -ENODEV;
> +	}
> +
> +	__ad5820_set_power(coil, false);
> +	return ret;
> +}
> +
> +static int ad5820_probe(struct i2c_client *client,
> +			const struct i2c_device_id *devid)
> +{
> +	struct ad5820_platform_data *pdata = client->dev.platform_data;
> +	struct ad5820_device *coil;
> +	unsigned int i;
> +	int ret = -ENODEV;
> +
> +	coil = kzalloc(sizeof(*coil), GFP_KERNEL);
> +	if (coil == NULL)
> +		return -ENOMEM;
> +
> +	mutex_init(&coil->power_lock);
> +	coil->focus_absolute = 0;
> +	coil->focus_ramp_time = 0;
> +	coil->focus_ramp_mode = 0;
> +	coil->xshutdown = -1;
> +
> +	coil->vana = regulator_get(&client->dev, "VANA");
> +	if (IS_ERR(coil->vana)) {
> +		dev_err(&client->dev, "could not get regulator for vana\n");
> +		goto done;
> +	}
> +
> +	if (pdata && pdata->xshutdown != -1) {
> +		ret = gpio_request_one(pdata->xshutdown, GPIOF_OUT_INIT_LOW,
> +				       "ad5820_xshutdown");
> +		if (ret < 0)
> +			goto done;
> +
> +		coil->xshutdown = pdata->xshutdown;
> +	}
> +
> +	ret = ad5820_detect(coil);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "not detected\n");
> +		goto done;
> +	}
> +
> +	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) {
> +		ret = coil->ctrls.error;
> +		goto done;
> +	}
> +
> +	coil->subdev.ctrl_handler = &coil->ctrls;
> +
> +	v4l2_i2c_subdev_init(&coil->subdev, client, &ad5820_ops);
> +	coil->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	coil->subdev.internal_ops = &ad5820_internal_ops;
> +
> +	ret = media_entity_init(&coil->subdev.entity, 0, NULL, 0);
> +	if (ret < 0)
> +		goto done;
> +
> +	ret = 0;
> +
> +done:
> +	if (ret < 0) {
> +		v4l2_ctrl_handler_free(&coil->ctrls);
> +		media_entity_cleanup(&coil->subdev.entity);
> +
> +		if (coil->xshutdown != -1)
> +			gpio_free(coil->xshutdown);
> +		regulator_put(coil->vana);
> +
> +		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->xshutdown != -1)
> +		gpio_free(coil->xshutdown);
> +	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_ops, ad5820_suspend, ad5820_resume);
> +
> +static struct i2c_driver ad5820_i2c_driver = {
> +	.driver		= {
> +		.name	= AD5820_NAME,
> +		.pm	= &ad5820_pm_ops,
> +	},
> +	.probe		= ad5820_probe,
> +	.remove		= __exit_p(ad5820_remove),
> +	.id_table	= ad5820_id_table,
> +};
> +
> +module_i2c_driver(ad5820_i2c_driver);
> +
> +MODULE_AUTHOR("Tuukka Toivonen <tuukka.o.toivonen@nokia.com>");

I think Tuukka's e-mail is wrong here. tuukkat76@gmail.com? Cc Tuukka.

> +MODULE_DESCRIPTION("AD5820 camera lens driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/media/ad5820.h b/include/media/ad5820.h
> new file mode 100644
> index 0000000..2c28131
> --- /dev/null
> +++ b/include/media/ad5820.h
> @@ -0,0 +1,30 @@
> +/*
> + * AD5820 DAC driver for camera voice coil focus.
> + *
> + * Copyright (C) 2008 Nokia Corporation
> + * Copyright (C) 2007 Texas Instruments
> + *
> + * Contact: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + *          Sakari Ailus <sakari.ailus@iki.fi>
> + *
> + * 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.
> + */
> +
> +#ifndef __AD5820_H__
> +#define __AD5820_H__
> +
> +#define AD5820_NAME		"ad5820"
> +#define AD5820_I2C_ADDR		(0x18 >> 1)
> +
> +struct ad5820_platform_data {
> +	int xshutdown;
> +};
> +
> +#endif /* __AD5820_H__ */

The patch itself looks fine, but I think the bigger question is: what's the
interface for such low-level lens control chips?

Based on what I've seen in the past, the vast majority of these are very
simple but a yet diverse set of devices in terms of how they are controlled.
So until we've got more of them the private controls are probably best we
can do.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk