From: s.nawrocki@samsung•com (Sylwester Nawrocki)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v6 03/10] Documentation: devicetree: Update Samsung FIMC DT binding
Date: Tue, 11 Mar 2014 14:38:37 +0100 [thread overview]
Message-ID: <531F11DD.5040300@samsung.com> (raw)
In-Reply-To: <1608087.RUCeTiNcRR@avalon>
Hi Laurent,
Thanks for your review.
On 11/03/14 13:30, Laurent Pinchart wrote:
[...]
>> ---
>> .../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.
>> +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.
>> '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 ?
>> 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.
>> + #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,
Sylwester
next prev parent reply other threads:[~2014-03-11 13:38 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-06 16:20 [PATCH v6 00/10] Add device tree support for Exynos4 camera interface Sylwester Nawrocki
2014-03-06 16:20 ` [PATCH v6 01/10] Documentation: dt: Add binding documentation for S5K6A3 image sensor Sylwester Nawrocki
2014-03-06 18:08 ` Philipp Zabel
2014-03-06 20:15 ` Sylwester Nawrocki
2014-03-06 16:20 ` [PATCH v6 02/10] Documentation: dt: Add binding documentation for S5C73M3 camera Sylwester Nawrocki
2014-03-06 16:20 ` [PATCH v6 03/10] Documentation: devicetree: Update Samsung FIMC DT binding Sylwester Nawrocki
2014-03-11 12:30 ` Laurent Pinchart
2014-03-11 13:38 ` Sylwester Nawrocki [this message]
2014-03-11 14:58 ` Laurent Pinchart
2014-03-06 16:20 ` [PATCH v6 04/10] V4L: Add driver for s5k6a3 image sensor Sylwester Nawrocki
2014-03-06 16:20 ` [PATCH v6 05/10] V4L: s5c73m3: Add device tree support Sylwester Nawrocki
2014-03-18 10:05 ` Arnd Bergmann
2014-03-18 11:07 ` Sylwester Nawrocki
2014-03-06 16:20 ` [PATCH v6 06/10] exynos4-is: Use external s5k6a3 sensor driver Sylwester Nawrocki
2014-03-06 16:20 ` [PATCH v6 07/10] exynos4-is: Add clock provider for the SCLK_CAM clock outputs Sylwester Nawrocki
2014-03-06 16:20 ` [PATCH v6 08/10] exynos4-is: Add support for asynchronous subdevices registration Sylwester Nawrocki
2014-03-06 16:20 ` [PATCH v6 09/10] ARM: dts: Add rear camera nodes for Exynos4412 TRATS2 board Sylwester Nawrocki
2014-03-07 10:07 ` Sylwester Nawrocki
2014-03-06 16:20 ` [PATCH v6 10/10] ARM: dts: exynos4: Update camera clk provider and s5k6a3 sensor node Sylwester Nawrocki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=531F11DD.5040300@samsung.com \
--to=s.nawrocki@samsung$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox