Re: [PATCH] OMAP3: mailbox initialization for all omap versions

[Felipe Balbi <felipe.balbi@nokia.com>]

hi,

On Mon, Mar 29, 2010 at 08:40:17AM +0200, Doyu Hiroshi (Nokia-D/Helsinki) wrote:
>> From 47783fc3e030d4e49d20ba412661cc1f38d8aec0 Mon Sep 17 00:00:00 2001
>> From: Fernando Guzman Lugo <x0095840@ti.com>
>> Date: Fri, 26 Mar 2010 13:06:32 -0600
>> Subject: [PATCH] OMAP3: mailbox initialization for all omap versions
>>
>> This removes the check for omap2420, omap3430, omap44xx
>> and initialize mailbox for all versions
>>
>> Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
>
>It seems that I missed one point previously when I commented. I have
>just now found that "multi omap arch support" for mailbox is broken at
>the previous commit:454bf340c986b798cd4c2fd676caffa2c1e61482
>
>Considering to fix the above to keep "multi omap arch support", "omap
>cpu type check" is necessary here. Could this be fixed as something below?
>
>#if defined(CONFIG_ARCH_OMAP2)
>static struct resource omap2_mbox_resources[] = {
>       ...
>};
>#endif
>
>#if defined(CONFIG_ARCH_OMAP3)
>static struct resource omap3_mbox_resources[] = {
>       ...
>};
>#endif
>
>#if defined(CONFIG_ARCH_OMAP4)
>static struct resource omap4_mbox_resources[] = {
>       ...
>};
>#endif
>
>static struct platform_device mbox_device = {
>	.name		= "omap2-mailbox",
>	.id		= -1,
>};
>
>static inline void omap_init_mbox(void)
>{
>#if defined(CONFIG_ARCH_OMAP2)
>	if (cpu_is_omap2420()) {
>		mbox_device.num_resources = ARRAY_SIZE(omap2_mbox_resources);
>		mbox_device.resource = omap2_mbox_resources;
>	}
>#endif
>#if defined(CONFIG_ARCH_OMAP3)
>	if (cpu_is_omap3430()) {
>		mbox_device.num_resources = ARRAY_SIZE(omap3_mbox_resources);
>		mbox_device.resource = omap3_mbox_resources;
>	}
>#endif
>#if defined(CONFIG_ARCH_OMAP4)
>	if (cpu_is_omap44xx()) {
>		mbox_device.num_resources = ARRAY_SIZE(omap4_mbox_resources);
>		mbox_device.resource = omap4_mbox_resources;
>	}
>#endif
>	if (!mbox_device.resource) {
>		pr_err("%s: platform not supported\n", __func__);
>		return;
>	}
>	platform_device_register(&mbox_device);
>}

in that case, wouldn't it be better to split that into 
arch/arm/omap1/mbox.c arch/arm/omap2/mbox24xx.c 
arch/arm/omap2/mbox34xx.c arch/arm/omap2/mbox44xx.c ??

that way we don't need ifdefs on the code and we will only compile what 
we really need.

just my 2 cents.

-- 
balbi