[PATCH v6 03/10] Documentation: devicetree: Update Samsung FIMC DT binding
From: laurent.pinchart@ideasonboard.com (Laurent Pinchart)
Date: 2014-03-11 12:30:39
Also in:
linux-devicetree, linux-media, linux-samsung-soc
Hi Sylwester, Thank you for the patch. On Thursday 06 March 2014 17:20:12 Sylwester Nawrocki wrote:
quoted hunk ↗ jump to hunk
This patch documents following updates of the Exynos4 SoC camera subsystem devicetree binding: - addition of #clock-cells property to 'camera' node - the #clock-cells property is needed when the sensor sub-devices use clock provided by the camera host interface, - addition of an optional clock-output-names property, - change of the clock-frequency at image sensor node from mandatory to an optional property - the sensor devices can now control their clock themselves and there should be no need to require this property by the camera host device binding, a default frequency value can often be used. Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> Acked-by: Kyungmin Park <kyungmin.park@samsung.com> --- Changes since v5: - none. Changes since v4: - dropped a requirement of specific order of values in clocks/ clock-names properties (Mark) and reference to clock-names in clock-output-names property description (Mark). --- .../devicetree/bindings/media/samsung-fimc.txt | 34 ++++++++++++----- 1 file changed, 26 insertions(+), 8 deletions(-)diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txtb/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" ?
+ +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 ?
quoted hunk ↗ jump to hunk
'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 ?
quoted hunk ↗ jump to hunk
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.
+ #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