Re: [RFC PATCH v5 13/14] media: tegra-video: Add CSI MIPI pads calibration

[Sowjanya Komatineni]

On 7/28/20 8:59 AM, Sowjanya Komatineni wrote:
> On 7/28/20 3:30 AM, Dmitry Osipenko wrote:
> > 27.07.2020 23:57, Sowjanya Komatineni ÐÐÑÐÑ:
> > > +ÂÂÂ /*
> > > +ÂÂÂÂ * TRM has incorrectly documented to wait for done status from
> > > +ÂÂÂÂ * calibration logic after CSI interface power on.
> > > +ÂÂÂÂ * As per the design, calibration results are latched and applied
> > > +ÂÂÂÂ * to the pads only when the link is in LP11 state which will 
> > > happen
> > > +ÂÂÂÂ * during the sensor stream-on.
> > > +ÂÂÂÂ * CSI subdev stream-on triggers start of MIPI pads calibration.
> > > +ÂÂÂÂ * Wait for calibration to finish here after sensor subdev 
> > > stream-on
> > > +ÂÂÂÂ * and in case of sensor stream-on failure, cancel the 
> > > calibration.
> > > +ÂÂÂÂ */
> > > ÂÂÂÂÂ subdev = on ? src_subdev : csi_subdev;
> > > ÂÂÂÂÂ ret = v4l2_subdev_call(subdev, video, s_stream, on);
> > > -ÂÂÂ if (ret < 0 && ret != -ENOIOCTLCMD)
> > > +ÂÂÂ if (ret < 0 && ret != -ENOIOCTLCMD) {
> > I assume -ENOIOCTLCMD means that camera wasn't turned ON, so why
> > -ENOIOCTLCMD is special?
> No -ENOIOCTLCMD mean subdev don't have s_stream ops
> > > +ÂÂÂÂÂÂÂ if (on && csi_chan->mipi)
> > > +ÂÂÂÂÂÂÂÂÂÂÂ tegra_mipi_cancel_calibration(csi_chan->mipi);
> > > ÂÂÂÂÂÂÂÂÂ return ret;
> > > +ÂÂÂ }
> > > +
> > > +ÂÂÂ if (on && csi_chan->mipi) {
> > Does finish_calibration() really need to be called for ret=-ENOIOCTLCMD?
> >
> > Shouldn't it be cancel_calibration( for the -ENOIOCTLCMD?
>
> start calibration happens during csi sensor streaming which happens 
> prior to this point.
>
> In case if sensor subdev does not have s_stream ops, then either 
> finish/cancel calibration should happen to disable the clock.

For -ENOIOCTLCMD, calling finish calibration as some sensors might keep 
pads in LP-11 on power up and for such sensors calibration logic will 
apply results to pads and done bit will be set.

Also avoiding additional check to specifically call cancel calibration 
on ENOIOCTLCMD and making it fall into finish calibration as both does 
disable clock except finish will wait for done bit to be set.

Also, most sensor subdev have s_stream ops implemented.

> > > +ÂÂÂÂÂÂÂ ret = tegra_mipi_finish_calibration(csi_chan->mipi);
> > > +ÂÂÂÂÂÂÂ if (ret < 0)
> > > +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(csi_chan->csi->dev,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "MIPI calibration failed: %d\n", ret);
> > Doesn't v4l2_subdev_call(OFF) need to be invoked here on error?
>
> Not required as on error streaming fails and runtime PM will turn off 
> power anyway.
>
> Also we only did csi subdev s_stream on and during sensor subdev 
> s_stream on fail, actual stream dont happen and on tegra side frame 
> capture by HW happens only when kthreads run.
> > > +ÂÂÂÂÂÂÂ return ret;
> > > +ÂÂÂ }
> > > Â ÂÂÂÂÂ return 0;
> > > Â }