Hi Bingbu,
> -----Original Message-----
> From: Bing Bu Cao [mailto:bingbu....@linux.intel.com]
> Sent: Monday, June 04, 2018 11:27 PM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>; Cao, Bingbu
> <bingbu....@intel.com>; linux-media@vger.kernel.org
> Cc: sakari.ai...@linux.intel.com; tf...@google.com; jac...@jmondi.org; Qiu,
> Tian Shu <tian.shu....@intel.com>; Zheng, Jian Xu <jian.xu.zh...@intel.com>
> Subject: Re: [PATCH v3 2/2] media: ak7375: Add ak7375 lens voice coil driver
>
>
>
> On 20180605 08:23, Mani, Rajmohan wrote:
> > Hi Bingbu,
> >
> > Please see a couple of comments below.
> >
> >> -----Original Message-----
> >> From: Cao, Bingbu
> >> Sent: Monday, June 04, 2018 2:00 AM
> >> To: linux-media@vger.kernel.org
> >> Cc: sakari.ai...@linux.intel.com; tf...@google.com;
> >> jac...@jmondi.org; Mani, Rajmohan <rajmohan.m...@intel.com>;
> >> bingbu....@linux.intel.com; Qiu, Tian Shu <tian.shu....@intel.com>;
> >> Zheng, Jian Xu <jian.xu.zh...@intel.com>
> >> Subject: [PATCH v3 2/2] media: ak7375: Add ak7375 lens voice coil
> >> driver
> >>
> >> From: Bingbu Cao <bingbu....@intel.com>
> >>
> >> Add a v4l2 sub-device driver for the ak7375 lens voice coil.
> >> This is a voice coil module using the i2c bus to control the focus
> >> position.
> >>
> >> ak7375 can write multiple bytes of data at a time. If more data is
> >> received instead of the stop condition after receiving one byte of
> >> data, the address inside the chip is automatically incremented and
> >> the data is written into the next address.
> >>
> >> The ak7375 can control the position with 12 bits value and consists
> >> of two 8 bit registers show as below:
> >> register 0x00(AK7375_REG_POSITION):
> >> +---+---+---+---+---+---+---+---+
> >> |D11|D10|D09|D08|D07|D06|D05|D04|
> >> +---+---+---+---+---+---+---+---+ register 0x01:
> >> +---+---+---+---+---+---+---+---+
> >> |D03|D02|D01|D00|---|---|---|---|
> >> +---+---+---+---+---+---+---+---+
> >>
> >> This driver support :
> >> - set ak7375 to standby mode once suspend and
> >> turn it back to active if resume
> >> - set the position via V4L2_CID_FOCUS_ABSOLUTE ctrl
> >>
> >> Signed-off-by: Tianshu Qiu <tian.shu....@intel.com>
> >> Signed-off-by: Bingbu Cao <bingbu....@intel.com>
> >>
> >> ---
> >> Changes from v1:
> >> - correct i2c write
> >> - add media_entity_pads_init() into probe
> >> - move the MAINTAINERs change into dt-bindings change
> >> - correct the compatible string
> >> ---
> >> ---
> >> drivers/media/i2c/Kconfig | 10 ++
> >> drivers/media/i2c/Makefile | 1 +
> >> drivers/media/i2c/ak7375.c | 278
> >> +++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 289 insertions(+)
> >> create mode 100644 drivers/media/i2c/ak7375.c
> >>
> >> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> >> index
> >> 341452fe98df..ff3cb5afb0e1 100644
> >> --- a/drivers/media/i2c/Kconfig
> >> +++ b/drivers/media/i2c/Kconfig
> >> @@ -326,6 +326,16 @@ 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_AK7375
> >> + tristate "AK7375 lens voice coil support"
> >> + depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
> >> + depends on VIDEO_V4L2_SUBDEV_API
> >> + help
> >> + This is a driver for the AK7375 camera lens voice coil.
> >> + AK7375 is a 12 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_DW9714
> >> tristate "DW9714 lens voice coil support"
> >> depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER diff --git
> >> a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index
> >> d679d57cd3b3..05b97e319ea9 100644
> >> --- a/drivers/media/i2c/Makefile
> >> +++ b/drivers/media/i2c/Makefile
> >> @@ -23,6 +23,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_AK7375) += ak7375.o
> >> obj-$(CONFIG_VIDEO_DW9714) += dw9714.o
> >> obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
> >> obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o diff --git
> >> a/drivers/media/i2c/ak7375.c b/drivers/media/i2c/ak7375.c new file
> >> mode
> >> 100644 index 000000000000..94bcadae4258
> >> --- /dev/null
> >> +++ b/drivers/media/i2c/ak7375.c
> >> @@ -0,0 +1,278 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +// Copyright (C) 2018 Intel Corporation
> >> +
> >> +#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 AK7375_MAX_FOCUS_POS 4095
> >> +/*
> >> + * This sets the minimum granularity for the focus positions.
> >> + * A value of 1 gives maximum accuracy for a desired focus position */
> >> +#define AK7375_FOCUS_STEPS 1
> >> +/*
> >> + * 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 AK7375_CTRL_STEPS 64
> >> +#define AK7375_CTRL_DELAY_US 1000
> >> +
> >> +#define AK7375_REG_POSITION 0x0
> >> +#define AK7375_REG_CONT 0x2
> >> +#define AK7375_MODE_ACTIVE 0x0
> >> +#define AK7375_MODE_STANDBY 0x40
> >> +
> >> +/* ak7375 device structure */
> >> +struct ak7375_device {
> >> + struct v4l2_ctrl_handler ctrls_vcm;
> >> + struct v4l2_subdev sd;
> >> + struct v4l2_ctrl *focus;
> >> +};
> >> +
> >> +static inline struct ak7375_device *to_ak7375_vcm(struct v4l2_ctrl
> >> +*ctrl) {
> >> + return container_of(ctrl->handler, struct ak7375_device,
> >> +ctrls_vcm); }
> >> +
> >> +static inline struct ak7375_device *sd_to_ak7375_vcm(struct
> >> +v4l2_subdev
> >> +*subdev) {
> >> + return container_of(subdev, struct ak7375_device, sd); }
> >> +
> >> +static int ak7375_i2c_write(struct ak7375_device *ak7375,
> >> + u8 addr, u16 data, int size)
> >> +{
> >> + struct i2c_client *client = v4l2_get_subdevdata(&ak7375->sd);
> >> + int ret;
> >> + u8 buf[3];
> >> +
> >> + if (size != 1 && size != 2)
> >> + return -EINVAL;
> >> + buf[0] = addr;
> >> + buf[size] = data & 0xff;
> >> + if (size == 2)
> >> + buf[1] = (data >> 8) & 0xff;
> >> + ret = i2c_master_send(client, (const char *)buf, size + 1);
> >> + if (ret < 0)
> >> + return ret;
> >> + if (ret != size + 1)
> >> + return -EIO;
> >> + return 0;
> >> +}
> >> +
> >> +static int ak7375_set_ctrl(struct v4l2_ctrl *ctrl) {
> >> + struct ak7375_device *dev_vcm = to_ak7375_vcm(ctrl);
> >> +
> >> + if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE)
> >> + return ak7375_i2c_write(dev_vcm, AK7375_REG_POSITION,
> >> + ctrl->val << 4, 2);
> >> +
> >> + return -EINVAL;
> >> +}
> >> +
> >> +static const struct v4l2_ctrl_ops ak7375_vcm_ctrl_ops = {
> >> + .s_ctrl = ak7375_set_ctrl,
> >> +};
> >> +
> >> +static int ak7375_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> >> +*fh) {
> >> + int rval;
> >> +
> >> + rval = pm_runtime_get_sync(sd->dev);
> >> + if (rval < 0) {
> >> + pm_runtime_put_noidle(sd->dev);
> >> + return rval;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int ak7375_close(struct v4l2_subdev *sd, struct
> >> +v4l2_subdev_fh
> >> +*fh) {
> >> + pm_runtime_put(sd->dev);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static const struct v4l2_subdev_internal_ops ak7375_int_ops = {
> >> + .open = ak7375_open,
> >> + .close = ak7375_close,
> >> +};
> >> +
> >> +static const struct v4l2_subdev_ops ak7375_ops = { };
> >> +
> >> +static void ak7375_subdev_cleanup(struct ak7375_device *ak7375_dev) {
> >> + v4l2_async_unregister_subdev(&ak7375_dev->sd);
> >> + v4l2_ctrl_handler_free(&ak7375_dev->ctrls_vcm);
> >> + media_entity_cleanup(&ak7375_dev->sd.entity);
> >> +}
> >> +
> >> +static int ak7375_init_controls(struct ak7375_device *dev_vcm) {
> >> + struct v4l2_ctrl_handler *hdl = &dev_vcm->ctrls_vcm;
> >> + const struct v4l2_ctrl_ops *ops = &ak7375_vcm_ctrl_ops;
> >> +
> >> + v4l2_ctrl_handler_init(hdl, 1);
> >> +
> >> + dev_vcm->focus = v4l2_ctrl_new_std(hdl, ops,
> >> V4L2_CID_FOCUS_ABSOLUTE,
> >> + 0, AK7375_MAX_FOCUS_POS, AK7375_FOCUS_STEPS, 0);
> >> +
> >> + if (hdl->error)
> >> + dev_err(dev_vcm->sd.dev, "%s fail error: 0x%x\n",
> >> + __func__, hdl->error);
> >> + dev_vcm->sd.ctrl_handler = hdl;
> >> + return hdl->error;
> >> +}
> >> +
> >> +static int ak7375_probe(struct i2c_client *client) {
> >> + struct ak7375_device *ak7375_dev;
> >> + int val;
> >> +
> >> + ak7375_dev = devm_kzalloc(&client->dev, sizeof(*ak7375_dev),
> >> + GFP_KERNEL);
> >> + if (!ak7375_dev)
> >> + return -ENOMEM;
> >> +
> >> + v4l2_i2c_subdev_init(&ak7375_dev->sd, client, &ak7375_ops);
> >> + ak7375_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >> + ak7375_dev->sd.internal_ops = &ak7375_int_ops;
> >> + ak7375_dev->sd.entity.function = MEDIA_ENT_F_LENS;
> >> +
> >> + val = ak7375_init_controls(ak7375_dev);
> >> + if (val)
> >> + goto err_cleanup;
> >> +
> >> + val = media_entity_pads_init(&ak7375_dev->sd.entity, 0, NULL);
> >> + if (val < 0)
> >> + goto err_cleanup;
> >> +
> >> + val = v4l2_async_register_subdev(&ak7375_dev->sd);
> >> + if (val < 0)
> >> + goto err_cleanup;
> >> +
> >> + pm_runtime_set_active(&client->dev);
> >> + pm_runtime_enable(&client->dev);
> >> + pm_runtime_idle(&client->dev);
> >> +
> >> + return 0;
> >> +
> >> +err_cleanup:
> >> + v4l2_ctrl_handler_free(&ak7375_dev->ctrls_vcm);
> >> + media_entity_cleanup(&ak7375_dev->sd.entity);
> >> + dev_err(&client->dev, "Probe failed: %d\n", val);
> >> + return val;
> >> +}
> >> +
> >> +static int ak7375_remove(struct i2c_client *client) {
> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >> + struct ak7375_device *ak7375_dev = sd_to_ak7375_vcm(sd);
> >> +
> >> + ak7375_subdev_cleanup(ak7375_dev);
> >> + pm_runtime_disable(&client->dev);
> >> + pm_runtime_set_suspended(&client->dev);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/*
> >> + * This function sets the vcm position, so it consumes least current
> >> + * The lens position is gradually moved in units of
> >> +AK7375_CTRL_STEPS,
> >> + * to make the movements smoothly.
> >> + */
> >> +static int __maybe_unused ak7375_vcm_suspend(struct device *dev) {
> >> +
> >> + struct i2c_client *client = to_i2c_client(dev);
> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >> + struct ak7375_device *ak7375_dev = sd_to_ak7375_vcm(sd);
> >> + int ret, val;
> >> +
> > AK7375 enters stand-by mode after power-on, per data sheet.
> >
> > When the driver gets probed and this PM function is called, AK7375
> > should still be in stand-by mode (as active mode is set only in
> ak7375_vcm_resume).
> >
> > Per the data sheet, the vcm should be set to "active" mode, for the
> > magnet to move (which uses AK7375_REG_POSITION values).
> Yes, you are right, the magnet moves should be only applied when active, for
> the 1st suspend from probe should skip the position change, I will change in
> next version.
> >
> >> + for (val = ak7375_dev->focus->val & ~(AK7375_CTRL_STEPS - 1);
> >> + val >= 0; val -= AK7375_CTRL_STEPS) {
> >> + ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_POSITION,
> >> + val << 4, 2);
> >> + if (ret)
> >> + dev_err_once(dev, "%s I2C failure: %d\n",
> >> + __func__, ret);
> >> + usleep_range(AK7375_CTRL_DELAY_US,
> >> AK7375_CTRL_DELAY_US + 10);
> >> + }
> >> +
> >> + ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_CONT,
> >> + AK7375_MODE_STANDBY, 1);
> >> + if (ret) {
> >> + dev_err(dev, "%s I2C failure: %d\n", __func__, ret);
> >> + return ret;
> > I think we should proceed with the suspend, without returning the
> > error code (just like how it is done for REG_POSITION writes a little
> > earlier), as it might not be worth preventing the system suspend for
> > this issue.
> I think the logic here should be aligned with runtime resume.
> If the device is lost of control for some reason, maybe in an undefined state
> or
> active, the driver should return the REAL state as what it is instead of
> returning
> a success to cheat the system.
> what do you think?
These _suspend/_resume functions are also used in system sleep pm ops besides
runtime suspend/resume.
By proceeding with the suspend (instead of returning error as such), we say
that this vcm driver specific error is not critical enough to block system
sleep.
In a hypothetical case of this unrecoverable error, the system may never enter
sleep, which is not something that a user would desire.
If it were a processor specific PM driver code that is critical to successfully
get the processor out of sleep and is failing here, then it makes sense to block
the system from sleep (as we know the system may never resume, if we do
not pass on the error).
> >
> >> + }
> >> + 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
> >> +AK7375_CTRL_STEPS,
> >> + * to make the movements smoothly.
> >> + */
> >> +static int __maybe_unused ak7375_vcm_resume(struct device *dev) {
> >> + struct i2c_client *client = to_i2c_client(dev);
> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >> + struct ak7375_device *ak7375_dev = sd_to_ak7375_vcm(sd);
> >> + int ret, val;
> >> +
> >> + ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_CONT,
> >> + AK7375_MODE_ACTIVE, 1);
> >> + if (ret) {
> >> + dev_err(dev, "%s I2C failure: %d\n", __func__, ret);
> >> + return ret;
> >> + }
> >> +
> >> + for (val = ak7375_dev->focus->val % AK7375_CTRL_STEPS;
> >> + val <= ak7375_dev->focus->val;
> >> + val += AK7375_CTRL_STEPS) {
> >> + ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_POSITION,
> >> + val << 4, 2);
> >> + if (ret)
> >> + dev_err_ratelimited(dev, "%s I2C failure: %d\n",
> >> + __func__, ret);
> >> + usleep_range(AK7375_CTRL_DELAY_US,
> >> AK7375_CTRL_DELAY_US + 10);
> >> + }
> >> + return 0;
> >> +}
> >> +
> >> +static const struct of_device_id ak7375_of_table[] = {
> >> + { .compatible = "asahi-kasei,ak7375" },
> >> + { /* sentinel */ }
> >> +};
> >> +MODULE_DEVICE_TABLE(of, ak7375_of_table);
> >> +
> >> +static const struct dev_pm_ops ak7375_pm_ops = {
> >> + SET_SYSTEM_SLEEP_PM_OPS(ak7375_vcm_suspend,
> >> ak7375_vcm_resume)
> >> + SET_RUNTIME_PM_OPS(ak7375_vcm_suspend, ak7375_vcm_resume,
> >> NULL) };
> >> +
> >> +static struct i2c_driver ak7375_i2c_driver = {
> >> + .driver = {
> >> + .name = "ak7375",
> >> + .pm = &ak7375_pm_ops,
> >> + .of_match_table = ak7375_of_table,
> >> + },
> >> + .probe_new = ak7375_probe,
> >> + .remove = ak7375_remove,
> >> +};
> >> +module_i2c_driver(ak7375_i2c_driver);
> >> +
> >> +MODULE_AUTHOR("Tianshu Qiu <tian.shu....@intel.com>");
> >> +MODULE_AUTHOR("Bingbu Cao <bingbu....@intel.com>");
> >> +MODULE_DESCRIPTION("AK7375 VCM driver"); MODULE_LICENSE("GPL
> v2");
> >> --
> >> 1.9.1
