* [PATCH] dw9714: Initial driver for dw9714 VCM
@ 2017-05-07 11:33 rajmohan.mani
2017-05-08 10:59 ` Hans Verkuil
2017-05-08 20:55 ` Sakari Ailus
0 siblings, 2 replies; 13+ messages in thread
From: rajmohan.mani @ 2017-05-07 11:33 UTC (permalink / raw)
To: linux-media, mchehab; +Cc: Rajmohan Mani
From: Rajmohan Mani <rajmohan.mani@intel.com>
DW9714 is a 10 bit DAC, designed for linear
control of voice coil motor.
This driver creates a V4L2 subdevice and
provides control to set the desired focus.
Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
---
drivers/media/i2c/Kconfig | 9 ++
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/dw9714.c | 333 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 343 insertions(+)
create mode 100644 drivers/media/i2c/dw9714.c
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index fd181c9..4f0a7ad 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -300,6 +300,15 @@ config VIDEO_AD5820
This is a driver for the AD5820 camera lens voice coil.
It is used for example in Nokia N900 (RX-51).
+config VIDEO_DW9714
+ tristate "DW9714 lens voice coil support"
+ depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
+ ---help---
+ This is a driver for the DW9714 camera lens voice coil.
+ DW9714 is a 10 bit DAC with 120mA output current sink
+ capability. This is designed for linear control of
+ voice coil motors, controlled via I2C serial interface.
+
config VIDEO_SAA7110
tristate "Philips SAA7110 video decoder"
depends on VIDEO_V4L2 && I2C
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 62323ec..987bd1f 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
obj-$(CONFIG_VIDEO_AD5820) += ad5820.o
+obj-$(CONFIG_VIDEO_DW9714) += dw9714.o
obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o
diff --git a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c
new file mode 100644
index 0000000..276d3f2
--- /dev/null
+++ b/drivers/media/i2c/dw9714.c
@@ -0,0 +1,333 @@
+/*
+ * Copyright (c) 2015--2017 Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+
+#define DW9714_NAME "dw9714"
+#define DW9714_MAX_FOCUS_POS 1023
+#define DW9714_CTRL_STEPS 16 /* Keep this value power of 2 */
+#define DW9714_CTRL_DELAY_US 1000
+/*
+ * S[3:2] = 0x00, codes per step for "Linear Slope Control"
+ * S[1:0] = 0x00, step period
+ */
+#define DW9714_DEFAULT_S 0x0
+#define DW9714_VAL(data, s) (u16)((data) << 4 | (s))
+
+/* dw9714 device structure */
+struct dw9714_device {
+ struct i2c_client *client;
+ struct v4l2_ctrl_handler ctrls_vcm;
+ struct v4l2_subdev sd;
+ u16 current_val;
+};
+
+#define to_dw9714_vcm(_ctrl) \
+ container_of(_ctrl->handler, struct dw9714_device, ctrls_vcm)
+
+static int dw9714_i2c_write(struct i2c_client *client, u16 data)
+{
+ const int num_msg = 1;
+ int ret;
+ u16 val = cpu_to_be16(data);
+ struct i2c_msg msg = {
+ .addr = client->addr,
+ .flags = 0,
+ .len = sizeof(val),
+ .buf = (u8 *) &val,
+ };
+
+ ret = i2c_transfer(client->adapter, &msg, num_msg);
+
+ /*One retry */
+ if (ret != num_msg)
+ ret = i2c_transfer(client->adapter, &msg, num_msg);
+
+ if (ret != num_msg) {
+ dev_err(&client->dev, "I2C write fail fail\n");
+ return -EIO;
+ } else {
+ return 0;
+ }
+}
+
+static int dw9714_t_focus_vcm(struct dw9714_device *dw9714_dev, u16 val)
+{
+ struct i2c_client *client = dw9714_dev->client;
+ int ret = -EINVAL;
+
+ dev_dbg(&client->dev, "Setting new value VCM: %d\n", val);
+ dw9714_dev->current_val = val;
+
+ ret = dw9714_i2c_write(client, DW9714_VAL(val, DW9714_DEFAULT_S));
+ return ret;
+}
+
+static int dw9714_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct dw9714_device *dev_vcm = to_dw9714_vcm(ctrl);
+
+ if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE)
+ return dw9714_t_focus_vcm(dev_vcm, ctrl->val);
+ else
+ return -EINVAL;
+}
+
+static const struct v4l2_ctrl_ops dw9714_vcm_ctrl_ops = {
+ .s_ctrl = dw9714_set_ctrl,
+};
+
+static int dw9714_init_controls(struct dw9714_device *dev_vcm)
+{
+ struct v4l2_ctrl_handler *hdl = &dev_vcm->ctrls_vcm;
+ const struct v4l2_ctrl_ops *ops = &dw9714_vcm_ctrl_ops;
+ struct i2c_client *client = dev_vcm->client;
+
+ v4l2_ctrl_handler_init(hdl, 1);
+
+ v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE,
+ 0, DW9714_MAX_FOCUS_POS, 1, 0);
+
+ if (hdl->error)
+ dev_err(&client->dev, "dw9714_init_controls fail\n");
+ dev_vcm->sd.ctrl_handler = hdl;
+ return hdl->error;
+}
+
+static void dw9714_subdev_cleanup(struct dw9714_device *dw9714_dev)
+{
+ v4l2_ctrl_handler_free(&dw9714_dev->ctrls_vcm);
+ v4l2_async_unregister_subdev(&dw9714_dev->sd);
+ v4l2_device_unregister_subdev(&dw9714_dev->sd);
+ media_entity_cleanup(&dw9714_dev->sd.entity);
+}
+
+static int dw9714_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ struct dw9714_device *dw9714_dev = container_of(sd,
+ struct dw9714_device,
+ sd);
+ struct device *dev = &dw9714_dev->client->dev;
+ int rval;
+
+ rval = pm_runtime_get_sync(dev);
+ dev_dbg(dev, "%s rval = %d\n", __func__, rval);
+
+ return rval;
+}
+
+static int dw9714_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ struct dw9714_device *dw9714_dev = container_of(sd,
+ struct dw9714_device,
+ sd);
+ struct device *dev = &dw9714_dev->client->dev;
+
+ dev_dbg(dev, "%s\n", __func__);
+ pm_runtime_put(dev);
+
+ return 0;
+}
+
+static const struct v4l2_subdev_internal_ops dw9714_int_ops = {
+ .open = dw9714_open,
+ .close = dw9714_close,
+};
+
+static const struct v4l2_subdev_ops dw9714_ops = { };
+
+static int dw9714_probe(struct i2c_client *client,
+ const struct i2c_device_id *devid)
+{
+ struct dw9714_device *dw9714_dev;
+ int rval;
+
+ dw9714_dev = devm_kzalloc(&client->dev, sizeof(*dw9714_dev),
+ GFP_KERNEL);
+
+ if (dw9714_dev == NULL)
+ return -ENOMEM;
+
+ dw9714_dev->client = client;
+
+ v4l2_i2c_subdev_init(&dw9714_dev->sd, client, &dw9714_ops);
+ dw9714_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+ dw9714_dev->sd.internal_ops = &dw9714_int_ops;
+
+ snprintf(dw9714_dev->sd.name,
+ sizeof(dw9714_dev->sd.name),
+ DW9714_NAME " %d-%4.4x", i2c_adapter_id(client->adapter),
+ client->addr);
+
+ rval = dw9714_init_controls(dw9714_dev);
+ if (rval)
+ goto err_cleanup;
+
+ rval = media_entity_pads_init(&dw9714_dev->sd.entity, 0, NULL);
+ if (rval < 0)
+ goto err_cleanup;
+
+ dw9714_dev->sd.entity.function = MEDIA_ENT_F_LENS;
+
+ rval = v4l2_async_register_subdev(&dw9714_dev->sd);
+ if (rval < 0)
+ goto err_cleanup;
+
+ pm_runtime_enable(&client->dev);
+
+ return 0;
+
+err_cleanup:
+ dw9714_subdev_cleanup(dw9714_dev);
+ dev_err(&client->dev, "Probe failed: %d\n", rval);
+ return rval;
+}
+
+static int dw9714_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct dw9714_device *dw9714_dev = container_of(sd,
+ struct dw9714_device,
+ sd);
+
+ pm_runtime_disable(&client->dev);
+ dw9714_subdev_cleanup(dw9714_dev);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+
+static int dw9714_runtime_suspend(struct device *dev)
+{
+ return 0;
+}
+
+static int dw9714_runtime_resume(struct device *dev)
+{
+ return 0;
+}
+
+/* This function sets the vcm position, so it consumes least current */
+static int dw9714_suspend(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct dw9714_device *dw9714_dev = container_of(sd,
+ struct dw9714_device,
+ sd);
+ int ret, val;
+
+ dev_dbg(dev, "%s\n", __func__);
+
+ for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS - 1);
+ val >= 0; val -= DW9714_CTRL_STEPS) {
+ ret = dw9714_i2c_write(client,
+ DW9714_VAL((u16) val, DW9714_DEFAULT_S));
+ if (ret)
+ dev_err(dev, "%s I2C failure: %d", __func__, ret);
+ usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
+ }
+ return 0;
+}
+
+/*
+ * This function sets the vcm position, so the focus position is set
+ * closer to the camera
+ */
+static int dw9714_resume(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct dw9714_device *dw9714_dev = container_of(sd,
+ struct dw9714_device,
+ sd);
+ int ret, val;
+
+ dev_dbg(dev, "%s\n", __func__);
+
+ for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
+ val < dw9714_dev->current_val + DW9714_CTRL_STEPS - 1;
+ val += DW9714_CTRL_STEPS) {
+ ret = dw9714_i2c_write(client,
+ DW9714_VAL((u16) val, DW9714_DEFAULT_S));
+ if (ret)
+ dev_err(dev, "%s I2C failure: %d", __func__, ret);
+ usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
+ }
+
+ /* restore v4l2 control values */
+ ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm);
+ dev_dbg(dev, "%s ret = %d\n", __func__, ret);
+ return ret;
+}
+
+#else
+
+#define dw9714_suspend NULL
+#define dw9714_resume NULL
+#define dw9714_runtime_suspend NULL
+#define dw9714_runtime_resume NULL
+
+#endif /* CONFIG_PM */
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id dw9714_acpi_match[] = {
+ {"DW9714", 0},
+ {},
+};
+MODULE_DEVICE_TABLE(acpi, dw9714_acpi_match);
+#endif
+
+static const struct i2c_device_id dw9714_id_table[] = {
+ {DW9714_NAME, 0},
+ {}
+};
+
+MODULE_DEVICE_TABLE(i2c, dw9714_id_table);
+
+static const struct dev_pm_ops dw9714_pm_ops = {
+ .suspend = dw9714_suspend,
+ .resume = dw9714_resume,
+ .runtime_suspend = dw9714_runtime_suspend,
+ .runtime_resume = dw9714_runtime_resume,
+};
+
+static struct i2c_driver dw9714_i2c_driver = {
+ .driver = {
+ .name = DW9714_NAME,
+ .pm = &dw9714_pm_ops,
+#ifdef CONFIG_ACPI
+ .acpi_match_table = ACPI_PTR(dw9714_acpi_match),
+#endif
+ },
+ .probe = dw9714_probe,
+ .remove = dw9714_remove,
+ .id_table = dw9714_id_table,
+};
+
+module_i2c_driver(dw9714_i2c_driver);
+
+MODULE_AUTHOR("Tianshu Qiu <tian.shu.qiu@intel.com>");
+MODULE_AUTHOR("Jian Xu Zheng <jian.xu.zheng@intel.com>");
+MODULE_AUTHOR("Yuning Pu <yuning.pu@intel.com>");
+MODULE_AUTHOR("Jouni Ukkonen <jouni.ukkonen@intel.com>");
+MODULE_AUTHOR("Tommi Franttila <tommi.franttila@intel.com>");
+MODULE_DESCRIPTION("DW9714 VCM driver");
+MODULE_LICENSE("GPL");
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] dw9714: Initial driver for dw9714 VCM
2017-05-07 11:33 [PATCH] dw9714: Initial driver for dw9714 VCM rajmohan.mani
@ 2017-05-08 10:59 ` Hans Verkuil
2017-05-08 14:14 ` Mani, Rajmohan
2017-05-08 20:55 ` Sakari Ailus
1 sibling, 1 reply; 13+ messages in thread
From: Hans Verkuil @ 2017-05-08 10:59 UTC (permalink / raw)
To: rajmohan.mani, linux-media, mchehab
Hi Rajmohan,
Thanks for the patch!
A quick code review:
On 05/07/2017 01:33 PM, rajmohan.mani@intel.com wrote:
> From: Rajmohan Mani <rajmohan.mani@intel.com>
>
> DW9714 is a 10 bit DAC, designed for linear
> control of voice coil motor.
>
> This driver creates a V4L2 subdevice and
> provides control to set the desired focus.
>
> Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
> ---
> drivers/media/i2c/Kconfig | 9 ++
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/dw9714.c | 333 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 343 insertions(+)
> create mode 100644 drivers/media/i2c/dw9714.c
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index fd181c9..4f0a7ad 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -300,6 +300,15 @@ config VIDEO_AD5820
> This is a driver for the AD5820 camera lens voice coil.
> It is used for example in Nokia N900 (RX-51).
>
> +config VIDEO_DW9714
> + tristate "DW9714 lens voice coil support"
> + depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
> + ---help---
> + This is a driver for the DW9714 camera lens voice coil.
> + DW9714 is a 10 bit DAC with 120mA output current sink
> + capability. This is designed for linear control of
> + voice coil motors, controlled via I2C serial interface.
> +
> config VIDEO_SAA7110
> tristate "Philips SAA7110 video decoder"
> depends on VIDEO_V4L2 && I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 62323ec..987bd1f 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
> obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
> obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
> obj-$(CONFIG_VIDEO_AD5820) += ad5820.o
> +obj-$(CONFIG_VIDEO_DW9714) += dw9714.o
> obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
> obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
> obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o
> diff --git a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c
> new file mode 100644
> index 0000000..276d3f2
> --- /dev/null
> +++ b/drivers/media/i2c/dw9714.c
> @@ -0,0 +1,333 @@
> +/*
> + * Copyright (c) 2015--2017 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version
> + * 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +
> +#define DW9714_NAME "dw9714"
> +#define DW9714_MAX_FOCUS_POS 1023
> +#define DW9714_CTRL_STEPS 16 /* Keep this value power of 2 */
> +#define DW9714_CTRL_DELAY_US 1000
> +/*
> + * S[3:2] = 0x00, codes per step for "Linear Slope Control"
> + * S[1:0] = 0x00, step period
> + */
> +#define DW9714_DEFAULT_S 0x0
> +#define DW9714_VAL(data, s) (u16)((data) << 4 | (s))
> +
> +/* dw9714 device structure */
> +struct dw9714_device {
> + struct i2c_client *client;
> + struct v4l2_ctrl_handler ctrls_vcm;
> + struct v4l2_subdev sd;
> + u16 current_val;
> +};
> +
> +#define to_dw9714_vcm(_ctrl) \
> + container_of(_ctrl->handler, struct dw9714_device, ctrls_vcm)
> +
> +static int dw9714_i2c_write(struct i2c_client *client, u16 data)
> +{
> + const int num_msg = 1;
> + int ret;
> + u16 val = cpu_to_be16(data);
> + struct i2c_msg msg = {
> + .addr = client->addr,
> + .flags = 0,
> + .len = sizeof(val),
> + .buf = (u8 *) &val,
> + };
> +
> + ret = i2c_transfer(client->adapter, &msg, num_msg);
> +
> + /*One retry */
> + if (ret != num_msg)
> + ret = i2c_transfer(client->adapter, &msg, num_msg);
> +
> + if (ret != num_msg) {
> + dev_err(&client->dev, "I2C write fail fail\n");
> + return -EIO;
> + } else {
No need for the 'else' here, since the 'if' already returns.
> + return 0;
> + }
> +}
> +
> +static int dw9714_t_focus_vcm(struct dw9714_device *dw9714_dev, u16 val)
> +{
> + struct i2c_client *client = dw9714_dev->client;
> + int ret = -EINVAL;
> +
> + dev_dbg(&client->dev, "Setting new value VCM: %d\n", val);
> + dw9714_dev->current_val = val;
> +
> + ret = dw9714_i2c_write(client, DW9714_VAL(val, DW9714_DEFAULT_S));
> + return ret;
Just do return dw9714_i2c_write(client, DW9714_VAL(val, DW9714_DEFAULT_S));
Thus dropping the 'ret' variable.
> +}
> +
> +static int dw9714_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct dw9714_device *dev_vcm = to_dw9714_vcm(ctrl);
> +
> + if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE)
> + return dw9714_t_focus_vcm(dev_vcm, ctrl->val);
Personally I would merge the dw9714_t_focus_vcm in here. And probably use
a switch instead of an if. Up to you, though.
> + else
No need for 'else'
> + return -EINVAL;
> +}
> +
> +static const struct v4l2_ctrl_ops dw9714_vcm_ctrl_ops = {
> + .s_ctrl = dw9714_set_ctrl,
> +};
> +
> +static int dw9714_init_controls(struct dw9714_device *dev_vcm)
> +{
> + struct v4l2_ctrl_handler *hdl = &dev_vcm->ctrls_vcm;
> + const struct v4l2_ctrl_ops *ops = &dw9714_vcm_ctrl_ops;
> + struct i2c_client *client = dev_vcm->client;
> +
> + v4l2_ctrl_handler_init(hdl, 1);
> +
> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE,
> + 0, DW9714_MAX_FOCUS_POS, 1, 0);
> +
> + if (hdl->error)
> + dev_err(&client->dev, "dw9714_init_controls fail\n");
> + dev_vcm->sd.ctrl_handler = hdl;
> + return hdl->error;
> +}
> +
> +static void dw9714_subdev_cleanup(struct dw9714_device *dw9714_dev)
> +{
> + v4l2_ctrl_handler_free(&dw9714_dev->ctrls_vcm);
> + v4l2_async_unregister_subdev(&dw9714_dev->sd);
> + v4l2_device_unregister_subdev(&dw9714_dev->sd);
> + media_entity_cleanup(&dw9714_dev->sd.entity);
> +}
> +
> +static int dw9714_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> + struct dw9714_device *dw9714_dev = container_of(sd,
> + struct dw9714_device,
> + sd);
> + struct device *dev = &dw9714_dev->client->dev;
> + int rval;
> +
> + rval = pm_runtime_get_sync(dev);
> + dev_dbg(dev, "%s rval = %d\n", __func__, rval);
The error is propagated to userspace anyway as errno, so do you really need a dev_dbg
here?
> +
> + return rval;
> +}
> +
> +static int dw9714_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> + struct dw9714_device *dw9714_dev = container_of(sd,
> + struct dw9714_device,
> + sd);
> + struct device *dev = &dw9714_dev->client->dev;
> +
> + dev_dbg(dev, "%s\n", __func__);
Seems useless here as well.
> + pm_runtime_put(dev);
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_internal_ops dw9714_int_ops = {
> + .open = dw9714_open,
> + .close = dw9714_close,
> +};
> +
> +static const struct v4l2_subdev_ops dw9714_ops = { };
> +
> +static int dw9714_probe(struct i2c_client *client,
> + const struct i2c_device_id *devid)
> +{
> + struct dw9714_device *dw9714_dev;
> + int rval;
> +
> + dw9714_dev = devm_kzalloc(&client->dev, sizeof(*dw9714_dev),
> + GFP_KERNEL);
> +
> + if (dw9714_dev == NULL)
> + return -ENOMEM;
> +
> + dw9714_dev->client = client;
> +
> + v4l2_i2c_subdev_init(&dw9714_dev->sd, client, &dw9714_ops);
> + dw9714_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + dw9714_dev->sd.internal_ops = &dw9714_int_ops;
> +
> + snprintf(dw9714_dev->sd.name,
> + sizeof(dw9714_dev->sd.name),
> + DW9714_NAME " %d-%4.4x", i2c_adapter_id(client->adapter),
> + client->addr);
v4l2_i2c_subdev_init() already fills in the sd.name, so this can be dropped.
> +
> + rval = dw9714_init_controls(dw9714_dev);
> + if (rval)
> + goto err_cleanup;
> +
> + rval = media_entity_pads_init(&dw9714_dev->sd.entity, 0, NULL);
> + if (rval < 0)
> + goto err_cleanup;
> +
> + dw9714_dev->sd.entity.function = MEDIA_ENT_F_LENS;
> +
> + rval = v4l2_async_register_subdev(&dw9714_dev->sd);
> + if (rval < 0)
> + goto err_cleanup;
> +
> + pm_runtime_enable(&client->dev);
> +
> + return 0;
> +
> +err_cleanup:
> + dw9714_subdev_cleanup(dw9714_dev);
> + dev_err(&client->dev, "Probe failed: %d\n", rval);
> + return rval;
> +}
> +
> +static int dw9714_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct dw9714_device *dw9714_dev = container_of(sd,
> + struct dw9714_device,
> + sd);
> +
> + pm_runtime_disable(&client->dev);
> + dw9714_subdev_cleanup(dw9714_dev);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +
> +static int dw9714_runtime_suspend(struct device *dev)
> +{
> + return 0;
> +}
> +
> +static int dw9714_runtime_resume(struct device *dev)
> +{
> + return 0;
> +}
> +
> +/* This function sets the vcm position, so it consumes least current */
> +static int dw9714_suspend(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct dw9714_device *dw9714_dev = container_of(sd,
> + struct dw9714_device,
> + sd);
> + int ret, val;
> +
> + dev_dbg(dev, "%s\n", __func__);
> +
> + for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS - 1);
> + val >= 0; val -= DW9714_CTRL_STEPS) {
> + ret = dw9714_i2c_write(client,
> + DW9714_VAL((u16) val, DW9714_DEFAULT_S));
> + if (ret)
> + dev_err(dev, "%s I2C failure: %d", __func__, ret);
> + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
> + }
> + return 0;
> +}
> +
> +/*
> + * This function sets the vcm position, so the focus position is set
> + * closer to the camera
> + */
> +static int dw9714_resume(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct dw9714_device *dw9714_dev = container_of(sd,
> + struct dw9714_device,
> + sd);
> + int ret, val;
> +
> + dev_dbg(dev, "%s\n", __func__);
> +
> + for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
> + val < dw9714_dev->current_val + DW9714_CTRL_STEPS - 1;
> + val += DW9714_CTRL_STEPS) {
> + ret = dw9714_i2c_write(client,
> + DW9714_VAL((u16) val, DW9714_DEFAULT_S));
> + if (ret)
> + dev_err(dev, "%s I2C failure: %d", __func__, ret);
> + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
> + }
> +
> + /* restore v4l2 control values */
> + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm);
> + dev_dbg(dev, "%s ret = %d\n", __func__, ret);
> + return ret;
> +}
> +
> +#else
> +
> +#define dw9714_suspend NULL
> +#define dw9714_resume NULL
> +#define dw9714_runtime_suspend NULL
> +#define dw9714_runtime_resume NULL
> +
> +#endif /* CONFIG_PM */
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id dw9714_acpi_match[] = {
> + {"DW9714", 0},
> + {},
> +};
> +MODULE_DEVICE_TABLE(acpi, dw9714_acpi_match);
> +#endif
> +
> +static const struct i2c_device_id dw9714_id_table[] = {
> + {DW9714_NAME, 0},
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, dw9714_id_table);
> +
> +static const struct dev_pm_ops dw9714_pm_ops = {
> + .suspend = dw9714_suspend,
> + .resume = dw9714_resume,
> + .runtime_suspend = dw9714_runtime_suspend,
> + .runtime_resume = dw9714_runtime_resume,
> +};
> +
> +static struct i2c_driver dw9714_i2c_driver = {
> + .driver = {
> + .name = DW9714_NAME,
> + .pm = &dw9714_pm_ops,
> +#ifdef CONFIG_ACPI
> + .acpi_match_table = ACPI_PTR(dw9714_acpi_match),
> +#endif
> + },
> + .probe = dw9714_probe,
> + .remove = dw9714_remove,
> + .id_table = dw9714_id_table,
> +};
> +
> +module_i2c_driver(dw9714_i2c_driver);
> +
> +MODULE_AUTHOR("Tianshu Qiu <tian.shu.qiu@intel.com>");
> +MODULE_AUTHOR("Jian Xu Zheng <jian.xu.zheng@intel.com>");
> +MODULE_AUTHOR("Yuning Pu <yuning.pu@intel.com>");
> +MODULE_AUTHOR("Jouni Ukkonen <jouni.ukkonen@intel.com>");
> +MODULE_AUTHOR("Tommi Franttila <tommi.franttila@intel.com>");
> +MODULE_DESCRIPTION("DW9714 VCM driver");
> +MODULE_LICENSE("GPL");
>
Regards,
Hans
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] dw9714: Initial driver for dw9714 VCM
2017-05-08 10:59 ` Hans Verkuil
@ 2017-05-08 14:14 ` Mani, Rajmohan
0 siblings, 0 replies; 13+ messages in thread
From: Mani, Rajmohan @ 2017-05-08 14:14 UTC (permalink / raw)
To: Hans Verkuil, linux-media, mchehab
Hi Hans,
Thanks for the quick review. Please see my comments inline...
I will post the new v2 patch soon.
> -----Original Message-----
> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
> Sent: Monday, May 08, 2017 6:59 PM
> To: Mani, Rajmohan <rajmohan.mani@intel.com>; linux-
> media@vger.kernel.org; mchehab@kernel.org
> Subject: Re: [PATCH] dw9714: Initial driver for dw9714 VCM
>
> Hi Rajmohan,
>
> Thanks for the patch!
>
> A quick code review:
>
> On 05/07/2017 01:33 PM, rajmohan.mani@intel.com wrote:
> > From: Rajmohan Mani <rajmohan.mani@intel.com>
> >
> > DW9714 is a 10 bit DAC, designed for linear control of voice coil
> > motor.
> >
> > This driver creates a V4L2 subdevice and provides control to set the
> > desired focus.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
> > ---
> > drivers/media/i2c/Kconfig | 9 ++
> > drivers/media/i2c/Makefile | 1 +
> > drivers/media/i2c/dw9714.c | 333
> > +++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 343 insertions(+)
> > create mode 100644 drivers/media/i2c/dw9714.c
> >
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index fd181c9..4f0a7ad 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -300,6 +300,15 @@ config VIDEO_AD5820
> > This is a driver for the AD5820 camera lens voice coil.
> > It is used for example in Nokia N900 (RX-51).
> >
> > +config VIDEO_DW9714
> > + tristate "DW9714 lens voice coil support"
> > + depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
> > + ---help---
> > + This is a driver for the DW9714 camera lens voice coil.
> > + DW9714 is a 10 bit DAC with 120mA output current sink
> > + capability. This is designed for linear control of
> > + voice coil motors, controlled via I2C serial interface.
> > +
> > config VIDEO_SAA7110
> > tristate "Philips SAA7110 video decoder"
> > depends on VIDEO_V4L2 && I2C
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index 62323ec..987bd1f 100644
> > --- a/drivers/media/i2c/Makefile
> > +++ b/drivers/media/i2c/Makefile
> > @@ -21,6 +21,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
> > obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
> > obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
> > obj-$(CONFIG_VIDEO_AD5820) += ad5820.o
> > +obj-$(CONFIG_VIDEO_DW9714) += dw9714.o
> > obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
> > obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
> > obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o diff --git
> > a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c new file
> > mode 100644 index 0000000..276d3f2
> > --- /dev/null
> > +++ b/drivers/media/i2c/dw9714.c
> > @@ -0,0 +1,333 @@
> > +/*
> > + * Copyright (c) 2015--2017 Intel Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > +version
> > + * 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/pm_runtime.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-device.h>
> > +
> > +#define DW9714_NAME "dw9714"
> > +#define DW9714_MAX_FOCUS_POS 1023
> > +#define DW9714_CTRL_STEPS 16 /* Keep this value power of 2
> */
> > +#define DW9714_CTRL_DELAY_US 1000
> > +/*
> > + * S[3:2] = 0x00, codes per step for "Linear Slope Control"
> > + * S[1:0] = 0x00, step period
> > + */
> > +#define DW9714_DEFAULT_S 0x0
> > +#define DW9714_VAL(data, s) (u16)((data) << 4 | (s))
> > +
> > +/* dw9714 device structure */
> > +struct dw9714_device {
> > + struct i2c_client *client;
> > + struct v4l2_ctrl_handler ctrls_vcm;
> > + struct v4l2_subdev sd;
> > + u16 current_val;
> > +};
> > +
> > +#define to_dw9714_vcm(_ctrl) \
> > + container_of(_ctrl->handler, struct dw9714_device, ctrls_vcm)
> > +
> > +static int dw9714_i2c_write(struct i2c_client *client, u16 data) {
> > + const int num_msg = 1;
> > + int ret;
> > + u16 val = cpu_to_be16(data);
> > + struct i2c_msg msg = {
> > + .addr = client->addr,
> > + .flags = 0,
> > + .len = sizeof(val),
> > + .buf = (u8 *) &val,
> > + };
> > +
> > + ret = i2c_transfer(client->adapter, &msg, num_msg);
> > +
> > + /*One retry */
> > + if (ret != num_msg)
> > + ret = i2c_transfer(client->adapter, &msg, num_msg);
> > +
> > + if (ret != num_msg) {
> > + dev_err(&client->dev, "I2C write fail fail\n");
> > + return -EIO;
> > + } else {
>
> No need for the 'else' here, since the 'if' already returns.
Ack
I also removed the redundant " fail" in the dev_err() message.
>
> > + return 0;
> > + }
> > +}
> > +
> > +static int dw9714_t_focus_vcm(struct dw9714_device *dw9714_dev, u16
> > +val) {
> > + struct i2c_client *client = dw9714_dev->client;
> > + int ret = -EINVAL;
> > +
> > + dev_dbg(&client->dev, "Setting new value VCM: %d\n", val);
> > + dw9714_dev->current_val = val;
> > +
> > + ret = dw9714_i2c_write(client, DW9714_VAL(val,
> DW9714_DEFAULT_S));
> > + return ret;
>
> Just do return dw9714_i2c_write(client, DW9714_VAL(val,
> DW9714_DEFAULT_S));
>
> Thus dropping the 'ret' variable.
Ack
>
> > +}
> > +
> > +static int dw9714_set_ctrl(struct v4l2_ctrl *ctrl) {
> > + struct dw9714_device *dev_vcm = to_dw9714_vcm(ctrl);
> > +
> > + if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE)
> > + return dw9714_t_focus_vcm(dev_vcm, ctrl->val);
>
> Personally I would merge the dw9714_t_focus_vcm in here. And probably use a
> switch instead of an if. Up to you, though.
With just one ctrl->id, dw9714_set_ctrl() looks to be more readable with the if and dw9714_t_focus_vcm() call.
When more ids are added, merging of dw9714_t_focus_vcm() and switch statement becomes more apt. So, for now, I am leaving it as such.
>
> > + else
>
> No need for 'else'
Ack
>
> > + return -EINVAL;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops dw9714_vcm_ctrl_ops = {
> > + .s_ctrl = dw9714_set_ctrl,
> > +};
> > +
> > +static int dw9714_init_controls(struct dw9714_device *dev_vcm) {
> > + struct v4l2_ctrl_handler *hdl = &dev_vcm->ctrls_vcm;
> > + const struct v4l2_ctrl_ops *ops = &dw9714_vcm_ctrl_ops;
> > + struct i2c_client *client = dev_vcm->client;
> > +
> > + v4l2_ctrl_handler_init(hdl, 1);
> > +
> > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE,
> > + 0, DW9714_MAX_FOCUS_POS, 1, 0);
> > +
> > + if (hdl->error)
> > + dev_err(&client->dev, "dw9714_init_controls fail\n");
> > + dev_vcm->sd.ctrl_handler = hdl;
> > + return hdl->error;
> > +}
> > +
> > +static void dw9714_subdev_cleanup(struct dw9714_device *dw9714_dev) {
> > + v4l2_ctrl_handler_free(&dw9714_dev->ctrls_vcm);
> > + v4l2_async_unregister_subdev(&dw9714_dev->sd);
> > + v4l2_device_unregister_subdev(&dw9714_dev->sd);
> > + media_entity_cleanup(&dw9714_dev->sd.entity);
> > +}
> > +
> > +static int dw9714_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> > +*fh) {
> > + struct dw9714_device *dw9714_dev = container_of(sd,
> > + struct
> dw9714_device,
> > + sd);
> > + struct device *dev = &dw9714_dev->client->dev;
> > + int rval;
> > +
> > + rval = pm_runtime_get_sync(dev);
> > + dev_dbg(dev, "%s rval = %d\n", __func__, rval);
>
> The error is propagated to userspace anyway as errno, so do you really need a
> dev_dbg here?
Ack
I removed the dev_dbg message. I also got rid of rval, by just doing ret pm_runtime_get_sync(dev)
>
> > +
> > + return rval;
> > +}
> > +
> > +static int dw9714_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> > +*fh) {
> > + struct dw9714_device *dw9714_dev = container_of(sd,
> > + struct
> dw9714_device,
> > + sd);
> > + struct device *dev = &dw9714_dev->client->dev;
> > +
> > + dev_dbg(dev, "%s\n", __func__);
>
> Seems useless here as well.
Ack
>
> > + pm_runtime_put(dev);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_internal_ops dw9714_int_ops = {
> > + .open = dw9714_open,
> > + .close = dw9714_close,
> > +};
> > +
> > +static const struct v4l2_subdev_ops dw9714_ops = { };
> > +
> > +static int dw9714_probe(struct i2c_client *client,
> > + const struct i2c_device_id *devid) {
> > + struct dw9714_device *dw9714_dev;
> > + int rval;
> > +
> > + dw9714_dev = devm_kzalloc(&client->dev, sizeof(*dw9714_dev),
> > + GFP_KERNEL);
> > +
> > + if (dw9714_dev == NULL)
> > + return -ENOMEM;
> > +
> > + dw9714_dev->client = client;
> > +
> > + v4l2_i2c_subdev_init(&dw9714_dev->sd, client, &dw9714_ops);
> > + dw9714_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > + dw9714_dev->sd.internal_ops = &dw9714_int_ops;
> > +
> > + snprintf(dw9714_dev->sd.name,
> > + sizeof(dw9714_dev->sd.name),
> > + DW9714_NAME " %d-%4.4x", i2c_adapter_id(client-
> >adapter),
> > + client->addr);
>
> v4l2_i2c_subdev_init() already fills in the sd.name, so this can be dropped.
Ack
>
> > +
> > + rval = dw9714_init_controls(dw9714_dev);
> > + if (rval)
> > + goto err_cleanup;
> > +
> > + rval = media_entity_pads_init(&dw9714_dev->sd.entity, 0, NULL);
> > + if (rval < 0)
> > + goto err_cleanup;
> > +
> > + dw9714_dev->sd.entity.function = MEDIA_ENT_F_LENS;
> > +
> > + rval = v4l2_async_register_subdev(&dw9714_dev->sd);
> > + if (rval < 0)
> > + goto err_cleanup;
> > +
> > + pm_runtime_enable(&client->dev);
> > +
> > + return 0;
> > +
> > +err_cleanup:
> > + dw9714_subdev_cleanup(dw9714_dev);
> > + dev_err(&client->dev, "Probe failed: %d\n", rval);
> > + return rval;
> > +}
> > +
> > +static int dw9714_remove(struct i2c_client *client) {
> > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > + struct dw9714_device *dw9714_dev = container_of(sd,
> > + struct
> dw9714_device,
> > + sd);
> > +
> > + pm_runtime_disable(&client->dev);
> > + dw9714_subdev_cleanup(dw9714_dev);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +
> > +static int dw9714_runtime_suspend(struct device *dev) {
> > + return 0;
> > +}
> > +
> > +static int dw9714_runtime_resume(struct device *dev) {
> > + return 0;
> > +}
> > +
> > +/* This function sets the vcm position, so it consumes least current
> > +*/ static int dw9714_suspend(struct device *dev) {
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > + struct dw9714_device *dw9714_dev = container_of(sd,
> > + struct
> dw9714_device,
> > + sd);
> > + int ret, val;
> > +
> > + dev_dbg(dev, "%s\n", __func__);
> > +
> > + for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS - 1);
> > + val >= 0; val -= DW9714_CTRL_STEPS) {
> > + ret = dw9714_i2c_write(client,
> > + DW9714_VAL((u16) val,
> DW9714_DEFAULT_S));
> > + if (ret)
> > + dev_err(dev, "%s I2C failure: %d", __func__, ret);
> > + usleep_range(DW9714_CTRL_DELAY_US,
> DW9714_CTRL_DELAY_US + 10);
> > + }
> > + return 0;
> > +}
> > +
> > +/*
> > + * This function sets the vcm position, so the focus position is set
> > + * closer to the camera
> > + */
> > +static int dw9714_resume(struct device *dev) {
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > + struct dw9714_device *dw9714_dev = container_of(sd,
> > + struct
> dw9714_device,
> > + sd);
> > + int ret, val;
> > +
> > + dev_dbg(dev, "%s\n", __func__);
> > +
> > + for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
> > + val < dw9714_dev->current_val + DW9714_CTRL_STEPS - 1;
> > + val += DW9714_CTRL_STEPS) {
> > + ret = dw9714_i2c_write(client,
> > + DW9714_VAL((u16) val,
> DW9714_DEFAULT_S));
> > + if (ret)
> > + dev_err(dev, "%s I2C failure: %d", __func__, ret);
> > + usleep_range(DW9714_CTRL_DELAY_US,
> DW9714_CTRL_DELAY_US + 10);
> > + }
> > +
> > + /* restore v4l2 control values */
> > + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm);
> > + dev_dbg(dev, "%s ret = %d\n", __func__, ret);
> > + return ret;
> > +}
> > +
> > +#else
> > +
> > +#define dw9714_suspend NULL
> > +#define dw9714_resume NULL
> > +#define dw9714_runtime_suspend NULL
> > +#define dw9714_runtime_resume NULL
> > +
> > +#endif /* CONFIG_PM */
> > +
> > +#ifdef CONFIG_ACPI
> > +static const struct acpi_device_id dw9714_acpi_match[] = {
> > + {"DW9714", 0},
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(acpi, dw9714_acpi_match); #endif
> > +
> > +static const struct i2c_device_id dw9714_id_table[] = {
> > + {DW9714_NAME, 0},
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, dw9714_id_table);
> > +
> > +static const struct dev_pm_ops dw9714_pm_ops = {
> > + .suspend = dw9714_suspend,
> > + .resume = dw9714_resume,
> > + .runtime_suspend = dw9714_runtime_suspend,
> > + .runtime_resume = dw9714_runtime_resume, };
> > +
> > +static struct i2c_driver dw9714_i2c_driver = {
> > + .driver = {
> > + .name = DW9714_NAME,
> > + .pm = &dw9714_pm_ops,
> > +#ifdef CONFIG_ACPI
> > + .acpi_match_table = ACPI_PTR(dw9714_acpi_match), #endif
> > + },
> > + .probe = dw9714_probe,
> > + .remove = dw9714_remove,
> > + .id_table = dw9714_id_table,
> > +};
> > +
> > +module_i2c_driver(dw9714_i2c_driver);
> > +
> > +MODULE_AUTHOR("Tianshu Qiu <tian.shu.qiu@intel.com>");
> > +MODULE_AUTHOR("Jian Xu Zheng <jian.xu.zheng@intel.com>");
> > +MODULE_AUTHOR("Yuning Pu <yuning.pu@intel.com>");
> > +MODULE_AUTHOR("Jouni Ukkonen <jouni.ukkonen@intel.com>");
> > +MODULE_AUTHOR("Tommi Franttila <tommi.franttila@intel.com>");
> > +MODULE_DESCRIPTION("DW9714 VCM driver"); MODULE_LICENSE("GPL");
> >
>
> Regards,
>
> Hans
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dw9714: Initial driver for dw9714 VCM
2017-05-07 11:33 [PATCH] dw9714: Initial driver for dw9714 VCM rajmohan.mani
2017-05-08 10:59 ` Hans Verkuil
@ 2017-05-08 20:55 ` Sakari Ailus
2017-05-09 8:30 ` Tomasz Figa
2017-05-09 13:27 ` Mani, Rajmohan
1 sibling, 2 replies; 13+ messages in thread
From: Sakari Ailus @ 2017-05-08 20:55 UTC (permalink / raw)
To: rajmohan.mani; +Cc: linux-media, mchehab
Hi Rajmohan,
A few comments below...
On Sun, May 07, 2017 at 04:33:24AM -0700, rajmohan.mani@intel.com wrote:
> From: Rajmohan Mani <rajmohan.mani@intel.com>
>
> DW9714 is a 10 bit DAC, designed for linear
> control of voice coil motor.
>
> This driver creates a V4L2 subdevice and
> provides control to set the desired focus.
>
> Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
> ---
> drivers/media/i2c/Kconfig | 9 ++
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/dw9714.c | 333 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 343 insertions(+)
> create mode 100644 drivers/media/i2c/dw9714.c
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index fd181c9..4f0a7ad 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -300,6 +300,15 @@ config VIDEO_AD5820
> This is a driver for the AD5820 camera lens voice coil.
> It is used for example in Nokia N900 (RX-51).
>
> +config VIDEO_DW9714
> + tristate "DW9714 lens voice coil support"
> + depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
You should also depend on VIDEO_V4L2_SUBDEV_API .
> + ---help---
> + This is a driver for the DW9714 camera lens voice coil.
> + DW9714 is a 10 bit DAC with 120mA output current sink
> + capability. This is designed for linear control of
> + voice coil motors, controlled via I2C serial interface.
> +
> config VIDEO_SAA7110
> tristate "Philips SAA7110 video decoder"
> depends on VIDEO_V4L2 && I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 62323ec..987bd1f 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
> obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
> obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
> obj-$(CONFIG_VIDEO_AD5820) += ad5820.o
> +obj-$(CONFIG_VIDEO_DW9714) += dw9714.o
> obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
> obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
> obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o
> diff --git a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c
> new file mode 100644
> index 0000000..276d3f2
> --- /dev/null
> +++ b/drivers/media/i2c/dw9714.c
> @@ -0,0 +1,333 @@
> +/*
> + * Copyright (c) 2015--2017 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version
> + * 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +
> +#define DW9714_NAME "dw9714"
> +#define DW9714_MAX_FOCUS_POS 1023
> +#define DW9714_CTRL_STEPS 16 /* Keep this value power of 2 */
> +#define DW9714_CTRL_DELAY_US 1000
> +/*
> + * S[3:2] = 0x00, codes per step for "Linear Slope Control"
> + * S[1:0] = 0x00, step period
> + */
> +#define DW9714_DEFAULT_S 0x0
> +#define DW9714_VAL(data, s) (u16)((data) << 4 | (s))
> +
> +/* dw9714 device structure */
> +struct dw9714_device {
> + struct i2c_client *client;
> + struct v4l2_ctrl_handler ctrls_vcm;
> + struct v4l2_subdev sd;
> + u16 current_val;
> +};
> +
> +#define to_dw9714_vcm(_ctrl) \
> + container_of(_ctrl->handler, struct dw9714_device, ctrls_vcm)
> +
> +static int dw9714_i2c_write(struct i2c_client *client, u16 data)
> +{
> + const int num_msg = 1;
> + int ret;
> + u16 val = cpu_to_be16(data);
> + struct i2c_msg msg = {
> + .addr = client->addr,
> + .flags = 0,
> + .len = sizeof(val),
> + .buf = (u8 *) &val,
> + };
> +
> + ret = i2c_transfer(client->adapter, &msg, num_msg);
> +
> + /*One retry */
> + if (ret != num_msg)
> + ret = i2c_transfer(client->adapter, &msg, num_msg);
> +
> + if (ret != num_msg) {
> + dev_err(&client->dev, "I2C write fail fail\n");
> + return -EIO;
> + } else {
> + return 0;
> + }
> +}
> +
> +static int dw9714_t_focus_vcm(struct dw9714_device *dw9714_dev, u16 val)
> +{
> + struct i2c_client *client = dw9714_dev->client;
> + int ret = -EINVAL;
> +
> + dev_dbg(&client->dev, "Setting new value VCM: %d\n", val);
> + dw9714_dev->current_val = val;
> +
> + ret = dw9714_i2c_write(client, DW9714_VAL(val, DW9714_DEFAULT_S));
> + return ret;
> +}
> +
> +static int dw9714_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct dw9714_device *dev_vcm = to_dw9714_vcm(ctrl);
> +
> + if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE)
> + return dw9714_t_focus_vcm(dev_vcm, ctrl->val);
> + else
> + return -EINVAL;
> +}
> +
> +static const struct v4l2_ctrl_ops dw9714_vcm_ctrl_ops = {
> + .s_ctrl = dw9714_set_ctrl,
> +};
> +
> +static int dw9714_init_controls(struct dw9714_device *dev_vcm)
> +{
> + struct v4l2_ctrl_handler *hdl = &dev_vcm->ctrls_vcm;
> + const struct v4l2_ctrl_ops *ops = &dw9714_vcm_ctrl_ops;
> + struct i2c_client *client = dev_vcm->client;
> +
> + v4l2_ctrl_handler_init(hdl, 1);
> +
> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE,
> + 0, DW9714_MAX_FOCUS_POS, 1, 0);
> +
> + if (hdl->error)
> + dev_err(&client->dev, "dw9714_init_controls fail\n");
> + dev_vcm->sd.ctrl_handler = hdl;
> + return hdl->error;
> +}
> +
> +static void dw9714_subdev_cleanup(struct dw9714_device *dw9714_dev)
> +{
> + v4l2_ctrl_handler_free(&dw9714_dev->ctrls_vcm);
Please release the control handler after unregistering the sub-device.
> + v4l2_async_unregister_subdev(&dw9714_dev->sd);
> + v4l2_device_unregister_subdev(&dw9714_dev->sd);
> + media_entity_cleanup(&dw9714_dev->sd.entity);
> +}
> +
> +static int dw9714_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> + struct dw9714_device *dw9714_dev = container_of(sd,
> + struct dw9714_device,
> + sd);
> + struct device *dev = &dw9714_dev->client->dev;
> + int rval;
> +
> + rval = pm_runtime_get_sync(dev);
If this fails you'll need to do pm_runtime_put(dev);
> + dev_dbg(dev, "%s rval = %d\n", __func__, rval);
> +
> + return rval;
> +}
> +
> +static int dw9714_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> + struct dw9714_device *dw9714_dev = container_of(sd,
> + struct dw9714_device,
> + sd);
> + struct device *dev = &dw9714_dev->client->dev;
> +
> + dev_dbg(dev, "%s\n", __func__);
> + pm_runtime_put(dev);
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_internal_ops dw9714_int_ops = {
> + .open = dw9714_open,
> + .close = dw9714_close,
> +};
> +
> +static const struct v4l2_subdev_ops dw9714_ops = { };
You should use v4l2_subdev_internal_ops here.
> +
> +static int dw9714_probe(struct i2c_client *client,
> + const struct i2c_device_id *devid)
> +{
> + struct dw9714_device *dw9714_dev;
> + int rval;
> +
> + dw9714_dev = devm_kzalloc(&client->dev, sizeof(*dw9714_dev),
> + GFP_KERNEL);
> +
> + if (dw9714_dev == NULL)
> + return -ENOMEM;
> +
> + dw9714_dev->client = client;
> +
> + v4l2_i2c_subdev_init(&dw9714_dev->sd, client, &dw9714_ops);
> + dw9714_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + dw9714_dev->sd.internal_ops = &dw9714_int_ops;
> +
> + snprintf(dw9714_dev->sd.name,
> + sizeof(dw9714_dev->sd.name),
> + DW9714_NAME " %d-%4.4x", i2c_adapter_id(client->adapter),
> + client->addr);
> +
> + rval = dw9714_init_controls(dw9714_dev);
> + if (rval)
> + goto err_cleanup;
> +
> + rval = media_entity_pads_init(&dw9714_dev->sd.entity, 0, NULL);
> + if (rval < 0)
> + goto err_cleanup;
> +
> + dw9714_dev->sd.entity.function = MEDIA_ENT_F_LENS;
> +
> + rval = v4l2_async_register_subdev(&dw9714_dev->sd);
> + if (rval < 0)
> + goto err_cleanup;
> +
> + pm_runtime_enable(&client->dev);
Getting PM runtime right doesn't seem to be easy. :-I
pm_runtime_enable() alone doesn't do the trick. I wonder if adding
pm_runtime_suspend() would do the trick.
> +
> + return 0;
> +
> +err_cleanup:
> + dw9714_subdev_cleanup(dw9714_dev);
> + dev_err(&client->dev, "Probe failed: %d\n", rval);
> + return rval;
> +}
> +
> +static int dw9714_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct dw9714_device *dw9714_dev = container_of(sd,
> + struct dw9714_device,
> + sd);
> +
> + pm_runtime_disable(&client->dev);
> + dw9714_subdev_cleanup(dw9714_dev);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +
> +static int dw9714_runtime_suspend(struct device *dev)
> +{
> + return 0;
> +}
> +
> +static int dw9714_runtime_resume(struct device *dev)
> +{
> + return 0;
I think it'd be fine to remove empty callbacks.
> +}
> +
> +/* This function sets the vcm position, so it consumes least current */
> +static int dw9714_suspend(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct dw9714_device *dw9714_dev = container_of(sd,
> + struct dw9714_device,
> + sd);
> + int ret, val;
> +
> + dev_dbg(dev, "%s\n", __func__);
> +
> + for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS - 1);
> + val >= 0; val -= DW9714_CTRL_STEPS) {
> + ret = dw9714_i2c_write(client,
> + DW9714_VAL((u16) val, DW9714_DEFAULT_S));
> + if (ret)
> + dev_err(dev, "%s I2C failure: %d", __func__, ret);
> + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
> + }
> + return 0;
> +}
> +
> +/*
> + * This function sets the vcm position, so the focus position is set
> + * closer to the camera
> + */
> +static int dw9714_resume(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct dw9714_device *dw9714_dev = container_of(sd,
> + struct dw9714_device,
> + sd);
> + int ret, val;
> +
> + dev_dbg(dev, "%s\n", __func__);
> +
> + for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
> + val < dw9714_dev->current_val + DW9714_CTRL_STEPS - 1;
> + val += DW9714_CTRL_STEPS) {
> + ret = dw9714_i2c_write(client,
> + DW9714_VAL((u16) val, DW9714_DEFAULT_S));
> + if (ret)
> + dev_err(dev, "%s I2C failure: %d", __func__, ret);
> + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
> + }
> +
> + /* restore v4l2 control values */
> + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm);
Doesn't this need to be done for runtime_resume as well?
> + dev_dbg(dev, "%s ret = %d\n", __func__, ret);
> + return ret;
> +}
> +
> +#else
> +
> +#define dw9714_suspend NULL
> +#define dw9714_resume NULL
> +#define dw9714_runtime_suspend NULL
> +#define dw9714_runtime_resume NULL
> +
> +#endif /* CONFIG_PM */
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id dw9714_acpi_match[] = {
> + {"DW9714", 0},
> + {},
> +};
> +MODULE_DEVICE_TABLE(acpi, dw9714_acpi_match);
> +#endif
> +
> +static const struct i2c_device_id dw9714_id_table[] = {
> + {DW9714_NAME, 0},
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, dw9714_id_table);
> +
> +static const struct dev_pm_ops dw9714_pm_ops = {
> + .suspend = dw9714_suspend,
> + .resume = dw9714_resume,
> + .runtime_suspend = dw9714_runtime_suspend,
> + .runtime_resume = dw9714_runtime_resume,
> +};
> +
> +static struct i2c_driver dw9714_i2c_driver = {
> + .driver = {
> + .name = DW9714_NAME,
> + .pm = &dw9714_pm_ops,
> +#ifdef CONFIG_ACPI
> + .acpi_match_table = ACPI_PTR(dw9714_acpi_match),
> +#endif
> + },
> + .probe = dw9714_probe,
> + .remove = dw9714_remove,
> + .id_table = dw9714_id_table,
> +};
> +
> +module_i2c_driver(dw9714_i2c_driver);
> +
> +MODULE_AUTHOR("Tianshu Qiu <tian.shu.qiu@intel.com>");
> +MODULE_AUTHOR("Jian Xu Zheng <jian.xu.zheng@intel.com>");
> +MODULE_AUTHOR("Yuning Pu <yuning.pu@intel.com>");
> +MODULE_AUTHOR("Jouni Ukkonen <jouni.ukkonen@intel.com>");
> +MODULE_AUTHOR("Tommi Franttila <tommi.franttila@intel.com>");
> +MODULE_DESCRIPTION("DW9714 VCM driver");
> +MODULE_LICENSE("GPL");
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dw9714: Initial driver for dw9714 VCM
2017-05-08 20:55 ` Sakari Ailus
@ 2017-05-09 8:30 ` Tomasz Figa
2017-05-09 10:40 ` Sakari Ailus
2017-05-09 13:27 ` Mani, Rajmohan
1 sibling, 1 reply; 13+ messages in thread
From: Tomasz Figa @ 2017-05-09 8:30 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Mani, Rajmohan, linux-media, mchehab
Hi Sakari,
On Tue, May 9, 2017 at 4:55 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> Hi Rajmohan,
>
> A few comments below...
>
> On Sun, May 07, 2017 at 04:33:24AM -0700, rajmohan.mani@intel.com wrote:
[snip]
>> + rval = v4l2_async_register_subdev(&dw9714_dev->sd);
>> + if (rval < 0)
>> + goto err_cleanup;
>> +
>> + pm_runtime_enable(&client->dev);
>
> Getting PM runtime right doesn't seem to be easy. :-I
>
> pm_runtime_enable() alone doesn't do the trick. I wonder if adding
> pm_runtime_suspend() would do the trick.
Is this something specific for I2C devices? For platform devices,
typically pm_runtime_enable() is the only thing you would need to do.
>
>> +
>> + return 0;
>> +
>> +err_cleanup:
>> + dw9714_subdev_cleanup(dw9714_dev);
>> + dev_err(&client->dev, "Probe failed: %d\n", rval);
>> + return rval;
>> +}
>> +
>> +static int dw9714_remove(struct i2c_client *client)
>> +{
>> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> + struct dw9714_device *dw9714_dev = container_of(sd,
>> + struct dw9714_device,
>> + sd);
>> +
>> + pm_runtime_disable(&client->dev);
>> + dw9714_subdev_cleanup(dw9714_dev);
>> +
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +
>> +static int dw9714_runtime_suspend(struct device *dev)
>> +{
>> + return 0;
>> +}
>> +
>> +static int dw9714_runtime_resume(struct device *dev)
>> +{
>> + return 0;
>
> I think it'd be fine to remove empty callbacks.
It's actually a bit more complicated (if a PM domain is attached, the
callbacks must be present), however in case of external I2C devices it
should be fine indeed. However, AFAIK, pm_runtime_no_callbacks()
should be called.
>
>> +}
>> +
>> +/* This function sets the vcm position, so it consumes least current */
>> +static int dw9714_suspend(struct device *dev)
>> +{
>> + struct i2c_client *client = to_i2c_client(dev);
>> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> + struct dw9714_device *dw9714_dev = container_of(sd,
>> + struct dw9714_device,
>> + sd);
>> + int ret, val;
>> +
>> + dev_dbg(dev, "%s\n", __func__);
>> +
>> + for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS - 1);
>> + val >= 0; val -= DW9714_CTRL_STEPS) {
>> + ret = dw9714_i2c_write(client,
>> + DW9714_VAL((u16) val, DW9714_DEFAULT_S));
>> + if (ret)
>> + dev_err(dev, "%s I2C failure: %d", __func__, ret);
>> + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
>> + }
>> + return 0;
>> +}
>> +
>> +/*
>> + * This function sets the vcm position, so the focus position is set
>> + * closer to the camera
>> + */
>> +static int dw9714_resume(struct device *dev)
>> +{
>> + struct i2c_client *client = to_i2c_client(dev);
>> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> + struct dw9714_device *dw9714_dev = container_of(sd,
>> + struct dw9714_device,
>> + sd);
>> + int ret, val;
>> +
>> + dev_dbg(dev, "%s\n", __func__);
>> +
>> + for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
>> + val < dw9714_dev->current_val + DW9714_CTRL_STEPS - 1;
>> + val += DW9714_CTRL_STEPS) {
>> + ret = dw9714_i2c_write(client,
>> + DW9714_VAL((u16) val, DW9714_DEFAULT_S));
>> + if (ret)
>> + dev_err(dev, "%s I2C failure: %d", __func__, ret);
>> + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
>> + }
>> +
>> + /* restore v4l2 control values */
>> + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm);
>
> Doesn't this need to be done for runtime_resume as well?
This driver doesn't seem to be doing any physical power off in its
runtime_suspend and I don't expect an I2C device to be put in a PM
domain, so possibly no need for it.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dw9714: Initial driver for dw9714 VCM
2017-05-09 8:30 ` Tomasz Figa
@ 2017-05-09 10:40 ` Sakari Ailus
2017-05-09 11:38 ` Tomasz Figa
0 siblings, 1 reply; 13+ messages in thread
From: Sakari Ailus @ 2017-05-09 10:40 UTC (permalink / raw)
To: Tomasz Figa; +Cc: Mani, Rajmohan, linux-media, mchehab
Hi Tomasz,
On Tue, May 09, 2017 at 04:30:40PM +0800, Tomasz Figa wrote:
> Hi Sakari,
>
> On Tue, May 9, 2017 at 4:55 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > Hi Rajmohan,
> >
> > A few comments below...
> >
> > On Sun, May 07, 2017 at 04:33:24AM -0700, rajmohan.mani@intel.com wrote:
> [snip]
> >> + rval = v4l2_async_register_subdev(&dw9714_dev->sd);
> >> + if (rval < 0)
> >> + goto err_cleanup;
> >> +
> >> + pm_runtime_enable(&client->dev);
> >
> > Getting PM runtime right doesn't seem to be easy. :-I
> >
> > pm_runtime_enable() alone doesn't do the trick. I wonder if adding
> > pm_runtime_suspend() would do the trick.
>
> Is this something specific for I2C devices? For platform devices,
> typically pm_runtime_enable() is the only thing you would need to do.
I think you're right --- driver_probe_device() will call pm_request_idle()
to the device right after probe. So indeed calling pm_runtime_enable() is
enough.
> >> +
> >> + return 0;
> >> +
> >> +err_cleanup:
> >> + dw9714_subdev_cleanup(dw9714_dev);
> >> + dev_err(&client->dev, "Probe failed: %d\n", rval);
> >> + return rval;
> >> +}
> >> +
> >> +static int dw9714_remove(struct i2c_client *client)
> >> +{
> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >> + struct dw9714_device *dw9714_dev = container_of(sd,
> >> + struct dw9714_device,
> >> + sd);
> >> +
> >> + pm_runtime_disable(&client->dev);
> >> + dw9714_subdev_cleanup(dw9714_dev);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +#ifdef CONFIG_PM
> >> +
> >> +static int dw9714_runtime_suspend(struct device *dev)
> >> +{
> >> + return 0;
> >> +}
> >> +
> >> +static int dw9714_runtime_resume(struct device *dev)
> >> +{
> >> + return 0;
> >
> > I think it'd be fine to remove empty callbacks.
>
> It's actually a bit more complicated (if a PM domain is attached, the
> callbacks must be present), however in case of external I2C devices it
> should be fine indeed. However, AFAIK, pm_runtime_no_callbacks()
> should be called.
I wonder if I'm missing something --- acpi_subsys_runtime_resume() first
calls acpi_dev_runtime_resume() and if all goes well, the proceeds to call
pm_generic_runtime_resume() which calls device's runtime_resume() if it's
non-NULL.
In other words, having a runtime_resume() and runtime_suspend() callbacks
that return zero is equivalent of having neither of the callbacks.
>
> >
> >> +}
> >> +
> >> +/* This function sets the vcm position, so it consumes least current */
> >> +static int dw9714_suspend(struct device *dev)
> >> +{
> >> + struct i2c_client *client = to_i2c_client(dev);
> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >> + struct dw9714_device *dw9714_dev = container_of(sd,
> >> + struct dw9714_device,
> >> + sd);
> >> + int ret, val;
> >> +
> >> + dev_dbg(dev, "%s\n", __func__);
> >> +
> >> + for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS - 1);
> >> + val >= 0; val -= DW9714_CTRL_STEPS) {
> >> + ret = dw9714_i2c_write(client,
> >> + DW9714_VAL((u16) val, DW9714_DEFAULT_S));
> >> + if (ret)
> >> + dev_err(dev, "%s I2C failure: %d", __func__, ret);
> >> + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
> >> + }
> >> + return 0;
> >> +}
> >> +
> >> +/*
> >> + * This function sets the vcm position, so the focus position is set
> >> + * closer to the camera
> >> + */
> >> +static int dw9714_resume(struct device *dev)
> >> +{
> >> + struct i2c_client *client = to_i2c_client(dev);
> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >> + struct dw9714_device *dw9714_dev = container_of(sd,
> >> + struct dw9714_device,
> >> + sd);
> >> + int ret, val;
> >> +
> >> + dev_dbg(dev, "%s\n", __func__);
> >> +
> >> + for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
> >> + val < dw9714_dev->current_val + DW9714_CTRL_STEPS - 1;
> >> + val += DW9714_CTRL_STEPS) {
> >> + ret = dw9714_i2c_write(client,
> >> + DW9714_VAL((u16) val, DW9714_DEFAULT_S));
> >> + if (ret)
> >> + dev_err(dev, "%s I2C failure: %d", __func__, ret);
> >> + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
> >> + }
> >> +
> >> + /* restore v4l2 control values */
> >> + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm);
> >
> > Doesn't this need to be done for runtime_resume as well?
>
> This driver doesn't seem to be doing any physical power off in its
> runtime_suspend and I don't expect an I2C device to be put in a PM
> domain, so possibly no need for it.
I'd expect runtime PM suspend callback to power the device off through ACPI
PM. For this reason the device state must be restored when it's powered on,
i.e. its runtime_resume callback.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dw9714: Initial driver for dw9714 VCM
2017-05-09 10:40 ` Sakari Ailus
@ 2017-05-09 11:38 ` Tomasz Figa
2017-05-09 12:16 ` Sakari Ailus
0 siblings, 1 reply; 13+ messages in thread
From: Tomasz Figa @ 2017-05-09 11:38 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Mani, Rajmohan, linux-media, mchehab
On Tue, May 9, 2017 at 6:40 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> Hi Tomasz,
>
> On Tue, May 09, 2017 at 04:30:40PM +0800, Tomasz Figa wrote:
>> Hi Sakari,
>>
>> On Tue, May 9, 2017 at 4:55 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>> > Hi Rajmohan,
>> >
>> > A few comments below...
>> >
>> > On Sun, May 07, 2017 at 04:33:24AM -0700, rajmohan.mani@intel.com wrote:
>> [snip]
>> >> + rval = v4l2_async_register_subdev(&dw9714_dev->sd);
>> >> + if (rval < 0)
>> >> + goto err_cleanup;
>> >> +
>> >> + pm_runtime_enable(&client->dev);
>> >
>> > Getting PM runtime right doesn't seem to be easy. :-I
>> >
>> > pm_runtime_enable() alone doesn't do the trick. I wonder if adding
>> > pm_runtime_suspend() would do the trick.
>>
>> Is this something specific for I2C devices? For platform devices,
>> typically pm_runtime_enable() is the only thing you would need to do.
>
> I think you're right --- driver_probe_device() will call pm_request_idle()
> to the device right after probe. So indeed calling pm_runtime_enable() is
> enough.
>
>> >> +
>> >> + return 0;
>> >> +
>> >> +err_cleanup:
>> >> + dw9714_subdev_cleanup(dw9714_dev);
>> >> + dev_err(&client->dev, "Probe failed: %d\n", rval);
>> >> + return rval;
>> >> +}
>> >> +
>> >> +static int dw9714_remove(struct i2c_client *client)
>> >> +{
>> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> >> + struct dw9714_device *dw9714_dev = container_of(sd,
>> >> + struct dw9714_device,
>> >> + sd);
>> >> +
>> >> + pm_runtime_disable(&client->dev);
>> >> + dw9714_subdev_cleanup(dw9714_dev);
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +#ifdef CONFIG_PM
>> >> +
>> >> +static int dw9714_runtime_suspend(struct device *dev)
>> >> +{
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static int dw9714_runtime_resume(struct device *dev)
>> >> +{
>> >> + return 0;
>> >
>> > I think it'd be fine to remove empty callbacks.
>>
>> It's actually a bit more complicated (if a PM domain is attached, the
>> callbacks must be present), however in case of external I2C devices it
>> should be fine indeed. However, AFAIK, pm_runtime_no_callbacks()
>> should be called.
>
> I wonder if I'm missing something --- acpi_subsys_runtime_resume() first
> calls acpi_dev_runtime_resume() and if all goes well, the proceeds to call
> pm_generic_runtime_resume() which calls device's runtime_resume() if it's
> non-NULL.
>
> In other words, having a runtime_resume() and runtime_suspend() callbacks
> that return zero is equivalent of having neither of the callbacks.
Ah, I missed the fact this device is instantiated by ACPI and it has
different handling of runtime PM, which apparently means it doesn't
use the code paths affected by the PM domain thing I mentioned.
>
>>
>> >
>> >> +}
>> >> +
>> >> +/* This function sets the vcm position, so it consumes least current */
>> >> +static int dw9714_suspend(struct device *dev)
>> >> +{
>> >> + struct i2c_client *client = to_i2c_client(dev);
>> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> >> + struct dw9714_device *dw9714_dev = container_of(sd,
>> >> + struct dw9714_device,
>> >> + sd);
>> >> + int ret, val;
>> >> +
>> >> + dev_dbg(dev, "%s\n", __func__);
>> >> +
>> >> + for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS - 1);
>> >> + val >= 0; val -= DW9714_CTRL_STEPS) {
>> >> + ret = dw9714_i2c_write(client,
>> >> + DW9714_VAL((u16) val, DW9714_DEFAULT_S));
>> >> + if (ret)
>> >> + dev_err(dev, "%s I2C failure: %d", __func__, ret);
>> >> + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
>> >> + }
>> >> + return 0;
>> >> +}
>> >> +
>> >> +/*
>> >> + * This function sets the vcm position, so the focus position is set
>> >> + * closer to the camera
>> >> + */
>> >> +static int dw9714_resume(struct device *dev)
>> >> +{
>> >> + struct i2c_client *client = to_i2c_client(dev);
>> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> >> + struct dw9714_device *dw9714_dev = container_of(sd,
>> >> + struct dw9714_device,
>> >> + sd);
>> >> + int ret, val;
>> >> +
>> >> + dev_dbg(dev, "%s\n", __func__);
>> >> +
>> >> + for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
>> >> + val < dw9714_dev->current_val + DW9714_CTRL_STEPS - 1;
>> >> + val += DW9714_CTRL_STEPS) {
>> >> + ret = dw9714_i2c_write(client,
>> >> + DW9714_VAL((u16) val, DW9714_DEFAULT_S));
>> >> + if (ret)
>> >> + dev_err(dev, "%s I2C failure: %d", __func__, ret);
>> >> + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
>> >> + }
>> >> +
>> >> + /* restore v4l2 control values */
>> >> + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm);
>> >
>> > Doesn't this need to be done for runtime_resume as well?
>>
>> This driver doesn't seem to be doing any physical power off in its
>> runtime_suspend and I don't expect an I2C device to be put in a PM
>> domain, so possibly no need for it.
>
> I'd expect runtime PM suspend callback to power the device off through ACPI
> PM. For this reason the device state must be restored when it's powered on,
> i.e. its runtime_resume callback.
Ah, again ACPI here, that's something not usual for me. Sorry for the noise.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dw9714: Initial driver for dw9714 VCM
2017-05-09 11:38 ` Tomasz Figa
@ 2017-05-09 12:16 ` Sakari Ailus
2017-05-09 13:22 ` Tomasz Figa
0 siblings, 1 reply; 13+ messages in thread
From: Sakari Ailus @ 2017-05-09 12:16 UTC (permalink / raw)
To: Tomasz Figa; +Cc: Mani, Rajmohan, linux-media, mchehab
Hi Tomasz,
On Tue, May 09, 2017 at 07:38:26PM +0800, Tomasz Figa wrote:
> On Tue, May 9, 2017 at 6:40 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > Hi Tomasz,
> >
> > On Tue, May 09, 2017 at 04:30:40PM +0800, Tomasz Figa wrote:
> >> Hi Sakari,
> >>
> >> On Tue, May 9, 2017 at 4:55 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >> > Hi Rajmohan,
> >> >
> >> > A few comments below...
> >> >
> >> > On Sun, May 07, 2017 at 04:33:24AM -0700, rajmohan.mani@intel.com wrote:
> >> [snip]
> >> >> + rval = v4l2_async_register_subdev(&dw9714_dev->sd);
> >> >> + if (rval < 0)
> >> >> + goto err_cleanup;
> >> >> +
> >> >> + pm_runtime_enable(&client->dev);
> >> >
> >> > Getting PM runtime right doesn't seem to be easy. :-I
> >> >
> >> > pm_runtime_enable() alone doesn't do the trick. I wonder if adding
> >> > pm_runtime_suspend() would do the trick.
> >>
> >> Is this something specific for I2C devices? For platform devices,
> >> typically pm_runtime_enable() is the only thing you would need to do.
> >
> > I think you're right --- driver_probe_device() will call pm_request_idle()
> > to the device right after probe. So indeed calling pm_runtime_enable() is
> > enough.
> >
> >> >> +
> >> >> + return 0;
> >> >> +
> >> >> +err_cleanup:
> >> >> + dw9714_subdev_cleanup(dw9714_dev);
> >> >> + dev_err(&client->dev, "Probe failed: %d\n", rval);
> >> >> + return rval;
> >> >> +}
> >> >> +
> >> >> +static int dw9714_remove(struct i2c_client *client)
> >> >> +{
> >> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >> >> + struct dw9714_device *dw9714_dev = container_of(sd,
> >> >> + struct dw9714_device,
> >> >> + sd);
> >> >> +
> >> >> + pm_runtime_disable(&client->dev);
> >> >> + dw9714_subdev_cleanup(dw9714_dev);
> >> >> +
> >> >> + return 0;
> >> >> +}
> >> >> +
> >> >> +#ifdef CONFIG_PM
> >> >> +
> >> >> +static int dw9714_runtime_suspend(struct device *dev)
> >> >> +{
> >> >> + return 0;
> >> >> +}
> >> >> +
> >> >> +static int dw9714_runtime_resume(struct device *dev)
> >> >> +{
> >> >> + return 0;
> >> >
> >> > I think it'd be fine to remove empty callbacks.
> >>
> >> It's actually a bit more complicated (if a PM domain is attached, the
> >> callbacks must be present), however in case of external I2C devices it
> >> should be fine indeed. However, AFAIK, pm_runtime_no_callbacks()
> >> should be called.
> >
> > I wonder if I'm missing something --- acpi_subsys_runtime_resume() first
> > calls acpi_dev_runtime_resume() and if all goes well, the proceeds to call
> > pm_generic_runtime_resume() which calls device's runtime_resume() if it's
> > non-NULL.
> >
> > In other words, having a runtime_resume() and runtime_suspend() callbacks
> > that return zero is equivalent of having neither of the callbacks.
>
> Ah, I missed the fact this device is instantiated by ACPI and it has
> different handling of runtime PM, which apparently means it doesn't
> use the code paths affected by the PM domain thing I mentioned.
I have to admit I'm no expert in the topic but I'd presume that other
implementations should still maintain consistent behaviour towards drivers.
acpi_subsys_runtime_resume() is the PM domain runtime_resume() callback in
acpi_general_pm_domain.
>
> >
> >>
> >> >
> >> >> +}
> >> >> +
> >> >> +/* This function sets the vcm position, so it consumes least current */
> >> >> +static int dw9714_suspend(struct device *dev)
> >> >> +{
> >> >> + struct i2c_client *client = to_i2c_client(dev);
> >> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >> >> + struct dw9714_device *dw9714_dev = container_of(sd,
> >> >> + struct dw9714_device,
> >> >> + sd);
> >> >> + int ret, val;
> >> >> +
> >> >> + dev_dbg(dev, "%s\n", __func__);
> >> >> +
> >> >> + for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS - 1);
> >> >> + val >= 0; val -= DW9714_CTRL_STEPS) {
> >> >> + ret = dw9714_i2c_write(client,
> >> >> + DW9714_VAL((u16) val, DW9714_DEFAULT_S));
> >> >> + if (ret)
> >> >> + dev_err(dev, "%s I2C failure: %d", __func__, ret);
> >> >> + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
> >> >> + }
> >> >> + return 0;
> >> >> +}
> >> >> +
> >> >> +/*
> >> >> + * This function sets the vcm position, so the focus position is set
> >> >> + * closer to the camera
> >> >> + */
> >> >> +static int dw9714_resume(struct device *dev)
> >> >> +{
> >> >> + struct i2c_client *client = to_i2c_client(dev);
> >> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >> >> + struct dw9714_device *dw9714_dev = container_of(sd,
> >> >> + struct dw9714_device,
> >> >> + sd);
> >> >> + int ret, val;
> >> >> +
> >> >> + dev_dbg(dev, "%s\n", __func__);
> >> >> +
> >> >> + for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
> >> >> + val < dw9714_dev->current_val + DW9714_CTRL_STEPS - 1;
> >> >> + val += DW9714_CTRL_STEPS) {
> >> >> + ret = dw9714_i2c_write(client,
> >> >> + DW9714_VAL((u16) val, DW9714_DEFAULT_S));
> >> >> + if (ret)
> >> >> + dev_err(dev, "%s I2C failure: %d", __func__, ret);
> >> >> + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
> >> >> + }
> >> >> +
> >> >> + /* restore v4l2 control values */
> >> >> + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm);
> >> >
> >> > Doesn't this need to be done for runtime_resume as well?
> >>
> >> This driver doesn't seem to be doing any physical power off in its
> >> runtime_suspend and I don't expect an I2C device to be put in a PM
> >> domain, so possibly no need for it.
> >
> > I'd expect runtime PM suspend callback to power the device off through ACPI
> > PM. For this reason the device state must be restored when it's powered on,
> > i.e. its runtime_resume callback.
>
> Ah, again ACPI here, that's something not usual for me. Sorry for the noise.
No harm done. Very good comments in general IMO, thanks for those!
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dw9714: Initial driver for dw9714 VCM
2017-05-09 12:16 ` Sakari Ailus
@ 2017-05-09 13:22 ` Tomasz Figa
2017-05-09 13:26 ` Tomasz Figa
0 siblings, 1 reply; 13+ messages in thread
From: Tomasz Figa @ 2017-05-09 13:22 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mani, Rajmohan, linux-media, mchehab, Rafael J. Wysocki,
Kevin Hilman, Ulf Hansson
+Rafael, Kevin and Ulf,
On Tue, May 9, 2017 at 8:16 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> Hi Tomasz,
>
> On Tue, May 09, 2017 at 07:38:26PM +0800, Tomasz Figa wrote:
>> On Tue, May 9, 2017 at 6:40 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>> > Hi Tomasz,
>> >
>> > On Tue, May 09, 2017 at 04:30:40PM +0800, Tomasz Figa wrote:
>> >> Hi Sakari,
>> >>
>> >> On Tue, May 9, 2017 at 4:55 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>> >> > Hi Rajmohan,
>> >> >
>> >> > A few comments below...
>> >> >
>> >> > On Sun, May 07, 2017 at 04:33:24AM -0700, rajmohan.mani@intel.com wrote:
>> >> [snip]
>> >> >> + rval = v4l2_async_register_subdev(&dw9714_dev->sd);
>> >> >> + if (rval < 0)
>> >> >> + goto err_cleanup;
>> >> >> +
>> >> >> + pm_runtime_enable(&client->dev);
>> >> >
>> >> > Getting PM runtime right doesn't seem to be easy. :-I
>> >> >
>> >> > pm_runtime_enable() alone doesn't do the trick. I wonder if adding
>> >> > pm_runtime_suspend() would do the trick.
>> >>
>> >> Is this something specific for I2C devices? For platform devices,
>> >> typically pm_runtime_enable() is the only thing you would need to do.
>> >
>> > I think you're right --- driver_probe_device() will call pm_request_idle()
>> > to the device right after probe. So indeed calling pm_runtime_enable() is
>> > enough.
>> >
>> >> >> +
>> >> >> + return 0;
>> >> >> +
>> >> >> +err_cleanup:
>> >> >> + dw9714_subdev_cleanup(dw9714_dev);
>> >> >> + dev_err(&client->dev, "Probe failed: %d\n", rval);
>> >> >> + return rval;
>> >> >> +}
>> >> >> +
>> >> >> +static int dw9714_remove(struct i2c_client *client)
>> >> >> +{
>> >> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> >> >> + struct dw9714_device *dw9714_dev = container_of(sd,
>> >> >> + struct dw9714_device,
>> >> >> + sd);
>> >> >> +
>> >> >> + pm_runtime_disable(&client->dev);
>> >> >> + dw9714_subdev_cleanup(dw9714_dev);
>> >> >> +
>> >> >> + return 0;
>> >> >> +}
>> >> >> +
>> >> >> +#ifdef CONFIG_PM
>> >> >> +
>> >> >> +static int dw9714_runtime_suspend(struct device *dev)
>> >> >> +{
>> >> >> + return 0;
>> >> >> +}
>> >> >> +
>> >> >> +static int dw9714_runtime_resume(struct device *dev)
>> >> >> +{
>> >> >> + return 0;
>> >> >
>> >> > I think it'd be fine to remove empty callbacks.
>> >>
>> >> It's actually a bit more complicated (if a PM domain is attached, the
>> >> callbacks must be present), however in case of external I2C devices it
>> >> should be fine indeed. However, AFAIK, pm_runtime_no_callbacks()
>> >> should be called.
>> >
>> > I wonder if I'm missing something --- acpi_subsys_runtime_resume() first
>> > calls acpi_dev_runtime_resume() and if all goes well, the proceeds to call
>> > pm_generic_runtime_resume() which calls device's runtime_resume() if it's
>> > non-NULL.
>> >
>> > In other words, having a runtime_resume() and runtime_suspend() callbacks
>> > that return zero is equivalent of having neither of the callbacks.
>>
>> Ah, I missed the fact this device is instantiated by ACPI and it has
>> different handling of runtime PM, which apparently means it doesn't
>> use the code paths affected by the PM domain thing I mentioned.
>
> I have to admit I'm no expert in the topic but I'd presume that other
> implementations should still maintain consistent behaviour towards drivers.
> acpi_subsys_runtime_resume() is the PM domain runtime_resume() callback in
> acpi_general_pm_domain.
Let's see. For platform bus this seems to be reasonable indeed -
__rpm_get_callback() will use the callbacks of PM domain, device type,
device class or device bus, whichever is available first, in this
order. Looking at platform_bus_type, the equivalent resume callback is
pm_generic_runtime_resume() and it will indeed silently return 0 if
there is no dev->pm->runtime_resume.
However for I2C bus, i2c_bus_type doesn't seem to have .pm defined.
Type and class are device/driver-specific things, so let's assume they
don't have .pm set. If the driver doesn't have .pm, then the only way
__rpm_get_callback() can return a non-NULL value is when
dev->pm_domain is non NULL (which seems to be the ACPI case actually).
Otherwise, if __rpm_get_callback() returns NULL, pm_runtime_get*()
will fail with -ENOSYS. This is because rpm_resume() calls
rpm_callback(callback, dev), where callback is the value returned by
__rpm_get_callback() and rpm_callback() returns -ENOSYS if callback is
NULL. This is where pm_runtime_no_callbacks() comes handy, as it
bypasses the code getting and calling the callback, so it doesn't
return the error anymore.
There is however the other side of the coin. If
pm_runtime_no_callbacks() is called, all kind of callbacks are
bypassed, so even the PM domain code is not invoked. This is kind of
tricky, because the driver must be now aware of whether it's running
under a PM domain or not and call this pm_runtime_no_callbacks()
depending on the outcome, to guarantee correct operation (PM domain
callbacks must be called, even if device driver doesn't have its
own)... Which IMHO doesn't make sense.
I guess the correct way to proceed here would be adding .pm ops to
i2c_bus_type, just as it's done with platform_bus_type.
>
>>
>> >
>> >>
>> >> >
>> >> >> +}
>> >> >> +
>> >> >> +/* This function sets the vcm position, so it consumes least current */
>> >> >> +static int dw9714_suspend(struct device *dev)
>> >> >> +{
>> >> >> + struct i2c_client *client = to_i2c_client(dev);
>> >> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> >> >> + struct dw9714_device *dw9714_dev = container_of(sd,
>> >> >> + struct dw9714_device,
>> >> >> + sd);
>> >> >> + int ret, val;
>> >> >> +
>> >> >> + dev_dbg(dev, "%s\n", __func__);
>> >> >> +
>> >> >> + for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS - 1);
>> >> >> + val >= 0; val -= DW9714_CTRL_STEPS) {
>> >> >> + ret = dw9714_i2c_write(client,
>> >> >> + DW9714_VAL((u16) val, DW9714_DEFAULT_S));
>> >> >> + if (ret)
>> >> >> + dev_err(dev, "%s I2C failure: %d", __func__, ret);
>> >> >> + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
>> >> >> + }
>> >> >> + return 0;
>> >> >> +}
>> >> >> +
>> >> >> +/*
>> >> >> + * This function sets the vcm position, so the focus position is set
>> >> >> + * closer to the camera
>> >> >> + */
>> >> >> +static int dw9714_resume(struct device *dev)
>> >> >> +{
>> >> >> + struct i2c_client *client = to_i2c_client(dev);
>> >> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> >> >> + struct dw9714_device *dw9714_dev = container_of(sd,
>> >> >> + struct dw9714_device,
>> >> >> + sd);
>> >> >> + int ret, val;
>> >> >> +
>> >> >> + dev_dbg(dev, "%s\n", __func__);
>> >> >> +
>> >> >> + for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
>> >> >> + val < dw9714_dev->current_val + DW9714_CTRL_STEPS - 1;
>> >> >> + val += DW9714_CTRL_STEPS) {
>> >> >> + ret = dw9714_i2c_write(client,
>> >> >> + DW9714_VAL((u16) val, DW9714_DEFAULT_S));
>> >> >> + if (ret)
>> >> >> + dev_err(dev, "%s I2C failure: %d", __func__, ret);
>> >> >> + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
>> >> >> + }
>> >> >> +
>> >> >> + /* restore v4l2 control values */
>> >> >> + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm);
>> >> >
>> >> > Doesn't this need to be done for runtime_resume as well?
>> >>
>> >> This driver doesn't seem to be doing any physical power off in its
>> >> runtime_suspend and I don't expect an I2C device to be put in a PM
>> >> domain, so possibly no need for it.
>> >
>> > I'd expect runtime PM suspend callback to power the device off through ACPI
>> > PM. For this reason the device state must be restored when it's powered on,
>> > i.e. its runtime_resume callback.
>>
>> Ah, again ACPI here, that's something not usual for me. Sorry for the noise.
>
> No harm done. Very good comments in general IMO, thanks for those!
>
> --
> Kind regards,
>
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dw9714: Initial driver for dw9714 VCM
2017-05-09 13:22 ` Tomasz Figa
@ 2017-05-09 13:26 ` Tomasz Figa
0 siblings, 0 replies; 13+ messages in thread
From: Tomasz Figa @ 2017-05-09 13:26 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mani, Rajmohan, linux-media, mchehab, Rafael J. Wysocki,
Kevin Hilman, Ulf Hansson
On Tue, May 9, 2017 at 9:22 PM, Tomasz Figa <tfiga@chromium.org> wrote:
> +Rafael, Kevin and Ulf,
>
> On Tue, May 9, 2017 at 8:16 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>> Hi Tomasz,
>>
>> On Tue, May 09, 2017 at 07:38:26PM +0800, Tomasz Figa wrote:
>>> On Tue, May 9, 2017 at 6:40 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>>> > Hi Tomasz,
>>> >
>>> > On Tue, May 09, 2017 at 04:30:40PM +0800, Tomasz Figa wrote:
>>> >> Hi Sakari,
>>> >>
>>> >> On Tue, May 9, 2017 at 4:55 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>>> >> > Hi Rajmohan,
>>> >> >
>>> >> > A few comments below...
>>> >> >
>>> >> > On Sun, May 07, 2017 at 04:33:24AM -0700, rajmohan.mani@intel.com wrote:
>>> >> >> +#ifdef CONFIG_PM
>>> >> >> +
>>> >> >> +static int dw9714_runtime_suspend(struct device *dev)
>>> >> >> +{
>>> >> >> + return 0;
>>> >> >> +}
>>> >> >> +
>>> >> >> +static int dw9714_runtime_resume(struct device *dev)
>>> >> >> +{
>>> >> >> + return 0;
>>> >> >
>>> >> > I think it'd be fine to remove empty callbacks.
>>> >>
>>> >> It's actually a bit more complicated (if a PM domain is attached, the
>>> >> callbacks must be present), however in case of external I2C devices it
>>> >> should be fine indeed. However, AFAIK, pm_runtime_no_callbacks()
>>> >> should be called.
>>> >
>>> > I wonder if I'm missing something --- acpi_subsys_runtime_resume() first
>>> > calls acpi_dev_runtime_resume() and if all goes well, the proceeds to call
>>> > pm_generic_runtime_resume() which calls device's runtime_resume() if it's
>>> > non-NULL.
>>> >
>>> > In other words, having a runtime_resume() and runtime_suspend() callbacks
>>> > that return zero is equivalent of having neither of the callbacks.
>>>
>>> Ah, I missed the fact this device is instantiated by ACPI and it has
>>> different handling of runtime PM, which apparently means it doesn't
>>> use the code paths affected by the PM domain thing I mentioned.
>>
>> I have to admit I'm no expert in the topic but I'd presume that other
>> implementations should still maintain consistent behaviour towards drivers.
>> acpi_subsys_runtime_resume() is the PM domain runtime_resume() callback in
>> acpi_general_pm_domain.
>
> Let's see. For platform bus this seems to be reasonable indeed -
> __rpm_get_callback() will use the callbacks of PM domain, device type,
> device class or device bus, whichever is available first, in this
> order. Looking at platform_bus_type, the equivalent resume callback is
> pm_generic_runtime_resume() and it will indeed silently return 0 if
> there is no dev->pm->runtime_resume.
>
> However for I2C bus, i2c_bus_type doesn't seem to have .pm defined.
> Type and class are device/driver-specific things, so let's assume they
> don't have .pm set. If the driver doesn't have .pm, then the only way
> __rpm_get_callback() can return a non-NULL value is when
> dev->pm_domain is non NULL (which seems to be the ACPI case actually).
> Otherwise, if __rpm_get_callback() returns NULL, pm_runtime_get*()
> will fail with -ENOSYS. This is because rpm_resume() calls
> rpm_callback(callback, dev), where callback is the value returned by
> __rpm_get_callback() and rpm_callback() returns -ENOSYS if callback is
> NULL. This is where pm_runtime_no_callbacks() comes handy, as it
> bypasses the code getting and calling the callback, so it doesn't
> return the error anymore.
>
> There is however the other side of the coin. If
> pm_runtime_no_callbacks() is called, all kind of callbacks are
> bypassed, so even the PM domain code is not invoked. This is kind of
> tricky, because the driver must be now aware of whether it's running
> under a PM domain or not and call this pm_runtime_no_callbacks()
> depending on the outcome, to guarantee correct operation (PM domain
> callbacks must be called, even if device driver doesn't have its
> own)... Which IMHO doesn't make sense.
>
> I guess the correct way to proceed here would be adding .pm ops to
> i2c_bus_type, just as it's done with platform_bus_type.
Ah, sorry, I mean, the correct way to proceed here to make the
behavior consistent. For Raj's patch, since a) the driver needs to
restore the control value and b) it's supposed to be primarily
instantiated by ACPI, we shouldn't hit the above problem for now.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] dw9714: Initial driver for dw9714 VCM
2017-05-08 20:55 ` Sakari Ailus
2017-05-09 8:30 ` Tomasz Figa
@ 2017-05-09 13:27 ` Mani, Rajmohan
1 sibling, 0 replies; 13+ messages in thread
From: Mani, Rajmohan @ 2017-05-09 13:27 UTC (permalink / raw)
To: Sakari Ailus, Tomasz Figa; +Cc: linux-media, mchehab
Hi Sakari,
Please see comments inline below.
> -----Original Message-----
> From: Sakari Ailus [mailto:sakari.ailus@iki.fi]
> Sent: Tuesday, May 09, 2017 4:55 AM
> To: Mani, Rajmohan <rajmohan.mani@intel.com>
> Cc: linux-media@vger.kernel.org; mchehab@kernel.org
> Subject: Re: [PATCH] dw9714: Initial driver for dw9714 VCM
>
> Hi Rajmohan,
>
> A few comments below...
>
> On Sun, May 07, 2017 at 04:33:24AM -0700, rajmohan.mani@intel.com
> wrote:
> > From: Rajmohan Mani <rajmohan.mani@intel.com>
> >
> > DW9714 is a 10 bit DAC, designed for linear control of voice coil
> > motor.
> >
> > This driver creates a V4L2 subdevice and provides control to set the
> > desired focus.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
> > ---
> > drivers/media/i2c/Kconfig | 9 ++
> > drivers/media/i2c/Makefile | 1 +
> > drivers/media/i2c/dw9714.c | 333
> > +++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 343 insertions(+)
> > create mode 100644 drivers/media/i2c/dw9714.c
> >
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index fd181c9..4f0a7ad 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -300,6 +300,15 @@ config VIDEO_AD5820
> > This is a driver for the AD5820 camera lens voice coil.
> > It is used for example in Nokia N900 (RX-51).
> >
> > +config VIDEO_DW9714
> > + tristate "DW9714 lens voice coil support"
> > + depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
>
> You should also depend on VIDEO_V4L2_SUBDEV_API .
Ack
>
> > + ---help---
> > + This is a driver for the DW9714 camera lens voice coil.
> > + DW9714 is a 10 bit DAC with 120mA output current sink
> > + capability. This is designed for linear control of
> > + voice coil motors, controlled via I2C serial interface.
> > +
> > config VIDEO_SAA7110
> > tristate "Philips SAA7110 video decoder"
> > depends on VIDEO_V4L2 && I2C
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index 62323ec..987bd1f 100644
> > --- a/drivers/media/i2c/Makefile
> > +++ b/drivers/media/i2c/Makefile
> > @@ -21,6 +21,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
> > obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
> > obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
> > obj-$(CONFIG_VIDEO_AD5820) += ad5820.o
> > +obj-$(CONFIG_VIDEO_DW9714) += dw9714.o
> > obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
> > obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
> > obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o diff --git
> > a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c new file
> > mode 100644 index 0000000..276d3f2
> > --- /dev/null
> > +++ b/drivers/media/i2c/dw9714.c
> > @@ -0,0 +1,333 @@
> > +/*
> > + * Copyright (c) 2015--2017 Intel Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > +version
> > + * 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/pm_runtime.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-device.h>
> > +
> > +#define DW9714_NAME "dw9714"
> > +#define DW9714_MAX_FOCUS_POS 1023
> > +#define DW9714_CTRL_STEPS 16 /* Keep this value power of 2
> */
> > +#define DW9714_CTRL_DELAY_US 1000
> > +/*
> > + * S[3:2] = 0x00, codes per step for "Linear Slope Control"
> > + * S[1:0] = 0x00, step period
> > + */
> > +#define DW9714_DEFAULT_S 0x0
> > +#define DW9714_VAL(data, s) (u16)((data) << 4 | (s))
> > +
> > +/* dw9714 device structure */
> > +struct dw9714_device {
> > + struct i2c_client *client;
> > + struct v4l2_ctrl_handler ctrls_vcm;
> > + struct v4l2_subdev sd;
> > + u16 current_val;
> > +};
> > +
> > +#define to_dw9714_vcm(_ctrl) \
> > + container_of(_ctrl->handler, struct dw9714_device, ctrls_vcm)
> > +
> > +static int dw9714_i2c_write(struct i2c_client *client, u16 data) {
> > + const int num_msg = 1;
> > + int ret;
> > + u16 val = cpu_to_be16(data);
> > + struct i2c_msg msg = {
> > + .addr = client->addr,
> > + .flags = 0,
> > + .len = sizeof(val),
> > + .buf = (u8 *) &val,
> > + };
> > +
> > + ret = i2c_transfer(client->adapter, &msg, num_msg);
> > +
> > + /*One retry */
> > + if (ret != num_msg)
> > + ret = i2c_transfer(client->adapter, &msg, num_msg);
> > +
> > + if (ret != num_msg) {
> > + dev_err(&client->dev, "I2C write fail fail\n");
> > + return -EIO;
> > + } else {
> > + return 0;
> > + }
> > +}
> > +
> > +static int dw9714_t_focus_vcm(struct dw9714_device *dw9714_dev, u16
> > +val) {
> > + struct i2c_client *client = dw9714_dev->client;
> > + int ret = -EINVAL;
> > +
> > + dev_dbg(&client->dev, "Setting new value VCM: %d\n", val);
> > + dw9714_dev->current_val = val;
> > +
> > + ret = dw9714_i2c_write(client, DW9714_VAL(val,
> DW9714_DEFAULT_S));
> > + return ret;
> > +}
> > +
> > +static int dw9714_set_ctrl(struct v4l2_ctrl *ctrl) {
> > + struct dw9714_device *dev_vcm = to_dw9714_vcm(ctrl);
> > +
> > + if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE)
> > + return dw9714_t_focus_vcm(dev_vcm, ctrl->val);
> > + else
> > + return -EINVAL;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops dw9714_vcm_ctrl_ops = {
> > + .s_ctrl = dw9714_set_ctrl,
> > +};
> > +
> > +static int dw9714_init_controls(struct dw9714_device *dev_vcm) {
> > + struct v4l2_ctrl_handler *hdl = &dev_vcm->ctrls_vcm;
> > + const struct v4l2_ctrl_ops *ops = &dw9714_vcm_ctrl_ops;
> > + struct i2c_client *client = dev_vcm->client;
> > +
> > + v4l2_ctrl_handler_init(hdl, 1);
> > +
> > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE,
> > + 0, DW9714_MAX_FOCUS_POS, 1, 0);
> > +
> > + if (hdl->error)
> > + dev_err(&client->dev, "dw9714_init_controls fail\n");
> > + dev_vcm->sd.ctrl_handler = hdl;
> > + return hdl->error;
> > +}
> > +
> > +static void dw9714_subdev_cleanup(struct dw9714_device *dw9714_dev) {
> > + v4l2_ctrl_handler_free(&dw9714_dev->ctrls_vcm);
>
> Please release the control handler after unregistering the sub-device.
Ack
>
> > + v4l2_async_unregister_subdev(&dw9714_dev->sd);
> > + v4l2_device_unregister_subdev(&dw9714_dev->sd);
> > + media_entity_cleanup(&dw9714_dev->sd.entity);
> > +}
> > +
> > +static int dw9714_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> > +*fh) {
> > + struct dw9714_device *dw9714_dev = container_of(sd,
> > + struct
> dw9714_device,
> > + sd);
> > + struct device *dev = &dw9714_dev->client->dev;
> > + int rval;
> > +
> > + rval = pm_runtime_get_sync(dev);
>
> If this fails you'll need to do pm_runtime_put(dev);
Ack
>
> > + dev_dbg(dev, "%s rval = %d\n", __func__, rval);
> > +
> > + return rval;
> > +}
> > +
> > +static int dw9714_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> > +*fh) {
> > + struct dw9714_device *dw9714_dev = container_of(sd,
> > + struct
> dw9714_device,
> > + sd);
> > + struct device *dev = &dw9714_dev->client->dev;
> > +
> > + dev_dbg(dev, "%s\n", __func__);
> > + pm_runtime_put(dev);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_internal_ops dw9714_int_ops = {
> > + .open = dw9714_open,
> > + .close = dw9714_close,
> > +};
> > +
> > +static const struct v4l2_subdev_ops dw9714_ops = { };
>
> You should use v4l2_subdev_internal_ops here.
>
I am not sure I understand what you mean here. Can you please elaborate?
> > +
> > +static int dw9714_probe(struct i2c_client *client,
> > + const struct i2c_device_id *devid) {
> > + struct dw9714_device *dw9714_dev;
> > + int rval;
> > +
> > + dw9714_dev = devm_kzalloc(&client->dev, sizeof(*dw9714_dev),
> > + GFP_KERNEL);
> > +
> > + if (dw9714_dev == NULL)
> > + return -ENOMEM;
> > +
> > + dw9714_dev->client = client;
> > +
> > + v4l2_i2c_subdev_init(&dw9714_dev->sd, client, &dw9714_ops);
> > + dw9714_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > + dw9714_dev->sd.internal_ops = &dw9714_int_ops;
> > +
> > + snprintf(dw9714_dev->sd.name,
> > + sizeof(dw9714_dev->sd.name),
> > + DW9714_NAME " %d-%4.4x", i2c_adapter_id(client-
> >adapter),
> > + client->addr);
> > +
> > + rval = dw9714_init_controls(dw9714_dev);
> > + if (rval)
> > + goto err_cleanup;
> > +
> > + rval = media_entity_pads_init(&dw9714_dev->sd.entity, 0, NULL);
> > + if (rval < 0)
> > + goto err_cleanup;
> > +
> > + dw9714_dev->sd.entity.function = MEDIA_ENT_F_LENS;
> > +
> > + rval = v4l2_async_register_subdev(&dw9714_dev->sd);
> > + if (rval < 0)
> > + goto err_cleanup;
> > +
> > + pm_runtime_enable(&client->dev);
>
> Getting PM runtime right doesn't seem to be easy. :-I
>
> pm_runtime_enable() alone doesn't do the trick. I wonder if adding
> pm_runtime_suspend() would do the trick.
>
Based on the comments exchanged between you and Tomasz, I believe it is ok if we stop with just pm_runtime_enable().
P.S: Adding pm_runtime_suspend() did not result in dw9714_runtime_suspend() being invoked.
> > +
> > + return 0;
> > +
> > +err_cleanup:
> > + dw9714_subdev_cleanup(dw9714_dev);
> > + dev_err(&client->dev, "Probe failed: %d\n", rval);
> > + return rval;
> > +}
> > +
> > +static int dw9714_remove(struct i2c_client *client) {
> > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > + struct dw9714_device *dw9714_dev = container_of(sd,
> > + struct
> dw9714_device,
> > + sd);
> > +
> > + pm_runtime_disable(&client->dev);
> > + dw9714_subdev_cleanup(dw9714_dev);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +
> > +static int dw9714_runtime_suspend(struct device *dev) {
> > + return 0;
> > +}
> > +
> > +static int dw9714_runtime_resume(struct device *dev) {
> > + return 0;
>
> I think it'd be fine to remove empty callbacks.
Given that we need to do whatever that's done in dw9714_suspend/resume functions in dw9714_runtime_suspend/resume, these functions are not going to be empty callbacks.
>
> > +}
> > +
> > +/* This function sets the vcm position, so it consumes least current
> > +*/ static int dw9714_suspend(struct device *dev) {
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > + struct dw9714_device *dw9714_dev = container_of(sd,
> > + struct
> dw9714_device,
> > + sd);
> > + int ret, val;
> > +
> > + dev_dbg(dev, "%s\n", __func__);
> > +
> > + for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS - 1);
> > + val >= 0; val -= DW9714_CTRL_STEPS) {
> > + ret = dw9714_i2c_write(client,
> > + DW9714_VAL((u16) val,
> DW9714_DEFAULT_S));
> > + if (ret)
> > + dev_err(dev, "%s I2C failure: %d", __func__, ret);
> > + usleep_range(DW9714_CTRL_DELAY_US,
> DW9714_CTRL_DELAY_US + 10);
> > + }
> > + return 0;
> > +}
> > +
> > +/*
> > + * This function sets the vcm position, so the focus position is set
> > + * closer to the camera
> > + */
> > +static int dw9714_resume(struct device *dev) {
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > + struct dw9714_device *dw9714_dev = container_of(sd,
> > + struct
> dw9714_device,
> > + sd);
> > + int ret, val;
> > +
> > + dev_dbg(dev, "%s\n", __func__);
> > +
> > + for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
> > + val < dw9714_dev->current_val + DW9714_CTRL_STEPS - 1;
> > + val += DW9714_CTRL_STEPS) {
> > + ret = dw9714_i2c_write(client,
> > + DW9714_VAL((u16) val,
> DW9714_DEFAULT_S));
> > + if (ret)
> > + dev_err(dev, "%s I2C failure: %d", __func__, ret);
> > + usleep_range(DW9714_CTRL_DELAY_US,
> DW9714_CTRL_DELAY_US + 10);
> > + }
> > +
> > + /* restore v4l2 control values */
> > + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm);
>
> Doesn't this need to be done for runtime_resume as well?
Ack.
I will add dw9714_vcm_suspend/resume functions and invoke them from dw9714_suspend/resume and dw9714_runtime_suspend/resume functions
>
> > + dev_dbg(dev, "%s ret = %d\n", __func__, ret);
> > + return ret;
> > +}
> > +
> > +#else
> > +
> > +#define dw9714_suspend NULL
> > +#define dw9714_resume NULL
> > +#define dw9714_runtime_suspend NULL
> > +#define dw9714_runtime_resume NULL
> > +
> > +#endif /* CONFIG_PM */
> > +
> > +#ifdef CONFIG_ACPI
> > +static const struct acpi_device_id dw9714_acpi_match[] = {
> > + {"DW9714", 0},
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(acpi, dw9714_acpi_match); #endif
> > +
> > +static const struct i2c_device_id dw9714_id_table[] = {
> > + {DW9714_NAME, 0},
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, dw9714_id_table);
> > +
> > +static const struct dev_pm_ops dw9714_pm_ops = {
> > + .suspend = dw9714_suspend,
> > + .resume = dw9714_resume,
> > + .runtime_suspend = dw9714_runtime_suspend,
> > + .runtime_resume = dw9714_runtime_resume, };
> > +
> > +static struct i2c_driver dw9714_i2c_driver = {
> > + .driver = {
> > + .name = DW9714_NAME,
> > + .pm = &dw9714_pm_ops,
> > +#ifdef CONFIG_ACPI
> > + .acpi_match_table = ACPI_PTR(dw9714_acpi_match), #endif
> > + },
> > + .probe = dw9714_probe,
> > + .remove = dw9714_remove,
> > + .id_table = dw9714_id_table,
> > +};
> > +
> > +module_i2c_driver(dw9714_i2c_driver);
> > +
> > +MODULE_AUTHOR("Tianshu Qiu <tian.shu.qiu@intel.com>");
> > +MODULE_AUTHOR("Jian Xu Zheng <jian.xu.zheng@intel.com>");
> > +MODULE_AUTHOR("Yuning Pu <yuning.pu@intel.com>");
> > +MODULE_AUTHOR("Jouni Ukkonen <jouni.ukkonen@intel.com>");
> > +MODULE_AUTHOR("Tommi Franttila <tommi.franttila@intel.com>");
> > +MODULE_DESCRIPTION("DW9714 VCM driver"); MODULE_LICENSE("GPL");
>
> --
> Regards,
>
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] dw9714: Initial driver for dw9714 VCM
2017-05-26 1:49 Rajmohan Mani
@ 2017-05-26 1:58 ` Mani, Rajmohan
0 siblings, 0 replies; 13+ messages in thread
From: Mani, Rajmohan @ 2017-05-26 1:58 UTC (permalink / raw)
To: linux-media
Cc: mchehab, hverkuil, tfiga, sakari.ailus, s.nawrocki, Toivonen, Tuukka
Hi All,
Please follow v5 of this patch for the latest and ignore this patch.
Sorry for the noise.
Raj
> -----Original Message-----
> From: Mani, Rajmohan
> Sent: Thursday, May 25, 2017 6:50 PM
> To: linux-media@vger.kernel.org
> Cc: mchehab@kernel.org; hverkuil@xs4all.nl; tfiga@chromium.org;
> sakari.ailus@iki.fi; s.nawrocki@samsung.com; Toivonen, Tuukka
> <tuukka.toivonen@intel.com>; Mani, Rajmohan <rajmohan.mani@intel.com>
> Subject: [PATCH] dw9714: Initial driver for dw9714 VCM
>
> DW9714 is a 10 bit DAC, designed for linear control of voice coil motor.
>
> This driver creates a V4L2 subdevice and provides control to set the desired
> focus.
>
> Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
> ---
> drivers/media/i2c/Kconfig | 9 ++
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/dw9714.c | 292
> +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 302 insertions(+)
> create mode 100644 drivers/media/i2c/dw9714.c
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index
> fd181c9..516e2f2 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -300,6 +300,15 @@ config VIDEO_AD5820
> This is a driver for the AD5820 camera lens voice coil.
> It is used for example in Nokia N900 (RX-51).
>
> +config VIDEO_DW9714
> + tristate "DW9714 lens voice coil support"
> + depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER &&
> VIDEO_V4L2_SUBDEV_API
> + ---help---
> + This is a driver for the DW9714 camera lens voice coil.
> + DW9714 is a 10 bit DAC with 120mA output current sink
> + capability. This is designed for linear control of
> + voice coil motors, controlled via I2C serial interface.
> +
> config VIDEO_SAA7110
> tristate "Philips SAA7110 video decoder"
> depends on VIDEO_V4L2 && I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index
> 62323ec..987bd1f 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
> obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
> obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
> obj-$(CONFIG_VIDEO_AD5820) += ad5820.o
> +obj-$(CONFIG_VIDEO_DW9714) += dw9714.o
> obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
> obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
> obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o diff --git
> a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c new file mode
> 100644 index 0000000..22c84de
> --- /dev/null
> +++ b/drivers/media/i2c/dw9714.c
> @@ -0,0 +1,292 @@
> +/*
> + * Copyright (c) 2015--2017 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version
> + * 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +
> +#define DW9714_NAME "dw9714"
> +#define DW9714_MAX_FOCUS_POS 1023
> +/*
> + * This acts as the minimum granularity of lens movement.
> + * Keep this value power of 2, so the control steps can be
> + * uniformly adjusted for gradual lens movement, with desired
> + * number of control steps.
> + */
> +#define DW9714_CTRL_STEPS 16
> +#define DW9714_CTRL_DELAY_US 1000
> +/*
> + * S[3:2] = 0x00, codes per step for "Linear Slope Control"
> + * S[1:0] = 0x00, step period
> + */
> +#define DW9714_DEFAULT_S 0x0
> +#define DW9714_VAL(data, s) ((data) << 4 | (s))
> +
> +/* dw9714 device structure */
> +struct dw9714_device {
> + struct i2c_client *client;
> + struct v4l2_ctrl_handler ctrls_vcm;
> + struct v4l2_subdev sd;
> + u16 current_val;
> +};
> +
> +static inline struct dw9714_device *to_dw9714_vcm(struct v4l2_ctrl
> +*ctrl) {
> + return container_of(ctrl->handler, struct dw9714_device, ctrls_vcm); }
> +
> +static inline struct dw9714_device *sd_to_dw9714_vcm(struct v4l2_subdev
> +*subdev) {
> + return container_of(subdev, struct dw9714_device, sd); }
> +
> +static int dw9714_i2c_write(struct i2c_client *client, u16 data) {
> + int ret;
> + u16 val = cpu_to_be16(data);
> +
> + ret = i2c_master_send(client, (const char *)&val, sizeof(val));
> + if (ret != sizeof(val)) {
> + dev_err(&client->dev, "I2C write fail\n");
> + return -EIO;
> + }
> + return 0;
> +}
> +
> +static int dw9714_t_focus_vcm(struct dw9714_device *dw9714_dev, u16
> +val) {
> + struct i2c_client *client = dw9714_dev->client;
> +
> + dw9714_dev->current_val = val;
> +
> + return dw9714_i2c_write(client, DW9714_VAL(val,
> DW9714_DEFAULT_S)); }
> +
> +static int dw9714_set_ctrl(struct v4l2_ctrl *ctrl) {
> + struct dw9714_device *dev_vcm = to_dw9714_vcm(ctrl);
> +
> + if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE)
> + return dw9714_t_focus_vcm(dev_vcm, ctrl->val);
> +
> + return -EINVAL;
> +}
> +
> +static const struct v4l2_ctrl_ops dw9714_vcm_ctrl_ops = {
> + .s_ctrl = dw9714_set_ctrl,
> +};
> +
> +static int dw9714_init_controls(struct dw9714_device *dev_vcm) {
> + struct v4l2_ctrl_handler *hdl = &dev_vcm->ctrls_vcm;
> + const struct v4l2_ctrl_ops *ops = &dw9714_vcm_ctrl_ops;
> + struct i2c_client *client = dev_vcm->client;
> +
> + v4l2_ctrl_handler_init(hdl, 1);
> +
> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE,
> + 0, DW9714_MAX_FOCUS_POS,
> DW9714_CTRL_STEPS, 0);
> +
> + if (hdl->error)
> + dev_err(&client->dev, "dw9714_init_controls fail\n");
> + dev_vcm->sd.ctrl_handler = hdl;
> + return hdl->error;
> +}
> +
> +static void dw9714_subdev_cleanup(struct dw9714_device *dw9714_dev) {
> + v4l2_async_unregister_subdev(&dw9714_dev->sd);
> + v4l2_device_unregister_subdev(&dw9714_dev->sd);
> + v4l2_ctrl_handler_free(&dw9714_dev->ctrls_vcm);
> + media_entity_cleanup(&dw9714_dev->sd.entity);
> +}
> +
> +static int dw9714_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> +*fh) {
> + struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
> + struct device *dev = &dw9714_dev->client->dev;
> + int rval;
> +
> + rval = pm_runtime_get_sync(dev);
> + if (rval < 0) {
> + pm_runtime_put_noidle(dev);
> + return rval;
> + }
> +
> + return 0;
> +}
> +
> +static int dw9714_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> +*fh) {
> + struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
> + struct device *dev = &dw9714_dev->client->dev;
> +
> + pm_runtime_put(dev);
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_internal_ops dw9714_int_ops = {
> + .open = dw9714_open,
> + .close = dw9714_close,
> +};
> +
> +static const struct v4l2_subdev_ops dw9714_ops = { };
> +
> +static int dw9714_probe(struct i2c_client *client,
> + const struct i2c_device_id *devid)
> +{
> + struct dw9714_device *dw9714_dev;
> + int rval;
> +
> + dw9714_dev = devm_kzalloc(&client->dev, sizeof(*dw9714_dev),
> + GFP_KERNEL);
> +
> + if (dw9714_dev == NULL)
> + return -ENOMEM;
> +
> + dw9714_dev->client = client;
> +
> + v4l2_i2c_subdev_init(&dw9714_dev->sd, client, &dw9714_ops);
> + dw9714_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + dw9714_dev->sd.internal_ops = &dw9714_int_ops;
> +
> + rval = dw9714_init_controls(dw9714_dev);
> + if (rval)
> + goto err_cleanup;
> +
> + rval = media_entity_pads_init(&dw9714_dev->sd.entity, 0, NULL);
> + if (rval < 0)
> + goto err_cleanup;
> +
> + dw9714_dev->sd.entity.function = MEDIA_ENT_F_LENS;
> +
> + rval = v4l2_async_register_subdev(&dw9714_dev->sd);
> + if (rval < 0)
> + goto err_cleanup;
> +
> + pm_runtime_enable(&client->dev);
> +
> + return 0;
> +
> +err_cleanup:
> + dw9714_subdev_cleanup(dw9714_dev);
> + dev_err(&client->dev, "Probe failed: %d\n", rval);
> + return rval;
> +}
> +
> +static int dw9714_remove(struct i2c_client *client) {
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
> +
> + pm_runtime_disable(&client->dev);
> + dw9714_subdev_cleanup(dw9714_dev);
> +
> + return 0;
> +}
> +
> +/*
> + * This function sets the vcm position, so it consumes least current
> + * The lens position is gradually moved in units of DW9714_CTRL_STEPS,
> + * to make the movements smoothly.
> + */
> +static int __maybe_unused dw9714_vcm_suspend(struct device *dev) {
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
> + int ret, val;
> +
> + for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS - 1);
> + val >= 0; val -= DW9714_CTRL_STEPS) {
> + ret = dw9714_i2c_write(client,
> + DW9714_VAL(val, DW9714_DEFAULT_S));
> + if (ret)
> + dev_err_once(dev, "%s I2C failure: %d", __func__, ret);
> + usleep_range(DW9714_CTRL_DELAY_US,
> DW9714_CTRL_DELAY_US + 10);
> + }
> + return 0;
> +}
> +
> +/*
> + * This function sets the vcm position to the value set by the user
> + * through v4l2_ctrl_ops s_ctrl handler
> + * The lens position is gradually moved in units of DW9714_CTRL_STEPS,
> + * to make the movements smoothly.
> + */
> +static int __maybe_unused dw9714_vcm_resume(struct device *dev) {
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
> + int ret, val;
> +
> + for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
> + val < dw9714_dev->current_val + DW9714_CTRL_STEPS - 1;
> + val += DW9714_CTRL_STEPS) {
> + ret = dw9714_i2c_write(client,
> + DW9714_VAL(val, DW9714_DEFAULT_S));
> + if (ret)
> + dev_err_ratelimited(dev, "%s I2C failure: %d",
> + __func__, ret);
> + usleep_range(DW9714_CTRL_DELAY_US,
> DW9714_CTRL_DELAY_US + 10);
> + }
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id dw9714_acpi_match[] = {
> + {"DW9714", 0},
> + {},
> +};
> +MODULE_DEVICE_TABLE(acpi, dw9714_acpi_match); #endif
> +
> +static const struct i2c_device_id dw9714_id_table[] = {
> + {DW9714_NAME, 0},
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, dw9714_id_table);
> +
> +static const struct dev_pm_ops dw9714_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(dw9714_vcm_suspend,
> dw9714_vcm_resume)
> + SET_RUNTIME_PM_OPS(dw9714_vcm_suspend,
> dw9714_vcm_resume, NULL) };
> +
> +static struct i2c_driver dw9714_i2c_driver = {
> + .driver = {
> + .name = DW9714_NAME,
> + .pm = &dw9714_pm_ops,
> + .acpi_match_table = ACPI_PTR(dw9714_acpi_match),
> + },
> + .probe = dw9714_probe,
> + .remove = dw9714_remove,
> + .id_table = dw9714_id_table,
> +};
> +
> +module_i2c_driver(dw9714_i2c_driver);
> +
> +MODULE_AUTHOR("Tianshu Qiu <tian.shu.qiu@intel.com>");
> +MODULE_AUTHOR("Jian Xu Zheng <jian.xu.zheng@intel.com>");
> +MODULE_AUTHOR("Yuning Pu <yuning.pu@intel.com>");
> MODULE_AUTHOR("Jouni
> +Ukkonen <jouni.ukkonen@intel.com>"); MODULE_AUTHOR("Tommi Franttila
> +<tommi.franttila@intel.com>");
> +MODULE_DESCRIPTION("DW9714 VCM driver"); MODULE_LICENSE("GPL");
> --
> 1.9.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] dw9714: Initial driver for dw9714 VCM
@ 2017-05-26 1:49 Rajmohan Mani
2017-05-26 1:58 ` Mani, Rajmohan
0 siblings, 1 reply; 13+ messages in thread
From: Rajmohan Mani @ 2017-05-26 1:49 UTC (permalink / raw)
To: linux-media
Cc: mchehab, hverkuil, tfiga, sakari.ailus, s.nawrocki,
tuukka.toivonen, Rajmohan Mani
DW9714 is a 10 bit DAC, designed for linear
control of voice coil motor.
This driver creates a V4L2 subdevice and
provides control to set the desired focus.
Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
---
drivers/media/i2c/Kconfig | 9 ++
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/dw9714.c | 292 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 302 insertions(+)
create mode 100644 drivers/media/i2c/dw9714.c
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index fd181c9..516e2f2 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -300,6 +300,15 @@ config VIDEO_AD5820
This is a driver for the AD5820 camera lens voice coil.
It is used for example in Nokia N900 (RX-51).
+config VIDEO_DW9714
+ tristate "DW9714 lens voice coil support"
+ depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER && VIDEO_V4L2_SUBDEV_API
+ ---help---
+ This is a driver for the DW9714 camera lens voice coil.
+ DW9714 is a 10 bit DAC with 120mA output current sink
+ capability. This is designed for linear control of
+ voice coil motors, controlled via I2C serial interface.
+
config VIDEO_SAA7110
tristate "Philips SAA7110 video decoder"
depends on VIDEO_V4L2 && I2C
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 62323ec..987bd1f 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
obj-$(CONFIG_VIDEO_AD5820) += ad5820.o
+obj-$(CONFIG_VIDEO_DW9714) += dw9714.o
obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o
diff --git a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c
new file mode 100644
index 0000000..22c84de
--- /dev/null
+++ b/drivers/media/i2c/dw9714.c
@@ -0,0 +1,292 @@
+/*
+ * Copyright (c) 2015--2017 Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+
+#define DW9714_NAME "dw9714"
+#define DW9714_MAX_FOCUS_POS 1023
+/*
+ * This acts as the minimum granularity of lens movement.
+ * Keep this value power of 2, so the control steps can be
+ * uniformly adjusted for gradual lens movement, with desired
+ * number of control steps.
+ */
+#define DW9714_CTRL_STEPS 16
+#define DW9714_CTRL_DELAY_US 1000
+/*
+ * S[3:2] = 0x00, codes per step for "Linear Slope Control"
+ * S[1:0] = 0x00, step period
+ */
+#define DW9714_DEFAULT_S 0x0
+#define DW9714_VAL(data, s) ((data) << 4 | (s))
+
+/* dw9714 device structure */
+struct dw9714_device {
+ struct i2c_client *client;
+ struct v4l2_ctrl_handler ctrls_vcm;
+ struct v4l2_subdev sd;
+ u16 current_val;
+};
+
+static inline struct dw9714_device *to_dw9714_vcm(struct v4l2_ctrl *ctrl)
+{
+ return container_of(ctrl->handler, struct dw9714_device, ctrls_vcm);
+}
+
+static inline struct dw9714_device *sd_to_dw9714_vcm(struct v4l2_subdev *subdev)
+{
+ return container_of(subdev, struct dw9714_device, sd);
+}
+
+static int dw9714_i2c_write(struct i2c_client *client, u16 data)
+{
+ int ret;
+ u16 val = cpu_to_be16(data);
+
+ ret = i2c_master_send(client, (const char *)&val, sizeof(val));
+ if (ret != sizeof(val)) {
+ dev_err(&client->dev, "I2C write fail\n");
+ return -EIO;
+ }
+ return 0;
+}
+
+static int dw9714_t_focus_vcm(struct dw9714_device *dw9714_dev, u16 val)
+{
+ struct i2c_client *client = dw9714_dev->client;
+
+ dw9714_dev->current_val = val;
+
+ return dw9714_i2c_write(client, DW9714_VAL(val, DW9714_DEFAULT_S));
+}
+
+static int dw9714_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct dw9714_device *dev_vcm = to_dw9714_vcm(ctrl);
+
+ if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE)
+ return dw9714_t_focus_vcm(dev_vcm, ctrl->val);
+
+ return -EINVAL;
+}
+
+static const struct v4l2_ctrl_ops dw9714_vcm_ctrl_ops = {
+ .s_ctrl = dw9714_set_ctrl,
+};
+
+static int dw9714_init_controls(struct dw9714_device *dev_vcm)
+{
+ struct v4l2_ctrl_handler *hdl = &dev_vcm->ctrls_vcm;
+ const struct v4l2_ctrl_ops *ops = &dw9714_vcm_ctrl_ops;
+ struct i2c_client *client = dev_vcm->client;
+
+ v4l2_ctrl_handler_init(hdl, 1);
+
+ v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE,
+ 0, DW9714_MAX_FOCUS_POS, DW9714_CTRL_STEPS, 0);
+
+ if (hdl->error)
+ dev_err(&client->dev, "dw9714_init_controls fail\n");
+ dev_vcm->sd.ctrl_handler = hdl;
+ return hdl->error;
+}
+
+static void dw9714_subdev_cleanup(struct dw9714_device *dw9714_dev)
+{
+ v4l2_async_unregister_subdev(&dw9714_dev->sd);
+ v4l2_device_unregister_subdev(&dw9714_dev->sd);
+ v4l2_ctrl_handler_free(&dw9714_dev->ctrls_vcm);
+ media_entity_cleanup(&dw9714_dev->sd.entity);
+}
+
+static int dw9714_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
+ struct device *dev = &dw9714_dev->client->dev;
+ int rval;
+
+ rval = pm_runtime_get_sync(dev);
+ if (rval < 0) {
+ pm_runtime_put_noidle(dev);
+ return rval;
+ }
+
+ return 0;
+}
+
+static int dw9714_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
+ struct device *dev = &dw9714_dev->client->dev;
+
+ pm_runtime_put(dev);
+
+ return 0;
+}
+
+static const struct v4l2_subdev_internal_ops dw9714_int_ops = {
+ .open = dw9714_open,
+ .close = dw9714_close,
+};
+
+static const struct v4l2_subdev_ops dw9714_ops = { };
+
+static int dw9714_probe(struct i2c_client *client,
+ const struct i2c_device_id *devid)
+{
+ struct dw9714_device *dw9714_dev;
+ int rval;
+
+ dw9714_dev = devm_kzalloc(&client->dev, sizeof(*dw9714_dev),
+ GFP_KERNEL);
+
+ if (dw9714_dev == NULL)
+ return -ENOMEM;
+
+ dw9714_dev->client = client;
+
+ v4l2_i2c_subdev_init(&dw9714_dev->sd, client, &dw9714_ops);
+ dw9714_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+ dw9714_dev->sd.internal_ops = &dw9714_int_ops;
+
+ rval = dw9714_init_controls(dw9714_dev);
+ if (rval)
+ goto err_cleanup;
+
+ rval = media_entity_pads_init(&dw9714_dev->sd.entity, 0, NULL);
+ if (rval < 0)
+ goto err_cleanup;
+
+ dw9714_dev->sd.entity.function = MEDIA_ENT_F_LENS;
+
+ rval = v4l2_async_register_subdev(&dw9714_dev->sd);
+ if (rval < 0)
+ goto err_cleanup;
+
+ pm_runtime_enable(&client->dev);
+
+ return 0;
+
+err_cleanup:
+ dw9714_subdev_cleanup(dw9714_dev);
+ dev_err(&client->dev, "Probe failed: %d\n", rval);
+ return rval;
+}
+
+static int dw9714_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
+
+ pm_runtime_disable(&client->dev);
+ dw9714_subdev_cleanup(dw9714_dev);
+
+ return 0;
+}
+
+/*
+ * This function sets the vcm position, so it consumes least current
+ * The lens position is gradually moved in units of DW9714_CTRL_STEPS,
+ * to make the movements smoothly.
+ */
+static int __maybe_unused dw9714_vcm_suspend(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
+ int ret, val;
+
+ for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS - 1);
+ val >= 0; val -= DW9714_CTRL_STEPS) {
+ ret = dw9714_i2c_write(client,
+ DW9714_VAL(val, DW9714_DEFAULT_S));
+ if (ret)
+ dev_err_once(dev, "%s I2C failure: %d", __func__, ret);
+ usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
+ }
+ return 0;
+}
+
+/*
+ * This function sets the vcm position to the value set by the user
+ * through v4l2_ctrl_ops s_ctrl handler
+ * The lens position is gradually moved in units of DW9714_CTRL_STEPS,
+ * to make the movements smoothly.
+ */
+static int __maybe_unused dw9714_vcm_resume(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
+ int ret, val;
+
+ for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
+ val < dw9714_dev->current_val + DW9714_CTRL_STEPS - 1;
+ val += DW9714_CTRL_STEPS) {
+ ret = dw9714_i2c_write(client,
+ DW9714_VAL(val, DW9714_DEFAULT_S));
+ if (ret)
+ dev_err_ratelimited(dev, "%s I2C failure: %d",
+ __func__, ret);
+ usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
+ }
+
+ return 0;
+}
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id dw9714_acpi_match[] = {
+ {"DW9714", 0},
+ {},
+};
+MODULE_DEVICE_TABLE(acpi, dw9714_acpi_match);
+#endif
+
+static const struct i2c_device_id dw9714_id_table[] = {
+ {DW9714_NAME, 0},
+ {}
+};
+
+MODULE_DEVICE_TABLE(i2c, dw9714_id_table);
+
+static const struct dev_pm_ops dw9714_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(dw9714_vcm_suspend, dw9714_vcm_resume)
+ SET_RUNTIME_PM_OPS(dw9714_vcm_suspend, dw9714_vcm_resume, NULL)
+};
+
+static struct i2c_driver dw9714_i2c_driver = {
+ .driver = {
+ .name = DW9714_NAME,
+ .pm = &dw9714_pm_ops,
+ .acpi_match_table = ACPI_PTR(dw9714_acpi_match),
+ },
+ .probe = dw9714_probe,
+ .remove = dw9714_remove,
+ .id_table = dw9714_id_table,
+};
+
+module_i2c_driver(dw9714_i2c_driver);
+
+MODULE_AUTHOR("Tianshu Qiu <tian.shu.qiu@intel.com>");
+MODULE_AUTHOR("Jian Xu Zheng <jian.xu.zheng@intel.com>");
+MODULE_AUTHOR("Yuning Pu <yuning.pu@intel.com>");
+MODULE_AUTHOR("Jouni Ukkonen <jouni.ukkonen@intel.com>");
+MODULE_AUTHOR("Tommi Franttila <tommi.franttila@intel.com>");
+MODULE_DESCRIPTION("DW9714 VCM driver");
+MODULE_LICENSE("GPL");
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-05-26 1:58 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-07 11:33 [PATCH] dw9714: Initial driver for dw9714 VCM rajmohan.mani
2017-05-08 10:59 ` Hans Verkuil
2017-05-08 14:14 ` Mani, Rajmohan
2017-05-08 20:55 ` Sakari Ailus
2017-05-09 8:30 ` Tomasz Figa
2017-05-09 10:40 ` Sakari Ailus
2017-05-09 11:38 ` Tomasz Figa
2017-05-09 12:16 ` Sakari Ailus
2017-05-09 13:22 ` Tomasz Figa
2017-05-09 13:26 ` Tomasz Figa
2017-05-09 13:27 ` Mani, Rajmohan
2017-05-26 1:49 Rajmohan Mani
2017-05-26 1:58 ` Mani, Rajmohan
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.