Hi Sylwester,
Thank you for the patch.
On Thursday 06 March 2014 17:20:12 Sylwester Nawrocki wrote:
> 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@xxxxxxxxxxx>
> Acked-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> ---
> 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.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" ?
> +
> +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 ?
> '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 ?
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
