RE: [RESEND PATCH v7 2/2] media: dw9807: Add dw9807 vcm driver

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

 



Hi Jocopo, Tomasz,

Thanks for your comments.
Resend patches to https://patchwork.linuxtv.org/patch/49029/

Most of the comments are addressed. However some remain no change based on the discussion with Sakari and Tomasz. Posted as below.

>	if (MAX_RETRY == ++retry) {
>		dev_err(&client->dev,
>			"Cannot do the write operation because VCM is busy\n");

Jacopo:
Nit: this is over 80 cols, it's fine, but I think you can really
shorten the error messag without losing context.

Sakari:
dev_warn() or dev_info() might be more appropriate actually. Or even
dev_dbg(). This isn't a grave problem; just a sign the user space is trying
to move the lens before it has reached its previous target position.

Tomasz:
On the other hand, we print this only if we reach MAX_RETRY, which probably
means that the lens is stuck or some other unexpected failure.

Sakari:
MAX_RETRY is only ten, so I'd expect you could hit this if you're tring to
move the lens again very quickly. It usually takes several ms (but could
well be more than 10 ms) to reach the target position. This depends on the
lens and the driver, too, and I don't know the properties of this driver
(nor the lens).


> usleep_range(DW9807_CTRL_DELAY_US, DW9807_CTRL_DELAY_US + 10);

Jacopo:
mmm, I wonder if a sleep range of 10usecs is really a strict
requirement. Have a look at Documentation/timers/timers-howto.txt.
With such a small range you're likely fire some unrequired interrupt.

If I got this right, here you're just polling a register every
1msec-ish (usleep_range(1000, 1010)). I think you can enlarge the
range safely (maybe lowering the number of retries if you wish to) and
give more space to coalesce your wakeup with others.

What is a good range? Good question. How effective is this to have
your wakeup coalesced with others? I think this greatly depends on the
system you're running on and its load at this specific time. So I
would reply to both questions with "not sure", but I let you consider
if you could enlarge your range to say 1000-1500 usec at least.

Sakari:
If the user is trying to tell where to move the lens next, no time should
be wasted on waiting. It'd perhaps rather make sense to return an error
(-EBUSY): the user application (as well as the application developer) would
know about the attempt to move the lens too fast and could take an informed
decision on what to do next. This could include changing the target
position, waiting more or changing the program to adjust the 3A loop
behaviour.

Tomasz:
Actually, shouldn't we wait for the lens to finish moving after we set the
position? If we don't do it, we risk the userspace requesting a capture
with the lens still moving.

If "time wasted on waiting" is a concern here, userspace could as well just
have a separate thread for controlling the lens (as something that is
expected to take time due to physical limitations).

Sakari:
For that purpose I'd add a new control. The user process shouldn't wait in
the kernel for just the sake of this. In order to meaningfully control the
focussing process, the user space would have to know some properties of the
lens anyway, so this information would primarily be useful for checking
things are working out as expected.



Regards, Andy


-----Original Message-----
From: jacopo mondi [mailto:jacopo@xxxxxxxxxx] 
Sent: Thursday, April 12, 2018 4:57 PM
To: Yeh, Andy <andy.yeh@xxxxxxxxx>
Cc: linux-media@xxxxxxxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; tfiga@xxxxxxxxxxxx; Chiang, AlanX <alanx.chiang@xxxxxxxxx>
Subject: Re: [RESEND PATCH v7 2/2] media: dw9807: Add dw9807 vcm driver

HI Andy,
   thanks for addressing my comments on v6.
Some more questions below.

On Tue, Apr 10, 2018 at 11:48:44PM +0800, Andy Yeh wrote:
> From: Alan Chiang <alanx.chiang@xxxxxxxxx>
>
> DW9807 is a 10 bit DAC from Dongwoon, 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: Andy Yeh <andy.yeh@xxxxxxxxx>
> ---
> since v1:
> - changed author.
> since v2:
> - addressed outstanding comments.
> - enabled sequential write to update 2 registers in a single transaction.
> since v3:
> - addressed comments for v3.
> - Remove redundant codes and declare some variables as constant variable.
> - separate DT binding to another patch since v4:
> - sent patchset included DT binding with cover page since v6:
> - change the return code of i2c_check
> - fix long cols exceed 80 chars
> - remove #define DW9807_NAME since only used once
>
>  MAINTAINERS                |   7 +
>  drivers/media/i2c/Kconfig  |  10 ++
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/dw9807.c | 335 
> +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 353 insertions(+)
>  create mode 100644 drivers/media/i2c/dw9807.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS index 845fc25..a339bb5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4385,6 +4385,13 @@ T:	git git://linuxtv.org/media_tree.git
>  S:	Maintained
>  F:	drivers/media/i2c/dw9714.c
>
> +DONGWOON DW9807 LENS VOICE COIL DRIVER
> +M:	Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> +L:	linux-media@xxxxxxxxxxxxxxx
> +T:	git git://linuxtv.org/media_tree.git
> +S:	Maintained
> +F:	drivers/media/i2c/dw9807.c
> +
>  DOUBLETALK DRIVER
>  M:	"James R. Van Zandt" <jrv@xxxxxxxxxxxxxxx>
>  L:	blinux-list@xxxxxxxxxx
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig 
> index cb5d7ff..fd01842 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -325,6 +325,16 @@ config VIDEO_DW9714
>  	  capability. This is designed for linear control of
>  	  voice coil motors, controlled via I2C serial interface.
>
> +config VIDEO_DW9807
> +	tristate "DW9807 lens voice coil support"
> +	depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
> +	depends on VIDEO_V4L2_SUBDEV_API
> +	---help---

iirc checkpatch warned me multiple times to prefer 'help' on '---help---' for newly introduced symbols. Could you check please (maybe with using the --strict option?)

> +	  This is a driver for the DW9807 camera lens voice coil.
> +	  DW9807 is a 10 bit DAC with 100mA 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 548a9ef..1b62639 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -23,6 +23,7 @@ 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_DW9807)  += dw9807.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/dw9807.c b/drivers/media/i2c/dw9807.c new file 
> mode 100644 index 0000000..062c30f
> --- /dev/null
> +++ b/drivers/media/i2c/dw9807.c
> @@ -0,0 +1,335 @@
> +// 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 DW9807_MAX_FOCUS_POS	1023
> +/*
> + * This sets the minimum granularity for the focus positions.
> + * A value of 1 gives maximum accuracy for a desired focus position.
> + */
> +#define DW9807_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 DW9807_CTRL_STEPS	16
> +#define DW9807_CTRL_DELAY_US	1000
> +
> +#define DW9807_CTL_ADDR		0x02
> +/*
> + * DW9807 separates two registers to control the VCM position.
> + * One for MSB value, another is LSB value.
> + */
> +#define DW9807_MSB_ADDR		0x03
> +#define DW9807_LSB_ADDR		0x04
> +#define DW9807_STATUS_ADDR	0x05
> +#define DW9807_MODE_ADDR	0x06
> +#define DW9807_RESONANCE_ADDR	0x07
> +
> +#define MAX_RETRY		10
> +
> +struct dw9807_device {
> +	struct v4l2_ctrl_handler ctrls_vcm;
> +	struct v4l2_subdev sd;
> +	u16 current_val;
> +};
> +
> +static inline struct dw9807_device *sd_to_dw9807_vcm(
> +					struct v4l2_subdev *subdev)
> +{
> +	return container_of(subdev, struct dw9807_device, sd); }
> +
> +static int dw9807_i2c_check(struct i2c_client *client) {
> +	const char status_addr = DW9807_STATUS_ADDR;
> +	char status_result;
> +	int ret;
> +
> +	ret = i2c_master_send(client, (const char *)&status_addr,
> +		sizeof(status_addr));

> As reported by checkpatch, please fix alignement to the first open brace, and as per Tomasz comment remove the cast

Done.

> +	if (ret < 0) {
> +		dev_err(&client->dev, "I2C write STATUS address fail ret = %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = i2c_master_recv(client, (char *)&status_result,
> +		sizeof(status_result));

> Align the last argument to the first open brace.

Done

	ret = i2c_master_recv(client, (char *)&status_result,
		              sizeof(status_result));

> +	if (ret != sizeof(status_result)) {

> My comment on i2c functions return values applies to i2c_master_recv() as well. Please just check for (ret < 0) here

Done

> +		dev_err(&client->dev, "I2C read STATUS value fail ret=%d\n",

> The previous errror message has spaces in between 'ret','=' and '%d'.
>Please be consistent.

Done

> +			ret);
> +		return ret;
> +	}
> +
> +	return status_result;
> +}
> +
> +static int dw9807_set_dac(struct i2c_client *client, u16 data) {
> +	const char tx_data[3] = {
> +		DW9807_MSB_ADDR, ((data >> 8) & 0x03), (data & 0xff)
> +	};
> +	int ret, retry = 0;
> +
> +	/*
> +	 * According to the datasheet, need to check the bus status before we
> +	 * write VCM position. This ensure that we really write the value
> +	 * into the register
> +	 */
> +	while ((ret = dw9807_i2c_check(client)) != 0) {

> Tomasz: 
> One could use readx_poll_timeout() here:

> int val;

> ret = readx_poll_timeout(dw9807_i2c_check, client, val, !val,
>                           DW9807_CTRL_DELAY_US,
>                           MAX_RETRY * DW9807_CTRL_DELAY_US);

> if (ret) {
>          dev_err(&client->dev,
>                  "Cannot do the write operation because VCM is busy\n");
>          return -EIO;
> }

Done,

> +		if (ret < 0)
> +			return ret;
> +
> +		if (MAX_RETRY == ++retry) {
> +			dev_err(&client->dev,
> +				"Cannot do the write operation because VCM is busy\n");

Nit: this is over 80 cols, it's fine, but I think you can really shorten the error messag without losing context.

> +			return -EIO;
> +		}
> +		usleep_range(DW9807_CTRL_DELAY_US, DW9807_CTRL_DELAY_US + 10);

mmm, I wonder if a sleep range of 10usecs is really a strict requirement. Have a look at Documentation/timers/timers-howto.txt.
With such a small range you're likely fire some unrequired interrupt.

If I got this right, here you're just polling a register every 1msec-ish (usleep_range(1000, 1010)). I think you can enlarge the range safely (maybe lowering the number of retries if you wish to) and give more space to coalesce your wakeup with others.

What is a good range? Good question. How effective is this to have your wakeup coalesced with others? I think this greatly depends on the system you're running on and its load at this specific time. So I would reply to both questions with "not sure", but I let you consider if you could enlarge your range to say 1000-1500 usec at least.


> +	}
> +
> +	/* Write VCM position to registers */
> +	ret = i2c_master_send(client, tx_data, sizeof(tx_data));
> +	if (ret != sizeof(tx_data)) {

> As per previous comments, check for ret < 0

Done

> +		if (ret < 0) {
> +			dev_err(&client->dev,
> +				"I2C write MSB fail ret=%d\n", ret);
> +			return ret;
> +		} else {

> There cannot be any else case here as i2c_master_send returns < 0  or
> sizeof(tx_data) only.

Done

> +			dev_err(&client->dev, "I2C write MSB fail, transmission size is not equal the size expected\n");
> +			return -EIO;

> I believe this can't happen, both by looking at implementation of
> i2c_master_send() and existing drivers checking only for (ret < 0).

Done

> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int dw9807_set_ctrl(struct v4l2_ctrl *ctrl) {
> +	struct dw9807_device *dev_vcm = container_of(ctrl->handler,
> +		struct dw9807_device, ctrls_vcm);
> +
> +	if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE) {
> +		struct i2c_client *client = v4l2_get_subdevdata(&dev_vcm->sd);
> +
> +		dev_vcm->current_val = ctrl->val;
> +		return dw9807_set_dac(client, ctrl->val);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct v4l2_ctrl_ops dw9807_vcm_ctrl_ops = {
> +	.s_ctrl = dw9807_set_ctrl,
> +};
> +
> +static int dw9807_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 dw9807_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh 
> +*fh) {
> +	pm_runtime_put(sd->dev);
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_internal_ops dw9807_int_ops = {
> +	.open = dw9807_open,
> +	.close = dw9807_close,
> +};
> +
> +static const struct v4l2_subdev_ops dw9807_ops = { };
> +
> +static void dw9807_subdev_cleanup(struct dw9807_device *dw9807_dev) {
> +	v4l2_async_unregister_subdev(&dw9807_dev->sd);
> +	v4l2_ctrl_handler_free(&dw9807_dev->ctrls_vcm);
> +	media_entity_cleanup(&dw9807_dev->sd.entity);
> +}
> +
> +static int dw9807_init_controls(struct dw9807_device *dev_vcm) {
> +	struct v4l2_ctrl_handler *hdl = &dev_vcm->ctrls_vcm;
> +	const struct v4l2_ctrl_ops *ops = &dw9807_vcm_ctrl_ops;
> +	struct i2c_client *client = v4l2_get_subdevdata(&dev_vcm->sd);
> +
> +	v4l2_ctrl_handler_init(hdl, 1);
> +
> +	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE,
> +			  0, DW9807_MAX_FOCUS_POS, DW9807_FOCUS_STEPS, 0);
> +
> +	dev_vcm->sd.ctrl_handler = hdl;
> +	if (hdl->error) {
> +		dev_err(&client->dev, "%s fail error: 0x%x\n",
> +			__func__, hdl->error);
> +		return hdl->error;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dw9807_probe(struct i2c_client *client) {
> +	struct dw9807_device *dw9807_dev;
> +	int rval;
> +
> +	dw9807_dev = devm_kzalloc(&client->dev, sizeof(*dw9807_dev),
> +				  GFP_KERNEL);
> +	if (!dw9807_dev)
> +		return -ENOMEM;
> +
> +	v4l2_i2c_subdev_init(&dw9807_dev->sd, client, &dw9807_ops);
> +	dw9807_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	dw9807_dev->sd.internal_ops = &dw9807_int_ops;
> +
> +	rval = dw9807_init_controls(dw9807_dev);
> +	if (rval)
> +		goto err_cleanup;
> +
> +	rval = media_entity_pads_init(&dw9807_dev->sd.entity, 0, NULL);
> +	if (rval < 0)
> +		goto err_cleanup;
> +
> +	dw9807_dev->sd.entity.function = MEDIA_ENT_F_LENS;

Not super sure here, Sakari may confirm or not, but you don't have pads, you don't have pad operations, why are initializing entity pads and depend on MEDIA_CONTROLLER in Kconfig? I -think- you can remove these lines above here.

> +
> +	rval = v4l2_async_register_subdev(&dw9807_dev->sd);
> +	if (rval < 0)
> +		goto err_cleanup;
> +
> +	pm_runtime_set_active(&client->dev);
> +	pm_runtime_enable(&client->dev);
> +	pm_runtime_idle(&client->dev);
> +
> +	return 0;
> +
> +err_cleanup:
> +	dw9807_subdev_cleanup(dw9807_dev);
> +
> +	return rval;
> +}
> +
> +static int dw9807_remove(struct i2c_client *client) {
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct dw9807_device *dw9807_dev = sd_to_dw9807_vcm(sd);
> +
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);
> +
> +	dw9807_subdev_cleanup(dw9807_dev);
> +
> +	return 0;
> +}
> +
> +/*
> + * This function sets the vcm position, so it consumes least current
> + * The lens position is gradually moved in units of 
> +DW9807_CTRL_STEPS,
> + * to make the movements smoothly.
> + */
> +static int __maybe_unused dw9807_vcm_suspend(struct device *dev) {
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct dw9807_device *dw9807_dev = sd_to_dw9807_vcm(sd);
> +	const char tx_data[2] = { DW9807_CTL_ADDR, 0x01 };
> +	int ret, val;
> +
> +	for (val = dw9807_dev->current_val & ~(DW9807_CTRL_STEPS - 1);
> +	     val >= 0; val -= DW9807_CTRL_STEPS) {
> +		ret = dw9807_set_dac(client, val);
> +		if (ret)
> +			dev_err_once(dev, "%s I2C failure: %d", __func__, ret);
> +		usleep_range(DW9807_CTRL_DELAY_US, DW9807_CTRL_DELAY_US + 10);

> ditto

Done

> +	}
> +
> +	/* Power down */
> +	ret = i2c_master_send(client, tx_data, sizeof(tx_data));
> +

> ditch this empty line please

Done

> +	if (ret != sizeof(tx_data)) {

> ditto, check for ret < 0

Done

> +		dev_err(&client->dev, "I2C write CTL fail\n");
> +		return -EIO;

consider returning ret to propagate i2c 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 
> +DW9807_CTRL_STEPS,
> + * to make the movements smoothly.
> + */
> +static int  __maybe_unused dw9807_vcm_resume(struct device *dev) {
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct dw9807_device *dw9807_dev = sd_to_dw9807_vcm(sd);
> +	const char tx_data[2] = { DW9807_CTL_ADDR, 0x00 };
> +	int ret, val;
> +
> +	/* Power on */
> +	ret = i2c_master_send(client, tx_data, sizeof(tx_data));
> +	if (ret != sizeof(tx_data)) {

> ditto, check for ret < 0

Done

> +		dev_err(&client->dev, "I2C write CTL fail\n");
> +		return -EIO;

> consider returning ret to propagate i2c error

Done


> +	}
> +
> +	for (val = dw9807_dev->current_val % DW9807_CTRL_STEPS;
> +	     val < dw9807_dev->current_val + DW9807_CTRL_STEPS - 1;
> +	     val += DW9807_CTRL_STEPS) {
> +		ret = dw9807_set_dac(client, val);
> +		if (ret)
> +			dev_err_ratelimited(dev, "%s I2C failure: %d",
> +						__func__, ret);
> +		usleep_range(DW9807_CTRL_DELAY_US, DW9807_CTRL_DELAY_US + 10);

> ditto

Done

> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id dw9807_of_table[] = {
> +	{ .compatible = "dongwoon,dw9807" },
> +	{ { 0 } }

> Tomasz:
> It looks like we may need other changes, so I'd go with
> { /* sentinel */ },
> here. That's (+/- the comment) what other drivers use normally.

done

> +};
> +MODULE_DEVICE_TABLE(of, dw9807_of_table);
> +
> +static const struct dev_pm_ops dw9807_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(dw9807_vcm_suspend, dw9807_vcm_resume)
> +	SET_RUNTIME_PM_OPS(dw9807_vcm_suspend, dw9807_vcm_resume, NULL) };
> +
> +static struct i2c_driver dw9807_i2c_driver = {
> +	.driver = {
> +		.name = "dw9807",
> +		.pm = &dw9807_pm_ops,
> +		.of_match_table = dw9807_of_table,
> +	},
> +	.probe_new = dw9807_probe,
> +	.remove = dw9807_remove,
> +};
> +
> +module_i2c_driver(dw9807_i2c_driver);
> +
> +MODULE_AUTHOR("Chiang, Alan <alanx.chiang@xxxxxxxxx>");
> +MODULE_DESCRIPTION("DW9807 VCM driver"); MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux