diff mbox

[v6,5/5] ARM: dts: Add FIMD DT binding Documentation

Message ID 1362044559-7496-6-git-send-email-vikas.sajjan@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Vikas C Sajjan Feb. 28, 2013, 9:42 a.m. UTC 
Adds FIMD DT binding documentation both Samsung SoC and Board, with an example

Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
---
 .../devicetree/bindings/video/samsung-fimd.txt     |   54 ++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/samsung-fimd.txt

Comments

Sylwester Nawrocki March 3, 2013, 7:11 p.m. UTC | #1 
On 02/28/2013 10:42 AM, Vikas Sajjan wrote:
> Adds FIMD DT binding documentation both Samsung SoC and Board, with an example
>
> Signed-off-by: Vikas Sajjan<vikas.sajjan@linaro.org>
> ---
>   .../devicetree/bindings/video/samsung-fimd.txt     |   54 ++++++++++++++++++++
>   1 file changed, 54 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/video/samsung-fimd.txt
>
> diff --git a/Documentation/devicetree/bindings/video/samsung-fimd.txt b/Documentation/devicetree/bindings/video/samsung-fimd.txt
> new file mode 100644
> index 0000000..8d201e7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/samsung-fimd.txt
> @@ -0,0 +1,54 @@
> +Device-Tree bindings for Samsung SoC display controller (FIMD)
> +
> +FIMD stands for Fully Interactive Mobile Display, is the Display Controller for
> +the Samsung series of SoCs which transfers the image data from a video buffer
> +located in the system memory to an external LCD interface.
> +
> +Required properties:
> +- compatible := value should be one of the following
> +		"samsung,s3c2443-fimd"; /* for S3C24XX SoCs */
> +		"samsung,s3c6400-fimd"; /* for S3C64XX SoCs */
> +		"samsung,s5p6440-fimd"; /* for S5P64X0 SoCs */
> +		"samsung,s5pc100-fimd"; /* for S5PC100 SoC  */
> +		"samsung,s5pv210-fimd"; /* for S5PV210 SoC */
> +		"samsung,exynos4210-fimd"; /* for Exynos4 SoCs */
> +		"samsung,exynos5250-fimd"; /* for Exynos5 SoCs */
> +
> +- reg := physical base address of the fimd and length of memory mapped region

I think FIMD should be capitalized.

> +
> +- interrupt-parent := reference to the interrupt combiner node with phandle

Perhaps this would have been more clear:

- interrupt-parent : a phandle to the interrupt combiner node;

?

And could we just use a colon instead of ":=", to keep it more consistent
with other Samsung SoC DT bindings documentation ?

> +
> +- interrupts := interrupt number from the combiner to the cpu.
> +		We have 3 interrupts and the interrupt combiner order is
> +		FIFO Level, VSYNC, and LCD_SYSTEM.
> +		but since the driver expects VSYNC to be the first IRQ,
> +		make sure to mention order as VSYNC, FIFO Level and LCD_SYSTEM
> +		keeping VSYNC as first IRQ as shown below.
> +		for example: interrupts =<11 1>,<11 0>,<11 2>;

I have a suggestion here. What about using reg-names property to make this
more explicit, e.g.

- interrupts : should contain a list of all FIMD IP block interrupts:
   FIFO Level, VSYNC, LCD_SYSTEM. The interrupt specifier format depends
   on the interrupt controller used;

- interrupt-names : should contain the interrupt names: "fifo", "vsync",
   "lcd_sys", in the order in which they were listed in the interrupts
   property;

Or something similar to that. Then the drivers would need to be modified
to get interrupt resource by name, and not to rely on the VSYNC interrupt
be listed as the first one.

> +
> +- pinctrl := property defining the pinctrl configurations with a phandle
> +
> +- pinctrl-names := name of the pinctrl

Might be sufficient to just mention that the "default" state needs to be
specified in the fimd node, with reference to the pinctrl bindings
documentation, e.g.

"The pinctrl bindings defined in ../../pinctrl/pinctrl-bindings.txt must be
used to define a pinctrl state named "default".

