On 2018年06月14日 21:02, Sakari Ailus wrote:
> Hi Bingbu,
>
> Thanks for the update. A few comments below; then I think we're done...
>
> On Thu, Jun 07, 2018 at 11:50:33AM +0800, bingbu.cao@xxxxxxxxx wrote:
>> From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>>
>> 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.qiu@xxxxxxxxx>
>> Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>>
>> ---
>> Changes from v1 -> v3:
>> - correct i2c write
>> - add media_entity_pads_init() into probe
>> - move the MAINTAINERs change into dt-binding change
>> - correct the compatible stringa
>> Changes since v3:
>> - add active flag to indicate the mode
>> ---
>> ---
>> drivers/media/i2c/Kconfig | 10 ++
>> drivers/media/i2c/Makefile | 1 +
>> drivers/media/i2c/ak7375.c | 289 +++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 300 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..e716821cf438
>> --- /dev/null
>> +++ b/drivers/media/i2c/ak7375.c
>> @@ -0,0 +1,289 @@
>> +// 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;
>> + /* active or standby mode */
>> + bool active;
>> +};
>> +
>> +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)
> unsigned int size
>
>> +{
>> + struct i2c_client *client = v4l2_get_subdevdata(&ak7375->sd);
>> + int ret;
>> + u8 buf[3];
> ret would be better declared after buf.
>
>> +
>> + 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;
> Please name variables for integer return values consistently. The driver
> now uses ret, rval and val.
>
>> +
>> + 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;
> Newline here?
>
>> + 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);
> Here, too.
>
>> + 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;
>> +
>> + if (!ak7375_dev->active)
>> + return 0;
>> +
>> + 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);
>> +
>> + ak7375_dev->active = false;
> Newline?
>
>> + 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;
>> +
>> + if (ak7375_dev->active)
>> + return 0;
>> +
>> + 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);
>> + }
>> +
>> + ak7375_dev->active = true;
>> +
>> + 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.qiu@xxxxxxxxx>");
>> +MODULE_AUTHOR("Bingbu Cao <bingbu.cao@xxxxxxxxx>");
>> +MODULE_DESCRIPTION("AK7375 VCM driver");
>> +MODULE_LICENSE("GPL v2");
>
>
Acks for all the comments, I will fix asap.
