Re: [PATCH v3 08/13] OMAP1: DMA: Introduce DMA driver as platform device

[Kevin Hilman <khilman@deeprootsystems.com>]

"G, Manjunath Kondaiah" <manjugk@ti.com> writes:

> Register OMAP1 DMA driver as platform device and add support
> for registering through platform device layer using resource
> structures.
>
> Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
>  arch/arm/mach-omap1/dma.c              |  182 ++++++++++++++++++++++++++++++++
>  arch/arm/mach-omap1/include/mach/dma.h |   29 +++++
>  2 files changed, 211 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-omap1/dma.c
>  create mode 100644 arch/arm/mach-omap1/include/mach/dma.h
>
> diff --git a/arch/arm/mach-omap1/dma.c b/arch/arm/mach-omap1/dma.c
> new file mode 100644
> index 0000000..e756069
> --- /dev/null
> +++ b/arch/arm/mach-omap1/dma.c
> @@ -0,0 +1,182 @@
> +/*
> + * dma.c - OMAP1/OMAP7xx-specific DMA code
> + *
> + * Copyright (C) 2003 - 2008 Nokia Corporation
> + * Author: Juha Yrjölä <juha.yrjola@nokia.com>
> + * DMA channel linking for 1610 by Samuel Ortiz <samuel.ortiz@nokia.com>
> + * Graphics DMA and LCD DMA graphics tranformations
> + * by Imre Deak <imre.deak@nokia.com>
> + * OMAP2/3 support Copyright (C) 2004-2007 Texas Instruments, Inc.
> + * Some functions based on earlier dma-omap.c Copyright (C) 2001 RidgeRun, Inc.
> + *
> + * Copyright (C) 2010 Texas Instruments, Inc.
> + * Converted DMA library into platform driver
> + *                   - G, Manjunath Kondaiah <manjugk@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/pm_runtime.h>

no pm_runtime API usage in this patch.    Please add only when needed.

> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/sched.h>

why is sched.h needed?

> +#include <linux/spinlock.h>

ditto

> +#include <linux/errno.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/device.h>

When creating a new file like this, it's important to only include
headers that are actually used/needed.

> +#include <plat/dma.h>
> +#include <plat/tc.h>
> +
> +#define OMAP1_DMA_BASE			(0xfffed800)
> +
> +static struct resource res[] __initdata = {
> +	[0] = {
> +		.start	= OMAP1_DMA_BASE,
> +		.end	= OMAP1_DMA_BASE + SZ_2K - 1,
> +		.flags	= IORESOURCE_MEM,
> +	},
> +	[1] = {
> +		.name   = "0",
> +		.start  = INT_DMA_CH0_6,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +	[2] = {
> +		.name   = "1",
> +		.start  = INT_DMA_CH1_7,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +	[3] = {
> +		.name   = "2",
> +		.start  = INT_DMA_CH2_8,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +	[4] = {
> +		.name   = "3",
> +		.start  = INT_DMA_CH3,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +	[5] = {
> +		.name   = "4",
> +		.start  = INT_DMA_CH4,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +	[6] = {
> +		.name   = "5",
> +		.start  = INT_DMA_CH5,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +	[7] = {
> +		.name   = "6",
> +		.start  = INT_DMA_LCD,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +	/* irq's for omap16xx and omap7xx */
> +	[8] = {
> +		.name   = "7",
> +		.start  = 53 + IH2_BASE,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +	[9] = {
> +		.name   = "8",
> +		.start  = 54 + IH2_BASE,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +	[10] = {
> +		.name  = "9",
> +		.start = 55 + IH2_BASE,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +	[11] = {
> +		.name  = "10",
> +		.start = 56 + IH2_BASE,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +	[12] = {
> +		.name  = "11",
> +		.start = 57 + IH2_BASE,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +	[13] = {
> +		.name  = "12",
> +		.start = 58 + IH2_BASE,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +	[14] = {
> +		.name  = "13",
> +		.start = 59 + IH2_BASE,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +	[15] = {
> +		.name  = "14",
> +		.start = 60 + IH2_BASE,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +	[16] = {
> +		.name  = "15",
> +		.start = 61 + IH2_BASE,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +	[17] = {
> +		.name  = "16",
> +		.start = 62 + IH2_BASE,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +};
> +
> +static int __init omap1_system_dma_init(void)
> +{
> +	struct omap_system_dma_plat_info	*p;
> +	struct platform_device			*pdev;
> +	int ret;
> +
> +	pdev = platform_device_alloc("omap_dma_system", 0);
> +	if (!pdev) {
> +		pr_err("%s: Unable to device alloc for dma\n",
> +			__func__);
> +		return -ENOMEM;
> +	}
> +
> +	ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
> +	if (ret) {
> +		pr_err("%s: Unable to add resources for %s%d\n",
> +			__func__, pdev->name, pdev->id);
> +		goto exit_device_del;
> +	}
> +
> +	p = kzalloc(sizeof(struct omap_system_dma_plat_info), GFP_KERNEL);
> +	if (!p) {
> +		dev_err(&pdev->dev, "%s: Unable to allocate 'p' for %s\n",
> +			__func__, pdev->name);
> +		ret = -ENOMEM;
> +		goto exit_device_put;
> +	}
> +
> +	ret = platform_device_add_data(pdev, p, sizeof(*p));
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: Unable to add resources for %s%d\n",
> +			__func__, pdev->name, pdev->id);
> +		goto exit_device_put;
> +	}
> +	ret = platform_device_add(pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: Unable to add resources for %s%d\n",
> +			__func__, pdev->name, pdev->id);
> +		goto exit_device_put;
> +	}
> +	return ret;
> +
> +exit_device_put:
> +	platform_device_put(pdev);
> +exit_device_del:
> +	platform_device_del(pdev);
> +
> +	return ret;
> +}
> +arch_initcall(omap1_system_dma_init);
> diff --git a/arch/arm/mach-omap1/include/mach/dma.h b/arch/arm/mach-omap1/include/mach/dma.h
> new file mode 100644
> index 0000000..7949e3f
> --- /dev/null
> +++ b/arch/arm/mach-omap1/include/mach/dma.h
> @@ -0,0 +1,29 @@
> +/*
> + *  OMAP DMA controller register offsets.
> + *
> + *  Copyright (C) 2003 Nokia Corporation
> + *  Author: Juha Yrjölä <juha.yrjola@nokia.com>
> + *
> + *  Copyright (C) 2010 Texas Instruments
> + *  Converted DMA library into platform driver
> + *       by G, Manjunath Kondaiah <manjugk@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +#ifndef __ASM_ARCH_OMAP1_DMA_H
> +#define __ASM_ARCH_OMAP1_DMA_H
> +
> +#endif /* __ASM_ARCH_OMAP1_DMA_H */

Please don't create an empty header.  Just create it in the series when
(if) it's needed.  Same goes for next patch.

I see you populate mach/dma.h later in the series, but I don't
understand why those values need to be in the header.   More comments on
those patches...

Kevin


--
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