See https://patchwork.kernel.org/patch/2082311.

> +Optional Properties:
> +- samsung,power-domain := power domain property defined with a phandle

"a phandle to FIMD power domain node" ?
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki March 3, 2013, 8:03 p.m. UTC | #2 
On 03/03/2013 08:11 PM, Sylwester Nawrocki wrote:
> I have a suggestion here. What about using reg-names property to make this

Oops, this should read "interrupt-names", not "reg-names".

> more explicit, e.g.
>
> - interrupts : should contain a list of all FIMD IP block interrupts:
>    FIFO Level, VSYNC, LCD_SYSTEM. The interrupt specifier format depends
>    on the interrupt controller used;
>
> - interrupt-names : should contain the interrupt names: "fifo", "vsync",
>    "lcd_sys", in the order in which they were listed in the interrupts
>    property;
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vikas C Sajjan March 6, 2013, 10:18 a.m. UTC | #3 
Hi,

On 4 March 2013 00:41, Sylwester Nawrocki <sylvester.nawrocki@gmail.com> wrote:
> On 02/28/2013 10:42 AM, Vikas Sajjan wrote:
>>
>> Adds FIMD DT binding documentation both Samsung SoC and Board, with an
>> example
>>
>> Signed-off-by: Vikas Sajjan<vikas.sajjan@linaro.org>
>> ---
>>   .../devicetree/bindings/video/samsung-fimd.txt     |   54
>> ++++++++++++++++++++
>>   1 file changed, 54 insertions(+)
>>   create mode 100644
>> Documentation/devicetree/bindings/video/samsung-fimd.txt
>>
>> diff --git a/Documentation/devicetree/bindings/video/samsung-fimd.txt
>> b/Documentation/devicetree/bindings/video/samsung-fimd.txt
>> new file mode 100644
>> index 0000000..8d201e7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/video/samsung-fimd.txt
>> @@ -0,0 +1,54 @@
>> +Device-Tree bindings for Samsung SoC display controller (FIMD)
>> +
>> +FIMD stands for Fully Interactive Mobile Display, is the Display
>> Controller for
>> +the Samsung series of SoCs which transfers the image data from a video
>> buffer
>> +located in the system memory to an external LCD interface.
>> +
>> +Required properties:
>> +- compatible := value should be one of the following
>> +               "samsung,s3c2443-fimd"; /* for S3C24XX SoCs */
>> +               "samsung,s3c6400-fimd"; /* for S3C64XX SoCs */
>> +               "samsung,s5p6440-fimd"; /* for S5P64X0 SoCs */
>> +               "samsung,s5pc100-fimd"; /* for S5PC100 SoC  */
>> +               "samsung,s5pv210-fimd"; /* for S5PV210 SoC */
>> +               "samsung,exynos4210-fimd"; /* for Exynos4 SoCs */
>> +               "samsung,exynos5250-fimd"; /* for Exynos5 SoCs */
>> +
>> +- reg := physical base address of the fimd and length of memory mapped
>> region
>
>
> I think FIMD should be capitalized.
>
Right.
>
>> +
>> +- interrupt-parent := reference to the interrupt combiner node with
>> phandle
>
>
> Perhaps this would have been more clear:
>
> - interrupt-parent : a phandle to the interrupt combiner node;
>
> ?
OK.
>
> And could we just use a colon instead of ":=", to keep it more consistent
> with other Samsung SoC DT bindings documentation ?
>
OK.
>
>> +
>> +- interrupts := interrupt number from the combiner to the cpu.
>> +               We have 3 interrupts and the interrupt combiner order is
>> +               FIFO Level, VSYNC, and LCD_SYSTEM.
>> +               but since the driver expects VSYNC to be the first IRQ,
>> +               make sure to mention order as VSYNC, FIFO Level and
>> LCD_SYSTEM
>> +               keeping VSYNC as first IRQ as shown below.
>> +               for example: interrupts =<11 1>,<11 0>,<11 2>;
>
>
> I have a suggestion here. What about using reg-names property to make this
> more explicit, e.g.
>
> - interrupts : should contain a list of all FIMD IP block interrupts:
>   FIFO Level, VSYNC, LCD_SYSTEM. The interrupt specifier format depends
>   on the interrupt controller used;
>
> - interrupt-names : should contain the interrupt names: "fifo", "vsync",
>   "lcd_sys", in the order in which they were listed in the interrupts
>   property;
>
Good idea, but i am just wondering is it a good idea to modify the
fimd driver ?

> Or something similar to that. Then the drivers would need to be modified
> to get interrupt resource by name, and not to rely on the VSYNC interrupt
> be listed as the first one.
>
>
>> +
>> +- pinctrl := property defining the pinctrl configurations with a phandle
>> +
>> +- pinctrl-names := name of the pinctrl
>
>
> Might be sufficient to just mention that the "default" state needs to be
> specified in the fimd node, with reference to the pinctrl bindings
> documentation, e.g.
>
> "The pinctrl bindings defined in ../../pinctrl/pinctrl-bindings.txt must be
> used to define a pinctrl state named "default".
>
> See https://patchwork.kernel.org/patch/2082311.
>
OK.
>
>> +Optional Properties:
>> +- samsung,power-domain := power domain property defined with a phandle
>
>
> "a phandle to FIMD power domain node" ?
OK
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/video/samsung-fimd.txt b/Documentation/devicetree/bindings/video/samsung-fimd.txt
new file mode 100644
index 0000000..8d201e7
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/samsung-fimd.txt
@@ -0,0 +1,54 @@ 
+Device-Tree bindings for Samsung SoC display controller (FIMD)
+
+FIMD stands for Fully Interactive Mobile Display, is the Display Controller for
+the Samsung series of SoCs which transfers the image data from a video buffer
+located in the system memory to an external LCD interface.
+
+Required properties:
+- compatible := value should be one of the following
+		"samsung,s3c2443-fimd"; /* for S3C24XX SoCs */
+		"samsung,s3c6400-fimd"; /* for S3C64XX SoCs */
+		"samsung,s5p6440-fimd"; /* for S5P64X0 SoCs */
+		"samsung,s5pc100-fimd"; /* for S5PC100 SoC  */
+		"samsung,s5pv210-fimd"; /* for S5PV210 SoC */
+		"samsung,exynos4210-fimd"; /* for Exynos4 SoCs */
+		"samsung,exynos5250-fimd"; /* for Exynos5 SoCs */
+
+- reg := physical base address of the fimd and length of memory mapped region
+
+- interrupt-parent := reference to the interrupt combiner node with phandle
+
+- interrupts := interrupt number from the combiner to the cpu.
+		We have 3 interrupts and the interrupt combiner order is
+		FIFO Level, VSYNC, and LCD_SYSTEM.
+		but since the driver expects VSYNC to be the first IRQ,
+		make sure to mention order as VSYNC, FIFO Level and LCD_SYSTEM
+		keeping VSYNC as first IRQ as shown below.
+		for example: interrupts = <11 1>, <11 0>, <11 2>;
+
+- pinctrl := property defining the pinctrl configurations with a phandle
+
+- pinctrl-names := name of the pinctrl
+
+Optional Properties:
+- samsung,power-domain := power domain property defined with a phandle
+
+Example:
+
+SoC specific DT Entry:
+
+	fimd@11c00000 {
+		compatible = "samsung,exynos4210-fimd";
+		interrupt-parent = <&combiner>;
+		reg = <0x11c00000 0x20000>;
+		interrupts = <11 1>, <11 0>, <11 2>;
+	};
+
+Board specific DT Entry:
+
+	fimd@11c00000 {
+		samsung,power-domain = <&pd_lcd0>;
+		pinctrl-0 = <&lcd_sync &lcd_clk &lcd_en &lcd0_data &pwm1_out>;
+		pinctrl-names = "default";
+		status = "okay";
+	};