Hello, Dominic
May i make few comments ?
On Wed, 2009-02-04 at 12:40 -0600, Dominic Curran wrote:
> Hi
> Below is a new driver for the LV8093 lens actuator.
>
> Of course all comments are welcome, however I have a specific issue that i am
> concerned about...
>
> The lens position cannot be read back from this device, and it
> only takes relative positions (it uses V4L2_CID_FOCUS_RELATIVE ctrl ID).
>
> The device has only one read register which contains a single BUSY bit.
> For large relative lens movements the device can be busy for sometime and we need
> to know when the lens has stopped moving.
>
> My question is what is the most appropriate mechanism to read the BUSY bit ?
>
> Currently the driver uses the VIDIOC_G_CTRL ioctl + V4L2_CID_FOCUS_RELATIVE ctrl
> ID to return the BUSY bit. Is this acceptable ?
>
> The only other solution I can think of is:
> * Use does VIDIOC_S_CTRL ioctl
> .id = V4L2_CID_FOCUS_RELATIVE
> .value = 0
> * If driver detects .value=0 then it reads the BUSY bit and returns either
> 0 - Ready
> -EBUSY - lens busy
>
> A 3rd solution is to read the BUSY bit everytime after a write and not return
> until device is ready. However this adds extra time to the operation
> (particularly for small lens moves) and I would like the user to be in change of
> when the reads of BUSY bit occur.
>
> I hope I have made my question clear.
>
> Any guidance appreciated
> thanks
> dom
>
> From: Dominic Curran <dcurran@xxxxxx>
> Subject: [PATCH v2 1/3] LV8093: Add driver for LV8093 lens actuator.
>
> Support for the Sanyo LV8093CS piezo-actuator lens driver.
> The lens position cannot be read back from this device, and it
> only takes relative positions. Thus it:
> - Supports the VIDIOC_G_CTRL ioctl to return BUSY bit (0 - Ready, ~0 - Busy)
> - Supports the VIDIOC_S_CTRL ioctl with the V4L2_CID_FOCUS_RELATIVE control ID.
>
> One relative step requested through the V4L2_CID_FOCUS_RELATIVE control will
> produce a 5.0um step @ 5.2mm/s of the lens.
>
> Signed-off-by: Kraig Proehl <kraig.proehl@xxxxxx>
> Signed-off-by: Dominic Curran <dcurran@xxxxxx>
> ---
> drivers/media/video/lv8093.c | 595 +++++++++++++++++++++++++++++++++++++++++++
> drivers/media/video/lv8093.h | 92 ++++++
> 2 files changed, 687 insertions(+)
>
> Index: omapzoom04/drivers/media/video/lv8093.c
> ===================================================================
> --- /dev/null
> +++ omapzoom04/drivers/media/video/lv8093.c
> @@ -0,0 +1,595 @@
> +/*
> + * drivers/media/video/lv8093.c
> + *
> + * LV8093 Piezo Motor (LENS) driver
> + *
> + * Copyright (C) 2008-2009 Texas Instruments.
> + * Copyright (C) 2009 Hewlett-Packard.
> + *
> + * This package is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
> + * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
> + */
> +
> +#include <linux/mutex.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +
> +#include <media/v4l2-int-device.h>
> +#include <mach/gpio.h>
> +
> +#include "lv8093.h"
> +
> +static int
> +lv8093_probe(struct i2c_client *client, const struct i2c_device_id *id);
> +static int __exit lv8093_remove(struct i2c_client *client);
> +
> +struct lv8093_device {
> + const struct lv8093_platform_data *pdata;
> + struct v4l2_int_device *v4l2_int_device;
> + struct i2c_client *i2c_client;
> + int state;
> + int power_state;
> +};
> +
> +static const struct i2c_device_id lv8093_id[] = {
> + {LV8093_NAME, 0},
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, lv8093_id);
> +
> +static struct i2c_driver lv8093_i2c_driver = {
> + .driver = {
> + .name = LV8093_NAME,
> + .owner = THIS_MODULE,
> + },
> + .probe = lv8093_probe,
> + .remove = __exit_p(lv8093_remove),
> + .id_table = lv8093_id,
> +};
> +
> +static struct lv8093_device lv8093 = {
> + .state = LENS_NOT_DETECTED,
> +};
> +
> +static struct vcontrol {
> + struct v4l2_queryctrl qc;
> +} video_control[] = {
> + {
> + {
> + .id = V4L2_CID_FOCUS_RELATIVE,
> + .type = V4L2_CTRL_TYPE_INTEGER,
> + .name = "Lens Relative Position",
> + .minimum = 0,
> + .maximum = 0,
> + .step = LV8093_MAX_RELATIVE_STEP,
> + .default_value = 0,
> + }
> + ,}
> +};
> +
> +static struct i2c_driver lv8093_i2c_driver;
> +
> +static struct lv8093_lens_settings {
> + u8 reg;
> + u8 val;
> +} lens_settings[] = {
> + { /* Set control register */
> + .reg = CAMAF_LV8093_CTL_REG,
> + .val = CAMAF_LV8093_GATE0 |
> + CAMAF_LV8093_ENIN |
> + CAMAF_LV8093_CKSEL_ONE |
> + CAMAF_LV8093_RET2 |
> + CAMAF_LV8093_INIT_OFF,
> + },
> + { /* Specify number of clocks per period */
> + .reg = CAMAF_LV8093_RST_REG,
> + .val = (LV8093_CLK_PER_PERIOD - 1),
> + },
> + { /* Set the GATE_A pulse set value */
> + .reg = CAMAF_LV8093_GTAS_REG,
> + .val = (LV8093_TIME_GATEA + 1),
> + },
> + { /* Set the GATE_B pulse reset value */
> + .reg = CAMAF_LV8093_GTBR_REG,
> + .val = (LV8093_TIME_GATEA + 1 + LV8093_TIME_OFF),
> + },
> + { /* Set the GATE_B pulse set value */
> + .reg = CAMAF_LV8093_GTBS_REG,
> + .val = (LV8093_TIME_GATEA + 1 +
> + LV8093_TIME_OFF + LV8093_TIME_GATEB),
> + },
> + { /* Specific the number of output pulse steps */
> + .reg = CAMAF_LV8093_STP_REG,
> + .val = LV8093_STP,
> + },
> + { /* Set the number of swing back of init sequence performed */
> + .reg = CAMAF_LV8093_MOV_REG,
> + .val = 0,
> + },
> +};
> +
> +/**
> + * 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;
> +}
> +
> +/**
> + * lv8093_reg_read - Reads a value from a register in LV8093 Piezo 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 lv8093_reg_read(struct i2c_client *client, u8 *value)
> +{
> + int err;
> + struct i2c_msg msg[1];
> +
> + if (!client->adapter)
> + return -ENODEV;
> +
> + msg->addr = client->addr;
> + msg->flags = I2C_M_RD;
> + msg->len = 1;
> + msg->buf = value;
> +
> + err = i2c_transfer(client->adapter, msg, 1);
> +
> + if (err < 0)
> + v4l_err(client, "i2c read failed with error %i", err);
> +
> + return err;
> +}
> +
> +/**
> + * lv8093_reg_write - Writes a value to a register in LV8093 Piezo driver
> device.
> + * @client: Pointer to structure of I2C client.
> + * @reg: Register to write.
> + * @value: Value of register to write.
> + *
> + * Returns zero or +ve if successful, -ve for error.
> + **/
> +static int lv8093_reg_write(struct i2c_client *client, u8 reg, u8 value)
> +{
> + int err;
> + struct i2c_msg msg[1];
> + unsigned char data[2];
> +
> + if (!client->adapter)
> + return -ENODEV;
> +
> + msg->addr = client->addr;
> + msg->flags = 0;
> + msg->len = 2;
> + msg->buf = data;
> +
> + data[0] = reg;
> + data[1] = value;
> +
> + err = i2c_transfer(client->adapter, msg, 1);
> + if (err < 0)
> + v4l_err(client, "i2c write failed with error %i", err);
> +
> + return err;
> +}
> +
> +/**
> + * lv8093_detect - Detects LV8093 Piezo 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 return error from lv8093_reg_write function.
> + **/
> +static int lv8093_detect(struct i2c_client *client)
> +{
> + int err = 0;
> + u8 rposn = 0;
> +
> + err = lv8093_reg_write(client, CAMAF_LV8093_CTL_REG,
> + CAMAF_LV8093_GATE0 |
> + CAMAF_LV8093_CKSEL_ONE |
> + CAMAF_LV8093_RET2 |
> + CAMAF_LV8093_INIT_OFF);
> +
> + if (err < 0) {
> + v4l_err(client, "Unable to write LV8093\n");
> + return err;
> + }
> +
> + err = lv8093_reg_read(client, &rposn);
> + if (err < 0) {
> + v4l_err(client, "Unable to read LV8093\n");
> + return err;
> + }
> +
> + return err;
> +}
> +
> +/**
> + * lv8093_reginit - Initializes LV8093 Piezo driver device.
> + * @client: Pointer to structure of I2C client.
> + *
> + * Returns 0 if successful, or returns errors from lv8093_reg_write.
> + **/
> +static int lv8093_reginit(struct i2c_client *client)
> +{
> + int i, err = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(lens_settings); i++) {
> +
> + err = lv8093_reg_write(client,
> + lens_settings[i].reg, lens_settings[i].val);
> +
> + if (err < 0) {
> + v4l_err(client, "Unable to initialize LV8093\n");
> + return err;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * lv8093_af_setfocus - Sets the desired focus.
> + * @relpos: Relative focus position:
> + * -ve - Direction INFINITY.
> + * +ve - Direction MACRO.
> + * abs(relpos) gives number of steps in desired direction.
> + *
> + * Returns 0 on success, -EINVAL if camera is off or returned errors
> + * from lv8093_reg_write function.
> + **/
> +int lv8093_af_setfocus(s16 relpos)
> +{
> + struct lv8093_device *af_dev = &lv8093;
> + struct i2c_client *client = af_dev->i2c_client;
> + u8 data;
> + u8 num_pulses = abs(relpos);
> + int ret;
> +
> + if ((af_dev->power_state == V4L2_POWER_OFF) ||
> + (af_dev->power_state == V4L2_POWER_STANDBY))
> + return -EINVAL;
> +
> + if (relpos >= 0)
> + /* Move lens in Macro direction */
> + data = CAMAF_LV8093_MAC_DIR | num_pulses;
> + else
> + /* Move lens in Infinite direction */
> + data = CAMAF_LV8093_INF_DIR | num_pulses;
> +
> + ret = lv8093_reg_write(client, CAMAF_LV8093_DRVPLS_REG, data);
> + if (ret < 0) {
> + v4l_err(client, "Unable to write " LV8093_NAME
> + " lens HW\n");
> + return -EINVAL;
> + }
> +
> + return ret;
> +}
> +
> +/**
> + * lv8093_get_busy - Get busy bit.
> + * @busy: where to store the output
> + *
> + * Returns 0 on success, negative on error.
> + **/
> +int lv8093_get_busy(int *busy)
> +{
> + struct lv8093_device *af_dev = &lv8093;
> + struct i2c_client *client = af_dev->i2c_client;
> + int ret;
> + u8 regval;
> +
> + ret = lv8093_reg_read(client, ®val);
> +
> + if (ret < 0) {
> + v4l_err(client, "Unable to read " LV8093_NAME
> + " lens HW\n");
> + return -EINVAL;
> + }
> +
> + *busy = regval & CAMAF_LV8093_BUSY;
> + return ret;
> +}
> +
> +/**
> + * 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_s_ctrl - V4L2 LV8093 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.
> + * 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_RELATIVE:
> + retval = lv8093_af_setfocus(vc->value);
> + break;
> + }
> +
> + return retval;
> +}
> +
> +/**
> + * ioctl_g_ctrl - V4L2 LV8093 lens interface handler for VIDIOC_G_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.
> + * Otherwise, returns -EINVAL if the control is not supported.
> + */
> +static int ioctl_g_ctrl(struct v4l2_int_device *s, struct v4l2_control *vc)
> +{
> + int retval = -EINVAL;
> + int i;
> + struct vcontrol *lvc;
> + int busy;
> +
> + i = find_vctrl(vc->id);
> + if (i < 0)
> + return -EINVAL;
> + lvc = &video_control[i];
> +
> + switch (vc->id) {
> + case V4L2_CID_FOCUS_RELATIVE:
> + retval = lv8093_get_busy(&busy);
> + vc->value = busy;
> + 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 lv8093_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 requested state, if possible.
> + */
> +static int ioctl_s_power(struct v4l2_int_device *s, enum v4l2_power on)
> +{
> + struct lv8093_device *lens = s->priv;
> + struct i2c_client *c = lens->i2c_client;
> + int rval;
> +
> + if (lens->pdata->power_set)
> + rval = lens->pdata->power_set(on);
> +
> + lens->power_state = on;
> +
> + if ((on == V4L2_POWER_ON) && (lens->state == LENS_NOT_DETECTED)) {
> + rval = lv8093_detect(c);
> + if (rval < 0) {
> + v4l_err(c, "Unable to detect " LV8093_NAME
> + " lens HW\n");
> + lens->state = LENS_NOT_DETECTED;
> + return rval;
> + }
> + lens->state = LENS_DETECTED;
> + pr_info(LV8093_NAME " Lens HW detected\n");
> +
> + rval = lv8093_reginit(c);
> + if (rval < 0) {
> + v4l_err(c, "Unable to initialize " LV8093_NAME
> + " lens HW\n");
> + lens->state = LENS_NOT_DETECTED;
> + return rval;
> + }
> + }
> +
> + if ((lens->power_state == V4L2_POWER_STANDBY) && (on == V4L2_POWER_ON)
> + && (lens->state == LENS_DETECTED)) {
> + rval = lv8093_reginit(c);
> + if (rval < 0) {
> + v4l_err(c, "Unable to initialize " LV8093_NAME
> + " lens HW\n");
> + lens->state = LENS_NOT_DETECTED;
> + return rval;
> + }
> + }
> + return 0;
> +}
> +
> +static struct v4l2_int_ioctl_desc lv8093_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_s_ctrl_num,
> + .func = (v4l2_int_ioctl_func *) ioctl_s_ctrl},
> + {.num = vidioc_int_g_ctrl_num,
> + .func = (v4l2_int_ioctl_func *) ioctl_g_ctrl},
> +};
> +
> +static struct v4l2_int_slave lv8093_slave = {
> + .ioctls = lv8093_ioctl_desc,
> + .num_ioctls = ARRAY_SIZE(lv8093_ioctl_desc),
> +};
> +
> +static struct v4l2_int_device lv8093_int_device = {
> + .module = THIS_MODULE,
> + .name = LV8093_NAME,
> + .priv = &lv8093,
> + .type = v4l2_int_type_slave,
> + .u = {
> + .slave = &lv8093_slave,
> + },
> +};
> +
> +/**
> + * lv8093_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
> +lv8093_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> + struct lv8093_device *lens = &lv8093;
> + int err;
> +
> + if (i2c_get_clientdata(client)) {
> + v4l_err(client, "DTA BUSY %s\n", client->name);
> + return -EBUSY;
> + }
> +
> + lens->pdata = client->dev.platform_data;
> +
> + if (!lens->pdata) {
> + v4l_err(client, "no platform data?\n");
> + return -ENODEV;
> + }
> +
> + lens->v4l2_int_device = &lv8093_int_device;
> +
> + lens->i2c_client = client;
> + i2c_set_clientdata(client, lens);
> +
> + err = v4l2_int_device_register(lens->v4l2_int_device);
> + if (err) {
> + v4l_err(client, "Failed to Register "
> + LV8093_NAME " as V4L2 device.\n");
> + i2c_set_clientdata(client, NULL);
> + } else {
> + printk(KERN_ERR "Registered " LV8093_NAME
> + " as V4L2 device.\n");
This is cool :)
First, it's better to use v4l_err here (because you use v4l_err in
module).
Second, it should be not KERN_ERR nor v4l_err! Your code says that you
want printk(KERN_INFO "message") (see also comments below)
Third, this two lines (if joined) fit to 80 symbols.
> + }
> +
> + return err;
> +}
> +
> +/**
> + * lv8093_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 lv8093_remove(struct i2c_client *client)
> +{
> + struct lv8093_device *lens = i2c_get_clientdata(client);
> +
> + if (!client->adapter)
> + return -ENODEV;
> +
> + v4l2_int_device_unregister(lens->v4l2_int_device);
> + i2c_set_clientdata(client, NULL);
> + return 0;
> +}
> +
> +/**
> + * lv8093_init - Module initialisation.
> + *
> + * Returns 0 if successful, or -EINVAL if device couldn't be initialized, or
> + * added as a character device.
> + **/
> +static int __init lv8093_init(void)
> +{
> + int err;
> +
> + err = i2c_add_driver(&lv8093_i2c_driver);
> + if (err)
> + goto fail;
> + printk(KERN_INFO "Registered " LV8093_NAME
> + " as i2c device.\n");
Well, i don't know if this is really important, but i saw in different
e-mails that developers prefer to use one interface for messages
everywhere in driver. I mean that you use pr_info and printk
(KERN_INFO.. If you switch to one interface it becames more comfortable
to read code.
If you decide to use pr_info you can also use pr_err for your
convenience.
--
Best regards, Klimov Alexey
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
