On Thu, May 11, 2017 at 4:24 PM, Sakari Ailus <sakari.ailus@xxxxxx> wrote:
> 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.
After thinking a bit more, I realized that the error would be actually
noticeable in terms of the camera not focusing properly, so maybe
there is no reason to care too much here indeed.
>
> ...
>
>> >> > +/*
>> >> > + * 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.
Okay, fair enough. I assume then it's not unsafe for the hardware to
allow userspace to control it over the full range and the worst thing
that can happen is getting some sound effects? (Rather than some
malicious userspace burning the motor or so.
Best regards,
Tomasz
