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

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Tomasz,

On Tue, May 09, 2017 at 04:44:13PM +0800, Tomasz Figa wrote:
...
> > +/* 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);
> 
> Is it necessary to change the value in such steps? If so, shouldn't
> the control handler behave in the same way, making sure that userspace
> does not request changes in bigger steps?

I believe the reason for this is to gradually move the lens to the resting
position in order to avoid an audible click that results from the lens
hitting the mechanical stopper.

That said, this chip should have ringing compensation so I wonder if
gradually setting the desired value necessary. I let Rajmohan to comment on
that.

What still might be needed is to delay powering the device off by a small
amount of time.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk