Re: [PATCH 4/7] ARM: dts: Add FIMD DT node to exynos5420 DTS files

[Tomasz Figa <tomasz.figa@gmail.com>]

Hi Vikas,

Please see my comments inline.

On Monday 29 of July 2013 19:19:28 Vikas Sajjan wrote:
> Adds FIMD DT node to exynos5420 based SMDK. Also adds display-timimg
> information node.
> 
> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
> ---
>  arch/arm/boot/dts/exynos5420-smdk5420.dts |   18 ++++++++++++++++++
>  arch/arm/boot/dts/exynos5420.dtsi         |    8 ++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos5420-smdk5420.dts
> b/arch/arm/boot/dts/exynos5420-smdk5420.dts index 08607df..7c2f477 100644
> --- a/arch/arm/boot/dts/exynos5420-smdk5420.dts
> +++ b/arch/arm/boot/dts/exynos5420-smdk5420.dts
> @@ -30,4 +30,22 @@
>  			clock-frequency = <24000000>;
>  		};
>  	};
> +
> +	fimd {
> +		display-timings {
> +			native-mode = <&timing0>;
> +			timing0: timing@0 {
> +				clock-frequency = <50000>;
> +				hactive = <2560>;
> +				vactive = <1600>;
> +				hfront-porch = <48>;
> +				hback-porch = <80>;
> +				hsync-len = <32>;
> +				vback-porch = <16>;
> +				vfront-porch = <8>;
> +				vsync-len = <6>;
> +			};
> +		};
> +	};
> +
>  };
> diff --git a/arch/arm/boot/dts/exynos5420.dtsi
> b/arch/arm/boot/dts/exynos5420.dtsi index bd6b310..1f659f4 100644
> --- a/arch/arm/boot/dts/exynos5420.dtsi
> +++ b/arch/arm/boot/dts/exynos5420.dtsi
> @@ -180,4 +180,12 @@
>  		clocks = <&clock 260>, <&clock 131>;
>  		clock-names = "uart", "clk_uart_baud0";
>  	};
> +
> +	fimd {
> +		samsung,power-domain = <&disp_pd>;
> +		clocks = <&clock 147>, <&clock 421>;
> +		clock-names = "sclk_fimd", "fimd";
> +		status = "okay";

FIMD can't operate without display timings, so status = "okay" doesn't 
represent real device state here. Please move status override to board dts.

I know that dtsi files of Exynos 5 SoCs have "okay" status as default, but this 
is broken and needs to be fixed, because it moves the responsibility of knowing 
about all SoC hardware to board dts.

Generally, the ePAPR document, chapter 2.3.4, specifies following definition of 
status property:

	The status property indicates the operational status of a device. Valid 
	values are listed and defined in the following table.

The table specifies following values:

	“okay”
	Indicates the device is operational

	“disabled”
	Indicates that the device is not presently operational, but it might become 
	operational in the future (for example, something is not plugged in, or 		
	switched off). Refer to the device binding for details on what disabled 
	means for a given device.

	“fail”
	Indicates that the device is not operational. A serious error was detected 
	in the device, and it is unlikely to become operational without repair.

	“fail-sss”
	Indicates that the device is not operational. A serious error was detected 	
	in the device and it is unlikely to become operational without repair. The 
	sss portion of the value is specific to the device and indicates the error 
	condition detected.

Without all required properties the device will not be operational, so its 
status should be set to "disabled" and then overriden to "okay" when missing 
properties are provided (e.g. in board dts).

Best regards,
Tomasz

> +	};
> +
>  };