diff mbox

[RFC/PATCH,1/3] omap3-iommu: reorganize

Message ID 1242468350-23190-2-git-send-email-felipe.contreras@gmail.com (mailing list archive)
State Awaiting Upstream, archived
Headers show

Commit Message

Felipe Contreras May 16, 2009, 10:05 a.m. UTC 
From: Felipe Contreras <felipe.contreras@nokia.com>

No functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@nokia.com>
---
 arch/arm/mach-omap2/omap3-iommu.c |   72 ++++++++++++++++++------------------
 1 files changed, 36 insertions(+), 36 deletions(-)

Comments

Russell King - ARM Linux May 18, 2009, 12:07 p.m. UTC | #1 
On Sat, May 16, 2009 at 01:05:48PM +0300, Felipe Contreras wrote:
> +struct iommu_device {
> +	resource_size_t base;
> +	resource_size_t irq;
> +	struct iommu_platform_data pdata;
> +	struct resource res[2];
>  };

The data which is needed per device is:

- base address
- IRQ (no need for this to be resource_size_t - int will do)
- platform data

There's no need for that res[2] being there.

> @@ -63,23 +51,35 @@ static int __init omap3_iommu_init(void)
>  
>  	for (i = 0; i < NR_IOMMU_DEVICES; i++) {
>  		struct platform_device *pdev;
> +		const struct iommu_device *d = &devices[i];
> +		struct resource res[] = {
> +			{ .flags = IORESOURCE_MEM },
> +			{ .flags = IORESOURCE_IRQ },
> +		};

This initialization doesn't buy you anything, in fact quite the opposite.
The compiler actually interprets this as:

static struct resource __initial_res[] = {
	{ .flags = IORESOURCE_MEM },
	{ .flags = IORESOURCE_IRQ },
};
...
	for () {
		struct resource res[...];
		memcpy(res, __initial_res, sizeof(__initial_res));

> +
> +		res[0].start = d->base;
> +		res[0].end = d->base + MMU_REG_SIZE - 1;
> +
> +		res[1].start = d->irq;

It would be far better to initialize the flags element here for both.
And please also set res[1].end as I did in my patch.

> +
> +		err = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
>  		if (err)
>  			goto err_out;
> -		err = platform_device_add_data(pdev, &omap3_iommu_pdata[i],
> -					       sizeof(omap3_iommu_pdata[0]));
> +
> +		err = platform_device_add_data(pdev, &d->pdata, sizeof(d->pdata));

This will fail checkpatch.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Contreras May 18, 2009, 12:59 p.m. UTC | #2 
On Mon, May 18, 2009 at 3:07 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, May 16, 2009 at 01:05:48PM +0300, Felipe Contreras wrote:
>> +struct iommu_device {
>> +     resource_size_t base;
>> +     resource_size_t irq;
>> +     struct iommu_platform_data pdata;
>> +     struct resource res[2];
>>  };
>
> The data which is needed per device is:
>
> - base address
> - IRQ (no need for this to be resource_size_t - int will do)
> - platform data
>
> There's no need for that res[2] being there.

Right, I was using 'res' but not any more; I forgot to remove it.

So resource_size_t is ok for 'base'?

>> @@ -63,23 +51,35 @@ static int __init omap3_iommu_init(void)
>>
>>       for (i = 0; i < NR_IOMMU_DEVICES; i++) {
>>               struct platform_device *pdev;
>> +             const struct iommu_device *d = &devices[i];
>> +             struct resource res[] = {
>> +                     { .flags = IORESOURCE_MEM },
>> +                     { .flags = IORESOURCE_IRQ },
>> +             };
>
> This initialization doesn't buy you anything, in fact quite the opposite.
> The compiler actually interprets this as:
>
> static struct resource __initial_res[] = {
>        { .flags = IORESOURCE_MEM },
>        { .flags = IORESOURCE_IRQ },
> };
> ...
>        for () {
>                struct resource res[...];
>                memcpy(res, __initial_res, sizeof(__initial_res));
>
>> +
>> +             res[0].start = d->base;
>> +             res[0].end = d->base + MMU_REG_SIZE - 1;
>> +
>> +             res[1].start = d->irq;
>
> It would be far better to initialize the flags element here for both.
> And please also set res[1].end as I did in my patch.

I think I saw that res initialization somewhere and I found it much
easier to read.

Ok. Will do.

>> +
>> +             err = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
>>               if (err)
>>                       goto err_out;
>> -             err = platform_device_add_data(pdev, &omap3_iommu_pdata[i],
>> -                                            sizeof(omap3_iommu_pdata[0]));
>> +
>> +             err = platform_device_add_data(pdev, &d->pdata, sizeof(d->pdata));
>
> This will fail checkpatch.

I'll make sure it passes.

Cheers.
Russell King - ARM Linux May 18, 2009, 1:03 p.m. UTC | #3 
On Mon, May 18, 2009 at 03:59:42PM +0300, Felipe Contreras wrote:
> So resource_size_t is ok for 'base'?

Yes.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" 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/mach-omap2/omap3-iommu.c b/arch/arm/mach-omap2/omap3-iommu.c
index 367f36a..f4232ec 100644
--- a/arch/arm/mach-omap2/omap3-iommu.c
+++ b/arch/arm/mach-omap2/omap3-iommu.c
@@ -14,43 +14,31 @@ 
 
 #include <mach/iommu.h>
 
-#define OMAP3_MMU1_BASE	0x480bd400
-#define OMAP3_MMU2_BASE	0x5d000000
-#define OMAP3_MMU1_IRQ	24
-#define OMAP3_MMU2_IRQ	28
-
-static struct resource omap3_iommu_res[] = {
-	{ /* Camera ISP MMU */
-		.start		= OMAP3_MMU1_BASE,
-		.end		= OMAP3_MMU1_BASE + MMU_REG_SIZE - 1,
-		.flags		= IORESOURCE_MEM,
-	},
-	{
-		.start		= OMAP3_MMU1_IRQ,
-		.flags		= IORESOURCE_IRQ,
-	},
-	{ /* IVA2.2 MMU */
-		.start		= OMAP3_MMU2_BASE,
-		.end		= OMAP3_MMU2_BASE + MMU_REG_SIZE - 1,
-		.flags		= IORESOURCE_MEM,
-	},
-	{
-		.start		= OMAP3_MMU2_IRQ,
-		.flags		= IORESOURCE_IRQ,
-	},
+struct iommu_device {
+	resource_size_t base;
+	resource_size_t irq;
+	struct iommu_platform_data pdata;
+	struct resource res[2];
 };
-#define NR_IOMMU_RES (ARRAY_SIZE(omap3_iommu_res) / 2)
 
-static const struct iommu_platform_data omap3_iommu_pdata[] __initconst = {
+static struct iommu_device devices[] = {
 	{
-		.name = "isp",
-		.nr_tlb_entries = 8,
-		.clk_name = "cam_ick",
+		.base = 0x480bd400,
+		.irq = 24,
+		.pdata = {
+			.name = "isp",
+			.nr_tlb_entries = 8,
+			.clk_name = "cam_ick",
+		},
 	},
 	{
-		.name = "iva2",
-		.nr_tlb_entries = 32,
-		.clk_name = "iva2_ck",
+		.base = 0x5d000000,
+		.irq = 28,
+		.pdata = {
+			.name = "iva2",
+			.nr_tlb_entries = 32,
+			.clk_name = "iva2_ck",
+		},
 	},
 };
 #define NR_IOMMU_DEVICES ARRAY_SIZE(omap3_iommu_pdata)
@@ -63,23 +51,35 @@  static int __init omap3_iommu_init(void)
 
 	for (i = 0; i < NR_IOMMU_DEVICES; i++) {
 		struct platform_device *pdev;
+		const struct iommu_device *d = &devices[i];
+		struct resource res[] = {
+			{ .flags = IORESOURCE_MEM },
+			{ .flags = IORESOURCE_IRQ },
+		};
 
 		pdev = platform_device_alloc("omap-iommu", i + 1);
 		if (!pdev) {
 			err = -ENOMEM;
 			goto err_out;
 		}
-		err = platform_device_add_resources(pdev,
-				    &omap3_iommu_res[2 * i], NR_IOMMU_RES);
+
+		res[0].start = d->base;
+		res[0].end = d->base + MMU_REG_SIZE - 1;
+
+		res[1].start = d->irq;
+
+		err = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
 		if (err)
 			goto err_out;
-		err = platform_device_add_data(pdev, &omap3_iommu_pdata[i],
-					       sizeof(omap3_iommu_pdata[0]));
+
+		err = platform_device_add_data(pdev, &d->pdata, sizeof(d->pdata));
 		if (err)
 			goto err_out;
+
 		err = platform_device_add(pdev);
 		if (err)
 			goto err_out;
+
 		omap3_iommu_pdev[i] = pdev;
 	}
 	return 0;