Re: [PATCH v3 1/6] dts: exynos: add device tree support for exynos5 hdmi

Hi Rahul, Kgene,

On Wednesday 17 of October 2012 20:32:05 Kukjin Kim wrote:

Rahul Sharma wrote:

quoted

This patch adds support for device tree based discovery for exynos5
hdmi. Hdmi node is also renamed with "exynos5-hdmi".

Signed-off-by: Rahul Sharma <redacted>
---

 .../devicetree/bindings/drm/exynos/hdmi.txt        |   22

++++++++++++++++++++

 arch/arm/boot/dts/exynos5250-smdk5250.dts          |    4 +++
 arch/arm/boot/dts/exynos5250.dtsi                  |    6 +++++
 arch/arm/mach-exynos/include/mach/map.h            |    1 +
 arch/arm/mach-exynos/mach-exynos5-dt.c             |    2 +
 5 files changed, 35 insertions(+), 0 deletions(-)
 create mode 100644
 Documentation/devicetree/bindings/drm/exynos/hdmi.txt

[...]

quoted

+	hdmi {
+		compatible = "samsung,exynos5-hdmi";
+		reg = <0x14530000 0x100000>;
+		interrupts = <0 95 0>;
+		hpd-gpio = <&gpx3 7 0xf 1 3>;
+	};
\ No newline at end of file

What's this?

[...]

quoted

+	hdmi@14530000 {

+	hdmi {

Isn't there a convention to use @ADDR whenever a node contains a reg 
property? Please see exynos4.dtsi, all the nodes with reg have @ADDR.

quoted

+		hpd-gpio = <&gpx3 7 0xf 1 3>;
+	};

 };

diff --git a/arch/arm/boot/dts/exynos5250.dtsi

b/arch/arm/boot/dts/exynos5250.dtsi
index f69e389..ec7ea2d 100644

--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi

@@ -492,4 +492,10 @@

 			#gpio-cells = <4>;
 		
 		};
 	
 	};

+
+	hdmi@14530000 {

Ditto.

Ditto.

[...]

quoted

+#define EXYNOS5_PA_HDMI		0x14530000

I think, we don't need this because see below.

[...]

quoted

+	OF_DEV_AUXDATA("samsung,exynos5-hdmi", EXYNOS5_PA_HDMI,
+				"exynos5-hdmi", NULL),

+	OF_DEV_AUXDATA("samsung,exynos5-hdmi", 0x14530000, "exynos5-hdmi",
NULL),

I think, if the address as defined above is used only here, let's just
the value because nobody don't know it is the address.

I don't like the idea of having such magic numbers in code, but since the 
whole auxdata array is going to be removed in future anyway, I guess this 
is OK.

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center