Hi Sylwester,

On Tuesday 11 March 2014 14:38:37 Sylwester Nawrocki wrote:

Hi Laurent,

Thanks for your review.

You're welcome.

On 11/03/14 13:30, Laurent Pinchart wrote:
[...]

quoted

quoted

---

 .../devicetree/bindings/media/samsung-fimc.txt     |   34 +++++++++-----
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt

b/Documentation/devicetree/bindings/media/samsung-fimc.txt index
96312f6..dbd4020 100644

--- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
+++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt

@@ -32,6 +32,21 @@ way around.

 The 'camera' node must include at least one 'fimc' child node.

+Optional properties:
+
+- #clock-cells: from the common clock bindings

(../clock/clock-bindings.txt),
+  must be 1. A clock provider is associated with the 'camera' node and
it should
+  be referenced by external sensors that use clocks provided by the SoC
on
+  CAM_*_CLKOUT pins. The clock specifier cell stores an index of a
clock.
+  The indices are 0, 1 for CAM_A_CLKOUT, CAM_B_CLKOUT clocks
respectively.
+
+- clock-output-names: from the common clock bindings, should contain
names of
+  clocks registered by the camera subsystem corresponding to
CAM_A_CLKOUT,
+  CAM_B_CLKOUT output clocks respectively.

Wouldn't it be better to document the "cam_mclk_a" and "cam_mclk_b" names
explicitly ? Or do you expect different names to be used in different DT
files ? And as they correspond to the CAM_A_CLKOUT and CAM_B_CLKOUT pins,
shouldn't they be named "cam_a_clkout" and "cam_b_clkout" ?

Basically I could use fixed names for these clocks, I just wanted to keep
a possibility to override them in dts to avoid any possible clock name
collisions, rather than keep a list of different names per SoC in the
driver. Right now fixed names could also be used for all SoCs I'm aware of,
nevertheless I would prefer to keep the clock-output-names property.
"cam_a_clkout", "cam_b_clkout" may be indeed better names, I'll change
that.

OK, variable names are fine with me.

quoted

quoted

+Note: #clock-cells and clock-output-names are mandatory properties if
external
+image sensor devices reference 'camera' device node as a clock provider.
+

What's the reason not to make them always mandatory ? Backward
compatibility only ? If so wouldn't it make sense to document the
properties as mandatory from now on, and treating them as optional in the
driver for backward compatibility ?

Yes, it's for backwards compatibility only. It may be a good idea to just
document them as required, since this is how the device is expected to be
described in DT from now. I'll just make these a required properties,
the driver already handles them as optional.

OK.

quoted

quoted

 'fimc' device nodes
 -------------------

@@ -97,8 +112,8 @@ Image sensor nodes

 The sensor device nodes should be added to their control bus controller

(e.g. I2C0) nodes and linked to a port node in the csis or the
parallel-ports node, using the common video interfaces bindings, defined
in video-interfaces.txt.
-The implementation of this bindings requires clock-frequency property to
be
-present in the sensor device nodes.
+An optional clock-frequency property needs to be present in the sensor
device
+nodes. Default value when this property is not present is 24 MHz.

This bothers me. Having the FIMC driver read the clock-frequence property
from the sensor DT nodes feels like a layering violation. Shouldn't the
sensor drivers call clk_set_rate() explicitly instead ?

It is supposed to do so, after this whole patch series. So the camera
controller driver will not need such properties. What do you think about
removing this sentence altogether ?

Sure. As the FIMC won't access the clock-frequency property of the sensor 
anymore after this patch series, let's just drop the mention of clock-
frequency.

quoted

quoted

 Example:

@@ -114,7 +129,7 @@ Example:
 			vddio-supply = <...>;
 			
 			clock-frequency = <24000000>;

-			clocks = <...>;
+			clocks = <&camera 1>;

 			clock-names = "mclk";
 			
 			port {

@@ -135,7 +150,7 @@ Example:
 			vddio-supply = <...>;
 			
 			clock-frequency = <24000000>;

-			clocks = <...>;
+			clocks = <&camera 0>;

 			clock-names = "mclk";
 			
 			port {

@@ -149,12 +164,15 @@ Example:
 	camera {
 	
 		compatible = "samsung,fimc", "simple-bus";

-		#address-cells = <1>;
-		#size-cells = <1>;
-		status = "okay";
-
+		clocks = <&clock 132>, <&clock 133>;
+		clock-names = "sclk_cam0", "sclk_cam1";

The documentation mentions that clock-names must contain "sclk_cam0",
"sclk_cam1", "pxl_async0", "pxl_async1". Are the last two optional ? If so
I think you should clarify the description of the clock-names property.
This can be done in a separate patch.

"pxl_async0", "pxl_async1" are mandatory, I'll add them also into
this example dts.

OK.

quoted

quoted

+		#clock-cells = <1>;
+		clock-output-names = "cam_mclk_a", "cam_mclk_b";

 		pinctrl-names = "default";
 		pinctrl-0 = <&cam_port_a_clk_active>;

+		status = "okay";
+		#address-cells = <1>;
+		#size-cells = <1>;

 		/* parallel camera ports */
 		parallel-ports {

-- 
Regards,

Laurent Pinchart