[U-Boot] [PATCH 04/11] Blackfin: bf60x: add dma support

[Wolfgang Denk]

Dear Sonic Zhang,

In message <1360223258-6945-5-git-send-email-sonic.adi at gmail.com> you wrote:
> From: Bob Liu <lliubbo at gmail.com>
> 
> Add dma support for bf60x.
> 
> Signed-off-by: Bob Liu <lliubbo at gmail.com>
> Signed-off-by: Sonic Zhang <sonic.zhang at analog.com>
> ---
>  arch/blackfin/include/asm/dma.h                  |  113 ++++++++++++----------
>  arch/blackfin/include/asm/mach-common/bits/dma.h |   48 +++++++++-
>  arch/blackfin/lib/string.c                       |   97 ++++++++++---------
>  3 files changed, 159 insertions(+), 99 deletions(-)

There are checkpatch errors and warnings.  Please fix these!!


>  struct dmasg_large {
>  	void *next_desc_addr;
> -	unsigned long start_addr;
> -	unsigned short cfg;
> -	unsigned short x_count;
> -	short x_modify;
> -	unsigned short y_count;
> -	short y_modify;
> +	u32 start_addr;
> +	u16 cfg;
> +	u16 x_count;
> +	s16 x_modify;
> +	u16 y_count;
> +	s16 y_modify;
>  } __attribute__((packed));
>  
>  struct dmasg {
> -	unsigned long start_addr;
> -	unsigned short cfg;
> -	unsigned short x_count;
> -	short x_modify;
> -	unsigned short y_count;
> -	short y_modify;
> +	u32 start_addr;
> +	u16 cfg;
> +	u16 x_count;
> +	s16 x_modify;
> +	u16 y_count;
> +	s16 y_modify;
>  } __attribute__((packed));

These appear totally unrelated changes.  These should be split out
into separate patches.

> +/*
> + * All Blackfin system MMRs are padded to 32bits even if the register
> + * itself is only 16bits.  So use a helper macro to streamline this.
> + */
> +#define __BFP(m) m; u16 __pad_##m
>  struct dma_register {
> -	void *next_desc_ptr;	/* DMA Next Descriptor Pointer register */
> -	unsigned long start_addr;	/* DMA Start address  register */
> -
> -	unsigned short cfg;	/* DMA Configuration register */
> -	unsigned short dummy1;	/* DMA Configuration register */
> -
> -	unsigned long reserved;
> -
> -	unsigned short x_count;	/* DMA x_count register */
> -	unsigned short dummy2;
> -
> -	short x_modify;	/* DMA x_modify register */
> -	unsigned short dummy3;
> -
> -	unsigned short y_count;	/* DMA y_count register */
> -	unsigned short dummy4;
> -
> -	short y_modify;	/* DMA y_modify register */
> -	unsigned short dummy5;
> -
> -	void *curr_desc_ptr;	/* DMA Current Descriptor Pointer
> -					   register */
> -	unsigned long curr_addr_ptr;	/* DMA Current Address Pointer
> -						   register */
> -	unsigned short irq_status;	/* DMA irq status register */
> -	unsigned short dummy6;
> -
> -	unsigned short peripheral_map;	/* DMA peripheral map register */
> -	unsigned short dummy7;
> -
> -	unsigned short curr_x_count;	/* DMA Current x-count register */
> -	unsigned short dummy8;
> -
> -	unsigned long reserved2;
> -
> -	unsigned short curr_y_count;	/* DMA Current y-count register */
> -	unsigned short dummy9;
> -
> -	unsigned long reserved3;
> -
> +#ifdef __ADSPBF60x__
> +	void *next_desc_ptr;
> +	u32 start_addr;
> +	u32 config;

You drop all the comments; please don't do that.

Also, "cfg" used to be a short, now it gets replaced by "config" which
is a u32.  Is this correct?

> +	void *next_desc_ptr;
> +	u32 start_addr;
> +	u16 __BFP(config);
> +	u32 __pad0;
> +	u16 __BFP(x_count);
> +	s16 __BFP(x_modify);
> +	u16 __BFP(y_count);
> +	s16 __BFP(y_modify);
> +	void *curr_desc_ptr;
> +	u32 curr_addr_ptr;
> +	u16 __BFP(status);
> +	u16 __BFP(peripheral_map);
> +	u16 __BFP(curr_x_count);
> +	u32 __pad1;
> +	u16 __BFP(curr_y_count);
> +	u32 __pad2;

Sorry, but this is unreadable.  NAK.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Software suppliers are trying to make their  software  packages  more
``user-friendly''.  .  .  .  Their best approach, so far, has been to
take all the old brochures, and stamp the words, ``user-friendly'' on
the cover.                                               - Bill Gates