Re: [PATCH 6/6] omap: iommu: code reorganization and cleanup

[Felipe Contreras <felipe.contreras@xxxxxxxxx>]

On Sat, Nov 6, 2010 at 3:19 AM, Omar Ramirez Luna <omar.ramirez@xxxxxx> wrote:
> Since omap-iommu is now using hwmod, there are structures and
> arrays that can be cleaned up to increase the readability of
> the code.
>
> Signed-off-by: Omar Ramirez Luna <omar.ramirez@xxxxxx>

NAK.

> ---
> Âarch/arm/mach-omap2/omap-iommu.c    Â|  95 +++++++++++--------------------
> Âarch/arm/plat-omap/include/plat/iommu.h | Â Â2 +-
> Â2 files changed, 34 insertions(+), 63 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
> index 0a76bce..135474b 100644
> --- a/arch/arm/mach-omap2/omap-iommu.c
> +++ b/arch/arm/mach-omap2/omap-iommu.c
> @@ -17,53 +17,17 @@
> Â#include <plat/omap_hwmod.h>
> Â#include <plat/omap_device.h>
>
> -struct iommu_device {
> - Â Â Â struct iommu_platform_data pdata;
> +static char *omap3_devices[] = {

Shouldn't this be __init?

> + Â Â Â "isp",

#if defined(CONFIG_MPU_BRIDGE_IOMMU)

> + Â Â Â "iva2",

#endif

> + Â Â Â NULL,
> Â};

If you want to get rid of CONFIG_MPU_BRIDGE_IOMMU, do it in a separate patch.

> -static int num_iommu_devices;
> -
> -#ifdef CONFIG_ARCH_OMAP3
> -static struct iommu_device omap3_devices[] = {
> - Â Â Â {
> - Â Â Â Â Â Â Â .pdata = {
> - Â Â Â Â Â Â Â Â Â Â Â .name = "isp",
> - Â Â Â Â Â Â Â },
> - Â Â Â },
> -#if defined(CONFIG_MPU_BRIDGE_IOMMU)
> - Â Â Â {
> - Â Â Â Â Â Â Â .pdata = {
> - Â Â Â Â Â Â Â Â Â Â Â .name = "iva2",
> - Â Â Â Â Â Â Â },
> - Â Â Â },
> -#endif
> -};
> -#define NR_OMAP3_IOMMU_DEVICES ARRAY_SIZE(omap3_devices)
> -#else
> -#define omap3_devices     ÂNULL
> -#define NR_OMAP3_IOMMU_DEVICES 0
> -#endif
> -
> -#ifdef CONFIG_ARCH_OMAP4
> -static struct iommu_device omap4_devices[] = {
> - Â Â Â {
> - Â Â Â Â Â Â Â .pdata = {
> - Â Â Â Â Â Â Â Â Â Â Â .name = "ducati",
> - Â Â Â Â Â Â Â },
> - Â Â Â },
> -#if defined(CONFIG_MPU_TESLA_IOMMU)
> - Â Â Â {
> - Â Â Â Â Â Â Â .pdata = {
> - Â Â Â Â Â Â Â Â Â Â Â .name = "tesla",
> - Â Â Â Â Â Â Â },
> - Â Â Â },
> -#endif
> +
> +static char *omap4_devices[] = {
> + Â Â Â "ducati",
> + Â Â Â "tesla",
> + Â Â Â NULL,
> Â};
> -#define NR_OMAP4_IOMMU_DEVICES ARRAY_SIZE(omap4_devices)
> -#else
> -#define omap4_devices     ÂNULL
> -#define NR_OMAP4_IOMMU_DEVICES 0
> -#endif
>
> Âstatic struct omap_device_pm_latency iommu_latencies[] = {
> Â Â Â Â[0] = {
> @@ -73,36 +37,28 @@ static struct omap_device_pm_latency iommu_latencies[] = {
> Â Â Â Â},
> Â};
>
> -static int __init omap_iommu_init(void)
> +static int __init omap_iommu_add(char **devices)
> Â{
> Â Â Â Âint i;
>
> - Â Â Â if (cpu_is_omap34xx()) {
> - Â Â Â Â Â Â Â devices = omap3_devices;
> - Â Â Â Â Â Â Â num_iommu_devices = NR_OMAP3_IOMMU_DEVICES;
> - Â Â Â } else if (cpu_is_omap44xx()) {
> - Â Â Â Â Â Â Â devices = omap4_devices;
> - Â Â Â Â Â Â Â num_iommu_devices = NR_OMAP4_IOMMU_DEVICES;
> - Â Â Â } else
> - Â Â Â Â Â Â Â return -ENODEV;
> -
> - Â Â Â for (i = 0; i < num_iommu_devices; i++) {
> + Â Â Â for (i = 0; devices[i]; i++) {
> Â Â Â Â Â Â Â Âstruct omap_hwmod *oh;
> Â Â Â Â Â Â Â Âstruct omap_device *od;
> + Â Â Â Â Â Â Â struct iommu_platform_data pdata;
>
> - Â Â Â Â Â Â Â oh = omap_hwmod_lookup(devices[i].pdata.name);
> + Â Â Â Â Â Â Â oh = omap_hwmod_lookup(devices[i]);
> Â Â Â Â Â Â Â Âif (!oh) {
> Â Â Â Â Â Â Â Â Â Â Â Âpr_err("%s: hwmod not found\n", __func__);
> Â Â Â Â Â Â Â Â Â Â Â Âreturn -ENODEV;
> Â Â Â Â Â Â Â Â}
>
> - Â Â Â Â Â Â Â devices[i].pdata.mmu_attr =
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (struct omap_mmu_dev_attr *)oh->dev_attr;
> - Â Â Â Â Â Â Â devices[i].pdata.device_enable = omap_device_enable;
> - Â Â Â Â Â Â Â devices[i].pdata.device_disable = omap_device_idle;
> + Â Â Â Â Â Â Â pdata.name = devices[i];
> + Â Â Â Â Â Â Â pdata.mmu_attr = (struct omap_mmu_dev_attr *)oh->dev_attr;

No need for casting on a 'void *'.

> + Â Â Â Â Â Â Â pdata.device_enable = omap_device_enable;
> + Â Â Â Â Â Â Â pdata.device_disable = omap_device_idle;
>
> Â Â Â Â Â Â Â Âod = omap_device_build("omap-iommu", i, oh,
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â &devices[i].pdata, sizeof(devices[i].pdata),
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â &pdata, sizeof(pdata),
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âiommu_latencies, ARRAY_SIZE(iommu_latencies),
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â0);
> Â Â Â Â Â Â Â Âif (!od) {
> @@ -110,8 +66,23 @@ static int __init omap_iommu_init(void)
> Â Â Â Â Â Â Â Â Â Â Â Âreturn -EPERM;
> Â Â Â Â Â Â Â Â}
> Â Â Â Â}
> +
> Â Â Â Âreturn 0;
> Â}
> +
> +static int __init omap_iommu_init(void)
> +{
> + Â Â Â int err;
> +
> + Â Â Â if (cpu_is_omap34xx())
> + Â Â Â Â Â Â Â err = omap_iommu_add(omap3_devices);
> + Â Â Â else if (cpu_is_omap44xx())
> + Â Â Â Â Â Â Â err = omap_iommu_add(omap4_devices);
> + Â Â Â else
> + Â Â Â Â Â Â Â return -ENODEV;
> +
> + Â Â Â return err;
> +}

I don't see what's the point of having a separate omap_iommu_add. The
code of omap_iommu_add() can be moved to the end of omap_iommu_init()
very easily.

Cheers.

-- 
Felipe Contreras
ÿô.nlj·Ÿ®‰­†+%ŠË±é¥Šwÿº{.nlj·¥Š{±þ‰š§ø¡Ü}©ž²ÆzÚj:+v‰¨þø®w¥þŠàÞ¨è&¢)ß«a¶Úÿûz¹ÞúŽŠÝjÿŠwèf