Re: [V3, 2/2] media: i2c: Add DW9768 VCM driver

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Dongchun,

On Tue, Mar 10, 2020 at 06:00:19PM +0800, Dongchun Zhu wrote:
> Hi Sakari, Rob, Andy, Tomasz,
> 
> On Fri, 2020-02-28 at 23:59 +0800, Dongchun Zhu wrote:
...
> > +static int dw9768_init(struct dw9768 *dw9768)
> > +{
> > +	struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> > +	int ret, val;
> > +
> > +	/* Reset DW9768_RING_PD_CONTROL_REG to default status 0x00 */
> > +	ret = i2c_smbus_write_byte_data(client, DW9768_RING_PD_CONTROL_REG,
> > +					DW9768_PD_MODE_OFF);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/*
> > +	 * DW9769 requires waiting delay time of t_OPR
> > +	 * after PD reset takes place.
> > +	 */
> > +	usleep_range(DW9768_T_OPR_US, DW9768_T_OPR_US + 100);
> > +
> > +	ret = dw9768_write_array(dw9768, dw9768_init_regs,
> > +				 ARRAY_SIZE(dw9768_init_regs));
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (val = dw9768->focus->val % DW9768_MOVE_STEPS;
> > +	     val <= dw9768->focus->val;
> > +	     val += DW9768_MOVE_STEPS) {
> > +		ret = dw9768_set_dac(dw9768, val);
> > +		if (ret) {
> > +			dev_err(&client->dev, "%s I2C failure: %d",
> > +				__func__, ret);
> > +			return ret;
> > +		}
> > +		usleep_range(DW9768_MOVE_DELAY_US,
> > +			     DW9768_MOVE_DELAY_US + 1000);
> > +	}
> > +
> 
> What do you think about the approach taken by this patch?
> From the view of VCM hardware, the collision sound of lens should only
> happen when moving position back to zero.
> When opening camera, one should be able to move lens to the position
> directly.
> I tried to replace this code to a single dw9768_set_dac(dw9768,
> dw9768->focus->val),
> there is no collision sound when open camera and it could reduce several
> hundred ms when open lens driver fd.
> Are we okay with this?

I think so. Usually on VCMs with ringing compensation the only problematic
case is when the power is cut. :-)

-- 
Sakari Ailus