On Thu, Apr 12, 2018 at 6:57 PM Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
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
