Hi Tomasz,
On Thu, May 11, 2017 at 04:02:35PM +0800, Tomasz Figa wrote:
> Hi Sakari,
>
> On Thu, May 11, 2017 at 3:55 PM, Sakari Ailus <sakari.ailus@xxxxxx> wrote:
> > Hi Tomasz,
> >
> > On Thu, May 11, 2017 at 02:30:31PM +0800, Tomasz Figa wrote:
> > ...
> >> > +
> >> > +/*
> >> > + * 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.
> >
> > The result from an error here is that the user would hear an audible click.
> > I don't think it's worth failing system suspend. :-)
> >
>
> Hmm, the result of an error here would be higher power consumption in
> suspend (unless there is also some other mechanism that actually cuts
> the power). Moreover, if an error here happens it would rather mean
On systems where power consumption matters the device would presumably be
powered off (e.g. by ACPI).
> that there is something wrong with hardware itself (or I2C driver) and
> not bailing out here would make it easier to let the error go
> unnoticed.
I'm not sure an error here matters much. :-)
The same function is also used as runtime_suspend(), in which case the
result won't be cared much about to begin with.
...
> >> > +/*
> >> > + * 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.
> >
> > Or drop the v4l2_ctrl_handler_setup() here.
> >
> > The reason is that the driver uses direct drive method for the lens and is
> > thus responsible for managing ringing compensation as well. Ringing
> > compensation support could be added to the driver later on; I think another
> > control will be needed to control the mode.
>
> Given that we already have some kind of ringing compensation in
> suspend and resume, can't we just reuse this in control handler? On
The way it's done here is unlikely to be helpful for the user space that
needs to drive the lens to a new position as fast as possible. The code
above is presumably enough to prevent the lens from hitting the mechanical
stopper but I'd equally expect it to interfere badly with the user space
trying to control the lens.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
