* Re: [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support
2013-03-27 19:21 [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support Laurent Pinchart
@ 2013-03-28 12:46 ` Sergei Shtylyov
2013-03-28 14:42 ` Laurent Pinchart
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Sergei Shtylyov @ 2013-03-28 12:46 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support
2013-03-27 19:21 [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support Laurent Pinchart
2013-03-28 12:46 ` Sergei Shtylyov
@ 2013-03-28 14:42 ` Laurent Pinchart
2013-03-28 17:44 ` Sergei Shtylyov
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2013-03-28 14:42 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support
2013-03-27 19:21 [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support Laurent Pinchart
2013-03-28 12:46 ` Sergei Shtylyov
2013-03-28 14:42 ` Laurent Pinchart
@ 2013-03-28 17:44 ` Sergei Shtylyov
2013-04-04 14:54 ` Laurent Pinchart
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Sergei Shtylyov @ 2013-03-28 17:44 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support
2013-03-27 19:21 [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support Laurent Pinchart
` (2 preceding siblings ...)
2013-03-28 17:44 ` Sergei Shtylyov
@ 2013-04-04 14:54 ` Laurent Pinchart
2013-04-04 17:27 ` Sergei Shtylyov
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2013-04-04 14:54 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support
2013-03-27 19:21 [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support Laurent Pinchart
` (3 preceding siblings ...)
2013-04-04 14:54 ` Laurent Pinchart
@ 2013-04-04 17:27 ` Sergei Shtylyov
2013-04-04 19:17 ` Sergei Shtylyov
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Sergei Shtylyov @ 2013-04-04 17:27 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support
2013-03-27 19:21 [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support Laurent Pinchart
` (4 preceding siblings ...)
2013-04-04 17:27 ` Sergei Shtylyov
@ 2013-04-04 19:17 ` Sergei Shtylyov
2013-04-04 23:52 ` Laurent Pinchart
2013-04-05 0:02 ` Laurent Pinchart
7 siblings, 0 replies; 9+ messages in thread
From: Sergei Shtylyov @ 2013-04-04 19:17 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support
2013-03-27 19:21 [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support Laurent Pinchart
` (5 preceding siblings ...)
2013-04-04 19:17 ` Sergei Shtylyov
@ 2013-04-04 23:52 ` Laurent Pinchart
2013-04-05 0:02 ` Laurent Pinchart
7 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2013-04-04 23:52 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support
2013-03-27 19:21 [PATCH v2 1/2] ARM: mach-shmobile: r8a7779: Add DU support Laurent Pinchart
` (6 preceding siblings ...)
2013-04-04 23:52 ` Laurent Pinchart
@ 2013-04-05 0:02 ` Laurent Pinchart
7 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2013-04-05 0:02 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 9+ messages in thread