diff mbox

[v2,1/7] ARM: dts: Move display-timimg information inside FIMD DT node for exynos5250

Message ID 1375183178-8201-2-git-send-email-vikas.sajjan@linaro.org
State Accepted
Commit d1bf2abe3a17a9f7ce5e1e04d439cdfc34fb98d0
Headers show

Commit Message

Vikas C Sajjan July 30, 2013, 11:19 a.m. UTC
As the display-timing information is parsed by FIMD driver, it makes
sense to move the display-timimg DT node inside FIMD DT node for exynos5250

Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
---
 arch/arm/boot/dts/exynos5250-smdk5250.dts |   29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

Comments

Tomasz Figa Aug. 1, 2013, 2:49 p.m. UTC | #1
Hi Vikas,

On Tuesday 30 of July 2013 16:49:32 Vikas Sajjan wrote:
> As the display-timing information is parsed by FIMD driver, it makes
> sense to move the display-timimg DT node inside FIMD DT node for
> exynos5250
> 
> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
> ---
>  arch/arm/boot/dts/exynos5250-smdk5250.dts |   29
> ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13
> deletions(-)
> 
> diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts
> b/arch/arm/boot/dts/exynos5250-smdk5250.dts index 49f18c2..d176dbb
> 100644
> --- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
> +++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
> @@ -262,19 +262,22 @@
>  		pinctrl-0 = <&dp_hpd>;
>  	};
> 
> -	display-timings {
> -		native-mode = <&timing0>;
> -		timing0: timing@0 {
> -			/* 1280x800 */
> -			clock-frequency = <50000>;
> -			hactive = <1280>;
> -			vactive = <800>;
> -			hfront-porch = <4>;
> -			hback-porch = <4>;
> -			hsync-len = <4>;
> -			vback-porch = <4>;
> -			vfront-porch = <4>;
> -			vsync-len = <4>;
> +	fimd@14400000 {
> +		status = "okay";
> +		display-timings {
> +			native-mode = <&timing0>;
> +			timing0: timing@0 {
> +				/* 1280x800 */
> +				clock-frequency = <50000>;
> +				hactive = <1280>;
> +				vactive = <800>;
> +				hfront-porch = <4>;
> +				hback-porch = <4>;
> +				hsync-len = <4>;
> +				vback-porch = <4>;
> +				vfront-porch = <4>;
> +				vsync-len = <4>;
> +			};
>  		};
>  	};

Looks good to me, but I would like some other people from the device tree 
mailing list to comment on node naming: Do we want to conform to the 
recommendation of ePAPR about node naming, which states that node names 
should be generic, not platform specific or we are free to ignore it?

Best regards,
Tomasz
Vikas Sajjan Aug. 5, 2013, 10:04 a.m. UTC | #2
Hi All,

On Thu, Aug 1, 2013 at 8:19 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Vikas,
>
> On Tuesday 30 of July 2013 16:49:32 Vikas Sajjan wrote:
>> As the display-timing information is parsed by FIMD driver, it makes
>> sense to move the display-timimg DT node inside FIMD DT node for
>> exynos5250
>>
>> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
>> ---
>>  arch/arm/boot/dts/exynos5250-smdk5250.dts |   29
>> ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13
>> deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts
>> b/arch/arm/boot/dts/exynos5250-smdk5250.dts index 49f18c2..d176dbb
>> 100644
>> --- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
>> +++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
>> @@ -262,19 +262,22 @@
>>               pinctrl-0 = <&dp_hpd>;
>>       };
>>
>> -     display-timings {
>> -             native-mode = <&timing0>;
>> -             timing0: timing@0 {
>> -                     /* 1280x800 */
>> -                     clock-frequency = <50000>;
>> -                     hactive = <1280>;
>> -                     vactive = <800>;
>> -                     hfront-porch = <4>;
>> -                     hback-porch = <4>;
>> -                     hsync-len = <4>;
>> -                     vback-porch = <4>;
>> -                     vfront-porch = <4>;
>> -                     vsync-len = <4>;
>> +     fimd@14400000 {
>> +             status = "okay";
>> +             display-timings {
>> +                     native-mode = <&timing0>;
>> +                     timing0: timing@0 {
>> +                             /* 1280x800 */
>> +                             clock-frequency = <50000>;
>> +                             hactive = <1280>;
>> +                             vactive = <800>;
>> +                             hfront-porch = <4>;
>> +                             hback-porch = <4>;
>> +                             hsync-len = <4>;
>> +                             vback-porch = <4>;
>> +                             vfront-porch = <4>;
>> +                             vsync-len = <4>;
>> +                     };
>>               };
>>       };
>
> Looks good to me, but I would like some other people from the device tree
> mailing list to comment on node naming: Do we want to conform to the
> recommendation of ePAPR about node naming, which states that node names
> should be generic, not platform specific or we are free to ignore it?
>

Any more comments on this series.

> Best regards,
> Tomasz
>
> --
> 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
Sachin Kamat Aug. 5, 2013, 10:29 a.m. UTC | #3
On 5 August 2013 15:34, Vikas Sajjan <sajjan.linux@gmail.com> wrote:
> Hi All,
>
> On Thu, Aug 1, 2013 at 8:19 PM, Tomasz Figa <t.figa@samsung.com> wrote:
>> Hi Vikas,
>>
>> On Tuesday 30 of July 2013 16:49:32 Vikas Sajjan wrote:
>>> As the display-timing information is parsed by FIMD driver, it makes
>>> sense to move the display-timimg DT node inside FIMD DT node for
>>> exynos5250
>>>
>>> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
>>> ---
>>>  arch/arm/boot/dts/exynos5250-smdk5250.dts |   29
>>> ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13
>>> deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts
>>> b/arch/arm/boot/dts/exynos5250-smdk5250.dts index 49f18c2..d176dbb
>>> 100644
>>> --- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
>>> +++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
>>> @@ -262,19 +262,22 @@
>>>               pinctrl-0 = <&dp_hpd>;
>>>       };
>>>
>>> -     display-timings {
>>> -             native-mode = <&timing0>;
>>> -             timing0: timing@0 {
>>> -                     /* 1280x800 */
>>> -                     clock-frequency = <50000>;
>>> -                     hactive = <1280>;
>>> -                     vactive = <800>;
>>> -                     hfront-porch = <4>;
>>> -                     hback-porch = <4>;
>>> -                     hsync-len = <4>;
>>> -                     vback-porch = <4>;
>>> -                     vfront-porch = <4>;
>>> -                     vsync-len = <4>;
>>> +     fimd@14400000 {
>>> +             status = "okay";
>>> +             display-timings {
>>> +                     native-mode = <&timing0>;
>>> +                     timing0: timing@0 {
>>> +                             /* 1280x800 */
>>> +                             clock-frequency = <50000>;
>>> +                             hactive = <1280>;
>>> +                             vactive = <800>;
>>> +                             hfront-porch = <4>;
>>> +                             hback-porch = <4>;
>>> +                             hsync-len = <4>;
>>> +                             vback-porch = <4>;
>>> +                             vfront-porch = <4>;
>>> +                             vsync-len = <4>;
>>> +                     };
>>>               };
>>>       };
>>
>> Looks good to me, but I would like some other people from the device tree
>> mailing list to comment on node naming: Do we want to conform to the
>> recommendation of ePAPR about node naming, which states that node names
>> should be generic, not platform specific or we are free to ignore it?
>>
>
> Any more comments on this series.

One trivial typo in patch subject and commit message:
s/timimg/timing
Vikas Sajjan Aug. 12, 2013, 6:57 a.m. UTC | #4
Hi Kukjin,

On Mon, Aug 5, 2013 at 3:59 PM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> On 5 August 2013 15:34, Vikas Sajjan <sajjan.linux@gmail.com> wrote:
>> Hi All,
>>
>> On Thu, Aug 1, 2013 at 8:19 PM, Tomasz Figa <t.figa@samsung.com> wrote:
>>> Hi Vikas,
>>>
>>> On Tuesday 30 of July 2013 16:49:32 Vikas Sajjan wrote:
>>>> As the display-timing information is parsed by FIMD driver, it makes
>>>> sense to move the display-timimg DT node inside FIMD DT node for
>>>> exynos5250
>>>>
>>>> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
>>>> ---
>>>>  arch/arm/boot/dts/exynos5250-smdk5250.dts |   29
>>>> ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13
>>>> deletions(-)
>>>>
>>>> diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts
>>>> b/arch/arm/boot/dts/exynos5250-smdk5250.dts index 49f18c2..d176dbb
>>>> 100644
>>>> --- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
>>>> +++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
>>>> @@ -262,19 +262,22 @@
>>>>               pinctrl-0 = <&dp_hpd>;
>>>>       };
>>>>
>>>> -     display-timings {
>>>> -             native-mode = <&timing0>;
>>>> -             timing0: timing@0 {
>>>> -                     /* 1280x800 */
>>>> -                     clock-frequency = <50000>;
>>>> -                     hactive = <1280>;
>>>> -                     vactive = <800>;
>>>> -                     hfront-porch = <4>;
>>>> -                     hback-porch = <4>;
>>>> -                     hsync-len = <4>;
>>>> -                     vback-porch = <4>;
>>>> -                     vfront-porch = <4>;
>>>> -                     vsync-len = <4>;
>>>> +     fimd@14400000 {
>>>> +             status = "okay";
>>>> +             display-timings {
>>>> +                     native-mode = <&timing0>;
>>>> +                     timing0: timing@0 {
>>>> +                             /* 1280x800 */
>>>> +                             clock-frequency = <50000>;
>>>> +                             hactive = <1280>;
>>>> +                             vactive = <800>;
>>>> +                             hfront-porch = <4>;
>>>> +                             hback-porch = <4>;
>>>> +                             hsync-len = <4>;
>>>> +                             vback-porch = <4>;
>>>> +                             vfront-porch = <4>;
>>>> +                             vsync-len = <4>;
>>>> +                     };
>>>>               };
>>>>       };
>>>
>>> Looks good to me, but I would like some other people from the device tree
>>> mailing list to comment on node naming: Do we want to conform to the
>>> recommendation of ePAPR about node naming, which states that node names
>>> should be generic, not platform specific or we are free to ignore it?
>>>
>>
>> Any more comments on this series.
>
> One trivial typo in patch subject and commit message:
> s/timimg/timing
>

Can you apply this series (by correcting typo s/timimg/timing) OR you
want me to respin V3 with this typo correction.


>
>
> --
> With warm regards,
> Sachin
Kukjin Kim Aug. 14, 2013, 5:56 a.m. UTC | #5
Tomasz Figa wrote:
> 
> Hi Vikas,
> 
> On Tuesday 30 of July 2013 16:49:32 Vikas Sajjan wrote:
> > As the display-timing information is parsed by FIMD driver, it makes
> > sense to move the display-timimg DT node inside FIMD DT node for
> > exynos5250
> >
> > Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
> > ---
> >  arch/arm/boot/dts/exynos5250-smdk5250.dts |   29
> > ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13
> > deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts
> > b/arch/arm/boot/dts/exynos5250-smdk5250.dts index 49f18c2..d176dbb
> > 100644
> > --- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
> > +++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
> > @@ -262,19 +262,22 @@
> >  		pinctrl-0 = <&dp_hpd>;
> >  	};
> >
> > -	display-timings {
> > -		native-mode = <&timing0>;
> > -		timing0: timing@0 {
> > -			/* 1280x800 */
> > -			clock-frequency = <50000>;
> > -			hactive = <1280>;
> > -			vactive = <800>;
> > -			hfront-porch = <4>;
> > -			hback-porch = <4>;
> > -			hsync-len = <4>;
> > -			vback-porch = <4>;
> > -			vfront-porch = <4>;
> > -			vsync-len = <4>;
> > +	fimd@14400000 {
> > +		status = "okay";
> > +		display-timings {
> > +			native-mode = <&timing0>;
> > +			timing0: timing@0 {
> > +				/* 1280x800 */
> > +				clock-frequency = <50000>;
> > +				hactive = <1280>;
> > +				vactive = <800>;
> > +				hfront-porch = <4>;
> > +				hback-porch = <4>;
> > +				hsync-len = <4>;
> > +				vback-porch = <4>;
> > +				vfront-porch = <4>;
> > +				vsync-len = <4>;
> > +			};
> >  		};
> >  	};
> 
> Looks good to me, but I would like some other people from the device tree
> mailing list to comment on node naming: Do we want to conform to the
> recommendation of ePAPR about node naming, which states that node names
> should be generic, not platform specific or we are free to ignore it?
>
The name of node looks good to me and the name had been being used for a
long time, and if we change that now, that will cause the bigger confusion.

- Kukjin
Tomasz Figa Aug. 14, 2013, 8:16 a.m. UTC | #6
On Wednesday 14 of August 2013 14:56:55 Kukjin Kim wrote:
> Tomasz Figa wrote:
> > Hi Vikas,
> > 
> > On Tuesday 30 of July 2013 16:49:32 Vikas Sajjan wrote:
> > > As the display-timing information is parsed by FIMD driver, it makes
> > > sense to move the display-timimg DT node inside FIMD DT node for
> > > exynos5250
> > > 
> > > Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
> > > ---
> > > 
> > >  arch/arm/boot/dts/exynos5250-smdk5250.dts |   29
> > > 
> > > ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13
> > > deletions(-)
> > > 
> > > diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts
> > > b/arch/arm/boot/dts/exynos5250-smdk5250.dts index 49f18c2..d176dbb
> > > 100644
> > > --- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
> > > +++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
> > > @@ -262,19 +262,22 @@
> > > 
> > >  		pinctrl-0 = <&dp_hpd>;
> > >  	
> > >  	};
> > > 
> > > -	display-timings {
> > > -		native-mode = <&timing0>;
> > > -		timing0: timing@0 {
> > > -			/* 1280x800 */
> > > -			clock-frequency = <50000>;
> > > -			hactive = <1280>;
> > > -			vactive = <800>;
> > > -			hfront-porch = <4>;
> > > -			hback-porch = <4>;
> > > -			hsync-len = <4>;
> > > -			vback-porch = <4>;
> > > -			vfront-porch = <4>;
> > > -			vsync-len = <4>;
> > > +	fimd@14400000 {
> > > +		status = "okay";
> > > +		display-timings {
> > > +			native-mode = <&timing0>;
> > > +			timing0: timing@0 {
> > > +				/* 1280x800 */
> > > +				clock-frequency = <50000>;
> > > +				hactive = <1280>;
> > > +				vactive = <800>;
> > > +				hfront-porch = <4>;
> > > +				hback-porch = <4>;
> > > +				hsync-len = <4>;
> > > +				vback-porch = <4>;
> > > +				vfront-porch = <4>;
> > > +				vsync-len = <4>;
> > > +			};
> > > 
> > >  		};
> > >  	
> > >  	};
> > 
> > Looks good to me, but I would like some other people from the device
> > tree mailing list to comment on node naming: Do we want to conform to
> > the recommendation of ePAPR about node naming, which states that node
> > names should be generic, not platform specific or we are free to
> > ignore it?
> The name of node looks good to me and the name had been being used for a
> long time

Like a lot of things used in device trees of ARM platforms, since we did 
not have any proper processes defined for creating device tree bindings 
and device trees themselves, including any recommended set of good 
practices.

We are now trying to define all of this and so it's time to rethink all 
the DT related things that has been added for ARM platforms since the time 
DT for ARM was introduced and make sure that everything is reasonable.

> , and if we change that now, that will cause the bigger
> confusion.

Still, I think this patch can go as is and node naming can be fixed in 
separate patches, including any other quirks of Exynos device trees.

So, for the whole series:

Reviewed-by: Tomasz Figa <t.figa@samsung.com>

Best regards,
Tomasz
Vikas Sajjan Aug. 14, 2013, 8:40 a.m. UTC | #7
Hi Tomasz,

On Wed, Aug 14, 2013 at 1:46 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On Wednesday 14 of August 2013 14:56:55 Kukjin Kim wrote:
>> Tomasz Figa wrote:
>> > Hi Vikas,
>> >
>> > On Tuesday 30 of July 2013 16:49:32 Vikas Sajjan wrote:
>> > > As the display-timing information is parsed by FIMD driver, it makes
>> > > sense to move the display-timimg DT node inside FIMD DT node for
>> > > exynos5250
>> > >
>> > > Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
>> > > ---
>> > >
>> > >  arch/arm/boot/dts/exynos5250-smdk5250.dts |   29
>> > >
>> > > ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13
>> > > deletions(-)
>> > >
>> > > diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts
>> > > b/arch/arm/boot/dts/exynos5250-smdk5250.dts index 49f18c2..d176dbb
>> > > 100644
>> > > --- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
>> > > +++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
>> > > @@ -262,19 +262,22 @@
>> > >
>> > >           pinctrl-0 = <&dp_hpd>;
>> > >
>> > >   };
>> > >
>> > > - display-timings {
>> > > -         native-mode = <&timing0>;
>> > > -         timing0: timing@0 {
>> > > -                 /* 1280x800 */
>> > > -                 clock-frequency = <50000>;
>> > > -                 hactive = <1280>;
>> > > -                 vactive = <800>;
>> > > -                 hfront-porch = <4>;
>> > > -                 hback-porch = <4>;
>> > > -                 hsync-len = <4>;
>> > > -                 vback-porch = <4>;
>> > > -                 vfront-porch = <4>;
>> > > -                 vsync-len = <4>;
>> > > + fimd@14400000 {
>> > > +         status = "okay";
>> > > +         display-timings {
>> > > +                 native-mode = <&timing0>;
>> > > +                 timing0: timing@0 {
>> > > +                         /* 1280x800 */
>> > > +                         clock-frequency = <50000>;
>> > > +                         hactive = <1280>;
>> > > +                         vactive = <800>;
>> > > +                         hfront-porch = <4>;
>> > > +                         hback-porch = <4>;
>> > > +                         hsync-len = <4>;
>> > > +                         vback-porch = <4>;
>> > > +                         vfront-porch = <4>;
>> > > +                         vsync-len = <4>;
>> > > +                 };
>> > >
>> > >           };
>> > >
>> > >   };
>> >
>> > Looks good to me, but I would like some other people from the device
>> > tree mailing list to comment on node naming: Do we want to conform to
>> > the recommendation of ePAPR about node naming, which states that node
>> > names should be generic, not platform specific or we are free to
>> > ignore it?
>> The name of node looks good to me and the name had been being used for a
>> long time
>
> Like a lot of things used in device trees of ARM platforms, since we did
> not have any proper processes defined for creating device tree bindings
> and device trees themselves, including any recommended set of good
> practices.
>
> We are now trying to define all of this and so it's time to rethink all
> the DT related things that has been added for ARM platforms since the time
> DT for ARM was introduced and make sure that everything is reasonable.
>
>> , and if we change that now, that will cause the bigger
>> confusion.
>
> Still, I think this patch can go as is and node naming can be fixed in
> separate patches, including any other quirks of Exynos device trees.
>
> So, for the whole series:
>
> Reviewed-by: Tomasz Figa <t.figa@samsung.com>
>

Thanks for the review.


> Best regards,
> Tomasz
>
> --
> 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
diff mbox

Patch

diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts b/arch/arm/boot/dts/exynos5250-smdk5250.dts
index 49f18c2..d176dbb 100644
--- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
+++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
@@ -262,19 +262,22 @@ 
 		pinctrl-0 = <&dp_hpd>;
 	};
 
-	display-timings {
-		native-mode = <&timing0>;
-		timing0: timing@0 {
-			/* 1280x800 */
-			clock-frequency = <50000>;
-			hactive = <1280>;
-			vactive = <800>;
-			hfront-porch = <4>;
-			hback-porch = <4>;
-			hsync-len = <4>;
-			vback-porch = <4>;
-			vfront-porch = <4>;
-			vsync-len = <4>;
+	fimd@14400000 {
+		status = "okay";
+		display-timings {
+			native-mode = <&timing0>;
+			timing0: timing@0 {
+				/* 1280x800 */
+				clock-frequency = <50000>;
+				hactive = <1280>;
+				vactive = <800>;
+				hfront-porch = <4>;
+				hback-porch = <4>;
+				hsync-len = <4>;
+				vback-porch = <4>;
+				vfront-porch = <4>;
+				vsync-len = <4>;
+			};
 		};
 	};