Hi Josh,

On Wednesday 19 March 2014 17:15:02 Josh Wu wrote:

On 3/18/2014 9:36 PM, Laurent Pinchart wrote:

quoted

On Tuesday 18 March 2014 19:19:54 Josh Wu wrote:

quoted

This patch add the DT support for Atmel ISI driver.
It use the same v4l2 DT interface that defined in video-interfaces.txt.

Signed-off-by: Josh Wu <redacted>
Cc: devicetree at vger.kernel.org
---

 .../devicetree/bindings/media/atmel-isi.txt        |   51 ++++++++++++++
 drivers/media/platform/soc_camera/atmel-isi.c      |   33 ++++++++++++-
 2 files changed, 82 insertions(+), 2 deletions(-)
 create mode 100644
 Documentation/devicetree/bindings/media/atmel-isi.txt

diff --git a/Documentation/devicetree/bindings/media/atmel-isi.txt

b/Documentation/devicetree/bindings/media/atmel-isi.txt new file mode
100644
index 0000000..07f00eb

--- /dev/null
+++ b/Documentation/devicetree/bindings/media/atmel-isi.txt

@@ -0,0 +1,51 @@
+Atmel Image Sensor Interface (ISI) SoC Camera Subsystem
+----------------------------------------------
+
+Required properties:
+- compatible: must be "atmel,at91sam9g45-isi"
+- reg: physical base address and length of the registers set for the

device;
+- interrupts: should contain IRQ line for the ISI;
+- clocks: list of clock specifiers, corresponding to entries in
+          the clock-names property;
+- clock-names: must contain "isi_clk", which is the isi peripherial
clock.
+               "isi_mck" is optinal, it is the master clock output to
sensor.

The mck clock should be handled by the sensor driver instead. I know we
have a legacy mode in the atmel-isi driver to manage that clock
internally, but let's not propagate that to DT.

I agree with you.

I put the isi_mck as optional here because current the sensor driver
code only managed the v4l2 clock not the common clock.
There should add additional code to manager mck clock.
So if you want to ISI work for now, you should put the isi_mck in
atmel-isi DT node.

But for sure I can remove the isi_mck in atmel-isi DT document. In the
future it will be add in sensor's DT document.

I think that's the way to go, yes. I know we have existing platforms that 
require sensor clock management in the ISI driver. That should be fixed, and a 
move to DT is a perfect opportunity to do so :-)

quoted

I would also drop the "isi_" prefix from the isi_clk name.

hmm,  I think "isi_clk" indicates it is a ISI peripheral clock. And
which is consistent with other peripheral clock name in sama5.

I believe the "isi_" prefix is redundant, given that the clock-names property 
is inside the ISI DT node. However, if this style matches the rest of the 
platform there's no need to change it.

quoted

You should also describe the port node. You can just mention the related
bindings document, and state that the ISI has a single port.

OK. will add in the v2.

quoted

quoted

+Optional properties:
+- atmel,isi-disable-preview: a boolean property to disable the preview
channel;

That doesn't really sound like a hardware property to me. Isn't it full
mode related to software configuration instead, which should be performed
at runtime by userspace ?

yes, this configuration can be disable/enable by driver according to
user select format.
I will remove it in v2. Thanks.

Best Regards,
Josh Wu

quoted

quoted

+
+Example:
+	isi: isi at f0034000 {
+		compatible = "atmel,at91sam9g45-isi";
+		reg = <0xf0034000 0x4000>;
+		interrupts = <37 IRQ_TYPE_LEVEL_HIGH 5>;
+
+		clocks = <&isi_clk>, <&pck1>;
+		clock-names = "isi_clk", "isi_mck";
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_isi &pinctrl_pck1_as_isi_mck>;
+
+		port {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			isi_0: endpoint {
+				remote-endpoint = <&ov2640_0>;
+			};
+		};
+	};
+
+	i2c1: i2c at f0018000 {
+		ov2640: camera at 0x30 {
+			compatible = "omnivision,ov2640";
+			reg = <0x30>;
+
+			port {
+				ov2640_0: endpoint {
+					remote-endpoint = <&isi_0>;
+					bus-width = <8>;
+				};
+			};
+		};
+	};

-- 
Regards,

Laurent Pinchart