Re: [PATCH v4 2/2] media: ak7375: Add ak7375 lens voice coil driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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.



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux