RE: [PATCH v3 08/13] OMAP1: DMA: Introduce DMA driver as platform device
> -Original Message- > From: Kevin Hilman [mailto:khil...@deeprootsystems.com] > Sent: Wednesday, November 10, 2010 3:54 AM > To: G, Manjunath Kondaiah > Cc: linux-omap@vger.kernel.org; > linux-arm-ker...@lists.infradead.org; Cousson, Benoit; > Shilimkar, Santosh > Subject: Re: [PATCH v3 08/13] OMAP1: DMA: Introduce DMA > driver as platform device > > "G, Manjunath Kondaiah" 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 > > Cc: Benoit Cousson > > Cc: Kevin Hilman > > Cc: Santosh Shilimkar > > --- > > 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 000..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ä > > + * DMA channel linking for 1610 by Samuel Ortiz > > > + * Graphics DMA and LCD DMA graphics tranformations > > + * by Imre Deak > > + * 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 > > + * > > + * 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 > > +#include > > +#include > > no pm_runtime API usage in this patch.Please add only when needed. > > > +#include > > +#include > > +#include > > +#include > > why is sched.h needed? > > > +#include > > ditto > > > +#include > > +#include > > +#include > > +#include > > When creating a new file like this, it's important to only include > headers that are actually used/needed. Ok. I will check these headers. It was copy paste from old dma driver. > > > +#include > > +#include > > + [...] > > 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 000..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ä > > + * > > + * Copyright (C) 2010 Texas Instruments > > + * Converted DMA library into platform driver > > + * by G, Manjunath Kondaiah > > + * > > + * 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. ok. -Manjunath > > 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 > > >
Re: [PATCH v3 08/13] OMAP1: DMA: Introduce DMA driver as platform device
"G, Manjunath Kondaiah" 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
> Cc: Benoit Cousson
> Cc: Kevin Hilman
> Cc: Santosh Shilimkar
> ---
> 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 000..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ä
> + * DMA channel linking for 1610 by Samuel Ortiz
> + * Graphics DMA and LCD DMA graphics tranformations
> + * by Imre Deak
> + * 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
> + *
> + * 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
> +#include
> +#include
no pm_runtime API usage in this patch.Please add only when needed.
> +#include
> +#include
> +#include
> +#include
why is sched.h needed?
> +#include
ditto
> +#include
> +#include
> +#include
> +#include
When creating a new file like this, it's important to only include
headers that are actually used/needed.
> +#include
> +#include
> +
> +#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\
