Re: [PATCH 2/2] v4l: cadence: Add Cadence MIPI-CSI2 TX driver

[Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3013 bytes --]

Hi Sakari,

Sorry for the belated answer.

On Tue, Sep 26, 2017 at 08:00:14AM +0000, Sakari Ailus wrote:
> > On Fri, Sep 22, 2017 at 12:38:49PM +0000, Sakari Ailus wrote:
> > > > +	/*
> > > > +	 * Create a static mapping between the CSI virtual channels
> > > > +	 * and the input streams.
> > > 
> > > Which virtual channel is used here?
> > 
> > Like I was trying to explain in the cover letter, the virtual channel
> > is not under that block's control. The input video interfaces have an
> > additional signal that comes from the upstream device which sets the
> > virtual channel.
> 
> Oh, I missed while reviewing the set.
> 
> Presumably either driver would be in control of that then (this one or the
> upstream sub-device).

I don't really see how this driver could be under such control. I
guess it would depend on the integreation, but the upstream (sub-)
device is very likely to be under control of that signal, so I guess
it should be its role to change that. But we should also have that
information available so that the mixing in the CSI link is reported
properly in the media API (looking at Niklas' initial implementation).

> > > 
> > > > +		 */
> > > > +		writel(CSI2TX_STREAM_IF_CFG_FILL_LEVEL(4),
> > > > +		       csi2tx->base + CSI2TX_STREAM_IF_CFG_REG(stream));
> > > > +	}
> > > > +
> > > > +	/* Disable the configuration mode */
> > > > +	writel(0, csi2tx->base + CSI2TX_CONFIG_REG);
> > > 
> > > Shouldn't you start streaming on the downstream sub-device as well?
> > 
> > I appreciate it's a pretty weak argument, but the current setup we
> > have is in the FPGA is:
> > 
> > capture <- CSI2-RX <- CSI2-TX <- pattern generator
> > 
> > So far, the CSI2-RX block is calling its remote sub-device, which is
> > CSI2-TX. If CSI2-RX is calling its remote sub-device (CSI2-RX), we
> > just found ourselves in an endless loop.
> > 
> > I guess it should be easier, and fixable, when we'll have an actual
> > device without such a loopback.
> 
> What's the intended use case of the device, capture or output?

By device, you mean the CSI2-TX block, or something else?

If CSI2-TX, I guess it's more likely to be for output, but that might
be used for capture as well.

> How do you currently start the pipeline?

The capture device is the v4l2 device, and when the capture starts, it
enables the CSI2-RX which in turn enables CSI2-TX. The pattern
generator is enabled all the time.

> We have a few corner cases in V4L2 for such devices in graph parsing and
> stream control. The parsing of the device's fwnode graph endpoints are what
> the device can do, but it doesn't know where the parsing should continue
> and which part of the graph is already parsed.
> 
> That will be addressed but right now a driver just needs to know.

I'm not quite sure I got what you wanted me to fix or change.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]