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@xxxxxxxxx> 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
