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

[Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2327 bytes --]

Hi Rahul, Kgene,

On Wednesday 17 of October 2012 20:32:05 Kukjin Kim wrote:
> Rahul Sharma wrote:
> > 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 <rahul.sharma-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > ---
> > 
> >  .../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
> [...]
> 
> > +	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?
> 
> [...]
> 
> > +	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.

> > +		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.

> [...]
> 
> > +#define EXYNOS5_PA_HDMI		0x14530000
> 
> I think, we don't need this because see below.
> 
> [...]
> 
> > +	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

[-- Attachment #1.2: Type: text/html, Size: 16207 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss