Hi Kieran,
Thanks for your feedback.
On 2018-03-13 23:23:49 +0100, Kieran Bingham wrote:
> Hi Niklas
>
> Thank you for this patch ...
> I know it has been a lot of work getting this and the VIN together!
>
> On 13/02/18 00:01, Niklas Söderlund wrote:
> > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> > supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are
> > connected between the video sources and the video grabbers (VIN).
> >
> > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > Reviewed-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>
> I don't think there's actually anything major in the below - so with any
> comments addressed, or specifically ignored you can add my:
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
Thanks, see bellow for answers to your questions. I have removed the
questions Laurent already have provided a reply for, thanks for doing
that!
>
> tag if you like.
>
>
> > ---
> > drivers/media/platform/rcar-vin/Kconfig | 12 +
> > drivers/media/platform/rcar-vin/Makefile | 1 +
> > drivers/media/platform/rcar-vin/rcar-csi2.c | 884 ++++++++++++++++++++++++++++
> > 3 files changed, 897 insertions(+)
> > create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c
> >
> > diff --git a/drivers/media/platform/rcar-vin/Kconfig b/drivers/media/platform/rcar-vin/Kconfig
> > index af4c98b44d2e22cb..6875f30c1ae42631 100644
> > --- a/drivers/media/platform/rcar-vin/Kconfig
> > +++ b/drivers/media/platform/rcar-vin/Kconfig
> > @@ -1,3 +1,15 @@
> > +config VIDEO_RCAR_CSI2
> > + tristate "R-Car MIPI CSI-2 Receiver"
> > + depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF
>
> I think I recall recently seeing that depending upon OF is redundant and not
> necessary (though I think that's minor in this instance)
I can't seem to find anything to indicate this, but as you state if
there is such a transition ongoing we can delete this later IMHO.
$ find . -name Kconfig -exec grep OF {} \; | grep "depends on" | wc -l
622
But I'm happy to be proven wrong and remove it now as well.
[snip]
> > +static const struct rcar_csi2_format
> > *rcar_csi2_code_to_fmt(unsigned int code)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
> > + if (rcar_csi2_formats[i].code == code)
> > + return rcar_csi2_formats + i;
>
> I would have written this as:
> return &rcar_csi2_formats[i]; but I think your way is valid too :)
This seems to be the popular opinion among reviewers so I will adopt
this style for the next version :-)
>
> I have a nice 'for_each_entity_in_array' macro I keep meaning to post which
> would be nice here too - but hey - for another day.
Looking forward to it.
[snip]
> > +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv)
> > +{
> > + int timeout;
> > +
> > + /* Wait for the clock and data lanes to enter LP-11 state. */
> > + for (timeout = 100; timeout > 0; timeout--) {
> > + const u32 lane_mask = (1 << priv->lanes) - 1;
> > +
> > + if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 &&
>
> The '1' is not very clear here.. Could this be:
>
> if ((rcar_csi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATE) &&
Great catch I have clearly missed this define. I will call it
PHCLM_STOPSTATECLK since that it's what the latest datasheet I have
calls it. Thanks for pointing this out.
[snip]
> > +static int rcar_csi2_s_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > + struct rcar_csi2 *priv = sd_to_csi2(sd);
> > + struct v4l2_subdev *nextsd;
> > + int ret = 0;
> > +
> > + mutex_lock(&priv->lock);
> > +
> > + if (!priv->remote) {
> > + ret = -ENODEV;
> > + goto out;
> > + }
> > +
> > + nextsd = priv->remote;
> > +
> > + if (enable && priv->stream_count == 0) {
> > + pm_runtime_get_sync(priv->dev);
> > +
> > + ret = rcar_csi2_start(priv);
> > + if (ret) {
> > + pm_runtime_put(priv->dev);
> > + goto out;
> > + }
> > +
> > + ret = v4l2_subdev_call(nextsd, video, s_stream, 1);
>
> Would it be nicer to pass 'enable' through instead of the '1'? (of course
> enable==1 here)
>
> > + if (ret) {
> > + rcar_csi2_stop(priv);
> > + pm_runtime_put(priv->dev);
> > + goto out;
> > + }
> > + } else if (!enable && priv->stream_count == 1) {
> > + rcar_csi2_stop(priv);
> > + v4l2_subdev_call(nextsd, video, s_stream, 0);
>
> likewise here...
>
I agree with Laurent's comment here that it's much clearer to use 1/0
instead of 'enable' when it comes to read the code and will keep it as
such :-)
> > + pm_runtime_put(priv->dev);
> > + }
>
> What's the 'nextsd' in this instance? We only call the s_stream on nextsd if it
> is the first one to be started, or the last one to be stopped...
The 'nextsd' here is the next subdevice towards the video source, and
it's a CSI-2 transmitter.
>
> I can understand this logic for calling rcar_csi2_stop/rcar_csi2_start ... but
> shouldn't we always call :
> v4l2_subdev_call(nextsd, video, s_stream, enable); ?
It might seem logical to do so yes, in my first try to add multiplexed
stream support to v4l2 I even attempted to move s_stream to pad ops
structure and extend it with a stream number argument :-) But after
discussing it and toying with different solution this turned out to be a
bad idea.
The CSI-2 transmitter and CSI-2 receiver needs to know about all links
when they are started for the first time. So the first on in turns on
everything and the last one out turns off the lights make sens here. The
user shall only enable the links it wish to use and they shall all be
started at the same time. Then we need this logic since we still need to
start streaming from the different VIN instances individually and the
CSI-2 receiver is the logical sync point to handle this as it has all
the knowledge of the links it is involved in.
>
> in GMSL context, I guess 'nextsd' is the MAX9286?
Yes.
> At which point - it would be called for start/stop, priv->stream_count times,
> and would also have to provide it's own call balancing...
Yes and what could it do different for each call even if it implemented
call balancing? It have no information about which of the enabled
multiplexed streams are started. The logic here is a s_stream() shall
start all enabled streams, if a user don't want a stream he shall not
enabled it in the first place. As all enabled links will be subject to
format validation in the media pipeline I think this will sort it self
out ;-P
>
> Call me crazy and ignore me here if you like :-)
I will not call you crazy, but I will ignore this comment :-)
[snip]
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Async and registered of subdevices and links
>
> "Async handling and registration of subdevices and links" ?
>
> or does of mean OF here ?
No of means of, your English is much better then mine and I will update
it for next version, thanks.
[snip]
> > +
> > + /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */
>
> /*
> * r8a7795 ES1.x behaves differently than the ES2.0+ but doesn't have it's own
> * compatible string
> */
>
Ditto.
[snip]
--
Regards,
Niklas Söderlund
