Hi Laurent, Philipp,
Thank you for the prompt review! I was pretty sure this would
raise your wise eyebrows :-)
On Wed, 2019-06-26 at 11:00 +0300, Laurent Pinchart wrote:
> Hi Ezequiel,
>
> Thank you for the patch.
>
> On Tue, Jun 25, 2019 at 05:39:45PM -0300, Ezequiel Garcia wrote:
> > Not all sensors will be able to guarantee a proper initial state.
> > This may be either because the driver is not properly written,
> > or (probably unlikely) because the hardware won't support it.
> >
> > While the right solution in the former case is to fix the sensor
> > driver, the real world not always allows right solutions, due to lack
> > of available documentation and support on these sensors.
> >
> > Let's relax this requirement, and allow the driver to support stream start,
> > even if the sensor initial sequence wasn't the expected.
> > A warning is still emitted, so users should be hinted that something is off.
>
> I'm not sure this is a very good idea. Failure to detect the LP-11 state
> may mean that the sensor is completely powered off, but it may also mean
> that it is already streaming data. I don't know how the CSI-2 receiver
> state machine will operate in the first case, but in the second case it
> will not be able to synchronise to the incoming stream, so it won't work
> anyway.
>
> I think you should instead fix the problem in the sensor driver, as you
> hinted.
Sure, we all agree that the sensor fix is the right solution.
> Relaxing the requirement here will only make it more confusing,
> it's a hack, and isn't portable across CSI-2 receivers.
We can emit a warning as suggested by Philipp, stating that the sensor
is buggy and needs fixing.
> The same buggy
> sensor driver won't work with other CSI-2 receivers whose internal state
> machine require starting in the LP-11 state.
>
Right. But the same buggy sensor driver probaly does work already with other
CSI-2 receivers, hence why it hasn't been detected when it was submitted.
> Which sensor are you using ?
>
I noticed this problem on OV5645 on a Wandoboard. Looks to be
basically the same issue as the one Jacopo fixed here:
Author: Jacopo Mondi <jacopo@xxxxxxxxxx>
Date: Fri Jul 6 05:51:52 2018 -0400
media: ov5640: Re-work MIPI startup sequence
I fixed it for OV5645 (and will submit soon), but that is
not the point.
The point is that buggy drivers exist, and while I'd love
to fix them all, it won't be always possible (due to lack of
datasheets, and due to some of these sensors having no active
maintainer).
Without this commit, such a buggy sensor will not work for sure;
while with the commit, there is a chance it will work.
Why would we prevent the latter from happening?
Thanks,
Eze
