| Message ID | 1494478820-22199-1-git-send-email-rajmohan.mani@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Raj, Thanks for re-spin. Still a bit more comments inline. (I missed few more before, sorry.) On Thu, May 11, 2017 at 1:00 PM, Rajmohan Mani <rajmohan.mani@intel.com> wrote: > DW9714 is a 10 bit DAC, designed for linear > control of voice coil motor. [snip] > +static int dw9714_i2c_write(struct i2c_client *client, u16 data) > +{ > + int ret; > + u16 val = cpu_to_be16(data); > + const int num_bytes = sizeof(val); > + > + ret = i2c_master_send(client, (const char *) &val, sizeof(val)); nit: No need for space between cast and casted value. > + > + /*One retry */ > + if (ret != num_bytes) > + ret = i2c_master_send(client, (const char *) &val, sizeof(val)); Why do we need this retry? > + > + if (ret != num_bytes) { > + 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)); This still doesn't seem to apply the control gradually as suspend and resume do. > +} [snip] > +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 (rval >= 0) > + return 0; > + > + pm_runtime_put(dev); > + return rval; nit: The typical coding style is to return early in case of a special case and keep the common path linear, i.e. rval = pm_runtime_get_sync(dev); if (rval < 0) { pm_runtime_put(dev); return rval; } return 0; > +} > + [snip] > +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 #if defined(CONFIG_PM) || defined(CONFIG_PM_SLEEP) > + > +/* > + * 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 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 = container_of(sd, > + struct dw9714_device, > + 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((u16) val, DW9714_DEFAULT_S)); DW9714_VAL() already contains such cast. Anyway, I still think they don't really give us anything and should be removed. > + if (ret) > + dev_err(dev, "%s I2C failure: %d", __func__, ret); I think we should just return an error code here and fail the suspend. > + 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 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 = container_of(sd, > + struct dw9714_device, > + 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((u16) val, DW9714_DEFAULT_S)); Ditto. > + if (ret) > + dev_err(dev, "%s I2C failure: %d", __func__, ret); Ditto. > + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10); > + } > + > + /* restore v4l2 control values */ > + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm); > + return ret; Hmm, actually I believe v4l2_ctrl_handler_setup() will call .s_ctrl() here and set the motor value again. If we just make .s_ctrl() do the adjustment in steps properly, we can simplify the resume to simply call v4l2_ctrl_handler_setup() alone. > +} #endif #ifdef CONFIG_PM > + > +static int dw9714_runtime_suspend(struct device *dev) > +{ > + return dw9714_vcm_suspend(dev); > +} > + > +static int dw9714_runtime_resume(struct device *dev) > +{ > + return dw9714_vcm_resume(dev); > +} #endif #ifdef CONFIG_PM_SLEEP > + > +static int dw9714_suspend(struct device *dev) > +{ > + return dw9714_vcm_suspend(dev); > +} > + > +static int dw9714_resume(struct device *dev) > +{ > + return dw9714_vcm_resume(dev); > +} #endif Or you could actually just use dw9714_vcm_{suspend,resume}() directly for the callbacks and avoid the duplicates above. > + > +#else > + > +#define dw9714_vcm_suspend NULL > +#define dw9714_vcm_resume NULL This #else block is not needed. Best regards, Tomasz
Hi Rajmohan, A few minor comments below. On Wed, May 10, 2017 at 10:00:20PM -0700, Rajmohan Mani wrote: > 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> > --- > Changes in v4: > - Addressed review comments from Tomasz > Changes in v3: > - Addressed most of the review comments from Sakari > on v1 of this patch > Changes in v2: > - Addressed review comments from Hans Verkuil > - Fixed a debug message typo > - Got rid of a return variable > --- > drivers/media/i2c/Kconfig | 9 ++ > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/dw9714.c | 332 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 342 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..fefd5d2 > --- /dev/null > +++ b/drivers/media/i2c/dw9714.c > @@ -0,0 +1,332 @@ > +/* > + * 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) (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; > +}; > + > +static inline struct dw9714_device *to_dw9714_vcm(struct v4l2_ctrl *ctrl) > +{ > + return container_of(ctrl->handler, struct dw9714_device, ctrls_vcm); > +} > + > +static int dw9714_i2c_write(struct i2c_client *client, u16 data) > +{ > + int ret; > + u16 val = cpu_to_be16(data); > + const int num_bytes = sizeof(val); > + > + ret = i2c_master_send(client, (const char *) &val, sizeof(val)); > + > + /*One retry */ > + if (ret != num_bytes) > + ret = i2c_master_send(client, (const char *) &val, sizeof(val)); > + > + if (ret != num_bytes) { Use sizeof(val) here and three linees above and you can remove num_bytes. > + 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 = container_of(sd, > + struct dw9714_device, > + sd); > + struct device *dev = &dw9714_dev->client->dev; > + int rval; > + > + rval = pm_runtime_get_sync(dev); > + if (rval >= 0) > + return 0; > + > + pm_runtime_put(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; > + > + 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 = container_of(sd, > + struct dw9714_device, > + sd); > + > + pm_runtime_disable(&client->dev); > + dw9714_subdev_cleanup(dw9714_dev); > + > + return 0; > +} > + > +#ifdef CONFIG_PM > + > +/* > + * 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 dw9714_vcm_suspend(struct device *dev) static int __maybe_unused > +{ > + 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); Could you add an inline function for converting v4l2_subdev to dw9714_device? It's done in quite a few places. > + 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((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 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 dw9714_vcm_resume(struct device *dev) static int __maybe_unused > +{ > + 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; > + > + 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); > + return ret; > +} > + > +static int dw9714_runtime_suspend(struct device *dev) > +{ > + return dw9714_vcm_suspend(dev); You can remove the four callbacks and use dw9714_vcm_{suspend,resume} directly. > +} > + > +static int dw9714_runtime_resume(struct device *dev) > +{ > + return dw9714_vcm_resume(dev); > +} > + > +static int dw9714_suspend(struct device *dev) > +{ > + return dw9714_vcm_suspend(dev); > +} > + > +static int dw9714_resume(struct device *dev) > +{ > + return dw9714_vcm_resume(dev); > +} > + > +#else And please remove the #ifdefs. They're not needed here. > + > +#define dw9714_vcm_suspend NULL > +#define dw9714_vcm_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 = { > + SET_SYSTEM_SLEEP_PM_OPS(dw9714_suspend, dw9714_resume) > + SET_RUNTIME_PM_OPS(dw9714_runtime_suspend, dw9714_runtime_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");
Hi Tomasz, On Thu, May 11, 2017 at 02:30:31PM +0800, Tomasz Figa wrote: ... > > + > > +/* > > + * 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 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 = container_of(sd, > > + struct dw9714_device, > > + 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((u16) val, DW9714_DEFAULT_S)); > > DW9714_VAL() already contains such cast. Anyway, I still think they > don't really give us anything and should be removed. > > > + if (ret) > > + dev_err(dev, "%s I2C failure: %d", __func__, ret); > > I think we should just return an error code here and fail the suspend. The result from an error here is that the user would hear an audible click. I don't think it's worth failing system suspend. :-) But as no action is taken based on the error code, there could be quite a few of these messages. How about dev_err_once()? For resume I might use dev_err_ratelimited(). > > > + 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 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 = container_of(sd, > > + struct dw9714_device, > > + 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((u16) val, DW9714_DEFAULT_S)); > > Ditto. > > > + if (ret) > > + dev_err(dev, "%s I2C failure: %d", __func__, ret); > > Ditto. > > > + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10); > > + } > > + > > + /* restore v4l2 control values */ > > + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm); > > + return ret; > > Hmm, actually I believe v4l2_ctrl_handler_setup() will call .s_ctrl() > here and set the motor value again. If we just make .s_ctrl() do the > adjustment in steps properly, we can simplify the resume to simply > call v4l2_ctrl_handler_setup() alone. Or drop the v4l2_ctrl_handler_setup() here. The reason is that the driver uses direct drive method for the lens and is thus responsible for managing ringing compensation as well. Ringing compensation support could be added to the driver later on; I think another control will be needed to control the mode. > > > +} > > #endif > > #ifdef CONFIG_PM > > > + > > +static int dw9714_runtime_suspend(struct device *dev) > > +{ > > + return dw9714_vcm_suspend(dev); > > +} > > + > > +static int dw9714_runtime_resume(struct device *dev) > > +{ > > + return dw9714_vcm_resume(dev); > > +} > > #endif > > #ifdef CONFIG_PM_SLEEP It's hard to get these right, and in 99 % of the cases you'll have them anyway. __maybe_unused is quite useful in such cases.
Hi Sakari, On Thu, May 11, 2017 at 3:55 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: > Hi Tomasz, > > On Thu, May 11, 2017 at 02:30:31PM +0800, Tomasz Figa wrote: > ... >> > + >> > +/* >> > + * 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 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 = container_of(sd, >> > + struct dw9714_device, >> > + 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((u16) val, DW9714_DEFAULT_S)); >> >> DW9714_VAL() already contains such cast. Anyway, I still think they >> don't really give us anything and should be removed. >> >> > + if (ret) >> > + dev_err(dev, "%s I2C failure: %d", __func__, ret); >> >> I think we should just return an error code here and fail the suspend. > > The result from an error here is that the user would hear an audible click. > I don't think it's worth failing system suspend. :-) > Hmm, the result of an error here would be higher power consumption in suspend (unless there is also some other mechanism that actually cuts the power). Moreover, if an error here happens it would rather mean that there is something wrong with hardware itself (or I2C driver) and not bailing out here would make it easier to let the error go unnoticed. > But as no action is taken based on the error code, there could be quite a > few of these messages. How about dev_err_once()? For resume I might use > dev_err_ratelimited(). > >> >> > + 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 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 = container_of(sd, >> > + struct dw9714_device, >> > + 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((u16) val, DW9714_DEFAULT_S)); >> >> Ditto. >> >> > + if (ret) >> > + dev_err(dev, "%s I2C failure: %d", __func__, ret); >> >> Ditto. >> >> > + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10); >> > + } >> > + >> > + /* restore v4l2 control values */ >> > + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm); >> > + return ret; >> >> Hmm, actually I believe v4l2_ctrl_handler_setup() will call .s_ctrl() >> here and set the motor value again. If we just make .s_ctrl() do the >> adjustment in steps properly, we can simplify the resume to simply >> call v4l2_ctrl_handler_setup() alone. > > Or drop the v4l2_ctrl_handler_setup() here. > > The reason is that the driver uses direct drive method for the lens and is > thus responsible for managing ringing compensation as well. Ringing > compensation support could be added to the driver later on; I think another > control will be needed to control the mode. Given that we already have some kind of ringing compensation in suspend and resume, can't we just reuse this in control handler? On the other hand, if there is really no hard reason to do the compensation in control handler case then maybe there is no reason to do that for suspend/resume in current code as well (it would be added when the control handler gains support for it)? > >> >> > +} >> >> #endif >> >> #ifdef CONFIG_PM >> >> > + >> > +static int dw9714_runtime_suspend(struct device *dev) >> > +{ >> > + return dw9714_vcm_suspend(dev); >> > +} >> > + >> > +static int dw9714_runtime_resume(struct device *dev) >> > +{ >> > + return dw9714_vcm_resume(dev); >> > +} >> >> #endif >> >> #ifdef CONFIG_PM_SLEEP > > It's hard to get these right, and in 99 % of the cases you'll have them > anyway. __maybe_unused is quite useful in such cases. Right, I forgot about it. Thanks for this useful suggestion. Best regards, Tomasz
Hi Tomasz, On Thu, May 11, 2017 at 04:02:35PM +0800, Tomasz Figa wrote: > Hi Sakari, > > On Thu, May 11, 2017 at 3:55 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > Hi Tomasz, > > > > On Thu, May 11, 2017 at 02:30:31PM +0800, Tomasz Figa wrote: > > ... > >> > + > >> > +/* > >> > + * 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 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 = container_of(sd, > >> > + struct dw9714_device, > >> > + 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((u16) val, DW9714_DEFAULT_S)); > >> > >> DW9714_VAL() already contains such cast. Anyway, I still think they > >> don't really give us anything and should be removed. > >> > >> > + if (ret) > >> > + dev_err(dev, "%s I2C failure: %d", __func__, ret); > >> > >> I think we should just return an error code here and fail the suspend. > > > > The result from an error here is that the user would hear an audible click. > > I don't think it's worth failing system suspend. :-) > > > > Hmm, the result of an error here would be higher power consumption in > suspend (unless there is also some other mechanism that actually cuts > the power). Moreover, if an error here happens it would rather mean On systems where power consumption matters the device would presumably be powered off (e.g. by ACPI). > that there is something wrong with hardware itself (or I2C driver) and > not bailing out here would make it easier to let the error go > unnoticed. I'm not sure an error here matters much. :-) The same function is also used as runtime_suspend(), in which case the result won't be cared much about to begin with. ... > >> > +/* > >> > + * 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 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 = container_of(sd, > >> > + struct dw9714_device, > >> > + 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((u16) val, DW9714_DEFAULT_S)); > >> > >> Ditto. > >> > >> > + if (ret) > >> > + dev_err(dev, "%s I2C failure: %d", __func__, ret); > >> > >> Ditto. > >> > >> > + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10); > >> > + } > >> > + > >> > + /* restore v4l2 control values */ > >> > + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm); > >> > + return ret; > >> > >> Hmm, actually I believe v4l2_ctrl_handler_setup() will call .s_ctrl() > >> here and set the motor value again. If we just make .s_ctrl() do the > >> adjustment in steps properly, we can simplify the resume to simply > >> call v4l2_ctrl_handler_setup() alone. > > > > Or drop the v4l2_ctrl_handler_setup() here. > > > > The reason is that the driver uses direct drive method for the lens and is > > thus responsible for managing ringing compensation as well. Ringing > > compensation support could be added to the driver later on; I think another > > control will be needed to control the mode. > > Given that we already have some kind of ringing compensation in > suspend and resume, can't we just reuse this in control handler? On The way it's done here is unlikely to be helpful for the user space that needs to drive the lens to a new position as fast as possible. The code above is presumably enough to prevent the lens from hitting the mechanical stopper but I'd equally expect it to interfere badly with the user space trying to control the lens.
On Thu, May 11, 2017 at 4:24 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: > Hi Tomasz, > > On Thu, May 11, 2017 at 04:02:35PM +0800, Tomasz Figa wrote: >> Hi Sakari, >> >> On Thu, May 11, 2017 at 3:55 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: >> > Hi Tomasz, >> > >> > On Thu, May 11, 2017 at 02:30:31PM +0800, Tomasz Figa wrote: >> > ... >> >> > + >> >> > +/* >> >> > + * 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 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 = container_of(sd, >> >> > + struct dw9714_device, >> >> > + 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((u16) val, DW9714_DEFAULT_S)); >> >> >> >> DW9714_VAL() already contains such cast. Anyway, I still think they >> >> don't really give us anything and should be removed. >> >> >> >> > + if (ret) >> >> > + dev_err(dev, "%s I2C failure: %d", __func__, ret); >> >> >> >> I think we should just return an error code here and fail the suspend. >> > >> > The result from an error here is that the user would hear an audible click. >> > I don't think it's worth failing system suspend. :-) >> > >> >> Hmm, the result of an error here would be higher power consumption in >> suspend (unless there is also some other mechanism that actually cuts >> the power). Moreover, if an error here happens it would rather mean > > On systems where power consumption matters the device would presumably be > powered off (e.g. by ACPI). > >> that there is something wrong with hardware itself (or I2C driver) and >> not bailing out here would make it easier to let the error go >> unnoticed. > > I'm not sure an error here matters much. :-) > > The same function is also used as runtime_suspend(), in which case the > result won't be cared much about to begin with. After thinking a bit more, I realized that the error would be actually noticeable in terms of the camera not focusing properly, so maybe there is no reason to care too much here indeed. > > ... > >> >> > +/* >> >> > + * 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 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 = container_of(sd, >> >> > + struct dw9714_device, >> >> > + 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((u16) val, DW9714_DEFAULT_S)); >> >> >> >> Ditto. >> >> >> >> > + if (ret) >> >> > + dev_err(dev, "%s I2C failure: %d", __func__, ret); >> >> >> >> Ditto. >> >> >> >> > + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10); >> >> > + } >> >> > + >> >> > + /* restore v4l2 control values */ >> >> > + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm); >> >> > + return ret; >> >> >> >> Hmm, actually I believe v4l2_ctrl_handler_setup() will call .s_ctrl() >> >> here and set the motor value again. If we just make .s_ctrl() do the >> >> adjustment in steps properly, we can simplify the resume to simply >> >> call v4l2_ctrl_handler_setup() alone. >> > >> > Or drop the v4l2_ctrl_handler_setup() here. >> > >> > The reason is that the driver uses direct drive method for the lens and is >> > thus responsible for managing ringing compensation as well. Ringing >> > compensation support could be added to the driver later on; I think another >> > control will be needed to control the mode. >> >> Given that we already have some kind of ringing compensation in >> suspend and resume, can't we just reuse this in control handler? On > > The way it's done here is unlikely to be helpful for the user space that > needs to drive the lens to a new position as fast as possible. The code > above is presumably enough to prevent the lens from hitting the mechanical > stopper but I'd equally expect it to interfere badly with the user space > trying to control the lens. Okay, fair enough. I assume then it's not unsafe for the hardware to allow userspace to control it over the full range and the worst thing that can happen is getting some sound effects? (Rather than some malicious userspace burning the motor or so. Best regards, Tomasz
Hi Tomasz, On Thu, May 11, 2017 at 04:37:08PM +0800, Tomasz Figa wrote: > >> >> > +/* > >> >> > + * 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 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 = container_of(sd, > >> >> > + struct dw9714_device, > >> >> > + 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((u16) val, DW9714_DEFAULT_S)); > >> >> > >> >> Ditto. > >> >> > >> >> > + if (ret) > >> >> > + dev_err(dev, "%s I2C failure: %d", __func__, ret); > >> >> > >> >> Ditto. > >> >> > >> >> > + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10); > >> >> > + } > >> >> > + > >> >> > + /* restore v4l2 control values */ > >> >> > + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm); > >> >> > + return ret; > >> >> > >> >> Hmm, actually I believe v4l2_ctrl_handler_setup() will call .s_ctrl() > >> >> here and set the motor value again. If we just make .s_ctrl() do the > >> >> adjustment in steps properly, we can simplify the resume to simply > >> >> call v4l2_ctrl_handler_setup() alone. > >> > > >> > Or drop the v4l2_ctrl_handler_setup() here. > >> > > >> > The reason is that the driver uses direct drive method for the lens and is > >> > thus responsible for managing ringing compensation as well. Ringing > >> > compensation support could be added to the driver later on; I think another > >> > control will be needed to control the mode. > >> > >> Given that we already have some kind of ringing compensation in > >> suspend and resume, can't we just reuse this in control handler? On > > > > The way it's done here is unlikely to be helpful for the user space that > > needs to drive the lens to a new position as fast as possible. The code > > above is presumably enough to prevent the lens from hitting the mechanical > > stopper but I'd equally expect it to interfere badly with the user space > > trying to control the lens. > > Okay, fair enough. I assume then it's not unsafe for the hardware to > allow userspace to control it over the full range and the worst thing > that can happen is getting some sound effects? (Rather than some > malicious userspace burning the motor or so. Correct. It's a coil, and the dw9714 chip controls the current to the coil. The higher is the current, the greater is the force that deviating the lens from its resting position.
Hi, On 05/11/2017 08:30 AM, Tomasz Figa wrote: >> +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 (rval >= 0) >> + return 0; >> + >> + pm_runtime_put(dev); >> + return rval; >> > nit: The typical coding style is to return early in case of a special > case and keep the common path linear, i.e. > > rval = pm_runtime_get_sync(dev); > if (rval < 0) { > pm_runtime_put(dev); > return rval; > } Aren't we supposed to call pm_runtime_put() only when corresponding pm_runtime_get() succeeds? I think the pm_runtime_put() call above is not needed. -- Regards, Sylwester
On Thu, May 11, 2017 at 04:39:41PM +0200, Sylwester Nawrocki wrote: > Hi, > > On 05/11/2017 08:30 AM, Tomasz Figa wrote: > >> +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 (rval >= 0) > >> + return 0; > >> + > >> + pm_runtime_put(dev); > >> + return rval; > >> > > nit: The typical coding style is to return early in case of a special > > case and keep the common path linear, i.e. > > > > rval = pm_runtime_get_sync(dev); > > if (rval < 0) { > > pm_runtime_put(dev); > > return rval; > > } > > Aren't we supposed to call pm_runtime_put() only when corresponding > pm_runtime_get() succeeds? I think the pm_runtime_put() call above > is not needed. pm_runtime_get() increments the usage_count independently of whether it succeeded. See __pm_runtime_resume().
On 05/11/2017 04:59 PM, Sakari Ailus wrote: >> On 05/11/2017 08:30 AM, Tomasz Figa wrote: [...] >>> rval = pm_runtime_get_sync(dev); >>> if (rval < 0) { >>> pm_runtime_put(dev); >>> return rval; >>> } >> Aren't we supposed to call pm_runtime_put() only when corresponding >> pm_runtime_get() succeeds? I think the pm_runtime_put() call above >> is not needed. > > pm_runtime_get() increments the usage_count independently of whether it > succeeded. See __pm_runtime_resume(). You're right, sorry. I'd expect such things to be better covered in the API documentation. Probably pm_runtime_put_noidle() is a better match for just decreasing usage_count. Now many drivers appear to not be balancing usage_count when when pm_runtime_get_sync() fails.
Hi Sylwester, On Fri, May 12, 2017 at 10:57:39AM +0200, Sylwester Nawrocki wrote: > On 05/11/2017 04:59 PM, Sakari Ailus wrote: > >>On 05/11/2017 08:30 AM, Tomasz Figa wrote: > [...] > >>> rval = pm_runtime_get_sync(dev); > >>> if (rval < 0) { > >>> pm_runtime_put(dev); > >>> return rval; > >>> } > >>Aren't we supposed to call pm_runtime_put() only when corresponding > >>pm_runtime_get() succeeds? I think the pm_runtime_put() call above > >>is not needed. > > > >pm_runtime_get() increments the usage_count independently of whether it > >succeeded. See __pm_runtime_resume(). > > You're right, sorry. I'd expect such things to be better covered in > the API documentation. Probably pm_runtime_put_noidle() is a better Well, the documentation tells what the function does. It'd be good if it pointed that the usage count needs to be decremented if the function fails. I guess the reason is that it's just a synchronous variant of pm_runtime_get(), which could not handle the error anyway. > match for just decreasing usage_count. Now many drivers appear to not > be balancing usage_count when when pm_runtime_get_sync() fails. Ah, quite a few drivers seem to be using pm_runtime_put_noidle() which seems to be the correct thing to do as the device won't be on then anyway.
Hi Raj, On Thu, May 11, 2017 at 3:30 PM, Tomasz Figa <tfiga@chromium.org> wrote: > Hi Raj, > > Thanks for re-spin. Still a bit more comments inline. (I missed few > more before, sorry.) > > On Thu, May 11, 2017 at 1:00 PM, Rajmohan Mani <rajmohan.mani@intel.com> wrote: >> DW9714 is a 10 bit DAC, designed for linear >> control of voice coil motor. > [snip] >> +static int dw9714_i2c_write(struct i2c_client *client, u16 data) >> +{ >> + int ret; >> + u16 val = cpu_to_be16(data); >> + const int num_bytes = sizeof(val); >> + >> + ret = i2c_master_send(client, (const char *) &val, sizeof(val)); > > nit: No need for space between cast and casted value. > >> + >> + /*One retry */ >> + if (ret != num_bytes) >> + ret = i2c_master_send(client, (const char *) &val, sizeof(val)); > > Why do we need this retry? > >> + >> + if (ret != num_bytes) { >> + 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)); > > This still doesn't seem to apply the control gradually as suspend and resume do. > >> +} > [snip] >> +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 (rval >= 0) >> + return 0; >> + >> + pm_runtime_put(dev); >> + return rval; > > nit: The typical coding style is to return early in case of a special > case and keep the common path linear, i.e. > > rval = pm_runtime_get_sync(dev); > if (rval < 0) { > pm_runtime_put(dev); > return rval; > } > > return 0; > >> +} >> + > [snip] >> +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 > > #if defined(CONFIG_PM) || defined(CONFIG_PM_SLEEP) > >> + >> +/* >> + * 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 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 = container_of(sd, >> + struct dw9714_device, >> + 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((u16) val, DW9714_DEFAULT_S)); > > DW9714_VAL() already contains such cast. Anyway, I still think they > don't really give us anything and should be removed. > >> + if (ret) >> + dev_err(dev, "%s I2C failure: %d", __func__, ret); > > I think we should just return an error code here and fail the suspend. > >> + 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 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 = container_of(sd, >> + struct dw9714_device, >> + 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((u16) val, DW9714_DEFAULT_S)); > > Ditto. > >> + if (ret) >> + dev_err(dev, "%s I2C failure: %d", __func__, ret); > > Ditto. > >> + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10); >> + } >> + >> + /* restore v4l2 control values */ >> + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm); >> + return ret; > > Hmm, actually I believe v4l2_ctrl_handler_setup() will call .s_ctrl() > here and set the motor value again. If we just make .s_ctrl() do the > adjustment in steps properly, we can simplify the resume to simply > call v4l2_ctrl_handler_setup() alone. > >> +} > > #endif > > #ifdef CONFIG_PM > >> + >> +static int dw9714_runtime_suspend(struct device *dev) >> +{ >> + return dw9714_vcm_suspend(dev); >> +} >> + >> +static int dw9714_runtime_resume(struct device *dev) >> +{ >> + return dw9714_vcm_resume(dev); >> +} > > #endif > > #ifdef CONFIG_PM_SLEEP > >> + >> +static int dw9714_suspend(struct device *dev) >> +{ >> + return dw9714_vcm_suspend(dev); >> +} >> + >> +static int dw9714_resume(struct device *dev) >> +{ >> + return dw9714_vcm_resume(dev); >> +} > > #endif > > Or you could actually just use dw9714_vcm_{suspend,resume}() directly > for the callbacks and avoid the duplicates above. > >> + >> +#else >> + >> +#define dw9714_vcm_suspend NULL >> +#define dw9714_vcm_resume NULL > > This #else block is not needed. > Any updates on this and other comments? Best regards, Tomasz
Hi Tomasz, Thanks for the reviews. I have consolidated my responses to comments from all of you below and are addressed in v5 of this patch. > -----Original Message----- > From: Tomasz Figa [mailto:tfiga@chromium.org] > Sent: Wednesday, May 10, 2017 11:31 PM > To: Mani, Rajmohan <rajmohan.mani@intel.com> > Cc: linux-media@vger.kernel.org; mchehab@kernel.org; Hans Verkuil > <hverkuil@xs4all.nl>; Sakari Ailus <sakari.ailus@iki.fi> > Subject: Re: [PATCH v4] dw9714: Initial driver for dw9714 VCM > > Hi Raj, > > Thanks for re-spin. Still a bit more comments inline. (I missed few more before, > sorry.) > > On Thu, May 11, 2017 at 1:00 PM, Rajmohan Mani > <rajmohan.mani@intel.com> wrote: > > DW9714 is a 10 bit DAC, designed for linear control of voice coil > > motor. > [snip] > > +static int dw9714_i2c_write(struct i2c_client *client, u16 data) { > > + int ret; > > + u16 val = cpu_to_be16(data); > > + const int num_bytes = sizeof(val); > > + > > + ret = i2c_master_send(client, (const char *) &val, > > + sizeof(val)); > > nit: No need for space between cast and casted value. > Ack > > + > > + /*One retry */ > > + if (ret != num_bytes) > > + ret = i2c_master_send(client, (const char *) &val, > > + sizeof(val)); > > Why do we need this retry? > This was found to be useful in the early bring up days of this vcm. I think this can be removed now. > > + > > + if (ret != num_bytes) { > > + 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)); > > This still doesn't seem to apply the control gradually as suspend and resume do. > Ack As was discussed with Sakari on the follow up threads, we can leave it as it is here, so the user space can control this as needed. > > +} > [snip] > > +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 (rval >= 0) > > + return 0; > > + > > + pm_runtime_put(dev); > > + return rval; > > nit: The typical coding style is to return early in case of a special case and keep > the common path linear, i.e. > Ack > rval = pm_runtime_get_sync(dev); > if (rval < 0) { > pm_runtime_put(dev); > return rval; > } > > return 0; > > > +} > > + > [snip] > > +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 > > #if defined(CONFIG_PM) || defined(CONFIG_PM_SLEEP) > Ack. __maybe_unused is added for vcm_*suspend/resume functions > > + > > +/* > > + * 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 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 = container_of(sd, > > + struct dw9714_device, > > + 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((u16) val, > > + DW9714_DEFAULT_S)); > > DW9714_VAL() already contains such cast. Anyway, I still think they don't really > give us anything and should be removed. > Ack > > + if (ret) > > + dev_err(dev, "%s I2C failure: %d", __func__, > > + ret); > > I think we should just return an error code here and fail the suspend. > As discussed, we are going to ignore this error and not let it affect system suspend. > > + 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 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 = container_of(sd, > > + struct dw9714_device, > > + 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((u16) val, > > + DW9714_DEFAULT_S)); > > Ditto. > > > + if (ret) > > + dev_err(dev, "%s I2C failure: %d", __func__, > > + ret); > > Ditto. > > > + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US > + 10); > > + } > > + > > + /* restore v4l2 control values */ > > + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm); > > + return ret; > > Hmm, actually I believe v4l2_ctrl_handler_setup() will call .s_ctrl() here and set > the motor value again. If we just make .s_ctrl() do the adjustment in steps > properly, we can simplify the resume to simply call v4l2_ctrl_handler_setup() > alone. > As Sakari suggested, v4l2_ctrl_handler_setup() code is removed, as the preceding code should restore the vcm position prior to suspend > > +} > > #endif > > #ifdef CONFIG_PM > > > + > > +static int dw9714_runtime_suspend(struct device *dev) { > > + return dw9714_vcm_suspend(dev); } > > + > > +static int dw9714_runtime_resume(struct device *dev) { > > + return dw9714_vcm_resume(dev); } > > #endif > > #ifdef CONFIG_PM_SLEEP > > > + > > +static int dw9714_suspend(struct device *dev) { > > + return dw9714_vcm_suspend(dev); } > > + > > +static int dw9714_resume(struct device *dev) { > > + return dw9714_vcm_resume(dev); } > > #endif > > Or you could actually just use dw9714_vcm_{suspend,resume}() directly for the > callbacks and avoid the duplicates above. > Ack > > + > > +#else > > + > > +#define dw9714_vcm_suspend NULL > > +#define dw9714_vcm_resume NULL > > This #else block is not needed. > Ack > Best regards, > Tomasz
Hi Sakari, > -----Original Message----- > From: Sakari Ailus [mailto:sakari.ailus@iki.fi] > Sent: Thursday, May 11, 2017 12:41 AM > To: Mani, Rajmohan <rajmohan.mani@intel.com> > Cc: linux-media@vger.kernel.org; mchehab@kernel.org; hverkuil@xs4all.nl; > tfiga@chromium.org > Subject: Re: [PATCH v4] dw9714: Initial driver for dw9714 VCM > > Hi Rajmohan, > > A few minor comments below. > > On Wed, May 10, 2017 at 10:00:20PM -0700, Rajmohan Mani wrote: > > 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> > > --- > > Changes in v4: > > - Addressed review comments from Tomasz Changes in v3: > > - Addressed most of the review comments from Sakari > > on v1 of this patch > > Changes in v2: > > - Addressed review comments from Hans Verkuil > > - Fixed a debug message typo > > - Got rid of a return variable > > --- > > drivers/media/i2c/Kconfig | 9 ++ > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/dw9714.c | 332 > > +++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 342 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..fefd5d2 > > --- /dev/null > > +++ b/drivers/media/i2c/dw9714.c > > @@ -0,0 +1,332 @@ > > +/* > > + * 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) (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; > > +}; > > + > > +static inline struct dw9714_device *to_dw9714_vcm(struct v4l2_ctrl > > +*ctrl) { > > + return container_of(ctrl->handler, struct dw9714_device, ctrls_vcm); > > +} > > + > > +static int dw9714_i2c_write(struct i2c_client *client, u16 data) { > > + int ret; > > + u16 val = cpu_to_be16(data); > > + const int num_bytes = sizeof(val); > > + > > + ret = i2c_master_send(client, (const char *) &val, sizeof(val)); > > + > > + /*One retry */ > > + if (ret != num_bytes) > > + ret = i2c_master_send(client, (const char *) &val, sizeof(val)); > > + > > + if (ret != num_bytes) { > > Use sizeof(val) here and three linees above and you can remove num_bytes. > Ack > > + 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 = container_of(sd, > > + struct > dw9714_device, > > + sd); > > + struct device *dev = &dw9714_dev->client->dev; > > + int rval; > > + > > + rval = pm_runtime_get_sync(dev); > > + if (rval >= 0) > > + return 0; > > + > > + pm_runtime_put(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; > > + > > + 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 = container_of(sd, > > + struct > dw9714_device, > > + sd); > > + > > + pm_runtime_disable(&client->dev); > > + dw9714_subdev_cleanup(dw9714_dev); > > + > > + return 0; > > +} > > + > > +#ifdef CONFIG_PM > > + > > +/* > > + * 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 dw9714_vcm_suspend(struct device *dev) > > static int __maybe_unused > > > +{ > > + 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); > > Could you add an inline function for converting v4l2_subdev to > dw9714_device? It's done in quite a few places. > Ack > > + 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((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 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 dw9714_vcm_resume(struct device *dev) > > static int __maybe_unused > Ack > > +{ > > + 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; > > + > > + 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); > > + return ret; > > +} > > + > > +static int dw9714_runtime_suspend(struct device *dev) { > > + return dw9714_vcm_suspend(dev); > > You can remove the four callbacks and use dw9714_vcm_{suspend,resume} > directly. > Ack > > +} > > + > > +static int dw9714_runtime_resume(struct device *dev) { > > + return dw9714_vcm_resume(dev); > > +} > > + > > +static int dw9714_suspend(struct device *dev) { > > + return dw9714_vcm_suspend(dev); > > +} > > + > > +static int dw9714_resume(struct device *dev) { > > + return dw9714_vcm_resume(dev); > > +} > > + > > +#else > > And please remove the #ifdefs. They're not needed here. > Ack > > + > > +#define dw9714_vcm_suspend NULL > > +#define dw9714_vcm_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 = { > > + SET_SYSTEM_SLEEP_PM_OPS(dw9714_suspend, dw9714_resume) > > + SET_RUNTIME_PM_OPS(dw9714_runtime_suspend, > dw9714_runtime_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"); > > -- > Regards, > > Sakari Ailus > e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
Hi Sakari, Sylwester, > > > > You're right, sorry. I'd expect such things to be better covered in > > the API documentation. Probably pm_runtime_put_noidle() is a better > > Well, the documentation tells what the function does. It'd be good if it pointed > that the usage count needs to be decremented if the function fails. > > I guess the reason is that it's just a synchronous variant of pm_runtime_get(), > which could not handle the error anyway. > > > match for just decreasing usage_count. Now many drivers appear to not > > be balancing usage_count when when pm_runtime_get_sync() fails. > > Ah, quite a few drivers seem to be using pm_runtime_put_noidle() which seems > to be the correct thing to do as the device won't be on then anyway. > Ack > -- > Regards, > > Sakari Ailus > e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
> > > + if (ret) > > > + dev_err(dev, "%s I2C failure: %d", __func__, > > > + ret); > > > > I think we should just return an error code here and fail the suspend. > > The result from an error here is that the user would hear an audible click. > I don't think it's worth failing system suspend. :-) > > But as no action is taken based on the error code, there could be quite a few of > these messages. How about dev_err_once()? For resume I might use > dev_err_ratelimited(). > Ack (addressed with v5 of this patch)
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..fefd5d2 --- /dev/null +++ b/drivers/media/i2c/dw9714.c @@ -0,0 +1,332 @@ +/* + * 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) (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; +}; + +static inline struct dw9714_device *to_dw9714_vcm(struct v4l2_ctrl *ctrl) +{ + return container_of(ctrl->handler, struct dw9714_device, ctrls_vcm); +} + +static int dw9714_i2c_write(struct i2c_client *client, u16 data) +{ + int ret; + u16 val = cpu_to_be16(data); + const int num_bytes = sizeof(val); + + ret = i2c_master_send(client, (const char *) &val, sizeof(val)); + + /*One retry */ + if (ret != num_bytes) + ret = i2c_master_send(client, (const char *) &val, sizeof(val)); + + if (ret != num_bytes) { + 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 = container_of(sd, + struct dw9714_device, + sd); + struct device *dev = &dw9714_dev->client->dev; + int rval; + + rval = pm_runtime_get_sync(dev); + if (rval >= 0) + return 0; + + pm_runtime_put(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; + + 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 = container_of(sd, + struct dw9714_device, + sd); + + pm_runtime_disable(&client->dev); + dw9714_subdev_cleanup(dw9714_dev); + + return 0; +} + +#ifdef CONFIG_PM + +/* + * 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 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 = container_of(sd, + struct dw9714_device, + 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((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 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 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 = container_of(sd, + struct dw9714_device, + 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((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); + return ret; +} + +static int dw9714_runtime_suspend(struct device *dev) +{ + return dw9714_vcm_suspend(dev); +} + +static int dw9714_runtime_resume(struct device *dev) +{ + return dw9714_vcm_resume(dev); +} + +static int dw9714_suspend(struct device *dev) +{ + return dw9714_vcm_suspend(dev); +} + +static int dw9714_resume(struct device *dev) +{ + return dw9714_vcm_resume(dev); +} + +#else + +#define dw9714_vcm_suspend NULL +#define dw9714_vcm_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 = { + SET_SYSTEM_SLEEP_PM_OPS(dw9714_suspend, dw9714_resume) + SET_RUNTIME_PM_OPS(dw9714_runtime_suspend, dw9714_runtime_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");
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> --- Changes in v4: - Addressed review comments from Tomasz Changes in v3: - Addressed most of the review comments from Sakari on v1 of this patch Changes in v2: - Addressed review comments from Hans Verkuil - Fixed a debug message typo - Got rid of a return variable --- drivers/media/i2c/Kconfig | 9 ++ drivers/media/i2c/Makefile | 1 + drivers/media/i2c/dw9714.c | 332 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 342 insertions(+) create mode 100644 drivers/media/i2c/dw9714.c