Re: [RESEND PATCH v7 2/2] media: dw9807: Add dw9807 vcm driver

[Tomasz Figa <tfiga@chromium.org>]

On Thu, Apr 12, 2018 at 6:57 PM Sakari Ailus <sakari.ailus@linux.intel.com>
wrote:

> Hi Jacopo,

> On Thu, Apr 12, 2018 at 10:57:01AM +0200, jacopo mondi wrote:
> ...
> > > +           if (MAX_RETRY == ++retry) {
> > > +                   dev_err(&client->dev,
> > > +                           "Cannot do the write operation because
VCM is busy\n");
> >
> > Nit: this is over 80 cols, it's fine, but I think you can really
> > shorten the error messag without losing context.

> dev_warn() or dev_info() might be more appropriate actually. Or even
> dev_dbg(). This isn't a grave problem; just a sign the user space is
trying
> to move the lens before it has reached its previous target position.

On the other hand, we print this only if we reach MAX_RETRY, which probably
means that the lens is stuck or some other unexpected failure.


> >
> > > +                   return -EIO;
> > > +           }
> > > +           usleep_range(DW9807_CTRL_DELAY_US, DW9807_CTRL_DELAY_US +
10);
> >
> > mmm, I wonder if a sleep range of 10usecs is really a strict
> > requirement. Have a look at Documentation/timers/timers-howto.txt.
> > With such a small range you're likely fire some unrequired interrupt.

> If the user is trying to tell where to move the lens next, no time should
> be wasted on waiting. It'd perhaps rather make sense to return an error
> (-EBUSY): the user application (as well as the application developer)
would
> know about the attempt to move the lens too fast and could take an
informed
> decision on what to do next. This could include changing the target
> position, waiting more or changing the program to adjust the 3A loop
> behaviour.

Actually, shouldn't we wait for the lens to finish moving after we set the
position? If we don't do it, we risk the userspace requesting a capture
with the lens still moving.

If "time wasted on waiting" is a concern here, userspace could as well just
have a separate thread for controlling the lens (as something that is
expected to take time due to physical limitations).

Best regards,
Tomasz