Hi Guennadi,

On 07/16/2012 10:55 AM, Guennadi Liakhovetski wrote:

Hi Sylwester

Thanks for your comments to my RFC and for pointing out to this your
earlier patch series. Unfortunately, I missed in in May, let me try to
provide some thoughts about this, we should really try to converge our
proposals. Maybe a discussion at KS would help too.

Thank you for the review. I was happy to see your RFC, as previously
there seemed to be not much interest in DT among the media guys.
Certainly, we need to work on a common approach to ensure interoperability
of existing drivers and to avoid having people inventing different
bindings for common features. I would also expect some share of device
specific bindings, as diversity of media devices is significant.

I'd be great to discuss these things at KS, especially support for 
proper suspend/resume sequences. Also having common sessions with 
other subsystems folks, like ASoC, for example, might be a good idea.

I'm not sure if I'll be travelling to the KS though. :)

On Fri, 25 May 2012, Sylwester Nawrocki wrote:

quoted

s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
(camera host interface DMA engine and image processor). This patch
adds support for instantiating the MIPI-CSIS devices from DT and
parsing all SoC and board specific properties from device tree.
The MIPI DPHY control callback is now called directly from within
the driver, the platform code must ensure this callback does the
right thing for each SoC.

The cell-index property is used to ensure proper signal routing,
from physical camera port, through MIPI-CSI2 receiver to the DMA
engine (FIMC?). It's also helpful in exposing the device topology
in user space at /dev/media? devnode (Media Controller API).

This patch also defines a common property ("data-lanes") for MIPI-CSI
receivers and transmitters.

Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
---
  Documentation/devicetree/bindings/video/mipi.txt   |    5 +
  .../bindings/video/samsung-mipi-csis.txt           |   47 ++++++++++
  drivers/media/video/s5p-fimc/mipi-csis.c           |   97 +++++++++++++++-----
  drivers/media/video/s5p-fimc/mipi-csis.h           |    1 +
  4 files changed, 126 insertions(+), 24 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/video/mipi.txt
  create mode 100644 Documentation/devicetree/bindings/video/samsung-mipi-csis.txt

diff --git a/Documentation/devicetree/bindings/video/mipi.txt b/Documentation/devicetree/bindings/video/mipi.txt
new file mode 100644
index 0000000..5aed285
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/mipi.txt

@@ -0,0 +1,5 @@
+Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and transmitters
+
+ - data-lanes : number of differential data lanes wired and actively used in
+		communication between the transmitter and the receiver, this
+		excludes the clock lane;

Wouldn't it be better to use the standard "bus-width" DT property?

I can't see any problems with using "bus-width". It seems sufficient
and could indeed be better, without a need to invent new MIPI-CSI 
specific names. That was my first RFC on that and my perspective 
wasn't probably broad enough. :)

quoted

diff --git a/Documentation/devicetree/bindings/video/samsung-mipi-csis.txt b/Documentation/devicetree/bindings/video/samsung-mipi-csis.txt
new file mode 100644
index 0000000..7bce6f4
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/samsung-mipi-csis.txt

@@ -0,0 +1,47 @@
+Samsung S5P/EXYNOS SoC MIPI-CSI2 receiver (MIPI CSIS)
+-----------------------------------------------------
+
+Required properties:
+
+- compatible - one of :
+		"samsung,s5pv210-csis",
+		"samsung,exynos4210-csis",
+		"samsung,exynos4212-csis",
+		"samsung,exynos4412-csis";
+- reg : physical base address and size of the device memory mapped registers;
+- interrupts      : should contain MIPI CSIS interrupt; the format of the
+		    interrupt specifier depends on the interrupt controller;
+- cell-index      : the hardware instance index;

Not sure whether this is absolutely needed... Wouldn't it be sufficient to
just enumerate them during probing?

As Grant pointed to me, the "cell-index" property is something that we should
be avoiding. But I needed something like this in the driver,
to differentiate between the multiple IP instances. I cannot simply assign
the indexes in random way to the hardware instances. Each of the MIPI-CSI 
Slaves has different properties (e.g. supporting max. 2 or 4 data lanes).
Additionally, a particular MIPI-CSI Slave instance is hard-wired inside the
SoC to the FIMC input mux (cross-bar) and physical video port. IOW, an image
sensor/video port/MIPI-CSIS instance assignment is fixed. If two MIPI-CSI 
image sensors are connected, one of them will work only with MIPI-CSIS0, and
the other only with MIPI-CSIS1.

So the driver must be able to identify it's physical device (IO region + 
IRQ) precisely.

That said, I found out recently that a proper entries in the "aliases"
could be used instead, and I could finally abandon the "cell-index" idea.
Not sure how aliases approach is better but from what I can see it is
a preferred way to handle things like the above.

Please see Documentation/devicetree/bindings/sound/mxs-saif.txt and users
of of_alias_get_id() for more details.

So to summarize, the indexes are needed, but the current implementation 
is not necessarily good, and AFAICT the aliases approach could be the way
to go.

quoted

+- clock-frequency : The IP's main (system bus) clock frequency in Hz, the default
+		    value when this property is not specified is 166 MHz;
+- data-lanes      : number of physical MIPI-CSI2 lanes used;

ditto - bus-width?

Yes, agreed. Let's drop it in favour of "bus-width".

--

Regards,
Sylwester