Message ID | 1242468350-23190-2-git-send-email-felipe.contreras@gmail.com (mailing list archive) |
---|---|
State | Awaiting Upstream, archived |
Headers | show |
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
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.
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 --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;