On Tuesday 25 of June 2013 00:23:09 Linus Walleij wrote:
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.
OK. I didn't have any preference for one or the other way and I randomly chose to use enum, but since you seem to have one (and hard to disagree with), I will change this patch to use #define.
@@ -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.
As we're at it, is there any reason to choose this particular number of LLis? I have calculated it from the buffer size defined originally, but I wonder if 512 LLIs for single transfer isn't too much?
+static void pl08x_write_lli(struct pl08x_driver_data *pl08x,
struct pl08x_phy_chan *phychan, const u32 *lli, u32ccfg) +{
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.
OK.
Apart from that this looks nice!
Thanks.
Best regards, Tomasz