* [REVIEW PATCH 13/14] OMAP: CAM: Add Lens Driver
@ 2008-12-11 20:38 Aguirre Rodriguez, Sergio Alberto
2008-12-12 6:03 ` Alexey Klimov
2008-12-16 17:38 ` Hans Verkuil
0 siblings, 2 replies; 8+ messages in thread
From: Aguirre Rodriguez, Sergio Alberto @ 2008-12-11 20:38 UTC (permalink / raw)
To: linux-omap, video4linux-list
Cc: Sakari Ailus, Tuukka.O Toivonen, Nagalla, Hari
>From 1341b74f5a90e4aa079a4fcb4fe2127ff344cce7 Mon Sep 17 00:00:00 2001
From: Sergio Aguirre <saaguirre@ti.com>
Date: Thu, 11 Dec 2008 13:35:46 -0600
Subject: [PATCH] OMAP: CAM: Add Lens Driver
This adds the DW9710 Lens driver
Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
---
drivers/media/video/Kconfig | 8 +
drivers/media/video/Makefile | 1 +
drivers/media/video/dw9710.c | 569 ++++++++++++++++++++++++++++++++++++++++++
drivers/media/video/dw9710.h | 59 +++++
4 files changed, 637 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/dw9710.c
create mode 100644 drivers/media/video/dw9710.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 24957bc..50651f2 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -313,6 +313,14 @@ config VIDEO_MT9P012
MT9P012 camera. It is currently working with the TI OMAP3
camera controller.
+config VIDEO_DW9710
+ tristate "Lens driver for DW9710"
+ depends on I2C && VIDEO_V4L2
+ ---help---
+ This is a Video4Linux2 lens driver for the Dongwoon
+ DW9710 coil. It is currently working with the TI OMAP3
+ camera controller and micron MT9P012 sensor.
+
config VIDEO_SAA7110
tristate "Philips SAA7110 video decoder"
depends on VIDEO_V4L1 && I2C
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index f73b65c..41c71d5 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o
obj-$(CONFIG_VIDEO_MT9P012) += mt9p012.o
+obj-$(CONFIG_VIDEO_DW9710) += dw9710.o
obj-$(CONFIG_VIDEO_OMAP3) += omap34xxcam.o isp/
diff --git a/drivers/media/video/dw9710.c b/drivers/media/video/dw9710.c
new file mode 100644
index 0000000..b5d5247
--- /dev/null
+++ b/drivers/media/video/dw9710.c
@@ -0,0 +1,569 @@
+/*
+ * drivers/media/video/dw9710.c
+ *
+ * DW9710 Coil Motor (LENS) driver
+ *
+ * Copyright (C) 2008 Texas Instruments.
+ *
+ * Contributors:
+ * Troy Laramy <t-laramy@ti.com>
+ * Mohit Jalori <mjalori@ti.com>
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2. This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ *
+ */
+
+#include <linux/mutex.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+#include <media/v4l2-int-device.h>
+#include <mach/gpio.h>
+#include <linux/platform_device.h>
+#include <linux/cdev.h>
+#include <linux/device.h>
+
+#include "dw9710.h"
+
+#define DRIVER_NAME "dw9710"
+
+static int
+dw9710_probe(struct i2c_client *client, const struct i2c_device_id *id);
+static int __exit dw9710_remove(struct i2c_client *client);
+
+struct dw9710_device {
+ const struct dw9710_platform_data *pdata;
+ struct v4l2_int_device *v4l2_int_device;
+ struct i2c_client *i2c_client;
+ int opened;
+ u16 current_lens_posn;
+ u16 saved_lens_posn;
+ int state;
+ int power_state;
+};
+
+static const struct i2c_device_id dw9710_id[] = {
+ { DW9710_NAME, 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, dw9710_id);
+
+static struct i2c_driver dw9710_i2c_driver = {
+ .driver = {
+ .name = DW9710_NAME,
+ .owner = THIS_MODULE,
+ },
+ .probe = dw9710_probe,
+ .remove = __exit_p(dw9710_remove),
+ .id_table = dw9710_id,
+};
+
+static struct dw9710_device dw9710 = {
+ .state = LENS_NOT_DETECTED,
+ .current_lens_posn = DEF_LENS_POSN,
+};
+
+static struct vcontrol {
+ struct v4l2_queryctrl qc;
+ int current_value;
+} video_control[] = {
+ {
+ {
+ .id = V4L2_CID_FOCUS_ABSOLUTE,
+ .type = V4L2_CTRL_TYPE_INTEGER,
+ .name = "Lens Position",
+ .minimum = 0,
+ .maximum = MAX_FOCUS_POS,
+ .step = LENS_POSN_STEP,
+ .default_value = DEF_LENS_POSN,
+ },
+ .current_value = DEF_LENS_POSN,
+ }
+};
+
+static struct i2c_driver dw9710_i2c_driver;
+
+/**
+ * find_vctrl - Finds the requested ID in the video control structure array
+ * @id: ID of control to search the video control array for
+ *
+ * Returns the index of the requested ID from the control structure array
+ */
+static int
+find_vctrl(int id)
+{
+ int i;
+
+ if (id < V4L2_CID_BASE)
+ return -EDOM;
+
+ for (i = (ARRAY_SIZE(video_control) - 1); i >= 0; i--)
+ if (video_control[i].qc.id == id)
+ break;
+ if (i < 0)
+ i = -EINVAL;
+ return i;
+}
+
+/**
+ * camaf_reg_read - Reads a value from a register in DW9710 Coil driver device.
+ * @client: Pointer to structure of I2C client.
+ * @value: Pointer to u16 for returning value of register to read.
+ *
+ * Returns zero if successful, or non-zero otherwise.
+ **/
+static int camaf_reg_read(struct i2c_client *client, u16 *value)
+{
+ int err;
+ struct i2c_msg msg[1];
+ unsigned char data[2];
+
+ if (!client->adapter)
+ return -ENODEV;
+
+ msg->addr = client->addr;
+ msg->flags = I2C_M_RD;
+ msg->len = 2;
+ msg->buf = data;
+
+ data[0] = 0;
+ data[1] = 0;
+
+ err = i2c_transfer(client->adapter, msg, 1);
+
+ if (err >= 0) {
+ *value = err = ((data[0] & 0xFF) << 8) | (data[1]);
+ return 0;
+ }
+ return err;
+}
+
+/**
+ * camaf_reg_write - Writes a value to a register in DW9710 Coil driver device.
+ * @client: Pointer to structure of I2C client.
+ * @value: Value of register to write.
+ *
+ * Returns zero if successful, or non-zero otherwise.
+ **/
+static int camaf_reg_write(struct i2c_client *client, u16 value)
+{
+ int err;
+ struct i2c_msg msg[1];
+ unsigned char data[2];
+ int retry = 0;
+
+ if (!client->adapter)
+ return -ENODEV;
+
+again:
+ msg->addr = client->addr;
+ msg->flags = 0;
+ msg->len = 2;
+ msg->buf = data;
+
+ data[0] = (u8)(value >> 8);
+ data[1] = (u8)(value & 0xFF);
+
+ err = i2c_transfer(client->adapter, msg, 1);
+
+ if (err >= 0)
+ return 0;
+
+ if (retry <= DW9710_I2C_RETRY_COUNT) {
+ dev_dbg(&client->dev, "retry ... %d", retry);
+ retry++;
+ set_current_state(TASK_UNINTERRUPTIBLE);
+ schedule_timeout(msecs_to_jiffies(20));
+ goto again;
+ }
+ return err;
+}
+
+/**
+ * dw9710_detect - Detects DW9710 Coil driver device.
+ * @client: Pointer to structure of I2C client.
+ *
+ * Returns 0 if successful, -1 if camera is off or if test register value
+ * wasn't stored properly, or returned errors from either camaf_reg_write or
+ * camaf_reg_read functions.
+ **/
+static int dw9710_detect(struct i2c_client *client)
+{
+ int err = 0;
+ u16 wposn = 0, rposn = 0;
+ u16 posn = 0x05;
+
+ wposn = (CAMAF_DW9710_POWERDN(CAMAF_DW9710_ENABLE) |
+ CAMAF_DW9710_DATA(posn));
+
+ err = camaf_reg_write(client, wposn);
+ if (err) {
+ printk(KERN_ERR "Unable to write DW9710 \n");
+ return err;
+ }
+
+ err = camaf_reg_read(client, &rposn);
+ if (err) {
+ printk(KERN_ERR "Unable to read DW9710 \n");
+ return err;
+ }
+
+ if (wposn != rposn) {
+ printk(KERN_ERR "W/R MISMATCH!\n");
+ return -1;
+ }
+ posn = 0;
+ wposn = (CAMAF_DW9710_POWERDN(CAMAF_DW9710_ENABLE) |
+ CAMAF_DW9710_DATA(posn));
+ err = camaf_reg_write(client, wposn);
+
+ return err;
+}
+
+/**
+ * dw9710_af_setfocus - Sets the desired focus.
+ * @posn: Desired focus position, 0 (far) - 100 (close).
+ *
+ * Returns 0 on success, -EINVAL if camera is off or focus value is out of
+ * bounds, or returned errors from either camaf_reg_write or camaf_reg_read
+ * functions.
+ **/
+int dw9710_af_setfocus(u16 posn)
+{
+ struct dw9710_device *af_dev = &dw9710;
+ struct i2c_client *client = af_dev->i2c_client;
+ u16 cur_focus_value = 0;
+ int ret = -EINVAL;
+
+ if (posn > MAX_FOCUS_POS) {
+ printk(KERN_ERR "Bad posn params 0x%x \n", posn);
+ return ret;
+ }
+
+ if ((af_dev->power_state == V4L2_POWER_OFF) ||
+ (af_dev->power_state == V4L2_POWER_STANDBY)) {
+ af_dev->current_lens_posn = posn;
+ return 0;
+ }
+
+ ret = camaf_reg_read(client, &cur_focus_value);
+
+ if (ret) {
+ printk(KERN_ERR "Read of current Lens position failed\n");
+ return ret;
+ }
+
+ if (CAMAF_DW9710_DATA_R(cur_focus_value) == posn) {
+ printk(KERN_DEBUG "Device already in requested focal point\n");
+ return ret;
+ }
+
+ ret = camaf_reg_write(client,
+ CAMAF_DW9710_POWERDN(CAMAF_DW9710_ENABLE) |
+ CAMAF_DW9710_DATA(posn));
+
+ if (ret)
+ printk(KERN_ERR "Setfocus register write failed\n");
+ dw9710.current_lens_posn = posn;
+ return ret;
+}
+EXPORT_SYMBOL(dw9710_af_setfocus);
+
+/**
+ * dw9710_af_getfocus - Gets the focus value from device.
+ * @value: Pointer to u16 variable which will contain the focus value.
+ *
+ * Returns 0 if successful, -EINVAL if camera is off, or return value of
+ * camaf_reg_read if fails.
+ **/
+int dw9710_af_getfocus(u16 *value)
+{
+ int ret = -EINVAL;
+ u16 posn = 0;
+
+ struct dw9710_device *af_dev = &dw9710;
+ struct i2c_client *client = af_dev->i2c_client;
+
+ if ((af_dev->power_state == V4L2_POWER_OFF) ||
+ (af_dev->power_state == V4L2_POWER_STANDBY))
+ return ret;
+
+ ret = camaf_reg_read(client, &posn);
+
+ if (ret) {
+ printk(KERN_ERR "Read of current Lens position failed\n");
+ return ret;
+ }
+ *value = CAMAF_DW9710_DATA_R(posn);
+ dw9710.current_lens_posn = CAMAF_DW9710_DATA_R(posn);
+ return ret;
+}
+EXPORT_SYMBOL(dw9710_af_getfocus);
+
+/**
+ * ioctl_queryctrl - V4L2 lens interface handler for VIDIOC_QUERYCTRL ioctl
+ * @s: pointer to standard V4L2 device structure
+ * @qc: standard V4L2 VIDIOC_QUERYCTRL ioctl structure
+ *
+ * If the requested control is supported, returns the control information
+ * from the video_control[] array. Otherwise, returns -EINVAL if the
+ * control is not supported.
+ */
+static int ioctl_queryctrl(struct v4l2_int_device *s,
+ struct v4l2_queryctrl *qc)
+{
+ int i;
+
+ i = find_vctrl(qc->id);
+ if (i == -EINVAL)
+ qc->flags = V4L2_CTRL_FLAG_DISABLED;
+
+ if (i < 0)
+ return -EINVAL;
+
+ *qc = video_control[i].qc;
+ return 0;
+}
+
+/**
+ * ioctl_g_ctrl - V4L2 DW9710 lens interface handler for VIDIOC_G_CTRL ioctl
+ * @s: pointer to standard V4L2 device structure
+ * @vc: standard V4L2 VIDIOC_G_CTRL ioctl structure
+ *
+ * If the requested control is supported, returns the control's current
+ * value from the video_control[] array. Otherwise, returns -EINVAL
+ * if the control is not supported.
+ */
+static int ioctl_g_ctrl(struct v4l2_int_device *s,
+ struct v4l2_control *vc)
+{
+ struct vcontrol *lvc;
+ int i;
+ u16 curr_posn;
+
+ i = find_vctrl(vc->id);
+ if (i < 0)
+ return -EINVAL;
+ lvc = &video_control[i];
+
+ switch (vc->id) {
+ case V4L2_CID_FOCUS_ABSOLUTE:
+ if (dw9710_af_getfocus(&curr_posn))
+ return -EFAULT;
+ vc->value = curr_posn;
+ lvc->current_value = curr_posn;
+ break;
+ }
+
+ return 0;
+}
+
+/**
+ * ioctl_s_ctrl - V4L2 DW9710 lens interface handler for VIDIOC_S_CTRL ioctl
+ * @s: pointer to standard V4L2 device structure
+ * @vc: standard V4L2 VIDIOC_S_CTRL ioctl structure
+ *
+ * If the requested control is supported, sets the control's current
+ * value in HW (and updates the video_control[] array). Otherwise,
+ * returns -EINVAL if the control is not supported.
+ */
+static int ioctl_s_ctrl(struct v4l2_int_device *s,
+ struct v4l2_control *vc)
+{
+ int retval = -EINVAL;
+ int i;
+ struct vcontrol *lvc;
+
+ i = find_vctrl(vc->id);
+ if (i < 0)
+ return -EINVAL;
+ lvc = &video_control[i];
+
+ switch (vc->id) {
+ case V4L2_CID_FOCUS_ABSOLUTE:
+ retval = dw9710_af_setfocus(vc->value);
+ if (!retval)
+ lvc->current_value = vc->value;
+ break;
+ }
+
+ return retval;
+}
+
+/**
+ * ioctl_g_priv - V4L2 sensor interface handler for vidioc_int_g_priv_num
+ * @s: pointer to standard V4L2 device structure
+ * @p: void pointer to hold sensor's private data address
+ *
+ * Returns device's (sensor's) private data area address in p parameter
+ */
+static int ioctl_g_priv(struct v4l2_int_device *s, void *p)
+{
+ struct dw9710_device *lens = s->priv;
+
+ return lens->pdata->priv_data_set(p);
+
+}
+
+/**
+ * ioctl_s_power - V4L2 sensor interface handler for vidioc_int_s_power_num
+ * @s: pointer to standard V4L2 device structure
+ * @on: power state to which device is to be set
+ *
+ * Sets devices power state to requrested state, if possible.
+ */
+static int ioctl_s_power(struct v4l2_int_device *s, enum v4l2_power on)
+{
+ struct dw9710_device *lens = s->priv;
+ struct i2c_client *c = lens->i2c_client;
+ int rval;
+
+ rval = lens->pdata->power_set(on);
+ if (rval < 0) {
+ dev_err(&c->dev, "Unable to set the power state: " DRIVER_NAME
+ " lens HW\n");
+ return rval;
+ }
+
+ if ((on == V4L2_POWER_ON) && (lens->state == LENS_NOT_DETECTED)) {
+ rval = dw9710_detect(c);
+ if (rval < 0) {
+ dev_err(&c->dev, "Unable to detect "
+ DRIVER_NAME " lens HW\n");
+ printk(KERN_ERR "Unable to detect "
+ DRIVER_NAME " lens HW\n");
+ lens->state = LENS_NOT_DETECTED;
+ return rval;
+ }
+ lens->state = LENS_DETECTED;
+ pr_info(DRIVER_NAME " lens HW detected\n");
+ }
+
+ if ((lens->power_state == V4L2_POWER_STANDBY) && (on == V4L2_POWER_ON)
+ && (lens->state == LENS_DETECTED))
+ dw9710_af_setfocus(lens->current_lens_posn);
+
+ lens->power_state = on;
+ return 0;
+}
+
+static struct v4l2_int_ioctl_desc dw9710_ioctl_desc[] = {
+ { .num = vidioc_int_s_power_num,
+ .func = (v4l2_int_ioctl_func *)ioctl_s_power },
+ { .num = vidioc_int_g_priv_num,
+ .func = (v4l2_int_ioctl_func *)ioctl_g_priv },
+ { .num = vidioc_int_queryctrl_num,
+ .func = (v4l2_int_ioctl_func *)ioctl_queryctrl },
+ { .num = vidioc_int_g_ctrl_num,
+ .func = (v4l2_int_ioctl_func *)ioctl_g_ctrl },
+ { .num = vidioc_int_s_ctrl_num,
+ .func = (v4l2_int_ioctl_func *)ioctl_s_ctrl },
+};
+
+static struct v4l2_int_slave dw9710_slave = {
+ .ioctls = dw9710_ioctl_desc,
+ .num_ioctls = ARRAY_SIZE(dw9710_ioctl_desc),
+};
+
+static struct v4l2_int_device dw9710_int_device = {
+ .module = THIS_MODULE,
+ .name = DRIVER_NAME,
+ .priv = &dw9710,
+ .type = v4l2_int_type_slave,
+ .u = {
+ .slave = &dw9710_slave,
+ },
+};
+
+/**
+ * dw9710_probe - Probes the driver for valid I2C attachment.
+ * @client: Pointer to structure of I2C client.
+ *
+ * Returns 0 if successful, or -EBUSY if unable to get client attached data.
+ **/
+static int
+dw9710_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+ struct dw9710_device *lens = &dw9710;
+ int err;
+
+ dev_info(&client->dev, "dw9710 probe called....\n");
+
+ if (i2c_get_clientdata(client)) {
+ printk(KERN_ERR " DTA BUSY %s\n", client->name);
+ return -EBUSY;
+ }
+
+ lens->pdata = client->dev.platform_data;
+
+ if (!lens->pdata) {
+ dev_err(&client->dev, "no platform data?\n");
+ return -ENODEV;
+ }
+
+ lens->v4l2_int_device = &dw9710_int_device;
+
+ lens->i2c_client = client;
+ i2c_set_clientdata(client, lens);
+
+ err = v4l2_int_device_register(lens->v4l2_int_device);
+ if (err) {
+ printk(KERN_ERR "Failed to Register "
+ DRIVER_NAME " as V4L2 device.\n");
+ i2c_set_clientdata(client, NULL);
+ } else {
+ printk(KERN_ERR "Registered "
+ DRIVER_NAME " as V4L2 device.\n");
+ }
+
+ return 0;
+}
+
+/**
+ * dw9710_remove - Routine when device its unregistered from I2C
+ * @client: Pointer to structure of I2C client.
+ *
+ * Returns 0 if successful, or -ENODEV if the client isn't attached.
+ **/
+static int __exit dw9710_remove(struct i2c_client *client)
+{
+ if (!client->adapter)
+ return -ENODEV;
+
+ i2c_set_clientdata(client, NULL);
+ return 0;
+}
+
+/**
+ * dw9710_init - Module initialisation.
+ *
+ * Returns 0 if successful, or -EINVAL if device couldn't be initialized, or
+ * added as a character device.
+ **/
+static int __init dw9710_init(void)
+{
+ int err = -EINVAL;
+
+ err = i2c_add_driver(&dw9710_i2c_driver);
+ if (err)
+ goto fail;
+ return err;
+fail:
+ printk(KERN_ERR "Failed to register " DRIVER_NAME ".\n");
+ return err;
+}
+late_initcall(dw9710_init);
+
+/**
+ * dw9710_cleanup - Module cleanup.
+ **/
+static void __exit dw9710_cleanup(void)
+{
+ i2c_del_driver(&dw9710_i2c_driver);
+}
+module_exit(dw9710_cleanup);
+
+MODULE_AUTHOR("Texas Instruments");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("DW9710 LENS driver");
diff --git a/drivers/media/video/dw9710.h b/drivers/media/video/dw9710.h
new file mode 100644
index 0000000..e4050a4
--- /dev/null
+++ b/drivers/media/video/dw9710.h
@@ -0,0 +1,59 @@
+/*
+ * drivers/media/video/dw9710.h
+ *
+ * Register defines for Auto Focus device
+ *
+ * Copyright (C) 2008 Texas Instruments.
+ *
+ * Contributors:
+ * Troy Laramy <t-laramy@ti.com>
+ * Mohit Jalori <mjalori@ti.com>
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2. This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ *
+ */
+
+#ifndef CAMAF_DW9710_H
+#define CAMAF_DW9710_H
+
+#define DW9710_AF_I2C_ADDR 0x0C
+#define DW9710_NAME "DW9710"
+#define DW9710_I2C_RETRY_COUNT 5
+#define MAX_FOCUS_POS 0xFF
+
+#define CAMAF_DW9710_DISABLE 0x1
+#define CAMAF_DW9710_ENABLE 0x0
+#define CAMAF_DW9710_POWERDN(ARG) (((ARG) & 0x1) << 15)
+#define CAMAF_DW9710_POWERDN_R(ARG) (((ARG) >> 15) & 0x1)
+
+#define CAMAF_DW9710_DATA(ARG) (((ARG) & 0xFF) << 6)
+#define CAMAF_DW9710_DATA_R(ARG) (((ARG) >> 6) & 0xFF)
+#define CAMAF_FREQUENCY_EQ1(mclk) ((u16)(mclk/16000))
+
+/* State of lens */
+#define LENS_DETECTED 1
+#define LENS_NOT_DETECTED 0
+
+/* Focus control values */
+#define DEF_LENS_POSN 0 /* 0x7F */
+#define LENS_POSN_STEP 1
+
+
+/**
+ * struct dw9710_platform_data - platform data values and access functions
+ * @power_set: Power state access function, zero is off, non-zero is on.
+ * @priv_data_set: device private data (pointer) access function
+ */
+struct dw9710_platform_data {
+ int (*power_set)(enum v4l2_power power);
+ int (*priv_data_set)(void *);
+};
+
+/*
+ * Sets the specified focus value [0(far) - 100(near)]
+ */
+int dw9710_af_setfocus(u16 posn);
+int dw9710_af_getfocus(u16 *value);
+#endif /* End of of CAMAF_DW9710_H */
--
1.5.6.5
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [REVIEW PATCH 13/14] OMAP: CAM: Add Lens Driver
2008-12-11 20:38 [REVIEW PATCH 13/14] OMAP: CAM: Add Lens Driver Aguirre Rodriguez, Sergio Alberto
@ 2008-12-12 6:03 ` Alexey Klimov
2008-12-16 17:38 ` Hans Verkuil
1 sibling, 0 replies; 8+ messages in thread
From: Alexey Klimov @ 2008-12-12 6:03 UTC (permalink / raw)
To: Aguirre Rodriguez, Sergio Alberto
Cc: video4linux-list, linux-omap, Sakari Ailus, Tuukka.O Toivonen,
Nagalla, Hari
Hello, all
Looked through the patch and have small suggestions, it's about coding
style and releted.
On Thu, Dec 11, 2008 at 11:38 PM, Aguirre Rodriguez, Sergio Alberto
<saaguirre@ti.com> wrote:
> >From 1341b74f5a90e4aa079a4fcb4fe2127ff344cce7 Mon Sep 17 00:00:00 2001
> From: Sergio Aguirre <saaguirre@ti.com>
> Date: Thu, 11 Dec 2008 13:35:46 -0600
> Subject: [PATCH] OMAP: CAM: Add Lens Driver
>
> This adds the DW9710 Lens driver
>
> Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
> ---
> drivers/media/video/Kconfig | 8 +
> drivers/media/video/Makefile | 1 +
> drivers/media/video/dw9710.c | 569 ++++++++++++++++++++++++++++++++++++++++++
> drivers/media/video/dw9710.h | 59 +++++
> 4 files changed, 637 insertions(+), 0 deletions(-)
> create mode 100644 drivers/media/video/dw9710.c
> create mode 100644 drivers/media/video/dw9710.h
>
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index 24957bc..50651f2 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -313,6 +313,14 @@ config VIDEO_MT9P012
> MT9P012 camera. It is currently working with the TI OMAP3
> camera controller.
>
> +config VIDEO_DW9710
> + tristate "Lens driver for DW9710"
> + depends on I2C && VIDEO_V4L2
> + ---help---
> + This is a Video4Linux2 lens driver for the Dongwoon
> + DW9710 coil. It is currently working with the TI OMAP3
> + camera controller and micron MT9P012 sensor.
> +
> config VIDEO_SAA7110
> tristate "Philips SAA7110 video decoder"
> depends on VIDEO_V4L1 && I2C
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index f73b65c..41c71d5 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
>
> obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o
> obj-$(CONFIG_VIDEO_MT9P012) += mt9p012.o
> +obj-$(CONFIG_VIDEO_DW9710) += dw9710.o
>
> obj-$(CONFIG_VIDEO_OMAP3) += omap34xxcam.o isp/
>
> diff --git a/drivers/media/video/dw9710.c b/drivers/media/video/dw9710.c
> new file mode 100644
> index 0000000..b5d5247
> --- /dev/null
> +++ b/drivers/media/video/dw9710.c
> @@ -0,0 +1,569 @@
> +/*
> + * drivers/media/video/dw9710.c
> + *
> + * DW9710 Coil Motor (LENS) driver
> + *
> + * Copyright (C) 2008 Texas Instruments.
> + *
> + * Contributors:
> + * Troy Laramy <t-laramy@ti.com>
> + * Mohit Jalori <mjalori@ti.com>
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2. This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + *
> + */
> +
> +#include <linux/mutex.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <media/v4l2-int-device.h>
> +#include <mach/gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +
> +#include "dw9710.h"
> +
> +#define DRIVER_NAME "dw9710"
Here you define DRIVER_NAME. I saw patch(for another module) that
remover MODULE_NAME and places VIVI_MODULE_NAME there. Here it is:
http://linuxtv.org/hg/v4l-dvb/rev/58b95134acb8
May be it's better to define something like this: #define
DW9710_DRIVER_NAME "dw9710" ?
Probably it will help you to avoid possible namespace conflicts and
code become a bit more readible.
> +static int
> +dw9710_probe(struct i2c_client *client, const struct i2c_device_id *id);
> +static int __exit dw9710_remove(struct i2c_client *client);
> +
> +struct dw9710_device {
> + const struct dw9710_platform_data *pdata;
> + struct v4l2_int_device *v4l2_int_device;
> + struct i2c_client *i2c_client;
> + int opened;
> + u16 current_lens_posn;
> + u16 saved_lens_posn;
> + int state;
> + int power_state;
> +};
> +
> +static const struct i2c_device_id dw9710_id[] = {
> + { DW9710_NAME, 0 },
> + { }
> +};
Hmmm, looks like you already defined DW9710_NAME in header-file. Why
didn't you reformat to use _one_ define for this and previous place ?
> +MODULE_DEVICE_TABLE(i2c, dw9710_id);
> +
> +static struct i2c_driver dw9710_i2c_driver = {
> + .driver = {
> + .name = DW9710_NAME,
Actually, the same thing here.
> + .owner = THIS_MODULE,
> + },
> + .probe = dw9710_probe,
> + .remove = __exit_p(dw9710_remove),
> + .id_table = dw9710_id,
> +};
> +
> +static struct dw9710_device dw9710 = {
> + .state = LENS_NOT_DETECTED,
> + .current_lens_posn = DEF_LENS_POSN,
> +};
> +
> +static struct vcontrol {
> + struct v4l2_queryctrl qc;
> + int current_value;
> +} video_control[] = {
> + {
> + {
> + .id = V4L2_CID_FOCUS_ABSOLUTE,
> + .type = V4L2_CTRL_TYPE_INTEGER,
> + .name = "Lens Position",
> + .minimum = 0,
> + .maximum = MAX_FOCUS_POS,
> + .step = LENS_POSN_STEP,
> + .default_value = DEF_LENS_POSN,
> + },
> + .current_value = DEF_LENS_POSN,
> + }
> +};
> +
> +static struct i2c_driver dw9710_i2c_driver;
> +
> +/**
> + * find_vctrl - Finds the requested ID in the video control structure array
> + * @id: ID of control to search the video control array for
> + *
> + * Returns the index of the requested ID from the control structure array
> + */
> +static int
> +find_vctrl(int id)
> +{
> + int i;
> +
> + if (id < V4L2_CID_BASE)
> + return -EDOM;
> +
> + for (i = (ARRAY_SIZE(video_control) - 1); i >= 0; i--)
> + if (video_control[i].qc.id == id)
> + break;
> + if (i < 0)
> + i = -EINVAL;
> + return i;
> +}
In this function you use return after if, and in second place you set
i equals to -EINVAL, and then return. Probably, to make it more easily
to read it's better to do such thing:
if (i < 0)
return -EINVAL;
> +/**
> + * camaf_reg_read - Reads a value from a register in DW9710 Coil driver device.
> + * @client: Pointer to structure of I2C client.
> + * @value: Pointer to u16 for returning value of register to read.
> + *
> + * Returns zero if successful, or non-zero otherwise.
> + **/
> +static int camaf_reg_read(struct i2c_client *client, u16 *value)
> +{
> + int err;
> + struct i2c_msg msg[1];
> + unsigned char data[2];
> +
> + if (!client->adapter)
> + return -ENODEV;
> +
> + msg->addr = client->addr;
> + msg->flags = I2C_M_RD;
> + msg->len = 2;
> + msg->buf = data;
> +
> + data[0] = 0;
> + data[1] = 0;
> +
> + err = i2c_transfer(client->adapter, msg, 1);
> +
> + if (err >= 0) {
> + *value = err = ((data[0] & 0xFF) << 8) | (data[1]);
I didn't ever see two "=" in one line in code. Is it okay ?
> + return 0;
> + }
> + return err;
> +}
> +
> +/**
> + * camaf_reg_write - Writes a value to a register in DW9710 Coil driver device.
> + * @client: Pointer to structure of I2C client.
> + * @value: Value of register to write.
> + *
> + * Returns zero if successful, or non-zero otherwise.
> + **/
> +static int camaf_reg_write(struct i2c_client *client, u16 value)
> +{
> + int err;
> + struct i2c_msg msg[1];
> + unsigned char data[2];
> + int retry = 0;
> +
> + if (!client->adapter)
> + return -ENODEV;
> +
> +again:
> + msg->addr = client->addr;
> + msg->flags = 0;
> + msg->len = 2;
> + msg->buf = data;
> +
> + data[0] = (u8)(value >> 8);
> + data[1] = (u8)(value & 0xFF);
> +
> + err = i2c_transfer(client->adapter, msg, 1);
> +
> + if (err >= 0)
> + return 0;
> +
> + if (retry <= DW9710_I2C_RETRY_COUNT) {
> + dev_dbg(&client->dev, "retry ... %d", retry);
> + retry++;
> + set_current_state(TASK_UNINTERRUPTIBLE);
> + schedule_timeout(msecs_to_jiffies(20));
> + goto again;
> + }
> + return err;
> +}
> +
> +/**
> + * dw9710_detect - Detects DW9710 Coil driver device.
> + * @client: Pointer to structure of I2C client.
> + *
> + * Returns 0 if successful, -1 if camera is off or if test register value
> + * wasn't stored properly, or returned errors from either camaf_reg_write or
> + * camaf_reg_read functions.
> + **/
> +static int dw9710_detect(struct i2c_client *client)
> +{
> + int err = 0;
> + u16 wposn = 0, rposn = 0;
> + u16 posn = 0x05;
> +
> + wposn = (CAMAF_DW9710_POWERDN(CAMAF_DW9710_ENABLE) |
> + CAMAF_DW9710_DATA(posn));
> +
> + err = camaf_reg_write(client, wposn);
> + if (err) {
> + printk(KERN_ERR "Unable to write DW9710 \n");
> + return err;
> + }
> +
> + err = camaf_reg_read(client, &rposn);
> + if (err) {
> + printk(KERN_ERR "Unable to read DW9710 \n");
> + return err;
> + }
> +
> + if (wposn != rposn) {
> + printk(KERN_ERR "W/R MISMATCH!\n");
If I were writing it, I'd do the following:
printk(KERN_ERR DRIVER_NAME "W/R MISMATCH!\n");
And driver-name have to be unique, see above.
To be onest it's better to provide module name to dmesg if you(or any
user) want to catch bad behavour.
> + return -1;
> + }
> + posn = 0;
> + wposn = (CAMAF_DW9710_POWERDN(CAMAF_DW9710_ENABLE) |
> + CAMAF_DW9710_DATA(posn));
> + err = camaf_reg_write(client, wposn);
> +
> + return err;
> +}
> +
> +/**
> + * dw9710_af_setfocus - Sets the desired focus.
> + * @posn: Desired focus position, 0 (far) - 100 (close).
> + *
> + * Returns 0 on success, -EINVAL if camera is off or focus value is out of
> + * bounds, or returned errors from either camaf_reg_write or camaf_reg_read
> + * functions.
> + **/
> +int dw9710_af_setfocus(u16 posn)
> +{
> + struct dw9710_device *af_dev = &dw9710;
> + struct i2c_client *client = af_dev->i2c_client;
> + u16 cur_focus_value = 0;
> + int ret = -EINVAL;
> +
> + if (posn > MAX_FOCUS_POS) {
> + printk(KERN_ERR "Bad posn params 0x%x \n", posn);
> + return ret;
> + }
> +
> + if ((af_dev->power_state == V4L2_POWER_OFF) ||
> + (af_dev->power_state == V4L2_POWER_STANDBY)) {
> + af_dev->current_lens_posn = posn;
> + return 0;
> + }
> +
> + ret = camaf_reg_read(client, &cur_focus_value);
> +
> + if (ret) {
> + printk(KERN_ERR "Read of current Lens position failed\n");
> + return ret;
> + }
> +
> + if (CAMAF_DW9710_DATA_R(cur_focus_value) == posn) {
> + printk(KERN_DEBUG "Device already in requested focal point\n");
> + return ret;
> + }
> +
> + ret = camaf_reg_write(client,
> + CAMAF_DW9710_POWERDN(CAMAF_DW9710_ENABLE) |
> + CAMAF_DW9710_DATA(posn));
> +
> + if (ret)
> + printk(KERN_ERR "Setfocus register write failed\n");
> + dw9710.current_lens_posn = posn;
> + return ret;
> +}
> +EXPORT_SYMBOL(dw9710_af_setfocus);
> +
> +/**
> + * dw9710_af_getfocus - Gets the focus value from device.
> + * @value: Pointer to u16 variable which will contain the focus value.
> + *
> + * Returns 0 if successful, -EINVAL if camera is off, or return value of
> + * camaf_reg_read if fails.
> + **/
> +int dw9710_af_getfocus(u16 *value)
> +{
> + int ret = -EINVAL;
> + u16 posn = 0;
Why just don't use int ret;
And later in code use return -EINVAL; ?
Anyway, you change ret variable with reg_read call.
> + struct dw9710_device *af_dev = &dw9710;
> + struct i2c_client *client = af_dev->i2c_client;
> +
> + if ((af_dev->power_state == V4L2_POWER_OFF) ||
> + (af_dev->power_state == V4L2_POWER_STANDBY))
> + return ret;
> +
> + ret = camaf_reg_read(client, &posn);
> +
> + if (ret) {
> + printk(KERN_ERR "Read of current Lens position failed\n");
> + return ret;
> + }
> + *value = CAMAF_DW9710_DATA_R(posn);
> + dw9710.current_lens_posn = CAMAF_DW9710_DATA_R(posn);
> + return ret;
> +}
> +EXPORT_SYMBOL(dw9710_af_getfocus);
> +
> +/**
> + * ioctl_queryctrl - V4L2 lens interface handler for VIDIOC_QUERYCTRL ioctl
> + * @s: pointer to standard V4L2 device structure
> + * @qc: standard V4L2 VIDIOC_QUERYCTRL ioctl structure
> + *
> + * If the requested control is supported, returns the control information
> + * from the video_control[] array. Otherwise, returns -EINVAL if the
> + * control is not supported.
> + */
> +static int ioctl_queryctrl(struct v4l2_int_device *s,
> + struct v4l2_queryctrl *qc)
> +{
> + int i;
> +
> + i = find_vctrl(qc->id);
> + if (i == -EINVAL)
> + qc->flags = V4L2_CTRL_FLAG_DISABLED;
> +
> + if (i < 0)
> + return -EINVAL;
> +
> + *qc = video_control[i].qc;
> + return 0;
> +}
> +
> +/**
> + * ioctl_g_ctrl - V4L2 DW9710 lens interface handler for VIDIOC_G_CTRL ioctl
> + * @s: pointer to standard V4L2 device structure
> + * @vc: standard V4L2 VIDIOC_G_CTRL ioctl structure
> + *
> + * If the requested control is supported, returns the control's current
> + * value from the video_control[] array. Otherwise, returns -EINVAL
> + * if the control is not supported.
> + */
> +static int ioctl_g_ctrl(struct v4l2_int_device *s,
> + struct v4l2_control *vc)
> +{
> + struct vcontrol *lvc;
> + int i;
> + u16 curr_posn;
> +
> + i = find_vctrl(vc->id);
> + if (i < 0)
> + return -EINVAL;
> + lvc = &video_control[i];
> +
> + switch (vc->id) {
> + case V4L2_CID_FOCUS_ABSOLUTE:
> + if (dw9710_af_getfocus(&curr_posn))
> + return -EFAULT;
> + vc->value = curr_posn;
> + lvc->current_value = curr_posn;
> + break;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * ioctl_s_ctrl - V4L2 DW9710 lens interface handler for VIDIOC_S_CTRL ioctl
> + * @s: pointer to standard V4L2 device structure
> + * @vc: standard V4L2 VIDIOC_S_CTRL ioctl structure
> + *
> + * If the requested control is supported, sets the control's current
> + * value in HW (and updates the video_control[] array). Otherwise,
> + * returns -EINVAL if the control is not supported.
> + */
> +static int ioctl_s_ctrl(struct v4l2_int_device *s,
> + struct v4l2_control *vc)
> +{
> + int retval = -EINVAL;
> + int i;
> + struct vcontrol *lvc;
> +
> + i = find_vctrl(vc->id);
> + if (i < 0)
> + return -EINVAL;
> + lvc = &video_control[i];
> +
> + switch (vc->id) {
> + case V4L2_CID_FOCUS_ABSOLUTE:
> + retval = dw9710_af_setfocus(vc->value);
> + if (!retval)
> + lvc->current_value = vc->value;
> + break;
> + }
> +
> + return retval;
> +}
> +
> +/**
> + * ioctl_g_priv - V4L2 sensor interface handler for vidioc_int_g_priv_num
> + * @s: pointer to standard V4L2 device structure
> + * @p: void pointer to hold sensor's private data address
> + *
> + * Returns device's (sensor's) private data area address in p parameter
> + */
> +static int ioctl_g_priv(struct v4l2_int_device *s, void *p)
> +{
> + struct dw9710_device *lens = s->priv;
> +
> + return lens->pdata->priv_data_set(p);
> +
> +}
> +
> +/**
> + * ioctl_s_power - V4L2 sensor interface handler for vidioc_int_s_power_num
> + * @s: pointer to standard V4L2 device structure
> + * @on: power state to which device is to be set
> + *
> + * Sets devices power state to requrested state, if possible.
> + */
> +static int ioctl_s_power(struct v4l2_int_device *s, enum v4l2_power on)
> +{
> + struct dw9710_device *lens = s->priv;
> + struct i2c_client *c = lens->i2c_client;
> + int rval;
> +
> + rval = lens->pdata->power_set(on);
> + if (rval < 0) {
> + dev_err(&c->dev, "Unable to set the power state: " DRIVER_NAME
> + " lens HW\n");
> + return rval;
> + }
> +
> + if ((on == V4L2_POWER_ON) && (lens->state == LENS_NOT_DETECTED)) {
> + rval = dw9710_detect(c);
> + if (rval < 0) {
> + dev_err(&c->dev, "Unable to detect "
> + DRIVER_NAME " lens HW\n");
> + printk(KERN_ERR "Unable to detect "
> + DRIVER_NAME " lens HW\n");
Two thing here. As i know it's more preferable to use dev_macro
instead of printk if possible.
As i know(may be i'm wrong) it's not good to use DRIVER_NAME in dev_err here.
Why do you print the same text two times here ?
> + lens->state = LENS_NOT_DETECTED;
> + return rval;
> + }
> + lens->state = LENS_DETECTED;
> + pr_info(DRIVER_NAME " lens HW detected\n");
> + }
> +
> + if ((lens->power_state == V4L2_POWER_STANDBY) && (on == V4L2_POWER_ON)
> + && (lens->state == LENS_DETECTED))
> + dw9710_af_setfocus(lens->current_lens_posn);
> +
> + lens->power_state = on;
> + return 0;
> +}
> +
> +static struct v4l2_int_ioctl_desc dw9710_ioctl_desc[] = {
> + { .num = vidioc_int_s_power_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_s_power },
> + { .num = vidioc_int_g_priv_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_g_priv },
> + { .num = vidioc_int_queryctrl_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_queryctrl },
> + { .num = vidioc_int_g_ctrl_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_g_ctrl },
> + { .num = vidioc_int_s_ctrl_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_s_ctrl },
> +};
> +
> +static struct v4l2_int_slave dw9710_slave = {
> + .ioctls = dw9710_ioctl_desc,
> + .num_ioctls = ARRAY_SIZE(dw9710_ioctl_desc),
> +};
> +
> +static struct v4l2_int_device dw9710_int_device = {
> + .module = THIS_MODULE,
> + .name = DRIVER_NAME,
> + .priv = &dw9710,
> + .type = v4l2_int_type_slave,
> + .u = {
> + .slave = &dw9710_slave,
> + },
> +};
> +
> +/**
> + * dw9710_probe - Probes the driver for valid I2C attachment.
> + * @client: Pointer to structure of I2C client.
> + *
> + * Returns 0 if successful, or -EBUSY if unable to get client attached data.
> + **/
> +static int
> +dw9710_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> + struct dw9710_device *lens = &dw9710;
> + int err;
> +
> + dev_info(&client->dev, "dw9710 probe called....\n");
> +
> + if (i2c_get_clientdata(client)) {
> + printk(KERN_ERR " DTA BUSY %s\n", client->name);
> + return -EBUSY;
> + }
> +
> + lens->pdata = client->dev.platform_data;
> +
> + if (!lens->pdata) {
> + dev_err(&client->dev, "no platform data?\n");
> + return -ENODEV;
> + }
> +
> + lens->v4l2_int_device = &dw9710_int_device;
> +
> + lens->i2c_client = client;
> + i2c_set_clientdata(client, lens);
> +
> + err = v4l2_int_device_register(lens->v4l2_int_device);
> + if (err) {
> + printk(KERN_ERR "Failed to Register "
> + DRIVER_NAME " as V4L2 device.\n");
> + i2c_set_clientdata(client, NULL);
> + } else {
> + printk(KERN_ERR "Registered "
> + DRIVER_NAME " as V4L2 device.\n");
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * dw9710_remove - Routine when device its unregistered from I2C
> + * @client: Pointer to structure of I2C client.
> + *
> + * Returns 0 if successful, or -ENODEV if the client isn't attached.
> + **/
> +static int __exit dw9710_remove(struct i2c_client *client)
> +{
> + if (!client->adapter)
> + return -ENODEV;
> +
> + i2c_set_clientdata(client, NULL);
> + return 0;
> +}
> +
> +/**
> + * dw9710_init - Module initialisation.
> + *
> + * Returns 0 if successful, or -EINVAL if device couldn't be initialized, or
> + * added as a character device.
> + **/
> +static int __init dw9710_init(void)
> +{
> + int err = -EINVAL;
int err = something, and then changed with call to i2c..
This is all i can suggest, it's not about functionality, it's about
coding style..
Please, use dev_macros if possible instead of printk.
> + err = i2c_add_driver(&dw9710_i2c_driver);
> + if (err)
> + goto fail;
> + return err;
> +fail:
> + printk(KERN_ERR "Failed to register " DRIVER_NAME ".\n");
> + return err;
> +}
> +late_initcall(dw9710_init);
> +
> +/**
> + * dw9710_cleanup - Module cleanup.
> + **/
> +static void __exit dw9710_cleanup(void)
> +{
> + i2c_del_driver(&dw9710_i2c_driver);
> +}
> +module_exit(dw9710_cleanup);
> +
> +MODULE_AUTHOR("Texas Instruments");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("DW9710 LENS driver");
> diff --git a/drivers/media/video/dw9710.h b/drivers/media/video/dw9710.h
> new file mode 100644
> index 0000000..e4050a4
> --- /dev/null
> +++ b/drivers/media/video/dw9710.h
> @@ -0,0 +1,59 @@
> +/*
> + * drivers/media/video/dw9710.h
> + *
> + * Register defines for Auto Focus device
> + *
> + * Copyright (C) 2008 Texas Instruments.
> + *
> + * Contributors:
> + * Troy Laramy <t-laramy@ti.com>
> + * Mohit Jalori <mjalori@ti.com>
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2. This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + *
> + */
> +
> +#ifndef CAMAF_DW9710_H
> +#define CAMAF_DW9710_H
> +
> +#define DW9710_AF_I2C_ADDR 0x0C
> +#define DW9710_NAME "DW9710"
> +#define DW9710_I2C_RETRY_COUNT 5
> +#define MAX_FOCUS_POS 0xFF
> +
> +#define CAMAF_DW9710_DISABLE 0x1
> +#define CAMAF_DW9710_ENABLE 0x0
> +#define CAMAF_DW9710_POWERDN(ARG) (((ARG) & 0x1) << 15)
> +#define CAMAF_DW9710_POWERDN_R(ARG) (((ARG) >> 15) & 0x1)
> +
> +#define CAMAF_DW9710_DATA(ARG) (((ARG) & 0xFF) << 6)
> +#define CAMAF_DW9710_DATA_R(ARG) (((ARG) >> 6) & 0xFF)
> +#define CAMAF_FREQUENCY_EQ1(mclk) ((u16)(mclk/16000))
> +
> +/* State of lens */
> +#define LENS_DETECTED 1
> +#define LENS_NOT_DETECTED 0
> +
> +/* Focus control values */
> +#define DEF_LENS_POSN 0 /* 0x7F */
> +#define LENS_POSN_STEP 1
> +
> +
> +/**
> + * struct dw9710_platform_data - platform data values and access functions
> + * @power_set: Power state access function, zero is off, non-zero is on.
> + * @priv_data_set: device private data (pointer) access function
> + */
> +struct dw9710_platform_data {
> + int (*power_set)(enum v4l2_power power);
> + int (*priv_data_set)(void *);
> +};
> +
> +/*
> + * Sets the specified focus value [0(far) - 100(near)]
> + */
> +int dw9710_af_setfocus(u16 posn);
> +int dw9710_af_getfocus(u16 *value);
> +#endif /* End of of CAMAF_DW9710_H */
> --
> 1.5.6.5
>
>
> --
> video4linux-list mailing list
> Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> https://www.redhat.com/mailman/listinfo/video4linux-list
>
--
Best regards, Klimov Alexey
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [REVIEW PATCH 13/14] OMAP: CAM: Add Lens Driver
@ 2008-12-12 6:03 ` Alexey Klimov
0 siblings, 0 replies; 8+ messages in thread
From: Alexey Klimov @ 2008-12-12 6:03 UTC (permalink / raw)
To: Aguirre Rodriguez, Sergio Alberto
Cc: linux-omap, video4linux-list, Sakari Ailus, Tuukka.O Toivonen,
Nagalla, Hari
Hello, all
Looked through the patch and have small suggestions, it's about coding
style and releted.
On Thu, Dec 11, 2008 at 11:38 PM, Aguirre Rodriguez, Sergio Alberto
<saaguirre@ti.com> wrote:
> >From 1341b74f5a90e4aa079a4fcb4fe2127ff344cce7 Mon Sep 17 00:00:00 2001
> From: Sergio Aguirre <saaguirre@ti.com>
> Date: Thu, 11 Dec 2008 13:35:46 -0600
> Subject: [PATCH] OMAP: CAM: Add Lens Driver
>
> This adds the DW9710 Lens driver
>
> Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
> ---
> drivers/media/video/Kconfig | 8 +
> drivers/media/video/Makefile | 1 +
> drivers/media/video/dw9710.c | 569 ++++++++++++++++++++++++++++++++++++++++++
> drivers/media/video/dw9710.h | 59 +++++
> 4 files changed, 637 insertions(+), 0 deletions(-)
> create mode 100644 drivers/media/video/dw9710.c
> create mode 100644 drivers/media/video/dw9710.h
>
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index 24957bc..50651f2 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -313,6 +313,14 @@ config VIDEO_MT9P012
> MT9P012 camera. It is currently working with the TI OMAP3
> camera controller.
>
> +config VIDEO_DW9710
> + tristate "Lens driver for DW9710"
> + depends on I2C && VIDEO_V4L2
> + ---help---
> + This is a Video4Linux2 lens driver for the Dongwoon
> + DW9710 coil. It is currently working with the TI OMAP3
> + camera controller and micron MT9P012 sensor.
> +
> config VIDEO_SAA7110
> tristate "Philips SAA7110 video decoder"
> depends on VIDEO_V4L1 && I2C
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index f73b65c..41c71d5 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
>
> obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o
> obj-$(CONFIG_VIDEO_MT9P012) += mt9p012.o
> +obj-$(CONFIG_VIDEO_DW9710) += dw9710.o
>
> obj-$(CONFIG_VIDEO_OMAP3) += omap34xxcam.o isp/
>
> diff --git a/drivers/media/video/dw9710.c b/drivers/media/video/dw9710.c
> new file mode 100644
> index 0000000..b5d5247
> --- /dev/null
> +++ b/drivers/media/video/dw9710.c
> @@ -0,0 +1,569 @@
> +/*
> + * drivers/media/video/dw9710.c
> + *
> + * DW9710 Coil Motor (LENS) driver
> + *
> + * Copyright (C) 2008 Texas Instruments.
> + *
> + * Contributors:
> + * Troy Laramy <t-laramy@ti.com>
> + * Mohit Jalori <mjalori@ti.com>
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2. This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + *
> + */
> +
> +#include <linux/mutex.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <media/v4l2-int-device.h>
> +#include <mach/gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +
> +#include "dw9710.h"
> +
> +#define DRIVER_NAME "dw9710"
Here you define DRIVER_NAME. I saw patch(for another module) that
remover MODULE_NAME and places VIVI_MODULE_NAME there. Here it is:
http://linuxtv.org/hg/v4l-dvb/rev/58b95134acb8
May be it's better to define something like this: #define
DW9710_DRIVER_NAME "dw9710" ?
Probably it will help you to avoid possible namespace conflicts and
code become a bit more readible.
> +static int
> +dw9710_probe(struct i2c_client *client, const struct i2c_device_id *id);
> +static int __exit dw9710_remove(struct i2c_client *client);
> +
> +struct dw9710_device {
> + const struct dw9710_platform_data *pdata;
> + struct v4l2_int_device *v4l2_int_device;
> + struct i2c_client *i2c_client;
> + int opened;
> + u16 current_lens_posn;
> + u16 saved_lens_posn;
> + int state;
> + int power_state;
> +};
> +
> +static const struct i2c_device_id dw9710_id[] = {
> + { DW9710_NAME, 0 },
> + { }
> +};
Hmmm, looks like you already defined DW9710_NAME in header-file. Why
didn't you reformat to use _one_ define for this and previous place ?
> +MODULE_DEVICE_TABLE(i2c, dw9710_id);
> +
> +static struct i2c_driver dw9710_i2c_driver = {
> + .driver = {
> + .name = DW9710_NAME,
Actually, the same thing here.
> + .owner = THIS_MODULE,
> + },
> + .probe = dw9710_probe,
> + .remove = __exit_p(dw9710_remove),
> + .id_table = dw9710_id,
> +};
> +
> +static struct dw9710_device dw9710 = {
> + .state = LENS_NOT_DETECTED,
> + .current_lens_posn = DEF_LENS_POSN,
> +};
> +
> +static struct vcontrol {
> + struct v4l2_queryctrl qc;
> + int current_value;
> +} video_control[] = {
> + {
> + {
> + .id = V4L2_CID_FOCUS_ABSOLUTE,
> + .type = V4L2_CTRL_TYPE_INTEGER,
> + .name = "Lens Position",
> + .minimum = 0,
> + .maximum = MAX_FOCUS_POS,
> + .step = LENS_POSN_STEP,
> + .default_value = DEF_LENS_POSN,
> + },
> + .current_value = DEF_LENS_POSN,
> + }
> +};
> +
> +static struct i2c_driver dw9710_i2c_driver;
> +
> +/**
> + * find_vctrl - Finds the requested ID in the video control structure array
> + * @id: ID of control to search the video control array for
> + *
> + * Returns the index of the requested ID from the control structure array
> + */
> +static int
> +find_vctrl(int id)
> +{
> + int i;
> +
> + if (id < V4L2_CID_BASE)
> + return -EDOM;
> +
> + for (i = (ARRAY_SIZE(video_control) - 1); i >= 0; i--)
> + if (video_control[i].qc.id == id)
> + break;
> + if (i < 0)
> + i = -EINVAL;
> + return i;
> +}
In this function you use return after if, and in second place you set
i equals to -EINVAL, and then return. Probably, to make it more easily
to read it's better to do such thing:
if (i < 0)
return -EINVAL;
> +/**
> + * camaf_reg_read - Reads a value from a register in DW9710 Coil driver device.
> + * @client: Pointer to structure of I2C client.
> + * @value: Pointer to u16 for returning value of register to read.
> + *
> + * Returns zero if successful, or non-zero otherwise.
> + **/
> +static int camaf_reg_read(struct i2c_client *client, u16 *value)
> +{
> + int err;
> + struct i2c_msg msg[1];
> + unsigned char data[2];
> +
> + if (!client->adapter)
> + return -ENODEV;
> +
> + msg->addr = client->addr;
> + msg->flags = I2C_M_RD;
> + msg->len = 2;
> + msg->buf = data;
> +
> + data[0] = 0;
> + data[1] = 0;
> +
> + err = i2c_transfer(client->adapter, msg, 1);
> +
> + if (err >= 0) {
> + *value = err = ((data[0] & 0xFF) << 8) | (data[1]);
I didn't ever see two "=" in one line in code. Is it okay ?
> + return 0;
> + }
> + return err;
> +}
> +
> +/**
> + * camaf_reg_write - Writes a value to a register in DW9710 Coil driver device.
> + * @client: Pointer to structure of I2C client.
> + * @value: Value of register to write.
> + *
> + * Returns zero if successful, or non-zero otherwise.
> + **/
> +static int camaf_reg_write(struct i2c_client *client, u16 value)
> +{
> + int err;
> + struct i2c_msg msg[1];
> + unsigned char data[2];
> + int retry = 0;
> +
> + if (!client->adapter)
> + return -ENODEV;
> +
> +again:
> + msg->addr = client->addr;
> + msg->flags = 0;
> + msg->len = 2;
> + msg->buf = data;
> +
> + data[0] = (u8)(value >> 8);
> + data[1] = (u8)(value & 0xFF);
> +
> + err = i2c_transfer(client->adapter, msg, 1);
> +
> + if (err >= 0)
> + return 0;
> +
> + if (retry <= DW9710_I2C_RETRY_COUNT) {
> + dev_dbg(&client->dev, "retry ... %d", retry);
> + retry++;
> + set_current_state(TASK_UNINTERRUPTIBLE);
> + schedule_timeout(msecs_to_jiffies(20));
> + goto again;
> + }
> + return err;
> +}
> +
> +/**
> + * dw9710_detect - Detects DW9710 Coil driver device.
> + * @client: Pointer to structure of I2C client.
> + *
> + * Returns 0 if successful, -1 if camera is off or if test register value
> + * wasn't stored properly, or returned errors from either camaf_reg_write or
> + * camaf_reg_read functions.
> + **/
> +static int dw9710_detect(struct i2c_client *client)
> +{
> + int err = 0;
> + u16 wposn = 0, rposn = 0;
> + u16 posn = 0x05;
> +
> + wposn = (CAMAF_DW9710_POWERDN(CAMAF_DW9710_ENABLE) |
> + CAMAF_DW9710_DATA(posn));
> +
> + err = camaf_reg_write(client, wposn);
> + if (err) {
> + printk(KERN_ERR "Unable to write DW9710 \n");
> + return err;
> + }
> +
> + err = camaf_reg_read(client, &rposn);
> + if (err) {
> + printk(KERN_ERR "Unable to read DW9710 \n");
> + return err;
> + }
> +
> + if (wposn != rposn) {
> + printk(KERN_ERR "W/R MISMATCH!\n");
If I were writing it, I'd do the following:
printk(KERN_ERR DRIVER_NAME "W/R MISMATCH!\n");
And driver-name have to be unique, see above.
To be onest it's better to provide module name to dmesg if you(or any
user) want to catch bad behavour.
> + return -1;
> + }
> + posn = 0;
> + wposn = (CAMAF_DW9710_POWERDN(CAMAF_DW9710_ENABLE) |
> + CAMAF_DW9710_DATA(posn));
> + err = camaf_reg_write(client, wposn);
> +
> + return err;
> +}
> +
> +/**
> + * dw9710_af_setfocus - Sets the desired focus.
> + * @posn: Desired focus position, 0 (far) - 100 (close).
> + *
> + * Returns 0 on success, -EINVAL if camera is off or focus value is out of
> + * bounds, or returned errors from either camaf_reg_write or camaf_reg_read
> + * functions.
> + **/
> +int dw9710_af_setfocus(u16 posn)
> +{
> + struct dw9710_device *af_dev = &dw9710;
> + struct i2c_client *client = af_dev->i2c_client;
> + u16 cur_focus_value = 0;
> + int ret = -EINVAL;
> +
> + if (posn > MAX_FOCUS_POS) {
> + printk(KERN_ERR "Bad posn params 0x%x \n", posn);
> + return ret;
> + }
> +
> + if ((af_dev->power_state == V4L2_POWER_OFF) ||
> + (af_dev->power_state == V4L2_POWER_STANDBY)) {
> + af_dev->current_lens_posn = posn;
> + return 0;
> + }
> +
> + ret = camaf_reg_read(client, &cur_focus_value);
> +
> + if (ret) {
> + printk(KERN_ERR "Read of current Lens position failed\n");
> + return ret;
> + }
> +
> + if (CAMAF_DW9710_DATA_R(cur_focus_value) == posn) {
> + printk(KERN_DEBUG "Device already in requested focal point\n");
> + return ret;
> + }
> +
> + ret = camaf_reg_write(client,
> + CAMAF_DW9710_POWERDN(CAMAF_DW9710_ENABLE) |
> + CAMAF_DW9710_DATA(posn));
> +
> + if (ret)
> + printk(KERN_ERR "Setfocus register write failed\n");
> + dw9710.current_lens_posn = posn;
> + return ret;
> +}
> +EXPORT_SYMBOL(dw9710_af_setfocus);
> +
> +/**
> + * dw9710_af_getfocus - Gets the focus value from device.
> + * @value: Pointer to u16 variable which will contain the focus value.
> + *
> + * Returns 0 if successful, -EINVAL if camera is off, or return value of
> + * camaf_reg_read if fails.
> + **/
> +int dw9710_af_getfocus(u16 *value)
> +{
> + int ret = -EINVAL;
> + u16 posn = 0;
Why just don't use int ret;
And later in code use return -EINVAL; ?
Anyway, you change ret variable with reg_read call.
> + struct dw9710_device *af_dev = &dw9710;
> + struct i2c_client *client = af_dev->i2c_client;
> +
> + if ((af_dev->power_state == V4L2_POWER_OFF) ||
> + (af_dev->power_state == V4L2_POWER_STANDBY))
> + return ret;
> +
> + ret = camaf_reg_read(client, &posn);
> +
> + if (ret) {
> + printk(KERN_ERR "Read of current Lens position failed\n");
> + return ret;
> + }
> + *value = CAMAF_DW9710_DATA_R(posn);
> + dw9710.current_lens_posn = CAMAF_DW9710_DATA_R(posn);
> + return ret;
> +}
> +EXPORT_SYMBOL(dw9710_af_getfocus);
> +
> +/**
> + * ioctl_queryctrl - V4L2 lens interface handler for VIDIOC_QUERYCTRL ioctl
> + * @s: pointer to standard V4L2 device structure
> + * @qc: standard V4L2 VIDIOC_QUERYCTRL ioctl structure
> + *
> + * If the requested control is supported, returns the control information
> + * from the video_control[] array. Otherwise, returns -EINVAL if the
> + * control is not supported.
> + */
> +static int ioctl_queryctrl(struct v4l2_int_device *s,
> + struct v4l2_queryctrl *qc)
> +{
> + int i;
> +
> + i = find_vctrl(qc->id);
> + if (i == -EINVAL)
> + qc->flags = V4L2_CTRL_FLAG_DISABLED;
> +
> + if (i < 0)
> + return -EINVAL;
> +
> + *qc = video_control[i].qc;
> + return 0;
> +}
> +
> +/**
> + * ioctl_g_ctrl - V4L2 DW9710 lens interface handler for VIDIOC_G_CTRL ioctl
> + * @s: pointer to standard V4L2 device structure
> + * @vc: standard V4L2 VIDIOC_G_CTRL ioctl structure
> + *
> + * If the requested control is supported, returns the control's current
> + * value from the video_control[] array. Otherwise, returns -EINVAL
> + * if the control is not supported.
> + */
> +static int ioctl_g_ctrl(struct v4l2_int_device *s,
> + struct v4l2_control *vc)
> +{
> + struct vcontrol *lvc;
> + int i;
> + u16 curr_posn;
> +
> + i = find_vctrl(vc->id);
> + if (i < 0)
> + return -EINVAL;
> + lvc = &video_control[i];
> +
> + switch (vc->id) {
> + case V4L2_CID_FOCUS_ABSOLUTE:
> + if (dw9710_af_getfocus(&curr_posn))
> + return -EFAULT;
> + vc->value = curr_posn;
> + lvc->current_value = curr_posn;
> + break;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * ioctl_s_ctrl - V4L2 DW9710 lens interface handler for VIDIOC_S_CTRL ioctl
> + * @s: pointer to standard V4L2 device structure
> + * @vc: standard V4L2 VIDIOC_S_CTRL ioctl structure
> + *
> + * If the requested control is supported, sets the control's current
> + * value in HW (and updates the video_control[] array). Otherwise,
> + * returns -EINVAL if the control is not supported.
> + */
> +static int ioctl_s_ctrl(struct v4l2_int_device *s,
> + struct v4l2_control *vc)
> +{
> + int retval = -EINVAL;
> + int i;
> + struct vcontrol *lvc;
> +
> + i = find_vctrl(vc->id);
> + if (i < 0)
> + return -EINVAL;
> + lvc = &video_control[i];
> +
> + switch (vc->id) {
> + case V4L2_CID_FOCUS_ABSOLUTE:
> + retval = dw9710_af_setfocus(vc->value);
> + if (!retval)
> + lvc->current_value = vc->value;
> + break;
> + }
> +
> + return retval;
> +}
> +
> +/**
> + * ioctl_g_priv - V4L2 sensor interface handler for vidioc_int_g_priv_num
> + * @s: pointer to standard V4L2 device structure
> + * @p: void pointer to hold sensor's private data address
> + *
> + * Returns device's (sensor's) private data area address in p parameter
> + */
> +static int ioctl_g_priv(struct v4l2_int_device *s, void *p)
> +{
> + struct dw9710_device *lens = s->priv;
> +
> + return lens->pdata->priv_data_set(p);
> +
> +}
> +
> +/**
> + * ioctl_s_power - V4L2 sensor interface handler for vidioc_int_s_power_num
> + * @s: pointer to standard V4L2 device structure
> + * @on: power state to which device is to be set
> + *
> + * Sets devices power state to requrested state, if possible.
> + */
> +static int ioctl_s_power(struct v4l2_int_device *s, enum v4l2_power on)
> +{
> + struct dw9710_device *lens = s->priv;
> + struct i2c_client *c = lens->i2c_client;
> + int rval;
> +
> + rval = lens->pdata->power_set(on);
> + if (rval < 0) {
> + dev_err(&c->dev, "Unable to set the power state: " DRIVER_NAME
> + " lens HW\n");
> + return rval;
> + }
> +
> + if ((on == V4L2_POWER_ON) && (lens->state == LENS_NOT_DETECTED)) {
> + rval = dw9710_detect(c);
> + if (rval < 0) {
> + dev_err(&c->dev, "Unable to detect "
> + DRIVER_NAME " lens HW\n");
> + printk(KERN_ERR "Unable to detect "
> + DRIVER_NAME " lens HW\n");
Two thing here. As i know it's more preferable to use dev_macro
instead of printk if possible.
As i know(may be i'm wrong) it's not good to use DRIVER_NAME in dev_err here.
Why do you print the same text two times here ?
> + lens->state = LENS_NOT_DETECTED;
> + return rval;
> + }
> + lens->state = LENS_DETECTED;
> + pr_info(DRIVER_NAME " lens HW detected\n");
> + }
> +
> + if ((lens->power_state == V4L2_POWER_STANDBY) && (on == V4L2_POWER_ON)
> + && (lens->state == LENS_DETECTED))
> + dw9710_af_setfocus(lens->current_lens_posn);
> +
> + lens->power_state = on;
> + return 0;
> +}
> +
> +static struct v4l2_int_ioctl_desc dw9710_ioctl_desc[] = {
> + { .num = vidioc_int_s_power_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_s_power },
> + { .num = vidioc_int_g_priv_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_g_priv },
> + { .num = vidioc_int_queryctrl_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_queryctrl },
> + { .num = vidioc_int_g_ctrl_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_g_ctrl },
> + { .num = vidioc_int_s_ctrl_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_s_ctrl },
> +};
> +
> +static struct v4l2_int_slave dw9710_slave = {
> + .ioctls = dw9710_ioctl_desc,
> + .num_ioctls = ARRAY_SIZE(dw9710_ioctl_desc),
> +};
> +
> +static struct v4l2_int_device dw9710_int_device = {
> + .module = THIS_MODULE,
> + .name = DRIVER_NAME,
> + .priv = &dw9710,
> + .type = v4l2_int_type_slave,
> + .u = {
> + .slave = &dw9710_slave,
> + },
> +};
> +
> +/**
> + * dw9710_probe - Probes the driver for valid I2C attachment.
> + * @client: Pointer to structure of I2C client.
> + *
> + * Returns 0 if successful, or -EBUSY if unable to get client attached data.
> + **/
> +static int
> +dw9710_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> + struct dw9710_device *lens = &dw9710;
> + int err;
> +
> + dev_info(&client->dev, "dw9710 probe called....\n");
> +
> + if (i2c_get_clientdata(client)) {
> + printk(KERN_ERR " DTA BUSY %s\n", client->name);
> + return -EBUSY;
> + }
> +
> + lens->pdata = client->dev.platform_data;
> +
> + if (!lens->pdata) {
> + dev_err(&client->dev, "no platform data?\n");
> + return -ENODEV;
> + }
> +
> + lens->v4l2_int_device = &dw9710_int_device;
> +
> + lens->i2c_client = client;
> + i2c_set_clientdata(client, lens);
> +
> + err = v4l2_int_device_register(lens->v4l2_int_device);
> + if (err) {
> + printk(KERN_ERR "Failed to Register "
> + DRIVER_NAME " as V4L2 device.\n");
> + i2c_set_clientdata(client, NULL);
> + } else {
> + printk(KERN_ERR "Registered "
> + DRIVER_NAME " as V4L2 device.\n");
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * dw9710_remove - Routine when device its unregistered from I2C
> + * @client: Pointer to structure of I2C client.
> + *
> + * Returns 0 if successful, or -ENODEV if the client isn't attached.
> + **/
> +static int __exit dw9710_remove(struct i2c_client *client)
> +{
> + if (!client->adapter)
> + return -ENODEV;
> +
> + i2c_set_clientdata(client, NULL);
> + return 0;
> +}
> +
> +/**
> + * dw9710_init - Module initialisation.
> + *
> + * Returns 0 if successful, or -EINVAL if device couldn't be initialized, or
> + * added as a character device.
> + **/
> +static int __init dw9710_init(void)
> +{
> + int err = -EINVAL;
int err = something, and then changed with call to i2c..
This is all i can suggest, it's not about functionality, it's about
coding style..
Please, use dev_macros if possible instead of printk.
> + err = i2c_add_driver(&dw9710_i2c_driver);
> + if (err)
> + goto fail;
> + return err;
> +fail:
> + printk(KERN_ERR "Failed to register " DRIVER_NAME ".\n");
> + return err;
> +}
> +late_initcall(dw9710_init);
> +
> +/**
> + * dw9710_cleanup - Module cleanup.
> + **/
> +static void __exit dw9710_cleanup(void)
> +{
> + i2c_del_driver(&dw9710_i2c_driver);
> +}
> +module_exit(dw9710_cleanup);
> +
> +MODULE_AUTHOR("Texas Instruments");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("DW9710 LENS driver");
> diff --git a/drivers/media/video/dw9710.h b/drivers/media/video/dw9710.h
> new file mode 100644
> index 0000000..e4050a4
> --- /dev/null
> +++ b/drivers/media/video/dw9710.h
> @@ -0,0 +1,59 @@
> +/*
> + * drivers/media/video/dw9710.h
> + *
> + * Register defines for Auto Focus device
> + *
> + * Copyright (C) 2008 Texas Instruments.
> + *
> + * Contributors:
> + * Troy Laramy <t-laramy@ti.com>
> + * Mohit Jalori <mjalori@ti.com>
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2. This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + *
> + */
> +
> +#ifndef CAMAF_DW9710_H
> +#define CAMAF_DW9710_H
> +
> +#define DW9710_AF_I2C_ADDR 0x0C
> +#define DW9710_NAME "DW9710"
> +#define DW9710_I2C_RETRY_COUNT 5
> +#define MAX_FOCUS_POS 0xFF
> +
> +#define CAMAF_DW9710_DISABLE 0x1
> +#define CAMAF_DW9710_ENABLE 0x0
> +#define CAMAF_DW9710_POWERDN(ARG) (((ARG) & 0x1) << 15)
> +#define CAMAF_DW9710_POWERDN_R(ARG) (((ARG) >> 15) & 0x1)
> +
> +#define CAMAF_DW9710_DATA(ARG) (((ARG) & 0xFF) << 6)
> +#define CAMAF_DW9710_DATA_R(ARG) (((ARG) >> 6) & 0xFF)
> +#define CAMAF_FREQUENCY_EQ1(mclk) ((u16)(mclk/16000))
> +
> +/* State of lens */
> +#define LENS_DETECTED 1
> +#define LENS_NOT_DETECTED 0
> +
> +/* Focus control values */
> +#define DEF_LENS_POSN 0 /* 0x7F */
> +#define LENS_POSN_STEP 1
> +
> +
> +/**
> + * struct dw9710_platform_data - platform data values and access functions
> + * @power_set: Power state access function, zero is off, non-zero is on.
> + * @priv_data_set: device private data (pointer) access function
> + */
> +struct dw9710_platform_data {
> + int (*power_set)(enum v4l2_power power);
> + int (*priv_data_set)(void *);
> +};
> +
> +/*
> + * Sets the specified focus value [0(far) - 100(near)]
> + */
> +int dw9710_af_setfocus(u16 posn);
> +int dw9710_af_getfocus(u16 *value);
> +#endif /* End of of CAMAF_DW9710_H */
> --
> 1.5.6.5
>
>
> --
> video4linux-list mailing list
> Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> https://www.redhat.com/mailman/listinfo/video4linux-list
>
--
Best regards, Klimov Alexey
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [REVIEW PATCH 13/14] OMAP: CAM: Add Lens Driver
2008-12-12 6:03 ` Alexey Klimov
(?)
@ 2008-12-12 8:44 ` Robert William Fuller
2008-12-12 12:25 ` Douglas Schilling Landgraf
-1 siblings, 1 reply; 8+ messages in thread
From: Robert William Fuller @ 2008-12-12 8:44 UTC (permalink / raw)
To: video4linux-list
Alexey Klimov wrote:
<snip>
> I didn't ever see two "=" in one line in code. Is it okay ?
If you do not understand the C programming language, why are you in a
position to review code?
The "=" operator associates from right to left. Hence, in the case of
"a = b = c", c is first assigned to b, then b is assigned to a.
You should understand the concepts of operator precedence and operator
associativity before you are in a position to review C code. This is
elementary.
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [REVIEW PATCH 13/14] OMAP: CAM: Add Lens Driver
2008-12-12 8:44 ` Robert William Fuller
@ 2008-12-12 12:25 ` Douglas Schilling Landgraf
0 siblings, 0 replies; 8+ messages in thread
From: Douglas Schilling Landgraf @ 2008-12-12 12:25 UTC (permalink / raw)
To: Robert William Fuller; +Cc: video4linux-list
Hello Robert,
On Fri, 12 Dec 2008 03:44:03 -0500
Robert William Fuller <hydrologiccycle@gmail.com> wrote:
> Alexey Klimov wrote:
>
> <snip>
>
> > I didn't ever see two "=" in one line in code. Is it okay ?
>
> If you do not understand the C programming language, why are you in a
> position to review code?
humm, everyone can review patches and give their opinions.
The technique of reviewing code will always help programmers to
improve their knowledge and helps the quality of modules or even
of subsystem. On the other hand, if the author of patch or maintainer
will accept those suggestions is other case.
IMO, the above answer discourage reviewing code, please
don't do that again.
Cheers,
Douglas
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [REVIEW PATCH 13/14] OMAP: CAM: Add Lens Driver
2008-12-11 20:38 [REVIEW PATCH 13/14] OMAP: CAM: Add Lens Driver Aguirre Rodriguez, Sergio Alberto
@ 2008-12-16 17:38 ` Hans Verkuil
2008-12-16 17:38 ` Hans Verkuil
1 sibling, 0 replies; 8+ messages in thread
From: Hans Verkuil @ 2008-12-16 17:38 UTC (permalink / raw)
To: video4linux-list
Cc: Sakari Ailus, linux-omap, Tuukka.O Toivonen, Nagalla, Hari
On Thursday 11 December 2008 21:38:28 Aguirre Rodriguez, Sergio Alberto wrote:
> >From 1341b74f5a90e4aa079a4fcb4fe2127ff344cce7 Mon Sep 17 00:00:00 2001
> From: Sergio Aguirre <saaguirre@ti.com>
> Date: Thu, 11 Dec 2008 13:35:46 -0600
> Subject: [PATCH] OMAP: CAM: Add Lens Driver
>
> This adds the DW9710 Lens driver
>
> Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
> ---
> drivers/media/video/Kconfig | 8 +
> drivers/media/video/Makefile | 1 +
> drivers/media/video/dw9710.c | 569 ++++++++++++++++++++++++++++++++++++++++++
> drivers/media/video/dw9710.h | 59 +++++
> 4 files changed, 637 insertions(+), 0 deletions(-)
> create mode 100644 drivers/media/video/dw9710.c
> create mode 100644 drivers/media/video/dw9710.h
>
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index 24957bc..50651f2 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -313,6 +313,14 @@ config VIDEO_MT9P012
> MT9P012 camera. It is currently working with the TI OMAP3
> camera controller.
>
> +config VIDEO_DW9710
> + tristate "Lens driver for DW9710"
> + depends on I2C && VIDEO_V4L2
> + ---help---
> + This is a Video4Linux2 lens driver for the Dongwoon
> + DW9710 coil. It is currently working with the TI OMAP3
> + camera controller and micron MT9P012 sensor.
> +
> config VIDEO_SAA7110
> tristate "Philips SAA7110 video decoder"
> depends on VIDEO_V4L1 && I2C
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index f73b65c..41c71d5 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
>
> obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o
> obj-$(CONFIG_VIDEO_MT9P012) += mt9p012.o
> +obj-$(CONFIG_VIDEO_DW9710) += dw9710.o
>
> obj-$(CONFIG_VIDEO_OMAP3) += omap34xxcam.o isp/
>
> diff --git a/drivers/media/video/dw9710.c b/drivers/media/video/dw9710.c
> new file mode 100644
> index 0000000..b5d5247
> --- /dev/null
> +++ b/drivers/media/video/dw9710.c
> @@ -0,0 +1,569 @@
> +/*
> + * drivers/media/video/dw9710.c
> + *
> + * DW9710 Coil Motor (LENS) driver
> + *
> + * Copyright (C) 2008 Texas Instruments.
> + *
> + * Contributors:
> + * Troy Laramy <t-laramy@ti.com>
> + * Mohit Jalori <mjalori@ti.com>
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2. This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + *
> + */
> +
> +#include <linux/mutex.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <media/v4l2-int-device.h>
> +#include <mach/gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +
> +#include "dw9710.h"
> +
> +#define DRIVER_NAME "dw9710"
> +
> +static int
> +dw9710_probe(struct i2c_client *client, const struct i2c_device_id *id);
> +static int __exit dw9710_remove(struct i2c_client *client);
> +
> +struct dw9710_device {
> + const struct dw9710_platform_data *pdata;
> + struct v4l2_int_device *v4l2_int_device;
> + struct i2c_client *i2c_client;
> + int opened;
> + u16 current_lens_posn;
> + u16 saved_lens_posn;
> + int state;
> + int power_state;
> +};
> +
> +static const struct i2c_device_id dw9710_id[] = {
> + { DW9710_NAME, 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, dw9710_id);
> +
> +static struct i2c_driver dw9710_i2c_driver = {
> + .driver = {
> + .name = DW9710_NAME,
> + .owner = THIS_MODULE,
> + },
> + .probe = dw9710_probe,
> + .remove = __exit_p(dw9710_remove),
> + .id_table = dw9710_id,
> +};
> +
> +static struct dw9710_device dw9710 = {
> + .state = LENS_NOT_DETECTED,
> + .current_lens_posn = DEF_LENS_POSN,
> +};
> +
> +static struct vcontrol {
> + struct v4l2_queryctrl qc;
> + int current_value;
> +} video_control[] = {
> + {
> + {
> + .id = V4L2_CID_FOCUS_ABSOLUTE,
> + .type = V4L2_CTRL_TYPE_INTEGER,
> + .name = "Lens Position",
Please use the standard string for this: "Focus, Absolute".
> + .minimum = 0,
> + .maximum = MAX_FOCUS_POS,
> + .step = LENS_POSN_STEP,
> + .default_value = DEF_LENS_POSN,
> + },
> + .current_value = DEF_LENS_POSN,
> + }
> +};
> +
> +static struct i2c_driver dw9710_i2c_driver;
> +
> +/**
> + * find_vctrl - Finds the requested ID in the video control structure array
> + * @id: ID of control to search the video control array for
> + *
> + * Returns the index of the requested ID from the control structure array
> + */
> +static int
> +find_vctrl(int id)
> +{
> + int i;
> +
> + if (id < V4L2_CID_BASE)
> + return -EDOM;
> +
> + for (i = (ARRAY_SIZE(video_control) - 1); i >= 0; i--)
> + if (video_control[i].qc.id == id)
> + break;
> + if (i < 0)
> + i = -EINVAL;
> + return i;
> +}
> +
> +/**
> + * camaf_reg_read - Reads a value from a register in DW9710 Coil driver device.
> + * @client: Pointer to structure of I2C client.
> + * @value: Pointer to u16 for returning value of register to read.
> + *
> + * Returns zero if successful, or non-zero otherwise.
> + **/
> +static int camaf_reg_read(struct i2c_client *client, u16 *value)
> +{
> + int err;
> + struct i2c_msg msg[1];
> + unsigned char data[2];
> +
> + if (!client->adapter)
> + return -ENODEV;
> +
> + msg->addr = client->addr;
> + msg->flags = I2C_M_RD;
> + msg->len = 2;
> + msg->buf = data;
> +
> + data[0] = 0;
> + data[1] = 0;
> +
> + err = i2c_transfer(client->adapter, msg, 1);
> +
> + if (err >= 0) {
> + *value = err = ((data[0] & 0xFF) << 8) | (data[1]);
> + return 0;
> + }
> + return err;
> +}
> +
> +/**
> + * camaf_reg_write - Writes a value to a register in DW9710 Coil driver device.
> + * @client: Pointer to structure of I2C client.
> + * @value: Value of register to write.
> + *
> + * Returns zero if successful, or non-zero otherwise.
> + **/
> +static int camaf_reg_write(struct i2c_client *client, u16 value)
> +{
> + int err;
> + struct i2c_msg msg[1];
> + unsigned char data[2];
> + int retry = 0;
> +
> + if (!client->adapter)
> + return -ENODEV;
> +
> +again:
> + msg->addr = client->addr;
> + msg->flags = 0;
> + msg->len = 2;
> + msg->buf = data;
> +
> + data[0] = (u8)(value >> 8);
> + data[1] = (u8)(value & 0xFF);
> +
> + err = i2c_transfer(client->adapter, msg, 1);
> +
> + if (err >= 0)
> + return 0;
> +
> + if (retry <= DW9710_I2C_RETRY_COUNT) {
> + dev_dbg(&client->dev, "retry ... %d", retry);
> + retry++;
> + set_current_state(TASK_UNINTERRUPTIBLE);
> + schedule_timeout(msecs_to_jiffies(20));
> + goto again;
> + }
> + return err;
> +}
> +
> +/**
> + * dw9710_detect - Detects DW9710 Coil driver device.
> + * @client: Pointer to structure of I2C client.
> + *
> + * Returns 0 if successful, -1 if camera is off or if test register value
> + * wasn't stored properly, or returned errors from either camaf_reg_write or
> + * camaf_reg_read functions.
> + **/
> +static int dw9710_detect(struct i2c_client *client)
> +{
> + int err = 0;
> + u16 wposn = 0, rposn = 0;
> + u16 posn = 0x05;
> +
> + wposn = (CAMAF_DW9710_POWERDN(CAMAF_DW9710_ENABLE) |
> + CAMAF_DW9710_DATA(posn));
> +
> + err = camaf_reg_write(client, wposn);
> + if (err) {
> + printk(KERN_ERR "Unable to write DW9710 \n");
> + return err;
> + }
> +
> + err = camaf_reg_read(client, &rposn);
> + if (err) {
> + printk(KERN_ERR "Unable to read DW9710 \n");
> + return err;
> + }
> +
> + if (wposn != rposn) {
> + printk(KERN_ERR "W/R MISMATCH!\n");
> + return -1;
> + }
> + posn = 0;
> + wposn = (CAMAF_DW9710_POWERDN(CAMAF_DW9710_ENABLE) |
> + CAMAF_DW9710_DATA(posn));
> + err = camaf_reg_write(client, wposn);
> +
> + return err;
> +}
> +
> +/**
> + * dw9710_af_setfocus - Sets the desired focus.
> + * @posn: Desired focus position, 0 (far) - 100 (close).
> + *
> + * Returns 0 on success, -EINVAL if camera is off or focus value is out of
> + * bounds, or returned errors from either camaf_reg_write or camaf_reg_read
> + * functions.
> + **/
> +int dw9710_af_setfocus(u16 posn)
> +{
> + struct dw9710_device *af_dev = &dw9710;
> + struct i2c_client *client = af_dev->i2c_client;
> + u16 cur_focus_value = 0;
> + int ret = -EINVAL;
> +
> + if (posn > MAX_FOCUS_POS) {
> + printk(KERN_ERR "Bad posn params 0x%x \n", posn);
> + return ret;
> + }
> +
> + if ((af_dev->power_state == V4L2_POWER_OFF) ||
> + (af_dev->power_state == V4L2_POWER_STANDBY)) {
> + af_dev->current_lens_posn = posn;
> + return 0;
> + }
> +
> + ret = camaf_reg_read(client, &cur_focus_value);
> +
> + if (ret) {
> + printk(KERN_ERR "Read of current Lens position failed\n");
> + return ret;
> + }
> +
> + if (CAMAF_DW9710_DATA_R(cur_focus_value) == posn) {
> + printk(KERN_DEBUG "Device already in requested focal point\n");
> + return ret;
> + }
> +
> + ret = camaf_reg_write(client,
> + CAMAF_DW9710_POWERDN(CAMAF_DW9710_ENABLE) |
> + CAMAF_DW9710_DATA(posn));
> +
> + if (ret)
> + printk(KERN_ERR "Setfocus register write failed\n");
> + dw9710.current_lens_posn = posn;
> + return ret;
> +}
> +EXPORT_SYMBOL(dw9710_af_setfocus);
> +
> +/**
> + * dw9710_af_getfocus - Gets the focus value from device.
> + * @value: Pointer to u16 variable which will contain the focus value.
> + *
> + * Returns 0 if successful, -EINVAL if camera is off, or return value of
> + * camaf_reg_read if fails.
> + **/
> +int dw9710_af_getfocus(u16 *value)
> +{
> + int ret = -EINVAL;
> + u16 posn = 0;
> +
> + struct dw9710_device *af_dev = &dw9710;
> + struct i2c_client *client = af_dev->i2c_client;
> +
> + if ((af_dev->power_state == V4L2_POWER_OFF) ||
> + (af_dev->power_state == V4L2_POWER_STANDBY))
> + return ret;
> +
> + ret = camaf_reg_read(client, &posn);
> +
> + if (ret) {
> + printk(KERN_ERR "Read of current Lens position failed\n");
> + return ret;
> + }
> + *value = CAMAF_DW9710_DATA_R(posn);
> + dw9710.current_lens_posn = CAMAF_DW9710_DATA_R(posn);
> + return ret;
> +}
> +EXPORT_SYMBOL(dw9710_af_getfocus);
> +
> +/**
> + * ioctl_queryctrl - V4L2 lens interface handler for VIDIOC_QUERYCTRL ioctl
> + * @s: pointer to standard V4L2 device structure
> + * @qc: standard V4L2 VIDIOC_QUERYCTRL ioctl structure
> + *
> + * If the requested control is supported, returns the control information
> + * from the video_control[] array. Otherwise, returns -EINVAL if the
> + * control is not supported.
> + */
> +static int ioctl_queryctrl(struct v4l2_int_device *s,
> + struct v4l2_queryctrl *qc)
> +{
> + int i;
> +
> + i = find_vctrl(qc->id);
> + if (i == -EINVAL)
> + qc->flags = V4L2_CTRL_FLAG_DISABLED;
> +
> + if (i < 0)
> + return -EINVAL;
> +
> + *qc = video_control[i].qc;
> + return 0;
> +}
> +
> +/**
> + * ioctl_g_ctrl - V4L2 DW9710 lens interface handler for VIDIOC_G_CTRL ioctl
> + * @s: pointer to standard V4L2 device structure
> + * @vc: standard V4L2 VIDIOC_G_CTRL ioctl structure
> + *
> + * If the requested control is supported, returns the control's current
> + * value from the video_control[] array. Otherwise, returns -EINVAL
> + * if the control is not supported.
> + */
> +static int ioctl_g_ctrl(struct v4l2_int_device *s,
> + struct v4l2_control *vc)
> +{
> + struct vcontrol *lvc;
> + int i;
> + u16 curr_posn;
> +
> + i = find_vctrl(vc->id);
> + if (i < 0)
> + return -EINVAL;
> + lvc = &video_control[i];
> +
> + switch (vc->id) {
> + case V4L2_CID_FOCUS_ABSOLUTE:
> + if (dw9710_af_getfocus(&curr_posn))
> + return -EFAULT;
> + vc->value = curr_posn;
> + lvc->current_value = curr_posn;
> + break;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * ioctl_s_ctrl - V4L2 DW9710 lens interface handler for VIDIOC_S_CTRL ioctl
> + * @s: pointer to standard V4L2 device structure
> + * @vc: standard V4L2 VIDIOC_S_CTRL ioctl structure
> + *
> + * If the requested control is supported, sets the control's current
> + * value in HW (and updates the video_control[] array). Otherwise,
> + * returns -EINVAL if the control is not supported.
> + */
> +static int ioctl_s_ctrl(struct v4l2_int_device *s,
> + struct v4l2_control *vc)
> +{
> + int retval = -EINVAL;
> + int i;
> + struct vcontrol *lvc;
> +
> + i = find_vctrl(vc->id);
> + if (i < 0)
> + return -EINVAL;
> + lvc = &video_control[i];
> +
> + switch (vc->id) {
> + case V4L2_CID_FOCUS_ABSOLUTE:
> + retval = dw9710_af_setfocus(vc->value);
> + if (!retval)
> + lvc->current_value = vc->value;
> + break;
> + }
> +
> + return retval;
> +}
> +
> +/**
> + * ioctl_g_priv - V4L2 sensor interface handler for vidioc_int_g_priv_num
> + * @s: pointer to standard V4L2 device structure
> + * @p: void pointer to hold sensor's private data address
> + *
> + * Returns device's (sensor's) private data area address in p parameter
> + */
> +static int ioctl_g_priv(struct v4l2_int_device *s, void *p)
> +{
> + struct dw9710_device *lens = s->priv;
> +
> + return lens->pdata->priv_data_set(p);
> +
> +}
> +
> +/**
> + * ioctl_s_power - V4L2 sensor interface handler for vidioc_int_s_power_num
> + * @s: pointer to standard V4L2 device structure
> + * @on: power state to which device is to be set
> + *
> + * Sets devices power state to requrested state, if possible.
> + */
> +static int ioctl_s_power(struct v4l2_int_device *s, enum v4l2_power on)
> +{
> + struct dw9710_device *lens = s->priv;
> + struct i2c_client *c = lens->i2c_client;
> + int rval;
> +
> + rval = lens->pdata->power_set(on);
> + if (rval < 0) {
> + dev_err(&c->dev, "Unable to set the power state: " DRIVER_NAME
> + " lens HW\n");
> + return rval;
> + }
> +
> + if ((on == V4L2_POWER_ON) && (lens->state == LENS_NOT_DETECTED)) {
> + rval = dw9710_detect(c);
> + if (rval < 0) {
> + dev_err(&c->dev, "Unable to detect "
> + DRIVER_NAME " lens HW\n");
> + printk(KERN_ERR "Unable to detect "
> + DRIVER_NAME " lens HW\n");
> + lens->state = LENS_NOT_DETECTED;
> + return rval;
> + }
> + lens->state = LENS_DETECTED;
> + pr_info(DRIVER_NAME " lens HW detected\n");
> + }
> +
> + if ((lens->power_state == V4L2_POWER_STANDBY) && (on == V4L2_POWER_ON)
> + && (lens->state == LENS_DETECTED))
> + dw9710_af_setfocus(lens->current_lens_posn);
> +
> + lens->power_state = on;
> + return 0;
> +}
> +
> +static struct v4l2_int_ioctl_desc dw9710_ioctl_desc[] = {
> + { .num = vidioc_int_s_power_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_s_power },
> + { .num = vidioc_int_g_priv_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_g_priv },
> + { .num = vidioc_int_queryctrl_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_queryctrl },
> + { .num = vidioc_int_g_ctrl_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_g_ctrl },
> + { .num = vidioc_int_s_ctrl_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_s_ctrl },
> +};
> +
> +static struct v4l2_int_slave dw9710_slave = {
> + .ioctls = dw9710_ioctl_desc,
> + .num_ioctls = ARRAY_SIZE(dw9710_ioctl_desc),
> +};
> +
> +static struct v4l2_int_device dw9710_int_device = {
> + .module = THIS_MODULE,
> + .name = DRIVER_NAME,
> + .priv = &dw9710,
> + .type = v4l2_int_type_slave,
> + .u = {
> + .slave = &dw9710_slave,
> + },
> +};
> +
> +/**
> + * dw9710_probe - Probes the driver for valid I2C attachment.
> + * @client: Pointer to structure of I2C client.
> + *
> + * Returns 0 if successful, or -EBUSY if unable to get client attached data.
> + **/
> +static int
> +dw9710_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> + struct dw9710_device *lens = &dw9710;
> + int err;
> +
> + dev_info(&client->dev, "dw9710 probe called....\n");
> +
> + if (i2c_get_clientdata(client)) {
> + printk(KERN_ERR " DTA BUSY %s\n", client->name);
> + return -EBUSY;
> + }
> +
> + lens->pdata = client->dev.platform_data;
> +
> + if (!lens->pdata) {
> + dev_err(&client->dev, "no platform data?\n");
> + return -ENODEV;
> + }
> +
> + lens->v4l2_int_device = &dw9710_int_device;
> +
> + lens->i2c_client = client;
> + i2c_set_clientdata(client, lens);
> +
> + err = v4l2_int_device_register(lens->v4l2_int_device);
> + if (err) {
> + printk(KERN_ERR "Failed to Register "
> + DRIVER_NAME " as V4L2 device.\n");
> + i2c_set_clientdata(client, NULL);
> + } else {
> + printk(KERN_ERR "Registered "
> + DRIVER_NAME " as V4L2 device.\n");
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * dw9710_remove - Routine when device its unregistered from I2C
> + * @client: Pointer to structure of I2C client.
> + *
> + * Returns 0 if successful, or -ENODEV if the client isn't attached.
> + **/
> +static int __exit dw9710_remove(struct i2c_client *client)
> +{
> + if (!client->adapter)
> + return -ENODEV;
> +
> + i2c_set_clientdata(client, NULL);
> + return 0;
> +}
> +
> +/**
> + * dw9710_init - Module initialisation.
> + *
> + * Returns 0 if successful, or -EINVAL if device couldn't be initialized, or
> + * added as a character device.
> + **/
> +static int __init dw9710_init(void)
> +{
> + int err = -EINVAL;
> +
> + err = i2c_add_driver(&dw9710_i2c_driver);
> + if (err)
> + goto fail;
> + return err;
> +fail:
> + printk(KERN_ERR "Failed to register " DRIVER_NAME ".\n");
> + return err;
> +}
> +late_initcall(dw9710_init);
> +
> +/**
> + * dw9710_cleanup - Module cleanup.
> + **/
> +static void __exit dw9710_cleanup(void)
> +{
> + i2c_del_driver(&dw9710_i2c_driver);
> +}
> +module_exit(dw9710_cleanup);
> +
> +MODULE_AUTHOR("Texas Instruments");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("DW9710 LENS driver");
> diff --git a/drivers/media/video/dw9710.h b/drivers/media/video/dw9710.h
> new file mode 100644
> index 0000000..e4050a4
> --- /dev/null
> +++ b/drivers/media/video/dw9710.h
> @@ -0,0 +1,59 @@
> +/*
> + * drivers/media/video/dw9710.h
> + *
> + * Register defines for Auto Focus device
> + *
> + * Copyright (C) 2008 Texas Instruments.
> + *
> + * Contributors:
> + * Troy Laramy <t-laramy@ti.com>
> + * Mohit Jalori <mjalori@ti.com>
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2. This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + *
> + */
> +
> +#ifndef CAMAF_DW9710_H
> +#define CAMAF_DW9710_H
> +
> +#define DW9710_AF_I2C_ADDR 0x0C
> +#define DW9710_NAME "DW9710"
> +#define DW9710_I2C_RETRY_COUNT 5
> +#define MAX_FOCUS_POS 0xFF
> +
> +#define CAMAF_DW9710_DISABLE 0x1
> +#define CAMAF_DW9710_ENABLE 0x0
> +#define CAMAF_DW9710_POWERDN(ARG) (((ARG) & 0x1) << 15)
> +#define CAMAF_DW9710_POWERDN_R(ARG) (((ARG) >> 15) & 0x1)
> +
> +#define CAMAF_DW9710_DATA(ARG) (((ARG) & 0xFF) << 6)
> +#define CAMAF_DW9710_DATA_R(ARG) (((ARG) >> 6) & 0xFF)
> +#define CAMAF_FREQUENCY_EQ1(mclk) ((u16)(mclk/16000))
> +
> +/* State of lens */
> +#define LENS_DETECTED 1
> +#define LENS_NOT_DETECTED 0
> +
> +/* Focus control values */
> +#define DEF_LENS_POSN 0 /* 0x7F */
> +#define LENS_POSN_STEP 1
> +
> +
> +/**
> + * struct dw9710_platform_data - platform data values and access functions
> + * @power_set: Power state access function, zero is off, non-zero is on.
> + * @priv_data_set: device private data (pointer) access function
> + */
> +struct dw9710_platform_data {
> + int (*power_set)(enum v4l2_power power);
> + int (*priv_data_set)(void *);
> +};
> +
> +/*
> + * Sets the specified focus value [0(far) - 100(near)]
> + */
> +int dw9710_af_setfocus(u16 posn);
> +int dw9710_af_getfocus(u16 *value);
> +#endif /* End of of CAMAF_DW9710_H */
This header seems to be a mix of kernel-public and driver-private things.
Please split this up. Anything kernel-public should go to
include/media/dw9710.h. Driver-private data should either be in the source
or in a header next to the source.
Regards,
Hans
> --
> 1.5.6.5
>
>
> --
> video4linux-list mailing list
> Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> https://www.redhat.com/mailman/listinfo/video4linux-list
>
>
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [REVIEW PATCH 13/14] OMAP: CAM: Add Lens Driver
@ 2008-12-16 17:38 ` Hans Verkuil
0 siblings, 0 replies; 8+ messages in thread
From: Hans Verkuil @ 2008-12-16 17:38 UTC (permalink / raw)
To: video4linux-list
Cc: Aguirre Rodriguez, Sergio Alberto, linux-omap, Sakari Ailus,
Tuukka.O Toivonen, Nagalla, Hari
On Thursday 11 December 2008 21:38:28 Aguirre Rodriguez, Sergio Alberto wrote:
> >From 1341b74f5a90e4aa079a4fcb4fe2127ff344cce7 Mon Sep 17 00:00:00 2001
> From: Sergio Aguirre <saaguirre@ti.com>
> Date: Thu, 11 Dec 2008 13:35:46 -0600
> Subject: [PATCH] OMAP: CAM: Add Lens Driver
>
> This adds the DW9710 Lens driver
>
> Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
> ---
> drivers/media/video/Kconfig | 8 +
> drivers/media/video/Makefile | 1 +
> drivers/media/video/dw9710.c | 569 ++++++++++++++++++++++++++++++++++++++++++
> drivers/media/video/dw9710.h | 59 +++++
> 4 files changed, 637 insertions(+), 0 deletions(-)
> create mode 100644 drivers/media/video/dw9710.c
> create mode 100644 drivers/media/video/dw9710.h
>
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index 24957bc..50651f2 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -313,6 +313,14 @@ config VIDEO_MT9P012
> MT9P012 camera. It is currently working with the TI OMAP3
> camera controller.
>
> +config VIDEO_DW9710
> + tristate "Lens driver for DW9710"
> + depends on I2C && VIDEO_V4L2
> + ---help---
> + This is a Video4Linux2 lens driver for the Dongwoon
> + DW9710 coil. It is currently working with the TI OMAP3
> + camera controller and micron MT9P012 sensor.
> +
> config VIDEO_SAA7110
> tristate "Philips SAA7110 video decoder"
> depends on VIDEO_V4L1 && I2C
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index f73b65c..41c71d5 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
>
> obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o
> obj-$(CONFIG_VIDEO_MT9P012) += mt9p012.o
> +obj-$(CONFIG_VIDEO_DW9710) += dw9710.o
>
> obj-$(CONFIG_VIDEO_OMAP3) += omap34xxcam.o isp/
>
> diff --git a/drivers/media/video/dw9710.c b/drivers/media/video/dw9710.c
> new file mode 100644
> index 0000000..b5d5247
> --- /dev/null
> +++ b/drivers/media/video/dw9710.c
> @@ -0,0 +1,569 @@
> +/*
> + * drivers/media/video/dw9710.c
> + *
> + * DW9710 Coil Motor (LENS) driver
> + *
> + * Copyright (C) 2008 Texas Instruments.
> + *
> + * Contributors:
> + * Troy Laramy <t-laramy@ti.com>
> + * Mohit Jalori <mjalori@ti.com>
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2. This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + *
> + */
> +
> +#include <linux/mutex.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <media/v4l2-int-device.h>
> +#include <mach/gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +
> +#include "dw9710.h"
> +
> +#define DRIVER_NAME "dw9710"
> +
> +static int
> +dw9710_probe(struct i2c_client *client, const struct i2c_device_id *id);
> +static int __exit dw9710_remove(struct i2c_client *client);
> +
> +struct dw9710_device {
> + const struct dw9710_platform_data *pdata;
> + struct v4l2_int_device *v4l2_int_device;
> + struct i2c_client *i2c_client;
> + int opened;
> + u16 current_lens_posn;
> + u16 saved_lens_posn;
> + int state;
> + int power_state;
> +};
> +
> +static const struct i2c_device_id dw9710_id[] = {
> + { DW9710_NAME, 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, dw9710_id);
> +
> +static struct i2c_driver dw9710_i2c_driver = {
> + .driver = {
> + .name = DW9710_NAME,
> + .owner = THIS_MODULE,
> + },
> + .probe = dw9710_probe,
> + .remove = __exit_p(dw9710_remove),
> + .id_table = dw9710_id,
> +};
> +
> +static struct dw9710_device dw9710 = {
> + .state = LENS_NOT_DETECTED,
> + .current_lens_posn = DEF_LENS_POSN,
> +};
> +
> +static struct vcontrol {
> + struct v4l2_queryctrl qc;
> + int current_value;
> +} video_control[] = {
> + {
> + {
> + .id = V4L2_CID_FOCUS_ABSOLUTE,
> + .type = V4L2_CTRL_TYPE_INTEGER,
> + .name = "Lens Position",
Please use the standard string for this: "Focus, Absolute".
> + .minimum = 0,
> + .maximum = MAX_FOCUS_POS,
> + .step = LENS_POSN_STEP,
> + .default_value = DEF_LENS_POSN,
> + },
> + .current_value = DEF_LENS_POSN,
> + }
> +};
> +
> +static struct i2c_driver dw9710_i2c_driver;
> +
> +/**
> + * find_vctrl - Finds the requested ID in the video control structure array
> + * @id: ID of control to search the video control array for
> + *
> + * Returns the index of the requested ID from the control structure array
> + */
> +static int
> +find_vctrl(int id)
> +{
> + int i;
> +
> + if (id < V4L2_CID_BASE)
> + return -EDOM;
> +
> + for (i = (ARRAY_SIZE(video_control) - 1); i >= 0; i--)
> + if (video_control[i].qc.id == id)
> + break;
> + if (i < 0)
> + i = -EINVAL;
> + return i;
> +}
> +
> +/**
> + * camaf_reg_read - Reads a value from a register in DW9710 Coil driver device.
> + * @client: Pointer to structure of I2C client.
> + * @value: Pointer to u16 for returning value of register to read.
> + *
> + * Returns zero if successful, or non-zero otherwise.
> + **/
> +static int camaf_reg_read(struct i2c_client *client, u16 *value)
> +{
> + int err;
> + struct i2c_msg msg[1];
> + unsigned char data[2];
> +
> + if (!client->adapter)
> + return -ENODEV;
> +
> + msg->addr = client->addr;
> + msg->flags = I2C_M_RD;
> + msg->len = 2;
> + msg->buf = data;
> +
> + data[0] = 0;
> + data[1] = 0;
> +
> + err = i2c_transfer(client->adapter, msg, 1);
> +
> + if (err >= 0) {
> + *value = err = ((data[0] & 0xFF) << 8) | (data[1]);
> + return 0;
> + }
> + return err;
> +}
> +
> +/**
> + * camaf_reg_write - Writes a value to a register in DW9710 Coil driver device.
> + * @client: Pointer to structure of I2C client.
> + * @value: Value of register to write.
> + *
> + * Returns zero if successful, or non-zero otherwise.
> + **/
> +static int camaf_reg_write(struct i2c_client *client, u16 value)
> +{
> + int err;
> + struct i2c_msg msg[1];
> + unsigned char data[2];
> + int retry = 0;
> +
> + if (!client->adapter)
> + return -ENODEV;
> +
> +again:
> + msg->addr = client->addr;
> + msg->flags = 0;
> + msg->len = 2;
> + msg->buf = data;
> +
> + data[0] = (u8)(value >> 8);
> + data[1] = (u8)(value & 0xFF);
> +
> + err = i2c_transfer(client->adapter, msg, 1);
> +
> + if (err >= 0)
> + return 0;
> +
> + if (retry <= DW9710_I2C_RETRY_COUNT) {
> + dev_dbg(&client->dev, "retry ... %d", retry);
> + retry++;
> + set_current_state(TASK_UNINTERRUPTIBLE);
> + schedule_timeout(msecs_to_jiffies(20));
> + goto again;
> + }
> + return err;
> +}
> +
> +/**
> + * dw9710_detect - Detects DW9710 Coil driver device.
> + * @client: Pointer to structure of I2C client.
> + *
> + * Returns 0 if successful, -1 if camera is off or if test register value
> + * wasn't stored properly, or returned errors from either camaf_reg_write or
> + * camaf_reg_read functions.
> + **/
> +static int dw9710_detect(struct i2c_client *client)
> +{
> + int err = 0;
> + u16 wposn = 0, rposn = 0;
> + u16 posn = 0x05;
> +
> + wposn = (CAMAF_DW9710_POWERDN(CAMAF_DW9710_ENABLE) |
> + CAMAF_DW9710_DATA(posn));
> +
> + err = camaf_reg_write(client, wposn);
> + if (err) {
> + printk(KERN_ERR "Unable to write DW9710 \n");
> + return err;
> + }
> +
> + err = camaf_reg_read(client, &rposn);
> + if (err) {
> + printk(KERN_ERR "Unable to read DW9710 \n");
> + return err;
> + }
> +
> + if (wposn != rposn) {
> + printk(KERN_ERR "W/R MISMATCH!\n");
> + return -1;
> + }
> + posn = 0;
> + wposn = (CAMAF_DW9710_POWERDN(CAMAF_DW9710_ENABLE) |
> + CAMAF_DW9710_DATA(posn));
> + err = camaf_reg_write(client, wposn);
> +
> + return err;
> +}
> +
> +/**
> + * dw9710_af_setfocus - Sets the desired focus.
> + * @posn: Desired focus position, 0 (far) - 100 (close).
> + *
> + * Returns 0 on success, -EINVAL if camera is off or focus value is out of
> + * bounds, or returned errors from either camaf_reg_write or camaf_reg_read
> + * functions.
> + **/
> +int dw9710_af_setfocus(u16 posn)
> +{
> + struct dw9710_device *af_dev = &dw9710;
> + struct i2c_client *client = af_dev->i2c_client;
> + u16 cur_focus_value = 0;
> + int ret = -EINVAL;
> +
> + if (posn > MAX_FOCUS_POS) {
> + printk(KERN_ERR "Bad posn params 0x%x \n", posn);
> + return ret;
> + }
> +
> + if ((af_dev->power_state == V4L2_POWER_OFF) ||
> + (af_dev->power_state == V4L2_POWER_STANDBY)) {
> + af_dev->current_lens_posn = posn;
> + return 0;
> + }
> +
> + ret = camaf_reg_read(client, &cur_focus_value);
> +
> + if (ret) {
> + printk(KERN_ERR "Read of current Lens position failed\n");
> + return ret;
> + }
> +
> + if (CAMAF_DW9710_DATA_R(cur_focus_value) == posn) {
> + printk(KERN_DEBUG "Device already in requested focal point\n");
> + return ret;
> + }
> +
> + ret = camaf_reg_write(client,
> + CAMAF_DW9710_POWERDN(CAMAF_DW9710_ENABLE) |
> + CAMAF_DW9710_DATA(posn));
> +
> + if (ret)
> + printk(KERN_ERR "Setfocus register write failed\n");
> + dw9710.current_lens_posn = posn;
> + return ret;
> +}
> +EXPORT_SYMBOL(dw9710_af_setfocus);
> +
> +/**
> + * dw9710_af_getfocus - Gets the focus value from device.
> + * @value: Pointer to u16 variable which will contain the focus value.
> + *
> + * Returns 0 if successful, -EINVAL if camera is off, or return value of
> + * camaf_reg_read if fails.
> + **/
> +int dw9710_af_getfocus(u16 *value)
> +{
> + int ret = -EINVAL;
> + u16 posn = 0;
> +
> + struct dw9710_device *af_dev = &dw9710;
> + struct i2c_client *client = af_dev->i2c_client;
> +
> + if ((af_dev->power_state == V4L2_POWER_OFF) ||
> + (af_dev->power_state == V4L2_POWER_STANDBY))
> + return ret;
> +
> + ret = camaf_reg_read(client, &posn);
> +
> + if (ret) {
> + printk(KERN_ERR "Read of current Lens position failed\n");
> + return ret;
> + }
> + *value = CAMAF_DW9710_DATA_R(posn);
> + dw9710.current_lens_posn = CAMAF_DW9710_DATA_R(posn);
> + return ret;
> +}
> +EXPORT_SYMBOL(dw9710_af_getfocus);
> +
> +/**
> + * ioctl_queryctrl - V4L2 lens interface handler for VIDIOC_QUERYCTRL ioctl
> + * @s: pointer to standard V4L2 device structure
> + * @qc: standard V4L2 VIDIOC_QUERYCTRL ioctl structure
> + *
> + * If the requested control is supported, returns the control information
> + * from the video_control[] array. Otherwise, returns -EINVAL if the
> + * control is not supported.
> + */
> +static int ioctl_queryctrl(struct v4l2_int_device *s,
> + struct v4l2_queryctrl *qc)
> +{
> + int i;
> +
> + i = find_vctrl(qc->id);
> + if (i == -EINVAL)
> + qc->flags = V4L2_CTRL_FLAG_DISABLED;
> +
> + if (i < 0)
> + return -EINVAL;
> +
> + *qc = video_control[i].qc;
> + return 0;
> +}
> +
> +/**
> + * ioctl_g_ctrl - V4L2 DW9710 lens interface handler for VIDIOC_G_CTRL ioctl
> + * @s: pointer to standard V4L2 device structure
> + * @vc: standard V4L2 VIDIOC_G_CTRL ioctl structure
> + *
> + * If the requested control is supported, returns the control's current
> + * value from the video_control[] array. Otherwise, returns -EINVAL
> + * if the control is not supported.
> + */
> +static int ioctl_g_ctrl(struct v4l2_int_device *s,
> + struct v4l2_control *vc)
> +{
> + struct vcontrol *lvc;
> + int i;
> + u16 curr_posn;
> +
> + i = find_vctrl(vc->id);
> + if (i < 0)
> + return -EINVAL;
> + lvc = &video_control[i];
> +
> + switch (vc->id) {
> + case V4L2_CID_FOCUS_ABSOLUTE:
> + if (dw9710_af_getfocus(&curr_posn))
> + return -EFAULT;
> + vc->value = curr_posn;
> + lvc->current_value = curr_posn;
> + break;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * ioctl_s_ctrl - V4L2 DW9710 lens interface handler for VIDIOC_S_CTRL ioctl
> + * @s: pointer to standard V4L2 device structure
> + * @vc: standard V4L2 VIDIOC_S_CTRL ioctl structure
> + *
> + * If the requested control is supported, sets the control's current
> + * value in HW (and updates the video_control[] array). Otherwise,
> + * returns -EINVAL if the control is not supported.
> + */
> +static int ioctl_s_ctrl(struct v4l2_int_device *s,
> + struct v4l2_control *vc)
> +{
> + int retval = -EINVAL;
> + int i;
> + struct vcontrol *lvc;
> +
> + i = find_vctrl(vc->id);
> + if (i < 0)
> + return -EINVAL;
> + lvc = &video_control[i];
> +
> + switch (vc->id) {
> + case V4L2_CID_FOCUS_ABSOLUTE:
> + retval = dw9710_af_setfocus(vc->value);
> + if (!retval)
> + lvc->current_value = vc->value;
> + break;
> + }
> +
> + return retval;
> +}
> +
> +/**
> + * ioctl_g_priv - V4L2 sensor interface handler for vidioc_int_g_priv_num
> + * @s: pointer to standard V4L2 device structure
> + * @p: void pointer to hold sensor's private data address
> + *
> + * Returns device's (sensor's) private data area address in p parameter
> + */
> +static int ioctl_g_priv(struct v4l2_int_device *s, void *p)
> +{
> + struct dw9710_device *lens = s->priv;
> +
> + return lens->pdata->priv_data_set(p);
> +
> +}
> +
> +/**
> + * ioctl_s_power - V4L2 sensor interface handler for vidioc_int_s_power_num
> + * @s: pointer to standard V4L2 device structure
> + * @on: power state to which device is to be set
> + *
> + * Sets devices power state to requrested state, if possible.
> + */
> +static int ioctl_s_power(struct v4l2_int_device *s, enum v4l2_power on)
> +{
> + struct dw9710_device *lens = s->priv;
> + struct i2c_client *c = lens->i2c_client;
> + int rval;
> +
> + rval = lens->pdata->power_set(on);
> + if (rval < 0) {
> + dev_err(&c->dev, "Unable to set the power state: " DRIVER_NAME
> + " lens HW\n");
> + return rval;
> + }
> +
> + if ((on == V4L2_POWER_ON) && (lens->state == LENS_NOT_DETECTED)) {
> + rval = dw9710_detect(c);
> + if (rval < 0) {
> + dev_err(&c->dev, "Unable to detect "
> + DRIVER_NAME " lens HW\n");
> + printk(KERN_ERR "Unable to detect "
> + DRIVER_NAME " lens HW\n");
> + lens->state = LENS_NOT_DETECTED;
> + return rval;
> + }
> + lens->state = LENS_DETECTED;
> + pr_info(DRIVER_NAME " lens HW detected\n");
> + }
> +
> + if ((lens->power_state == V4L2_POWER_STANDBY) && (on == V4L2_POWER_ON)
> + && (lens->state == LENS_DETECTED))
> + dw9710_af_setfocus(lens->current_lens_posn);
> +
> + lens->power_state = on;
> + return 0;
> +}
> +
> +static struct v4l2_int_ioctl_desc dw9710_ioctl_desc[] = {
> + { .num = vidioc_int_s_power_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_s_power },
> + { .num = vidioc_int_g_priv_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_g_priv },
> + { .num = vidioc_int_queryctrl_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_queryctrl },
> + { .num = vidioc_int_g_ctrl_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_g_ctrl },
> + { .num = vidioc_int_s_ctrl_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_s_ctrl },
> +};
> +
> +static struct v4l2_int_slave dw9710_slave = {
> + .ioctls = dw9710_ioctl_desc,
> + .num_ioctls = ARRAY_SIZE(dw9710_ioctl_desc),
> +};
> +
> +static struct v4l2_int_device dw9710_int_device = {
> + .module = THIS_MODULE,
> + .name = DRIVER_NAME,
> + .priv = &dw9710,
> + .type = v4l2_int_type_slave,
> + .u = {
> + .slave = &dw9710_slave,
> + },
> +};
> +
> +/**
> + * dw9710_probe - Probes the driver for valid I2C attachment.
> + * @client: Pointer to structure of I2C client.
> + *
> + * Returns 0 if successful, or -EBUSY if unable to get client attached data.
> + **/
> +static int
> +dw9710_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> + struct dw9710_device *lens = &dw9710;
> + int err;
> +
> + dev_info(&client->dev, "dw9710 probe called....\n");
> +
> + if (i2c_get_clientdata(client)) {
> + printk(KERN_ERR " DTA BUSY %s\n", client->name);
> + return -EBUSY;
> + }
> +
> + lens->pdata = client->dev.platform_data;
> +
> + if (!lens->pdata) {
> + dev_err(&client->dev, "no platform data?\n");
> + return -ENODEV;
> + }
> +
> + lens->v4l2_int_device = &dw9710_int_device;
> +
> + lens->i2c_client = client;
> + i2c_set_clientdata(client, lens);
> +
> + err = v4l2_int_device_register(lens->v4l2_int_device);
> + if (err) {
> + printk(KERN_ERR "Failed to Register "
> + DRIVER_NAME " as V4L2 device.\n");
> + i2c_set_clientdata(client, NULL);
> + } else {
> + printk(KERN_ERR "Registered "
> + DRIVER_NAME " as V4L2 device.\n");
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * dw9710_remove - Routine when device its unregistered from I2C
> + * @client: Pointer to structure of I2C client.
> + *
> + * Returns 0 if successful, or -ENODEV if the client isn't attached.
> + **/
> +static int __exit dw9710_remove(struct i2c_client *client)
> +{
> + if (!client->adapter)
> + return -ENODEV;
> +
> + i2c_set_clientdata(client, NULL);
> + return 0;
> +}
> +
> +/**
> + * dw9710_init - Module initialisation.
> + *
> + * Returns 0 if successful, or -EINVAL if device couldn't be initialized, or
> + * added as a character device.
> + **/
> +static int __init dw9710_init(void)
> +{
> + int err = -EINVAL;
> +
> + err = i2c_add_driver(&dw9710_i2c_driver);
> + if (err)
> + goto fail;
> + return err;
> +fail:
> + printk(KERN_ERR "Failed to register " DRIVER_NAME ".\n");
> + return err;
> +}
> +late_initcall(dw9710_init);
> +
> +/**
> + * dw9710_cleanup - Module cleanup.
> + **/
> +static void __exit dw9710_cleanup(void)
> +{
> + i2c_del_driver(&dw9710_i2c_driver);
> +}
> +module_exit(dw9710_cleanup);
> +
> +MODULE_AUTHOR("Texas Instruments");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("DW9710 LENS driver");
> diff --git a/drivers/media/video/dw9710.h b/drivers/media/video/dw9710.h
> new file mode 100644
> index 0000000..e4050a4
> --- /dev/null
> +++ b/drivers/media/video/dw9710.h
> @@ -0,0 +1,59 @@
> +/*
> + * drivers/media/video/dw9710.h
> + *
> + * Register defines for Auto Focus device
> + *
> + * Copyright (C) 2008 Texas Instruments.
> + *
> + * Contributors:
> + * Troy Laramy <t-laramy@ti.com>
> + * Mohit Jalori <mjalori@ti.com>
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2. This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + *
> + */
> +
> +#ifndef CAMAF_DW9710_H
> +#define CAMAF_DW9710_H
> +
> +#define DW9710_AF_I2C_ADDR 0x0C
> +#define DW9710_NAME "DW9710"
> +#define DW9710_I2C_RETRY_COUNT 5
> +#define MAX_FOCUS_POS 0xFF
> +
> +#define CAMAF_DW9710_DISABLE 0x1
> +#define CAMAF_DW9710_ENABLE 0x0
> +#define CAMAF_DW9710_POWERDN(ARG) (((ARG) & 0x1) << 15)
> +#define CAMAF_DW9710_POWERDN_R(ARG) (((ARG) >> 15) & 0x1)
> +
> +#define CAMAF_DW9710_DATA(ARG) (((ARG) & 0xFF) << 6)
> +#define CAMAF_DW9710_DATA_R(ARG) (((ARG) >> 6) & 0xFF)
> +#define CAMAF_FREQUENCY_EQ1(mclk) ((u16)(mclk/16000))
> +
> +/* State of lens */
> +#define LENS_DETECTED 1
> +#define LENS_NOT_DETECTED 0
> +
> +/* Focus control values */
> +#define DEF_LENS_POSN 0 /* 0x7F */
> +#define LENS_POSN_STEP 1
> +
> +
> +/**
> + * struct dw9710_platform_data - platform data values and access functions
> + * @power_set: Power state access function, zero is off, non-zero is on.
> + * @priv_data_set: device private data (pointer) access function
> + */
> +struct dw9710_platform_data {
> + int (*power_set)(enum v4l2_power power);
> + int (*priv_data_set)(void *);
> +};
> +
> +/*
> + * Sets the specified focus value [0(far) - 100(near)]
> + */
> +int dw9710_af_setfocus(u16 posn);
> +int dw9710_af_getfocus(u16 *value);
> +#endif /* End of of CAMAF_DW9710_H */
This header seems to be a mix of kernel-public and driver-private things.
Please split this up. Anything kernel-public should go to
include/media/dw9710.h. Driver-private data should either be in the source
or in a header next to the source.
Regards,
Hans
> --
> 1.5.6.5
>
>
> --
> video4linux-list mailing list
> Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> https://www.redhat.com/mailman/listinfo/video4linux-list
>
>
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [REVIEW PATCH 13/14] OMAP: CAM: Add Lens Driver
2008-12-12 6:03 ` Alexey Klimov
(?)
(?)
@ 2008-12-16 20:24 ` Aguirre Rodriguez, Sergio Alberto
-1 siblings, 0 replies; 8+ messages in thread
From: Aguirre Rodriguez, Sergio Alberto @ 2008-12-16 20:24 UTC (permalink / raw)
To: Alexey Klimov
Cc: video4linux-list, linux-omap, Sakari Ailus, Tuukka.O Toivonen,
Nagalla, Hari
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Alexey Klimov
> Sent: Friday, December 12, 2008 12:03 AM
> On Thu, Dec 11, 2008 at 11:38 PM, Aguirre Rodriguez, Sergio Alberto
> <saaguirre@ti.com> wrote:
> > >From 1341b74f5a90e4aa079a4fcb4fe2127ff344cce7 Mon Sep 17 00:00:00 2001
> > From: Sergio Aguirre <saaguirre@ti.com>
> > Date: Thu, 11 Dec 2008 13:35:46 -0600
> > Subject: [PATCH] OMAP: CAM: Add Lens Driver
> >
> > This adds the DW9710 Lens driver
> >
> > Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
> > ---
> > +#define DRIVER_NAME "dw9710"
>
> Here you define DRIVER_NAME. I saw patch(for another module) that
> remover MODULE_NAME and places VIVI_MODULE_NAME there. Here it is:
> http://linuxtv.org/hg/v4l-dvb/rev/58b95134acb8
>
> May be it's better to define something like this: #define
> DW9710_DRIVER_NAME "dw9710" ?
>
> Probably it will help you to avoid possible namespace conflicts and
> code become a bit more readible.
[Aguirre, Sergio] You're right. Thanks.
>
> > +static int
> > +dw9710_probe(struct i2c_client *client, const struct i2c_device_id
> *id);
> > +static int __exit dw9710_remove(struct i2c_client *client);
> > +
> > +struct dw9710_device {
> > + const struct dw9710_platform_data *pdata;
> > + struct v4l2_int_device *v4l2_int_device;
> > + struct i2c_client *i2c_client;
> > + int opened;
> > + u16 current_lens_posn;
> > + u16 saved_lens_posn;
> > + int state;
> > + int power_state;
> > +};
> > +
> > +static const struct i2c_device_id dw9710_id[] = {
> > + { DW9710_NAME, 0 },
> > + { }
> > +};
>
> Hmmm, looks like you already defined DW9710_NAME in header-file. Why
> didn't you reformat to use _one_ define for this and previous place ?
>
> > +MODULE_DEVICE_TABLE(i2c, dw9710_id);
> > +
> > +static struct i2c_driver dw9710_i2c_driver = {
> > + .driver = {
> > + .name = DW9710_NAME,
>
> Actually, the same thing here.
>
[Aguirre, Sergio] Ok, done. Now I use only one define: DW9710_NAME. Thanks.
> > +static int
> > +find_vctrl(int id)
> > +{
> > + int i;
> > +
> > + if (id < V4L2_CID_BASE)
> > + return -EDOM;
> > +
> > + for (i = (ARRAY_SIZE(video_control) - 1); i >= 0; i--)
> > + if (video_control[i].qc.id == id)
> > + break;
> > + if (i < 0)
> > + i = -EINVAL;
> > + return i;
> > +}
>
> In this function you use return after if, and in second place you set
> i equals to -EINVAL, and then return. Probably, to make it more easily
> to read it's better to do such thing:
>
> if (i < 0)
> return -EINVAL;
>
[Aguirre, Sergio] Ok, I agree it's a bit confusing... I have rewritten the function to be more simple, is it ok now?:
static int find_vctrl(int id)
{
int i;
if (id < V4L2_CID_BASE)
return -EDOM;
for (i = (ARRAY_SIZE(video_control) - 1); i >= 0; i--) {
if (video_control[i].qc.id == id)
return i;
}
return -EINVAL;
}
> > + if (wposn != rposn) {
> > + printk(KERN_ERR "W/R MISMATCH!\n");
>
> If I were writing it, I'd do the following:
>
> printk(KERN_ERR DRIVER_NAME "W/R MISMATCH!\n");
>
> And driver-name have to be unique, see above.
> To be onest it's better to provide module name to dmesg if you(or any
> user) want to catch bad behavour.
[Aguirre, Sergio] Ok, point taken. Thanks.
> > +int dw9710_af_getfocus(u16 *value)
> > +{
> > + int ret = -EINVAL;
> > + u16 posn = 0;
>
> Why just don't use int ret;
> And later in code use return -EINVAL; ?
> Anyway, you change ret variable with reg_read call.
[Aguirre, Sergio] Ok, Makes sense. Thanks.
>
> > + struct dw9710_device *af_dev = &dw9710;
> > + struct i2c_client *client = af_dev->i2c_client;
> > +
> > + if ((af_dev->power_state == V4L2_POWER_OFF) ||
> > + (af_dev->power_state == V4L2_POWER_STANDBY))
> > + return ret;
> > +
> > + ret = camaf_reg_read(client, &posn);
> > +
> > + if (ret) {
> > + printk(KERN_ERR "Read of current Lens position
> failed\n");
> > + return ret;
> > + }
> > + *value = CAMAF_DW9710_DATA_R(posn);
> > + dw9710.current_lens_posn = CAMAF_DW9710_DATA_R(posn);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(dw9710_af_getfocus);
> > + if ((on == V4L2_POWER_ON) && (lens->state == LENS_NOT_DETECTED))
> {
> > + rval = dw9710_detect(c);
> > + if (rval < 0) {
> > + dev_err(&c->dev, "Unable to detect "
> > + DRIVER_NAME " lens HW\n");
> > + printk(KERN_ERR "Unable to detect "
> > + DRIVER_NAME " lens HW\n");
>
> Two thing here. As i know it's more preferable to use dev_macro
> instead of printk if possible.
> As i know(may be i'm wrong) it's not good to use DRIVER_NAME in dev_err
> here.
> Why do you print the same text two times here ?
[Aguirre, Sergio] Ok, cleaning this up. Thanks
> > +static int __init dw9710_init(void)
> > +{
> > + int err = -EINVAL;
>
> int err = something, and then changed with call to i2c..
>
> This is all i can suggest, it's not about functionality, it's about
> coding style..
> Please, use dev_macros if possible instead of printk.
>
[Aguirre, Sergio] Ok, will do that. Thanks
I really appreciate all your time doing this.
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2008-12-16 20:25 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-12-11 20:38 [REVIEW PATCH 13/14] OMAP: CAM: Add Lens Driver Aguirre Rodriguez, Sergio Alberto
2008-12-12 6:03 ` Alexey Klimov
2008-12-12 6:03 ` Alexey Klimov
2008-12-12 8:44 ` Robert William Fuller
2008-12-12 12:25 ` Douglas Schilling Landgraf
2008-12-16 20:24 ` Aguirre Rodriguez, Sergio Alberto
2008-12-16 17:38 ` Hans Verkuil
2008-12-16 17:38 ` Hans Verkuil
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.