Hi Sakari,
On Tue, May 9, 2017 at 4:55 AM, Sakari Ailus <sakari.ailus@xxxxxx> wrote:
> Hi Rajmohan,
>
> A few comments below...
>
> On Sun, May 07, 2017 at 04:33:24AM -0700, rajmohan.mani@xxxxxxxxx wrote:
[snip]
>> + rval = v4l2_async_register_subdev(&dw9714_dev->sd);
>> + if (rval < 0)
>> + goto err_cleanup;
>> +
>> + pm_runtime_enable(&client->dev);
>
> Getting PM runtime right doesn't seem to be easy. :-I
>
> pm_runtime_enable() alone doesn't do the trick. I wonder if adding
> pm_runtime_suspend() would do the trick.
Is this something specific for I2C devices? For platform devices,
typically pm_runtime_enable() is the only thing you would need to do.
>
>> +
>> + return 0;
>> +
>> +err_cleanup:
>> + dw9714_subdev_cleanup(dw9714_dev);
>> + dev_err(&client->dev, "Probe failed: %d\n", rval);
>> + return rval;
>> +}
>> +
>> +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
>> +
>> +static int dw9714_runtime_suspend(struct device *dev)
>> +{
>> + return 0;
>> +}
>> +
>> +static int dw9714_runtime_resume(struct device *dev)
>> +{
>> + return 0;
>
> I think it'd be fine to remove empty callbacks.
It's actually a bit more complicated (if a PM domain is attached, the
callbacks must be present), however in case of external I2C devices it
should be fine indeed. However, AFAIK, pm_runtime_no_callbacks()
should be called.
>
>> +}
>> +
>> +/* This function sets the vcm position, so it consumes least current */
>> +static int dw9714_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;
>> +
>> + dev_dbg(dev, "%s\n", __func__);
>> +
>> + 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));
>> + if (ret)
>> + dev_err(dev, "%s I2C failure: %d", __func__, ret);
>> + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
>> + }
>> + return 0;
>> +}
>> +
>> +/*
>> + * This function sets the vcm position, so the focus position is set
>> + * closer to the camera
>> + */
>> +static int dw9714_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;
>> +
>> + dev_dbg(dev, "%s\n", __func__);
>> +
>> + 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));
>> + if (ret)
>> + dev_err(dev, "%s I2C failure: %d", __func__, ret);
>> + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
>> + }
>> +
>> + /* restore v4l2 control values */
>> + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm);
>
> Doesn't this need to be done for runtime_resume as well?
This driver doesn't seem to be doing any physical power off in its
runtime_suspend and I don't expect an I2C device to be put in a PM
domain, so possibly no need for it.
Best regards,
Tomasz
