Re: [PATCH v4] dw9714: Initial driver for dw9714 VCM

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

 



Hi Raj,

On Thu, May 11, 2017 at 3:30 PM, Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote:
> 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.
>
Any updates on this and other comments?

Best regards,
Tomasz



[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