[v10,01/12] exynos5-fimc-is: Add Exynos5 FIMC-IS device tree bindings documentation
Commit Message
The patch adds the DT binding documentation for Samsung
Exynos5 SoC series imaging subsystem (FIMC-IS).
Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
.../devicetree/bindings/media/exynos5-fimc-is.txt | 84 ++++++++++++++++++++
1 file changed, 84 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
Comments
Hi,
Apologies for the late reply. I have a few comments, but nothing major.
On Fri, Oct 18, 2013 at 06:37:28AM +0100, Arun Kumar K wrote:
> The patch adds the DT binding documentation for Samsung
> Exynos5 SoC series imaging subsystem (FIMC-IS).
>
> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> .../devicetree/bindings/media/exynos5-fimc-is.txt | 84 ++++++++++++++++++++
> 1 file changed, 84 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
>
> diff --git a/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
> new file mode 100644
> index 0000000..0525417
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
> @@ -0,0 +1,84 @@
> +Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS)
> +------------------------------------------------------
> +
> +The camera subsystem on Samsung Exynos5 SoC has some changes relative
> +to previous SoC versions. Exynos5 has almost similar MIPI-CSIS and
> +FIMC-LITE IPs but has a much improved version of FIMC-IS which can
> +handle sensor controls and camera post-processing operations. The
> +Exynos5 FIMC-IS has a dedicated ARM Cortex A5 processor, many
> +post-processing blocks (ISP, DRC, FD, ODC, DIS, 3DNR) and two
> +dedicated scalers (SCC and SCP).
> +
> +fimc-is node
> +------------
> +
> +Required properties:
> +
> +- compatible : must be "samsung,exynos5250-fimc-is"
s/must be/should contain/
> +- reg : physical base address and size of the memory mapped
> + registers
> +- interrupt-parent : parent interrupt controller
I don't think this is actually required in all cases. It's a standard property
that people can add if it happens to be required in a particular instance.
> +- interrupts : fimc-is interrupt to the parent interrupt controller
Is this the only interrupt the device generates? If so:
- interrupts: interrupt-specifier for the fimc-is interrupt.
> +- clocks : list of clock specifiers, corresponding to entries in
> + clock-names property
> +- clock-names : must contain "isp", "mcu_isp", "isp_div0", "isp_div1",
> + "isp_divmpwm", "mcu_isp_div0", "mcu_isp_div1" entries,
> + matching entries in the clocks property
> +- samsung,pmu : phandle to the Power Management Unit (PMU) node
> +
> +i2c-isp (ISP I2C bus controller) nodes
> +--------------------------------------
> +The i2c-isp node is defined as the child node of fimc-is.
There are multiple of these, so how about the following instead:
The i2c-isp nodes should be children of the fimc-is node.
It might be worth pointing out that ranges, #address-cells, and #size-cells
should be present as appropriate.
> +
> +Required properties:
> +
> +- compatible : should be "samsung,exynos4212-i2c-isp" for Exynos4212,
> + Exynos4412 and Exynos5250 SoCs
Similarly, s/should be/must contain/
> +- reg : physical base address and length of the registers set
> +- clocks : must contain gate clock specifier for this controller
> +- clock-names : must contain "i2c_isp" entry
I'd prefer clocks to be described as for the simc-is, with clock names being
something like:
- clock-names: should contain "i2c_isp" for the gate clock.
> +
> +For the i2c-isp node, it is required to specify a pinctrl state named "default",
> +according to the pinctrl bindings defined in ../pinctrl/pinctrl-bindings.txt.
I'd prefer a mention of pinctrl-0 and pinctrl-names, as that's what most other
bindings do.
Also, as this is described as required it should be in the example.
> +
> +Device tree nodes of the image sensors controlled directly by the FIMC-IS
> +firmware must be child nodes of their corresponding ISP I2C bus controller node.
> +The data link of these image sensors must be specified using the common video
> +interfaces bindings, defined in video-interfaces.txt.
These don't seem to be in the example and probably should be.
Otherwise, this looks fine. With those points fixed up:
Acked-by: Mark Rutland <mark.rutland@arm.com>
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark,
Thank you for the review.
Will address your comments.
Regards
Arun
On Tue, Oct 29, 2013 at 5:02 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> Apologies for the late reply. I have a few comments, but nothing major.
>
> On Fri, Oct 18, 2013 at 06:37:28AM +0100, Arun Kumar K wrote:
>> The patch adds the DT binding documentation for Samsung
>> Exynos5 SoC series imaging subsystem (FIMC-IS).
>>
>> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
>> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> ---
>> .../devicetree/bindings/media/exynos5-fimc-is.txt | 84 ++++++++++++++++++++
>> 1 file changed, 84 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
>> new file mode 100644
>> index 0000000..0525417
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
>> @@ -0,0 +1,84 @@
>> +Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS)
>> +------------------------------------------------------
>> +
>> +The camera subsystem on Samsung Exynos5 SoC has some changes relative
>> +to previous SoC versions. Exynos5 has almost similar MIPI-CSIS and
>> +FIMC-LITE IPs but has a much improved version of FIMC-IS which can
>> +handle sensor controls and camera post-processing operations. The
>> +Exynos5 FIMC-IS has a dedicated ARM Cortex A5 processor, many
>> +post-processing blocks (ISP, DRC, FD, ODC, DIS, 3DNR) and two
>> +dedicated scalers (SCC and SCP).
>> +
>> +fimc-is node
>> +------------
>> +
>> +Required properties:
>> +
>> +- compatible : must be "samsung,exynos5250-fimc-is"
>
> s/must be/should contain/
>
>> +- reg : physical base address and size of the memory mapped
>> + registers
>> +- interrupt-parent : parent interrupt controller
>
> I don't think this is actually required in all cases. It's a standard property
> that people can add if it happens to be required in a particular instance.
>
>> +- interrupts : fimc-is interrupt to the parent interrupt controller
>
> Is this the only interrupt the device generates? If so:
>
> - interrupts: interrupt-specifier for the fimc-is interrupt.
>
>> +- clocks : list of clock specifiers, corresponding to entries in
>> + clock-names property
>> +- clock-names : must contain "isp", "mcu_isp", "isp_div0", "isp_div1",
>> + "isp_divmpwm", "mcu_isp_div0", "mcu_isp_div1" entries,
>> + matching entries in the clocks property
>> +- samsung,pmu : phandle to the Power Management Unit (PMU) node
>> +
>> +i2c-isp (ISP I2C bus controller) nodes
>> +--------------------------------------
>> +The i2c-isp node is defined as the child node of fimc-is.
>
> There are multiple of these, so how about the following instead:
>
> The i2c-isp nodes should be children of the fimc-is node.
>
> It might be worth pointing out that ranges, #address-cells, and #size-cells
> should be present as appropriate.
>
>> +
>> +Required properties:
>> +
>> +- compatible : should be "samsung,exynos4212-i2c-isp" for Exynos4212,
>> + Exynos4412 and Exynos5250 SoCs
>
> Similarly, s/should be/must contain/
>
>> +- reg : physical base address and length of the registers set
>> +- clocks : must contain gate clock specifier for this controller
>> +- clock-names : must contain "i2c_isp" entry
>
> I'd prefer clocks to be described as for the simc-is, with clock names being
> something like:
>
> - clock-names: should contain "i2c_isp" for the gate clock.
>
>> +
>> +For the i2c-isp node, it is required to specify a pinctrl state named "default",
>> +according to the pinctrl bindings defined in ../pinctrl/pinctrl-bindings.txt.
>
> I'd prefer a mention of pinctrl-0 and pinctrl-names, as that's what most other
> bindings do.
>
> Also, as this is described as required it should be in the example.
>
>> +
>> +Device tree nodes of the image sensors controlled directly by the FIMC-IS
>> +firmware must be child nodes of their corresponding ISP I2C bus controller node.
>> +The data link of these image sensors must be specified using the common video
>> +interfaces bindings, defined in video-interfaces.txt.
>
> These don't seem to be in the example and probably should be.
>
> Otherwise, this looks fine. With those points fixed up:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
>
> Thanks,
> Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
new file mode 100644
@@ -0,0 +1,84 @@
+Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS)
+------------------------------------------------------
+
+The camera subsystem on Samsung Exynos5 SoC has some changes relative
+to previous SoC versions. Exynos5 has almost similar MIPI-CSIS and
+FIMC-LITE IPs but has a much improved version of FIMC-IS which can
+handle sensor controls and camera post-processing operations. The
+Exynos5 FIMC-IS has a dedicated ARM Cortex A5 processor, many
+post-processing blocks (ISP, DRC, FD, ODC, DIS, 3DNR) and two
+dedicated scalers (SCC and SCP).
+
+fimc-is node
+------------
+
+Required properties:
+
+- compatible : must be "samsung,exynos5250-fimc-is"
+- reg : physical base address and size of the memory mapped
+ registers
+- interrupt-parent : parent interrupt controller
+- interrupts : fimc-is interrupt to the parent interrupt controller
+- clocks : list of clock specifiers, corresponding to entries in
+ clock-names property
+- clock-names : must contain "isp", "mcu_isp", "isp_div0", "isp_div1",
+ "isp_divmpwm", "mcu_isp_div0", "mcu_isp_div1" entries,
+ matching entries in the clocks property
+- samsung,pmu : phandle to the Power Management Unit (PMU) node
+
+i2c-isp (ISP I2C bus controller) nodes
+--------------------------------------
+The i2c-isp node is defined as the child node of fimc-is.
+
+Required properties:
+
+- compatible : should be "samsung,exynos4212-i2c-isp" for Exynos4212,
+ Exynos4412 and Exynos5250 SoCs
+- reg : physical base address and length of the registers set
+- clocks : must contain gate clock specifier for this controller
+- clock-names : must contain "i2c_isp" entry
+
+For the i2c-isp node, it is required to specify a pinctrl state named "default",
+according to the pinctrl bindings defined in ../pinctrl/pinctrl-bindings.txt.
+
+Device tree nodes of the image sensors controlled directly by the FIMC-IS
+firmware must be child nodes of their corresponding ISP I2C bus controller node.
+The data link of these image sensors must be specified using the common video
+interfaces bindings, defined in video-interfaces.txt.
+
+Example:
+
+ fimc_is: fimc-is@13000000 {
+ compatible = "samsung,exynos5250-fimc-is";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+ reg = <0x13000000 0x200000>;
+ interrupt-parent = <&combiner>;
+ interrupts = <19 1>;
+ clocks = <&clock 346>, <&clock 347>, <&clock 512>,
+ <&clock 513>, <&clock 514>, <&clock 515>,
+ <&clock 516>;
+ clock-names = "isp", "mcu_isp", "isp_div0", "isp_div1",
+ "isp_divmpwm", "mcu_isp_div0",
+ "mcu_isp_div1";
+ samsung,pmu = <&pmu>;
+
+ i2c0_isp: i2c-isp@13130000 {
+ compatible = "samsung,exynos4212-i2c-isp";
+ reg = <0x13130000 0x100>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&clock 352>;
+ clock-names = "i2c_isp";
+ };
+
+ i2c1_isp: i2c-isp@13140000 {
+ compatible = "samsung,exynos4212-i2c-isp";
+ reg = <0x13140000 0x100>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&clock 353>;
+ clock-names = "i2c_isp";
+ };
+ };