[PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support

[PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support

[Laurent Pinchart]

Add a function to register the DU device with board-specific platform
data.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 arch/arm/mach-shmobile/include/mach/common.h |  1 +
 arch/arm/mach-shmobile/setup-r8a7779.c       | 30 ++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/arch/arm/mach-shmobile/include/mach/common.h b/arch/arm/mach-shmobile/include/mach/common.h
index 03f73de..4531872 100644
--- a/arch/arm/mach-shmobile/include/mach/common.h
+++ b/arch/arm/mach-shmobile/include/mach/common.h
@@ -68,6 +68,7 @@ extern void r8a7779_earlytimer_init(void);
 extern void r8a7779_add_early_devices(void);
 extern void r8a7779_add_standard_devices(void);
 extern void r8a7779_add_standard_devices_dt(void);
+extern void r8a7779_add_du_device(void *pdata);
 extern void r8a7779_clock_init(void);
 extern void r8a7779_pinmux_init(void);
 extern void r8a7779_pm_init(void);
diff --git a/arch/arm/mach-shmobile/setup-r8a7779.c b/arch/arm/mach-shmobile/setup-r8a7779.c
index a460ba3..2f817d8 100644
--- a/arch/arm/mach-shmobile/setup-r8a7779.c
+++ b/arch/arm/mach-shmobile/setup-r8a7779.c
@@ -393,6 +393,29 @@ static struct platform_device sata_device = {
 	},
 };
 
+static struct resource du_resources[] = {
+	[0] = {
+		.name	= "Display Unit",
+		.start	= 0xfff80000,
+		.end	= 0xfffb1007,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= gic_iid(0x3f),
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device du_device = {
+	.name		= "rcar-du",
+	.num_resources	= ARRAY_SIZE(du_resources),
+	.resource	= du_resources,
+	.id		= 0,
+	.dev	= {
+		.coherent_dma_mask = ~0,
+	},
+};
+
 static struct platform_device *r8a7779_devices_dt[] __initdata = {
 	&scif0_device,
 	&scif1_device,
@@ -428,6 +451,13 @@ void __init r8a7779_add_standard_devices(void)
 			    ARRAY_SIZE(r8a7779_late_devices));
 }
 
+void __init r8a7779_add_du_device(void *pdata)
+{
+	du_device.dev.platform_data = pdata;
+
+	platform_device_register(&du_device);
+}
+
 /* do nothing for !CONFIG_SMP or !CONFIG_HAVE_TWD */
 void __init __weak r8a7779_register_twd(void) { }
 
-- 
1.8.1.5

Re: [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support

[Sergei Shtylyov]

Hello.

On 27-03-2013 23:21, Laurent Pinchart wrote:

> Add a function to register the DU device with board-specific platform
> data.

> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>   arch/arm/mach-shmobile/include/mach/common.h |  1 +
>   arch/arm/mach-shmobile/setup-r8a7779.c       | 30 ++++++++++++++++++++++++++++
>   2 files changed, 31 insertions(+)

> diff --git a/arch/arm/mach-shmobile/include/mach/common.h b/arch/arm/mach-shmobile/include/mach/common.h
> index 03f73de..4531872 100644
> --- a/arch/arm/mach-shmobile/include/mach/common.h
> +++ b/arch/arm/mach-shmobile/include/mach/common.h
> @@ -68,6 +68,7 @@ extern void r8a7779_earlytimer_init(void);
>   extern void r8a7779_add_early_devices(void);
>   extern void r8a7779_add_standard_devices(void);
>   extern void r8a7779_add_standard_devices_dt(void);
> +extern void r8a7779_add_du_device(void *pdata);

    Maybe r8a7779_add_du0_device()?

> diff --git a/arch/arm/mach-shmobile/setup-r8a7779.c b/arch/arm/mach-shmobile/setup-r8a7779.c
> index a460ba3..2f817d8 100644
> --- a/arch/arm/mach-shmobile/setup-r8a7779.c
> +++ b/arch/arm/mach-shmobile/setup-r8a7779.c
> @@ -393,6 +393,29 @@ static struct platform_device sata_device = {
[...]
> +static struct platform_device du_device = {
> +	.name		= "rcar-du",
> +	.num_resources	= ARRAY_SIZE(du_resources),
> +	.resource	= du_resources,
> +	.id		= 0,

    You probably should have written in the subject "add DU0 support".

> +	.dev	= {
> +		.coherent_dma_mask = ~0,

    Use DMA_BIT_MASK() please.

WBR, Sergei

Re: [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support

[Laurent Pinchart]

Hi Sergei,

On Thursday 28 March 2013 16:46:01 Sergei Shtylyov wrote:
> On 27-03-2013 23:21, Laurent Pinchart wrote:
> > Add a function to register the DU device with board-specific platform
> > data.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >   arch/arm/mach-shmobile/include/mach/common.h |  1 +
> >   arch/arm/mach-shmobile/setup-r8a7779.c       | 30 ++++++++++++++++++++++
> >   2 files changed, 31 insertions(+)
> > 
> > diff --git a/arch/arm/mach-shmobile/include/mach/common.h
> > b/arch/arm/mach-shmobile/include/mach/common.h index 03f73de..4531872
> > 100644
> > --- a/arch/arm/mach-shmobile/include/mach/common.h
> > +++ b/arch/arm/mach-shmobile/include/mach/common.h
> > @@ -68,6 +68,7 @@ extern void r8a7779_earlytimer_init(void);
> > 
> >   extern void r8a7779_add_early_devices(void);
> >   extern void r8a7779_add_standard_devices(void);
> >   extern void r8a7779_add_standard_devices_dt(void);
> > +extern void r8a7779_add_du_device(void *pdata);
> 
>     Maybe r8a7779_add_du0_device()?

There's a single DU device on the r8a7779, with two display outputs named DU0 
and DU1. I agree that's a bit confusing.

> > diff --git a/arch/arm/mach-shmobile/setup-r8a7779.c
> > b/arch/arm/mach-shmobile/setup-r8a7779.c index a460ba3..2f817d8 100644
> > --- a/arch/arm/mach-shmobile/setup-r8a7779.c
> > +++ b/arch/arm/mach-shmobile/setup-r8a7779.c
> > @@ -393,6 +393,29 @@ static struct platform_device sata_device = {
> 
> [...]
> 
> > +static struct platform_device du_device = {
> > +	.name		= "rcar-du",
> > +	.num_resources	= ARRAY_SIZE(du_resources),
> > +	.resource	= du_resources,
> > +	.id		= 0,
> 
>     You probably should have written in the subject "add DU0 support".
> 
> > +	.dev	= {
> > +		.coherent_dma_mask = ~0,
> 
>     Use DMA_BIT_MASK() please.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support

[Sergei Shtylyov]

Hello.

On 03/28/2013 05:42 PM, Laurent Pinchart wrote:

>>> Add a function to register the DU device with board-specific platform
>>> data.
>>>
>>> Signed-off-by: Laurent Pinchart
>>> <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>>
>>>    arch/arm/mach-shmobile/include/mach/common.h |  1 +
>>>    arch/arm/mach-shmobile/setup-r8a7779.c       | 30 ++++++++++++++++++++++
>>>    2 files changed, 31 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-shmobile/include/mach/common.h
>>> b/arch/arm/mach-shmobile/include/mach/common.h index 03f73de..4531872
>>> 100644
>>> --- a/arch/arm/mach-shmobile/include/mach/common.h
>>> +++ b/arch/arm/mach-shmobile/include/mach/common.h
>>> @@ -68,6 +68,7 @@ extern void r8a7779_earlytimer_init(void);
>>>
>>>    extern void r8a7779_add_early_devices(void);
>>>    extern void r8a7779_add_standard_devices(void);
>>>    extern void r8a7779_add_standard_devices_dt(void);
>>> +extern void r8a7779_add_du_device(void *pdata);
>>      Maybe r8a7779_add_du0_device()?
> There's a single DU device on the r8a7779, with two display outputs named DU0
> and DU1. I agree that's a bit confusing.
>
>>> diff --git a/arch/arm/mach-shmobile/setup-r8a7779.c
>>> b/arch/arm/mach-shmobile/setup-r8a7779.c index a460ba3..2f817d8 100644
>>> --- a/arch/arm/mach-shmobile/setup-r8a7779.c
>>> +++ b/arch/arm/mach-shmobile/setup-r8a7779.c
>>> @@ -393,6 +393,29 @@ static struct platform_device sata_device = {
>> [...]
>>
>>> +static struct platform_device du_device = {
>>> +	.name		= "rcar-du",
>>> +	.num_resources	= ARRAY_SIZE(du_resources),
>>> +	.resource	= du_resources,
>>> +	.id		= 0,
>>      You probably should have written in the subject "add DU0 support".

     If there's only single display device, why you're not using 'id' of -1?

WBR, Sergei

Re: [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support

[Laurent Pinchart]

Hi Sergei,

On Thursday 28 March 2013 21:45:49 Sergei Shtylyov wrote:
> On 03/28/2013 05:42 PM, Laurent Pinchart wrote:
> >>> Add a function to register the DU device with board-specific platform
> >>> data.
> >>> 
> >>> Signed-off-by: Laurent Pinchart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >>> ---
> >>> 
> >>>    arch/arm/mach-shmobile/include/mach/common.h |  1 +
> >>>    arch/arm/mach-shmobile/setup-r8a7779.c       | 30
> >>>    ++++++++++++++++++++++
> >>>    2 files changed, 31 insertions(+)
> >>> 
> >>> diff --git a/arch/arm/mach-shmobile/include/mach/common.h
> >>> b/arch/arm/mach-shmobile/include/mach/common.h index 03f73de..4531872
> >>> 100644
> >>> --- a/arch/arm/mach-shmobile/include/mach/common.h
> >>> +++ b/arch/arm/mach-shmobile/include/mach/common.h
> >>> @@ -68,6 +68,7 @@ extern void r8a7779_earlytimer_init(void);
> >>> 
> >>>    extern void r8a7779_add_early_devices(void);
> >>>    extern void r8a7779_add_standard_devices(void);
> >>>    extern void r8a7779_add_standard_devices_dt(void);
> >>> 
> >>> +extern void r8a7779_add_du_device(void *pdata);
> >>> 
> >>      Maybe r8a7779_add_du0_device()?
> > 
> > There's a single DU device on the r8a7779, with two display outputs named
> > DU0 and DU1. I agree that's a bit confusing.
> > 
> >>> diff --git a/arch/arm/mach-shmobile/setup-r8a7779.c
> >>> b/arch/arm/mach-shmobile/setup-r8a7779.c index a460ba3..2f817d8 100644
> >>> --- a/arch/arm/mach-shmobile/setup-r8a7779.c
> >>> +++ b/arch/arm/mach-shmobile/setup-r8a7779.c
> >>> @@ -393,6 +393,29 @@ static struct platform_device sata_device = {
> >> 
> >> [...]
> >> 
> >>> +static struct platform_device du_device = {
> >>> +	.name		= "rcar-du",
> >>> +	.num_resources	= ARRAY_SIZE(du_resources),
> >>> +	.resource	= du_resources,
> >>> +	.id		= 0,
> >>> 
> >>      You probably should have written in the subject "add DU0 support".
> 
> If there's only single display device, why you're not using 'id' of -1?

Good point. I'll fix that.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support

[Sergei Shtylyov]

Hello.

On 04/04/2013 06:54 PM, Laurent Pinchart wrote:

>>>>> Add a function to register the DU device with board-specific platform
>>>>> data.
>>>>>
>>>>> Signed-off-by: Laurent Pinchart
>>>>> <laurent.pinchart+renesas@ideasonboard.com>
[...]
>>>
>>>>> diff --git a/arch/arm/mach-shmobile/setup-r8a7779.c
>>>>> b/arch/arm/mach-shmobile/setup-r8a7779.c index a460ba3..2f817d8 100644
>>>>> --- a/arch/arm/mach-shmobile/setup-r8a7779.c
>>>>> +++ b/arch/arm/mach-shmobile/setup-r8a7779.c
>>>>> @@ -393,6 +393,29 @@ static struct platform_device sata_device = {
>>>> [...]
>>>>
>>>>> +static struct platform_device du_device = {
>>>>> +	.name		= "rcar-du",
>>>>> +	.num_resources	= ARRAY_SIZE(du_resources),
>>>>> +	.resource	= du_resources,
>>>>> +	.id		= 0,
>>>>>
>>>>       You probably should have written in the subject "add DU0 support".
>> If there's only single display device, why you're not using 'id' of -1?
> Good point. I'll fix that.
>

    Note that the DU clock entry already has "rcar-du.0".

WBR, Sergei

Re: [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support

[Sergei Shtylyov]

Hello.

On 03/27/2013 10:21 PM, Laurent Pinchart wrote:

> Add a function to register the DU device with board-specific platform
> data.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
[...]
> diff --git a/arch/arm/mach-shmobile/setup-r8a7779.c b/arch/arm/mach-shmobile/setup-r8a7779.c
> index a460ba3..2f817d8 100644
> --- a/arch/arm/mach-shmobile/setup-r8a7779.c
> +++ b/arch/arm/mach-shmobile/setup-r8a7779.c
[...]
> @@ -428,6 +451,13 @@ void __init r8a7779_add_standard_devices(void)
>   			    ARRAY_SIZE(r8a7779_late_devices));
>   }
>   
> +void __init r8a7779_add_du_device(void *pdata)

    Just to repeat the criticism that your patch have somehow eluded
(but mine 2, with analogous approach, has not :-), you can't have 'void *'
here -- you should declare the function with the actual platform data
type pointer as an argument.

> +{
> +	du_device.dev.platform_data = pdata;
> +
> +	platform_device_register(&du_device);

    That function should also be avoided like plague (according to
Greg KH and Arnd Bergmann). I suggest you use

platform_device_register_resndata() instead, it's more preferable along with
all the platform_device_register_*() family.

WBR, Sergei

Re: [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support

[Laurent Pinchart]

Hi Sergei,

On Thursday 04 April 2013 21:27:46 Sergei Shtylyov wrote:
> Hello.
> 
> On 04/04/2013 06:54 PM, Laurent Pinchart wrote:
> >>>>> Add a function to register the DU device with board-specific platform
> >>>>> data.
> >>>>> 
> >>>>> Signed-off-by: Laurent Pinchart
> >>>>> <laurent.pinchart+renesas@ideasonboard.com>
> 
> [...]
> 
> >>>>> diff --git a/arch/arm/mach-shmobile/setup-r8a7779.c
> >>>>> b/arch/arm/mach-shmobile/setup-r8a7779.c index a460ba3..2f817d8 100644
> >>>>> --- a/arch/arm/mach-shmobile/setup-r8a7779.c
> >>>>> +++ b/arch/arm/mach-shmobile/setup-r8a7779.c
> >>>>> @@ -393,6 +393,29 @@ static struct platform_device sata_device = {
> >>>> 
> >>>> [...]
> >>>> 
> >>>>> +static struct platform_device du_device = {
> >>>>> +	.name		= "rcar-du",
> >>>>> +	.num_resources	= ARRAY_SIZE(du_resources),
> >>>>> +	.resource	= du_resources,
> >>>>> +	.id		= 0,
> >>>>> 
> >>>>       You probably should have written in the subject "add DU0
> >>>>       support".
> >> 
> >> If there's only single display device, why you're not using 'id' of -1?
> > 
> > Good point. I'll fix that.
> 
> Note that the DU clock entry already has "rcar-du.0".

I know. That's why I wasn't that keen on id = -1 in the first place :-) I'll 
send a patch.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support

[Laurent Pinchart]

Hi Sergei,

On Thursday 04 April 2013 23:17:59 Sergei Shtylyov wrote:
> On 03/27/2013 10:21 PM, Laurent Pinchart wrote:
> > Add a function to register the DU device with board-specific platform
> > data.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> 
> [...]
> 
> > diff --git a/arch/arm/mach-shmobile/setup-r8a7779.c
> > b/arch/arm/mach-shmobile/setup-r8a7779.c index a460ba3..2f817d8 100644
> > --- a/arch/arm/mach-shmobile/setup-r8a7779.c
> > +++ b/arch/arm/mach-shmobile/setup-r8a7779.c
> 
> [...]
> 
> > @@ -428,6 +451,13 @@ void __init r8a7779_add_standard_devices(void)
> >   			    ARRAY_SIZE(r8a7779_late_devices));
> >   }
> > 
> > +void __init r8a7779_add_du_device(void *pdata)
> 
> Just to repeat the criticism that your patch have somehow eluded (but mine
> 2, with analogous approach, has not :-), you can't have 'void *' here -- you
> should declare the function with the actual platform data type pointer as an
> argument.

OK I'll fix that.

> > +{
> > +	du_device.dev.platform_data = pdata;
> > +
> > +	platform_device_register(&du_device);
> 
> That function should also be avoided like plague (according to Greg KH and
> Arnd Bergmann). I suggest you use
> 
> platform_device_register_resndata() instead, it's more preferable along with
> all the platform_device_register_*() family.

OK.

-- 
Regards,

Laurent Pinchart