Re: [PATCH 01/11] OMAP: DMA: Introduce DMA device attributes

["Cousson, Benoit" <b-cousson@ti.com>]

Hi Manjunatha,

I just have a couple of minor comments.

On 7/29/2010 11:58 AM, G, Manjunath Kondaiah wrote:
> This patches introduces OMAP DMA device attributes for using the same in
> DMA platform driver for all OMAP's and HWMOD database(OMAP2PLUS onwards)
>
> Signed-off-by: Manjunatha GK<manjugk@ti.com>
> ---
>   arch/arm/plat-omap/include/plat/dma.h |   22 ++++++++++++++++++++++
>   1 files changed, 22 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/plat-omap/include/plat/dma.h b/arch/arm/plat-omap/include/plat/dma.h
> index 02232ca..ff60f11 100644
> --- a/arch/arm/plat-omap/include/plat/dma.h
> +++ b/arch/arm/plat-omap/include/plat/dma.h
> @@ -398,6 +398,22 @@
>   #define DMA_CH_PRIO_HIGH		0x1
>   #define DMA_CH_PRIO_LOW			0x0 /* Def */
>
> +/* Attributes for OMAP DMA Contrllers */
> +#define ENABLE_1510_MODE		(1<<  0)
> +#define DMA_LINKED_LCH			(1<<  1)
> +#define GLOBAL_PRIORITY			(1<<  2)
> +#define RESERVE_CHANNEL			(1<<  3)
> +#define SRC_PORT			(2<<  3)
> +#define DST_PORT			(2<<  4)
> +#define IS_CSSA_32			(2<<  5)
> +#define IS_CDSA_32			(2<<  6)
> +#define SRC_INDEX			(4<<  6)
> +#define DST_INDEX			(4<<  7)
> +#define IS_BURST_ONLY4			(4<<  8)
> +#define CLEAR_CSR_ON_READ		(4<<  9)
> +#define IS_WORD_16			(8<<  9)
> +#define IS_RW_PRIORIY			(8<<  0xA)

It is a minor detail, but why are you shifting both side of the
operator? It is really confusing, cannot you just do 1 << X?
With 0 < X < 13.

> +
>   enum omap_dma_burst_mode {
>   	OMAP_DMA_DATA_BURST_DIS = 0,
>   	OMAP_DMA_DATA_BURST_4,
> @@ -463,6 +479,12 @@ struct omap_dma_channel_params {
>   #endif
>   };
>
> +struct omap_dma_dev_attr {
> +	u32 dma_dev_attr;

That name is not very meaningful, maybe dma_cap for capability or 
something else will be better.


> +	u16 dma_lch_count;
> +	u16 dma_chan_count;
> +	struct omap_dma_lch *dma_chan;

In general, I think that there are too many "dma_" prefix in that 
structure. It is already inside a dma_XXX struct so I don't think that 
adding an extra prefix to every attribute is needed.

Regards,
Benoit