Hi,

On Wed, Nov 21, 2018 at 11:56:50PM +0200, Sakari Ailus wrote:

On Thu, Nov 15, 2018 at 08:04:24PM +0100, Maxime Ripard wrote:

quoted

On Tue, Nov 13, 2018 at 10:38:55AM +0200, Sakari Ailus wrote:

quoted

quoted

+  - allwinner,has-isp: Whether the CSI controller has an ISP
+                       associated to it or not

Is the ISP a part of the same device? It sounds like that this is actually
a different device if it contains an ISP as well, and that should be
apparent from the compatible string. What do you think?

I guess we can see it as both. It seems to be the exact same register
set, except for the fact that the first instance has that ISP in
addition, and several channels, as you pointed out in your other mail.

I'm simply referring to existing practices as far as I know them. If it's a
different device, it should have a different compatible string, not a
vendor-specific property to tell it's somehow different.

The line is blurrier than that. Different devices are indeed
represented using different compatibles, but different features set
can be represented using properties.

Many SoCs also separate the DMA and the CSI-2 receivers, and thus they have
separate drivers. I don't know about your case, but the ISP requiring a
different clock is a hint.

In this particular case, the datasheet represents the ISP as part of
the DMA, so it looks like a feature. And since we don't have any
source code for this, we can only do guesswork.

quoted

What these channels are is not exactly clear. It looks like it's
related to the BT656 interface where you could interleave channel
bytes over the bus. I haven't really looked into it, and it doesn't
look like we have any code (or hardware) able to do that though.

quoted

quoted

+
+If allwinner,has-isp is set, an additional "isp" clock is needed,
+being a phandle to the clock driving the ISP.
+
+The CSI node should contain one 'port' child node with one child
+'endpoint' node, according to the bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt. The
+endpoint's bus type must be parallel or BT656.
+
+Endpoint node properties for CSI
+---------------------------------
+
+- remote-endpoint	: (required) a phandle to the bus receiver's endpoint
+			   node

Rob's opinion has been (AFAIU) that this is not needed as it's already a
part of the graph bindings. Unless you want to say that it's required, that
is --- the graph bindings document it as optional.

Ok, I'll remove it.

quoted

quoted

+- bus-width:		: (required) must be 8

If this is the only value the hardware supports, I don't see why you should
specify it here.

Ditto :)

quoted

quoted

+- pclk-sample		: (optional) (default: sample on falling edge)
+- hsync-active		: (only required for parallel)
+- vsync-active		: (only required for parallel)
+
+Example:
+
+csi0: csi@1c09000 {
+	compatible = "allwinner,sun7i-a20-csi",
+		     "allwinner,sun4i-a10-csi";
+	reg = <0x01c09000 0x1000>;
+	interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
+	clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI0>,
+		 <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>;
+	clock-names = "ahb", "mod", "isp", "ram";
+	resets = <&ccu RST_CSI0>;
+	allwinner,csi-channels = <4>;
+	allwinner,has-isp;
+	
+	port {
+		csi_from_ov5640: endpoint {
+                        remote-endpoint = <&ov5640_to_csi>;
+                        bus-width = <8>;
+                        data-shift = <2>;

data-shift needs to be documented above if it's relevant for the device.

It's not really related to the CSI device in that case but the sensor,
so I'll just leave it out.

Hmm. data-shift should only be relevant for the receiver, shoudn't it?

Sorry, I mispoke. We're not using it anywhere in either drivers, so
it's totally useless.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com