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

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

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.