From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Mon, 3 Aug 2015 23:23:13 +0200
Subject: [U-Boot] [PATCH v3 1/5] dma: lpc32xx: add DMA driver
In-Reply-To: <1438631269-31670-2-git-send-email-slemieux.tyco@gmail.com>
References: <1438631269-31670-2-git-send-email-slemieux.tyco@gmail.com>
Message-ID: <201508032323.13654.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Monday, August 03, 2015 at 09:47:45 PM, slemieux.tyco at gmail.com wrote:
> From: Sylvain Lemieux <slemieux@tycoint.com>
> 
> Incorporate DMA driver from legacy LPCLinux NXP BSP.
> The files taken from the legacy patch are:
> - lpc32xx DMA driver
> - lpc3250 header file DMA registers definition.
> 
> The legacy driver was updated and clean-up as part of the integration with
> the latest u-boot.
> 
> Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
> ---

[...]

OK, let me nag a bit more ... :-)

> +#include <common.h>
> +
> +
> +/* The "dmac_ll" structure is mapping the DMA channel register,

The multiline comments (in Linux and U-Boot) have this form:

/*
 * foo
 * bar
 */

> + * as per UM10326, "LPC32x0 and LPC32x0/01 User manual" - Rev. 3
> + * tables 84, 85, 86 & 87.
> + *
> + * The DMA client (ex. NAND SLC) can use this macro to write the
> + * next DMA linked list item address. */
> +#define lpc32xx_dmac_next_lli(x) (uint32_t)x
> +
> +/* DMA linked list structure used with a channel's LLI register */
> +struct lpc32xx_dmac_ll {
> +	uint32_t dma_src;
> +	uint32_t dma_dest;
> +	uint32_t next_lli;
> +	uint32_t next_ctrl;

uintNNt should not be used, uNN should be used instead. This is to avoid
conflict with stdint.h , which is userspace-only.

> +};
> +
> +int lpc32xx_dma_get_channel(void);
> +int lpc32xx_dma_start_xfer(int channel, const struct lpc32xx_dmac_ll
> *desc, +			   uint32_t config);
> +int lpc32xx_dma_wait_status(int channel);
> +void lpc32xx_dma_put_channel(int channel);
> +
> +#endif /* _LPC32XX_DMA_H */

[...]

> +int lpc32xx_dma_wait_status(int channel)
> +{
> +	while (((readl(&dma->raw_tc_stat) | readl(dma->raw_err_stat))
> +		& BIT_MASK(channel)) == 0)
> +		;

Looksie, an unbounded loop :-)

> +	if (unlikely(readl(&dma->raw_err_stat) & BIT_MASK(channel))) {
> +		setbits_le32(&dma->int_err_clear, BIT_MASK(channel));
> +		setbits_le32(&dma->raw_err_stat, BIT_MASK(channel));
> +		return -1;
> +	}
> +	setbits_le32(&dma->int_tc_clear, BIT_MASK(channel));
> +	setbits_le32(&dma->raw_tc_stat, BIT_MASK(channel));
> +	return 0;
> +}

[...]

Looks much better than before though :-)