On Sat, Jun 22, 2013 at 10:42 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
+enum { + PL080_LLI_SRC, + PL080_LLI_DST, + PL080_LLI_LLI, + PL080_LLI_CCTL, + + PL080_LLI_WORDS +};
I usually don't like it when enums are not given names, and are implicitly cast to integers. I think it'd be better to just use #define for these so we know what is going on.
@@ -181,7 +177,7 @@ struct pl08x_txd { struct virt_dma_desc vd; struct list_head dsg_list; dma_addr_t llis_bus; - struct pl08x_lli *llis_va; + u32 *llis_va;
It's nice that you use the u32 * here for proper indexing into an array.
-/* Size (bytes) of each LLI buffer allocated for one transfer */ -# define PL08X_LLI_TSFR_SIZE 0x2000 - -/* Maximum times we call dma_pool_alloc on this pool without freeing */ -#define MAX_NUM_TSFR_LLIS (PL08X_LLI_TSFR_SIZE/sizeof(struct pl08x_lli)) +/* + * Number of LLIs in each LLI buffer allocated for one transfer + * (maximum times we call dma_pool_alloc on this pool without freeing) + */ +#define MAX_NUM_TSFR_LLIS 512
And I like this nice side effect that we allocate a number of LLIs rather than a fixed-size buffer.
+static void pl08x_write_lli(struct pl08x_driver_data *pl08x, + struct pl08x_phy_chan *phychan, const u32 *lli, u32 ccfg) +{ + dev_vdbg(&pl08x->adev->dev, + "WRITE channel %d: csrc=0x%08x, cdst=0x%08x, " + "clli=0x%08x, cctl=0x%08x, ccfg=0x%08x\n", + phychan->id, lli[PL080_LLI_SRC], lli[PL080_LLI_DST], + lli[PL080_LLI_LLI], lli[PL080_LLI_CCTL], ccfg); + + writel(lli[PL080_LLI_SRC], phychan->base + PL080_CH_SRC_ADDR); + writel(lli[PL080_LLI_DST], phychan->base + PL080_CH_DST_ADDR); + writel(lli[PL080_LLI_LLI], phychan->base + PL080_CH_LLI); + writel(lli[PL080_LLI_CCTL], phychan->base + PL080_CH_CONTROL); + + writel(ccfg, phychan->reg_config);
Take this opportunity to replace the first four writel() with writel_relaxed(), keep the last one to make sure all hit the hardware. Apart from that this looks nice! Yours, Linus Walleij